Ensure block toolbar doesn't overlap block by modifying forcePosition and shift popover props #42887
Conversation
|
Size Change: +520 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I'm a bit behind with my review queue at the moment, but I'll try to take a look soon enough! |
There was a problem hiding this comment.
I know that this is still a rough implementation and it's far from final code, but I stil gave this PR a try. The behavior is quite inconsistent:
- the toolbar is flipped also for blocks that shouldn't be flipped
- the "flip" doesn't always revert to the normal position
Kapture.2022-08-05.at.11.47.16.mp4
The main thing to discuss is the idea behind the approach:
Passes a different
placementto thePopovercomponent depending on the amount of space at the top of the canvas. It does this in theSelectedBlockPopovercomponent, which seems to be the one responsible for the floating block toolbar.The good thing is that this doesn't touch the popover component, it works consistently in the post and site editors, and it doesn't completely prevent the
shiftbehavior of the toolbar for blocks lower down in the canvas.
Personally I'm not sure about this approach — it's still potentially quite a lot of code with non-trivial logic (one we polish it), which can be error prone (and therefore not too different from a custom middleware).
I'd like to rely on floating-ui as much as possible, so here's another idea that came to mind:
- when a block is selected (or shifted across the document), we could still read (outside of the
Popovercomponent) the distance of the selected block from the start of the page (and not of the viewport) - if the distance if less than a certain threshold (which can be the toolbar's height or any arbitrary number), we could simply set a prop on
Popoverto enable theflipbehavior
This would keep the block editor-specific code separate from Popover, but would also avoid writing any popover-specific code outside of the Popover.
|
@ciampo Sorry for the delay in replying, have been working across a few areas lately.
I've updated the code to work like this. I agree that it is nicer 👍 I think it probably has the same problems to solve, that we'd need to update on block movement or resizing to determine the new props. That's still not an insurmountable problem, but I haven't attempted to solve it yet, just aiming for a proof of concept.
I noticed something like this. There's the same problem with the new approach too. I think floating-ui may not support modifying placement or middleware on the fly. I had little success in calling floating-ui's To solve it for now, I've forced the popover to remount when a different block is selected - see d06ade5. I don't think this is a long term fix, but I've pushed it so that the PR demonstrates the desired behavior. |
Yeah, we should calculate the distance of the currently selected block from the top of the page (not the viewport):
That's unfortunate — it was also my impression that at most we'd need to call |
| @@ -328,7 +329,7 @@ const Popover = ( | |||
| } | |||
|
|
|||
| return autoUpdate( usedRef, refs.floating.current, update ); | |||
| }, [ anchorRef, anchorRect, getAnchorRect ] ); | |||
| }, [ anchorRef, anchorRect, getAnchorRect, middlewares ] ); | |||
There was a problem hiding this comment.
Interesting that this is also not enough to trigger an update of the popover behavior when changing its behavior through enabling/disabling middlewares
talldan
force-pushed
the
try/flip-block-toolbar-by-modifying-placement
branch
from
d06ade5 to
afcd109
Compare
talldan
changed the title
talldan
added
[Type] Bug
|
With the other popover fixes, this is working much better now. I've removed any hacks and tidied up the code. Open to more feedback, testing and reviews now. I haven't fixed the issue where the toolbar doesn't shift fully below the block in the site editor, but there are other PRs looking at that problem. |
talldan
force-pushed
the
try/flip-block-toolbar-by-modifying-placement
branch
from
e504f79 to
8ca5616
Compare
|
Thanks Dan! I'll try to have a look soon at this |
There was a problem hiding this comment.
Code looks much cleaner!
I think we will still need to listen to more events that can change the distance of the selected block from the toolbar.
For example, if a block is re-ordered, the toolbar currently doesn't adjust its position:
Kapture.2022-08-18.at.20.44.45.mp4
What if we undo an action that causes the block to change its distance from the top of the page (eg adding a block on top, and then cmd+z, which would remove it)?
I also checked multi-block selection and it seems to work
packages/block-editor/src/components/block-tools/use-block-toolbar-popover-props.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-tools/use-block-toolbar-popover-props.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-tools/use-block-toolbar-popover-props.js
Outdated
Show resolved
Hide resolved
I thought I'd solved that, but yes I see that issue. It seems to work ok when a block is moved down, but that's because I'll look into it. |
talldan
force-pushed
the
try/flip-block-toolbar-by-modifying-placement
branch
from
9a14434 to
5a8c121
Compare
|
I think I've covered most of the edge cases now, but more testing is welcome. |
There was a problem hiding this comment.
This PR is testing well for me! I couldn't find a way to "trip" the logic, which is great — and the code looks clean.
Apart from a few remaining comments, my only reservation left is about performance — measuring the offsetHeight and calling getBoundingClientRect are expensive functions to run, and so I wonder if we can find ways to call getProps as sparingly as possible (without introducing regressions, of course). For example, in my tests I could notice that function being called up to 4 times when clicking a single block
packages/block-editor/src/components/block-tools/use-block-toolbar-popover-props.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-tools/use-block-toolbar-popover-props.js
Outdated
Show resolved
Hide resolved
This reverts commit 59b192f.
talldan
force-pushed
the
try/flip-block-toolbar-by-modifying-placement
branch
from
f33eae9 to
bc43d19
Compare
There was a problem hiding this comment.
Code changes LGTM 🚀
My only doubt, as mentioned before, is regarding performance: the getBoundingClientRect() function are a relatively expensive call to make (as they cause layout reflow), and as of now they are executed 3 to 6 times (instead of only 1).
Kapture.2022-08-23.at.10.57.29.mp4
I don't think it's a blocker for this PR, especially given how useful this fix is — but definitely something that I'd like to ideally follow-up on in order to keep the site editor as snappy as possible (for example, we could store the result of getBoundingClientRect() calls in a ref, and update it less frequently?)
|
@ciampo Thanks for helping this through! Looks like an oversight in one of the places useState( () => getProps( contentElement, selectedBlockElement, toolbarHeight ) )to set the initial state lazily instead of: useState( getProps( contentElement, selectedBlockElement, toolbarHeight ) )(this only solves one of the calls. There are still two happening whenever switching blocks.) |
That's definitely a step in the right direction! I believe we can merge this PR and iterate in a lower-priority follow-up. We could also now have a look at reworking the props around the |
|
I realized that the props applied conditionally by the |
|
Thanks for addressing this little obnoxious issue :) |
What?
Fixes #41575
Restores the previous behavior where the block toolbar flips below the block when there's not enough space at the top of the canvas.
How?
Modifies the popover props on the fly. When there's enough space at the top of the canvas, the props
{ __unstableForcePosition: true, __unstableShift: true }are used, disabling the toolbar 'flip' behavior and enabling the 'shift' behavior.When there's a restricted amount of space, the props
{ __unstableForcePosition: false, __unstableShift: false }are used, which enable the 'flip' behavior and disable the 'shift' behavior.Testing Instructions
Video
Kapture.2022-08-02.at.17.36.26.mp4