Make save panel a dialog with proper labels, fix site editor focus loss after save #59622
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
alexstine
added
[Focus] Accessibility (a11y)
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
alexstine
commented
Mar 6, 2024
| @@ -100,6 +101,14 @@ export default function SavePanel() { | |||
| const { setIsSaveViewOpened } = useDispatch( editSiteStore ); | |||
| const onClose = () => setIsSaveViewOpened( false ); | |||
|
|
|||
| const saveBtnRef = useRef(); | |||
| useEffect( () => { | |||
There was a problem hiding this comment.
Because it takes too long for the isSaveViewOpen to fully resolve, the Open save panel button is not rendered in time causing a focus loss to occur. Using the effect here prevents this.
|
Size Change: -113 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
jeryj
reviewed
Mar 8, 2024
There was a problem hiding this comment.
There are two ways to get to the save panel -- the Open save panel button and the Save button in the site editor header. We'll need to account for both of these. With this PR, clicking the header save button, then cancelling the dialog, moves focus to the Open save panel button instead of back to the header save button.
I think useDialog has some things that do the work of returning focus to the button that activated it. Maybe we can lean on that?
|
@jeryj Okay, this is making more sense now. So, the Thanks. |
jeryj
reviewed
Mar 8, 2024
| @@ -130,7 +139,8 @@ export default function SavePanel() { | |||
| variant="secondary" | |||
| className="edit-site-editor__toggle-save-panel-button" | |||
| onClick={ () => setIsSaveViewOpened( true ) } | |||
| aria-expanded={ false } | |||
| aria-haspopup={ 'dialog' } | |||
| ref={ saveBtnRef } | |||
There was a problem hiding this comment.
I think the bug here is that the save panel button gets removed when isSaveViewOpen is true. Because the save panel button is removed, <_EntitiesSavedStates onClose={ onClose } /> loses its reference for where to return focus to onClose. If we restructure this section so that the button isn't removed but hidden when isSaveViewOpen === true, the focus should return correctly. I'll do a quick PR to demo. Might be easier to explain that way.
There was a problem hiding this comment.
Here's the example: https://github.com/WordPress/gutenberg/pull/59716/files
jeryj
reviewed
Mar 8, 2024
Comment on lines
200
to
202
| role="dialog" | ||
| aria-labelledby={ dialogLabel } | ||
| aria-describedby={ dialogDescription } |
There was a problem hiding this comment.
We need this to be marked up appropriately, but by adding the role here I think it'll mean this modal will have two nested dialogs?
Yup! It needs to stay in the DOM so useDialog knows where to return focus to.
I think the core issue is that the ref has changed. Even if the button were still in the DOM, I think it being removed and remounted is what causes the Ref to change. Even if it's the same element, I believe the ref changes when it's unmounted/remounted. I'm not 100% positive on this though.
That's the same solution I came to! |
|
@jeryj Ready for another review round. |
jeryj
approved these changes
Mar 8, 2024
|
@jeryj Could you please assist me with the E2E test failing? It works in my browser so not sure why the test fails. |
|
I tried updating the failing test. 🤞🏻 |
|
On a procedural note, I'd like to remind everyone that non-trivial pull requests should be preceded by a related issue, to allow for broader discussion. @alexstine thank you for looking into fixing the focus loss, that's certainly something that needs to be fixed. Re: the dialog: as a reminder for everyone, a In this specific case, the save panel will become a non-modal dialog:
Theoretically, I wouldn't have objections to a non-modal dialog but I'm wondering why we would want to have it in the first place. Alex can you please clarify, when you have a chance? If the purpose of making the Save panel a
so that they are automatically announced when entering the dialog, then I'm wondering if there is another way to make that information be announced when entering the ARIA region instead of using a dialog. |
|
@afercia The way I see it, the save panel should be a dialog because tabbing is constrained. If there was no reason to constrain tabbing, I wouldn't have made it a dialog. Essentially, what makes this different from the list view is the list view can actually move you around the page and it is quite natural for focus to leave it. The save panel allows users to review changes and either save or cancel, I do not believe users should be able to navigate away from this with the keyboard. The The settings and styles panels from my understanding should not be dialogs because they do not implement constrained tabbing in any way and they are adjusting the content in the page so it would only make sense that you might make an adjustment and region navigate to see what changed. You could not do this if dialog role was used. Thanks. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…ss after save (WordPress#59622) * Accessibility fixes for save panel. * Add dialog support. * Revert edit post package changes for now. * Add wrapping div for title. * Add E2E test to prevent future regressions. * Fix label ID. * Try hiding the button visually instead of removing from DOM. * Add disabled. * Restructure how the dialog is rendered. * Pass down one more prop. * Move element hidden when save panel is open * Update button name to exact true since it resolves to two buttons * Fix test. --------- Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org>
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.
What?
Why?
Fix accessibility.
How?
See code.
Testing Instructions
aria-labelledbyandaria-describedbyattributes matching the proper IDs.Testing Instructions for Keyboard
Screenshots or screencast