From 355c86a6f6e80423e83a0bdba9084a73186dd09e Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 15:17:17 +0200 Subject: [PATCH] fix #374: open dropdown popups get first claim on pointer routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Campaign OP gate 2 root cause: UiElement.HitTest walks siblings front-to-back by z-order, so an OPEN UiMenu's extended button+popup hit-test union was never consulted when a LATER sibling's rect overlapped the popup area — on the Config tab every dropdown has rows below it, so Resolution-item clicks toggled the Full Screen / VSync rows underneath (the gate session's persisted fullscreen/vsync flips were exactly those stolen clicks). Latent since UiMenu existed; vendor/chat menus only worked by z-order luck. Fix: UiMenu's open/close now registers with UiRoot (SetActivePopup / ClearActivePopup); a registered popup gets FIRST claim on mouse-down, scroll, and hover routing; a press outside a live popup dismisses it and is SWALLOWED (the dismissing click must not act on what sat underneath); hidden/detached owners self-heal the registration on the next pointer event. UiMenu gains the IsOpen seam and a single SetOpen writer. Also in this commit, from the same investigation: - SilkRuntimeDisplayWindowTarget.Apply documents the fullscreen half honestly: IViewProperties.VideoMode is READ-ONLY, so a resolution pick while fullscreen cannot switch the display mode through Silk's abstract API — split out as #376 (native glfwSetWindowMonitor port) rather than half-shipping untested native interop at a gate tail. - Gate script §OP6 step 8 re-scoped: test resolution in WINDOWED mode. Regressed by tests/AcDream.App.Tests/UI/UiMenuPopupRoutingTests.cs — 4 tests driving the real UiRoot input path on a mounted overlapping tree, with an in-test overlap CONTROL click so the popup assertions cannot pass vacuously (the #372 lesson: only mount+drive-input tests catch this class; every fixture-conformance test stayed green through this bug). Full Release suite: 13,081 passed / 4 skipped / 0 failed. Co-Authored-By: Claude Fable 5 --- docs/ISSUES.md | 80 +++++++++ .../2026-08-11-campaign-op-test-script.md | 12 +- .../Settings/RuntimeSettingsTargets.cs | 15 +- src/AcDream.App/UI/UiMenu.cs | 30 +++- src/AcDream.App/UI/UiRoot.cs | 104 +++++++++++- .../UI/UiMenuPopupRoutingTests.cs | 157 ++++++++++++++++++ 6 files changed, 388 insertions(+), 10 deletions(-) create mode 100644 tests/AcDream.App.Tests/UI/UiMenuPopupRoutingTests.cs diff --git a/docs/ISSUES.md b/docs/ISSUES.md index aa026acf..702fe441 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -24,6 +24,86 @@ What does NOT go here: - Every session: scan OPEN issues at start; promote/close anything we touched during the session before ending. - Promoting to a Phase: mark as `DONE (promoted to Phase X)` + commit SHA where the Phase entry landed. +## #376 — Fullscreen resolution picks cannot switch the display mode (Silk API limit; needs native glfwSetWindowMonitor) + +**Status:** OPEN — filed 2026-08-11, split from #374's investigation. +While FULLSCREEN, the visible resolution is the display's video mode, and +Silk's abstract windowing API cannot change it: +`IViewProperties.VideoMode` is read-only, and Silk fullscreen is +desktop-mode borderless. `SilkRuntimeDisplayWindowTarget.Apply` +(`src/AcDream.App/Settings/RuntimeSettingsTargets.cs`) therefore applies a +Config-tab resolution pick to the WINDOWED size only — visible immediately +in windowed mode, and on the next return to windowed when picked while +fullscreen. Retail's own fullscreen switch is a real display-mode change +(`Device::ForceDisplayResolution`, `gmClient::Init @0x004047af`). Fix +shape: a native GLFW port — reach the underlying handle and call +`glfwSetWindowMonitor(window, monitor, 0, 0, width, height, refresh)` +through `Silk.NET.GLFW` when fullscreen, keeping the abstract path for +windowed. Needs a physical gate (mode switches can black-screen on bad +modes; validate against the monitor's mode list first). + +## #375 — Configure Keyboard screen renders as a visual mess at the live mount (missing button/tab captions, buttons outside the window, overlapping text) + +**Status:** OPEN — filed 2026-08-11 at Campaign OP's second connected gate +(user report, verbatim observations): "all buttons lacked descriptive +text", "some buttons were outside of the window", "lacked text in the +tabs", "text next to the buttons was overlapping. Looked like a mess." +The screen OPENED (the `[options] gameplay button 0x10000204 clicked` +log line fired and the user saw the window), so the OP8 mount/wiring is +alive — the defect family is presentation at the LIVE mount. + +**Same false-negative class as #372:** `KeyboardConfigControllerTests` +runs green against the committed `keyboard_config_21000009.json` fixture, +so the structural conformance suite cannot see whatever the live +DAT mount does differently (string resolution, template sizing, +anchor baselines, window extent). Diagnosis must start from live-mount +evidence, not the fixture: extend the `ACDREAM_PROBE_LIVE_MOUNT=1` probe +(`OptionsPanelLiveMountProbeTests` pattern) to mount `0x21000009` against +the real DATs and dump per-element rect + resolved caption, then compare +against the user's four observations. Candidate families (to CONFIRM, not +assume): caption lookups missing their real string table (the exact +`0x2300000D` class from #372's session), row-template text elements +sized/positioned from degenerate baselines, and the screen's authored +extent vs where children actually land. + +**Blocks the OP8 connected gate.** + +## #374 — Config tab: picking a new Resolution does not resize the window (live gate failure) + +**Status:** ROOT-CAUSED + FIXED (this commit) — pending the user's +re-gate. Filed 2026-08-11 at Campaign OP's second connected gate ("I was +not able to change the screen resolution"). + +**ROOT CAUSE — popup hit-test priority, a UiRoot-level routing hole.** +`UiElement.HitTest` walks siblings front-to-back by z-order; an OPEN +`UiMenu` extends its hit area beyond its own rect (the button+popup union +in `UiMenu.OnHitTest`), but any sibling added AFTER the menu whose rect +overlaps the popup area wins the walk before the menu's extended +hit-test is ever consulted. On the Config tab every dropdown has rows +BELOW it — so clicking a Resolution popup item actually clicked the rows +underneath (the session's persisted `fullscreen: true` + `vsync: false` +flips were exactly such stolen clicks toggling the Full Screen / VSync +rows under the open popup). Vendor's and chat's menus only ever worked +because no overlapping sibling sat in front of them — the hole was +latent since UiMenu existed. **Fix:** an open popup registers with +`UiRoot` (`SetActivePopup`) and gets FIRST claim on mouse-down, scroll, +and hover routing; a press outside the popup dismisses it and is +SWALLOWED (the standard dropdown-dismiss gesture — the dismissing click +must not act on whatever sat underneath); hidden/detached owners +self-heal the registration. Regressed by +`tests/AcDream.App.Tests/UI/UiMenuPopupRoutingTests.cs` (4 tests, with +an in-test overlap CONTROL so the assertions cannot pass vacuously). + +The investigation also surfaced the fullscreen half — a resolution pick +while fullscreen cannot switch the display mode through Silk's abstract +API at all — split out as #376. And the same session's log showed 64 +full `settings.json` disk writes from slider drags (one per drag tick); +noted here as a minor perf observation, not yet its own issue. + +**Re-gate (§OP6 step 8): test the Resolution row in WINDOWED mode** — +the pick should now register and resize immediately; fullscreen mode +switching stays #376. + ## #373 — Configure Keyboard: DAT `ActionMap.ConflictingMaps` not consulted — the combat cluster raises false conflict prompts **Status:** OPEN — filed 2026-08-11 at Campaign OP slice OP8's re-review diff --git a/docs/research/2026-08-11-campaign-op-test-script.md b/docs/research/2026-08-11-campaign-op-test-script.md index 776d4bf4..5770890b 100644 --- a/docs/research/2026-08-11-campaign-op-test-script.md +++ b/docs/research/2026-08-11-campaign-op-test-script.md @@ -603,8 +603,16 @@ should show no caption. ### Live rows — display 8. **Open the Resolution menu (Graphics Options, first row) and pick a - different resolution.** The window should resize IMMEDIATELY, live, no - restart needed. + different resolution — IN WINDOWED MODE.** The window should resize + IMMEDIATELY, live, no restart needed. **Gate-2 re-test note (#374):** + the first gate's failure here was a client-wide dropdown routing bug — + clicks on an open popup's items landed on the rows UNDERNEATH it (your + session's Full Screen and VSync flips were those stolen clicks). Fixed; + dropdown items now win while a popup is open, and a click outside an + open popup dismisses it without acting on what's below. While + FULLSCREEN a resolution pick cannot switch the display mode yet + (Silk API limit, #376) — it applies on the next return to windowed; + do not report that as a step-8 failure. 9. **Toggle "Full Screen".** The window should switch between windowed and fullscreen IMMEDIATELY, live. 10. **Toggle "Sync To Refresh" (VSync) and drag the "Field of View" diff --git a/src/AcDream.App/Settings/RuntimeSettingsTargets.cs b/src/AcDream.App/Settings/RuntimeSettingsTargets.cs index a81f8392..91fc945b 100644 --- a/src/AcDream.App/Settings/RuntimeSettingsTargets.cs +++ b/src/AcDream.App/Settings/RuntimeSettingsTargets.cs @@ -56,8 +56,9 @@ internal sealed class SilkRuntimeDisplayWindowTarget : IRuntimeDisplayWindowTarg public void Apply(DisplaySettings display) { ArgumentNullException.ThrowIfNull(display); - if (TryParseResolution(display.Resolution, out int width, out int height) - && (_window.Size.X != width || _window.Size.Y != height)) + bool haveResolution = + TryParseResolution(display.Resolution, out int width, out int height); + if (haveResolution && (_window.Size.X != width || _window.Size.Y != height)) { _window.Size = new Vector2D(width, height); } @@ -65,6 +66,16 @@ internal sealed class SilkRuntimeDisplayWindowTarget : IRuntimeDisplayWindowTarg WindowState desired = display.Fullscreen ? WindowState.Fullscreen : WindowState.Normal; + + // #374 investigation note: while FULLSCREEN, the visible resolution + // is the display's video mode, and Silk's abstract windowing API + // cannot change it (IViewProperties.VideoMode is read-only; Silk + // fullscreen is desktop-mode borderless). The Size write above is + // therefore only visible in windowed mode — a resolution pick while + // fullscreen changes what a later return to windowed restores, not + // the fullscreen mode itself. Retail's own fullscreen resolution + // switch (Device::ForceDisplayResolution) needs a native + // glfwSetWindowMonitor port — issue #376. if (_window.WindowState != desired) _window.WindowState = desired; } diff --git a/src/AcDream.App/UI/UiMenu.cs b/src/AcDream.App/UI/UiMenu.cs index 91afd2a6..29c62226 100644 --- a/src/AcDream.App/UI/UiMenu.cs +++ b/src/AcDream.App/UI/UiMenu.cs @@ -201,6 +201,28 @@ public sealed class UiMenu : UiElement public Vector4 TextColorGhosted { get; set; } = new(0.5f, 0.5f, 0.5f, 1f); private bool _open; + + /// Whether the popup is currently open (test/inspection seam, + /// same rationale as /). + public bool IsOpen => _open; + + /// The ONLY writer of : keeps the root's + /// transient-popup registration (#374 — an open popup gets first claim + /// on pointer routing, because the sibling z-order walk would otherwise + /// hand popup-area clicks to whatever front sibling overlaps it) in + /// lockstep with the widget's own state. A detached menu (no root yet) + /// still toggles locally — registration happens against the root that + /// dispatches the events, which by construction exists whenever a real + /// pointer event reaches this widget. + private void SetOpen(bool value) + { + if (_open == value) return; + _open = value; + if (FindRoot() is not { } root) return; + if (value) root.SetActivePopup(this, () => SetOpen(false)); + else root.ClearActivePopup(this); + } + // Interior = the row content; Outer = interior + the 8-piece bevel ring. // Scrollable: always exactly one column (RowsPerColumn is the VISIBLE window, // not a wrap threshold), widened by the docked scrollbar's own authored width. @@ -555,11 +577,11 @@ public sealed class UiMenu : UiElement OnSelect?.Invoke(Items[idx].Payload); } } - _open = false; + SetOpen(false); return true; } - _open = !_open; // toggle on button click + SetOpen(!_open); // toggle on button click return true; } @@ -581,7 +603,7 @@ public sealed class UiMenu : UiElement { OnSelect?.Invoke(Items[idx].Payload); } - _open = false; + SetOpen(false); return true; } @@ -611,7 +633,7 @@ public sealed class UiMenu : UiElement } // Clicked the bevel ring — close, matching the grid path. - _open = false; + SetOpen(false); return true; } diff --git a/src/AcDream.App/UI/UiRoot.cs b/src/AcDream.App/UI/UiRoot.cs index b948dd32..61dac04e 100644 --- a/src/AcDream.App/UI/UiRoot.cs +++ b/src/AcDream.App/UI/UiRoot.cs @@ -463,6 +463,67 @@ public sealed class UiRoot : UiElement WorldMouseMoveFallThrough?.Invoke(x, y); } + // ── Popup routing (#374) ──────────────────────────────────────────── + // + // An OPEN transient popup (a UiMenu dropdown) extends its owner's + // hit-test area beyond the owner's own rect (UiMenu.OnHitTest's + // button+popup union). But HitTestTopDown walks SIBLINGS front-to-back + // by z-order, and any sibling added after the owner whose rect overlaps + // the popup area wins the walk before the owner's extended OnHitTest is + // ever consulted — on the Options panel's Config tab every dropdown has + // rows BELOW it, so item clicks landed on those rows instead (toggling + // Full Screen / VSync underneath the open Resolution popup). Vendor's + // and chat's menus only ever worked because no overlapping sibling sat + // in front of them. While a popup is registered it gets FIRST claim on + // pointer events; a press outside it dismisses it and is swallowed (the + // standard dropdown-dismiss gesture — the dismissing click must not + // fall through and act on whatever sat under the popup). + private UiElement? _activePopup; + private Action? _activePopupDismiss; + + /// Registers as the transient popup + /// with first claim on pointer routing. Replaces any prior registration + /// (its owner keeps its own open state; the previous dismiss is invoked + /// so owner state cannot go stale). + internal void SetActivePopup(UiElement popup, Action dismiss) + { + if (!ReferenceEquals(_activePopup, popup)) + _activePopupDismiss?.Invoke(); + _activePopup = popup; + _activePopupDismiss = dismiss; + } + + /// Clears the registration if holds it + /// (the owner closed itself — item picked, bevel click, forced close). + internal void ClearActivePopup(UiElement popup) + { + if (!ReferenceEquals(_activePopup, popup)) return; + _activePopup = null; + _activePopupDismiss = null; + } + + /// The registered popup's hit-test claim on (x,y), with stale + /// registrations (owner hidden/detached, e.g. its window closed while + /// open) self-healing to a dismissed, unregistered state. + private UiElement? PopupHit(int x, int y) + { + if (_activePopup is not { } popup) return null; + for (UiElement? e = popup; e is not null; e = e.Parent) + { + if (ReferenceEquals(e, this)) break; + if (!e.Visible || !e.Enabled || e.Parent is null) + { + var stale = _activePopupDismiss; + _activePopup = null; + _activePopupDismiss = null; + stale?.Invoke(); + return null; + } + } + var pp = popup.ScreenPosition; + return popup.HitTest(x - pp.X, y - pp.Y); + } + public void OnMouseDown(UiMouseButton btn, int x, int y, uint flags = 0) { MouseX = x; MouseY = y; @@ -473,7 +534,30 @@ public sealed class UiRoot : UiElement if (Modal is not null && !ContainsAbsolute(Modal, x, y)) return; - var (target, _, _) = HitTestTopDown(x, y); + UiElement? target; + if (_activePopup is not null) + { + target = PopupHit(x, y); + if (target is null) + { + if (_activePopup is not null) + { + // Press outside a live popup: dismiss it, swallow the press. + var dismiss = _activePopupDismiss; + _activePopup = null; + _activePopupDismiss = null; + dismiss?.Invoke(); + return; + } + // Stale registration self-healed inside PopupHit — fall + // through to the ordinary walk for this press. + (target, _, _) = HitTestTopDown(x, y); + } + } + else + { + (target, _, _) = HitTestTopDown(x, y); + } if (target is null) { // Clicking the 3D world exits write mode (no submit) and returns control to @@ -696,6 +780,18 @@ public sealed class UiRoot : UiElement public void OnScroll(int dy) { + // An open popup (dropdown) claims the wheel first — its scrollable + // list must scroll even where a front sibling overlaps it (#374). + if (PopupHit(MouseX, MouseY) is { } popupTarget) + { + var pp = popupTarget.ScreenPosition; + var pe = new UiEvent(popupTarget.EventId, popupTarget, UiEventType.Scroll, + Data0: dy, + Data1: (int)(MouseX - pp.X), Data2: (int)(MouseY - pp.Y)); + BubbleEvent(popupTarget, in pe); + return; + } + // Scroll goes to the widget under the cursor (not the focused one). var (target, lx, ly) = HitTestTopDown(MouseX, MouseY); if (target is null) @@ -956,7 +1052,11 @@ public sealed class UiRoot : UiElement private void UpdateHover(int x, int y) { - var (w, _, _) = HitTestTopDown(x, y); + // An open popup claims hover first (#374) — its item highlight must + // track the cursor even where a front sibling overlaps the popup. + UiElement? w = PopupHit(x, y); + if (w is null) + (w, _, _) = HitTestTopDown(x, y); if (ReferenceEquals(w, _hoverWidget)) { if (w?.ReceivesHoverMouseMove == true) diff --git a/tests/AcDream.App.Tests/UI/UiMenuPopupRoutingTests.cs b/tests/AcDream.App.Tests/UI/UiMenuPopupRoutingTests.cs new file mode 100644 index 00000000..82bdcd52 --- /dev/null +++ b/tests/AcDream.App.Tests/UI/UiMenuPopupRoutingTests.cs @@ -0,0 +1,157 @@ +using AcDream.App.UI; + +namespace AcDream.App.Tests.UI; + +/// +/// #374 (Campaign OP gate 2): an OPEN UiMenu dropdown must get first claim on +/// pointer events. UiElement.HitTest walks siblings front-to-back by z-order, +/// so any sibling added AFTER the menu whose rect overlaps the popup area used +/// to win the walk before the menu's extended OnHitTest union was consulted — +/// on the Options panel's Config tab every dropdown has rows below it, so +/// picking a Resolution item actually toggled the Full Screen / VSync rows +/// underneath the popup. These tests drive the REAL UiRoot input path +/// (synthetic OnMouseDown/OnMouseUp on a mounted tree), the exact class of +/// coverage the fixture-conformance suites cannot provide (#372's lesson). +/// +public sealed class UiMenuPopupRoutingTests +{ + private sealed class ClickRecorder : UiElement + { + public int MouseDowns; + public int Clicks; + public override bool HandlesClick => true; + public override bool OnEvent(in UiEvent e) + { + if (e.Type == UiEventType.MouseDown) { MouseDowns++; return true; } + if (e.Type == UiEventType.Click) { Clicks++; return true; } + return false; + } + } + + private static (UiRoot root, UiMenu menu, ClickRecorder stealer, Func selected) + BuildOverlappingTree() + { + var root = new UiRoot { Width = 800, Height = 600 }; + var panel = new UiPanel { Left = 0, Top = 0, Width = 300, Height = 300 }; + + string? picked = null; + var menu = new UiMenu + { + Left = 10, Top = 20, Width = 46, Height = 20, + OpenUpward = false, + Items = new[] + { + new UiMenu.MenuItem("A", "a"), + new UiMenu.MenuItem("B", "b"), + }, + }; + menu.OnSelect = p => picked = p as string; + + // The sibling "row below" — added AFTER the menu, so the front-to-back + // sibling walk consults it FIRST. Its rect (screen y 44..74) overlaps + // the open popup's first item row (screen y 45..62). + var stealer = new ClickRecorder { Left = 0, Top = 44, Width = 250, Height = 30 }; + + panel.AddChild(menu); + panel.AddChild(stealer); + root.AddChild(panel); + return (root, menu, stealer, () => picked); + } + + private static void Click(UiRoot root, int x, int y) + { + root.OnMouseDown(UiMouseButton.Left, x, y); + root.OnMouseUp(UiMouseButton.Left, x, y); + } + + [Fact] + public void OpenPopup_ItemClick_ReachesTheMenu_NotTheOverlappingFrontSibling() + { + var (root, menu, stealer, selected) = BuildOverlappingTree(); + + // CONTROL: with the menu closed, this exact point belongs to the + // stealer — proving the overlap is real, so the popup assertion below + // cannot pass vacuously on wrong geometry. + Click(root, 30, 50); + Assert.Equal(1, stealer.MouseDowns); + + Click(root, 30, 30); // the menu button (screen 10..56 x 20..40) + Assert.True(menu.IsOpen); + + // First popup item row: menu-local (20, 30) → interior (15, 5) → row 0. + // The same screen point the control click just proved the stealer owns. + Click(root, 30, 50); + + Assert.Equal("a", selected()); + Assert.False(menu.IsOpen); + Assert.Equal(1, stealer.MouseDowns); // unchanged — the popup won + Assert.Equal(1, stealer.Clicks); + } + + [Fact] + public void OpenPopup_OutsidePress_DismissesAndSwallows_ThenNormalRoutingResumes() + { + var (root, menu, stealer, selected) = BuildOverlappingTree(); + + Click(root, 30, 30); + Assert.True(menu.IsOpen); + + // (230, 60): inside the stealer's rect (x < 250, y in 44..74) but past + // the popup's right edge (screen x 10 + outer width 201 = 211). The + // dismissing press must close the popup WITHOUT acting on the widget + // underneath — the standard dropdown-dismiss gesture, and exactly the + // accidental-toggle hazard #374's gate session demonstrated. + Click(root, 230, 60); + + Assert.False(menu.IsOpen); + Assert.Null(selected()); + Assert.Equal(0, stealer.MouseDowns); + Assert.Equal(0, stealer.Clicks); + + // With the popup gone, the same spot routes normally again. + Click(root, 230, 60); + Assert.Equal(1, stealer.MouseDowns); + Assert.Equal(1, stealer.Clicks); + } + + [Fact] + public void ReopeningAfterDismiss_StillRoutesItemClicks() + { + var (root, menu, _, selected) = BuildOverlappingTree(); + + Click(root, 30, 30); // open + Click(root, 230, 60); // dismiss (swallowed) + Assert.False(menu.IsOpen); + + Click(root, 30, 30); // reopen + Assert.True(menu.IsOpen); + Click(root, 30, 50); // pick item A again + Assert.Equal("a", selected()); + } + + [Fact] + public void HidingThePopupsWindowWhileOpen_SelfHeals_NoStalePopupRouting() + { + var (root, menu, stealer, selected) = BuildOverlappingTree(); + + Click(root, 30, 30); + Assert.True(menu.IsOpen); + + // The panel hosting the menu closes while the popup is open (e.g. the + // Options panel is toggled shut). The stale registration must not keep + // swallowing clicks for an invisible popup. + menu.Parent!.Visible = false; + Click(root, 230, 60); // self-heal press (dismissed, swallowed) + Assert.False(menu.IsOpen); + + Click(root, 230, 60); + // The stealer is inside the hidden panel too, so the press falls + // through to nothing — the point is that the popup no longer claims it. + Assert.Equal(0, stealer.MouseDowns); + + menu.Parent!.Visible = true; + Click(root, 230, 60); + Assert.Equal(1, stealer.MouseDowns); + Assert.Null(selected()); + } +}