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

Popover: consistently adjust position on scroll #17867

Merged
merged 4 commits into from
Nov 27, 2019
Merged

Popover: consistently adjust position on scroll #17867

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3af3643
Popover: consistently adjust position on scroll
ellatrix Nov 27, 2019
e4f9eb6
Fix docs
ellatrix Nov 27, 2019
0114ac4
Fix anchorRef doc
ellatrix Nov 27, 2019
9fd86c7
Fix unit test
ellatrix Nov 27, 2019
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
29 changes: 3 additions & 26 deletions packages/block-editor/src/components/rich-text/format-toolbar-container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,16 @@ import { Popover } from '@wordpress/components';
import BlockFormatControls from '../block-format-controls';
import FormatToolbar from './format-toolbar';

function getAnchorRect( anchorObj ) {
const { current } = anchorObj;
const rect = current.getBoundingClientRect();

// Add some space.
const buffer = 6;

// Subtract padding if any.
let { paddingTop } = window.getComputedStyle( current );

paddingTop = parseInt( paddingTop, 10 );

return {
x: rect.left,
y: rect.top + paddingTop - buffer,
width: rect.width,
height: rect.height - paddingTop + buffer,
left: rect.left,
right: rect.right,
top: rect.top + paddingTop - buffer,
bottom: rect.bottom,
};
}

