From e3c86aa697d441b447ee394e20cd9c25ad23eb3b Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Mon, 28 Sep 2026 14:46:17 -0700 Subject: [PATCH] fix(datagrid): keep keyboard focus inside the grid while an edit transition removes the focused element (#1288) Opening a DataGrid editor from inside the grid could commit it the moment it opened. Starting a row edit from the row's Edit button, or a commit-then-begin cell tap, removed the element holding keyboard focus (the Edit button, or the previous cell's editor) in the re-render that opened the new editor. XAML then moved focus to the next tab stop, which lies outside the grid because the grid is a single tab stop, and the grid's LostFocus blur-commit net raced the new editor's deferred focus request. Whenever its deferred check ran first, it found focus outside the grid and committed the edit just opened: the editor closed as it appeared, and onRowChanged fired with unchanged values. Two changes keep focus inside the grid: - DataGridState.BeforeEditTransition, wired to DataGridComponent.ParkFocusOnGridRoot, moves focus onto the grid root, a tab stop that survives renders, when focus is on one of the grid's descendants. BeginEdit and BeginRowEdit call it before they change state. The cell tap and the cross-row pointer press call it right after a commit that actually ended the in-flight edit. The re-render that removes the element is a later dispatcher tick, so the park lands first. A commit that validation refuses parks nothing, because that edit stays open and its editor keeps focus. Neither do a refused begin, row-mode Tab, or focus outside the grid. - While keyboard focus is on the root itself, the root claims an unhandled PointerReleased that can complete a primary-action press: a left mouse button, a touch contact, or a pen tip. WinUI's ScrollViewer takes focus on an unhandled release of a press whose IsLeftButtonPressed was true, unless it is not a tab stop and focus is already inside it (ScrollViewer::OnPointerReleased in microsoft-ui-xaml). The grid's data-area scroller cannot hold focus that is on the root, its ancestor, so the release reached the window's root ScrollViewer, a tab stop, which took focus out of the grid. A cross-row press parks from a deferred callback, so a release that arrived after that callback reopened the race. The PointerUpdateKind a touch or pen release reports is not documented, so only explicit right, middle, and X-button releases are left unclaimed. Measured with ci-stress (E2E, retries off) and a temporary probe that recorded every focus move: stock vs fixed, 60 iterations each: focus left the grid during the row Edit click 141/141 attempts vs 0/120 DoesNotSuppressNextCommit, first attempt failed 8/60 vs 0/60 WrapsToFirstEditorWithoutCommitting, first attempt 3/60 vs 0/60 ClickEditTabCommit failed 2/60 vs 1/60, the release path park only vs park + release handler, 30 iterations each: PressHeldOnAnotherRow_KeepsKeyboardFocusInGrid failed 30/30 vs passed 30/30 rest of the E2E suite 1 unrelated DragDrop flake vs green Tests: - Reactor.Tests: 13 unit tests pin when the hook runs, including commits that validation refuses in both edit modes, and a theory pins which release kinds the root claims. - Selftests: DataGrid_EditorFocusParkedFromEditButton and DataGrid_EditorFocusParkedOnCommitThenBegin read focus right after the transition, record every focus move until the new editor holds focus, check that a refused commit keeps focus in its editor, and pin IsFocusOnRoot. With the park no-oped, the focus-move checks failed on every run measured; the spurious commit itself appeared in 4 of 7 cell-mode runs. - E2E: PressHeldOnAnotherRow_KeepsKeyboardFocusInGrid holds a press so the park always runs before the release. TapWhileGridRootHasFocus_OpensTheTappedEditor guards that a release the root claims still lets the tapped cell open its editor. The row-edit setup helper, FreshRowEditGridWithFirstRowEditing, retries the whole setup with a fresh navigation, re-resolves the Edit button's volatile slug, names the shape of each failed attempt, and reports recovered attempts through TestContext. Mutation-checked: no-oping the park, parking before the commit, parking unconditionally after it, no-oping BeginRowEdit, removing the release handler, and narrowing its guard to LeftButtonReleased or widening it to every release each redden the tests aimed at them. Fixes #1288 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959d8f7-94ca-4e1d-a617-c7bbf5e93b4e --- CHANGELOG.md | 15 + .../Controls/DataGrid/DataGridComponent.cs | 130 +++++++ .../Controls/DataGrid/DataGridState.cs | 64 +++ .../SelfTest/Fixtures/DataGridEditFixtures.cs | 367 ++++++++++++++++++ .../SelfTest/SelfTestFixtureRegistry.cs | 6 + tests/Reactor.AppTests/Tests/DataGridTests.cs | 280 +++++++++++-- .../Reactor.Tests/DataGridEditorFocusTests.cs | 284 ++++++++++++++ 7 files changed, 1117 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e4e64169..eb379ab81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -228,6 +228,21 @@ Conventions for contributors: `SetInitialValue` records, and `AfterFirstSubmit` waits for `MarkAllTouched()`. +- **Opening a DataGrid editor from inside the grid could commit it the moment it opened + (issue #1288).** Starting a row edit from the row's own "Edit" button, or tapping a cell + while another cell was being edited, removed the element holding keyboard focus in the + re-render that opened the new editor. XAML then moved focus to the next element in tab + order, which is outside the grid, and when the grid's blur-commit safety net saw that + before the new editor had taken focus, it committed the edit the user had just opened. The + editor closed as it appeared, and `onRowChanged` fired with unchanged values. It showed on + CI runners, not on fast machines. The grid now moves focus onto its own root before any + such re-render, so a removed element never holds focus. Focus is moved only when it is + already inside the grid. The root also keeps that focus through the mouse or touch release + that follows: WinUI's root scroller used to take focus on the release, which reopened the + same race when the button came up after the move. For the same reason, clicking a cell + while the grid itself holds keyboard focus, after tabbing onto it for example, no longer + moves that focus out of the grid. + - **The Visual Studio preview failed to start every session.** The extension was built against a newer `System.Text.Json` than Visual Studio binds extensions to, so it failed to load at runtime. It now tracks the `Microsoft.VisualStudio.SDK` baseline, with a test diff --git a/src/Reactor.Advanced/Controls/DataGrid/DataGridComponent.cs b/src/Reactor.Advanced/Controls/DataGrid/DataGridComponent.cs index 665e5bdf2..c10747c0b 100644 --- a/src/Reactor.Advanced/Controls/DataGrid/DataGridComponent.cs +++ b/src/Reactor.Advanced/Controls/DataGrid/DataGridComponent.cs @@ -43,6 +43,71 @@ public class DataGridComponent<[DynamicallyAccessedMembers(DynamicallyAccessedMe private static readonly Action StableNoopTap = (_, _) => { }; private static readonly Action StableNoopPointer = (_, _) => { }; + /// + /// The grid root's PointerReleased handler: while keyboard focus is on the root itself, + /// claim an unhandled release that can complete a primary-action press, so no ancestor + /// ScrollViewer takes focus out of the grid (#1288). Static, so the root's modifier stays + /// reference-stable across renders. + /// + /// + /// WinUI's ScrollViewer takes focus on an unhandled release of a press whose + /// IsLeftButtonPressed was true, unless it is not a tab stop AND focus is already inside + /// it (ScrollViewer::OnPointerReleased in microsoft-ui-xaml). The grid's own data-area + /// scroller therefore keeps focus that sits in an editor, but not focus on the root, which is + /// its ancestor. The release then bubbles to an ancestor scroller — at the very least the + /// window's root ScrollViewer, which is a tab stop — and that takes keyboard focus out of the + /// grid. + /// + /// Focus is on the root when the user tabbed onto the grid, or when + /// parked it there. The second is why this + /// is part of #1288: a cross-row press parks focus from a deferred callback, and when the + /// button comes up after that callback has run, the steal happened while the tap's new editor + /// was opening, so the blur-commit net committed it. Measured on CI before this handler: 1 of + /// 120 cross-row taps. + /// + /// Claiming the release is exactly what the data-area scroller already does for focus + /// inside it, and it does not stop the tap the release completes: a cell's Tapped still + /// fires, which Interactive_DataGrid_TapWhileGridRootHasFocus_OpensTheTappedEditor + /// checks. One consequence app code can see: a PointerReleased handler on an ancestor of + /// the grid receives such a release as handled, just as it already does while an editor has + /// focus. Which releases count is 's call. + /// + private static readonly Action HoldRootFocusThroughRelease = + (sender, e) => + { + if (!e.Handled + && sender is FrameworkElement root + && IsPrimaryActionRelease(e.GetCurrentPoint(root).Properties.PointerUpdateKind) + && IsFocusOnRoot(root)) + { + e.Handled = true; + } + }; + + /// + /// Whether a release can complete a primary-action press: a left mouse button, a touch contact, + /// or a pen tip. The ScrollViewer takes focus after no other kind of press. Only an explicit + /// release of another button returns false. + /// + /// + /// A touch contact and a pen tip are the pointer's first button. Win32 sets + /// POINTER_FLAG_FIRSTBUTTON while a touch pointer is in contact, and a touch pointer + /// uses no other button; WinRT's IsLeftButtonPressed documents both as the primary + /// action mode. Which PointerUpdateKind their release reports is not documented, though, + /// so this does not rely on a mapping: anything that is not an explicit right, middle, or + /// X-button release counts, including Other, a release that names no button. + /// + /// Erring that way is cheap. Claiming a release that did not complete a primary press + /// changes nothing for the ScrollViewer, which would not have taken focus for it; the only + /// effect is that ancestors see that release as handled. Explicit right, middle, and X-button + /// releases are never claimed, so ancestors still receive those unhandled. + /// + internal static bool IsPrimaryActionRelease(Microsoft.UI.Input.PointerUpdateKind kind) + => kind is not (Microsoft.UI.Input.PointerUpdateKind.RightButtonReleased + or Microsoft.UI.Input.PointerUpdateKind.MiddleButtonReleased + or Microsoft.UI.Input.PointerUpdateKind.XButton1Released + or Microsoft.UI.Input.PointerUpdateKind.XButton2Released); + public override Element Render() { var el = Props; @@ -388,6 +453,15 @@ void OnDataChanged(object? sender, EventArgs e) // would otherwise change the hook call sequence and throw HookOrderException). var lostFocusWired = UseRef(false); var lostFocusSetter = UseRef?>(null); + + // The live root control, captured by the setter below on every mount/update (a pooled + // root can change across mounts), and the reference-stable park hook that reads it. Both + // declared unconditionally for the same hook-order reason as the refs above (#1288). + var gridRootRef = UseRef(null); + var parkFocusHook = UseRef(null); + parkFocusHook.Current ??= () => ParkFocusOnGridRoot(gridRootRef.Current); + state.BeforeEditTransition = parkFocusHook.Current; + if (el.Editable) { // Cache the LostFocus setter (and its closure) in a ref so the lambda isn't @@ -398,6 +472,7 @@ void OnDataChanged(object? sender, EventArgs e) lostFocusSetter.Current ??= g => { + gridRootRef.Current = g; if (lostFocusWired.Current) return; lostFocusWired.Current = true; g.LostFocus += (sender, e) => @@ -489,6 +564,7 @@ void OnDataChanged(object? sender, EventArgs e) // invocation is a statement expression, which C# converts to a void-returning delegate. grid = grid .IsTabStop(true) + .OnPointerReleased(HoldRootFocusThroughRelease) .OnMount(fe => WireGridKeyDown(fe, state, elRef)); return grid; @@ -1307,6 +1383,60 @@ private static bool IsFocusInside(FrameworkElement root) return false; } + /// + /// Move keyboard focus onto the grid root when it currently sits on one of the grid's own + /// DESCENDANTS. Returns whether focus was moved. Wired as + /// , which explains why (#1288). + /// + /// + /// The root is the right place to park: it is a tab stop (IsTabStop(true)), it is + /// the element the blur-commit net is wired to, and it survives every re-render, unlike the + /// row's Edit button or a previous cell's editor. When an editor opens, its own deferred focus + /// request then moves focus from the root into it, entirely inside the grid. When a row click + /// only commits (it landed on a read-only cell), focus stays on the grid the user clicked + /// rather than jumping to the next tab stop outside it. That second case also needs the root's + /// release handler: a press parks from a deferred callback, so the button can come up after the + /// park, and without an ancestor ScrollViewer would + /// take focus on that release. + /// + /// Leaves focus alone in every other case, each for a reason: + /// + /// Focus already on the root — nothing the render removes holds it. + /// Focus outside the grid — a programmatic BeginEdit() from a toolbar + /// must not steal focus into the grid (the editor's own request decides that, as before). + /// No focus, or no live root — nothing to protect. + /// + /// + /// Programmatic focus state, so parking shows no focus visual on the root. + /// + internal static bool ParkFocusOnGridRoot(FrameworkElement? root) + { + if (root?.XamlRoot is not { } xamlRoot) return false; + + if (Microsoft.UI.Xaml.Input.FocusManager.GetFocusedElement(xamlRoot) is not DependencyObject focused + || ReferenceEquals(focused, root)) + return false; + + for (var parent = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetParent(focused); parent is not null; + parent = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetParent(parent)) + { + if (ReferenceEquals(parent, root)) + return root.Focus(FocusState.Programmatic); + } + + return false; + } + + /// + /// Whether keyboard focus is on itself — not a descendant, not + /// elsewhere. The condition under which claims a + /// pointer release: focus inside the data area is already kept by its own ScrollViewer, and + /// focus elsewhere is not the grid's to hold. + /// + internal static bool IsFocusOnRoot(FrameworkElement root) + => root.XamlRoot is { } xamlRoot + && ReferenceEquals(Microsoft.UI.Xaml.Input.FocusManager.GetFocusedElement(xamlRoot), root); + // ── Header rendering ──────────────────────────────────────────── private static Element RenderHeaderRow( diff --git a/src/Reactor.Advanced/Controls/DataGrid/DataGridState.cs b/src/Reactor.Advanced/Controls/DataGrid/DataGridState.cs index 8b943a963..362655475 100644 --- a/src/Reactor.Advanced/Controls/DataGrid/DataGridState.cs +++ b/src/Reactor.Advanced/Controls/DataGrid/DataGridState.cs @@ -139,6 +139,44 @@ public class DataGridState /// internal bool SuppressNextLostFocusCommit { get; set; } + /// + /// Invoked synchronously once the grid has committed to an edit transition whose re-render + /// destroys the element that currently holds keyboard focus: opening an editor + /// (, ), or ending the in-flight + /// edit on the first half of a commit-then-begin click (the cell tap, and the row pointer + /// press that precedes it on a cross-row tap). Runs before that re-render, while the element + /// still exists: ahead of the state change when an editor opens, and right after a commit + /// that actually ended the edit on the click paths. + /// reassigns it every render to park focus on the grid root when focus sits on one of the + /// grid's own descendants. Null when the state is driven without a renderer. + /// + /// + /// Why (issue #1288, measured on CI): when the focused element — the row's "Edit" + /// button, or the previous cell's editor — is removed from the tree, XAML moves focus to the + /// next element in tab order, which lies OUTSIDE the grid because the grid is a single tab + /// stop. The grid's blur-commit net then races the new editor's deferred focus: when its + /// deferred check runs first, it finds focus outside the grid and commits the edit the user + /// just opened — a spurious unchanged onRowChanged, and an editor that closes as it + /// appears. + /// + /// Parking focus on the root first means no focused element is ever destroyed, so + /// XAML never moves focus out of the grid and there is no race to lose. It is not a timing + /// heuristic: the render that removes the element is always a later dispatcher tick than the + /// transition that calls this. + /// + /// Only a transition that really happens parks. A begin that is refused (read-only + /// column, row out of range) never calls this, and neither does a commit that validation + /// refuses: that edit stays open with its editor mounted, and focus must stay in the editor + /// for the user to fix the value. That is why the click paths park AFTER the commit rather + /// than before it — only then is the refusal known — and it is still early enough, because + /// the commit's own re-render is deferred like every other. + /// + /// Deliberately NOT invoked for row-mode Tab traversal, which does not destroy the + /// focused editor: native Tab has already moved focus, and the + /// claim owns that focus-out. + /// + internal Action? BeforeEditTransition { get; set; } + // ── Editor focus requests (#976) ───────────────────────────── // // The focus APIs above move a purely LOGICAL cell cursor. This is the seam that turns an @@ -1229,11 +1267,20 @@ internal void InvokeRowPointerClick(RowKey key, bool ctrlKey, bool shiftKey) // Commit any active edit when clicking a DIFFERENT row. Clicking within the same row is // handled by the cell's OnTapped handler (commit-then-begin); skipping it here prevents the // editing TextBox being dismissed when the user clicks to position the cursor. + // + // This runs on pointer PRESS, before the cell's Tapped (on release) opens the next editor, + // so for a cross-row commit-then-begin tap it is this commit that schedules the removal of + // the focused editor. Park focus once the commit has ended the edit: the removal is a + // deferred re-render, so the editor still exists. Not when validation refused the commit — + // that edit stays open with its editor mounted, and the caret must stay in it (#1288). if (IsEditing) { var editingKey = EditingRowKey; if (editingKey is null || !editingKey.Value.Equals(key)) + { CommitInFlightEditThroughDispatcher(); + if (!IsEditing) BeforeEditTransition?.Invoke(); + } } SetFocus(idx, _focusedColIndex >= 0 ? _focusedColIndex : 0); @@ -1309,8 +1356,17 @@ internal void InvokeCellEditClick(RowKey key, string columnName) // Commit any in-flight edit BEFORE starting a new one — BeginEdit overwrites the pending // value with the new cell's current value, which would destroy an in-flight edit otherwise. + // + // A commit that ends the edit schedules the removal of its (focused) editor, so park focus + // then, while that editor still exists: the removal is a deferred re-render. Not when + // validation refused the commit — that editor stays mounted and keeps focus, unless + // BeginEdit below replaces it, which parks on its own. After a park here, BeginEdit's is a + // no-op (#1288). if (IsEditing) + { CommitInFlightEditThroughDispatcher(); + if (!IsEditing) BeforeEditTransition?.Invoke(); + } SetFocus(rowIdx, colIdx); BeginEdit(rowIdx, colIdx); @@ -1692,6 +1748,10 @@ public bool BeginEdit(int rowIndex, int colIndex) var rowKey = new RowKey(keyStr); var currentValue = col.GetValue(item!); + // The editor is definitely opening; move focus off anything the re-render will destroy + // while it still exists (#1288). + BeforeEditTransition?.Invoke(); + _editingRowKey = rowKey; _editingColumnName = col.Name; _editingValue = currentValue; @@ -1862,6 +1922,10 @@ public bool BeginRowEdit(int rowIndex) if (values.Count == 0) return false; + // The row edit is definitely opening; move focus off anything the re-render will destroy + // — above all the row's own "Edit" button, which the user just pressed (#1288). + BeforeEditTransition?.Invoke(); + _editingRowKey = rowKey; _editingColumnName = null; // null signals row mode _editingValue = null; diff --git a/tests/Reactor.AppTests.Host/SelfTest/Fixtures/DataGridEditFixtures.cs b/tests/Reactor.AppTests.Host/SelfTest/Fixtures/DataGridEditFixtures.cs index 1da6bb1b8..aa71b738f 100644 --- a/tests/Reactor.AppTests.Host/SelfTest/Fixtures/DataGridEditFixtures.cs +++ b/tests/Reactor.AppTests.Host/SelfTest/Fixtures/DataGridEditFixtures.cs @@ -3,6 +3,7 @@ using Microsoft.UI.Reactor.Data; using Microsoft.UI.Reactor.Data.Providers; using Microsoft.UI.Reactor.Controls; +using Microsoft.UI.Reactor.Controls.Validation; using Microsoft.UI.Reactor.AppTests.Host.SelfTest; using Microsoft.UI.Xaml.Controls; using static Microsoft.UI.Reactor.Factories; @@ -47,6 +48,15 @@ private static IReadOnlyList CreateEditableColumns() }; } + /// + /// with Name required, so a test can make validation + /// refuse a commit by emptying it. + /// + private static IReadOnlyList CreateEditableColumnsWithRequiredName() + => CreateEditableColumns() + .Select(c => c.Name == "Name" ? c with { Validators = [Validate.Required()] } : c) + .ToList(); + /// /// Mount an editable DataGrid, programmatically begin editing via state, /// and verify a TextBox editor appears in the visual tree. @@ -542,6 +552,363 @@ await Harness.WaitFor( } } + /// + /// Issue #1288: opening a row edit from the row's own "Edit" button must move keyboard focus off + /// that button BEFORE the re-render destroys it, and must not move focus at all when it sits + /// outside the grid. + /// + /// + /// The race this guards only shows on a slow machine — a destroyed focused element + /// triggers a focus change that the grid's blur-commit net can read as "focus left the grid" + /// before the new editor has claimed focus, committing the edit the user just opened — so an + /// end-to-end "no spurious commit" check here would pass with or without the fix. Two checks + /// discriminate instead. The instant right after BeginRowEdit returns: renders are + /// deferred to a later dispatcher tick, so at that instant the Edit button still exists. + /// Unfixed, it still holds focus; fixed, focus is already parked on the grid root. And the + /// whole transition, through : unfixed, the destroyed Edit button + /// sends focus to the outside anchor on every run, even when the editor then wins the race and + /// pulls it back; fixed, every move stays inside the grid. + /// + internal class EditorFocusParkedFromEditButton(Harness h) : SelfTestFixtureBase(h) + { + public override async Task RunAsync() + { + DataGridState? state = null; + var commits = 0; + var anchorRef = new Microsoft.UI.Reactor.Input.ElementRef(); + + var host = H.CreateHost(); + host.Mount(ctx => + { + var source = ctx.UseMemo(() => CreateSource(4)); + return VStack( + Button("outside anchor", () => { }).Ref(anchorRef), + Component, DataGridElement>( + new DataGridElement + { + Source = source, + Columns = CreateEditableColumns(), + Editable = true, + EditMode = EditMode.Row, + RowHeight = 36, + OnRowChanged = (_, _) => { commits++; return Task.CompletedTask; }, + OnStateReadyInternal = s => state = s, + })); + }); + + H.Check("EditorFocusPark_Rendered", + await Harness.WaitFor(() => H.FindButton("Edit") is not null, maxPasses: 40, perPassMs: 25)); + var edit = H.FindButton("Edit"); + var anchor = anchorRef.Current as Microsoft.UI.Xaml.Controls.Button; + if (state is null || edit?.XamlRoot is not { } xamlRoot || anchor is null) + { + H.Check("EditorFocusPark_Mounted", false); + return; + } + + object? Focused() => Microsoft.UI.Xaml.Input.FocusManager.GetFocusedElement(xamlRoot); + string Describe(object? o) => o switch + { + null => "null", + Microsoft.UI.Xaml.Controls.ContentControl { Content: string s } cc => $"{cc.GetType().Name}({s})", + Microsoft.UI.Xaml.Controls.TextBox tb => $"TextBox({tb.Text})", + _ => o.GetType().Name, + }; + + // Positive control: prove the window can REPORT focus before asserting where it went. + edit.Focus(Microsoft.UI.Xaml.FocusState.Pointer); + await Harness.Render(60); + if (!ReferenceEquals(Focused(), edit)) + { + H.Skip("EditorFocusPark_PositiveControl", + $"window cannot report focus (got '{Describe(Focused())}' right after focusing the Edit button)"); + return; + } + H.Check("EditorFocusPark_PositiveControl", true); + + // The first Edit button in tree order is row 0's; its editors are named "Product 0". + var gridRoot = GridRootOf(edit); + using (var moves = new FocusMoveRecorder(gridRoot, Describe)) + { + var began = state.BeginRowEdit(0); + var focusedRightAfter = Focused(); + H.Check($"EditorFocusPark_RowEdit_FocusLeftTheDoomedEditButton (began={began}, focused={Describe(focusedRightAfter)})", + began && !ReferenceEquals(focusedRightAfter, edit)); + H.Check($"EditorFocusPark_RowEdit_ParkedOnTheGridRoot (focused={Describe(focusedRightAfter)})", + focusedRightAfter is Microsoft.UI.Xaml.Controls.Grid { IsTabStop: true } root && IsAncestor(root, edit)); + H.Check("EditorFocusPark_RowEdit_IsFocusOnRootWhileParked", + gridRoot is not null && DataGridComponent.IsFocusOnRoot(gridRoot)); + + // ...and the editor's own request then takes focus from the root, with nothing committed. + await Harness.WaitFor(() => Focused() is Microsoft.UI.Xaml.Controls.TextBox, maxPasses: 40, perPassMs: 25); + H.Check($"EditorFocusPark_RowEdit_EditorTakesFocus (focused={Describe(Focused())})", + Focused() is Microsoft.UI.Xaml.Controls.TextBox { Text: "Product 0" }); + H.Check("EditorFocusPark_RowEdit_IsFocusOnRootFalseInAnEditor", + gridRoot is not null && !DataGridComponent.IsFocusOnRoot(gridRoot)); + H.Check($"EditorFocusPark_RowEdit_StillEditingWithNoCommit (commits={commits})", + state.IsRowEditing && commits == 0 && H.FindButton("Save") is not null); + H.Check($"EditorFocusPark_RowEdit_FocusNeverLeftTheGrid (moves={moves})", + moves.StayedInside); + } + + // ── Focus OUTSIDE the grid is never moved by the park ───────────────────────── + // A programmatic BeginRowEdit() from a toolbar must not have the park yank focus into + // the grid; what happens next is the editor request's call, exactly as before #1288. + state.CancelRowEdit(); + await Harness.Render(60); + anchor.Focus(Microsoft.UI.Xaml.FocusState.Programmatic); + await Harness.Render(60); + var anchorHeld = ReferenceEquals(Focused(), anchor); + H.Check($"EditorFocusPark_Outside_AnchorHeldFocus (focused={Describe(Focused())})", anchorHeld); + H.Check("EditorFocusPark_Outside_IsFocusOnRootFalse", + gridRoot is not null && !DataGridComponent.IsFocusOnRoot(gridRoot)); + if (anchorHeld) + { + state.BeginRowEdit(1); + H.Check($"EditorFocusPark_Outside_ParkLeftFocusAlone (focused={Describe(Focused())})", + ReferenceEquals(Focused(), anchor)); + } + + state.CancelRowEdit(); + await Harness.Render(60); + } + } + + /// + /// Issue #1288, cell mode: a tap that commits one cell edit and opens another destroys the + /// in-flight editor, which holds focus. Focus must be parked on the grid root BEFORE that + /// commit schedules the editor's removal. Same two discriminating checks as + /// . + /// + /// + /// With the park no-oped, focus goes to the outside anchor on every run. Whether the blur-commit + /// net then commits the edit the tap just opened is the race: measured locally, it did in four + /// runs of seven (commits=[0:Product 0:A 1:Product 1:B], the #1288 symptom) and not in + /// the other three, which is exactly why the move record, not the commit count, is what + /// discriminates here. + /// + internal class EditorFocusParkedOnCommitThenBegin(Harness h) : SelfTestFixtureBase(h) + { + public override async Task RunAsync() + { + DataGridState? state = null; + var commits = new List(); + + var host = H.CreateHost(); + host.Mount(ctx => + { + var source = ctx.UseMemo(() => CreateSource(4)); + + // A focusable element OUTSIDE the grid, as any real app has. Without one XAML has + // nowhere to send focus from a destroyed editor but the grid root, so the + // FocusNeverLeftTheGrid checks below could not fail: measured, they passed with the + // park no-oped until this anchor was added. + return VStack( + Button("outside anchor", () => { }), + Component, DataGridElement>( + new DataGridElement + { + Source = source, + // Name is required so the last phase can make validation refuse a commit. + Columns = CreateEditableColumnsWithRequiredName(), + Editable = true, + EditMode = EditMode.Cell, + RowHeight = 36, + OnRowChanged = (key, item) => { commits.Add($"{key.Value}:{item.Name}:{item.Category}"); return Task.CompletedTask; }, + OnStateReadyInternal = s => state = s, + })); + }); + + H.Check("EditorFocusParkCell_Rendered", + await Harness.WaitFor(() => H.FindTextContaining("Product 1") is not null, maxPasses: 40, perPassMs: 25)); + if (state is null || H.FindTextContaining("Product 1")?.XamlRoot is not { } xamlRoot) + { + H.Check("EditorFocusParkCell_Mounted", false); + return; + } + + object? Focused() => Microsoft.UI.Xaml.Input.FocusManager.GetFocusedElement(xamlRoot); + string Describe(object? o) => o switch + { + null => "null", + Microsoft.UI.Xaml.Controls.TextBox tb => $"TextBox({tb.Text})", + _ => o.GetType().Name, + }; + + // Open row 0 / Name. Its editor taking focus doubles as the positive control. + state.BeginEdit(0, 1); + await Harness.WaitFor(() => Focused() is Microsoft.UI.Xaml.Controls.TextBox, maxPasses: 40, perPassMs: 25); + if (Focused() is not Microsoft.UI.Xaml.Controls.TextBox { Text: "Product 0" } firstEditor) + { + H.Skip("EditorFocusParkCell_PositiveControl", + $"window cannot report focus (got '{Describe(Focused())}' after opening row 0 / Name)"); + return; + } + H.Check("EditorFocusParkCell_PositiveControl", true); + + // Tap row 1 / Category while row 0 / Name is being edited: commit-then-begin. + var gridRoot = GridRootOf(firstEditor); + using (var moves = new FocusMoveRecorder(gridRoot, Describe)) + { + state.InvokeCellEditClick(new RowKey(state.GetRowKeyAt(1)!), "Category"); + var focusedRightAfter = Focused(); + H.Check($"EditorFocusParkCell_FocusLeftTheDoomedEditor (focused={Describe(focusedRightAfter)})", + !ReferenceEquals(focusedRightAfter, firstEditor)); + H.Check($"EditorFocusParkCell_ParkedOnTheGridRoot (focused={Describe(focusedRightAfter)})", + focusedRightAfter is Microsoft.UI.Xaml.Controls.Grid { IsTabStop: true } root && IsAncestor(root, firstEditor)); + + // Row 1's Category is "B". Exactly one commit — row 0's — and none for the edit just opened. + await Harness.WaitFor(() => Focused() is Microsoft.UI.Xaml.Controls.TextBox { Text: "B" }, maxPasses: 40, perPassMs: 25); + H.Check($"EditorFocusParkCell_NewEditorTakesFocus (focused={Describe(Focused())})", + Focused() is Microsoft.UI.Xaml.Controls.TextBox { Text: "B" }); + H.Check($"EditorFocusParkCell_OnlyTheTappedAwayEditCommitted (commits=[{string.Join(" ", commits)}])", + commits.Count == 1 && commits[0].StartsWith("0:", StringComparison.Ordinal) + && state.IsEditing && state.EditingColumnName == "Category"); + H.Check($"EditorFocusParkCell_FocusNeverLeftTheGrid (moves={moves})", + moves.StayedInside); + } + + // ── Cross-row tap: the row's pointer PRESS commits first ────────────────────── + // This is the path the CI diagnostics named. On a cross-row tap the row's PointerPressed + // commits the in-flight edit BEFORE the cell's Tapped (on release) opens the next editor, + // so it is the press, not the tap, whose commit dooms the focused editor. + using (var moves = new FocusMoveRecorder(gridRoot, Describe)) + { + var secondEditor = Focused() as Microsoft.UI.Xaml.Controls.TextBox; + state.InvokeRowPointerClick(new RowKey(state.GetRowKeyAt(2)!), ctrlKey: false, shiftKey: false); + var focusedAfterPress = Focused(); + H.Check($"EditorFocusParkCell_CrossRowPress_FocusLeftTheDoomedEditor (focused={Describe(focusedAfterPress)})", + secondEditor is not null && !ReferenceEquals(focusedAfterPress, secondEditor)); + H.Check($"EditorFocusParkCell_CrossRowPress_ParkedOnTheGridRoot (focused={Describe(focusedAfterPress)})", + secondEditor is not null + && focusedAfterPress is Microsoft.UI.Xaml.Controls.Grid { IsTabStop: true } pressRoot + && IsAncestor(pressRoot, secondEditor)); + + // The Tapped that follows opens row 2 / Name. Row 1's Category edit committed on the press; + // the edit just opened must not be committed with it. + state.InvokeCellEditClick(new RowKey(state.GetRowKeyAt(2)!), "Name"); + await Harness.WaitFor(() => Focused() is Microsoft.UI.Xaml.Controls.TextBox { Text: "Product 2" }, maxPasses: 40, perPassMs: 25); + H.Check($"EditorFocusParkCell_CrossRowPress_NewEditorTakesFocus (focused={Describe(Focused())})", + Focused() is Microsoft.UI.Xaml.Controls.TextBox { Text: "Product 2" }); + H.Check($"EditorFocusParkCell_CrossRowPress_NoSpuriousCommit (commits=[{string.Join(" ", commits)}])", + commits.Count == 2 && commits[1].StartsWith("1:", StringComparison.Ordinal) + && state.IsEditing && state.EditingColumnName == "Name"); + H.Check($"EditorFocusParkCell_CrossRowPress_FocusNeverLeftTheGrid (moves={moves})", + moves.StayedInside); + } + + // ── A commit that validation refuses moves nothing ──────────────────────────── + // CommitEdit refuses while the edit has validation errors, so the edit stays open with + // its editor mounted for the user to fix. Nothing destroys that editor, so nothing may + // move focus off it: a press on another row must leave the caret where the user was + // typing, exactly as before #1288. Parking is for a commit that actually ends the edit. + if (Focused() is Microsoft.UI.Xaml.Controls.TextBox invalidEditor) + { + state.UpdateEditingValue(""); // Name is required + H.Check($"EditorFocusParkCell_Refused_PositiveControl (errors={state.HasValidationErrors})", + state.HasValidationErrors); + + state.InvokeRowPointerClick(new RowKey(state.GetRowKeyAt(3)!), ctrlKey: false, shiftKey: false); + H.Check($"EditorFocusParkCell_Refused_FocusStayedInTheEditor (focused={Describe(Focused())}, editing={state.IsEditing})", + ReferenceEquals(Focused(), invalidEditor) && state.IsEditing && state.EditingColumnName == "Name"); + + await Harness.Render(120); + H.Check($"EditorFocusParkCell_Refused_EditorStillFocusedAfterRender (focused={Describe(Focused())}, editing={state.IsEditing})", + Focused() is Microsoft.UI.Xaml.Controls.TextBox && state.IsEditing); + } + else + { + H.Check($"EditorFocusParkCell_Refused_EditorFocused (focused={Describe(Focused())})", false); + } + + state.CancelEdit(); + await Harness.Render(60); + } + } + + /// + /// The innermost ancestor of that is a tab-stop Grid, which in a + /// DataGrid is its root: the root is the only element the grid marks IsTabStop(true). + /// + private static Microsoft.UI.Xaml.Controls.Grid? GridRootOf(Microsoft.UI.Xaml.DependencyObject node) + { + for (var p = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetParent(node); p is not null; + p = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetParent(p)) + { + if (p is Microsoft.UI.Xaml.Controls.Grid { IsTabStop: true } root) return root; + } + + return null; + } + + /// + /// Records every keyboard-focus move for as long as it is alive, through the static + /// FocusManager.LosingFocus event, which reports each move synchronously with both ends. + /// + /// + /// Issue #1288's invariant is about the WHOLE transition: between an edit starting and its editor + /// taking focus, keyboard focus must never leave the grid, because a moment outside is all the + /// blur-commit net needs. A read of the focused element at any single instant cannot establish + /// that. Unlike the "no spurious commit" checks, which only fail when the net happens to win the + /// race, this one is deterministic: on unfixed code focus leaves the grid on every transition. + /// + private sealed class FocusMoveRecorder : IDisposable + { + private readonly Microsoft.UI.Xaml.DependencyObject? _gridRoot; + private readonly Func _describe; + private readonly List<(string From, string To, bool FromInside, bool ToInside)> _moves = new(); + + public FocusMoveRecorder(Microsoft.UI.Xaml.DependencyObject? gridRoot, Func describe) + { + _gridRoot = gridRoot; + _describe = describe; + Microsoft.UI.Xaml.Input.FocusManager.LosingFocus += OnLosingFocus; + } + + // Classified when the move HAPPENS: the element losing focus is usually the one being + // destroyed, and once it has left the tree nothing can say where it used to be. + private void OnLosingFocus(object? sender, Microsoft.UI.Xaml.Input.LosingFocusEventArgs e) + => _moves.Add((_describe(e.OldFocusedElement), _describe(e.NewFocusedElement), + IsInside(e.OldFocusedElement), IsInside(e.NewFocusedElement))); + + /// + /// Whether focus STARTED inside the grid, moved at least once, and every move landed on the + /// grid root or inside it. + /// + /// + /// Both extra conditions keep the check from passing vacuously. A transition that destroys + /// the focused element always moves focus, so an empty record means the recorder saw nothing, + /// not that nothing happened. And a transition that began with focus already outside the + /// grid — say, left there by an earlier phase that failed — has nothing to escape from: + /// measured, in a run where the park was no-oped and the phase before had committed + /// spuriously, the cross-row phase began on the outside anchor and passed without this. + /// + public bool StayedInside + => _gridRoot is not null + && _moves.Count > 0 + && _moves[0].FromInside + && _moves.All(m => m.ToInside); + + private bool IsInside(Microsoft.UI.Xaml.DependencyObject? node) + => _gridRoot is not null && node is not null + && (ReferenceEquals(node, _gridRoot) || IsAncestor(_gridRoot, node)); + + public override string ToString() => string.Join(" ", _moves.Select(m => $"{m.From}>{m.To}")); + + public void Dispose() => Microsoft.UI.Xaml.Input.FocusManager.LosingFocus -= OnLosingFocus; + } + + private static bool IsAncestor(Microsoft.UI.Xaml.DependencyObject ancestor, Microsoft.UI.Xaml.DependencyObject node) + { + for (var p = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetParent(node); p is not null; + p = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetParent(p)) + { + if (ReferenceEquals(p, ancestor)) return true; + } + + return false; + } + /// /// Regression for GitHub #34. When a child element's type flips inside a Grid /// (TextBlock → TextBox → TextBlock — e.g. a DataGrid cell entering and leaving diff --git a/tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs b/tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs index 3c4fd3e9f..c651e601e 100644 --- a/tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs +++ b/tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs @@ -902,6 +902,9 @@ internal static class SelfTestFixtureRegistry "DataGrid_EditorFocusCustomEditors", "DataGrid_EditorFocusDebtRepaid", "DataGrid_EditorFocusDisconnectedRoot", + // Parking focus before an editor open destroys the focused element (issue #1288) + "DataGrid_EditorFocusParkedFromEditButton", + "DataGrid_EditorFocusParkedOnCommitThenBegin", // DataGrid row-detail expansion (issue #919) "DataGrid_ExpandRowKeepsRealizedRow", "DataGrid_LazyStackRootTypeFlip", @@ -2822,6 +2825,9 @@ internal static string[] StaleTierDeclarations() => "DataGrid_EditorFocusCustomEditors" => new DataGridEditFixtures.EditorFocusCustomEditors(harness), "DataGrid_EditorFocusDebtRepaid" => new DataGridEditFixtures.EditorFocusDebtRepaid(harness), "DataGrid_EditorFocusDisconnectedRoot" => new DataGridEditFixtures.EditorFocusDisconnectedRoot(harness), + // Parking focus before an editor open destroys the focused element (issue #1288) + "DataGrid_EditorFocusParkedFromEditButton" => new DataGridEditFixtures.EditorFocusParkedFromEditButton(harness), + "DataGrid_EditorFocusParkedOnCommitThenBegin" => new DataGridEditFixtures.EditorFocusParkedOnCommitThenBegin(harness), // DataGrid row-detail expansion (issue #919) "DataGrid_ExpandRowKeepsRealizedRow" => new DataGridExpandFixtures.ExpandRowKeepsRealizedRow(harness), "DataGrid_LazyStackRootTypeFlip" => new DataGridExpandFixtures.LazyStackRootTypeFlip(harness), diff --git a/tests/Reactor.AppTests/Tests/DataGridTests.cs b/tests/Reactor.AppTests/Tests/DataGridTests.cs index 347508056..312707f82 100644 --- a/tests/Reactor.AppTests/Tests/DataGridTests.cs +++ b/tests/Reactor.AppTests/Tests/DataGridTests.cs @@ -71,6 +71,89 @@ public void Interactive_DataGrid_ClickEditTabCommit() Assert.IsNotNull(WaitForName("Johnson"), "'Johnson' should be visible after commit"); } + /// + /// Issue #1288, the release that comes after the park. A press on another row commits the open + /// edit, and the grid parks keyboard focus on its root, from a deferred callback, so that the + /// editor about to be removed is not holding it. Holding the button makes that callback run + /// BEFORE the release, deterministically. WinUI's ScrollViewer takes focus on an unhandled + /// release unless focus is already inside it, and the grid's data-area scroller does not + /// contain the root, so without the root's own release handler an ancestor scroller took + /// keyboard focus out of the grid. On a tap that opens an editor, that let the blur-commit net + /// commit the new edit (1 of 120 cross-row taps on CI). Here the pressed cell is read-only, so + /// nothing opens and the loss is deterministic. + /// + /// + /// The oracle is keyboard input: F2 edits the cursor's cell only while the grid holds keyboard + /// focus. Before #1288, focus escaped when the open edit's editor was removed; with the park but + /// without the root's release handler, the ancestor scroller took it on the release. Either way + /// F2 went elsewhere and no editor opened. + /// + [E2eRetry(3)] + [TestMethod] + public void Interactive_DataGrid_PressHeldOnAnotherRow_KeepsKeyboardFocusInGrid() + { + ParkFocusOnGridRootByPressHoldingRowTwo(); + + App.SendKeys("f2", viaSendInput: true); + var editor = WaitForEditor(timeoutMs: 4000); + Assert.AreEqual("Bob", editor.Text, "F2 should have opened the editor on row 2 / FirstName."); + } + + /// + /// Guard for the root's release handler (#1288): claiming a release must not stop the tap that + /// the release completes. With keyboard focus on the grid root, the root claims the release of + /// a tap on an editable cell, and the cell's Tapped must still open its editor. This is + /// also the flow after a user tabs onto the grid and clicks a cell to edit it. + /// + /// + /// The setup is the one + /// shows leaves keyboard focus on the root. The press on row 3 moves no focus, because no edit + /// is open by then, so its release is claimed. This test passes without the handler too: the + /// window's root ScrollViewer then takes focus on the release and marks it handled instead. It + /// guards against the handler breaking click-to-edit; it does not detect #1288. + /// + [E2eRetry(3)] + [TestMethod] + public void Interactive_DataGrid_TapWhileGridRootHasFocus_OpensTheTappedEditor() + { + ParkFocusOnGridRootByPressHoldingRowTwo(); + + TapCell("Carol"); + var editor = WaitForEditor(timeoutMs: 4000); + Assert.AreEqual("Carol", editor.Text, "The tap should have opened the editor on row 3 / FirstName."); + } + + /// + /// Leave keyboard focus parked on the root of the cell-mode grid: open row 1 / FirstName, then + /// press and hold row 2's read-only Id cell and release in place. The press commits the open + /// edit, unchanged, and parks focus on the root from a deferred callback; the hold makes that + /// callback run before the release, which the root then claims. + /// + private void ParkFocusOnGridRootByPressHoldingRowTwo() + { + NavigateToFixtureFresh("DataGrid_EditableGrid"); + WaitForText("EditLog", "Edits:"); + Assert.IsNotNull(WaitForName("Alice"), "'Alice' should be visible"); + + // Open row 1 / FirstName; the grid's cursor is now in the FirstName column. + TapCell("Alice"); + _ = WaitForEditor(); + + // The Id cells are matched exactly, because a substring search for "2" also finds salaries. + var idCell = App.Search("2") + .Where(m => m.Name == "2" && !m.IsOffscreen) + .OrderBy(m => m.Y) + .FirstOrDefault(); + Assert.IsNotNull(idCell, "Row 2's Id cell ('2') should be visible."); + var x = idCell.X + idCell.Width / 2; + var y = idCell.Y + idCell.Height / 2; + App.Drag($"{x},{y}", $"{x},{y}", holdMs: 400); + + // The press committed the open edit, unchanged, and moved the cursor to row 2 while keeping + // its column: row 2 / FirstName. + WaitForTextContaining("EditLog", "[1:Alice,Smith]", timeoutMs: 5000); + } + /// /// Regression: pressing Tab WHILE EDITING must commit the current cell AND leave the inline /// editor reopened on the next cell — it must not be torn down. The grid's deferred LostFocus @@ -252,11 +335,7 @@ public void Interactive_DataGrid_EditingTabToReadOnly_DoesNotSuppressNextCommit( [TestMethod] public void Interactive_DataGrid_RowEditTab_WrapsToFirstEditorWithoutCommitting() { - NavigateToFixtureFresh("DataGrid_RowEditGrid"); - WaitForText("RowEditLog", "Edits:"); - Assert.IsNotNull(WaitForName("Alice"), "'Alice' (row 1 FirstName) should be visible"); - - BeginRowEditOnFirstRow(); + FreshRowEditGridWithFirstRowEditing(); // Focus starts in FirstName. Step forward through every editable column, asserting the // DESTINATION of each move. Three editable columns make direction expressible: forward @@ -305,11 +384,7 @@ public void Interactive_DataGrid_RowEditTab_WrapsToFirstEditorWithoutCommitting( [TestMethod] public void Interactive_DataGrid_RowEditTab_DoesNotSuppressNextCommit() { - NavigateToFixtureFresh("DataGrid_RowEditGrid"); - WaitForText("RowEditLog", "Edits:"); - Assert.IsNotNull(WaitForName("Alice"), "'Alice' (row 1 FirstName) should be visible"); - - BeginRowEditOnFirstRow(); + FreshRowEditGridWithFirstRowEditing(); // Type into FirstName, Tab (arms the guard), then type into MiddleName — only reachable if // the Tab moved focus without committing. @@ -377,33 +452,176 @@ private static void WaitForFocus(string expectedAutomationId, string step, int t } /// - /// Click the first row's "Edit" button to enter row-edit mode and wait until its editors are - /// realized. Row mode has one Edit button per row and they share a name, so take the topmost. + /// Land the row-edit fixture in the exact state both row-mode Tab tests require: freshly + /// navigated, RowEditLog still empty, and row 1 in row-edit mode with its editors + /// realized. /// - private void BeginRowEditOnFirstRow() + /// + /// This retries the WHOLE setup — navigate, then begin the row edit — rather than just + /// re-clicking, because two different things can undo a begin-edit before the caller looks, + /// and a caller that only asks "is there a Save button?" cannot tell them apart. Reporting a + /// bare null for both is what made issue #1288 unreadable from a CI log. + /// + /// Shape 1 — the click lands but nothing happens. winapp ui click is + /// SendInput: it goes to whatever window is foreground at that instant, so losing activation + /// between winapp's own foreground check and the injection sends the press elsewhere while the + /// verb still reports success. RowEditLog stays "Edits:". + /// + /// Shape 2 — the row edit starts and is committed before Save can be seen. The + /// tell is an unchanged entry appearing in the log, e.g. Edits:[1:Alice,Marie,Smith]. + /// This was a DataGrid bug (#1288): the re-render that opens the row editors removed the Edit + /// button while it still held keyboard focus, XAML moved focus to the next tab stop — the + /// blur anchor, outside the grid — and the grid's blur-commit net saw that before the new + /// editor had claimed focus. On stock code the CI stress lane recorded it as the FIRST attempt + /// of DoesNotSuppressNextCommit's setup in 13 of 60 runs, every one of this shape and + /// with the Host foreground throughout; its focus trace showed the jump to the blur anchor + /// every time. The grid now parks focus on its root before that re-render + /// (DataGridState.BeforeEditTransition). The retry has to RE-NAVIGATE rather than just + /// re-click: a spurious commit left in RowEditLog would break both callers' oracles, + /// which count '[' occurrences. + /// + /// What the retry can and cannot heal. It cannot heal a grid that no longer enters + /// row-edit mode: that produces no Save on any attempt AND no log entry, so the failure still + /// fires and names the shape of each attempt. Verified by mutation: with BeginRowEdit + /// no-oped, all four attempts report shape 1 and the test fails. It CAN heal an intermittent + /// shape 2, and #1288 was exactly that — a race lost on only some attempts — so this helper is + /// deliberately not what guards #1288. The DataGrid_EditorFocusParked* selftests do, + /// deterministically: their FocusNeverLeftTheGrid checks record every focus move, and + /// with the focus park removed they failed on every run measured, whether or not the race was + /// lost. + /// + /// A recovered attempt is written to the test's output rather than silently absorbed, + /// so the TRX of a passing run still says how often the setup had to be retried, and why. + /// A retry that heals a flake must not also erase the evidence of it. + /// + private void FreshRowEditGridWithFirstRowEditing() { - var deadline = DateTime.UtcNow.AddMilliseconds(5000); - while (DateTime.UtcNow < deadline) + const int MaxAttempts = 4; + var attempts = new List(); + + for (var attempt = 1; attempt <= MaxAttempts; attempt++) { - var buttons = App.Search("Edit").Where(m => m.Name == "Edit").ToList(); - if (buttons.Count > 0) + NavigateToFixtureFresh("DataGrid_RowEditGrid"); + WaitForText("RowEditLog", "Edits:"); + Assert.IsNotNull(WaitForName("Alice"), "'Alice' (row 1 FirstName) should be visible"); + + var note = TryBeginRowEditOnFirstRow(); + if (note is null) { - var first = buttons[0]; - // Normalize a missing AutomationId to null rather than "": UiElement.GetAttribute - // branches on `AutomationId != null`, so an empty-but-non-null id would send it - // down the read-by-automation-id path with an empty id. - var id = string.IsNullOrEmpty(first.AutomationId) ? null : first.AutomationId; - Element(id ?? first.Selector, id).Click(); - // Save/Cancel only exist while the row is being edited, so their arrival is proof - // the row edit actually started before we start pressing Tab. - Assert.IsNotNull(WaitForName("Save"), "Row edit did not start — no 'Save' button appeared."); - _ = WaitForEditor(); + if (attempts.Count > 0) + TestContext?.WriteLine( + $"[#1288] row edit started on attempt {attempt} of {MaxAttempts} after:\n " + + string.Join("\n ", attempts)); return; } + + attempts.Add($"attempt {attempt}: {note}"); + } + + Assert.Fail( + $"Row edit never started on row 1 after {MaxAttempts} attempts.\n " + + string.Join("\n ", attempts) + "\n" + DumpRowEditState()); + } + + /// + /// One attempt at putting row 1 into row-edit mode. Returns on success, + /// otherwise a description of what was observed instead. + /// + private string? TryBeginRowEditOnFirstRow() + { + var button = WaitForTopmostRowEditButton(); + if (button is null) + return "row-mode 'Edit' button never appeared"; + + var logBefore = App.GetValue("RowEditLog") ?? ""; + var foregroundBefore = HostIsForeground(); + try + { + // The row Edit buttons carry no AutomationId, so they are addressed by winapp's + // volatile slug — a display hint, not a stable handle. It is resolved fresh on every + // attempt because a previous attempt's re-render invalidates it. + Element(button.Selector).Click(); + } + catch (WinAppException ex) + { + return $"click on {button.Selector} was refused — {ex.Message.Trim()}"; + } + + // Save/Cancel only exist while the row is being edited, so their arrival is proof the row + // edit actually started before we start pressing Tab. + if (WaitForName("Save", timeoutMs: 2500) is not null) + { + _ = WaitForEditor(); + return null; + } + + var logAfter = App.GetValue("RowEditLog") ?? ""; + var where = $"{button.Selector} at ({button.X},{button.Y}); " + + $"host foreground before/after click = {foregroundBefore}/{HostIsForeground()}"; + + return logAfter != logBefore + ? $"row edit started but committed before 'Save' could be observed — RowEditLog went " + + $"'{logBefore}' -> '{logAfter}' (clicked {where})" + : $"click landed but nothing happened — RowEditLog still '{logAfter}' (clicked {where})"; + } + + /// + /// The on-screen row-mode "Edit" button of the FIRST row, or if none + /// appears. Row mode renders one Edit button per row and they all share the name, so the rows + /// are ordered by their vertical position rather than by whatever order UIA happens to return. + /// + private static UiMatch? WaitForTopmostRowEditButton(int timeoutMs = 5000) + { + var deadline = DateTime.UtcNow.AddMilliseconds(timeoutMs); + do + { + var topmost = App.Search("Edit") + .Where(m => m.Name == "Edit" && !m.IsOffscreen) + .OrderBy(m => m.Y) + .FirstOrDefault(); + if (topmost is not null) + return topmost; Thread.Sleep(100); } + while (DateTime.UtcNow < deadline); - Assert.Fail("Row-mode 'Edit' button never appeared."); + return null; + } + + /// + /// Whether the Host window currently holds the foreground, or when that + /// cannot be read. Recorded around the begin-edit click because both failure shapes described + /// on come from something else owning the + /// desktop, and the before/after pair is the cheapest evidence of whether it did. + /// + private static bool? HostIsForeground() + { + try { return App.ListWindows().FirstOrDefault(w => w.Hwnd == HostHwnd)?.IsForeground; } + catch (WinAppException) { return null; } + } + + /// + /// Everything needed to tell the failure shapes apart from a CI log alone, so a recurrence of + /// #1288 never again reports only actual: null. + /// + private string DumpRowEditState() + { + string Fmt(UiMatch m) => + $" type={m.Type} name='{m.Name}' aid='{m.AutomationId}' sel='{m.Selector}' " + + $"rect=({m.X},{m.Y},{m.Width}x{m.Height}) enabled={m.IsEnabled} offscreen={m.IsOffscreen}"; + + var sb = new System.Text.StringBuilder(); + foreach (var probe in new[] { "Save", "Cancel" }) + sb.AppendLine($" search('{probe}') -> {App.Search(probe).Count} match(es)"); + foreach (var m in App.Search("Edit").Where(m => m.Name == "Edit")) + sb.AppendLine($" row 'Edit' button:\n{Fmt(m)}"); + sb.AppendLine($" inline editor: {App.FindFirstEditableSelector() ?? ""}"); + sb.AppendLine($" focused AutomationId: '{Uia.GetFocusedAutomationId()}'"); + sb.AppendLine($" RowEditLog: '{App.GetValue("RowEditLog")}'"); + sb.AppendLine($" FixtureStatus: '{App.GetValue("FixtureStatus")}'"); + foreach (var w in App.ListWindows()) + sb.AppendLine($" window hwnd={w.Hwnd} foreground={w.IsForeground} title='{w.Title}'"); + return sb.ToString(); } /// @@ -574,7 +792,11 @@ private UiElement WaitForEditor(int timeoutMs = 5000) return Element(selector); Thread.Sleep(100); } - throw new WinAppException("DataGrid inline editor (Edit control) did not appear after the cell tap."); + // EditLog tells the two causes apart: an editor that never opened leaves it unchanged, while + // one that opened and was committed straight away (#1288) leaves an unchanged entry behind. + throw new WinAppException( + "DataGrid inline editor (Edit control) did not appear after the cell tap. " + + $"EditLog='{App.GetValue("EditLog")}'"); } /// diff --git a/tests/Reactor.Tests/DataGridEditorFocusTests.cs b/tests/Reactor.Tests/DataGridEditorFocusTests.cs index e8e7c9f43..838db0415 100644 --- a/tests/Reactor.Tests/DataGridEditorFocusTests.cs +++ b/tests/Reactor.Tests/DataGridEditorFocusTests.cs @@ -1,4 +1,5 @@ using Microsoft.UI.Reactor.Controls; +using Microsoft.UI.Reactor.Controls.Validation; using Microsoft.UI.Reactor.Data; using Xunit; using VirtualKey = global::Windows.System.VirtualKey; @@ -959,4 +960,287 @@ internal static class NonGenericInner internal static readonly object Marker = new(); } } + + // ── Parking focus before an edit transition destroys it (#1288) ── + // + // Opening an editor from inside the grid, or ending the in-flight edit on the first half of a + // commit-then-begin click, destroys the element that holds focus (the row's Edit button, or + // the previous cell's editor). DataGridComponent parks focus on the grid root through + // BeforeEditTransition so that element is never focused when it dies. The park itself is XAML + // and is covered by a selftest; what is pure state, and asserted here, is WHEN the hook runs. + // That is the part a refactor breaks silently: a hook that fires too late (or not at all), or + // one that fires for a commit validation refused, still compiles, and the harm only shows on a + // slow CI runner or with an invalid value in the editor. + + /// Records each hook call with the state it observed, and each StateChanged. + private sealed class TransitionLog + { + public readonly List Events = new(); + + public void Attach(DataGridState state) + { + state.BeforeEditTransition = () => Events.Add( + $"park(editing={state.IsEditing},row={state.IsRowEditing},col={state.EditingColumnName ?? "-"})"); + state.StateChanged += () => Events.Add("changed"); + } + + public int Parks => Events.Count(e => e.StartsWith("park", StringComparison.Ordinal)); + } + + [Fact] + public async Task BeginRowEdit_ParksFocusBeforeTheTransition() + { + var state = await LoadedState(); + var log = new TransitionLog(); + log.Attach(state); + + Assert.True(state.BeginRowEdit(1)); + + // Exactly one park, FIRST, observing the state before the row edit began. A park after + // "changed" would race the re-render that destroys the focused Edit button. + Assert.Equal(1, log.Parks); + Assert.Equal("park(editing=False,row=False,col=-)", log.Events[0]); + Assert.Contains("changed", log.Events.Skip(1)); + } + + [Fact] + public async Task BeginEdit_ParksFocusBeforeTheTransition() + { + var state = await LoadedState(); + var log = new TransitionLog(); + log.Attach(state); + + Assert.True(state.BeginEdit(1, ScoreCol)); + + Assert.Equal(1, log.Parks); + Assert.Equal("park(editing=False,row=False,col=-)", log.Events[0]); + Assert.Contains("changed", log.Events.Skip(1)); + } + + [Fact] + public async Task ACommitThenBeginTap_ParksOnceTheCommitHasEndedTheEdit() + { + var state = await LoadedState(); + Assert.True(state.BeginEdit(1, ScoreCol)); + state.UpdateEditingValue(42.0); + var log = new TransitionLog(); + log.Attach(state); + + state.InvokeCellEditClick(Row(0), "Notes"); + + // The first park comes right AFTER the commit ended the Score edit: only then is it known + // that the commit was not refused, and it is still in time, because the re-render that + // removes the editor is deferred (the selftests read focus at exactly that instant). The + // second park comes from BeginEdit and is a no-op in practice (focus is already on the root). + Assert.Equal("park(editing=False,row=False,col=-)", log.Events[log.Events.IndexOf("changed") + 1]); + Assert.Equal(2, log.Parks); + + // ...and the tap still did its job: the old edit committed, the new one is open. + Assert.Equal(42.0, state.GetItemAt(1)!.Score); + Assert.Equal("Notes", state.EditingColumnName); + Assert.Equal(Row(0), state.EditingRowKey); + } + + [Fact] + public async Task ATapWithNothingInFlight_ParksOnce() + { + var state = await LoadedState(); + var log = new TransitionLog(); + log.Attach(state); + + state.InvokeCellEditClick(Row(0), "Notes"); + + Assert.Equal(1, log.Parks); + Assert.Equal("Notes", state.EditingColumnName); + } + + [Fact] + public async Task ABeginThatFails_DoesNotPark() + { + var state = await LoadedState(); + var log = new TransitionLog(); + log.Attach(state); + + // An out-of-range row and a read-only column both refuse to open an editor. Parking on a + // refused begin would move the user's focus for an edit that never happened. + Assert.False(state.BeginRowEdit(99)); + Assert.False(state.BeginEdit(1, IdCol)); + + Assert.Equal(0, log.Parks); + } + + [Fact] + public async Task RowModeTab_DoesNotPark() + { + var state = await LoadedState(); + var el = Grid(EditMode.Row); + Assert.True(state.BeginRowEdit(1)); + var log = new TransitionLog(); + log.Attach(state); + + // Native Tab has already moved focus by the time the grid handles it, and the one-shot + // SuppressNextLostFocusCommit claim owns that focus-out. Parking here would add a focus + // hop and a second LostFocus to a path whose ordering the #976/#987 tests pin down. + DataGridComponent.HandleKeyDownForTests(state, el, KeyChord.Unmodified(VirtualKey.Tab)); + Assert.True(state.FocusNextRowEditColumn()); + + Assert.Equal(0, log.Parks); + Assert.True(state.IsRowEditing); // positive control: the Tabs really ran inside the edit + } + + [Fact] + public async Task ACrossRowPointerPress_ParksOnceTheCommitHasEndedTheEdit() + { + var state = await LoadedState(); + Assert.True(state.BeginEdit(1, ScoreCol)); + var log = new TransitionLog(); + log.Attach(state); + + state.InvokeRowPointerClick(Row(0), ctrlKey: false, shiftKey: false); + + // A press on ANOTHER row commits the in-flight edit before the cell's Tapped (on release) + // opens the next editor, so on a cross-row tap it is this commit that dooms the focused + // editor. This is the path the CI diagnostics showed for the cell-mode failures. The park + // follows the commit's state change and sees the edit already ended. + Assert.Equal("park(editing=False,row=False,col=-)", log.Events[log.Events.IndexOf("changed") + 1]); + Assert.Equal(1, log.Parks); + Assert.False(state.IsEditing); // positive control: the press really committed the edit + } + + // A commit that validation refuses leaves the edit open with its editor mounted, so nothing + // may move focus off that editor: the user has to be able to keep typing to fix the value. + + private static readonly FieldDescriptor[] ColumnsWithRequiredNotes = + Columns.Select(c => c.Name == "Notes" ? c with { Validators = [Validate.Required()] } : c).ToArray(); + + private static async Task> LoadedStateWithRequiredNotes() + { + var state = new DataGridState(new TestDataSource(), ColumnsWithRequiredNotes, SelectionMode.None); + await state.LoadDataAsync(); + return state; + } + + [Fact] + public async Task ACrossRowPressWhoseCommitValidationRefuses_DoesNotPark() + { + var state = await LoadedStateWithRequiredNotes(); + Assert.True(state.BeginEdit(1, NotesCol)); + state.UpdateEditingValue(""); + Assert.True(state.HasValidationErrors); // positive control: the commit WILL be refused + var log = new TransitionLog(); + log.Attach(state); + + state.InvokeRowPointerClick(Row(0), ctrlKey: false, shiftKey: false); + + Assert.Equal(0, log.Parks); + Assert.True(state.IsEditing); + Assert.Equal("Notes", state.EditingColumnName); + Assert.Equal(Row(1), state.EditingRowKey); + } + + [Fact] + public async Task ARowModeCrossRowPressWhoseCommitValidationRefuses_DoesNotPark() + { + var state = await LoadedStateWithRequiredNotes(); + Assert.True(state.BeginRowEdit(1)); + state.UpdateRowEditValue("Notes", ""); + Assert.True(state.HasValidationErrors); // positive control: the commit WILL be refused + var log = new TransitionLog(); + log.Attach(state); + + state.InvokeRowPointerClick(Row(0), ctrlKey: false, shiftKey: false); + + Assert.Equal(0, log.Parks); + Assert.True(state.IsRowEditing); + Assert.Equal(Row(1), state.EditingRowKey); + } + + [Fact] + public async Task ATapWhoseCommitValidationRefuses_ParksOnlyWhenANewEditorOpens() + { + var state = await LoadedStateWithRequiredNotes(); + Assert.True(state.BeginEdit(1, NotesCol)); + state.UpdateEditingValue(""); + Assert.True(state.HasValidationErrors); // positive control: the commit WILL be refused + var log = new TransitionLog(); + log.Attach(state); + + // A tap on a read-only cell opens nothing, so the refused edit keeps its editor — and focus. + state.InvokeCellEditClick(Row(0), "Id"); + Assert.Equal(0, log.Parks); + Assert.Equal("Notes", state.EditingColumnName); + + // A tap on an editable cell replaces that editor (BeginEdit discards the refused value, as + // it always has), so it parks exactly once — from BeginEdit, with the refused edit still + // open — and never on behalf of the refused commit. + state.InvokeCellEditClick(Row(0), "Score"); + Assert.Equal(1, log.Parks); + Assert.Equal("park(editing=True,row=False,col=Notes)", log.Events.Single(e => e.StartsWith("park", StringComparison.Ordinal))); + Assert.Equal("Score", state.EditingColumnName); + Assert.Equal(Row(0), state.EditingRowKey); + } + + [Fact] + public async Task APointerPressOnTheEditingRow_DoesNotPark() + { + var state = await LoadedState(); + Assert.True(state.BeginEdit(1, ScoreCol)); + var log = new TransitionLog(); + log.Attach(state); + + // Same row: that press is the user positioning the caret. Nothing commits and nothing is + // destroyed, so parking would only pull focus out of the editor mid-click. + state.InvokeRowPointerClick(Row(1), ctrlKey: false, shiftKey: false); + + Assert.Equal(0, log.Parks); + Assert.True(state.IsEditing); + } + + [Fact] + public async Task APointerPressWithNothingInFlight_DoesNotPark() + { + var state = await LoadedState(); + var log = new TransitionLog(); + log.Attach(state); + + state.InvokeRowPointerClick(Row(0), ctrlKey: false, shiftKey: false); + + Assert.Equal(0, log.Parks); + } + + [Fact] + public async Task WithoutARenderer_BeginningAnEditStillWorks() + { + var state = await LoadedState(); + + // The hook is null for a state driven headlessly; every call site must tolerate that. + Assert.Null(state.BeforeEditTransition); + Assert.True(state.BeginRowEdit(1)); + state.CancelRowEdit(); + Assert.True(state.BeginEdit(1, ScoreCol)); + state.InvokeCellEditClick(Row(0), "Notes"); + Assert.Equal("Notes", state.EditingColumnName); + state.InvokeRowPointerClick(Row(2), ctrlKey: false, shiftKey: false); + Assert.False(state.IsEditing); + } + + // ── Which pointer releases the grid root claims (#1288) ── + // + // The root claims a release only to stop an ancestor ScrollViewer from taking focus, and a + // ScrollViewer does that only after a primary-action press: a left mouse button, a touch + // contact, or a pen tip. The PointerUpdateKind a touch or pen release reports is not + // documented, so every release counts except an explicit release of another button. A guard + // narrowed back to LeftButtonReleased would fail the Other case; one that claimed everything + // would fail the rest. + + [Theory] + [InlineData(global::Microsoft.UI.Input.PointerUpdateKind.LeftButtonReleased, true)] + [InlineData(global::Microsoft.UI.Input.PointerUpdateKind.Other, true)] + [InlineData(global::Microsoft.UI.Input.PointerUpdateKind.RightButtonReleased, false)] + [InlineData(global::Microsoft.UI.Input.PointerUpdateKind.MiddleButtonReleased, false)] + [InlineData(global::Microsoft.UI.Input.PointerUpdateKind.XButton1Released, false)] + [InlineData(global::Microsoft.UI.Input.PointerUpdateKind.XButton2Released, false)] + public void TheRootClaimsEveryReleaseExceptOneOfAnotherButton( + global::Microsoft.UI.Input.PointerUpdateKind kind, bool claims) + => Assert.Equal(claims, DataGridComponent.IsPrimaryActionRelease(kind)); }