diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 096a28f8..2c4b1e46 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -46,6 +46,38 @@ Copy this block when adding a new issue: --- +## #210 — Mouse-up crashes when a UI callback changes pointer capture + +**Status:** IN-PROGRESS — implementation complete 2026-07-13, pending live gate +**Severity:** HIGH +**Component:** retained UI / input lifecycle + +**Description:** Jumping while changing combat mode could terminate the client +with a `NullReferenceException` in `UiRoot.OnMouseUp`. The crash was exposed by +the new jump/combat visibility transitions, but affected any click callback that +hid its captured window or transferred capture. + +**Root cause:** `OnMouseUp` repeatedly dereferenced the mutable global +`Captured` property while dispatching MouseUp, Click, and DoubleClick. Those +callbacks may synchronously clear or replace capture, so one physical release +could change target halfway through its own event transaction. The unconditional +final `ReleaseCapture` could also discard a replacement capture. + +**Resolution:** Mouse-up now snapshots the original mouse-down target and +double-click classification before dispatching callbacks, uses that target for +the complete event transaction, and releases capture only if the original target +still owns it. Regression tests cover hiding the captured window during the +second click and transferring capture to a newly opened element. + +**Files:** `src/AcDream.App/UI/UiRoot.cs`, +`tests/AcDream.App.Tests/UI/UiRootInputTests.cs` + +**Acceptance:** Hold/release Jump while repeatedly toggling combat and peace, +including rapid double-clicks. The client remains running, the combat bar follows +combat mode, and the jump bar still fills and hides normally. + +--- + ## #209 — Retail jump power bar missing **Status:** IN-PROGRESS — implementation complete 2026-07-13, pending live visual gate diff --git a/src/AcDream.App/UI/UiRoot.cs b/src/AcDream.App/UI/UiRoot.cs index 0dc075a3..ea7dc87e 100644 --- a/src/AcDream.App/UI/UiRoot.cs +++ b/src/AcDream.App/UI/UiRoot.cs @@ -528,7 +528,7 @@ public sealed class UiRoot : UiElement return; } - if (Captured is not null) + if (Captured is { } target) { int rawType = btn switch { @@ -538,46 +538,53 @@ public sealed class UiRoot : UiElement _ => UiEventType.MouseUp, }; - var sp = Captured.ScreenPosition; - var raw = new UiEvent(Captured.EventId, Captured, rawType, + // Event callbacks may synchronously hide/remove a window or transfer + // pointer capture. Keep the mouse-down target stable for this complete + // mouse-up transaction instead of rereading the mutable global owner. + var sp = target.ScreenPosition; + var raw = new UiEvent(target.EventId, target, rawType, Data0: (int)flags, Data1: (int)(x - sp.X), Data2: (int)(y - sp.Y)); - BubbleEvent(Captured, in raw); + BubbleEvent(target, in raw); // If left-up over the same element that received the down, emit Click. - if (btn == UiMouseButton.Left && ContainsAbsolute(Captured, x, y)) + if (btn == UiMouseButton.Left && ContainsAbsolute(target, x, y)) { - var click = new UiEvent(Captured.EventId, Captured, UiEventType.Click, - Data0: (int)flags, - Data1: (int)(x - sp.X), Data2: (int)(y - sp.Y)); - BubbleEvent(Captured, in click); - long now = _nowMs != 0 ? _nowMs : Environment.TickCount64; bool isDoubleClick = - ReferenceEquals(Captured, _lastClickTarget) + ReferenceEquals(target, _lastClickTarget) && now - _lastClickMs <= DoubleClickDelayMs && Math.Abs(x - _lastClickX) <= DragDistanceThreshold && Math.Abs(y - _lastClickY) <= DragDistanceThreshold; + + var click = new UiEvent(target.EventId, target, UiEventType.Click, + Data0: (int)flags, + Data1: (int)(x - sp.X), Data2: (int)(y - sp.Y)); + BubbleEvent(target, in click); + if (isDoubleClick) { - var dbl = new UiEvent(Captured.EventId, Captured, UiEventType.DoubleClick, + var dbl = new UiEvent(target.EventId, target, UiEventType.DoubleClick, Data0: (int)flags, Data1: (int)(x - sp.X), Data2: (int)(y - sp.Y)); - BubbleEvent(Captured, in dbl); + BubbleEvent(target, in dbl); } - _lastClickTarget = Captured; + _lastClickTarget = target; _lastClickMs = now; _lastClickX = x; _lastClickY = y; } - else if (btn == UiMouseButton.Right && ContainsAbsolute(Captured, x, y)) + else if (btn == UiMouseButton.Right && ContainsAbsolute(target, x, y)) { - var click = new UiEvent(Captured.EventId, Captured, UiEventType.RightClick, + var click = new UiEvent(target.EventId, target, UiEventType.RightClick, Data0: (int)flags); - BubbleEvent(Captured, in click); + BubbleEvent(target, in click); } - ReleaseCapture(); + // A callback may have moved capture to a newly opened modal/widget. + // Release only the capture that this mouse-up is completing. + if (ReferenceEquals(Captured, target)) + ReleaseCapture(); _dragCandidate = false; return; } diff --git a/tests/AcDream.App.Tests/UI/UiRootInputTests.cs b/tests/AcDream.App.Tests/UI/UiRootInputTests.cs index 31e4d221..e4451ece 100644 --- a/tests/AcDream.App.Tests/UI/UiRootInputTests.cs +++ b/tests/AcDream.App.Tests/UI/UiRootInputTests.cs @@ -20,11 +20,18 @@ public class UiRootInputTests public bool Clicked; public int Clicks; public int DoubleClicks; + public Action? ClickAction; public ClickRecorder(bool handlesClick) => _handlesClick = handlesClick; public override bool HandlesClick => _handlesClick; public override bool OnEvent(in UiEvent e) { - if (e.Type == UiEventType.Click) { Clicked = true; Clicks++; return true; } + if (e.Type == UiEventType.Click) + { + Clicked = true; + Clicks++; + ClickAction?.Invoke(); + return true; + } if (e.Type == UiEventType.DoubleClick) { DoubleClicks++; return true; } return false; } @@ -66,6 +73,47 @@ public class UiRootInputTests Assert.Equal(1, btn.DoubleClicks); } + [Fact] + public void DoubleClick_WhenClickHidesCapturedWindow_KeepsOriginalEventTarget() + { + var root = new UiRoot { Width = 800, Height = 600 }; + var window = new UiPanel { Width = 200, Height = 100 }; + var btn = new ClickRecorder(handlesClick: true) { Width = 120, Height = 20 }; + window.AddChild(btn); + root.AddChild(window); + root.RegisterWindow("test", window); + + root.Tick(0, nowMs: 1_000); + root.OnMouseDown(UiMouseButton.Left, 10, 10); + root.OnMouseUp(UiMouseButton.Left, 10, 10); + + btn.ClickAction = () => root.HideWindow("test"); + root.Tick(0, nowMs: 1_300); + root.OnMouseDown(UiMouseButton.Left, 10, 10); + root.OnMouseUp(UiMouseButton.Left, 10, 10); + + Assert.False(window.Visible); + Assert.Null(root.Captured); + Assert.Equal(2, btn.Clicks); + Assert.Equal(1, btn.DoubleClicks); + } + + [Fact] + public void MouseUp_WhenClickTransfersCapture_PreservesNewCaptureOwner() + { + var root = new UiRoot { Width = 800, Height = 600 }; + var btn = new ClickRecorder(handlesClick: true) { Width = 120, Height = 20 }; + var modal = new UiPanel { Left = 200, Width = 100, Height = 100 }; + root.AddChild(btn); + root.AddChild(modal); + btn.ClickAction = () => root.SetCapture(modal); + + root.OnMouseDown(UiMouseButton.Left, 10, 10); + root.OnMouseUp(UiMouseButton.Left, 10, 10); + + Assert.Same(modal, root.Captured); + } + [Fact] public void ItemDragReleasedOutsideUi_raisesOutsideReleaseEvent() {