Implement accessible version of Navigation overlay preview toggle control #53462
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
getdave
added
[Focus] Accessibility (a11y)
getdave
commented
Aug 9, 2023
Comment on lines
+543
to
+544
| <div id={ overlayMenuPreviewId }> | ||
| { overlayMenuPreview && ( |
There was a problem hiding this comment.
Accessibility question. I couldn't find any definitive best practice for whether the element being targetted by aria-controls should always remain in the DOM.
Currently the contents are added/removed by React on toggle. But should we instead leave it in the DOM and show/hide using CSS?
There was a problem hiding this comment.
I would think so. Here is an example that helps.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls
This uses the tab panel use-case where the tab content is hidden via CSS but not totally removed from the DOM.
Why is this panel being removed from the DOM anyway? Performance reasons?
There was a problem hiding this comment.
Just tested this and could not find any negative effects. No warnings in console after opening/closing.
There was a problem hiding this comment.
Why is this panel being removed from the DOM anyway? Performance reasons?
Generally it seems to be a common pattern in React. I believe it's because it's easier for the developer to simply not-render something based on state rather than figure out the best way to hide via CSS.
Sometimes I guess rendering a component when it's not needed might trigger performance issues so that's worth bearing in mind.
In this case however we should be ok to leave the items in the DOM.
|
Size Change: +1.3 kB (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 949a3f2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5807469099
|
Co-authored-by: Alex Stine <alex.stine@yourtechadvisors.com>
getdave
changed the title
getdave
deleted the
fix/nav-block-unlabelled-overlay-menu-controls-toggle
branch
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?
Make the Navigaiton block's overlay controls toggle perceivable to assistive tech.
Fixes #53461
Why?
Because otherwise a whole set of users cannot properly configure the block.
How?
aria-controlsto indicate which controls the button togglesaria-expandedto button to indicate current state of control toggleTesting Instructions
Testing Instructions for Keyboard
Settingstab.Displaypanel within that section.Toggle overlay menu controls.aria-expandedis set tofalse.aria-controlsattribute refers to an element with a valid DOMidattribute.aria-expandedupdate.aria-updates accordingly.Screenshots or screencast
Screen.Capture.on.2023-08-09.at.10-36-12.mp4