Fix DataGrid committing an edit the moment it opens (#1288)

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

src/Reactor.Advanced/Controls/DataGrid/DataGridComponent.cs

@@ -43,6 +43,71 @@ public class DataGridComponent<[DynamicallyAccessedMembers(DynamicallyAccessedMe
     private static readonly Action<object, Microsoft.UI.Xaml.Input.TappedRoutedEventArgs> StableNoopTap = (_, _) => { };
     private static readonly Action<object, Microsoft.UI.Xaml.Input.PointerRoutedEventArgs> StableNoopPointer = (_, _) => { };
 
+    /// <summary>
+    /// The grid root's <c>PointerReleased</c> 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.
+    /// </summary>
+    /// <remarks>
+    /// <para>WinUI's <c>ScrollViewer</c> takes focus on an unhandled release of a press whose
+    /// <c>IsLeftButtonPressed</c> was true, unless it is not a tab stop AND focus is already inside
+    /// it (<c>ScrollViewer::OnPointerReleased</c> 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.</para>
+    ///
+    /// <para>Focus is on the root when the user tabbed onto the grid, or when
+    /// <see cref="DataGridState{T}.BeforeEditTransition"/> 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.</para>
+    ///
+    /// <para>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 <c>Tapped</c> still
+    /// fires, which <c>Interactive_DataGrid_TapWhileGridRootHasFocus_OpensTheTappedEditor</c>
+    /// checks. One consequence app code can see: a <c>PointerReleased</c> 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 <see cref="IsPrimaryActionRelease"/>'s call.</para>
+    /// </remarks>
+    private static readonly Action<object, Microsoft.UI.Xaml.Input.PointerRoutedEventArgs> HoldRootFocusThroughRelease =
+        (sender, e) =>
+        {
+            if (!e.Handled
+                && sender is FrameworkElement root
+                && IsPrimaryActionRelease(e.GetCurrentPoint(root).Properties.PointerUpdateKind)
+                && IsFocusOnRoot(root))
+            {
+                e.Handled = true;
+            }
+        };
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <remarks>
+    /// <para>A touch contact and a pen tip are the pointer's first button. Win32 sets
+    /// <c>POINTER_FLAG_FIRSTBUTTON</c> while a touch pointer is in contact, and a touch pointer
+    /// uses no other button; WinRT's <c>IsLeftButtonPressed</c> documents both as the primary
+    /// action mode. Which <c>PointerUpdateKind</c> 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 <c>Other</c>, a release that names no button.</para>
+    ///
+    /// <para>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.</para>
+    /// </remarks>
+    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<Action<global::Microsoft.UI.Xaml.Controls.Grid>?>(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<FrameworkElement?>(null);
+        var parkFocusHook = UseRef<Action?>(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;
     }
 
+    /// <summary>
+    /// 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
+    /// <see cref="DataGridState{T}.BeforeEditTransition"/>, which explains why (#1288).
+    /// </summary>
+    /// <remarks>
+    /// <para>The root is the right place to park: it is a tab stop (<c>IsTabStop(true)</c>), 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 <see cref="HoldRootFocusThroughRelease"/> an ancestor ScrollViewer would
+    /// take focus on that release.</para>
+    ///
+    /// <para>Leaves focus alone in every other case, each for a reason:</para>
+    /// <list type="bullet">
+    /// <item><description>Focus already on the root — nothing the render removes holds it.</description></item>
+    /// <item><description>Focus outside the grid — a programmatic <c>BeginEdit()</c> from a toolbar
+    /// must not steal focus into the grid (the editor's own request decides that, as before).</description></item>
+    /// <item><description>No focus, or no live root — nothing to protect.</description></item>
+    /// </list>
+    ///
+    /// <para>Programmatic focus state, so parking shows no focus visual on the root.</para>
+    /// </remarks>
+    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;
+    }
+
+    /// <summary>
+    /// Whether keyboard focus is on <paramref name="root"/> itself — not a descendant, not
+    /// elsewhere. The condition under which <see cref="HoldRootFocusThroughRelease"/> 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.
+    /// </summary>
+    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(

src/Reactor.Advanced/Controls/DataGrid/DataGridState.cs

@@ -139,6 +139,44 @@ public class DataGridState<T>
     /// </summary>
     internal bool SuppressNextLostFocusCommit { get; set; }
 
+    /// <summary>
+    /// 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
+    /// (<see cref="BeginEdit(int, int)"/>, <see cref="BeginRowEdit"/>), 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. <see cref="DataGridComponent{T}"/>
+    /// 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.
+    /// </summary>
+    /// <remarks>
+    /// <para>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 <c>onRowChanged</c>, and an editor that closes as it
+    /// appears.</para>
+    ///
+    /// <para>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.</para>
+    ///
+    /// <para>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.</para>
+    ///
+    /// <para>Deliberately NOT invoked for row-mode Tab traversal, which does not destroy the
+    /// focused editor: native Tab has already moved focus, and the
+    /// <see cref="SuppressNextLostFocusCommit"/> claim owns that focus-out.</para>
+    /// </remarks>
+    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;