-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Accessibility: Move focus back to the modal dialog to More Menu when it was used to get there #16964
Conversation
if ( ! this.containerRef.current.contains( document.activeElement ) ) { | ||
if ( | ||
! this.containerRef.current.contains( document.activeElement ) && | ||
! document.activeElement.closest( '[role="dialog"]' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea whether we can assume that focus can be moved to the element with [role="dialog"]
in all cases. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely feels like there needs to be a more general way to track that a modal or popover was opened from the context of another modal or popover, and exclude it from the focus outside check.
There's a similar issue here with the inline link popover, where it needs to check whether focus is in the autocomplete list to avoid closing:
gutenberg/packages/format-library/src/link/inline.js
Lines 168 to 179 in fb910e5
onFocusOutside() { | |
// The autocomplete suggestions list renders in a separate popover (in a portal), | |
// so onFocusOutside fails to detect that a click on a suggestion occurred in the | |
// LinkContainer. Detect clicks on autocomplete suggestions using a ref here, and | |
// return to avoid the popover being closed. | |
const autocompleteElement = this.autocompleteRef.current; | |
if ( autocompleteElement && autocompleteElement.contains( document.activeElement ) ) { | |
return; | |
} | |
this.resetState(); | |
} |
Having said that, I can't think of a case where this logic fails, so I think it'd be pragmatic to ship this, and then consider what a more general solution might be.
This is exactly how More Menu stays wrongly opened: |
Thank you, I need to figure out how you search for those issues, there are too many of them :) Yes, I think this PR solves this exact problem. There is a similar issue for popovers like a calendar as well. |
3d290f3
to
1d890e5
Compare
Once #17297 gets merged, this issues will automatically get resolved 🎉 |
…it was used to get there
1d890e5
to
7bdecb5
Compare
…omponent with the dropdown menu
efed57c
to
fc08871
Compare
I rebased this branch with This PR is ready for the final review and testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works really nicely in testing 🎉
if ( ! this.containerRef.current.contains( document.activeElement ) ) { | ||
if ( | ||
! this.containerRef.current.contains( document.activeElement ) && | ||
! document.activeElement.closest( '[role="dialog"]' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely feels like there needs to be a more general way to track that a modal or popover was opened from the context of another modal or popover, and exclude it from the focus outside check.
There's a similar issue here with the inline link popover, where it needs to check whether focus is in the autocomplete list to avoid closing:
gutenberg/packages/format-library/src/link/inline.js
Lines 168 to 179 in fb910e5
onFocusOutside() { | |
// The autocomplete suggestions list renders in a separate popover (in a portal), | |
// so onFocusOutside fails to detect that a click on a suggestion occurred in the | |
// LinkContainer. Detect clicks on autocomplete suggestions using a ref here, and | |
// return to avoid the popover being closed. | |
const autocompleteElement = this.autocompleteRef.current; | |
if ( autocompleteElement && autocompleteElement.contains( document.activeElement ) ) { | |
return; | |
} | |
this.resetState(); | |
} |
Having said that, I can't think of a case where this logic fails, so I think it'd be pragmatic to ship this, and then consider what a more general solution might be.
Description
Related: #15321 (it's probably the fix).
This PR changes the behavior of More Menu in terms of maintaining focus. When a dialog is opened from one of the menu items, then we no longer close More Menu. Instead, we keep it opened and move focus back to the More Menu when the dialog is closed.
Known issue
When I mouse-click outside of the modal dialog then More Menu remains opened. This is because Modal dialog prevents all events to be propagated below the overlay layer. We need to find a fix for it, @youknowriad any hints?- this is resolved nowHow has this been tested?
esc
to close the dialog.See also the screencast.
Screenshots
Types of changes
Checklist: