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 popover: allow scrolling over #19769

Merged
merged 4 commits into from
Jan 22, 2020
Merged

Block popover: allow scrolling over #19769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29d136f
Block popover: allow scrolling over
ellatrix Jan 20, 2020
d699a38
Clean up
ellatrix Jan 21, 2020
7dc5d83
Fix overlapping inserter popover
ellatrix Jan 21, 2020
57ace36
Better comment
ellatrix Jan 22, 2020
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
3 changes: 3 additions & 0 deletions packages/base-styles/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ $z-layers: (
// Above the block list, under the header.
".block-editor-block-list__block-popover": 29,

// Under the block popover (block toolbar).
".block-editor-block-list__insertion-point-popover": 28,

// Show snackbars above everything (similar to popovers)
".components-snackbar-list": 100000,

Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-list/insertion-point.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export default function InsertionPoint( {
anchorRef={ inserterElement }
position="top right left"
focusOnMount={ false }
className="block-editor-block-list__block-popover"
className="block-editor-block-list__insertion-point-popover"
__unstableSlotName="block-toolbar"
>
<div className="block-editor-block-list__insertion-point" style={ { width: inserterElement.offsetWidth } }>
Expand Down
13 changes: 13 additions & 0 deletions packages/block-editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,8 +537,21 @@
}
}

.block-editor-block-list__insertion-point-popover {
z-index: z-index(".block-editor-block-list__insertion-point-popover");
position: absolute;

.components-popover__content {
background: none;
border: none;
box-shadow: none;
overflow-y: visible;
}
}

.components-popover.block-editor-block-list__block-popover {
z-index: z-index(".block-editor-block-list__block-popover");
position: absolute;

.components-popover__content {
margin: 0 !important;
Expand Down
25 changes: 23 additions & 2 deletions packages/components/src/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ const Popover = ( {
if ( ! containerRef.current || ! contentRef.current ) {
return;
}
const anchor = computeAnchorRect(

let anchor = computeAnchorRect(
anchorRefFallback,
anchorRect,
getAnchorRect,
Expand All @@ -273,14 +274,34 @@ const Popover = ( {
contentRect.current = contentRef.current.getBoundingClientRect();
}

const { offsetParent, ownerDocument } = containerRef.current;
let relativeOffsetTop = 0;

// If there is a positioned ancestor element that is not the body,
// subtract the position from the anchor rect. If the position of
// the popover is fixed, the offset parent is null or the body
// element, in which case the position is relative to the viewport.
// See https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent
if ( offsetParent && offsetParent !== ownerDocument.body ) {
const offsetParentRect = offsetParent.getBoundingClientRect();

relativeOffsetTop = offsetParentRect.top;
anchor = new window.DOMRect(
anchor.left - offsetParentRect.left,
anchor.top - offsetParentRect.top,
anchor.width,
anchor.height
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this assumes the parent is relatively positioned. Should it be opt-in, won't this break popovers if it's not the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a check for offsetParent. That should be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent

Interesting that offsetParent does seem to return the body element in Firefox. I'll adjust the check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can check additionally if the position is absolute.

Copy link
Member

Choose a reason for hiding this comment

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

Does the logic here depend that the developer apply a style to their popover to position it absolutely? Is that what you're saying by checking if the position is absolute? Otherwise, it doesn't seem very self-contained, or clear why this logic would exist here if it's something only relevant for custom popover behavior. Can we apply position: absolute within this component when it's appropriate? Is there any opportunity to be be smart about detecting that the popover is being rendered in a scrollable container?

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay on the point of introducing more formal support later. It's not entirely clear to me if I would expect some impact of these changes on existing usage which doesn't use the absolute-positioned approach. You mentioned earlier "There's a check for offsetParent", but I don't understand specifically how that's meant to help.

In any case, it seems from some brief testing that things work well enough.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it assumes that elsewhere, the Slot is rendered such that there wouldn't be an offsetParent ? Technically we also support an "in-place rendering" option, when there is no Slot context available. I don't think we use it anywhere ourselves. But it seems it could be impacted by these changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

In all other cases, the popover has fixed positioning, even when rendered in place, which means that it is offset relative to the viewport and that it doesn't have an offset parent. When the popover has absolute positioning, the offset parent is the first ancestor that has a relative position.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. I think I misunderstood how offsetParent was being used here. I see now it's used as an indicator of the component itself being fixed, so only if someone went out of their way to manually override the fixed property would it ever have an impact 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the comment could be clearer. 😄


const {
popoverTop,
popoverLeft,
xAxis,
yAxis,
contentHeight,
contentWidth,
} = computePopoverPosition( anchor, contentRect.current, position, __unstableSticky, anchorRef );
} = computePopoverPosition( anchor, contentRect.current, position, __unstableSticky, anchorRef, relativeOffsetTop );

if ( typeof popoverTop === 'number' && typeof popoverLeft === 'number' ) {
if ( subpixels && __unstableAllowVerticalSubpixelPosition ) {
Expand Down
46 changes: 25 additions & 21 deletions packages/components/src/popover/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,18 +111,20 @@ export function computePopoverXAxisPosition( anchorRect, contentSize, xAxis, cor
/**
* Utility used to compute the popover position over the yAxis
*
* @param {Object} anchorRect Anchor Rect.
* @param {Object} contentSize Content Size.
* @param {string} yAxis Desired yAxis.
* @param {string} corner Desired corner.
* @param {boolean} sticky Whether or not to stick the popover to the
* scroll container edge when part of the anchor
* leaves view.
* @param {Element} anchorRef The anchor element.
* @param {Object} anchorRect Anchor Rect.
* @param {Object} contentSize Content Size.
* @param {string} yAxis Desired yAxis.
* @param {string} corner Desired corner.
* @param {boolean} sticky Whether or not to stick the popover to the
* scroll container edge when part of the
* anchor leaves view.
* @param {Element} anchorRef The anchor element.
* @param {Element} relativeOffsetTop If applicable, top offset of the relative
* positioned parent container.
*
* @return {Object} Popover xAxis position and constraints.
*/
export function computePopoverYAxisPosition( anchorRect, contentSize, yAxis, corner, sticky, anchorRef ) {
export function computePopoverYAxisPosition( anchorRect, contentSize, yAxis, corner, sticky, anchorRef, relativeOffsetTop ) {
const { height } = contentSize;

if ( sticky ) {
Expand All @@ -146,7 +148,7 @@ export function computePopoverYAxisPosition( anchorRect, contentSize, yAxis, cor
if ( topRect.top - height <= scrollRect.top ) {
return {
yAxis,
popoverTop: Math.min( bottomRect.bottom, scrollRect.top + height ),
popoverTop: Math.min( bottomRect.bottom - relativeOffsetTop, scrollRect.top + height - relativeOffsetTop ),
};
}
}
Expand Down Expand Up @@ -212,23 +214,25 @@ export function computePopoverYAxisPosition( anchorRect, contentSize, yAxis, cor
}

/**
* Utility used to compute the popover position and the content max width/height for a popover
* given its anchor rect and its content size.
* Utility used to compute the popover position and the content max width/height
* for a popover given its anchor rect and its content size.
*
* @param {Object} anchorRect Anchor Rect.
* @param {Object} contentSize Content Size.
* @param {string} position Position.
* @param {boolean} sticky Whether or not to stick the popover to the
* scroll container edge when part of the anchor
* leaves view.
* @param {Element} anchorRef The anchor element.
* @param {Object} anchorRect Anchor Rect.
* @param {Object} contentSize Content Size.
* @param {string} position Position.
* @param {boolean} sticky Whether or not to stick the popover to the
* scroll container edge when part of the
* anchor leaves view.
* @param {Element} anchorRef The anchor element.
* @param {number} relativeOffsetTop If applicable, top offset of the relative
* positioned parent container.
*
* @return {Object} Popover position and constraints.
*/
export function computePopoverPosition( anchorRect, contentSize, position = 'top', sticky, anchorRef ) {
export function computePopoverPosition( anchorRect, contentSize, position = 'top', sticky, anchorRef, relativeOffsetTop ) {
const [ yAxis, xAxis = 'center', corner ] = position.split( ' ' );

const yAxisPosition = computePopoverYAxisPosition( anchorRect, contentSize, yAxis, corner, sticky, anchorRef );
const yAxisPosition = computePopoverYAxisPosition( anchorRect, contentSize, yAxis, corner, sticky, anchorRef, relativeOffsetTop );
const xAxisPosition = computePopoverXAxisPosition( anchorRect, contentSize, xAxis, corner, sticky, yAxisPosition.yAxis );

return {
Expand Down
1 change: 0 additions & 1 deletion packages/edit-post/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ function Layout() {
content={
<>
<EditorNotices />
<Popover.Slot name="block-toolbar" />
{ ( mode === 'text' || ! isRichEditingEnabled ) && <TextEditor /> }
{ isRichEditingEnabled && mode === 'visual' && <VisualEditor /> }
<div className="edit-post-layout__metaboxes">
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
__experimentalBlockSettingsMenuFirstItem,
__experimentalBlockSettingsMenuPluginsExtension,
} from '@wordpress/block-editor';
import { Popover } from '@wordpress/components';

/**
* Internal dependencies
Expand All @@ -28,6 +29,7 @@ function VisualEditor() {
<BlockSelectionClearer className="edit-post-visual-editor editor-styles-wrapper">
<VisualEditorGlobalKeyboardShortcuts />
<MultiSelectScrollIntoView />
<Popover.Slot name="block-toolbar" />
Copy link
Member Author

Choose a reason for hiding this comment

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

.edit-post-visual-editor provides the relative positioned container within the scroll container that the popover needs.

<Typewriter>
<WritingFlow>
<ObserveTyping>
Expand Down