const FormatToolbarContainer = ( { inline, anchorObj } ) => {
const FormatToolbarContainer = ( { inline, anchorRef } ) => {
if ( inline ) {
// Render in popover
return (
<Popover
noArrow
position="top center"
focusOnMount={ false }
getAnchorRect={ () => getAnchorRect( anchorObj ) }
anchorVerticalBuffer={ 6 }
anchorRef={ anchorRef }
className="block-editor-rich-text__inline-format-toolbar"
>
<FormatToolbar />
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ class RichTextWrapper extends Component {
{ ( { isSelected, value, onChange, Editable } ) =>
<>
{ children && children( { value, onChange } ) }
{ isSelected && hasFormats && ( <FormatToolbarContainer inline={ inlineToolbar } anchorObj={ this.ref } /> ) }
{ isSelected && hasFormats && ( <FormatToolbarContainer inline={ inlineToolbar } anchorRef={ this.ref.current } /> ) }
{ isSelected && <RemoveBrowserShortcuts /> }
<Autocomplete
onReplace={ onReplace }
Expand Down
11 changes: 3 additions & 8 deletions packages/components/src/autocomplete/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
isCollapsed,
getTextContent,
} from '@wordpress/rich-text';
import { getRectangleFromRange } from '@wordpress/dom';

/**
* Internal dependencies
Expand Down Expand Up @@ -130,13 +129,9 @@ function filterOptions( search, options = [], maxResults = 10 ) {
return filtered;
}

function getCaretRect() {
function getRange() {
const selection = window.getSelection();
const range = selection.rangeCount ? selection.getRangeAt( 0 ) : null;

if ( range ) {
return getRectangleFromRange( range );
}
return selection.rangeCount ? selection.getRangeAt( 0 ) : null;
}

export class Autocomplete extends Component {
Expand Down Expand Up @@ -426,7 +421,7 @@ export class Autocomplete extends Component {
onClose={ this.reset }
position="top right"
className="components-autocomplete__popover"
getAnchorRect={ getCaretRect }
anchorRef={ getRange() }
>
<div
id={ listBoxId }
Expand Down
184 changes: 142 additions & 42 deletions packages/components/src/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { useRef, useState, useEffect } from '@wordpress/element';
import { focus } from '@wordpress/dom';
import { focus, getRectangleFromRange } from '@wordpress/dom';
import { ESCAPE } from '@wordpress/keycodes';
import isShallowEqual from '@wordpress/is-shallow-equal';
import deprecated from '@wordpress/deprecated';
Expand Down Expand Up @@ -62,48 +62,135 @@ function useThrottledWindowScrollOrResize( handler, ignoredScrollableRef ) {
}, [] );
}

function computeAnchorRect(
anchorRefFallback,
anchorRect,
getAnchorRect,
anchorRef = false,
shouldAnchorIncludePadding
) {
if ( anchorRect ) {
return anchorRect;
}

if ( getAnchorRect ) {
if ( ! anchorRefFallback.current ) {
return;
}

return getAnchorRect( anchorRefFallback.current );
}

if ( anchorRef !== false ) {
if ( ! anchorRef ) {
return;
}

if ( anchorRef instanceof window.Range ) {
return getRectangleFromRange( anchorRef );
}

const rect = anchorRef.getBoundingClientRect();

if ( shouldAnchorIncludePadding ) {
return rect;
}

return withoutPadding( rect, anchorRef );
}

if ( ! anchorRefFallback.current ) {
return;
}

const { parentNode } = anchorRefFallback.current;
const rect = parentNode.getBoundingClientRect();

if ( shouldAnchorIncludePadding ) {
return rect;
}

return withoutPadding( rect, parentNode );
}

function addBuffer( rect, verticalBuffer = 0, horizontalBuffer = 0 ) {
return {
x: rect.left - horizontalBuffer,
y: rect.top - verticalBuffer,
width: rect.width + ( 2 * horizontalBuffer ),
height: rect.height + ( 2 * verticalBuffer ),
left: rect.left - horizontalBuffer,
right: rect.right + horizontalBuffer,
top: rect.top - verticalBuffer,
bottom: rect.bottom + verticalBuffer,
};
}

function withoutPadding( rect, element ) {
const {
paddingTop,
paddingBottom,
paddingLeft,
paddingRight,
} = window.getComputedStyle( element );
const top = paddingTop ? parseInt( paddingTop, 10 ) : 0;
const bottom = paddingBottom ? parseInt( paddingBottom, 10 ) : 0;
const left = paddingLeft ? parseInt( paddingLeft, 10 ) : 0;
const right = paddingRight ? parseInt( paddingRight, 10 ) : 0;

return {
x: rect.left + left,
y: rect.top + top,
width: rect.width - left - right,
height: rect.height - top - bottom,
left: rect.left + left,
right: rect.right - right,
top: rect.top + top,
bottom: rect.bottom - bottom,
};
}

/**
* Hook used to compute and update the anchor position properly.
*
* @param {Object} anchorRef reference to the popover anchor element.
* @param {Object} contentRef reference to the popover content element.
* @param {Object} anchorRect anchor Rect prop used to override the computed value.
* @param {Function} getAnchorRect function used to override the anchor value computation algorithm.
* @param {Object} anchorRefFallback Reference to the popover anchor fallback element.
* @param {Object} contentRef Reference to the popover content element.
* @param {Object} anchorRect Anchor Rect prop used to override the computed value.
* @param {Function} getAnchorRect Function used to override the anchor value computation algorithm.
* @param {Element|Range} anchorRef A live element or range reference.
* @param {boolean} shouldAnchorIncludePadding Whether to include the anchor padding.
* @param {number} anchorVerticalBuffer Vertical buffer for the anchor.
* @param {number} anchorHorizontalBuffer Horizontal buffer for the anchor.
*
* @return {Object} Anchor position.
*/
function useAnchor( anchorRef, contentRef, anchorRect, getAnchorRect ) {
function useAnchor(
anchorRefFallback,
contentRef,
anchorRect,
getAnchorRect,
anchorRef,
shouldAnchorIncludePadding,
anchorVerticalBuffer,
anchorHorizontalBuffer
) {
const [ anchor, setAnchor ] = useState( null );
const refreshAnchorRect = () => {
if ( ! anchorRef.current ) {
return;
}
let newAnchor = computeAnchorRect(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see much difference betweencomputeAnchorRect and useAnchor. I mean we could just inline it here. What's the reasoning?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's clearer to me to make early returns than to wrap inside else blocks. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

And to return a value to a constant instead of assigning it to a variable.

anchorRefFallback,
anchorRect,
getAnchorRect,
anchorRef,
shouldAnchorIncludePadding
);

let newAnchor;
if ( anchorRect ) {
newAnchor = anchorRect;
} else if ( getAnchorRect ) {
newAnchor = getAnchorRect( anchorRef.current );
} else {
const rect = anchorRef.current.parentNode.getBoundingClientRect();
// subtract padding
const { paddingTop, paddingBottom } = window.getComputedStyle( anchorRef.current.parentNode );
const topPad = parseInt( paddingTop, 10 );
const bottomPad = parseInt( paddingBottom, 10 );
newAnchor = {
x: rect.left,
y: rect.top + topPad,
width: rect.width,
height: rect.height - topPad - bottomPad,
left: rect.left,
right: rect.right,
top: rect.top + topPad,
bottom: rect.bottom - bottomPad,
};
}
newAnchor = addBuffer(
newAnchor,
anchorVerticalBuffer,
anchorHorizontalBuffer
);

const didAnchorRectChange = ! isShallowEqual( newAnchor, anchor );
if ( didAnchorRectChange ) {
if ( ! isShallowEqual( newAnchor, anchor ) ) {
setAnchor( newAnchor );
}
};
Expand Down Expand Up @@ -154,11 +241,11 @@ function useInitialContentSize( ref ) {
* Hook used to compute and update the position of the popover
* based on the anchor position and the content size.
*
* @param {Object} anchor Anchor Position.
* @param {Object} contentSize Content Size.
* @param {string} position Position prop.
* @param {boolean} expandOnMobile Whether to show the popover full width on mobile.
* @param {Object} contentRef Reference to the popover content element.
* @param {Object} anchor Anchor Position.
* @param {Object} contentSize Content Size.
* @param {string} position Position prop.
* @param {boolean} expandOnMobile Whether to show the popover full width on mobile.
* @param {Object} contentRef Reference to the popover content element.
*
* @return {Object} Popover position.
*/
Expand Down Expand Up @@ -206,7 +293,7 @@ function usePopoverPosition( anchor, contentSize, position, expandOnMobile, cont
* Hook used to focus the first tabbable element on mount.
*
* @param {boolean|string} focusOnMount Focus on mount mode.
* @param {Object} contentRef Reference to the popover content element.
* @param {Object} contentRef Reference to the popover content element.
*/
function useFocusContentOnMount( focusOnMount, contentRef ) {
// Focus handling
Expand Down Expand Up @@ -259,6 +346,10 @@ const Popover = ( {
position = 'top',
range,
focusOnMount = 'firstElement',
anchorRef,
shouldAnchorIncludePadding,
anchorVerticalBuffer,
anchorHorizontalBuffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should have a single prop instead with different options? anchor = { ref, padding, buffer }

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 don't like nesting that much for component props. That would break shallow comparison, right? Maybe we can name them differently...

Copy link
Contributor

Choose a reason for hiding this comment

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

It does break shallow comparison. we can use things like useMemo to avoid that but I'm not sure if it's worth it.

I like the approach of this PR, maybe we can get more thoughts on props naming here @mcsf

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how other popover libraries or design systems name/handle the positioning options. cc @ItsJonQ

Copy link

Choose a reason for hiding this comment

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

@youknowriad + @ellatrix 👋 Halloo!

A popular popover/positioner library folks use (under the hood) is Popper.js

Their React implementation uses the main library and share the same API.
They use nested objects, example:

offsets.popper
top, left, width, height values

data.offsets.reference
top, left, width, height values

I've used both shallow and nested approaches before. Both have their pros and cons :).

Typically component library components/primitives have lots of props. Way more compared to components created at the app level.

If this were going into@wordpress/components to update the Popover, I would vote for @youknowriad 's suggestion with nested.

However, since this is updating an "application" component, something that typically won't be consumed externally and used to build interfaces... I would agree with you @ellatrix, and vote for shallow.

Hope this helps! <3

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've used both shallow and nested approaches before. Both have their pros and cons :).

Could you elaborate? What are the pros of nesting? I'm glad to see that Popper also requires an element reference, just as I'm proposing here.

Copy link

Choose a reason for hiding this comment

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

@ellatrix

I'm glad to see that Popper also requires an element reference, just as I'm proposing here.

❤️

A pro of nesting would be that it keeps related props grouped together.
Also makes them easier to work with (IMO) those props are passed within a render prop.

I would only nest 1 level deep (2 max!!! if you really really need to)

An example would be react-select. The components prop is a good one. It allows the user to pass in custom components to replace their sub-components (e.g. Menu). If that wasn't there, they might need to define something like componentMenu, componentInput, etc...

It's not the end of the world. But that library has a ton of props. I think this keeps it tidier :)

That being said.. if one decides to nest, they'll need to do things like (as @youknowriad mentioned) memoize. Also to do some sort of defaultValues + userDefinedValues merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

useMemo puts extra responsibilities on the implementor. Not sure if it's worth it for these few props.

Copy link

Choose a reason for hiding this comment

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

🤦‍♂ My apologies! I didn't realize these changes were on @wordpress/components/popover.
(It was early for me)

useMemo puts extra responsibilities on the implementor.

I agree. These primitives should take care of this overhead.

If we were to use an object, would exporting it like this help?

import { useRef, useState, useEffect, memo } from '@wordpress/element';
...
const PopoverContainer = memo(Popover);

I'm guessing not, since memo only does shallow comparisons.

Not sure if it's worth it for these few props.

Given its current state, I think it might be better to keep it simple (and shallow).
My concern was maybe later the number of props + names of them may get unwieldy, but that may not happen :).

Apologies for the back and forth!

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, the interface provider can always manage itself with shouldComponentUpdate, and I get the benefit of keeping things tidy with nested props.

The question is probably how much we expect these interfaces to grow, since we aren't building an all-encompassing popover library. My first impression is that it's probably not worth the extra complexity to have nested props.

I like the approach of this PR, maybe we can get more thoughts on props naming here @mcsf

I think this PR is good. I would just double-check that the props are named in a way that conveys their type well, as pointed out in https://github.com/WordPress/gutenberg/pull/17867/files#r351238110.

anchorRect,
getAnchorRect,
expandOnMobile,
Expand All @@ -268,14 +359,23 @@ const Popover = ( {
/* eslint-enable no-unused-vars */
...contentProps
} ) => {
const anchorRef = useRef( null );
const anchorRefFallback = useRef( null );
const contentRef = useRef( null );

// Animation
const [ isReadyToAnimate, setIsReadyToAnimate ] = useState( false );

// Anchor position
const anchor = useAnchor( anchorRef, contentRef, anchorRect, getAnchorRect );
const anchor = useAnchor(
anchorRefFallback,
contentRef,
anchorRect,
getAnchorRect,
anchorRef,
shouldAnchorIncludePadding,
anchorVerticalBuffer,
anchorHorizontalBuffer
);

// Content size
const contentSize = useInitialContentSize( contentRef );
Expand Down Expand Up @@ -438,7 +538,7 @@ const Popover = ( {
}

return (
<span ref={ anchorRef }>
<span ref={ anchorRefFallback }>
{ content }
{ popoverPosition.isMobile && expandOnMobile && <ScrollLock /> }
</span>
Expand Down
28 changes: 10 additions & 18 deletions packages/format-library/src/image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
*/
import { Path, SVG, TextControl, Popover, IconButton } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { Component, useMemo } from '@wordpress/element';
import { Component } from '@wordpress/element';
import { insertObject } from '@wordpress/rich-text';
import { MediaUpload, RichTextToolbarButton, MediaUploadCheck } from '@wordpress/block-editor';
import { LEFT, RIGHT, UP, DOWN, BACKSPACE, ENTER } from '@wordpress/keycodes';
import { computeCaretRect } from '@wordpress/dom';

const ALLOWED_MEDIA_TYPES = [ 'image' ];

Expand All @@ -16,16 +15,10 @@ const title = __( 'Inline image' );

const stopKeyPropagation = ( event ) => event.stopPropagation();

const PopoverAtImage = ( { dependencies, ...props } ) => {
return (
<Popover
position="bottom center"
focusOnMount={ false }
anchorRect={ useMemo( () => computeCaretRect(), dependencies ) }
{ ...props }
/>
);
};
function getRange() {
const selection = window.getSelection();
return selection.rangeCount ? selection.getRangeAt( 0 ) : null;
}

export const image = {
name,
Expand Down Expand Up @@ -93,7 +86,6 @@ export const image = {

render() {
const { value, onChange, isObjectActive, activeObjectAttributes } = this.props;
const { style } = activeObjectAttributes;

return (
<MediaUploadCheck>
Expand Down Expand Up @@ -124,10 +116,10 @@ export const image = {
} }
/> }
{ isObjectActive &&
<PopoverAtImage
// Reposition Popover when the selection changes or
// when the width changes.
dependencies={ [ style, value.start ] }
<Popover
position="bottom center"
focusOnMount={ false }
anchorRef={ getRange() }
>
{ // Disable reason: KeyPress must be suppressed so the block doesn't hide the toolbar
/* eslint-disable jsx-a11y/no-noninteractive-element-interactions */ }
Expand Down Expand Up @@ -165,7 +157,7 @@ export const image = {
<IconButton icon="editor-break" label={ __( 'Apply' ) } type="submit" />
</form>
{ /* eslint-enable jsx-a11y/no-noninteractive-element-interactions */ }
</PopoverAtImage>
</Popover>
}
</MediaUploadCheck>
);
Expand Down
Loading