Skip to content

[Windows] Border: Keyboard tap support for TapGestureRecognizer#35578

Open
Vignesh-SF3580 wants to merge 3 commits into
dotnet:net11.0from
Vignesh-SF3580:fix-27627net11
Open

[Windows] Border: Keyboard tap support for TapGestureRecognizer#35578
Vignesh-SF3580 wants to merge 3 commits into
dotnet:net11.0from
Vignesh-SF3580:fix-27627net11

Conversation

@Vignesh-SF3580

Copy link
Copy Markdown
Contributor

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 of Change

This PR is the second half of the split from #27713. It adds keyboard interaction support for Border on Windows, allowing users to activate a Border with a TapGestureRecognizer using the keyboard.

A Border with a single-tap, primary-button TapGestureRecognizer is now keyboard actionable on Windows:

  • ContentPanel.IsTabStop is set to true, allowing the Border to participate in tab navigation.
  • Pressing Enter or Space while the Border has focus invokes the attached TapGestureRecognizer through SendTapped, using Point.Zero as the tap position, matching Android’s keyboard-tap behavior.

The tab-stop state stays synchronized with the gesture collection:

  • Adding a qualifying TapGestureRecognizer enables tab-stop behavior and subscribes to KeyDown.
  • Removing all qualifying recognizers, or clearing handlers, disables tab-stop behavior and unsubscribes from KeyDown.

Only gestures attached directly to the Border make it keyboard actionable. Gestures attached to child elements do not promote the parent Border into a tab stop, since child elements manage their own keyboard focus independently.

Internal bookkeeping updates:

  • SubscriptionFlags was widened from byte to ushort.
  • Added a new ContentPanelKeyDownSubscribed flag to track the KeyDown subscription lifecycle and prevent duplicate event subscriptions or removals.

How it works

On Windows, a MAUI Border is rendered by a native ContentPanel that, by default, does not participate in keyboard navigation or connect KeyDown events to MAUI gestures. Because of this, a Border with a TapGestureRecognizer was clickable but not keyboard accessible. This PR adds that missing behavior in GesturePlatformManager.Windows.

  • When to enable: UpdatingGestureRecognizers now sets hasSelfTapGestures = true only when the Border itself has a single-tap TapGestureRecognizer with a Primary button mask. Gestures on child elements do not promote the parent Border to a tab stop.
  • Toggling tab-stop and KeyDown: UpdateContentPanelIsTapStop(bool) manages both ContentPanel.IsTabStop and the ContentPanelOnKeyDown subscription, guarded by CrossPlatformLayout is IBorderView. A new SubscriptionFlags.ContentPanelKeyDownSubscribed flag prevents duplicate attach/detach, and the enum was widened from byte to ushort to support the new flag. ClearContainerEventHandlers calls the helper with false so teardown resets the state.
  • Handling the key: ContentPanelOnKeyDown listens for Enter and Space, retrieves matching TapGestureRecognizers from the View, calls SendTapped(view, _ => Point.Zero), and sets e.Handled = true if any recognizer is triggered. Point.Zero is used intentionally because keyboard taps do not have a physical pointer location, matching Android’s keyboard-tap convention.

Note:

AutomationPeer support for Border was submitted separately in #35577 against main. Once that change flows into net11.0, the keyboard-actionable Border introduced here will also be properly exposed to UI Automation, completing the end-to-end accessibility support for Border on Windows.

Issues Fixed

Fixes #27627

@github-actions

github-actions Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35578

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35578"

@dotnet-policy-service dotnet-policy-service Bot added the partner/syncfusion Issues / PR's with Syncfusion collaboration label May 22, 2026
@Vignesh-SF3580 Vignesh-SF3580 added the community ✨ Community Contribution label May 22, 2026
@sheiksyedm sheiksyedm marked this pull request as ready for review May 22, 2026 07:28
@kubaflo

kubaflo commented May 22, 2026

Copy link
Copy Markdown
Contributor

/review -b feature/regression-check

@dotnet dotnet deleted a comment from MauiBot May 22, 2026
@kubaflo

kubaflo commented May 22, 2026

Copy link
Copy Markdown
Contributor

🔍 Multimodal Code Review — PR #35578

[Windows] Border: Keyboard tap support for TapGestureRecognizer

