Block Toolbar & Popover component - Prevent sticky position from causing permanently obscured areas of the selected block. #33981
Conversation
|
Size Change: +60 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
Addison-Stavlo
changed the title
Addison-Stavlo
requested review from
ajitbohra and
chrisvanpatten
as code owners
Addison-Stavlo
requested review from
jasmussen,
vindl,
jeyip,
mattwiebe,
ellatrix,
Copons,
creativecoder and
david-szabo97
|
Nice 🔥 — this is working really well for the header in the site editor, and I feel validates the initial idea:
The behavior change does surface a side effect that might not be ideal: the toolbar changes position even if it started at the top, which feels jumpy in the post editor:
For comparison, before it was sticky:
Can we tweak the heuristic so that the block toolbar only ever shows up below the block if that's where it started? I.e. once you select the block, its permanent position thereafter is chosen:
What do you think? Nice work. |
|
Thanks for taking a look @jasmussen !
A little jumpy, yes. But I actually really like this behavior. The idea is that the toolbar does not impair any view or interaction with the corresponding block unless there is no other option and it is completely necessary to do so. To me, a small jump of the popover is much more preferable than having something stuck in the way of the item I am trying to edit. While jumpiness of items in the editor has been a concern, I think the jumpiness of a popover seems minor and much more forgivable than the sort of jumpiness we have seen in the past that actually causes a shift in the layout of blocks in the editor canvas itself.
Besides impairing the block when unnecessary, I think this idea has a handful of challenges technically and im not really sure how we would go about doing so. Right now, the popover component has no awareness of the previous states of the editor, and makes its calculations based off the current point in time. This makes sense for a general component. I can't currently think of a good way to add this sort of awareness and control without conflating concerns across package boundaries, inflating data stores, and still requiring a lot more complex logic hammered out around all possible edge cases. Although, maybe there is a much easier way to go about this that I am missing. Right now the approach is reflexive to the current state of the editor at any given point in time. As a result the logic is very simple, well contained, simple to maintain, and has a much lower probability to introduce any bugs. If we were to move away from the reflexive approach and towards the more controlled approach based off previous editor states I think the scope of this will inflate very quickly, have much more complicated logic that is more difficult to maintain, require specific handling for every edge case, and have a much higher probability of unexpected bugs. |
|
Good thoughts. I agree that if at all possible we should avoid obscuring content, and it's why the Top Toolbar option remains. In this case however, moving the block toolbar below a short paragraph just obscures the next paragraph instead of the preceeding one. The behavior is definitely valid for the popover component in cases where it means opening dropdown menus and the like. In the case of the block toolbar, my understanding is that it leveraging the popover is mostly a matter of extracting it from the DOM (to enable things like the iframe and such). I understand that writing a unique behavior just for this part of the interface is less optimal than a generic solution that works for the component as a whole, but being such a primary interface for the editor, I think it's important that we find a way forward that avoids the jump when it isn't necessary. Is there any path forward that isn't too complicated that would allow us to keep the sticky/scrolling behavior for a toolbar that starts above the block? Could it be a prop to toggle the behavior? For what it's worth, @ellatrix has had a desire to move additional block UI from the editing canvas into popovers, including for example the in-canvas inserter. Such a change would very probably leverage the same behavior. |
Possibly! I need more time to think about it and investigate. Often the simple solutions don't emerge until I have spent too much time thinking about the overly complicated ones. 😆 Il keep digging.
Yes, but its better to obscure the block you aren't currently editing than the block that you are trying to edit. Obviously I really prefer the jumpiness of popover when scrolling offscreen to obscuring the selected block, but Im happy to continue to investigate your proposed behavior as well.
This behavior is actually only applied when the |
annezazu
added
[Package] Interface
|
@jasmussen - I think thats a bit closer to what you were suggesting. Theres still some jumpyness to take care of at the bottom of the editor, when the toolbar starts below the block and you scroll down so it moves off the page. But if we leverage another stickyPosition at the bottom of the editor we could make it sticky there until it gets to a point where it makes sense resetting to the top position. 🤔 |
|
Updated with a bottom sticky position, so I think this has gotten rid of all the jumpiness. When the block popover mounts if it is initially in a state that would put it in sticky position it is moved underneath the block instead, otherwise it just retains the previous existing behavior. It remains in this bottom location until we scroll down far enough that the toolbar moves underneath the page, at this point it stays at the new bottom sticky position. If we continue to scroll such that the block moves off the screen, the popover is then reset to have the default behavior (top of block positioning):
Theres 1 more edge case that I am noticing at the moment. If a block is tall enough that neither its top or bottom boundaries are in the editor, selecting the block defaults the toolbar to the bottom sticky position instead of the top. I assume we want this to be at the top sticky position since that is the current behavior, Anyways, let me know how the current state of the PR feels to you. And if you happen to stumble across any other edge cases 😆 . |
|
Thanks so much for sticking on this. I still think it's a powerful feature. Here's what I see:
This is working much better than before, especially for basic writing flows. However I think we might still want to go with the behavior where if it starts above the block, always keep it there, if it starts below the block, allow it to flip above the block if space becomes available. The reason is sort of shown in the GIF itself — you might select a block where it starts above, then scroll, deselect and select again. Suddenly the block toolbar is below the block, even if you scroll up, which makes it appear as if it belongs to the block below. That makes it feel important to have "above the block" be the canonical and default place for the toolbar, and then have the other cases be edgecases that revert back to that as soon as possible. What do you think? |
|
Nice one, this is what I see:
I think this is pretty good and definitely close. I would appreciate more opinions, though. If I was to change a single thing, it would probably be related to keeping the toolbar above the block as much as we possibly can. In this case, the behavior is now right for the viewport, in that if there isn't space to see the toolbar perfectly above the block where you scrolled to, it will be below the block. But if we could change that so it's relative to the document itself, I think it might be even better, so:
What do you think? |
I think that makes sense, it will keep things very consistent yet solve the problem for blocks that don't have any room above them regardless of scroll position. 👍 |
|
@jasmussen how does this feel currently? I think its close to what you were describing. While this behavior is probably on the more simple end of our iterations, its the only one so far that has required updates to other packages as well due to needing to access the content wrapper for the editor canvas. Here we set up a new section in the block-editor store for the canvas wrapper element. It is set when the iframe component initializes the class on the content wrapper, then selected from the store when we pass props from block-popover to the actual popover component which then uses it for evaluation. Back to discussing edge case about the exceedingly tall block as the first item in the editor: At this point if there isn't visual room below the block it defaults to normal behavior (sets the toolbar in the top sticky position) and has some jumpiness when you scroll as the popover shifts to the bottom of the block once there is room. This doesn't seem correct as 1 - it has the unnecessary jumpiness and 2 - the top section of the block is always obscured when it is visible and the block is selected. Im thinking for this edge case what might make sense is adding that bottom sticky position back in, then the popover will be sticky at the bottom position until there is visually room beneath the block. This would both get rid of the jumpiness as well as allow every section of the block to be un-obscurable. Its a bit different than our default behavior to have it sticky at the bottom of the editor, but it seems like it might make sense to solve the problems in that case. Thoughts? |
|
Awesome, this is what I see:
To me, this threads the needle and fixes the original issue. I know it's somewhat conservative, but it seems safer to start that way and then expand as we go.
Indeed. I wasn't immediately able to reproduce, even if I tried with a very tall group. Can you provide a GIF of what you're seeing? It's hard to know exactly what feels the most right there. Instinctively I'd lean conservative and suggest that if there isn't a good obvious place below the block (like when selecting the header in the FSE example above), then we should always defer to the default behavior. But I might be missing a beat. Thanks so much for working on this 👌 |
Of course!
The concern I have here is that the top section of the block (what is underneath the block toolbar when we are scrolled all the way to the top), is always obscured when it is in view. This could prevent users from being able to interact with a child block in that location or other interfaces that may exist with really no way to get around it. If we added a bottom-of-editor sticky position that would only be seen in this odd edge case, then all sections of the block will be free and unobscured at some point in the available scrolling positions. |
|
Oh that's a good one, thank you for the GIF that was very helpful. So basically, if the topmost block is taller than the viewport, we get this. I feel like we might want to go for the simplest solution here just so the benefits of this PR can land for what's probably the majority of cases, and depending on how often we encounter that case we can revisit. I understand the complexity this adds to the positioning behavior already, so I'm definitely tempted to not want to add further to it. But if a "bottom sticky" feature as you describe can be built without adding too much complexity, and leverage that behavior only for this case, when the block is at the top of the document but also taller than the viewport, then it seems like it could work well. |
Its definitely an addition that doesnt require much complexity. About a +2 -1 loc change that makes some of the placement logic a little more simple too. 03404b1 That situation would look like this now:
So if there is no room above the block in the canvas (regardless of scroll - so basically just the top block), we just return the minimum (higher in editor) position between the bottom of the block and bottom sticky position. Thus the bottom sticky position should only be seen if both the block is at the very top of the editor canvas and is taller than the viewport. |
|
Just from a glance, this looks to me like it could work, nice. Seems like this one mostly needs a code review, and then it'd be nice to try it! 👍 👍 |
Addison-Stavlo
changed the title
Awesome, agreed! Updated PR description and this should be ready for review. |
There was a problem hiding this comment.
Thanks @Addison-Stavlo, this works great! ❤️
I've left a few comments, almost all minor.
My major concern is storing the iframe ref in the Redux store. I guess it's not a big deal per se, but I don't recall other similar examples, and I wonder if there are alternative ways to achieve the same thing. 🤔
packages/block-editor/src/components/block-tools/block-popover.js
Outdated
Show resolved
Hide resolved
|
Nice work @Addison-Stavlo ! I really appreciate the attention to detail here in refining the toolbar placement ❤️ |
Description
Attempts to resolve #29464 by moving the popover beneath the block if there is no room in the editor canvas to ever allow non-sticky above block positioning for said block.
Currently (on trunk), if a block is at the top of the editor canvas the block toolbar can never be above the block and is always set to the top sticky position. This can cause many conflicts with trying to perform standard actions such as interacting with block content, child blocks, placeholder interfaces, etc. by creating a permanently obscured area of the block whenever it is selected.
In this PR, if the sticky position is enabled we check to see if there is enough room above the block for the popover to ever be at its default non-sticky position. If there is, we default to current standard behavior. If there is not, we place the popover beneath the block to eliminate permanently obscured areas of the block caused by the popover toolbar at the top sticky position.
How has this been tested?
Verify toolbar does not overlap block when possible:
Similarly, select another block further down in the editor. Verify the toolbar is above the block as per the current expectation.
Test the above in the template editor as well.
Smoke test the post editor, there should be no changes to behavior.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).