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

[Alexandre Zollinger Chohfi (azchohfi)]

What was wrong

#1288 looked like an E2E flake, but it is a DataGrid bug: opening an editor from inside the grid could commit it the moment it opened. The editor closed as it appeared, and onRowChanged fired with unchanged values.

Mechanism

Starting a row edit from the row's Edit button, or a commit-then-begin cell tap, removes the element holding keyboard focus (the Edit button, or the previous cell's editor) in the re-render that opens the new editor. XAML then moves focus to the next tab stop, which is outside the grid, because the grid is a single tab stop. The grid's LostFocus blur-commit net schedules a deferred check. That check races the new editor's deferred focus request, and whenever it runs first, it finds focus outside the grid and commits the edit that was just opened.

The focus jump happens on every such click on stock code; only the commit depends on timing. That is why fast machines pass and loaded CI runners fail.

Fix

1. Park focus before the element is removed. DataGridState.BeforeEditTransition is wired to ParkFocusOnGridRoot. When focus is on one of the grid's descendants, the park moves it onto the grid root, a tab stop that survives renders. It runs at two points:

   • in BeginEdit / BeginRowEdit, before the state changes;
   • on the commit-then-begin click paths, right after a commit that actually ended the edit.

   The render that removes the element is always a later dispatcher tick, so nothing that still holds focus is ever removed. The park never runs when:

   • validation refuses the commit, because that editor stays open and keeps focus (found by Copilot review);
   • a begin is refused;
   • the user presses Tab in row mode;
   • focus is outside the grid.

2. Keep focus on the root through a pointer release. WinUI's ScrollViewer takes focus on an unhandled left-button release unless it is not a tab stop and focus is already inside it (ScrollViewer::OnPointerReleased). The grid's data-area scroller keeps focus that sits in an editor, but it cannot keep focus on the root, which is its ancestor. The release therefore reached the window's root ScrollViewer, which is a tab stop (a temporary selftest diagnostic found it at the top of the visual tree with IsTabStop=True), and it took focus. A cross-row press parks focus from a deferred callback, so when the button came up after that callback, the steal happened while the tap's editor was opening. The root now claims an unhandled release that can complete a primary-action press (a left mouse button, a touch contact, or a pen tip) while keyboard focus is on the root itself, which is what the data-area scroller already does for focus inside it. The PointerUpdateKind a touch or pen release reports is not documented, so only explicit right, middle, and X-button releases are left unclaimed. This path was found by the A/B below: 1 of 120 cross-row taps.

Evidence

CI A/B, ci-stress, target: e2e, e2e_retries: 0. A temporary focus probe recorded every focus move in every iteration, pass or fail. Real mouse input throughout.

Stock vs fixed, 60 iterations each	stock (36465397744)	fixed (36465403348)
Row-edit setup: focus left the grid during the Edit click	141/141 attempts (120/120 first attempts)	0/120
DoesNotSuppressNextCommit, first attempt failed	8/60	0/60
WrapsToFirstEditorWithoutCommitting, first attempt failed	3/60	0/60
Either row-edit test failed outright (all 4 setup attempts)	3/120	0/120
ClickEditTabCommit failed	2/60	1/60, the release path fixed in (2)

Release handler, 30 iterations each	park only (36474980032)	park + handler (36474984469)
New PressHeldOnAnotherRow_KeepsKeyboardFocusInGrid	failed 30/30 (Grid > ScrollViewer on the release)	passed 30/30, focus left the grid 0/30
Whole E2E suite	that test 30/30, plus 1 DragDrop_TypedReorder_MovesCard (a known unrelated flake, also seen on stock)	green in all 30 iterations

Earlier runs agree. Stock 36207044352 and 36211748809, against the fixed 36213516293 and 36214911933, showed:

• DoesNotSuppressNextCommit first attempts: 21/120 failed on stock vs 0/120 fixed;
• ClickEditTabCommit: 9/120 vs 0/120.

Deterministic, in-process. The selftests record every focus move across the transition (FocusNeverLeftTheGrid):

• Park removed: they failed on every run measured. Focus jumps to an outside anchor, and in cell mode the spurious commit itself appeared in 4 of 7 local runs.
• Fixed: every move stays inside the grid.

Does the release handler break click-to-edit? Claiming a release must not stop the cell's Tapped, or clicking a cell would stop opening its editor whenever the grid root has focus. The recorded focus data says it does not. In all 180 recorded ClickEditTabCommit runs, the first tap's release was taken and marked handled by the window's root ScrollViewer, and the tapped cell's editor still opened. The exact case, a tap on an editable cell whose release the grid root claims, is rare and invisible to the probe. The new TapWhileGridRootHasFocus_OpensTheTappedEditor test pins it deterministically, and it passes in CI.

Tests

• Unit: 13 tests pin when the hook runs, including the validation-refused cases in both edit modes. A 6-case theory pins which release kinds the root claims.
• Selftests: two fixtures read focus at the discriminating instant, record the whole transition, check that a refused commit keeps focus in its editor, and pin IsFocusOnRoot.
• E2E:
  • PressHeldOnAnotherRow_KeepsKeyboardFocusInGrid holds a press so the deferred park always runs before the release.
  • TapWhileGridRootHasFocus_OpensTheTappedEditor checks that a tap whose release the root claims still opens the tapped cell's editor. It guards the release handler; it also passes without the handler, so it is not a Flaky E2E: Interactive_DataGrid_RowEditTab_DoesNotSuppressNextCommit fails ~17% of runs ("no 'Save' button appeared") #1288 detector.
  • The row-edit setup helper retries the whole setup, re-navigating each time, and names the failure shape of each attempt. It is not what guards Flaky E2E: Interactive_DataGrid_RowEditTab_DoesNotSuppressNextCommit fails ~17% of runs ("no 'Save' button appeared") #1288; the selftests do.
• Mutation checks (each reddens the tests above):
  • park no-oped;
  • park run before the commit;
  • park run unconditionally after the commit;
  • BeginRowEdit no-oped;
  • release handler removed;
  • release guard narrowed back to LeftButtonReleased, or widened to every release.

Locally: 14,289 unit tests, all 211 DataGrid selftests, and the Release builds of the changed projects are green.

Review

• pr-review skill, 8 dimensions plus a GPT cross-check. Two medium findings, both fixed:
  • a changelog sentence described a public API that does not exist;
  • an E2E remark overstated what its retry can heal.
• Copilot review:
  • Round 1 found the validation-refused case, fixed with a red-to-green selftest.
  • Round 2, on 795fdc422, had no findings.
  • Rounds 3 and 4, on the commits adding the release handler and its guard test, were requested but never returned: the reviewer produced no reviews anywhere in this repo for about two and a half hours (Shard the selftest, E2E and coverage CI jobs across runners #1293 was stuck the same way). A GPT cross-check covered those commits instead and found no critical or high issues.
  • The branch was then squashed into one commit whose tree was identical to the last multi-commit head, cef95e71b, so the SHAs above predate the squash.
  • Round 5, on the squashed head, flagged that the release guard recognized only LeftButtonReleased and said touch reports Other. The Win32 and WinRT docs point the other way: a touch contact is the first button, and IsLeftButtonPressed documents it as the primary action. But the reported kind is not documented, so the guard now claims every release except an explicit release of another button, and a theory pins that.
  • Round 6, on the final head, converged: "Approval recommended", no findings, and no unresolved threads.

CI:

• All checks pass on the final head, e3c86aa69. Its E2E job ran all 213 tests; the only 2 skips are the Spec051 devtools tests, which are inconclusive by design.
• On the earlier squashed head, only Packaged Selftests (MSIX) failed, in LT_OnMountUnmountBalanced. That is the known flake [Flaky] LT_Unmounts_Exactly_30 / LT_NoLeak_MountsEqualUnmounts — real unmount-accounting deficit, not the #988 watchdog #994, in a lifecycle fixture this PR does not touch, and it passed on re-run.

Follow-ups (not fixed here)

• Enter/Escape end an edit and remove the focused editor without a park, so keyboard focus lands outside the grid afterwards. No edit is open at that point, so this cannot cause Flaky E2E: Interactive_DataGrid_RowEditTab_DoesNotSuppressNextCommit fails ~17% of runs ("no 'Save' button appeared") #1288, but it is a keyboard-navigation gap.
• A click on a read-only cell while focus is outside the grid gives focus to the window's root ScrollViewer, not the grid, so arrow keys do not reach it.
• The grid's LostFocus handler is not re-wired when ElementPool recycles a grid root (KeyDown is; see WireGridKeyDown), so stale handlers accumulate. They are harmless unless a grid is unmounted mid-edit.
• The issue's "~17%" also counted an unrelated WinFormsInteropTests flake (foreground_not_target).

Fixes #1288

[github-actions]

📦 Build metrics

Artifact sizes for e3c86aa vs the base branch (e5a3edc).

Packages (compressed .nupkg)

Artifact	base	PR	Δ
Microsoft.UI.Reactor.nupkg	1.75 MB	1.75 MB	-8 B (0.00%)	≈
Microsoft.UI.Reactor.Advanced.nupkg	487.6 KB	490.8 KB	+3.24 KB (+0.67%)	⚠️
Microsoft.UI.Reactor.Devtools.nupkg	284.0 KB	284.0 KB	+0 B (0.00%)	≈

Assemblies in Microsoft.UI.Reactor

Artifact	base	PR	Δ
Reactor.Analyzers.dll	373.0 KB	373.0 KB	+0 B (0.00%)	≈
Reactor.dll	2.47 MB	2.47 MB	+0 B (0.00%)	≈
Reactor.Localization.Generator.dll	16.0 KB	16.0 KB	+0 B (0.00%)	≈
Reactor.Wrappers.Abstractions.dll	10.5 KB	10.5 KB	+0 B (0.00%)	≈
Reactor.Wrappers.Generator.dll	99.5 KB	99.5 KB	+0 B (0.00%)	≈

Assemblies in Microsoft.UI.Reactor.Advanced

Artifact	base	PR	Δ
Reactor.Advanced.dll	1021.5 KB	1023.0 KB	+1.50 KB (+0.15%)	⚠️

Assemblies in Microsoft.UI.Reactor.Devtools

Artifact	base	PR	Δ
Microsoft.UI.Reactor.Devtools.dll	784.5 KB	784.5 KB	+0 B (0.00%)	≈

_{✅ smaller / ⚠️ larger / ≈ within noise. Sizes come from a Release dotnet pack on the CI runner: packages are the compressed .nupkg download size, assemblies the uncompressed DLL inside it.
workflow run.}

[github-actions]

🧪 Merged coverage

Coverage for e3c86aa vs the base branch (e5a3edc) — unit + selftest merged.

Metric	base	PR	Δ
Line	85.97%	85.99%	+0.02 pp	≈
Branch	77.32% (965/1248)	77.32% (965/1248)	0.00 pp	≈

No coverage change beyond the noise floor. ✅

_{✅ higher / ⚠️ lower / ≈ within noise. Δ is in percentage points; coverage is unit + selftest merged (Debug x64) on the CI runner. Cobertura reports attached to the workflow run as artifacts.}

[Copilot]

Copilot review overview

🔵 Needs a closer look

Focus can be parked even when validation refuses a commit, leaving the active editor unfocused.

Review effort: Lite
Findings: None

What changed in this PR

Fixes a DataGrid focus race that could immediately commit newly opened editors.

Changes:

• Parks focus on the grid root before destructive edit transitions.
• Adds unit, selftest, and hardened E2E regression coverage.
• Documents the fix in the changelog.

File	Description
tests/​Reactor.Tests/​DataGridEditorFocusTests.cs	Adds transition-order unit tests.
tests/​Reactor.AppTests/​Tests/​DataGridTests.cs	Hardens E2E setup and diagnostics.
tests/​Reactor.AppTests.Host/​SelfTest/​SelfTestFixtureRegistry.cs	Registers new fixtures.
tests/​Reactor.AppTests.Host/​SelfTest/​Fixtures/​DataGridEditFixtures.cs	Adds live focus regression fixtures.
src/​Reactor.Advanced/​Controls/​DataGrid/​DataGridState.cs	Adds pre-transition hook calls.
src/​Reactor.Advanced/​Controls/​DataGrid/​DataGridComponent.cs	Implements grid-root focus parking.
CHANGELOG.md	Documents the fix.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The focus-management changes span production logic and multiple UI test layers, warranting final human review.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🟡 Changes recommended

Touch releases are not covered by the focus-preservation guard and require implementation and test coverage.

Review effort: Lite
Findings: 1

Open (1)

• Handle touch releases to preserve focus from ancestor ScrollViewer · New

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Handle touch releases to preserve focus from ancestor ScrollViewer

[Copilot]

Copilot review overview

🟢 Approval recommended

The focus fix is narrowly scoped and comprehensively covered across unit, selftest, and E2E tiers.

Review effort: Balanced
Findings: None