Fix block toolbar positioning #42086
Conversation
talldan
added
[Type] Bug
|
Size Change: +59 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
| @@ -63,6 +63,7 @@ export default function BlockPopover( { | |||
| __unstableObserveElement={ selectedElement } | |||
| __unstableForcePosition | |||
| __unstableShift | |||
| __unstableAvoidOverflow | |||
There was a problem hiding this comment.
Thanks for the exploration @talldan! I've started looking at this too and for now it seems to me we already have too many flags for specific behavior, and it would be great if we can find a way to not introduce a new one, but consolidate some others or make use of the existing ones.
It might be unavoidable but let's see 😄
There was a problem hiding this comment.
Yeah, I agree that it'd be great to avoid these props.
Long term, I think floating-ui's middleware API could serve as an inspiration. It'd be nice to have something like it that's not quite as low level. Popover positioning could be more about composition of functions or hooks rather than boolean props:
function useBlockPopoverPositioning( selectedElement ) {
return [
usePopoverObservePosition( selectedElement ),
usePopoverForcePosition(),
usePopoverShift(),
];
}
function BlockPopover( selectedElement, // ... ) {
// ...
return (
<Popover
positioning={ useBlockPopoverPositioning( selectedElement ) }
// ...
/>
);
}
talldan
force-pushed
the
fix/block-toolbar-positioning
branch
2 times, most recently
from
8d71f68 to
60653d6
Compare
talldan
force-pushed
the
fix/block-toolbar-positioning
branch
from
60653d6 to
01ddbd3
Compare
|
I think this is now working as expected. It'd be good to get some testing to confirm that. As discussed above, implementing this in the popover component and adding yet another unstable prop isn't ideal, but I don't think this is the right PR to refactor the Popover component. I'm happy to work on a follow up that explores something like this - #42086 (comment). |
There was a problem hiding this comment.
Thanks for the explorations Dan! Besides my comments the changes which caused our issue were the ones that made the block toolbar to scroll inside the block - which in this PR is not there. So I'm not sure this is the direction we want to go and if it was, it might be simpler to just update the flags in BlockPopover.
This is a problem I personally find really tricky even after having invested some time on it. We need to be careful about adding even more complexity if we are not 100% sure..
Noting that much testing was in site editor which seems to have different behavior from the post editor(iframe).
| // Checking against `referenceRect.y` seems to be all that's | ||
| // needed. | ||
| if ( floatingRect.height > referenceRect.y ) { | ||
| return flip( { |
There was a problem hiding this comment.
Such an approach of having a wrapper middleware for floating-ui ones was something I thought about and experimented a bit. I think that might be a good approach but we need to probably consolidate some existing flags that affect different results. For example flip and shift are connected in our problem..
We need to be cautious though as we might end up in pitfalls like this one for example. For example if we don't have __unstableForcePosition we might end up with two flip and circumvent the safeguard of the library. I tried to replicate though and didn't observer an infinite loop, but my point is we need to sure about the changes we will introduce.
There was a problem hiding this comment.
That's a good point, they should be mutually exclusive. I'll update the PR.
I think we should reconsider both __unstableForcePosition (it does too much) and this new prop (which hopefully we'll find a better way to achieve). But I think it's important to fix the problem for users before prioritising the code.
| placement: currentPlacement, | ||
| } = middlewareProps; | ||
| const isTopPlacement = | ||
| currentPlacement.includes( 'top' ); |
There was a problem hiding this comment.
This in general lives in Popover but seems too specific for our block toolbar issue. Ideally we would consolidate the other flags to better adjust the core middlewares or our middleware would be probably in the end to handle any kind of overflow.. Not there yet to suggest a solution though..
|
I tend to agree with @ntsekouras's feedback. In general, I'm surprised that we can't find a solution that just relies on the right combination of floating UI's Could the problem be related to passing/computing incorrect "viewport edges" and/or anchors instead? |
@ntsekouras If there's some part that's missing in my PR let me know. I tried comparing to the block toolbar before the popover refactor and couldn't see any difference to what's in this PR. However, I might be misinterpreting your comment.
@ciampo I don't think so, but you should definitely try debugging the problem for yourself. I've personally found floating-ui a difficult library to work with and debug—the docs are lacking in detail and it's hard to determine what some of the I think the block toolbar use case is probably pushing the library quite far. It is very ad-hoc, and the way it behaves is quite specific to the structure of our UI. That's hard for a general purpose library to emulate. |
In site editor: trunktrunk.movthis PRpr.mov |
|
Thanks for the videos @ntsekouras, I must have missed testing the site editor with long enough content to cause a scroll bar. For some reason floating-ui calculates positions differently in the site editor. 🤯 I'm beginning to think it might be worth reconsidering use of this library, really struggling to understand these bugs. 😞 |
|
Closing this, as from conversations here and in the associated issue we've discovered there are a few deeper issues related to the iframed site editor. |
What?
Attempts to fix #41575, where the block toolbar doesn't flip position if there's no space at the top of the editor canvas.
How?
I've tried introducing a new prop
__unstableAvoidOverflowthat when set runs new floating-ui middleware.For Popovers that have a 'top' placement, this middleware detects if there's enough space for the popover in that top placement. If there isn't, it flips the popover to be positioned at the bottom using the
flipmiddleware.When there is space at the top of the , the middleware falls back to calling the
shiftmiddleware. This makes it so that when a larger block is selected, scrolling will cause the toolbar to adopt a sticky behavior at the top of the screen. It isn't possible to use the__unstableShiftprop for this second part because when that's active it causes the first 'flip' behavior I described to have no effect (I'm unsure if this is a floating-ui bug).Testing Instructions
Testing that the toolbar flips when there's no space at the top of the canvas
Expected - the toolbar should be under the block
Testing that the toolbar shifts over the block when scrolling (this is not currently working)
Expected - the toolbar should maintain a sticky position at the top of the viewport.
Screenshots or screencast
Before
After
Kapture.2022-07-13.at.17.13.40.mp4