Overall this is a well-structured PR that adds keyboard accessibility to Border on Windows. The SubscriptionFlags pattern, IBorderView guard, and ClearContainerEventHandlers integration are all solid. A few items to address before merge:


🐛 Bug: Missing view.IsEnabled check in ContentPanelOnKeyDown

The existing OnTap handler (line ~755) checks if (!view.IsEnabled) { return; } before processing gestures. The new ContentPanelOnKeyDown handler skips this check. A disabled Border would still respond to Enter/Space keyboard activation, which is inconsistent with pointer tap behavior.

Suggested fix: Add the same guard at the top of the key handler:

if (Element is View view)
{
    if (!view.IsEnabled)
        return;
    // ...existing code...
}

⚠️ Behavior Regression: hasSelfTapGestures narrows existing tap subscription condition

The original condition at UpdatingGestureRecognizers was:

gestures.HasAnyGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1)

The PR changes this to:

gestures.HasAnyGesturesFor<TapGestureRecognizer>(g =>
    g.NumberOfTapsRequired == 1 &&
    (g.Buttons & ButtonsMask.Primary) == ButtonsMask.Primary)

This changes the existing tap/right-tap subscription path — not just the new keyboard feature. A Border with only a ButtonsMask.Secondary single-tap TapGestureRecognizer would no longer enter this block, so _container.RightTapped += OnTap would never be subscribed, breaking secondary-only right-click gestures.

Suggested fix: Keep the original condition for tap subscription and use a separate variable for the keyboard-specific check:

bool hasSelfSingleTap = gestures.HasAnyGesturesFor<TapGestureRecognizer>(
    g => g.NumberOfTapsRequired == 1);

bool hasSelfPrimaryTap = gestures.HasAnyGesturesFor<TapGestureRecognizer>(g =>
    g.NumberOfTapsRequired == 1 &&
    (g.Buttons & ButtonsMask.Primary) == ButtonsMask.Primary);

if (hasSelfSingleTap || children?.GetChildGesturesFor<TapGestureRecognizer>(...).Any() == true)
{
    // ...existing tap subscription code...
    UpdateContentPanelIsTapStop(hasSelfPrimaryTap);
}

🔧 Minor: Missing KeyDown cleanup in Control property setter

The Control setter (line ~105) unsubscribes Tapped and DoubleTapped from the old _control before reassignment, but does not unsubscribe ContentPanelOnKeyDown. If _control were ever reassigned for a Border, the old ContentPanel would keep a dangling KeyDown handler. While unlikely in practice (ContentPanel typically does not change), adding UpdateContentPanelIsTapStop(false) before _control = value would maintain consistency with the existing cleanup pattern.


📝 Test Coverage Suggestions

The current test (ContentPanelIsTabStopReflectsTapGestureRecognizer) validates the initial IsTabStop state. Consider adding:

  • Dynamic toggle test: Add a TapGestureRecognizer, verify IsTabStop=true, remove it, verify IsTabStop=false.
  • Disabled border test: Verify keyboard tap does NOT fire when Border.IsEnabled = false (once the bug above is fixed).
  • Secondary-only gesture test: Verify that TapGestureRecognizer { Buttons = ButtonsMask.Secondary } does NOT set IsTabStop (documents the intended design).

✅ What looks good

  • SubscriptionFlags widened from byteushort cleanly accommodates the new flag at bit 8
  • UpdateContentPanelIsTapStop follows the flag-guarded subscribe/unsubscribe pattern used throughout the class
  • Point.Zero for keyboard taps is correct and documented as consistent with Android
  • Cleanup integrated into both ClearContainerEventHandlers and Dispose paths
  • IBorderView guard correctly scopes the feature to Border only

@kubaflo

kubaflo commented May 22, 2026

Copy link
Copy Markdown
Contributor

/review

@dotnet dotnet deleted a comment from MauiBot May 23, 2026
@kubaflo

kubaflo commented May 24, 2026

Copy link
Copy Markdown
Contributor

/review -b feature/refactor-copilot-yml

@Vignesh-SF3580

Copy link
Copy Markdown
Contributor Author

🔍 Multimodal Code Review — PR #35578

[Windows] Border: Keyboard tap support for TapGestureRecognizer

Overall this is a well-structured PR that adds keyboard accessibility to Border on Windows. The SubscriptionFlags pattern, IBorderView guard, and ClearContainerEventHandlers integration are all solid. A few items to address before merge:

