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

Accessibility: Move focus back to the modal dialog to More Menu when it was used to get there #16964

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 8, 2019

Description

Related: #15321 (it's probably the fix).

When the "Keyboard Shortcuts" and "Options" dialogs are dismissed,
the keyboard focus is reset to the top of the page. This may be
confusing or inconvenient for users, who would expect the focus to
return to the last element they were focused on.

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 now

How has this been tested?

  1. Open More Menu.
  2. Click on the Block Manager menu item (or anything else).
  3. Close the dialog by clicking on the close button.
  4. Focus should return to the item in More Menu.
  5. Repeat steps 2-4 but this time click outside of the modal dialog instead..
  6. Repeat using keyboard-only navigation (space to select an item and esc to close the dialog.

See also the screencast.

Screenshots

focus-more-menu-dialog

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [a11y] Keyboard & Focus labels Aug 8, 2019
@gziolo gziolo requested a review from afercia August 8, 2019 12:45
@gziolo gziolo self-assigned this Aug 8, 2019
@gziolo gziolo added the Needs Accessibility Feedback Need input from accessibility label Aug 8, 2019
if ( ! this.containerRef.current.contains( document.activeElement ) ) {
if (
! this.containerRef.current.contains( document.activeElement ) &&
! document.activeElement.closest( '[role="dialog"]' )
Copy link
Member Author

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?

Copy link
Contributor

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:

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.

@gziolo
Copy link
Member Author

gziolo commented Aug 8, 2019

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 exactly how More Menu stays wrongly opened:

more-menu-open

@afercia
Copy link
Contributor

afercia commented Aug 8, 2019

@afercia - do we have an open issue for this or did we talk about it only in comments on other PRs/issues?

@gziolo Besides mentions in other PRs/issues, I think #15321 is very specific to this problem.

@gziolo
Copy link
Member Author

gziolo commented Aug 8, 2019

@gziolo Besides mentions in other PRs/issues, I think #15321 is very specific to this problem.

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.

@gziolo gziolo force-pushed the fix/move-focus-back-from-dialog-more-menu branch from 3d290f3 to 1d890e5 Compare August 30, 2019 15:18
@gziolo gziolo mentioned this pull request Sep 3, 2019
5 tasks
@gziolo
Copy link
Member Author

gziolo commented Sep 3, 2019

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 exactly how More Menu stays wrongly opened:

more-menu-open

Once #17297 gets merged, this issues will automatically get resolved 🎉

@gziolo gziolo force-pushed the fix/move-focus-back-from-dialog-more-menu branch from 1d890e5 to 7bdecb5 Compare September 4, 2019 06:48
@gziolo gziolo requested review from nerrad and ntwb as code owners September 4, 2019 09:33
@gziolo gziolo force-pushed the fix/move-focus-back-from-dialog-more-menu branch from efed57c to fc08871 Compare September 4, 2019 09:51
@gziolo
Copy link
Member Author

gziolo commented Sep 4, 2019

I rebased this branch with master and refactored some test utils and tests which use them to make everything work with the updated behavior of the dropdown menu and modal dialogs.

This PR is ready for the final review and testing.

Copy link
Contributor

@talldan talldan left a 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"]' )
Copy link
Contributor

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:

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.

@gziolo gziolo merged commit 4baab9a into master Sep 9, 2019
@gziolo gziolo deleted the fix/move-focus-back-from-dialog-more-menu branch September 9, 2019 14:44
@gziolo gziolo added this to the Gutenberg 6.5 milestone Sep 9, 2019
@gziolo gziolo removed the Needs Accessibility Feedback Need input from accessibility label Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants