Improve Block Toolbar Semantics: Divergent Block Tool DOM #55223
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
…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
If we 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.
…der into slot or within block editor
Move isCollapsed state from the block-contextual-toolbar into the site editor header, as it is only needed and related to the site editor header. This is not a state the block toolbar needs to know about. Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
If the block toolbar was collapsed in the large viewport, then the viewport is resized to be small enough that the toolbar should move to underneath the header, the toolbar would be in the collapsed state so it would not show.
…lesVirtually from the toolbar group slot Removing bubblesVirtually from the toolbar group slot prevents any dynamically added toolbar buttons from firing their click event. This can be seen when cropping an image, you press Crop, then the Apply and Cancel buttons get added to the toolbar. Those button events don’t fire properly and is mentioned as an issue in the bubblesVirtually slot fill PR: #19242
Adding position absolute to all instances of __background-style lets it be excluded from all flexbox calculations, which is what we want. If the div is empty, it still gets calculated and pushes elements around.
This test was already covered well by 'ensures base block toolbars use roving tabindex'
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. Also, this is better than attaching it to all of the children in the block toolbar because when new items are attached to the toolbar (such as via the bubblesVirtually slots or the apply/cancel buttons when cropping an image) we can still catch the events. Otherwise, those buttons are added after the mount, and the children don't receive the listener.
…rder of the params matters
Didn't revert since there were a few useful things in it.
This reverts commit 42efbe6.
This reverts commit ac289cd.
…ck toolbar" This reverts commit 4440e98.
This reverts commit 330cfc0.
15 tasks
|
Size Change: -434 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
Closing in favor of #55787 |
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?
A version of #54513 to be able to easily test a different interaction of the toolbar refactoring. This one behaves as:
The top toolbar has a Slot within the header. If the Top Toolbar setting is on, then the block tools are rendered within the header DOM. If the Top Toolbar setting is off, then the block toolbar is rendered within the editor canvas DOM as on
trunkand theShift + Tabbehavior to reach the toolbar is preserved.Why?
To be able to test out the different interactions easily, as this is a problem where being able to interact with the different PRs helps provide a better "feel" for the right decision.
How?
See #54513
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
Top Toolbar
Non-Top Toolbar (Default floating block toolbar)