Accessibility improvements for List View Part 1 #37798
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
alexstine
requested review from
youknowriad,
kevin940726,
talldan and
aristath
alexstine
added
[a11y] Keyboard & Focus
[Package] Block editor
|
This focus issue really baffles me. 😞 If you press Escape while List View is open, focus is placed on List View button properly. However, if you select Close, focus fails and you just get hung out in the page. I switched to useEffect that way I could listen to the List View open from store. Probably not a great solution but it works 100% for both cases. What do you all think? Is there something I'm missing on useFocusReturn that could be causing this? I may try to debug later this weekend... Thanks! |
|
Size Change: -544 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
I think I see why. The ref returned by A way to test this, if you open List View, shift-tab to the close button and then press escape you'll see focus is lost then also. So it's more about where the focus resides when the panel is closed. Things seem to work ok if the line I did think that only |
|
Two separate focus return refs seems to resolve all issues. There might be a better way. Here's some code: const focusOnMountRef = useFocusOnMount( 'firstElement' );
const headerFocusReturnRef = useFocusReturn();
const contentFocusReturnRef = useFocusReturn();
function closeOnEscape( event ) {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
event.preventDefault();
setIsListViewOpened( false );
}
}
const instanceId = useInstanceId( ListViewSidebar );
const labelId = `edit-post-editor__list-view-panel-label-${ instanceId }`;
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
aria-labelledby={ labelId }
className="edit-post-editor__list-view-panel"
onKeyDown={ closeOnEscape }
>
<div
className="edit-post-editor__list-view-panel-header"
ref={ headerFocusReturnRef }
>
<strong id={ labelId }>{ __( 'List view' ) }</strong>
<Button
icon={ closeSmall }
label={ __( 'Close list view sidebar' ) }
onClick={ () => setIsListViewOpened( false ) }
/>
</div>
<div
className="edit-post-editor__list-view-panel-content"
ref={ useMergeRefs( [
contentFocusReturnRef,
focusOnMountRef,
] ) }
>
<ListView
onSelect={ selectEditorBlock }
showNestedBlocks
__experimentalFeatures
__experimentalPersistentListViewFeatures
/>
</div>
</div>
); |
|
@talldan Is there anything wrong with just using how I made the PR? I know it seems a little unconventional, but it works without much effort. I wish I could just use the dialog approach like the Inserter uses, but focus shouldn't be constrained to this element. |
|
@alexstine If we can use the existing |
|
@talldan I honestly have no idea how this even works but it works. Let me finish applying the fixes to other packages and I'll push fresh commit. Something weird happening in logic for sure. |
alexstine
requested review from
adamziel,
draganescu,
noisysocks and
tellthemachines
as code owners
tellthemachines
approved these changes
Jan 12, 2022
carolinan
approved these changes
Jan 12, 2022
|
The code looks good to me... I only have one question: Why did we change the case to some sentences here, capitalizing the 1st letter of every word? I was under the impression that we're generally trying to avoid that 🤔 |
|
I was wondering the same, but it seems we are capitalizing feature names: |
|
Ah OK, gotcha. Makes sense then 👍 |
aristath
approved these changes
Jan 12, 2022
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.
Description
How has this been tested?
I tested in Firefox with the NVDA screen reader. Changes only applied to Posts and Pages for now.
Screenshots
Types of changes
Bug fix and enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).