fix #210: stabilize mouse-up capture ownership
Pin each mouse-up to its original captured target before callbacks can hide a window or transfer capture. Classify double-clicks before dispatch, preserve replacement capture owners, and cover both re-entrant paths so jump/combat visibility changes cannot crash the UI root. Co-Authored-By: Codex <codex@openai.com>
This commit is contained in:
parent
db03b4bda8
commit
e74efc5c06
3 changed files with 106 additions and 19 deletions
|
|
@ -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
|
## #209 — Retail jump power bar missing
|
||||||
|
|
||||||
**Status:** IN-PROGRESS — implementation complete 2026-07-13, pending live visual gate
|
**Status:** IN-PROGRESS — implementation complete 2026-07-13, pending live visual gate
|
||||||
|
|
|
||||||
|
|
@ -528,7 +528,7 @@ public sealed class UiRoot : UiElement
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (Captured is not null)
|
if (Captured is { } target)
|
||||||
{
|
{
|
||||||
int rawType = btn switch
|
int rawType = btn switch
|
||||||
{
|
{
|
||||||
|
|
@ -538,46 +538,53 @@ public sealed class UiRoot : UiElement
|
||||||
_ => UiEventType.MouseUp,
|
_ => UiEventType.MouseUp,
|
||||||
};
|
};
|
||||||
|
|
||||||
var sp = Captured.ScreenPosition;
|
// Event callbacks may synchronously hide/remove a window or transfer
|
||||||
var raw = new UiEvent(Captured.EventId, Captured, rawType,
|
// 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,
|
Data0: (int)flags,
|
||||||
Data1: (int)(x - sp.X), Data2: (int)(y - sp.Y));
|
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 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;
|
long now = _nowMs != 0 ? _nowMs : Environment.TickCount64;
|
||||||
bool isDoubleClick =
|
bool isDoubleClick =
|
||||||
ReferenceEquals(Captured, _lastClickTarget)
|
ReferenceEquals(target, _lastClickTarget)
|
||||||
&& now - _lastClickMs <= DoubleClickDelayMs
|
&& now - _lastClickMs <= DoubleClickDelayMs
|
||||||
&& Math.Abs(x - _lastClickX) <= DragDistanceThreshold
|
&& Math.Abs(x - _lastClickX) <= DragDistanceThreshold
|
||||||
&& Math.Abs(y - _lastClickY) <= 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)
|
if (isDoubleClick)
|
||||||
{
|
{
|
||||||
var dbl = new UiEvent(Captured.EventId, Captured, UiEventType.DoubleClick,
|
var dbl = new UiEvent(target.EventId, target, UiEventType.DoubleClick,
|
||||||
Data0: (int)flags,
|
Data0: (int)flags,
|
||||||
Data1: (int)(x - sp.X), Data2: (int)(y - sp.Y));
|
Data1: (int)(x - sp.X), Data2: (int)(y - sp.Y));
|
||||||
BubbleEvent(Captured, in dbl);
|
BubbleEvent(target, in dbl);
|
||||||
}
|
}
|
||||||
_lastClickTarget = Captured;
|
_lastClickTarget = target;
|
||||||
_lastClickMs = now;
|
_lastClickMs = now;
|
||||||
_lastClickX = x;
|
_lastClickX = x;
|
||||||
_lastClickY = y;
|
_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);
|
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;
|
_dragCandidate = false;
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -20,11 +20,18 @@ public class UiRootInputTests
|
||||||
public bool Clicked;
|
public bool Clicked;
|
||||||
public int Clicks;
|
public int Clicks;
|
||||||
public int DoubleClicks;
|
public int DoubleClicks;
|
||||||
|
public Action? ClickAction;
|
||||||
public ClickRecorder(bool handlesClick) => _handlesClick = handlesClick;
|
public ClickRecorder(bool handlesClick) => _handlesClick = handlesClick;
|
||||||
public override bool HandlesClick => _handlesClick;
|
public override bool HandlesClick => _handlesClick;
|
||||||
public override bool OnEvent(in UiEvent e)
|
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; }
|
if (e.Type == UiEventType.DoubleClick) { DoubleClicks++; return true; }
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
@ -66,6 +73,47 @@ public class UiRootInputTests
|
||||||
Assert.Equal(1, btn.DoubleClicks);
|
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]
|
[Fact]
|
||||||
public void ItemDragReleasedOutsideUi_raisesOutsideReleaseEvent()
|
public void ItemDragReleasedOutsideUi_raisesOutsideReleaseEvent()
|
||||||
{
|
{
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue