From 8b8c875f74bf85ab103456d21a1ddc66a51ac86a Mon Sep 17 00:00:00 2001 From: "bakudies@microsoft.com" Date: Thu, 12 Nov 2026 19:47:37 -0800 Subject: [PATCH 1/2] fix(automations): remove confirmation can open with focus on body text, and Systems and Automations fixture checks fail during re-renders The Automations remove confirmation queued its Cancel focus from Opened, which raced WinUI's own initial-focus step and sometimes left focus on the selectable body text. DialogInitialFocus redirects the first focus entering the dialog to Cancel inside WinUI's focus step, retries after layout while Cancel is not yet realized or focusable (a dialog opened right after another closed), and stops on real user input. #2124's ownership checks ran as one-shot asserts inside polling waits. While Systems and Automations re-render, a just-found element can be torn down before ownership is proven. NativeUiOwnership.Classify now returns Owned, Unproven or Foreign: polls keep polling on Unproven and still fail immediately on Foreign. IsOwnedDescendant remains Owned-only. Systems captures re-find their controls inside the wait. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Automations/AutomationsRoot.cs | 17 +- .../Helpers/DialogInitialFocus.cs | 108 ++++++++++ .../AutomationsUiTests.cs | 15 +- .../DialogInitialFocusTests.cs | 197 ++++++++++++++++++ .../GatewayFixtureSystemsUiTests.cs | 80 ++++--- .../NativeUiOwnership.cs | 126 ++++++++--- .../NativeUiOwnershipTests.cs | 114 ++++++++++ .../OwnedWindowCapture.cs | 12 +- 8 files changed, 598 insertions(+), 71 deletions(-) create mode 100644 src/OpenClaw.Tray.WinUI/Helpers/DialogInitialFocus.cs create mode 100644 tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs diff --git a/src/OpenClaw.Tray.WinUI/Automations/AutomationsRoot.cs b/src/OpenClaw.Tray.WinUI/Automations/AutomationsRoot.cs index afb60df43c..f9aa2ae40b 100644 --- a/src/OpenClaw.Tray.WinUI/Automations/AutomationsRoot.cs +++ b/src/OpenClaw.Tray.WinUI/Automations/AutomationsRoot.cs @@ -13,6 +13,7 @@ using Microsoft.UI.Xaml; using Microsoft.UI.Xaml.Controls; using OpenClaw.Shared.Cron; +using OpenClawTray.Helpers; using OpenClawTray.Presentation; using static Microsoft.UI.Reactor.Factories; using static OpenClawTray.Automations.AutomationsVisuals; @@ -314,11 +315,12 @@ void RegisterRow(FrameworkElement row, string id) IsOpen = true, CloseButtonText = t["Cancel"], DefaultButton = ContentDialogButton.None, - OnOpened = () => { if (confirmDialogRef.Current is { } native) FocusConfirmationCancel(native); }, OnClosed = result => { confirmDialogRef.Current = null; pending.Result.TrySetResult(result == ContentDialogResult.Primary); setConfirm(null); }, }).Set(native => { confirmDialogRef.Current = native; + // Cancel must get initial focus deterministically, not race WinUI's own initial focus. + DialogInitialFocus.FocusCloseButton(native); ConfigureDialogWidth(native, dialogWidth); Microsoft.UI.Xaml.Automation.AutomationProperties.SetAutomationId(native, "AutomationsConfirmDialog"); Microsoft.UI.Xaml.Automation.AutomationProperties.SetName(native, pending.Title); @@ -384,19 +386,6 @@ private static Element RenderConfirmation(ConfirmRequest request, double width) .Padding(12).CornerRadius(6).Background(Theme.SubtleFill)) .MaxWidth(420); - private static void FocusConfirmationCancel(ContentDialog sender) => - sender.DispatcherQueue.TryEnqueue(Microsoft.UI.Dispatching.DispatcherQueuePriority.Low, - () => FindCloseButton(sender, sender.CloseButtonText)?.Focus(FocusState.Programmatic)); - - private static Button? FindCloseButton(DependencyObject parent, string label) - { - if (parent is Button button && (button.Name == "CloseButton" || string.Equals(button.Content as string, label, StringComparison.Ordinal))) - return button; - for (var i = 0; i < Microsoft.UI.Xaml.Media.VisualTreeHelper.GetChildrenCount(parent); i++) - if (FindCloseButton(Microsoft.UI.Xaml.Media.VisualTreeHelper.GetChild(parent, i), label) is { } close) - return close; - return null; - } private static Element RenderHeader(AutomationsHost host, AutomationsSnapshot state, AutomationsActions actions, AutomationsLook look, PageWidthTier tier) { diff --git a/src/OpenClaw.Tray.WinUI/Helpers/DialogInitialFocus.cs b/src/OpenClaw.Tray.WinUI/Helpers/DialogInitialFocus.cs new file mode 100644 index 0000000000..be72a15d9f --- /dev/null +++ b/src/OpenClaw.Tray.WinUI/Helpers/DialogInitialFocus.cs @@ -0,0 +1,108 @@ +// DialogInitialFocus: makes a ContentDialog's initial keyboard focus land on its Close (Cancel) button. +// +// When a ContentDialog opens, WinUI runs its own initial-focus transaction (with DefaultButton None +// it picks the first focusable element, for example selectable body text). A focus call merely +// queued from Opened can run before or after that transaction, so the outcome depends on timing. +// Redirecting the first GettingFocus into the dialog makes Cancel win every time. When a dialog +// opens right after another closes, Opened and even the first focus can arrive before the Cancel +// button is realized or focusable; the fallback then retries after each layout pass until Cancel takes +// focus. The fallback also covers a dialog that opened before this was attached. Once Cancel has focus, +// later focus moves are untouched. +// +// Usage (Reactor ContentDialog element): +// ContentDialog(...) with { DefaultButton = ContentDialogButton.None, CloseButtonText = "Cancel" } +// .Set(native => DialogInitialFocus.FocusCloseButton(native)); +using Microsoft.UI.Dispatching; +using Microsoft.UI.Xaml; +using Microsoft.UI.Xaml.Controls; +using Microsoft.UI.Xaml.Input; +using Microsoft.UI.Xaml.Media; + +namespace OpenClawTray.Helpers; + +internal static class DialogInitialFocus +{ + private const string CloseButtonPart = "CloseButton"; + + private static readonly DependencyProperty PendingProperty = DependencyProperty.RegisterAttached( + "Pending", typeof(object), typeof(DialogInitialFocus), new PropertyMetadata(null)); + + /// + /// Attaches once per native dialog and gives its first focus to Cancel. Callers mount a fresh keyed + /// dialog per confirmation (as AutomationsRoot does), so one open per native dialog is covered. + /// + public static void FocusCloseButton(ContentDialog dialog) + { + if (dialog.GetValue(PendingProperty) is not null) + return; + dialog.SetValue(PendingProperty, true); + dialog.GettingFocus += RedirectInitialFocus; + dialog.Opened += FocusOnOpened; + // Real user input ends the initial-focus duty, so a late fallback never takes focus back after + // the user starts interacting. Handled events count too (a TextBox handles its own keys). + dialog.AddHandler(UIElement.KeyDownEvent, new KeyEventHandler(EndOnUserInput), handledEventsToo: true); + dialog.AddHandler(UIElement.PointerPressedEvent, new PointerEventHandler(EndOnUserInput), handledEventsToo: true); + // A side-mounted dialog can open before the modifier that attaches this runs. + if (dialog.IsLoaded) + QueueFocus(dialog); + } + + internal static Button? FindCloseButton(DependencyObject parent) + { + if (parent is Button { Name: CloseButtonPart } button) + return button; + for (var index = 0; index < VisualTreeHelper.GetChildrenCount(parent); index++) + if (FindCloseButton(VisualTreeHelper.GetChild(parent, index)) is { } found) + return found; + return null; + } + + private static bool IsPending(ContentDialog dialog) => dialog.GetValue(PendingProperty) is true; + + private static void EndOnUserInput(object sender, RoutedEventArgs args) => + ((ContentDialog)sender).SetValue(PendingProperty, false); + + private static void RedirectInitialFocus(UIElement sender, GettingFocusEventArgs args) + { + var dialog = (ContentDialog)sender; + // Only the initial transaction (focus entering from outside); never redirect moves within the dialog. + if (!IsPending(dialog) || IsWithin(dialog, args.OldFocusedElement)) + return; + if (FindCloseButton(dialog) is not { } cancel || !args.TrySetNewFocusedElement(cancel)) + QueueFocus(dialog); + else + dialog.SetValue(PendingProperty, false); + } + + private static bool IsWithin(DependencyObject ancestor, DependencyObject? element) + { + for (var current = element; current is not null; current = VisualTreeHelper.GetParent(current)) + if (ReferenceEquals(current, ancestor)) + return true; + return false; + } + + private static void FocusOnOpened(ContentDialog sender, ContentDialogOpenedEventArgs args) => QueueFocus(sender); + + // Runs after WinUI's own initial-focus transaction; a no-op once Cancel has received focus. + private static void QueueFocus(ContentDialog dialog) => + dialog.DispatcherQueue.TryEnqueue(DispatcherQueuePriority.Low, () => FocusWhenRealized(dialog)); + + private static void FocusWhenRealized(ContentDialog dialog) + { + if (!IsPending(dialog) || !dialog.IsLoaded) + return; + if (FindCloseButton(dialog) is { } cancel && cancel.Focus(FocusState.Programmatic)) + { + dialog.SetValue(PendingProperty, false); + return; + } + // Cancel is not realized or not focusable yet: try again after the next layout pass. + void OnLayoutUpdated(object? sender, object args) + { + dialog.LayoutUpdated -= OnLayoutUpdated; + QueueFocus(dialog); + } + dialog.LayoutUpdated += OnLayoutUpdated; + } +} diff --git a/tests/OpenClaw.Tray.UITests/AutomationsUiTests.cs b/tests/OpenClaw.Tray.UITests/AutomationsUiTests.cs index 400b7345eb..4820453eb7 100644 --- a/tests/OpenClaw.Tray.UITests/AutomationsUiTests.cs +++ b/tests/OpenClaw.Tray.UITests/AutomationsUiTests.cs @@ -680,10 +680,12 @@ await WaitUiAsync(run, () => FindById(run, "AutomationsConfirmDialog") is not nu Assert.Equal($"It's removed from Fixture Gateway ({run.Profile.RunId[..8]}) along with its schedule and run history. This can't be undone.", removeText.DocumentRange.GetText(-1).TrimEnd('\r', '\n')); var removeDialog = FindById(run, "AutomationsConfirmDialog")!; - var cancel = removeDialog.FindFirst(TreeScope.Descendants, new AndCondition( - new PropertyCondition(AutomationElement.ControlTypeProperty, ControlType.Button), - new PropertyCondition(AutomationElement.NameProperty, "Cancel"))); - await WaitUiAsync(run, () => cancel.Current.HasKeyboardFocus, "Cancel receives destructive initial focus"); + // Re-acquire Cancel on every poll: a dialog re-render can replace the button's peer. + await WaitUiAsync(run, () => FindById(run, "AutomationsConfirmDialog")?.FindFirst(TreeScope.Descendants, + new AndCondition( + new PropertyCondition(AutomationElement.ControlTypeProperty, ControlType.Button), + new PropertyCondition(AutomationElement.NameProperty, "Cancel"))) is { Current.HasKeyboardFocus: true }, + "Cancel receives destructive initial focus"); Assert.Equal(ControlType.Window, removeDialog.Current.ControlType); Assert.True(((WindowPattern)removeDialog.GetCurrentPattern(WindowPattern.Pattern)).Current.IsModal); await ScanStateAsync("remove confirmation"); @@ -1564,8 +1566,11 @@ private static AutomationElement PageScrollPeer(GatewayFixtureRun run, bool dump var marker = window.FindFirst(TreeScope.Descendants, new PropertyCondition(AutomationElement.AutomationIdProperty, "CronPageMarker")); if (marker is null) return null; - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, marker, run.AppProcessId), + // Polled while Automations re-renders: a marker peer that is detached or replaced proves nothing yet. + var markerEvidence = NativeUiOwnership.Classify(window, marker, run.AppProcessId); + Assert.True(markerEvidence != NativeUiOwnership.Evidence.Foreign, "Automations page marker must belong to the retained native window."); + if (markerEvidence != NativeUiOwnership.Evidence.Owned) return null; var markerIdentity = marker.GetRuntimeId(); bool IsMarker(AutomationElement? element) => element is not null && NativeUiOwnership.IsOwnedDescendant(window, element, run.AppProcessId) diff --git a/tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs b/tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs new file mode 100644 index 0000000000..c3e3832d05 --- /dev/null +++ b/tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs @@ -0,0 +1,197 @@ +// Proves DialogInitialFocus gives a destructive ContentDialog's initial focus to Cancel within WinUI's +// own initial-focus transaction (already true when Opened fires), even when the dialog content holds +// a focusable element, and never steals later focus moves. Like AutomationsRoot, each confirmation +// mounts a fresh keyed dialog and removes it after it closes. +// Run with the DialogInitialFocusTests filter on an isolated Windows desktop. +using Microsoft.UI.Reactor; +using Microsoft.UI.Reactor.Core; +using Microsoft.UI.Reactor.Hosting; +using Microsoft.UI.Xaml; +using Microsoft.UI.Xaml.Controls; +using Microsoft.UI.Xaml.Input; +using OpenClawTray.Helpers; +using static Microsoft.UI.Reactor.Factories; + +namespace OpenClaw.Tray.UITests; + +[Collection(UICollection.Name)] +public sealed class DialogInitialFocusTests(UIThreadFixture ui) +{ + private static readonly TimeSpan DialogTransitionTimeout = TimeSpan.FromSeconds(10); + // Each run reopens this many confirmations right after the previous one closes (the late-template path). + private const int FollowUpConfirmations = 10; + + [Fact] + public async Task DestructiveDialog_CancelOwnsInitialFocusForEachConfirmationWithoutStealingLaterFocus() + { + await ui.ResetContainerAsync(); + ReactorHostControl? host = null; + Microsoft.UI.Xaml.Controls.ContentDialog? native = null; + Element? current = null; + object? focusedAtOpened = null; + TextBox? movedTo = null; + var moveFocusToBodyOnOpened = false; + var opened = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var closed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + // Each focus change routed through the dialog, read synchronously in GettingFocus after the helper's + // handler (so the final target after any redirect), with whether Cancel existed at that moment. + var focusLog = new List<(DependencyObject Element, bool CancelRealized)>(); + var recorded = new HashSet(ReferenceEqualityComparer.Instance); + void RecordFocus(UIElement sender, GettingFocusEventArgs args) + { + if (!args.Cancel && args.NewFocusedElement is { } element) + focusLog.Add((element, DialogInitialFocus.FindCloseButton(sender) is not null)); + } + void AssertCancelKeptFocus(Microsoft.UI.Xaml.Controls.ContentDialog dialog, string key) + { + var cancel = DialogInitialFocus.FindCloseButton(dialog)!; + Assert.Same(cancel, FocusManager.GetFocusedElement(dialog.XamlRoot)); + var inside = focusLog.Where(entry => IsWithin(dialog, entry.Element)).ToArray(); + // Before Cancel exists nothing can redirect to it; once it exists, the first focus must be Cancel. + var firstRealized = inside.FirstOrDefault(entry => entry.CancelRealized).Element; + Assert.True(ReferenceEquals(firstRealized, cancel), + $"{key}: first focus after Cancel was realized went to {firstRealized?.GetType().Name ?? "nothing"}."); + // WinUI can raise GettingFocus again for the same target; only a different element is a move. + var afterCancel = inside.SkipWhile(entry => !ReferenceEquals(entry.Element, cancel)) + .Where(entry => !ReferenceEquals(entry.Element, cancel)).ToArray(); + Assert.True(afterCancel.Length == 0, + $"{key}: focus moved inside the dialog after Cancel: {string.Join(", ", afterCancel.Select(entry => entry.Element.GetType().Name))}"); + } + Element Dialog(string key) => (ContentDialog("", TextBox("selectable body", _ => { }).AutomationName("Body"), "Remove") with + { + IsOpen = true, + CloseButtonText = "Cancel", + DefaultButton = ContentDialogButton.None, + OnClosed = _ => closed.TrySetResult(), + OnOpened = () => + { + // Before any queued dispatcher work: only a redirect inside WinUI's own initial + // focus transaction can have placed focus on Cancel by now. + focusedAtOpened = native?.XamlRoot is { } root ? FocusManager.GetFocusedElement(root) : null; + // A later focus move made before the queued Low-priority fallback can run. + if (moveFocusToBodyOnOpened && native is not null && FindDescendant(native) is { } body + && body.Focus(FocusState.Programmatic)) + movedTo = body; + opened.TrySetResult(); + }, + }).Set(dialog => + { + native = dialog; + DialogInitialFocus.FocusCloseButton(dialog); + // After the helper's handler, so the recorder sees the final target. OnMountAdd would receive + // Reactor's placeholder, not this native dialog. + AttachRecorder(dialog); + }).WithKey(key); + void AttachRecorder(Microsoft.UI.Xaml.Controls.ContentDialog dialog) + { + if (recorded.Add(dialog)) + dialog.GettingFocus += RecordFocus; + } + void Render(Element next) + { + host!.Content = current is null + ? host.Reconciler.Mount(next, static () => { }) + : host.Reconciler.Reconcile(current, next, (UIElement)host.Content!, static () => { }); + current = next; + } + async Task ConfirmAndAssertCancelAsync(string key, bool atOpened) + { + opened = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + closed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + focusedAtOpened = null; + focusLog.Clear(); + await ui.RunOnUIAsync(() => Render(Dialog(key))); + await opened.Task.WaitAsync(DialogTransitionTimeout); + return await ui.RunOnUIAsync(async () => + { + var dialog = native!; + if (atOpened) + { + var openedCancel = DialogInitialFocus.FindCloseButton(dialog); + Assert.NotNull(openedCancel); + Assert.True(ReferenceEquals(openedCancel, focusedAtOpened), $"{key}: Cancel must own focus when Opened fires."); + return dialog; + } + // Right after another dialog closes, Opened can precede the template and the first focus. + // Wait until Cancel holds focus, drain queued focus work, then require that it kept focus + // and that nothing inside the dialog took focus after it. + await TestSupport.WaitForRenderedConditionAsync(() => + DialogInitialFocus.FindCloseButton(dialog) is { } realized && dialog.XamlRoot is { } root && + ReferenceEquals(FocusManager.GetFocusedElement(root), realized), + $"{key}: Cancel receives initial focus in the realized dialog", + () => $"focused={FocusManager.GetFocusedElement(dialog.XamlRoot)?.GetType().Name ?? "none"}"); + await ui.YieldToRenderAsync(); + AssertCancelKeptFocus(dialog, key); + return dialog; + }); + } + async Task CloseAndRemoveAsync() + { + await ui.RunOnUIAsync(() => native!.Hide()); + await closed.Task.WaitAsync(DialogTransitionTimeout); + await ui.RunOnUIAsync(() => Render(Empty())); + } + try + { + await ui.RunOnUIAsync(() => + { + host = new ReactorHostControl { Width = 560, Height = 400 }; + ui.Container.Children.Add(host); + }); + moveFocusToBodyOnOpened = true; + var first = await ConfirmAndAssertCancelAsync("first-confirmation", atOpened: true); + moveFocusToBodyOnOpened = false; + Assert.NotNull(movedTo); + // Drains the Low-priority Opened fallback queued before the move: it must not take focus back. + await ui.YieldToRenderAsync(); + await ui.RunOnUIAsync(() => Assert.Same(movedTo, FocusManager.GetFocusedElement(native!.XamlRoot))); + + var previous = first; + for (var index = 0; index < FollowUpConfirmations; index++) + { + if (index > 0) + await ui.RunOnUIAsync(() => AssertCancelKeptFocus(previous, $"follow-up-{index - 1} before close")); + await CloseAndRemoveAsync(); + var next = await ConfirmAndAssertCancelAsync($"follow-up-{index}", atOpened: false); + Assert.NotSame(previous, next); + previous = next; + } + // The last follow-up stays open through the test's own async steps; Cancel must still hold focus. + await ui.RunOnUIAsync(() => AssertCancelKeptFocus(previous, $"follow-up-{FollowUpConfirmations - 1} before close")); + await CloseAndRemoveAsync(); + } + finally + { + await ui.RunOnUIAsync(() => + { + foreach (var dialog in recorded) + dialog.GettingFocus -= RecordFocus; + if (current is not null && native is { IsLoaded: true }) + native.Hide(); + if (host is null) return; + ui.Container.Children.Remove(host); + host.Content = null; + host.Dispose(); + }); + } + } + + private static bool IsWithin(DependencyObject ancestor, DependencyObject element) + { + for (DependencyObject? current = element; current is not null; current = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetParent(current)) + if (ReferenceEquals(current, ancestor)) + return true; + return false; + } + + private static T? FindDescendant(DependencyObject parent) where T : DependencyObject + { + for (var index = 0; index < Microsoft.UI.Xaml.Media.VisualTreeHelper.GetChildrenCount(parent); index++) + { + var child = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetChild(parent, index); + if (child is T match) return match; + if (FindDescendant(child) is { } nested) return nested; + } + return null; + } +} diff --git a/tests/OpenClaw.Tray.UITests/GatewayFixtureSystemsUiTests.cs b/tests/OpenClaw.Tray.UITests/GatewayFixtureSystemsUiTests.cs index 3763f5032d..ad330c6196 100644 --- a/tests/OpenClaw.Tray.UITests/GatewayFixtureSystemsUiTests.cs +++ b/tests/OpenClaw.Tray.UITests/GatewayFixtureSystemsUiTests.cs @@ -410,11 +410,25 @@ await WaitUiAsync(run, () => { var element = parent.FindFirst(TreeScope.Descendants, new PropertyCondition(AutomationElement.AutomationIdProperty, id)); - if (element is not null) - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, element, run.AppProcessId), - $"Systems observation '{id}' must belong to the retained native window and process."); + if (element is null) + return null; + // Polls run while Systems re-renders (refresh, error and live readings). A peer that is + // detached or replaced is not observable yet: reject the whole observation so a found but + // unproven element can never satisfy an absence check. Foreign evidence still fails. + var evidence = NativeUiOwnership.Classify(window, element, run.AppProcessId); + Assert.True(evidence != NativeUiOwnership.Evidence.Foreign, + $"Systems observation '{id}' must belong to the retained native window and process."); + if (evidence != NativeUiOwnership.Evidence.Owned) + throw new ElementNotAvailableException($"Systems observation '{id}' ownership is not observable yet."); return element; } + bool OwnedNow(AutomationElement element, string what) + { + var evidence = NativeUiOwnership.Classify(window, element, run.AppProcessId); + Assert.True(evidence != NativeUiOwnership.Evidence.Foreign, + $"{what} must belong to the captured native window and process."); + return evidence == NativeUiOwnership.Evidence.Owned; + } AutomationElement? Find(string id) => FindUnder(window, id); var hwnd = new IntPtr(window.Current.NativeWindowHandle); var scale = SystemsWindowDpi(hwnd) / 96d; @@ -433,15 +447,30 @@ await WaitUiAsync(run, () => }, "normal Systems window"); Assert.NotNull(paneToggle); var records = new List(); - async Task Capture(string state, params AutomationElement[] requiredVisible) + // Finders run inside the poll: Systems re-renders live readings, so a control found earlier + // can be replaced before the capture asserts it. + async Task Capture(string state, params Func[] requiredVisible) { var name = $"{theme}-{state}.png"; OwnedWindowCapture.ViewportSnapshot? frame = null; await WaitUiAsync(run, () => - (frame = OwnedWindowCapture.TryReadViewport(window, run.AppProcessId, "SystemsPageScroll")) is not null, - $"owned Systems viewport ready for {state}"); + { + frame = OwnedWindowCapture.TryReadViewport(window, run.AppProcessId, "SystemsPageScroll"); + if (frame is null) + return false; + var required = new List { frame.Element }; + foreach (var find in requiredVisible) + { + // A replacement peer can exist before its first layout; Save asserts settled geometry. + if (find() is not { Current.IsOffscreen: false } element || + element.Current.BoundingRectangle is not { Width: > 0, Height: > 0 }) + return false; + required.Add(element); + } + OwnedWindowCapture.Save(run, window, name, [.. required]); + return true; + }, $"owned Systems viewport and required controls captured for {state}"); Assert.NotNull(frame); - OwnedWindowCapture.Save(run, window, name, [frame.Element, .. requiredVisible]); frame.AssertUnchanged(); var bounds = frame.WindowBounds; var viewport = frame.Bounds; @@ -495,7 +524,7 @@ await WaitUiAsync(run, () => }, $"collapsed {width} DIP Systems content remains below navigation chrome"); Assert.NotNull(pageMarker); - await Capture($"collapsed-{width}-top", pageMarker); + await Capture($"collapsed-{width}-top", () => Find("SystemsPageMarker")); if (width == 1000) { currentScroll.SetScrollPercent(ScrollPattern.NoScroll, 35); @@ -518,10 +547,11 @@ await WaitUiAsync(run, () => previousCollapsedWidth = -1d; return false; } - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, frame.Element, run.AppProcessId), - "Systems scroll viewport must belong to the captured native window and process."); - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, marker, run.AppProcessId), - "Systems page marker must belong to the captured native window and process."); + if (!OwnedNow(frame.Element, "Systems scroll viewport") || !OwnedNow(marker, "Systems page marker")) + { + previousCollapsedWidth = -1d; + return false; + } var viewport = frame.Bounds; var pattern = (ScrollPattern)frame.Element.GetCurrentPattern(ScrollPattern.Pattern); if (viewport.IsEmpty || viewport.Width <= 0 || viewport.Height <= 0 || @@ -568,7 +598,7 @@ await WaitUiAsync(run, () => "expanding navigation removes the Systems scroll-only inset"); Assert.NotNull(currentViewport); Assert.NotNull(pageMarker); - await Capture("expanded-1000-top", pageMarker); + await Capture("expanded-1000-top", () => Find("SystemsPageMarker")); currentViewport.AssertUnchanged(); var chromeWidth = currentViewport.WindowBounds.Width - currentViewport.Bounds.Width; var statsWindowWidth = 1000 * scale + chromeWidth; @@ -711,8 +741,8 @@ await WaitUiAsync(run, () => memory.Current.BoundingRectangle.Width / scale >= 160 && disk.Current.BoundingRectangle.Width / scale >= 160)) return false; - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, gatewayRow, run.AppProcessId)); - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, studioRow, run.AppProcessId)); + if (!OwnedNow(gatewayRow, "Gateway machine row") || !OwnedNow(studioRow, "Studio machine row")) + return false; var gatewayPattern = (ExpandCollapsePattern)gatewayRow.GetCurrentPattern(ExpandCollapsePattern.Pattern); var studioPattern = (ExpandCollapsePattern)studioRow.GetCurrentPattern(ExpandCollapsePattern.Pattern); cpuTile = cpu; @@ -746,7 +776,8 @@ await File.WriteAllTextAsync(Path.Combine(run.ArtifactsDirectory, "tile-visibili Assert.NotNull(cpuTile); Assert.NotNull(memoryTile); Assert.NotNull(diskTile); - await Capture("expanded-1000-host-stat-tiles", cpuTile, memoryTile, diskTile); + await Capture("expanded-1000-host-stat-tiles", + () => Studio("SystemsStat_cpu"), () => Studio("SystemsStat_memory"), () => Studio("SystemsStat_disk")); gatewayDisclosure.Expand(); studioDisclosure.Collapse(); studioDisclosure.Expand(); @@ -768,7 +799,7 @@ await WaitUiAsync(run, () => !gatewayRow.Current.IsOffscreen, "both expanded machine rows occupy the viewport"); Assert.NotNull(selectedName); - await Capture("two-open-rows", selectedName); + await Capture("two-open-rows", () => Find("SystemsSelectedName")); statsPattern.SetScrollPercent(ScrollPattern.NoScroll, 0); AutomationElement? issuesButton = null; await WaitUiAsync(run, () => @@ -813,7 +844,7 @@ await WaitUiAsync(run, () => Assert.NotNull(retryButton); Assert.NotNull(inventoryConnection); Assert.Contains(run.Gateway.Requests, request => request.Method == "environments.list" && request.Outcome == "error:UNAVAILABLE"); - await Capture("inventory-error-recovery", retryButton, inventoryConnection); + await Capture("inventory-error-recovery", () => Find("SystemsRetry"), () => Find("SystemsInventoryConnection")); run.Gateway.SetSystemsInventoryUnavailable(false); Invoke(retryButton); await WaitUiAsync(run, () => Find("SystemsRetry") is null && @@ -847,7 +878,6 @@ await WaitUiAsync(run, () => var inventoryIds = new[] { "SystemsRow_gateway", "SystemsRow_node:fixture-studio", "SystemsRow_node:fixture-macbook", "SystemsRow_node:fixture-pi" }; AutomationElement? disconnectedConnection = null; - AutomationElement[] disconnectedRows = []; await WaitUiAsync(run, () => { if (Find("SystemsDisconnectedConnection") is not { Current.IsOffscreen: false } connection) @@ -864,7 +894,6 @@ await WaitUiAsync(run, () => rows.Add(row); } var gatewayPattern = (ExpandCollapsePattern)rows[0].GetCurrentPattern(ExpandCollapsePattern.Pattern); - disconnectedRows = rows.ToArray(); disconnectedConnection = connection; disconnectedGatewayDisclosure = gatewayPattern; return true; @@ -872,7 +901,8 @@ await WaitUiAsync(run, () => "settled disconnected inventory retains all four collapsed last-known machines"); Assert.NotNull(disconnectedConnection); await Capture("disconnected-last-known", - disconnectedRows.Prepend(disconnectedConnection).ToArray()); + [() => Find("SystemsDisconnectedConnection"), + .. inventoryIds.Select(id => (Func)(() => Find(id)))]); disconnectedGatewayDisclosure.Expand(); AutomationElement? knownGateway = null; OwnedWindowCapture.ViewportSnapshot? knownScrollFrame = null; @@ -880,10 +910,8 @@ await Capture("disconnected-last-known", await WaitUiAsync(run, () => { var row = Find("SystemsRow_gateway"); - if (row is null) + if (row is null || !OwnedNow(row, "Last-known Gateway row")) return false; - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, row, run.AppProcessId), - "Last-known Gateway row must belong to the captured native window and process."); if (((ExpandCollapsePattern)row.GetCurrentPattern(ExpandCollapsePattern.Pattern)) .Current.ExpandCollapseState != ExpandCollapseState.Expanded) return false; @@ -929,7 +957,9 @@ await WaitUiAsync(run, () => Assert.NotNull(knownStatistics); Assert.NotNull(knownSample); Assert.NotNull(knownDisk); - await Capture("disconnected-expanded-resources", knownCapabilities, knownStatistics, knownSample, knownDisk); + await Capture("disconnected-expanded-resources", + () => GatewayContent("SystemsCapabilitiesHeading"), () => GatewayContent("SystemsStatisticsHeading"), + () => GatewayContent("SystemsStatsSampled"), () => GatewayContent("SystemsStat_disk:/data")); await run.WriteReportAsync("passed"); output.WriteLine($"{theme} daily Systems frames: {run.ArtifactsDirectory}"); } diff --git a/tests/OpenClaw.Tray.UITests/NativeUiOwnership.cs b/tests/OpenClaw.Tray.UITests/NativeUiOwnership.cs index 48ab4ebecd..ddbd708cda 100644 --- a/tests/OpenClaw.Tray.UITests/NativeUiOwnership.cs +++ b/tests/OpenClaw.Tray.UITests/NativeUiOwnership.cs @@ -1,5 +1,6 @@ // Verifies a UIA control against an owned native window, including windowless XAML fragments. // Use IsOwnedDescendant(ownedWindow, control, appProcessId) before claiming control ownership. +// Inside a polling wait, use Classify: keep polling on Unproven, fail immediately on Foreign. using System.Runtime.InteropServices; using System.Windows.Automation; @@ -10,37 +11,112 @@ internal static class NativeUiOwnership { private const int MaxAncestorDepth = 64; + /// + /// Owned: the raw ancestry reaches the retained window inside the owning process. + /// Unproven: no contrary evidence, but ownership cannot be shown yet. The peer is torn down or + /// not yet attached (null parent, unavailable element) or reports no process id. + /// Foreign: positive contrary evidence, such as a different process, a foreign native window, an + /// ancestry that reaches another window or the desktop, or an invalid owner window. + /// + internal enum Evidence { Owned, Unproven, Foreign } + /// Rejects foreign processes, other windows, and ancestry without a verified native owner. - internal static bool IsOwnedDescendant(AutomationElement window, AutomationElement element, int processId) + internal static bool IsOwnedDescendant(AutomationElement window, AutomationElement element, int processId) => + Classify(window, element, processId) == Evidence.Owned; + + /// + /// Classifies ownership. Inside a polling wait, treat as "not yet" and + /// as an immediate failure. + /// + internal static Evidence Classify(AutomationElement window, AutomationElement element, int processId) => + Classify(window, element, processId, ReadFacts, TreeWalker.RawViewWalker.GetParent, NativeProcessId); + + /// + /// UIA facts the ancestry walk uses; separated so the decision rules are testable without a desktop. + /// A null marks a partial read: the peer disappeared after the facts before it. + /// + internal readonly record struct NodeFacts(int ProcessId, string? FrameworkId, int NativeWindowHandle, bool IsWindow, int[]? RuntimeId); + + /// + /// The ownership walk over any node source. Positive contrary evidence (Foreign) is checked before + /// missing evidence (Unproven) at every node, so an unknown process id never hides a foreign window. + /// + internal static Evidence Classify(T window, T element, int processId, Func read, + Func parentOf, Func windowProcessId) where T : class { - var owner = window.Current; - if (processId <= 0 || owner.ProcessId != processId || owner.ControlType != ControlType.Window || - owner.NativeWindowHandle == 0 || NativeProcessId(owner.NativeWindowHandle) != (uint)processId) - return false; + try + { + var owner = read(window); + if (processId <= 0 || owner.ProcessId != processId || + owner.NativeWindowHandle != 0 && windowProcessId(owner.NativeWindowHandle) != (uint)processId) + return Evidence.Foreign; + if (owner.RuntimeId is null) return Evidence.Unproven; + if (!owner.IsWindow || owner.NativeWindowHandle == 0) return Evidence.Foreign; + if (owner.RuntimeId.Length == 0) return Evidence.Unproven; + + T? current = element; + var unproven = false; + for (var depth = 0; current is not null && depth < MaxAncestorDepth; depth++) + { + var info = read(current); + if (info.NativeWindowHandle != 0) + { + // Zero means the HWND is already destroyed (a closing popup): missing, not contrary, evidence. + var windowProcess = windowProcessId(info.NativeWindowHandle); + if (windowProcess == 0) unproven = true; + else if (windowProcess != (uint)processId) return Evidence.Foreign; + } + if (info.ProcessId != processId && info.ProcessId != 0) + return Evidence.Foreign; + // XAML's windowless provider can return an explicit zero PID. Other frameworks give no proof, + // but keep walking so contrary evidence higher up still wins. + if (info.RuntimeId is null) return Evidence.Unproven; + if (info.ProcessId == 0 && !string.Equals(info.FrameworkId, "XAML", StringComparison.Ordinal)) + unproven = true; - var ownerRuntimeId = window.GetRuntimeId(); - if (ownerRuntimeId.Length == 0) return false; + if (info.RuntimeId.SequenceEqual(owner.RuntimeId)) + { + // The owner changed identity during the walk: contrary evidence, not missing evidence. + var again = read(window); + if (again.ProcessId != processId || + again.NativeWindowHandle != 0 && again.NativeWindowHandle != owner.NativeWindowHandle) + return Evidence.Foreign; + if (again.RuntimeId is null) return Evidence.Unproven; + if (again.NativeWindowHandle != owner.NativeWindowHandle || + !again.RuntimeId.SequenceEqual(owner.RuntimeId) || + windowProcessId(owner.NativeWindowHandle) != (uint)processId) + return Evidence.Foreign; + return unproven ? Evidence.Unproven : Evidence.Owned; + } - AutomationElement? current = element; - for (var depth = 0; current is not null && depth < MaxAncestorDepth; depth++) + // Another window's ancestry ends at the desktop root, whose process is not the app's. + current = parentOf(current); + } + // Detached (null parent) or unreasonably deep: no proof either way. + return Evidence.Unproven; + } + catch (ElementNotAvailableException) + { + // A peer torn down mid-walk proves nothing about ownership. + return Evidence.Unproven; + } + } + + // Each property is a separate live read. Read ownership facts first and keep them if the peer then + // disappears, so evidence already read is never discarded. + private static NodeFacts ReadFacts(AutomationElement element) + { + var current = element.Current; + var processId = current.ProcessId; + int handle; + try { handle = current.NativeWindowHandle; } + catch (ElementNotAvailableException) { return new(processId, null, 0, false, null); } + try { - var info = current.Current; - // XAML's windowless provider can return an explicit zero PID. It is not ownership evidence. - if (info.ProcessId != processId && - (info.ProcessId != 0 || !string.Equals(info.FrameworkId, "XAML", StringComparison.Ordinal))) - return false; - if (info.NativeWindowHandle != 0 && NativeProcessId(info.NativeWindowHandle) != (uint)processId) - return false; - - if (current.GetRuntimeId().SequenceEqual(ownerRuntimeId)) - return window.Current.NativeWindowHandle == owner.NativeWindowHandle && - window.Current.ProcessId == processId && - window.GetRuntimeId().SequenceEqual(ownerRuntimeId) && - NativeProcessId(owner.NativeWindowHandle) == (uint)processId; - - current = TreeWalker.RawViewWalker.GetParent(current); + return new(processId, current.FrameworkId, handle, + current.ControlType == ControlType.Window, element.GetRuntimeId()); } - return false; + catch (ElementNotAvailableException) { return new(processId, null, handle, false, null); } } private static uint NativeProcessId(int hwnd) diff --git a/tests/OpenClaw.Tray.UITests/NativeUiOwnershipTests.cs b/tests/OpenClaw.Tray.UITests/NativeUiOwnershipTests.cs index 693b82e0bb..1611944f29 100644 --- a/tests/OpenClaw.Tray.UITests/NativeUiOwnershipTests.cs +++ b/tests/OpenClaw.Tray.UITests/NativeUiOwnershipTests.cs @@ -44,6 +44,12 @@ public async Task OwnedXaml_RejectsAnotherWindowAndWrongOrUnverifiableOwners() Assert.False(NativeUiOwnership.IsOwnedDescendant(owner, ownedControl, Environment.ProcessId + 1)); Assert.False(NativeUiOwnership.IsOwnedDescendant(owner, AutomationElement.RootElement, Environment.ProcessId)); Assert.False(NativeUiOwnership.IsOwnedDescendant(ownedControl, ownedControl, Environment.ProcessId)); + // Every rejection above carries positive contrary evidence; polls must fail on these, not wait. + Assert.Equal(NativeUiOwnership.Evidence.Foreign, NativeUiOwnership.Classify(owner, otherControl, Environment.ProcessId)); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, NativeUiOwnership.Classify(other, ownedControl, Environment.ProcessId)); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, NativeUiOwnership.Classify(owner, ownedControl, Environment.ProcessId + 1)); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, NativeUiOwnership.Classify(owner, AutomationElement.RootElement, Environment.ProcessId)); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, NativeUiOwnership.Classify(ownedControl, ownedControl, Environment.ProcessId)); } finally { @@ -205,3 +211,111 @@ private static Window CreateWindow() return window; } } + +/// +/// Deterministic Owned/Unproven/Foreign rules over synthetic ancestry. SystemsDaily failed on CI when a +/// re-rendered peer was unavailable mid-walk; polls must keep observing on Unproven, but any positive +/// evidence of another process or window must stay Foreign so it fails immediately. +/// +public sealed class NativeUiOwnershipClassificationTests +{ + private const int AppPid = 4242; + private const int OtherPid = 777; + private const int OwnerHwnd = 0x100; + private const int OtherAppWindowHwnd = 0x200; + private const int ForeignHwnd = 0x300; + private const int DesktopHwnd = 0x10010; + private const int DestroyedHwnd = 0x400; + + private sealed class Node(int processId, string framework = "XAML", int hwnd = 0, bool isWindow = false, Node? parent = null) + { + private static int nextId = 1; + public int ProcessId = processId; + public int Hwnd = hwnd; + public bool Unavailable; + public bool PartialRead; + public Node? Parent = parent; + public readonly int[] RuntimeId = [42, Interlocked.Increment(ref nextId)]; + public NativeUiOwnership.NodeFacts Facts => Unavailable + ? throw new ElementNotAvailableException() + : PartialRead ? new(ProcessId, null, Hwnd, false, null) + : new(ProcessId, framework, Hwnd, isWindow, RuntimeId); + } + + private static uint WindowProcess(int hwnd) => hwnd switch + { + OwnerHwnd or OtherAppWindowHwnd => AppPid, + ForeignHwnd or DesktopHwnd => OtherPid, + _ => 0, + }; + + private static NativeUiOwnership.Evidence Classify(Node window, Node element) => + NativeUiOwnership.Classify(window, element, AppPid, node => node.Facts, node => node.Parent, WindowProcess); + + private static Node Owner() => new(AppPid, "Win32", OwnerHwnd, isWindow: true); + + [Fact] + public void WindowlessXamlReachingTheOwner_IsOwned() + { + var owner = Owner(); + var control = new Node(0, parent: new Node(AppPid, parent: owner)); + Assert.Equal(NativeUiOwnership.Evidence.Owned, Classify(owner, control)); + } + + [Fact] + public void MissingEvidence_IsUnproven() + { + var owner = Owner(); + var pane = new Node(AppPid, parent: owner); + Assert.Equal(NativeUiOwnership.Evidence.Unproven, Classify(owner, new Node(AppPid))); + Assert.Equal(NativeUiOwnership.Evidence.Unproven, Classify(owner, new Node(0, "Win32", parent: pane))); + Assert.Equal(NativeUiOwnership.Evidence.Unproven, Classify(owner, new Node(AppPid, hwnd: DestroyedHwnd, parent: pane))); + var torn = new Node(AppPid, parent: pane) { Unavailable = true }; + Assert.Equal(NativeUiOwnership.Evidence.Unproven, Classify(owner, new Node(AppPid, parent: torn))); + Assert.Equal(NativeUiOwnership.Evidence.Unproven, Classify(owner, torn)); + // The peer vanished after its process id was read: no contrary evidence, so not proven yet. + Assert.Equal(NativeUiOwnership.Evidence.Unproven, Classify(owner, new Node(AppPid, parent: pane) { PartialRead = true })); + } + + [Fact] + public void PositiveContraryEvidence_IsForeign() + { + var owner = Owner(); + var pane = new Node(AppPid, parent: owner); + var desktop = new Node(OtherPid, "Win32", DesktopHwnd); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(owner, new Node(OtherPid, parent: pane))); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(owner, + new Node(AppPid, parent: new Node(AppPid, "Win32", OtherAppWindowHwnd, isWindow: true, parent: desktop)))); + // Missing evidence never hides contrary evidence, at the same node or higher up. + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(owner, new Node(0, "Win32", ForeignHwnd, parent: pane))); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(owner, new Node(0, "Win32", parent: new Node(OtherPid)))); + // Ownership facts read before the peer vanished are kept, so a foreign process still wins. + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(owner, new Node(OtherPid, parent: pane) { PartialRead = true })); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(owner, new Node(AppPid, hwnd: ForeignHwnd, parent: pane) { PartialRead = true })); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(new Node(AppPid, "Win32", OwnerHwnd), pane)); + Assert.Equal(NativeUiOwnership.Evidence.Foreign, Classify(new Node(OtherPid, "Win32", ForeignHwnd, isWindow: true), pane)); + } + [Fact] + public void OwnerIdentityChangingDuringTheWalk_IsForeign() + { + Assert.Equal(NativeUiOwnership.Evidence.Foreign, ClassifyWithOwnerChangedOnRecheck(partialRecheck: false)); + // A partial recheck still keeps the changed window handle it read before the owner vanished. + Assert.Equal(NativeUiOwnership.Evidence.Foreign, ClassifyWithOwnerChangedOnRecheck(partialRecheck: true)); + } + + private static NativeUiOwnership.Evidence ClassifyWithOwnerChangedOnRecheck(bool partialRecheck) + { + var owner = Owner(); + var control = new Node(AppPid, parent: new Node(AppPid, parent: owner)); + var reads = 0; + return NativeUiOwnership.Classify(owner, control, AppPid, node => + { + if (ReferenceEquals(node, owner) && ++reads == 3) + { + owner.Hwnd = OtherAppWindowHwnd; + owner.PartialRead = partialRecheck; + } + return node.Facts; + }, node => node.Parent, WindowProcess); + } +} \ No newline at end of file diff --git a/tests/OpenClaw.Tray.UITests/OwnedWindowCapture.cs b/tests/OpenClaw.Tray.UITests/OwnedWindowCapture.cs index c897257448..3a29e0a4ea 100644 --- a/tests/OpenClaw.Tray.UITests/OwnedWindowCapture.cs +++ b/tests/OpenClaw.Tray.UITests/OwnedWindowCapture.cs @@ -109,8 +109,11 @@ internal void AssertUnchanged() new PropertyCondition(AutomationElement.AutomationIdProperty, automationId)); if (element is null) return null; - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, element, processId), + var evidence = NativeUiOwnership.Classify(window, element, processId); + Assert.True(evidence != NativeUiOwnership.Evidence.Foreign, "Viewport must belong to the capture window, not another window in the process."); + if (evidence != NativeUiOwnership.Evidence.Owned) + return null; var windowBounds = window.Current.BoundingRectangle; var bounds = element.Current.BoundingRectangle; static bool Usable(global::System.Windows.Rect area) => @@ -163,8 +166,13 @@ private static void SaveCore(int appProcessId, string runId, string artifactsDir qualifiedGeometry is null ? null : ReadCurrentProcessGeometry(handle)); var assertions = assertedElements.Select(element => { - Assert.True(NativeUiOwnership.IsOwnedDescendant(window, element, appProcessId), + var evidence = NativeUiOwnership.Classify(window, element, appProcessId); + Assert.True(evidence != NativeUiOwnership.Evidence.Foreign, "Asserted control must belong to the captured native window and process."); + // A re-rendered peer proves nothing yet. Polling callers observe again; others fail as unavailable. + if (evidence != NativeUiOwnership.Evidence.Owned) + throw new ElementNotAvailableException( + "Asserted control ownership is not observable yet; the control was detached or re-rendered."); Assert.False(element.Current.IsOffscreen); var area = element.Current.BoundingRectangle; Assert.True(area.Width > 0 && area.Height > 0); From 7f120bca98fee00f30118b55b0657212da757e5b Mon Sep 17 00:00:00 2001 From: "bakudies@microsoft.com" Date: Thu, 12 Nov 2026 21:34:55 -0800 Subject: [PATCH 2/2] fix(tests): dialog focus test no longer assumes Cancel exists when Opened fires CI showed ContentDialog.Opened firing before the Cancel template part is realized even on the first open. Every confirmation now uses the timing-independent check: the first focus inside the dialog once Cancel exists is Cancel, Cancel keeps it, and no other element takes focus afterwards. The "later focus is never stolen" proof focuses the body after Cancel holds focus and requires it to stay. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DialogInitialFocusTests.cs | 72 ++++++++----------- 1 file changed, 28 insertions(+), 44 deletions(-) diff --git a/tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs b/tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs index c3e3832d05..12fd8017d2 100644 --- a/tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs +++ b/tests/OpenClaw.Tray.UITests/DialogInitialFocusTests.cs @@ -1,7 +1,8 @@ -// Proves DialogInitialFocus gives a destructive ContentDialog's initial focus to Cancel within WinUI's -// own initial-focus transaction (already true when Opened fires), even when the dialog content holds -// a focusable element, and never steals later focus moves. Like AutomationsRoot, each confirmation -// mounts a fresh keyed dialog and removes it after it closes. +// Proves DialogInitialFocus gives a destructive ContentDialog's initial focus to Cancel even when the +// dialog content holds a focusable element: the first focus inside the dialog after Cancel exists is +// Cancel, and nothing else takes focus after it. Opened can fire before the template is realized, so +// focus is observed through the dialog's own GettingFocus, not sampled at Opened. A later focus move is +// never stolen back. Like AutomationsRoot, each confirmation mounts a fresh keyed dialog. // Run with the DialogInitialFocusTests filter on an isolated Windows desktop. using Microsoft.UI.Reactor; using Microsoft.UI.Reactor.Core; @@ -18,8 +19,8 @@ namespace OpenClaw.Tray.UITests; public sealed class DialogInitialFocusTests(UIThreadFixture ui) { private static readonly TimeSpan DialogTransitionTimeout = TimeSpan.FromSeconds(10); - // Each run reopens this many confirmations right after the previous one closes (the late-template path). - private const int FollowUpConfirmations = 10; + // Each run opens this many confirmations, each right after the previous one closes. + private const int Confirmations = 11; [Fact] public async Task DestructiveDialog_CancelOwnsInitialFocusForEachConfirmationWithoutStealingLaterFocus() @@ -28,9 +29,6 @@ public async Task DestructiveDialog_CancelOwnsInitialFocusForEachConfirmationWit ReactorHostControl? host = null; Microsoft.UI.Xaml.Controls.ContentDialog? native = null; Element? current = null; - object? focusedAtOpened = null; - TextBox? movedTo = null; - var moveFocusToBodyOnOpened = false; var opened = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var closed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); // Each focus change routed through the dialog, read synchronously in GettingFocus after the helper's @@ -63,17 +61,7 @@ Element Dialog(string key) => (ContentDialog("", TextBox("selectable body", _ => CloseButtonText = "Cancel", DefaultButton = ContentDialogButton.None, OnClosed = _ => closed.TrySetResult(), - OnOpened = () => - { - // Before any queued dispatcher work: only a redirect inside WinUI's own initial - // focus transaction can have placed focus on Cancel by now. - focusedAtOpened = native?.XamlRoot is { } root ? FocusManager.GetFocusedElement(root) : null; - // A later focus move made before the queued Low-priority fallback can run. - if (moveFocusToBodyOnOpened && native is not null && FindDescendant(native) is { } body - && body.Focus(FocusState.Programmatic)) - movedTo = body; - opened.TrySetResult(); - }, + OnOpened = () => opened.TrySetResult(), }).Set(dialog => { native = dialog; @@ -94,27 +82,18 @@ void Render(Element next) : host.Reconciler.Reconcile(current, next, (UIElement)host.Content!, static () => { }); current = next; } - async Task ConfirmAndAssertCancelAsync(string key, bool atOpened) + async Task ConfirmAndAssertCancelAsync(string key) { opened = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); closed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - focusedAtOpened = null; focusLog.Clear(); await ui.RunOnUIAsync(() => Render(Dialog(key))); await opened.Task.WaitAsync(DialogTransitionTimeout); return await ui.RunOnUIAsync(async () => { var dialog = native!; - if (atOpened) - { - var openedCancel = DialogInitialFocus.FindCloseButton(dialog); - Assert.NotNull(openedCancel); - Assert.True(ReferenceEquals(openedCancel, focusedAtOpened), $"{key}: Cancel must own focus when Opened fires."); - return dialog; - } - // Right after another dialog closes, Opened can precede the template and the first focus. - // Wait until Cancel holds focus, drain queued focus work, then require that it kept focus - // and that nothing inside the dialog took focus after it. + // Opened can precede the template and the first focus. Wait until Cancel holds focus, drain + // queued focus work, then require that Cancel was the first focus once it existed and kept it. await TestSupport.WaitForRenderedConditionAsync(() => DialogInitialFocus.FindCloseButton(dialog) is { } realized && dialog.XamlRoot is { } root && ReferenceEquals(FocusManager.GetFocusedElement(root), realized), @@ -138,26 +117,31 @@ await ui.RunOnUIAsync(() => host = new ReactorHostControl { Width = 560, Height = 400 }; ui.Container.Children.Add(host); }); - moveFocusToBodyOnOpened = true; - var first = await ConfirmAndAssertCancelAsync("first-confirmation", atOpened: true); - moveFocusToBodyOnOpened = false; - Assert.NotNull(movedTo); - // Drains the Low-priority Opened fallback queued before the move: it must not take focus back. + var first = await ConfirmAndAssertCancelAsync("confirmation-0"); + // Once Cancel has focus the initial-focus duty is over: a later move (here to the body, which + // also runs layout for any pending retry) must not be taken back. + TextBox? body = null; + await ui.RunOnUIAsync(() => + { + body = FindDescendant(first); + Assert.NotNull(body); + Assert.True(body.Focus(FocusState.Programmatic)); + }); await ui.YieldToRenderAsync(); - await ui.RunOnUIAsync(() => Assert.Same(movedTo, FocusManager.GetFocusedElement(native!.XamlRoot))); + await ui.RunOnUIAsync(() => Assert.Same(body, FocusManager.GetFocusedElement(first.XamlRoot))); var previous = first; - for (var index = 0; index < FollowUpConfirmations; index++) + for (var index = 1; index < Confirmations; index++) { - if (index > 0) - await ui.RunOnUIAsync(() => AssertCancelKeptFocus(previous, $"follow-up-{index - 1} before close")); + if (index > 1) + await ui.RunOnUIAsync(() => AssertCancelKeptFocus(previous, $"confirmation-{index - 1} before close")); await CloseAndRemoveAsync(); - var next = await ConfirmAndAssertCancelAsync($"follow-up-{index}", atOpened: false); + var next = await ConfirmAndAssertCancelAsync($"confirmation-{index}"); Assert.NotSame(previous, next); previous = next; } - // The last follow-up stays open through the test's own async steps; Cancel must still hold focus. - await ui.RunOnUIAsync(() => AssertCancelKeptFocus(previous, $"follow-up-{FollowUpConfirmations - 1} before close")); + // The last confirmation stays open through the test's own async steps; Cancel must still hold focus. + await ui.RunOnUIAsync(() => AssertCancelKeptFocus(previous, $"confirmation-{Confirmations - 1} before close")); await CloseAndRemoveAsync(); } finally