Fix useFocusFirstElement in non-contenteditable Blocks #37934
Conversation
alexstine
added
[Focus] Accessibility (a11y)
alexstine
requested review from
youknowriad,
aristath,
talldan and
kevin940726
|
Size Change: +71 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
| @@ -190,7 +190,7 @@ function BlockSelectionButton( { clientId, rootClientId, blockElement } ) { | |||
| if ( focusedBlockUid ) { | |||
| event.preventDefault(); | |||
| selectBlock( focusedBlockUid ); | |||
| } else if ( isTab && selectedBlockClientId ) { | |||
| }/* else if ( isTab && selectedBlockClientId ) { | |||
There was a problem hiding this comment.
What is this line for? It seems to not be working properly. Even checked out on trunk this line is an issue. If I have one Paragraph and one Columns, I will enter Gutenberg Navigation Mode. If I press Tab, I will land on Paragraph, then Columns, then I get the first button within Columns. After commenting this line, I get placed in the Title field and I can go through my list again which seems like a much better workflow than random focus of Columns block content and possibly other Blocks.
There was a problem hiding this comment.
I am trying to fix this in the below PR.
| > | ||
| { accessibleTitle } | ||
| </Button> | ||
| { isAccessibleModalOpen && ( |
There was a problem hiding this comment.
I really don't like this. I am duplicating too much code I think. Would be cool at some point to figure out how to pull this content in to the Modal via a ref or something. However, that may not be much more efficient.
There was a problem hiding this comment.
Hi, @alexstine
We could store __experimentalBlockVariationPicker into a variable then use it in both places. What do you think?
Example:
const variationPicker = (
<__experimentalBlockVariationPicker
icon={ get( blockType, [ 'icon', 'src' ] ) }
label={ get( blockType, [ 'title' ] ) }
variations={ variations }
onSelect={ ( nextVariation = defaultVariation ) => {
if ( nextVariation.attributes ) {
setAttributes( nextVariation.attributes );
}
if ( nextVariation.innerBlocks ) {
replaceInnerBlocks(
clientId,
createBlocksFromInnerBlocksTemplate(
nextVariation.innerBlocks
),
true
);
}
} }
allowSkip
/>
);
|
Sorry about that. |
There was a problem hiding this comment.
Testing with VoiceOver on Safari, the dialog provides a much better experience than the default block placeholder. It does require markup duplication though, so it would be nicer if we could just make the existing placeholder markup more accessible.
Comparing the dialog and the placeholder markup, the main difference seems to be in the use of aria-labelledby and aria-describedby in the dialog, that causes the label and description to be read. The placeholder markup is totally devoid of semantics, and VoiceOver ignores it.
The other aspect is, of course, that the dialog keeps focus inside it so users can't tab away from the placeholder before selecting an option. How important is this? Because if we don't have to constrain focus to the placeholder, we can experiment with other types of markup, for instance, putting the whole placeholder inside a fieldset, with the label and description as its legend.
|
@tellthemachines It was necessary to use the dialog because of the tabbing issues. Remember, for Windows users, Tab and Shift+Tab are primary navigation keys unlike the majority of cases on Mac with Voiceover. If there is some other approach that would allow us to get the same results of a dialog without using a dialog, that would be great. There are a ton of libraries at play in block-list though and it will be tough to figure out how to solve this without breaking it further. Essentially what needs to happen is this. The only difference being we need to only focus the Sidebar when there is no more focusable element in the block content. This could get tricky though playing around with focus which is why I actually do like the dialog better but I understand how intrusive it is. My thoughts:
Any of this sounding reasonable? It just sounds insane to have to mess with focus like this. |
I'm not sure I've got this 100%, so to clarify: do we want to move focus into the block placeholder with the Tab key, when a new block is inserted? Currently, when inserting a Columns block, focus moves to the whole block, and then we have to press Down Arrow in order to access the buttons inside. But once inside, all the controls can be navigated with Tab. Would focusing the first focusable element in the block after insertion solve the problem? Plus adding semantic markup so the block heading and description are read out, of course. Whatever we decide on, the solution should work for all blocks that have placeholders: Navigation, Cover, Image, Gallery, Table, etc. so ideally it should be a global change in the placeholder, as opposed to individual changes for each block. |
|
@tellthemachines Oh no, I forgot to test this. So, you are correct in one case.
Here is the problem I am seeing.
Sorry for not being more clear about this, but it works differently based on that setting. We've got to have this working the same way across the board. Thanks. |
|
It almost seems like Writing Flow is allowing some type of focus switching and whenever Writing Flow is disabled for switching from Block to Block, this is why it breaks. |
|
Oooh, I should have thought to test it with the "Contain text cursor inside block" setting! It makes sense that Arrow key navigation wouldn't work with that setting enabled. I'm thinking ultimately the only way we can make things work properly is making the whole editor content navigable with Tab. But perhaps to start with, we can make it so the block placeholder buttons can be accessed with Tab instead of Down Arrow. |
alexstine
changed the title
|
@tellthemachines I just pushed a fix which should help with this. Thoughts? I tested with Columns and Navigation Blocks and Tab works fine now. I also gave the Paragraph Block a test just to ensure this didn't break existing functionality. Finally, refreshed the PR with latest commits. This was a much easier solution, wish I had thought of this earlier. Thanks. |
…sible-block-content-dialog
…press/gutenberg into try/accessible-block-content-dialog
alexstine
added
the
[Feature] List View
alexstine
force-pushed
the
try/accessible-block-content-dialog
branch
from
49592b6
to
065ba6f
Compare
| if ( ! target.getAttribute( 'contenteditable' ) ) { | ||
| const focusElement = focus.tabbable.findNext( target ); | ||
| // Ensure is not block inserter trigger, don't want to focus that in the event of the group block which doesn't contain any other focussable elements. | ||
| const skipBlockInserterTrigger = target.classList.contains( |
There was a problem hiding this comment.
This has been something of a terrible pain. For some reason, hasAttribute/getAttribute throw errors on tests so I gave up and just used classList. I attempted to get it working with aria-label but for whatever reason, this is way easier in tests than it is in practice.
|
@ellatrix I have implemented the E2E tests. Does this help you understand what is going on? BTW, this is the first couple E2E tests I've added so still learning... It is an interesting mindset to take a step back and mock how your functionality should work. |
There was a problem hiding this comment.
Thanks for all the great work here @alexstine! I'm not all that familiar with the focusing on first element behaviour, but this is testing nicely for me, and it does feel like a good improvement when inserting a Columns, Table, or Image block, and having focus be on the first button within the placeholder state.
My testing:
- Before: when I insert an image block, the wrapper receives focus.
- After: when I insert an image block, the Upload button receives focus.
The additional and updated e2e tests are appreciated, as it's quite a detailed interaction to get right! There was a good point made about it being harder to immediately delete a block after insertion, but I agree, I think there'll be ways to come up with a fix for that in a follow-up.
It'd be good to also get a final seal of approval from @ellatrix and / or @tellthemachines, but I just wanted to chime in that from my perspective I think this PR is in good shape! 🎉
| @@ -192,6 +192,7 @@ describe( 'deleting all blocks', () => { | |||
| // Add and remove a block. | |||
| await insertBlock( 'Image' ); | |||
| await page.waitForSelector( 'figure[data-type="core/image"]' ); | |||
| await page.keyboard.press( 'ArrowUp' ); | |||
There was a problem hiding this comment.
This is only relevant when you add a block, immediately realise it's the wrong block, and want to quickly delete it. It would be nice to preserve that ability, but we should be able to find a workaround for it.
I wonder if in a follow-up, we can look at in placeholder states that don't have an editable input field (e.g. the Columns placeholder state, but not the Twitter embed input field), and see if we can get backspace to still delete the block?
|
I'm mostly AFK until Thursday, but would like to review this before merge. I'll try to do it today or tomorrow or otherwise Thursday. |
…sible-block-content-dialog
|
@andrewserong In theory, it should be possible. The only thing I never liked about this method of removing a block is focus gets super lost so this may give us a good opportunity to fix both issues.
|
alexstine
force-pushed
the
try/accessible-block-content-dialog
branch
from
9d67e9f
to
f8871ca
Compare
| let ariaLabel = await page.evaluate( () => | ||
| document.activeElement.getAttribute( 'aria-label' ) | ||
| ); | ||
| // If the labels don't match, try pressing Up Arrow to focus the block wrapper in non-content editable block. |
There was a problem hiding this comment.
I think this is way simpler compared to the modifications that were here and tests are still green. Seems logical to me. This whole file still is in desperate need of commenting.
There was a problem hiding this comment.
This is working really well 🎉 - tested by adding Image, Table and Columns blocks. Also tested moving focus from the List View into those blocks, and duplicating an Image block from the List View. In all cases, focus lands on the first focusable element within the block placeholder.
Updates to existing tests look good, and thanks for adding some new tests for this behaviour!
| @@ -192,6 +192,7 @@ describe( 'deleting all blocks', () => { | |||
| // Add and remove a block. | |||
| await insertBlock( 'Image' ); | |||
| await page.waitForSelector( 'figure[data-type="core/image"]' ); | |||
| await page.keyboard.press( 'ArrowUp' ); | |||
There was a problem hiding this comment.
That won't work for Table though, because the first focusable field is an input 😅
But we should be able to work something out. Definitely as a follow-up.
Description
Fixes useFocusFirstElement to take in to account non-contenteditable Blocks. In this case, it performs a secondary search to find a more suitable focus point rather than inserting the caret at the edge which doesn't work well for Blocks without contenteditable.
How has this been tested?
Tested in Firefox with NVDA screen reader on Windows 10.
Screenshots
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).