Fix focus loss when navigating the guide component #40324
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Bug
youknowriad
added
the
[Focus] Accessibility (a11y)
youknowriad
commented
Apr 13, 2022
|
|
||
| { ! canGoForward && ( | ||
| <FinishButton | ||
| className="components-guide__inline-finish-button" |
There was a problem hiding this comment.
AFAIK, this button was always hidden and useless.
|
Size Change: +195 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
alexstine
reviewed
Apr 14, 2022
There was a problem hiding this comment.
@youknowriad Thanks for catching this. Need to add an E2E test to catch focus loss?
Anyway, left one note. Other than that, looks good.
| return; | ||
| } | ||
|
|
||
| if ( nextButton.current ) { |
There was a problem hiding this comment.
Would it be possible to just focus the container? Starting at the bottom is not really the best situation for UX. Then the user must navigate back to the top. Could also focus on button "Close dialog". The dialog container should be able to focus as long as it has tabindex -1.
There was a problem hiding this comment.
I was wondering the same but it felt to me that when navigating in a guide, in generally you want to click the buttons "next" "next" "next" then "get started". Or "previous" then "next"....
There was a problem hiding this comment.
@youknowriad Correct, but the guide doesn't have any context like this. You need to start at the top here and let users work their way through the content. The other solution would be to add aria-labelledby and aria-describedby attributes and then the content will be read and focus can remain on the Next button. I just opted to start at the top since it would be an easier change.
There was a problem hiding this comment.
I think we should be consistent. This PR fixes the focus loss but the current "Next" buttons (even without this PR) still keep the focus on the button when changing pages.
I'm happy to change the behavior but we need to be consistent no matter which buttons are used.
There was a problem hiding this comment.
@youknowriad On latest trunk when the dialog first gains focus, it is on the Close button. I think it would be good to keep it like that and always start at the top.
There was a problem hiding this comment.
Oh, I see what you mean. Next/Previous buttons in trunk keep focus at the bottom, not on initial focus. Probably still better to start at the top since that is where initial focus is placed.
There was a problem hiding this comment.
Ok, I've updated this. The close button is now always focused when you switch pages.
|
There's already an e2e test that catches this, the problem is that it doesn't fail because |
|
@youknowriad Is there a tracking issue to fix useFocusOutside? |
No but I was hoping that we'd upgrade to React 18 which fixes the issue. |
|
Ah, okay. React 18 is the fix. 👍 I am also blocked here so I am also in favor. |
alexstine
approved these changes
Apr 15, 2022
gziolo
added
the
Backport to WP 6.7 Beta/RC
adamziel
pushed a commit
that referenced
this pull request
Apr 19, 2022
|
This was cherry-picked to |
adamziel
removed
the
Backport to WP 6.7 Beta/RC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from #32765
What?
For some reason, in the React 18 upgrade PR #32765,
useFocusOutsideworks better than trunk (I don't know why yet). Which means there were some failing e2e tests that actually show real bugs we have on trunk, more precisely "focus loss" bugs.One of them is in the guide component (editor welcome guide): On trunk, on chrome, you can try clicking "previous" on the second page, you'll notice that the focus is lost (document.activeElement equal to body) but for some weird reason in trunk, the modal doesn't close.
This PR solves the focus loss in the Guide component.
Why?
Focus loss is an a11y issue even if the modal is kept open right now.
How?
I've noticed that there was already a behavior in place that auto-focuses the "finish" button. this PR removes that specific behavior and implements a generic solution that handle all cases:
Testing Instructions
1- Open the editor welcome guide
2- Click once on "next"
3- Click on "previous" to get back to the first page
4- Check the active element, it should be in the "next button".
Screenshots or screencast