[Windows] Fix modal page keyboard focus not shifting to newly opened modal

[jfversluis]

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Description

Fixes #22938 - Modal page keyboard focus not shifting on Windows

When a modal page is pushed via Navigation.PushModalAsync on Windows, keyboard focus stays on the underlying page. This causes:

1. Enter key activates buttons beneath the modal — a serious UX and accessibility bug
2. Tab navigation cycles through ALL stacked pages — not just the topmost modal

Root Cause

WindowRootViewContainer (the WinUI Panel that stacks pages) had zero focus management. Pages were layered by z-order but keyboard focus was completely unmanaged.

Fix

Three coordinated changes in WindowRootViewContainer:

1. IsHitTestVisible toggling — When pushing a modal, sets IsHitTestVisible = false on the underlying page to block pointer/touch input. Restores on pop.

2. GettingFocus event trap — Registers a GettingFocus handler (with handledEventsToo=true) on the container that intercepts any focus attempt leaving the modal page and redirects it back. This mirrors how WinUI3's ContentDialog implements modal focus trapping.

3. Focus restoration — SetFocusToFirstElement moves focus to the first focusable element in the new top page, with a DispatcherQueue fallback for cases where the visual tree isn't ready.

Critical ordering fix: State (_topPage, focus trap) is updated BEFORE CachedChildren.Remove() because WinUI3 fires GettingFocus synchronously when removing a focused element. Without this, the handler would see stale state pointing to the removed modal.

Testing

• Device test: ModalPageDisablesHitTestOnUnderlyingPage — verifies IsHitTestVisible toggling
• Device test: ModalPageFocusTrapsAndRestoresCorrectly — verifies full push/pop cycle
• UI test: Issue22938 — End-to-end test with semi-transparent modal
• Unit tests: 69/69 existing modal tests pass
• Manual testing: Verified with MauiApp1 reproduction app (3 push/pop cycles, buttons functional after each pop)

[Copilot]

Pull request overview

Fixes a Windows-specific modal navigation issue where keyboard focus remains on the underlying page after PushModalAsync, allowing Enter/Tab to interact with controls behind the modal.

Changes:

• Adds focus management and hit-testing changes in the Windows WindowRootViewContainer when pushing/popping pages.
• Adds a new UI test + HostApp issue page for #22938.
• Adds a Windows device test asserting IsHitTestVisible toggling for underlying vs modal pages.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
src/Core/src/Platform/Windows/WindowRootViewContainer.cs	Disables hit testing on the covered page and attempts to programmatically move focus to the newly added top page.
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue22938.cs	Adds an Appium/NUnit UI regression test around Enter key behavior with modals.
src/Controls/tests/TestCases.HostApp/Issues/Issue22938.cs	Adds the HostApp reproduction page used by the new UI test.
src/Controls/tests/DeviceTests/Elements/Modal/ModalTests.Windows.cs	Adds a Windows device test asserting IsHitTestVisible behavior during modal push/pop.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[kubaflo]

Detailed Code Review

✅ Architecture & Approach — Excellent

The three-layered defense in WindowRootViewContainer is well-designed:

1. IsHitTestVisible toggling — blocks pointer/touch to underlying pages
2. GettingFocus event trap — intercepts keyboard focus escaping the modal (mirrors WinUI3 ContentDialog)
3. TabFocusNavigation = Cycle — constrains Tab cycling to the modal

The state-save/restore pattern using _originalIsHitTestVisible and _originalTabNavigation dictionaries is clean. The critical ordering fix (updating _topPage BEFORE CachedChildren.Remove()) correctly handles WinUI3's synchronous GettingFocus firing during element removal.

I traced through all code paths (push/pop with transparent vs. opaque backgrounds, nested modals, NavigationRootManager.Connect reconnections) and the logic is correct in all scenarios.

---

🔴 Issues Found

1. UI Test May Fail on Non-Windows Platforms (Medium-High)

The Issue22938 UI test calls App.SendTabKey() and App.PressEnter() without any platform guard, and the HostApp page uses PlatformAffected.All. These keyboard-focused tests are unreliable on iOS/Android where hardware keyboard focus semantics differ. The Tab cycling test especially — 10 Tab presses on mobile could cause flaky results.

Recommendation: Change PlatformAffected.All → PlatformAffected.Windows in the HostApp page, since this is fundamentally a Windows keyboard focus bug.

2. TrySetNewFocusedElement Return Value Unchecked (Low)

In OnContainerGettingFocus, if TrySetNewFocusedElement returns false, focus could momentarily escape the modal. Consider:

if (!args.TrySetNewFocusedElement(firstFocusable))
    args.TryCancel();

---

🟡 Suggestions (Non-Blocking)

3. Device Test Coverage Gap: Default Background Modal

All tests use colored/semi-transparent backgrounds. ModalNavigationManager.Windows.cs has a distinct code path where modals with default backgrounds cause RemovePage on the underlying page before AddPage for the modal. A test case with a default-background modal would cover this path.

4. RemovePage Type Check Assumption

The child is not WindowRootView filter in RemovePage works because all AddPage callers pass windowManager.RootView (which is WindowRootView). A brief comment documenting this assumption would help future maintainers.

---

✅ Test Quality — Good

Test	Verdict
ModalPageDisablesHitTestOnUnderlyingPage	✅ Thorough (covers BackgroundColor vs Brush)
ModalPageFocusTrapsAndRestoresCorrectly	✅ Good (full push/pop + container state)
NestedModalPagesMaintainHitTestVisibilityAndFocusTrap	✅ Excellent (2-deep stack, correct unwinding)
Issue22938 UI tests	🟡 Good but needs Windows platform scoping

---

📝 Summary

Aspect	Rating
Correctness	✅ Sound — all push/pop/nested flows verified
Design	✅ Mirrors WinUI3 ContentDialog pattern
State management	✅ Save/restore dictionaries properly managed
Tests	🟡 Good coverage, UI test needs platform scoping
Risk	Low — Windows-only, well-contained

Overall this is a well-implemented fix. Main actionable item is scoping the UI test to Windows-only to prevent potential flakiness on mobile platforms.