List view: Allow selected block to override roving tabindex #48339
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
[Type] Bug
|
Size Change: +15 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
I am shocked this did not break E2E tests. I guess we have no tests around tabindex and what could have focus at any given time. Interesting. |
There was a problem hiding this comment.
This is a clever fix, nice idea @alexstine! It's testing well for me so far, and I like the idea that we're adding a little logic to the List View component while using the roving tabIndex as a fallback. So essentially, it's simply adding additional taxIndex values to selected blocks to ensure they're available. I think that makes for a nice workaround.
For multi-block selections, though, I notice that there are now many blocks in the list view that can receive focus. For example, if I create 10 paragraph blocks and shift select them, and then open the the list view, tabbing will tab between each of the selected blocks in the list view, making it take longer if I wish to tab outside of the list view.
Is that an issue? If so, one idea might be to only override the tabIndex for either the first block in the selection, or the last block in the selection, whichever one seems more appropriate. If it isn't an issue, then this PR looks in good shape to me.
|
@andrewserong So, I actually thought I should open a new issue about this. The terms in code confuse the heck out of me in Gutenberg. Selected can mean two things.
I really wish we as devs would stop conflating the two because it makes it impossible to understand in what context Any idea on how to fix it? Thanks. |
|
That's a great point about distinguishing between the current block that's being edited or has focus, versus being selected. You're right, I think the two concepts are conflated in API naming. In terms of preventing the change when a multi selection is happening, we could perhaps try something like the following: Then, in the checks where you're currently using Would that work? |
…selected in canvas and not multi-selected.
|
@andrewserong Yes, it seems to work. I tried to give the variable a reasonable name that describes what we are after. There is a related PR where the list view E2E tests are currently being migrated. I want to write a test for this but need to wait until it is merged. I already held it up once. |
|
@andrewserong Nice catch. Not sure why ESLint did not catch that. Not sure why the page did not error out... |
|
Flaky tests detected in 436c61c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4265989427
|
andrewserong
approved these changes
Feb 24, 2023
There was a problem hiding this comment.
I think lenth didn't throw an error because selectedClientIds is technically an object (albeit an array), so possibly the typo meant that ESLint thought the code was seeing if there was a lenth property. If we attempted to do anything other than a comparison check, it might have thrown an error. No matter now, though! 🙂
Thanks for the follow-ups, this is testing well for me! I agree, I think adding an e2e test could wait until the refactored tests land. LGTM!
alexstine
commented
Feb 24, 2023
| @@ -42,7 +42,7 @@ test.describe( 'List View', () => { | |||
| ] ); | |||
|
|
|||
| // Drag the paragraph above the heading. | |||
| const paragraphBlockItem = await listView.getByRole( 'gridcell', { | |||
| const paragraphBlockItem = listView.getByRole( 'gridcell', { | |||
There was a problem hiding this comment.
Had to remove this. ESLint would not allow me to commit.
There was a problem hiding this comment.
geyByRole doesn't return a promise so this is the right fix 👍 .
|
Lets give this a try and see if anyone reports bugs. 👍 |
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?
Fixes #48337
Why?
See above.
How?
If a block is selected, I replace the roving tabindex value with 0. Roving tabindex only keeps track of the last focused block and has no way to know about blocks selected in the canvas.
Testing Instructions
tabindex="0"is set.Testing Instructions for Keyboard
No difference.
Screenshots or screencast
Notes
I know this is not a perfect solution due to the way things could get odd with the options button. In any case, the block will always receive focus but the correct options button will not. This is because the block will have 0 tabindex which would make it the first tabbable. You can better understand this by looking at
useFocusFirstElementhook. I think this is a fair trade-off for the benefit of not having a large refactor. The other option is, I disable the roving tabindex provider with a custom parameter and handle this in list view that way being in block-editor, I can have awareness of selected blocks.