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

Popover: add anchor prop #43691

Merged
merged 36 commits into from Sep 14, 2022
Merged

Popover: add anchor prop #43691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
157f8e7
Popover: add new anchor prop, mark other anchor props as deprecated
ciampo Aug 30, 2022
c185f24
Add `anchor` prop to Storybook
ciampo Aug 30, 2022
76fd3f2
Add WP version for deprecated props removal
ciampo Aug 30, 2022
28c7b18
Do not render fallback anchor if there is already a prop-derived anchor
ciampo Aug 30, 2022
16e66d4
Block inbetween inserter: use Popover's new anchor prop (#43693)
ciampo Aug 31, 2022
7b62c02
ListViewDropIndicator: use Popover s new anchor prop (#43694)
ciampo Aug 31, 2022
eef3dc6
Temporarily disable derpecation warnings
ciampo Aug 31, 2022
1866f3f
Block toolbar: use Popover's new anchor prop (#43692)
ciampo Aug 31, 2022
fb6f2ee
Dropdown: use Popover s new anchor prop (#43698)
ciampo Aug 31, 2022
3e59ec1
BlockPopover: prevent error when `selectedElement` is not defined
ciampo Sep 1, 2022
c19495c
Try to avoid infinite loop
ciampo Sep 1, 2022
c342d79
Update PanelRow docs
ciampo Sep 2, 2022
c3a3f61
Edit navigation menu actions: use Popover s new anchor prop
ciampo Sep 2, 2022
03eb730
BorderBoxControl: use Popover's new anchor prop (#43789)
ciampo Sep 5, 2022
33c87f1
Image URL Input: use new anchor prop for Popover (#43784)
ciampo Sep 5, 2022
d1890ed
Edit site Actions: use new anchor prop for Popover (#43810)
ciampo Sep 5, 2022
044dd4a
Buttons block: use new Popover anchor prop (#43785)
ciampo Sep 5, 2022
f9fbae5
Navigation block: use new anchor prop for Popover (#43786)
ciampo Sep 5, 2022
13327ef
Post Date block: use new anchor prop for Popover (#43787)
ciampo Sep 5, 2022
a659e87
Tooltip: refactor using Popover's new anchor prop (#43799)
ciampo Sep 5, 2022
4db67fc
Improve docs around using state instead of refs for the anchor element
ciampo Sep 6, 2022
aadd730
Allow `anchor` to be `null`
ciampo Sep 6, 2022
c787c54
Edit Post: use Popover's new anchor prop (#43808)
ciampo Sep 6, 2022
0ac5e79
Refactor `useAnchorRef` and related components to work with the new P…
ciampo Sep 6, 2022
f9a748a
Re-enable deprecation warnings
ciampo Sep 6, 2022
bc75d2f
Remove fall back to `undefined` from `null`
ciampo Sep 6, 2022
b4973f4
Ensure reactive updates when the popover anchor updates
ciampo Sep 6, 2022
7a7179f
Refactor SocialLinkEdit component to use `anchor` instead of `anchorRef`
ciampo Sep 7, 2022
42a72ac
CHANGELOG
ciampo Sep 7, 2022
8c76d7b
Add new `useAnchor` hook instead of changing existing `useAnchorRef` …
ciampo Sep 7, 2022
d09cbfe
Fix API docs
ciampo Sep 7, 2022
dffc311
Update Popover unit tests
ciampo Sep 7, 2022
dd1b771
Remove unused import
ciampo Sep 7, 2022
24c099a
Use DOMRect in the DomRectWithOwnerDocument type
ciampo Sep 9, 2022
1d11ed2
Improve the wording of deprecation warnings
ciampo Sep 14, 2022
caacb0e
Put more emphasis on storing anchor in local state
ciampo Sep 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

102 changes: 50 additions & 52 deletions packages/block-editor/src/components/block-popover/inbetween.js
View file
Open in desktop
Expand Up @@ -8,7 +8,6 @@ import classnames from 'classnames';
*/
import { useSelect } from '@wordpress/data';
import {
useCallback,
useMemo,
createContext,
useReducer,
Expand Down Expand Up @@ -107,66 +106,65 @@ function BlockPopoverInbetween( {
isVisible,
] );

const getAnchorRect = useCallback( () => {
const popoverAnchor = useMemo( () => {
if ( ( ! previousElement && ! nextElement ) || ! isVisible ) {
return {};
return undefined;
}

const { ownerDocument } = previousElement || nextElement;

const previousRect = previousElement
? previousElement.getBoundingClientRect()
: null;
const nextRect = nextElement
? nextElement.getBoundingClientRect()
: null;
return {
ownerDocument,
getBoundingClientRect() {
const previousRect = previousElement
? previousElement.getBoundingClientRect()
: null;
const nextRect = nextElement
? nextElement.getBoundingClientRect()
: null;

if ( isVertical ) {
if ( isRTL() ) {
return {
top: previousRect ? previousRect.bottom : nextRect.top,
left: previousRect ? previousRect.right : nextRect.right,
right: previousRect ? previousRect.right : nextRect.right,
bottom: previousRect ? previousRect.bottom : nextRect.top,
height: 0,
width: 0,
ownerDocument,
};
}
let left = 0;
let top = 0;

return {
top: previousRect ? previousRect.bottom : nextRect.top,
left: previousRect ? previousRect.left : nextRect.left,
right: previousRect ? previousRect.left : nextRect.left,
bottom: previousRect ? previousRect.bottom : nextRect.top,
height: 0,
width: 0,
ownerDocument,
};
}
if ( isVertical ) {
// vertical
top = previousRect ? previousRect.bottom : nextRect.top;

if ( isRTL() ) {
return {
top: previousRect ? previousRect.top : nextRect.top,
left: previousRect ? previousRect.left : nextRect.right,
right: previousRect ? previousRect.left : nextRect.right,
bottom: previousRect ? previousRect.top : nextRect.top,
height: 0,
width: 0,
ownerDocument,
};
}
if ( isRTL() ) {
// vertical, rtl
left = previousRect
? previousRect.right
: nextRect.right;
} else {
// vertical, ltr
left = previousRect ? previousRect.left : nextRect.left;
}
} else {
top = previousRect ? previousRect.top : nextRect.top;

return {
top: previousRect ? previousRect.top : nextRect.top,
left: previousRect ? previousRect.right : nextRect.left,
right: previousRect ? previousRect.right : nextRect.left,
bottom: previousRect ? previousRect.left : nextRect.right,
height: 0,
width: 0,
ownerDocument,
if ( isRTL() ) {
// non vertical, rtl
left = previousRect
? previousRect.left
: nextRect.right;
} else {
// non vertical, ltr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noting that, compared to trunk, this PR is not taking into account the fact that for non-vertical, LTR cases the bottom was supposed to be different from the top.

The assumption is that bottom should have been the same as top (see #41156 (comment))

cc @youknowriad

left = previousRect
? previousRect.right
: nextRect.left;
}
}

return new window.DOMRect( left, top, 0, 0 );
},
};
}, [ previousElement, nextElement, positionRecompute, isVisible ] );
}, [
previousElement,
nextElement,
positionRecompute,
isVertical,
isVisible,
] );

const popoverScrollRef = usePopoverScroll( __unstableContentRef );

Expand Down Expand Up @@ -229,7 +227,7 @@ function BlockPopoverInbetween( {
<Popover
ref={ popoverScrollRef }
animate={ false }
getAnchorRect={ getAnchorRect }
anchor={ popoverAnchor }
focusOnMount={ false }
// Render in the old slot if needed for backward compatibility,
// otherwise render in place (not in the default popover slot).
Expand Down
50 changes: 44 additions & 6 deletions packages/block-editor/src/components/block-popover/index.js
View file
Open in desktop
Expand Up @@ -47,22 +47,60 @@ function BlockPopover(
};
}, [ selectedElement, lastSelectedElement, __unstableRefreshSize ] );

const popoverAnchor = useMemo( () => {
if (
! selectedElement ||
( bottomClientId && ! lastSelectedElement )
) {
return undefined;
}

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 );
},
ownerDocument: selectedElement.ownerDocument,
};
}, [ bottomClientId, lastSelectedElement, selectedElement ] );

if ( ! selectedElement || ( bottomClientId && ! lastSelectedElement ) ) {
return null;
}

const anchorRef = {
top: selectedElement,
bottom: lastSelectedElement,
};

return (
<Popover
ref={ mergedRefs }
animate={ false }
position="top right left"
focusOnMount={ false }
anchorRef={ anchorRef }
anchor={ popoverAnchor }
// Render in the old slot if needed for backward compatibility,
// otherwise render in place (not in the default popover slot).
__unstableSlotName={ __unstablePopoverSlot || null }
Expand Down
65 changes: 33 additions & 32 deletions packages/block-editor/src/components/list-view/drop-indicator.js
View file
Open in desktop
Expand Up @@ -68,40 +68,41 @@ export default function ListViewDropIndicator( {
};
}, [ getDropIndicatorIndent, targetElement ] );

const getAnchorRect = useCallback( () => {
if ( ! targetElement ) {
return {};
const popoverAnchor = useMemo( () => {
const isValidDropPosition =
dropPosition === 'top' ||
dropPosition === 'bottom' ||
dropPosition === 'inside';
if ( ! targetElement || ! isValidDropPosition ) {
return undefined;
}

const ownerDocument = targetElement.ownerDocument;
const rect = targetElement.getBoundingClientRect();
const indent = getDropIndicatorIndent();

const anchorRect = {
left: rect.left + indent,
right: rect.right,
width: 0,
height: 0,
ownerDocument,
return {
ownerDocument: targetElement.ownerDocument,
getBoundingClientRect() {
const rect = targetElement.getBoundingClientRect();
const indent = getDropIndicatorIndent();

const left = rect.left + indent;
const right = rect.right;
let top = 0;
let bottom = 0;

if ( dropPosition === 'top' ) {
top = rect.top;
bottom = rect.top;
} else {
// `dropPosition` is either `bottom` or `inside`
top = rect.bottom;
bottom = rect.bottom;
}

const width = right - left;
const height = bottom - top;

return new window.DOMRect( left, top, width, height );
},
};

if ( dropPosition === 'top' ) {
return {
...anchorRect,
top: rect.top,
bottom: rect.top,
};
}

if ( dropPosition === 'bottom' || dropPosition === 'inside' ) {
return {
...anchorRect,
top: rect.bottom,
bottom: rect.bottom,
};
}

return {};
}, [ targetElement, dropPosition, getDropIndicatorIndent ] );

if ( ! targetElement ) {
Expand All @@ -111,7 +112,7 @@ export default function ListViewDropIndicator( {
return (
<Popover
animate={ false }
getAnchorRect={ getAnchorRect }
anchor={ popoverAnchor }
focusOnMount={ false }
className="block-editor-list-view-drop-indicator"
>
Expand Down
28 changes: 18 additions & 10 deletions packages/block-editor/src/components/rich-text/format-toolbar-container.js
View file
Open in desktop
Expand Up @@ -7,7 +7,7 @@ import { useSelect } from '@wordpress/data';
import {
isCollapsed,
getActiveFormats,
useAnchorRef,
useAnchor,
store as richTextStore,
} from '@wordpress/rich-text';

Expand All @@ -19,28 +19,32 @@ import FormatToolbar from './format-toolbar';
import NavigableToolbar from '../navigable-toolbar';
import { store as blockEditorStore } from '../../store';

function InlineSelectionToolbar( { value, anchorRef, activeFormats } ) {
function InlineSelectionToolbar( {
value,
editableContentElement,
activeFormats,
} ) {
const lastFormat = activeFormats[ activeFormats.length - 1 ];
const lastFormatType = lastFormat?.type;
const settings = useSelect(
( select ) => select( richTextStore ).getFormatType( lastFormatType ),
[ lastFormatType ]
);
const selectionRef = useAnchorRef( {
ref: anchorRef,
const popoverAnchor = useAnchor( {
editableContentElement,
value,
settings,
} );

return <InlineToolbar anchorRef={ selectionRef } />;
return <InlineToolbar popoverAnchor={ popoverAnchor } />;
}

function InlineToolbar( { anchorRef } ) {
function InlineToolbar( { popoverAnchor } ) {
return (
<Popover
position="top center"
focusOnMount={ false }
anchorRef={ anchorRef }
anchor={ popoverAnchor }
className="block-editor-rich-text__inline-format-toolbar"
__unstableSlotName="block-toolbar"
>
Expand All @@ -57,14 +61,18 @@ function InlineToolbar( { anchorRef } ) {
);
}

const FormatToolbarContainer = ( { inline, anchorRef, value } ) => {
const FormatToolbarContainer = ( {
inline,
editableContentElement,
value,
} ) => {
const hasInlineToolbar = useSelect(
( select ) => select( blockEditorStore ).getSettings().hasInlineToolbar,
[]
);

if ( inline ) {
return <InlineToolbar anchorRef={ anchorRef } />;
return <InlineToolbar popoverAnchor={ editableContentElement } />;
}

if ( hasInlineToolbar ) {
Expand All @@ -76,7 +84,7 @@ const FormatToolbarContainer = ( { inline, anchorRef, value } ) => {

return (
<InlineSelectionToolbar
anchorRef={ anchorRef }
editableContentElement={ editableContentElement }
value={ value }
activeFormats={ activeFormats }
/>
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Expand Up @@ -328,7 +328,7 @@ function RichTextWrapper(
}

function onFocus() {
anchorRef.current.focus();
anchorRef.current?.focus();
}

const TagName = tagName;
Expand All @@ -354,7 +354,7 @@ function RichTextWrapper(
{ isSelected && hasFormats && (
<FormatToolbarContainer
inline={ inlineToolbar }
anchorRef={ anchorRef }
editableContentElement={ anchorRef.current }
Copy link
Member

Choose a reason for hiding this comment

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

Refactoring anchorRef to anchor is the simplest and most frequent case — it's mostly about passing the actual element instead of a reference.

I think we need to be careful of this case when refactoring. Reading refs during render is not recommended and may throw a warning in the future. I think that's also why floating-ui recommends useState for storing the element instance rather than useRef.

I wonder if there's a way to warn against usage like this on the API level 🤔?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is tricky.

References were already used prior to this PR (for this specific case, they were read inside the FormatToolbarContainer component as part of the useAnchorRef hook, and my refactor just moved the reading outside of the component), and therefore I don't think that this PR really introduces this patter.

If anything, this PR slightly improves the situation by deprecating the anchorRef prop, which was previously accepting refs instead of actual values.

I think that's also why floating-ui recommends useState for storing the element instance rather than useRef.

That's correct, and in fact I tried to refactor as many parts of the code as possible to useState — I just didn't apply the refactor around the rich-text components because these references are forwarded from outside of these components, and I didn't want to grow the scope of this PR.

I wonder if there's a way to warn against usage like this on the API level 🤔?

I don't think so, at least from the Popover's point of view, since the component doesn't have a way of knowing if the value passed as the anchor comes from a ref or from internal state.

I did add a few paragraphs in the component's README about using internal state in order to ensure reactive updates (74af7eb)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's definitely tricky 🤔, I don't have a solution yet either.

I think refs are fine though, as long as we don't read them during render. A solution to that would be using a function to get the reference element (a separate getAnchor prop or extend anchor).

- anchor={ ref.current }
+ anchor={ () => ref.current }

As long as the underlying implementation of <Popover> doesn't call the function during render then we'll be fine.

But then it all just feels like an attempt to fix a false positive though, I'm not sure if it's worth it 😅. Let's keep it as is until we find a better solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then it all just feels like an attempt to fix a false positive though, I'm not sure if it's worth it 😅. Let's keep it as is until we find a better solution?

Agreed. The best way to solve this problem would be to refactor the code outside of the Popover and use internal state instead (as I already did partially in this PR), so maybe we can work on that refactor at a later point if/when necessary.

value={ value }
/>
) }
Expand Down