Proof of Concept: Improve Block Toolbar Semantics/Accessibility #53779
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
|
Size Change: +283 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
5 tasks
…erve all visual interactions This is a big commit with a large potential for bugs. Think of this as a spike or POC for now. There'll be a lot to address. Some general TODOs: - [ ] Refactor shared code between empty-block-inserter and selected-block-tools - [ ] More explicitly pass the popover slot and content ref into the SelectedBlockTools - [ ] Shortcut/keystrokes for returning from the toolbar to where the cursor was before moving into the toolbar - [ ] Inline Tools either move to the top toolbar or get inserted into the main block toolbar (think image caption formatting tools) - [ ] Visual styles for the top toolbar
…ock. Still more todo. This shouldn't go to the last focused block but the last _selection_ which will be a bit more work. Probably best to refactor how it works within use-tab-nav. Maybe turn it into a hook or something that can have centralized logic.
It's not working for buttons that contain dropdowns, such as the align or options buttons. The dropdown toggle is intercepting the escape keypress and not letting it bubble.
…n via onChildrenKeyDown passed to NavigableToolbar Note: Due to the use of portals in the toolbar slots, consistently handling event listeners cleanly is difficult. This method adds event listeners to all children of a toolbar if a `onChildrenKeyDown` prop is passed to the <NavigableToolbar /> via a querySelectorAll( '[data-toolbar-item="true"]' ) on the NavigableToolbar ref, then applying an addEventListener to all the returned buttons. Pros: Event listener is applied and handled in one location. Cons: Feels yucky. Feels like crossing our fingers and hoping it works consistently, which it might.
…edblock toolbar, attached to the NavigableToolbar This method requires forwarding a ref from the editor level down to the navigable toolbar so that the escape unselect shortcut can be blocked and the navigable toolbar event listener will still fire. Blocking the global escape event shouldn't be necessary, but we have a combination of a few things all combining to create a situation where: - Because the block toolbar uses createPortal to populate the block toolbar fills, we can't rely on the React event bubbling to hit the onKeyDown listener for the block toolbar - Since we can't use the React tree, we use the DOM tree which _should_ handle the event bubbling correctly from a `createPortal` element. - This bubbles via the React tree, which hits this `unselect` escape keypress before the block toolbar DOM event listener has access to it.
…-controls slot If remove the bubblesVirtually from the block-controls slot, then the event will bubble in the DOM as normal (instead of the React Tree only since bubblesVirtually uses createPortal for the fills). This means we could handle the event without any forwardRefs as we don't need to block the escape shortcut keypress event from the React Tree. However, we assume bubblesVirtually was made for a reason. So, we're unsure if something would break.
jeryj
force-pushed
the
refactor/block-toolbar-portal
branch
from
e9438ba
to
401cf4c
Compare
|
Flaky tests detected in 401cf4c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6098992510
|
|
@alexstine @joedolson - Could I get feedback on this implementation? I'm curious how you feel about the interaction and resulting (hopefully improved) accessibility of the menu bar. The toolbars are now all combined in one spot in the header, so I've wrapped the entire header in a Note: There are probably bugs here. Potentially some large ones. No need to do a code review or flag the bugs for now. I'm focused on this as a proof of concept first, and then if we decide it's worth moving forward with, I'll fix it up more. |
|
@jeryj This is pretty awesome. Here are some thoughts. I did find one bug that might not be related to your PR but worth mentioning anyway just in case. Steps to reproduce:
Still need to figure out how to communicate that Tab/Shift+Tab can navigate between different toolbars. Might be worth researching the ARIA spec to find out the best way to describe this. The toolbars are all together so in theory, communicating it should be easier. Maybe a one-time nag/notice to let keyboard users know? Slack started doing this and it is quite helpful. When focus enters the toolbar for the first time, trigger a notice.
Then could have a dismiss button. Maybe I really like how this frees up Tab/Shift+Tab for in-block actions. This has been needed for such a long time. In non-rich text blocks for example, this would be a perfect way to navigate through tabbables. While in rich text blocks, it could be used for indenting as your example shows above. Great work on this! Thanks. |
|
Thanks for the review, @alexstine! So glad to hear it has potential to move forwards! I'll get some wider eyes on it too. I think I know what the issue is with the bug on the captions toolbar. I'll try to get that fixed today. |
… by default on escape keypress Not committed to this, but it does work since we've removed bubblesVirtually from the toolbar slots and the inline rich editor doesn't use slots for its tools. I like that this brings this behavior by default to the toolbars, but unsure about if it's a good idea overall.
This was referenced Sep 14, 2023
14 tasks
This was referenced Sep 15, 2023
|
Closing in favor of #54513 |
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.
Large refactor/spike to test out how combining block tools in the DOM might be possible, as discussed in #53013. All mouse + visual placements should be the same.
This is a big commit with a large potential for bugs. Think of this as a spike or POC for now. There'll be a lot to address. Some general TODOs:
This PR is intended as an exploration if we can refactor how the block toolbar structure works to be more like a classic word editor where alt + f10 moves you to all of the block tools. An escape keypress would bring you back to where you were in the editor. If we move forward with this idea, this PR can be split into smaller, more production ready PRs. Think of this as a playground for how it could work.
What?
Moving all Document and Block Tools together into the DOM.
Why?
How?
Moves Block Tools into the editor header using slot/fill.
Ideas
Testing Instructions
Visual and mouse interactions should all be identical to trunk for both Top Toolbar and the default floating block toolbar. Ideally, you don't notice any changes.
Testing Instructions for Keyboard
Combined Toolbar Controls
Focusing the block toolbar
Returning focus to the block
Returning focus to the block from anywhere outside the block toolbar (Could be removed if superfluous, but I feel like it's quite nice for navigating)
Stretch goal, shouldn't be merged in this PR. Used as an example of what's possible now: Tab keypresses on rich text blocks enter tab space.
Screenshots or screencast