Render Selected Block Tools in Header when using Top Toolbar #55787
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
jeryj
force-pushed
the
base/top-toolbar-refactor
branch
from
ad492e6 to
3ed50ec
Compare
|
Size Change: -1.87 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
jeryj
force-pushed
the
base/top-toolbar-refactor
branch
from
3ed50ec to
ad87967
Compare
jeryj
force-pushed
the
add/top-toolbar-slot
branch
from
c16a8da to
1387c60
Compare
jeryj
force-pushed
the
add/top-toolbar-slot
branch
from
1387c60 to
20ce22b
Compare
|
Flaky tests detected in 5e57aee. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6817632386
|
jeryj
force-pushed
the
add/top-toolbar-slot
branch
2 times, most recently
from
84679e2 to
57e2f67
Compare
jeryj
added
the
[Focus] Accessibility (a11y)
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>
Allows block tools in dom header to overflow scroll using flex-shrink. Every element parent of the scrolling toolbar needs an overflow-x: hidden and flex-shrink: 2 (or higher) value. Also sets the small screen size to have the fixed toolbar underneath the header bar.
There were two forms of the small screen top toolbar -- one when the top toolbar option was on, and one when it was off. Both were intended to look and operate in the same way. Rather than maintain two identical systems, it makes more sense to only have one. This also makes the visual and tab order match for all versions, which is better accessibility too.
- remove repeated styles - remove unused styles, like some extraneous flex-shrinks that weren't doing anything
jeryj
force-pushed
the
add/top-toolbar-slot
branch
from
57e2f67 to
ea00096
Compare
isLargeViewport is used as both useViewportMatch( 'medium' ) and useViewportMatch( 'large' ). useViewportMatch( 'medium' ) to mean isLargeViewport seem s to be the standardized way, so useViewportMatch( 'large' ) should equate to the size above it - isWideViewport.
|
Edit: I just retested and it looks fine: 
|
scruffian
approved these changes
Nov 10, 2023
There was a problem hiding this comment.
I think this is looking in a very good shape. I have tested:
Post editor ✅
Post editor in template mode ✅
Site editor ✅
Widget editor ✅
Widget editor in Customizer ✅
I know there is some follow up to do, but I suggest we bring this in as is and then do the follow up in more PRs.
ajlende
approved these changes
Nov 10, 2023
|
Please keep me on the loop about the follow-ups. It doesn't feel to leave this in its current state as it's not clear for third-party editors how to render block toolbars however they want. (they have to use a private API?) |
youknowriad
reviewed
Nov 13, 2023
| @@ -56,6 +56,14 @@ | |||
| } | |||
| } | |||
|
|
|||
| .block-editor-block-contextual-toolbar.is-fixed { | |||
There was a problem hiding this comment.
I know it hasn't been introduce in this PR but the isFixed prop in the contextual toolbar is weird. It's "contextual" toolbar why have an isFixed prop :). Anyway, I'm curious when this was introduced and the reasoning for it.
There was a problem hiding this comment.
I don't know the reason why it was added, but my interpretation of contextual meant "tools filled contextually with relevant block actions." As in, the block tools were all contextual relevant to the block selected. I did not interpret "contextual" as "adjacent to." I think changing the component name to have Popover in it would be clearer, like <BlockToolbarPopover />.
There was a problem hiding this comment.
We already have things like BlockPopover and SelectedBlockPopover which is probably very similar to what you propose. It seems we do need to bring some clarity to the components and more importantly have a clear purpose for each component that stays clear as we iterate and refactor.
cbravobernal
pushed a commit
that referenced
this pull request
Nov 14, 2023
The Top Toolbar was relying on position: absolute; CSS and layoutEffect calculations to display the selected block tools visually within the top toolbar. This PR places it in the header DOM where we want it, allowing us to use native CSS and DOM flow to improve accessibility (tab order matches visual order) and have more maintainable code. * Imports `<BlockContextualToolbar />` via private-api and the <Popover /> for image captions to the edit site, edit post, edit widgets, and customize widget headers. * Removes position: absolute; and layoutEffect block toolbar positioning hacks. * CSS for the top toolbar to use flex-shrink and overflow-x: hidden; to allow for the block toolbar to fit its current space and scroll to reveal the hidden tools. * With top toolbar mode, Shift+Tab does not go directly to the toolbar but the first tabstop outside of the editor. Co-authored-by: Alex Lende <ajlende@gmail.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
This was referenced Nov 14, 2023
@youknowriad Will do! |
|
Just noting that this PR actually broke the "third-party" block editor use-cases. As you can see in the "undo-redo" playground https://wordpress.github.io/gutenberg/?path=/story/playground-block-editor--undo-redo the block toolbar is not visible there at the moment. The goal of that prototype is to show third-party developers how to render the block toolbar in a specific location of their choice and potentially add buttons to that toolbar (undo/redo buttons in the case of the linked prototype). Right now, it seems that it's not possible to do that anymore. We should follow-up with a fix to these prototypes and the corresponding docs |
|
Hi there! Looks like this PR introduced a regression for the post editor in template editing mode, where Steps to reproduce:
Here's a little video: wheresthebackbutton.mov |
|
The issue above is also visible in the site editor. |
|
I believe this issue is fixed by #56217 |
|
Noticed that some of the Please avoid to use |
11 tasks
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.
Addresses #53013. Likely others too.
What?
Render the Selected Block Tools in the Header when using Top Toolbar mode.
Why?
The Top Toolbar is relying on
position: absolute;CSS andlayoutEffectcalculations to display the selected block tools visually within the top toolbar. This PR places it in the DOM where we want it, allowing us to use native CSS and DOM flow to improve accessibility (tab order matches visual order) and have more maintainable code.How?
<BlockContextualToolbar />via private-api and the<Popover />for image captions to the edit site, edit post, edit widgets, and customize widget headers: If the top toolbar mode is on and we're on a large screen, render the block tools within the header. This logic is necessary, as the block editor should be able to be rendered as a standalone editor.position: absolute;andlayoutEffectblock toolbar positioning hacks.flex-shrinkandoverflow-x: hidden;to allow for the block toolbar to fit its current space and scroll to reveal the hidden tools. This is the same behavior as on trunk.The only functional change from
trunkis to match the visual order with tab order, which allows us to meet the focus order and meaningful sequence. So, when using the top toolbar mode at a large screen,shift+Tabwill bring you to the first tab stop outside of the visual editor.Testing Instructions for Keyboard
Tab order
Escape/Alt+F10
Especially check block tools that use one of the toolbar slots, like the text formatting tools on the paragraph.
Block Tool Overflow
Repeat test on Post Editor, Site Editor, and Widgets editor
Repeat tests for Post Editor, Site Editor, Widgets Editor, Widgets Customizer, and Post Edit -> Post Sidebar -> Template -> Edit Template
Widgets editor needs a theme like twenty twenty to be able to access it.
co-authored by @ajlende