🐛 Bug: Missing view.IsEnabled check in ContentPanelOnKeyDown

The existing OnTap handler (line ~755) checks if (!view.IsEnabled) { return; } before processing gestures. The new ContentPanelOnKeyDown handler skips this check. A disabled Border would still respond to Enter/Space keyboard activation, which is inconsistent with pointer tap behavior.

Suggested fix: Add the same guard at the top of the key handler:

if (Element is View view)
{
    if (!view.IsEnabled)
        return;
    // ...existing code...
}

⚠️ Behavior Regression: hasSelfTapGestures narrows existing tap subscription condition

The original condition at UpdatingGestureRecognizers was:

gestures.HasAnyGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1)

The PR changes this to:

gestures.HasAnyGesturesFor<TapGestureRecognizer>(g =>
    g.NumberOfTapsRequired == 1 &&
    (g.Buttons & ButtonsMask.Primary) == ButtonsMask.Primary)

This changes the existing tap/right-tap subscription path — not just the new keyboard feature. A Border with only a ButtonsMask.Secondary single-tap TapGestureRecognizer would no longer enter this block, so _container.RightTapped += OnTap would never be subscribed, breaking secondary-only right-click gestures.

Suggested fix: Keep the original condition for tap subscription and use a separate variable for the keyboard-specific check:

bool hasSelfSingleTap = gestures.HasAnyGesturesFor<TapGestureRecognizer>(
    g => g.NumberOfTapsRequired == 1);

bool hasSelfPrimaryTap = gestures.HasAnyGesturesFor<TapGestureRecognizer>(g =>
    g.NumberOfTapsRequired == 1 &&
    (g.Buttons & ButtonsMask.Primary) == ButtonsMask.Primary);

if (hasSelfSingleTap || children?.GetChildGesturesFor<TapGestureRecognizer>(...).Any() == true)
{
    // ...existing tap subscription code...
    UpdateContentPanelIsTapStop(hasSelfPrimaryTap);
}

🔧 Minor: Missing KeyDown cleanup in Control property setter

The Control setter (line ~105) unsubscribes Tapped and DoubleTapped from the old _control before reassignment, but does not unsubscribe ContentPanelOnKeyDown. If _control were ever reassigned for a Border, the old ContentPanel would keep a dangling KeyDown handler. While unlikely in practice (ContentPanel typically does not change), adding UpdateContentPanelIsTapStop(false) before _control = value would maintain consistency with the existing cleanup pattern.

📝 Test Coverage Suggestions

The current test (ContentPanelIsTabStopReflectsTapGestureRecognizer) validates the initial IsTabStop state. Consider adding:

  • Dynamic toggle test: Add a TapGestureRecognizer, verify IsTabStop=true, remove it, verify IsTabStop=false.
  • Disabled border test: Verify keyboard tap does NOT fire when Border.IsEnabled = false (once the bug above is fixed).
  • Secondary-only gesture test: Verify that TapGestureRecognizer { Buttons = ButtonsMask.Secondary } does NOT set IsTabStop (documents the intended design).

✅ What looks good

  • SubscriptionFlags widened from byteushort cleanly accommodates the new flag at bit 8
  • UpdateContentPanelIsTapStop follows the flag-guarded subscribe/unsubscribe pattern used throughout the class
  • Point.Zero for keyboard taps is correct and documented as consistent with Android
  • Cleanup integrated into both ClearContainerEventHandlers and Dispose paths
  • IBorderView guard correctly scopes the feature to Border only

@kubaflo I have addressed the suggestions.

  • Added the same view.IsEnabled check in ContentPanelOnKeyDown before processing gesture recognizers. Disabled Borders now ignore keyboard activation as well.
  • Split the logic into two booleans: hasSelfSingleTap is now used for mouse wiring (Tapped + RightTapped) to preserve the original right-click behavior, while hasSelfPrimarySingleTap is used only for keyboard wiring (IsTabStop + KeyDown) so keyboard activation remains limited to Primary taps.
  • Added UpdateContentPanelIsTapStop(false) before _control = value; to properly remove the KeyDown handler and reset IsTabStop on the old ContentPanel.

@kubaflo

kubaflo commented May 26, 2026

Copy link
Copy Markdown
Contributor

/review -b feature/refactor-copilot-yml -p windows

@MauiBot

