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 all 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
24 changes: 22 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 @@ -118,6 +118,22 @@ function isElementVisible( element ) {
return true;
}

/**
* Checks if the element is scrollable.
*
* @param {Element} element Element.
* @return {boolean} True if the element is scrollable.
*/
function isScrollable( element ) {
const style = window.getComputedStyle( element );
return (
style.overflowX === 'auto' ||
style.overflowX === 'scroll' ||
style.overflowY === 'auto' ||
style.overflowY === 'scroll'
);
}

/**
* Returns the rect of the element including all visible nested elements.
*
Expand All @@ -136,19 +152,23 @@ function isElementVisible( element ) {
*/
export function getVisibleElementBounds( element ) {
const viewport = element.ownerDocument.defaultView;

if ( ! viewport ) {
return new window.DOMRectReadOnly();
}

let bounds = element.getBoundingClientRect();

const stack = [ element ];
let currentElement;

while ( ( currentElement = stack.pop() ) ) {
for ( const child of currentElement.children ) {
if ( isElementVisible( child ) ) {
const childBounds = child.getBoundingClientRect();
let childBounds = child.getBoundingClientRect();
// If the parent is scrollable, use parent's scrollable bounds.
if ( isScrollable( currentElement ) ) {
childBounds = currentElement.getBoundingClientRect();
}
Comment on lines +167 to +171
Copy link
Contributor

@stokesman stokesman Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good work on the solution and nice collaboration here. While I was tinkering around on solutions for #66438, I noticed what seems to be a minor issue here so I’m leaving this late review comment.

I know scrollable elements aren’t expected to be a common case but from what I’ve tested this is wasteful. It loops over all their children yet gets and unions the bounds from the same element each time. I spun up #66546 to address it and included some instructions on testing my claim here. I haven’t requested reviews from anyone yet since I don’t think there’s any urgency to this but I thought it better to mention while still pretty fresh.

bounds = rectUnion( bounds, childBounds );
stack.push( child );
}
Expand Down
Loading