Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Block toolbar: account for scrollable blocks that affect the position of the block toolbar #66188

Merged
merged 3 commits into from
Oct 21, 2024
+22 −2
Merged

Block toolbar: account for scrollable blocks that affect the position of the block toolbar #66188

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cecad1
The while that gets the rect bounds of the child, including the x and…
ramonjd Oct 17, 2024
4403bb9
Check for parent overflow only once.
ramonjd Oct 18, 2024
218ab99
Try a mash up between code suggestions.
ramonjd Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions packages/block-editor/src/utils/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,15 +140,37 @@ export function getVisibleElementBounds( element ) {
return new window.DOMRectReadOnly();
}

let bounds = element.getBoundingClientRect();
const parentRect = element.getBoundingClientRect();
let bounds = parentRect;

const stack = [ element ];
const xValues = [];
const yValues = [];
let currentElement;

while ( ( currentElement = stack.pop() ) ) {
for ( const child of currentElement.children ) {
if ( isElementVisible( child ) ) {
const childBounds = child.getBoundingClientRect();
yValues.push( childBounds.y );
xValues.push( childBounds.x );
/*
* If the child is larger than the parent, adjust the x and y values to the greatest known
* to account for any scrolling.
*/
if ( childBounds.width > parentRect.width ) {
childBounds.x = Math.max(
parentRect.x,
xValues.sort().at( -1 ) || 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, but if we stick with the idea of caching values, do we need arrays? Could we set let maxX = 0 and only update maxX if childBounds.x is greater than it?

);
}
if ( childBounds.height > parentRect.height ) {
childBounds.y = Math.max(
parentRect.y,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, this all makes me suspect that the original logic might be over-engineered? I'm not sure, so don't quote me.

What do we care about here? Whether any children overflow their parents.

A flat list of element.querySelector(*) might do the same job.

I don't really know to be honest 😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good point, this is pretty complex and I'm finding it hard to reason about making changes that feel stable.

What do we care about here? Whether any children overflow their parents.
A flat list of element.querySelector(*) might do the same job.
I don't really know to be honest 😆

The more I look at the logic the less I feel sure of how it should work, but that's also possibly because of being at the end of the week!

I like your thinking in general about simplification. Overall we're trying to see if any children extend beyond the bounds of the top-level container visually. It'd be great to be able to do that without having to walk the entire DOM hierarchy, which depending on the circumstances could be fairly complex.

A flat list of element.querySelector(*) might do the same job.

I suppose the question then is which is more performant: iterating over currentElement.children and adding to a stack, or using the querySelector. In my mind what we're doing is fairly similar in each case, though element.querySelector(*) might be slightly easier to read as it'd be a forEach instead of a nested for within a while 🤔

For now, I'd probably lean toward seeing if we can keep the current overall shape of the code in place while fixing the bug, but I do agree it's worth revisiting the overall complexity here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good point, this is pretty complex and I'm finding it hard to reason about making changes that feel stable.

Ditto.

For now, I'd probably lean toward seeing if we can keep the current overall shape of the code in place while fixing the bug, but I do agree it's worth revisiting the overall complexity here.

In my testing it appears latest update in 4403bb9 has reintroduced the issue where a vertically scrolling block's toolbar isn't hidden until after the height of the scrollable content.

Here's what I'm now seeing
Screen.Recording.2024-10-18.at.4.35.41.pm.mp4

Personally, I'm starting to feel as though we should ensure we have clarity around what we have to achieve, then perhaps start from there, working to the simplest solution rather than persist with the complex approach and hope to work back to a simpler option. In other words, if we're struggling to reason about the changes etc it could be hard to reach the point where we're confident enough that it could be refactored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm starting to feel as though we should ensure we have clarity around what we have to achieve, then perhaps start from there, working to the simplest solution rather than persist with the complex approach and hope to work back to a simpler option. In other words, if we're struggling to reason about the changes etc it could be hard to reach the point where we're confident enough that it could be refactored.

Also a very good point! No objections from me if we think we can come up with a simpler solution 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the "Friday brain is mush" disclaimer established, take the following with a grain of salt.

Do we need to treat children that are scrollable within the parent differently to one rendered "outside" the parent. The former should be restricted by the parent's bounds, right? The latter shouldn't be?

If that makes any sense, I appreciate that it isn't making things simpler as we'd hoped.

yValues.sort().at( -1 ) || 0
);
}

bounds = rectUnion( bounds, childBounds );
stack.push( child );
}
Expand All @@ -165,12 +187,12 @@ export function getVisibleElementBounds( element ) {
*/
const left = Math.max( bounds.left, 0 );
const right = Math.min( bounds.right, viewport.innerWidth );

bounds = new window.DOMRectReadOnly(
left,
bounds.top,
right - left,
bounds.height
);

return bounds;
}
Loading