MauiBot commented May 26, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Merge Conflict Detected — This PR has merge conflicts with its target branch. Please rebase onto the target branch and resolve the conflicts.

@vishnumenon2684

Copy link
Copy Markdown
Contributor

/azp run maui-pr-uitests, maui-pr-devicetests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@kubaflo

kubaflo commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

AI code review refresh for net11.0 target

Head reviewed: 6040501

Verdict: Blocked/limited — I did not find a new code issue in the Windows Border keyboard-tap changes, but the latest explicitly-triggered CI is not green, so I cannot restate LGTM/merge-ready.

Findings

  • Rechecked activity since the prior marker: only /azp run maui-pr-uitests, maui-pr-devicetests and the Azure Pipelines start comment were new; no new commits after the reviewed head.
  • Independently reviewed the diff and full head files for GesturePlatformManager.Windows.cs and BorderTests.Windows.cs. The implementation is scoped to Windows ContentPanel used by IBorderView, subscribes/unsubscribes KeyDown through the existing gesture lifecycle, limits keyboard activation to enabled views with primary-button single-tap recognizers, and clears the handler when gestures/control change.
  • Devil's-advocate pass: I do not see a high-signal lifecycle leak or incorrect child-gesture activation path. The added test verifies the native IsTabStop state; it does not exercise actual Enter/Space dispatch, but that is a coverage limitation rather than a blocking correctness finding from this pass.

CI note

  • gh pr checks currently reports many successes but also failures/cancellations/in-progress checks from the latest reruns.
  • Latest maui-pr-devicetests build 1451063 has an unmatched Windows device-test failure: Controls.DeviceTests-packaged failed before category discovery (devicetestcategories.txt/test results not produced). It also shows unrelated-looking MacCatalyst build analyzer errors in src/Graphics/src/Graphics/Platforms/iOS/UIImageExtensions.cs.
  • Latest maui-pr-uitests build 1451062 also has unmatched Android/macOS UI-test failures and a CollectionView timeout. These do not obviously correlate with this Windows Border diff, but they still need CI owner/maintainer resolution or rerun evidence before calling the PR ready.

Confidence: medium-high on the code review; medium on CI non-correlation because Build Analysis did not match these latest failures to known issues.

Not an approval; this is an automated review refresh comment only.

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: NEEDS_CHANGES (confidence: medium)

Validating against the actual code resolves the split: opus-4.8's bubbled-KeyDown concern is confirmed real — ContentPanelOnKeyDown is attached to the ContentPanel (an ancestor of the Border's content) yet never checks e.OriginalSource/focus, so a focusable child whose Enter/Space isn't consumed (e.g. a single-line Entry on Enter) bubbles up and spuriously fires the Border's tap (then e.Handled = true swallows the key). gpt-5.5's auto-repeat concern is also real and corroborated in-repo: the Windows path fires on auto-repeating KeyDown, whereas the Android OnKeyPress it claims parity with deliberately acts only on KeyEventActions.Up. These are concrete code fixes, not just discussion, so the verdict lands past NEEDS_DISCUSSION/LGTM.

Why the models split: opus-4.6 (LGTM) verified the prior-round fixes landed and pattern-matched the subscription lifecycle, but dismissed the failing Windows device-test leg as infra/"no results" and didn't analyze WinUI routed-event bubbling, so it missed the missing origin guard. gemini (NEEDS_CHANGES, 0 inline) gated correctly on CI but produced no code finding. gpt-5.5 and opus-4.8 each found one of the two real handler gaps but framed them as discussion. Cross-pollination keeps both code findings plus the test-coverage gap.

Key findings (all novel — prior rounds only raised the now-fixed IsEnabled guard):

  • [warning] Missing origin/focus guard in ContentPanelOnKeyDown (GesturePlatformManager.Windows.cs:440): bubbled Enter/Space from a focusable child can spuriously activate the Border's TapGestureRecognizer. Fix: if (!ReferenceEquals(e.OriginalSource, sender)) return; or check FocusState.
  • [warning] Fires on auto-repeating KeyDown (GesturePlatformManager.Windows.cs:457): holding the key re-runs SendTapped. Android parity path fires once on KeyUp. Fix: guard e.KeyStatus.WasKeyDown or activate on KeyUp.
  • [suggestion] Test gap (BorderTests.Windows.cs:107): only asserts IsTabStop, never exercises actual Enter/Space dispatch — the core behavior is unprotected by regression tests.

