Block toolbar: fix error #20458
Block toolbar: fix error #20458
Conversation
|
Size Change: +26 B (0%) Total Size: 865 kB
ℹ️ View Unchanged
|
|
Something I don't understand I do see |
|
@youknowriad But not |
|
I think we might have to require it. Rendering the toolbar inline is not good since tabbing to it is broken then. |
|
Can we render it automatically at the beginning of |
|
That wouldn't work. It needs to be rendered outside of the writing flow. More specifically, outside the focus capture elements. It could be built into WritingFlow though. I do wish WritingFlow was built into BlockList... |
Me too :) I think the only thing prevent this is the PostTitle component in Gutenberg which in theory we could fix by duplicating some "arrow navigation handling in PostTitle" maybe? |
|
I think ideally the post title needs to be some sort of real block. We can also not move the post title out of writing flow, otherwise reverse tabbing from the content will focus it. :) Either it needs to be a block, or we should provide a slot at the top of the block list to render the title in it from the outside? |
|
I actually had already tried to move the popover slot to writing flow in #19431, but ran into some issues with reusable blocks I think. |
|
So maybe for now, let's just add the slot to the right position in the different screen/playground? |
|
Yes, either that, or we explicitly pass the boundary container to the popover. I'll add the slots for now so it's at least no longer broken |
ellatrix
force-pushed
the
fix/block-toolbar-widget
branch
from
b530efe
to
f2c961d
Compare
| @@ -38,6 +38,7 @@ function Layout( { blockEditorSettings } ) { | |||
| content={ | |||
| <> | |||
| <Notices /> | |||
| <Popover.Slot name="block-toolbar" /> | |||
There was a problem hiding this comment.
I'm wondering if we want one slot per widget area here or a single slot for the whole page. It's not clear how we should reason about this particular slot, where to put it if you're building your own editor. (maybe needs some docs)
Also, do we want to expose it as a BlockToolbarSlot in @wordpress/block-editor package?
There was a problem hiding this comment.
That's the thing... I was hoping that we could build it into WritingFlow or BlockList (together with WritingFlow or something) to avoid exposing it. But maybe there's no other way. I also had an earlier idea to unify it with the top toolbar, in which case exposing it maybe makes more sense.
There was a problem hiding this comment.
Ok, got it. I'm fine if we wait more and see if we can consolidate.
There was a problem hiding this comment.
What about the widgets screen question?
There was a problem hiding this comment.
I don't know so much about the widgets. Did I currently add one slot for all areas? If that works, that seems fine? What do you think?
There was a problem hiding this comment.
I think the tabbing order might not be great with this solution but at least the toolbar is showing.
There was a problem hiding this comment.
Oh, I guess it might be better to have one per area then. Let's iterate on it. Worth noting that previously, it wasn't possible to tab to the toolbar at all because it was mounted inside WritingFlow.
|
Thanks! |
Description
Fixes #20429.
This is a potential solution, but I'm not sure if it's the best.
The problem is that a popover slot may not always be provided, so the popover may not have a parent element with the
popover-slotclass.Either we:
popover-slotclass to the root container. Not a very clean solution, but it work when the popover is rendered in place (not in a slot). We could change the class to something else if necessary.Popoverto determine what the boundary container is.Perhaps the last solution is the best?
How has this been tested?
Screenshots
Types of changes
Checklist: