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.

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

Position BlockToolbar below all of the selected block's descendants #62711

Merged
merged 10 commits into from
Aug 26, 2024
35 changes: 7 additions & 28 deletions packages/block-editor/src/components/block-popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
*/
import { useBlockElement } from '../block-list/use-block-props/use-block-refs';
import usePopoverScroll from './use-popover-scroll';
import { rectUnion, getVisibleElementBounds } from '../../utils/dom';

const MAX_POPOVER_RECOMPUTE_COUNTER = Number.MAX_SAFE_INTEGER;

Expand Down Expand Up @@ -87,34 +88,12 @@ function BlockPopover(

return {
getBoundingClientRect() {
const selectedBCR = selectedElement.getBoundingClientRect();
const lastSelectedBCR =
lastSelectedElement?.getBoundingClientRect();

// Get the biggest rectangle that encompasses completely the currently
// selected element and the last selected element:
// - for top/left coordinates, use the smaller numbers
// - for the bottom/right coordinates, use the largest numbers
const left = Math.min(
selectedBCR.left,
lastSelectedBCR?.left ?? Infinity
);
const top = Math.min(
selectedBCR.top,
lastSelectedBCR?.top ?? Infinity
);
const right = Math.max(
selectedBCR.right,
lastSelectedBCR.right ?? -Infinity
);
const bottom = Math.max(
selectedBCR.bottom,
lastSelectedBCR.bottom ?? -Infinity
);
const width = right - left;
const height = bottom - top;

return new window.DOMRect( left, top, width, height );
return lastSelectedElement
? rectUnion(
getVisibleElementBounds( selectedElement ),
getVisibleElementBounds( lastSelectedElement )
)
: getVisibleElementBounds( selectedElement );
},
contextElement: selectedElement,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,19 @@ export default function BlockToolbarPopover( {
isToolbarForcedRef.current = false;
} );

// If the block has a parent with __experimentalCaptureToolbars enabled,
// the toolbar should be positioned over the topmost capturing parent.
const clientIdToPositionOver = capturingClientId || clientId;

const popoverProps = useBlockToolbarPopoverProps( {
contentElement: __unstableContentRef?.current,
clientId,
clientId: clientIdToPositionOver,
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
} );

return (
! isTyping && (
<BlockPopover
clientId={ capturingClientId || clientId }
clientId={ clientIdToPositionOver }
bottomClientId={ lastClientId }
className={ clsx( 'block-editor-block-list__block-popover', {
'is-insertion-point-visible': isInsertionPointVisible,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
import { store as blockEditorStore } from '../../store';
import { useBlockElement } from '../block-list/use-block-props/use-block-refs';
import { hasStickyOrFixedPositionValue } from '../../hooks/position';
import { getVisibleElementBounds } from '../../utils/dom';

const COMMON_PROPS = {
placement: 'top-start',
Expand Down Expand Up @@ -67,7 +68,7 @@ function getProps(
// Get how far the content area has been scrolled.
const scrollTop = scrollContainer?.scrollTop || 0;

const blockRect = selectedBlockElement.getBoundingClientRect();
const blockRect = getVisibleElementBounds( selectedBlockElement );
const contentRect = contentElement.getBoundingClientRect();

// Get the vertical position of top of the visible content area.
Expand Down
107 changes: 107 additions & 0 deletions packages/block-editor/src/utils/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,110 @@ export function getBlockClientId( node ) {

return blockNode.id.slice( 'block-'.length );
}

/**
* Calculates the union of two rectangles, and optionally constrains this union within a containerRect's
* left and right values.
* The function returns a new DOMRect object representing this union.
*
* @param {DOMRect} rect1 First rectangle.
* @param {DOMRect} rect2 Second rectangle.
* @param {DOMRectReadOnly?} containerRect An optional container rectangle. The union will be clipped to this rectangle.
* @return {DOMRect} Union of the two rectangles.
*/
export function rectUnion( rect1, rect2, containerRect ) {
let left = Math.min( rect1.left, rect2.left );
let right = Math.max( rect1.right, rect2.right );
const bottom = Math.max( rect1.bottom, rect2.bottom );
const top = Math.min( rect1.top, rect2.top );

/*
* To calculate visible bounds using rectUnion, take into account the outer
* horizontal limits of the container in which an element is supposed to be "visible".
* For example, if an element is positioned -10px to the left of the window x value (0),
* this function discounts the negative overhang because it's not visible and
* therefore not to be counted in the visibility calculations.
* Top and bottom values are not accounted for to accommodate vertical scroll.
*/
if ( containerRect ) {
left = Math.max( left, containerRect.left );
right = Math.min( right, containerRect.right );
}

return new window.DOMRect( left, top, right - left, bottom - top );
}
Comment on lines +77 to +91
Copy link
Member Author

Choose a reason for hiding this comment

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

The approach makes sense but I don't really think this belongs in rectUnion. In a previous commit we had rectIntersect which we could bring back.

Copy link
Member Author

Choose a reason for hiding this comment

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


/**
* Returns whether an element is visible.
*
* @param {Element} element Element.
* @return {boolean} Whether the element is visible.
*/
function isElementVisible( element ) {
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
const viewport = element.ownerDocument.defaultView;
if ( ! viewport ) {
return false;
}

// Check for <VisuallyHidden> component.
if ( element.classList.contains( 'components-visually-hidden' ) ) {
return false;
}

const bounds = element.getBoundingClientRect();
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this if we're using checkVisibility?

Copy link
Member

Choose a reason for hiding this comment

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

Good question.

I believe so because this tests for elements that would otherwise be visible if not for the 0 dimensions.

E.g., for <div id="test" style="width:0;height:0;">Test</div>, checkVisibility( ...options ) returns true.

https://drafts.csswg.org/cssom-view-1/#dom-element-checkvisibility

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is supported in all the browsers that we support. I keep getting errors when using Safari.

Copy link
Contributor

@getdave getdave Sep 4, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Based on the browserslist config we support, we support the last 2 versions of Safari (17.5, 17.4). Both versions support checkVisibility according to caniuse.

Maybe the config we use is too loose if this is unexpected?

Copy link
Contributor

Choose a reason for hiding this comment

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

Safari only updates with MacOS updates for me. (stable MacOS) and I'm still on 17.3 for this reason. I think we might want to update our Safari browser list config to 3 or 4 versions because of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch — yeah, I think we should handle calling this optionally. Even if we officially support the latest two Safari versions, we shouldn't be in a situation where we're breaking the editor in older Safari versions. Here's what I'm seeing when I select a block in the post editor in Safari 16.4.1:

image

I've put up a quick fix PR here: #65069

if ( bounds.width === 0 || bounds.height === 0 ) {
return false;
}

return element.checkVisibility( {
opacityProperty: true,
contentVisibilityAuto: true,
visibilityProperty: true,
} );
}

/**
* Returns the rect of the element including all visible nested elements.
*
* Visible nested elements, including elements that overflow the parent, are
* taken into account.
*
* This function is useful for calculating the visible area of a block that
* contains nested elements that overflow the block, e.g. the Navigation block,
* which can contain overflowing Submenu blocks.
*
* The returned rect represents the full extent of the element and its visible
* children, which may extend beyond the viewport.
*
* @param {Element} element Element.
* @return {DOMRect} Bounding client rect of the element and its visible children.
*/
export function getVisibleElementBounds( element ) {
const viewport = element.ownerDocument.defaultView;
if ( ! viewport ) {
return new window.DOMRect();
}

let bounds = element.getBoundingClientRect();
const viewportRect = new window.DOMRectReadOnly(
0,
0,
viewport.innerWidth,
viewport.innerHeight
);

const stack = [ element ];
let currentElement;

while ( ( currentElement = stack.pop() ) ) {
for ( const child of currentElement.children ) {
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
if ( isElementVisible( child ) ) {
const childBounds = child.getBoundingClientRect();
bounds = rectUnion( bounds, childBounds, viewportRect );
stack.push( child );
}
}
}

return bounds;
}
Loading