Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overlay popup focus issues #17326

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Conversation

MrJul
Copy link
Member

@MrJul MrJul commented Oct 22, 2024

Note: #17322 should be merged first for tests to correctly pass. This PR is based on it. Merged

What does the pull request do?

This PR fixes several problems related to overlay popups:

  • Key navigation didn't work inside them.
  • For flyouts, the first control wasn't automatically taking the focus.

How was the solution implemented (if it's not obvious)?

A KeyboardNavigationHandler has been added to OverlayPopupHost, mimicking the behavior of a standard popup.
For this to work correctly, IInputRoot had to be implemented on OverlayPopupHost.

OverlayPopupFlyoutTests has been added, based on FlyoutTests but with overlays (3 failing tests before the changes).
PopupTests.Focusable_Controls_In_Popup_Should_Get_Focus was passing for wrong reasons: it didn't try to really move the focus. This test has been adjusted.

Remarks

Access keys still don't work in overlay popups after this PR. I'll fix them in another PR.

Fixed issues

@MrJul MrJul added bug customer-priority Issue reported by a customer with a support agreement. labels Oct 22, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052765-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Oct 23, 2024
@MrJul MrJul force-pushed the fix/flyout-keyboard branch from 1b4df78 to 3619c26 Compare October 23, 2024 09:09
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052774-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member

Couldn't existing IFocusScope infra be reused here?

@MrJul
Copy link
Member Author

MrJul commented Oct 23, 2024

Couldn't existing IFocusScope infra be reused here?

OverlayPopupHost is already a IFocusScope, so once focus is inside the popup, it already works correctly.

However, for key events to work, this requires the Popup itself to be attached to the visual tree.
In the flyout case, the Popup is created dynamically: its InteractiveParent is effectively null (which is correct). Events will stop there and won't bubble up to the parent window containing the KeyboardNavigationHandler.

Rather than hacking this, I chose to have the OverlayPopupHost use its own KeyboardNavigationHandler: this better matches what happens in the non-overlay case (with the PopupRoot having its KeyboardNavigationHandler).

@MrJul MrJul added this pull request to the merge queue Oct 24, 2024
Merged via the queue into AvaloniaUI:master with commit b5fd40e Oct 24, 2024
10 checks passed
@MrJul MrJul deleted the fix/flyout-keyboard branch October 24, 2024 08:11
maxkatz6 pushed a commit that referenced this pull request Oct 27, 2024
* Add failing focus tests for flyouts inside overlay popups

* Implement IKeyboardNavigationHandler on OverlayPopupHost

* Layout OverlayPopupHost content for focus to work
@maxkatz6 maxkatz6 added backported-11.2.x and removed backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-11.2.x bug customer-priority Issue reported by a customer with a support agreement.
Projects
None yet
3 participants