From ab84b54dfaea5bd14f035cce206f3e587d05a878 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 17 Aug 2026 04:45:49 +0200 Subject: [PATCH] =?UTF-8?q?fix(ui):=20night-round=20review=20=E2=80=94=20F?= =?UTF-8?q?5/F6=20structural=20single-tooltip=20invariant?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F5: moved the unconditional RemovePopup() call into RetailTooltipPresenter.TryBuildAndMountPopup itself so the single- popup invariant (retail's own single m_pTooltipElement slot) is enforced structurally rather than relying on every caller to have already cleared a stale popup. Closes a real hole: UpdateWorldHoverTooltip's own clear is gated on _worldTooltipShowing (only true when the WORLD path itself mounted the current popup), and its "a UI popup cannot be showing here" comment assumed the host's hover query is null whenever that branch runs — an assumption that breaks the instant a modal opens over a stationary cursor. UiRoot.Modal claims EXCLUSIVE hit-testing, so Pick(MouseX, MouseY) can return null even though a UI-dwell tooltip is still mounted underneath; UpdateWorldHoverTooltip would then mount a second popup on top without ever clearing the first. F6: fixed WorldHover_ThenUiDwellTooltip_ReplacesRatherThanStacks to actually exercise the transition with a follow-up presenter.Tick() (the old test only proved OnTooltipShow's own clear worked, never checked the world-side bookkeeping after). Added UiDwellTooltip_ThenModalStealsHitTesting_WorldHoverReplacesRatherThanStacks for F5's own case, using UiRoot.Modal to reproduce the exclusive-hit- testing hole precisely — empirically verified this new test fails (2 popups instead of 1) with the structural RemovePopup() reverted, confirming it is a real regression test. Co-Authored-By: Claude Fable 5 --- .../UI/Layout/RetailTooltipPresenter.cs | 25 +++++++++ .../UI/Layout/RetailTooltipPresenterTests.cs | 52 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/AcDream.App/UI/Layout/RetailTooltipPresenter.cs b/src/AcDream.App/UI/Layout/RetailTooltipPresenter.cs index e58e6e9a..44f2d482 100644 --- a/src/AcDream.App/UI/Layout/RetailTooltipPresenter.cs +++ b/src/AcDream.App/UI/Layout/RetailTooltipPresenter.cs @@ -186,9 +186,34 @@ public sealed class RetailTooltipPresenter : IDisposable /// family resolves to). Extracted unchanged from the pre-#411-follow-on /// OnTooltipShow body — same F4/F5/F8 fixes, same failure /// handling. + /// + /// + /// Night-round review F5: the single-popup invariant (retail's own + /// single m_pTooltipElement slot) is now enforced HERE, + /// structurally, rather than relying on every caller to have already + /// cleared a stale popup before reaching this method. Both existing + /// callers already clear on their own early-return paths too (a hover + /// change that resolves to no valid tooltip text must still tear down + /// the PREVIOUS popup, which never reaches this method at all), so + /// those calls stay — this is a belt-and-braces guarantee, not a + /// replacement for them. It closes a real hole: 's + /// own clear is gated on _worldTooltipShowing (only true when the + /// WORLD path itself mounted the current popup) and its "a UI popup + /// cannot be showing here" comment assumed 's hover + /// query is null whenever that branch runs — an assumption that does + /// not hold the instant a modal dialog opens over a stationary cursor: + /// the UI dwell popup from stays mounted + /// (_owner/_popupRoot set, _worldTooltipShowing + /// still false) while the world path could independently find an + /// object and call this method, mounting a second popup on top. Now it + /// cannot: this call clears whatever is mounted, UI-owned or + /// world-owned, before either ever gets a chance to layer. + /// /// private bool TryBuildAndMountPopup(uint rootElementId, uint layoutDid, string tooltipText) { + RemovePopup(); + ImportedLayout? layout; try { diff --git a/tests/AcDream.App.Tests/UI/Layout/RetailTooltipPresenterTests.cs b/tests/AcDream.App.Tests/UI/Layout/RetailTooltipPresenterTests.cs index 0410533c..4cdc83a0 100644 --- a/tests/AcDream.App.Tests/UI/Layout/RetailTooltipPresenterTests.cs +++ b/tests/AcDream.App.Tests/UI/Layout/RetailTooltipPresenterTests.cs @@ -757,6 +757,58 @@ public sealed class RetailTooltipPresenterTests // childrenBefore world-target(none) + target(1) + popup(1) == +2 total, // never +3 (world popup replaced, not stacked). Assert.Equal(childrenBefore + 2, root.Children.Count); + + // Night-round review F6: the test previously stopped here, which + // only proved OnTooltipShow's OWN unconditional RemovePopup() + // cleared the world popup — it never actually exercised what + // happens on the NEXT presenter.Tick() (UpdateWorldHoverTooltip + // still thinks a world-hover target exists, since its own + // _worldHoverGuid/_worldTooltipShowing bookkeeping was never + // re-evaluated after the transition). The mouse is now over the UI + // target, so Pick(...) finds it and WorldHoverGuidProvider is + // ignored (found=0u) — this must leave the UI popup exactly as-is, + // no incorrect extra removal or re-mount. + presenter.Tick(); + Assert.Equal(childrenBefore + 2, root.Children.Count); + Assert.Same(popup, root.Children.Single(c => !ReferenceEquals(c, target))); + } + + [Fact] + public void UiDwellTooltip_ThenModalStealsHitTesting_WorldHoverReplacesRatherThanStacks() + { + // Night-round review F5's own reproduction: the UI->world hole. A + // UI element's dwell tooltip is showing; a modal then opens WITHOUT + // the mouse moving (UiRoot.Modal claims EXCLUSIVE hit-testing — + // HitTestTopDown @0x... "Modal gets exclusive hit-test" — so + // Pick(MouseX, MouseY) now returns null even though the tooltip's + // owner widget is still mounted, still visible, and its popup is + // still up). UpdateWorldHoverTooltip's own clear is gated on + // _worldTooltipShowing, which is FALSE here (the currently-mounted + // popup is UI-owned, not world-owned) — pre-fix, this let the world + // path mount a SECOND popup on top without ever clearing the first. + var (root, presenter, _) = CreateHarness(); + var target = AddFullyAuthoredTarget(root); + int childrenBefore = root.Children.Count; + + root.OnMouseMove(110, 110); + root.Tick(0.016, 0); + root.Tick(0.016, root.TooltipDelayMs); + Assert.Equal(childrenBefore + 1, root.Children.Count); // UI tooltip up + + // Modal opens elsewhere on screen, stealing exclusive hit-testing — + // the mouse never moves. + root.Modal = new UiPanel { Left = 0, Top = 0, Width = 10, Height = 10 }; + presenter.WorldHoverGuidProvider = () => WorldFoundGuid; + presenter.WorldHoverNameResolver = _ => "A Drudge"; + presenter.WorldTooltipsEnabled = () => true; + + presenter.Tick(); + + // Exactly one popup (the world one, having replaced the UI one) — + // never two stacked. + Assert.Equal(childrenBefore + 1, root.Children.Count); + UiElement popup = root.Children.Single(c => !ReferenceEquals(c, target)); + Assert.NotNull(popup); } [Fact]