[ListView] Allow deleting blocks using keyboard #50422
Conversation
kevin940726
added
the
[Feature] List View
|
Size Change: +458 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4274a5e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5098153081
|
|
Currently, pressing keyboard shortcut when the dropdown is opened doesn't work. Before jumping into a solution for that, I'd like to know where the focus should be once the users delete a block. On trunk, clicking on the delete button in the list view dropdown moves focus to the editor canvas. My intuition would be to retain focus in the list view and move the focus to the previous block's dropdown button. In this PR, deleting blocks inside the list view using keyboard will move focus to the previous row within the list view. I wonder which is the preferred way, and what the expected result should be for pressing keyboard shortcut when the dropdown is opened. c.c. @alexstine, @andrewserong, @talldan. |
|
@kevin940726 It would be nice to be able to keep the focus in the list view. I think another PR I suggested this and it was discovered that it would not be easy. |
There was a problem hiding this comment.
Very cool progress @kevin940726! I'm really excited about this one, and it's testing quite well for me 👍
The main issue I ran into is with deleting blocks that are outside the selection. Currently it seems to return focus to the next block up in the list view via selecting that block. If the block that's deleted isn't part of the selection, is it possible to shift focus to the block directly above the deleted block, without altering the block selection? Assuming that's the desired behaviour, of course.
There's another subtle issue, which is that a block deletion isn't always successful, so for shifting focus, we might need to check whether or not it was a success before updating? Here's an example with a locked block — after attempting to delete, focus (and selection) is shifted one position up unexpectedly:
One last question is whether or not deleting should always be available? I think the answer is probably yes, but it'd be good to test it out in other situations where the list view is in use. #50287 just landed yesterday I think, which now uses the ListView component in the Navigation menu in the left hand sidebar of the site editor when in Browse mode. After a rebase, we should be able to test this there, too:
packages/block-editor/src/components/list-view/block-select-button.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
Outdated
Show resolved
Hide resolved
|
@andrewserong Thanks for the review and nice suggestions! I'll address them later! 🙇 |
kevin940726
force-pushed
the
fix/list-view-delete-and-focus
branch
from
0dc67e8 to
d4cec92
Compare
| editor.canvas.getByRole( 'document', { | ||
| name: 'Block: Heading', | ||
| } ) | ||
| ).toBeFocused(); |
There was a problem hiding this comment.
These tests are deleted because they check if the focus is moved to the editor canvas but not the list view.
| const shouldUpdateSelection = | ||
| hasSelectedBlocks && getSelectedBlockClientIds().length === 0; | ||
|
|
||
| __experimentalSelectBlock( blockToFocus, shouldUpdateSelection ); |
There was a problem hiding this comment.
Still undecided if we should also rename this to __experimentalFocusAndSelectBlock. Even though it's an experimental API, for some reason it's still documented in the README 😅 .
There was a problem hiding this comment.
Interesting! I didn't realise it was documented, either. Perhaps we could keep the name the same, but update the docs to mention the additional param?
There was a problem hiding this comment.
I'm thinking maybe just delete the docs for this prop? It's an "experimental" prop anyway? 😅 Not sure about the impact though 🤔 .
There was a problem hiding this comment.
This is testing great for me @kevin940726, thanks for all the detailed work and follow-up!
✅ All keyboard shortcuts appear to be working as expected
✅ Attempting to delete a selection containing a locked block does nothing
✅ Deleting a block that is not part of the selection shifts focus one block up without altering selection
✅ Deleting block(s) in a selection moves focus and selection up to the previous block in the list view
The only issue I ran into is unrelated to this PR and exists on trunk — shift selecting blocks that contain a locked block causes focus to shift outside of the list view. It's a little hard to see in this gif, but I'm holding shift and pressing down, and once I pass the locked block, focus is lost from the list view:
At any rate, that's not a blocker for this PR. Great work, and appreciate the updated tests, too! LGTM ✨
| const shouldUpdateSelection = | ||
| hasSelectedBlocks && getSelectedBlockClientIds().length === 0; | ||
|
|
||
| __experimentalSelectBlock( blockToFocus, shouldUpdateSelection ); |
There was a problem hiding this comment.
Interesting! I didn't realise it was documented, either. Perhaps we could keep the name the same, but update the docs to mention the additional param?
|
@alexstine Do you want to take a look at this to make sure everything works accessibility-wise? I think the focus management should behave more intuitively now IMO. I'd like to know if I missed anything though! 🙇 |
|
@kevin940726 I would like to give this a look but currently my local environment is not working. I might be able to test tomorrow. |
alexstine
added
the
[Focus] Accessibility (a11y)
There was a problem hiding this comment.
@kevin940726 This is almost perfect, one last issue. It seems like the text that aria-describedby points to is not being re-rendered which delivers incorrect information to screen reader users. To replicate:
- Insert some blocks, I inserted a total of 5.
- Open the list view.
- Remove the first block.
- Notice how focus is not lost, great job. The block in position 2 now becomes position 1, leaving us with 4 blocks total.
- If you inspect the description text read to screen readers, it says the 1st block is block 2 of 5. The description text does not get an updated position or total.
Can you make sure this variable is updated before the focus event occurs so that way screen readers get an up to date description?
|
@alexstine Thanks for testing! I can't seem to reproduce it using Mac's VoiceOver (or I'm using it wrong somehow). I still pushed a commit e75c4e7 trying to fix it. LMK if it works for you, until I find a way to test it locally! 🙇 |
|
@kevin940726 Still not working. :( Here is some speech output.
You can clearly see that the screen reader is not picking up the new changes after a block is deleted. It is providing incorrect positioning information. Thanks. |
|
@alexstine Weird, I have no idea what's wrong here 😢. IIRC, you're using NVDA on Windows, right? I went ahead and download a Windows 11 VM on my mac and download NVDA to test this branch. Here's my speech output:
I deleted two blocks, one using the Delete key and the other using the options menu. Both work correctly and announced the correct amount of the list size. Could you pull the latest branch and try again locally 🙇♂️? Just want to eliminate the possibility of a broken build 😅. |
|
@kevin940726 Still not working. Even tried going back to default screen reader settings. The part not updating has an ID that looks like this: This is reproduceable in Firefox. However, this does work fine in Google Chrome. I wonder if we discovered some type of ARIA bug in Firefox. You happen to know anything about this @MarcoZehe ? Thanks. |
|
Sorry, I am retired, haven't worked on Firefox for over two years. |
|
@MarcoZehe I figured it was worth a try anyway. Do you have any suggestions on who I could reach out to? I have never tried contributing to Firefox personally. Maybe this is a good time to start. Thanks. |
|
The person I'd contact is @jcsteh from Mozilla. |
|
OMG yes! The missing part is Firefox apparently 😅. I was able to reproduce the issue in Firefox, even using Mac's VoiceOver. I'm not sure if it's a Firefox bug or deliberate decision. I'll try to debug some more! Thanks for the additional context! |
kevin940726
force-pushed
the
fix/list-view-delete-and-focus
branch
from
e75c4e7 to
4274a5e
Compare
|
I think I figured it out! I'll post my findings in a separate PR so that we can isolate the problem for future readers. I think the issue is not introduced in this PR so it shouldn't be a blocker. If everyone's okay with this I'll merge this 🙇♂️! |
|
Nice work landing this, folks! 🎉 |
| /** | ||
| * @param {KeyboardEvent} event | ||
| */ | ||
| onKeyDown( event ) { |
There was a problem hiding this comment.
Is the only reason for this callback to keep the focus in the list view after deleting/duplicating/inserting a block? Or is there another reason?
There was a problem hiding this comment.
It's to allow pressing keyboard shortcuts when the focus is in the menu dropdown. I couldn't find any other way to do this without having to duplicate a lot of the code 😢 .
| canRemove | ||
| ) { | ||
| event.preventDefault(); | ||
| updateSelectionAfterRemove( onRemove() ); |
There was a problem hiding this comment.
updateSelectionAfterRemove doesn't seem to be expecting any arguments — why do we pass onRemove() ? (same would apply for the instances of updateSelectionAfterRemove in this function)
There was a problem hiding this comment.
Oops. I think I was trying to follow the same style of pipe() used in the click handler below, and also to match the style of updateSelectionAfterDuplicate. I agree it could be confusing given that the function doesn't accept any arguments though!
* Allow deleting blocks using keyboard from listview * Update updateSelection instead * Add support for kyeboard shortcut in the dropdown * Only select block if there isn't any already * More things!
What?
Fix #49087. Allow deleting blocks using the
DeleteandBackspacekey, or the keyboard shortcutShift+Alt+z(Ctrl+Alt+zon macOS).Why?
To retain feature parity. See #49087.
How?
Added a
removeRowfunction and aKeyboardShortcutscomponent.Testing Instructions
Delete,Backspace, orShift+Alt+z(Ctrl+Alt+zon macOS) to delete the focused blocks.Testing Instructions for Keyboard
Shift+Alt+oorCtrl+Alt+oon macOS).Delete,Backspace, orShift+Alt+z(Ctrl+Alt+zon macOS) to delete the focused blocks.Screenshots or screencast
Kapture.2023-05-09.at.08.40.23.mp4