Already addressed in this head (do NOT re-raise): IsEnabled guard, secondary-tap (RightTapped) subscription regression, Control setter detaching KeyDown, and the byte → ushort widening for 1 << 8.

CI: Not green. The directly relevant maui-pr-devicetests (net11.0 Windows Helix Tests Run DeviceTests Windows) leg FAILED (the Windows Build leg passed). Overall 17 checks failing (also Windows-unrelated: MacCatalyst/iOS/Android device + UI test legs). Independently of the code findings, the failing net11.0 Windows Run DeviceTests leg makes this not LGTM-eligible; the new BorderTests.Windows should be confirmed running green before merge.

@Vignesh-SF3580

Copy link
Copy Markdown
Contributor Author

Verdict: NEEDS_CHANGES (confidence: medium)

Validating against the actual code resolves the split: opus-4.8's bubbled-KeyDown concern is confirmed real — ContentPanelOnKeyDown is attached to the ContentPanel (an ancestor of the Border's content) yet never checks e.OriginalSource/focus, so a focusable child whose Enter/Space isn't consumed (e.g. a single-line Entry on Enter) bubbles up and spuriously fires the Border's tap (then e.Handled = true swallows the key). gpt-5.5's auto-repeat concern is also real and corroborated in-repo: the Windows path fires on auto-repeating KeyDown, whereas the Android OnKeyPress it claims parity with deliberately acts only on KeyEventActions.Up. These are concrete code fixes, not just discussion, so the verdict lands past NEEDS_DISCUSSION/LGTM.

Why the models split: opus-4.6 (LGTM) verified the prior-round fixes landed and pattern-matched the subscription lifecycle, but dismissed the failing Windows device-test leg as infra/"no results" and didn't analyze WinUI routed-event bubbling, so it missed the missing origin guard. gemini (NEEDS_CHANGES, 0 inline) gated correctly on CI but produced no code finding. gpt-5.5 and opus-4.8 each found one of the two real handler gaps but framed them as discussion. Cross-pollination keeps both code findings plus the test-coverage gap.

Key findings (all novel — prior rounds only raised the now-fixed IsEnabled guard):

  • [warning] Missing origin/focus guard in ContentPanelOnKeyDown (GesturePlatformManager.Windows.cs:440): bubbled Enter/Space from a focusable child can spuriously activate the Border's TapGestureRecognizer. Fix: if (!ReferenceEquals(e.OriginalSource, sender)) return; or check FocusState.
  • [warning] Fires on auto-repeating KeyDown (GesturePlatformManager.Windows.cs:457): holding the key re-runs SendTapped. Android parity path fires once on KeyUp. Fix: guard e.KeyStatus.WasKeyDown or activate on KeyUp.
  • [suggestion] Test gap (BorderTests.Windows.cs:107): only asserts IsTabStop, never exercises actual Enter/Space dispatch — the core behavior is unprotected by regression tests.

Already addressed in this head (do NOT re-raise): IsEnabled guard, secondary-tap (RightTapped) subscription regression, Control setter detaching KeyDown, and the byte → ushort widening for 1 << 8.

CI: Not green. The directly relevant maui-pr-devicetests (net11.0 Windows Helix Tests Run DeviceTests Windows) leg FAILED (the Windows Build leg passed). Overall 17 checks failing (also Windows-unrelated: MacCatalyst/iOS/Android device + UI test legs). Independently of the code findings, the failing net11.0 Windows Run DeviceTests leg makes this not LGTM-eligible; the new BorderTests.Windows should be confirmed running green before merge.

@kubaflo I have addressed the valid changes.

  • Added a check at the beginning of ContentPanelOnKeyDown to verify that the key event originated from the ContentPanel. If the event source is a child element, the key event is ignored.
  • Added a check to ignore repeated key events after the original source validation. When the key event is an auto-repeat event, it is skipped. Only the initial key press continues and triggers the tap action.
  • Simulating a physical key press (Enter/Space) is not currently possible in Windows device tests because there is no existing keyboard injection support in MAUI's device test infrastructure. The existing device test covers the behavior that can be validated at this layer — ContentPanel.IsTabStop correctly reflects the presence of a TapGestureRecognizer. This validates the tab-stop behavior that enables keyboard focus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-controls-border Border community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration platform/windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants