Format Library: Assign Popover anchorRect to update selection position #14938

aduth · 2019-04-11T20:21:38Z

Fixes #14872
Fixes #11092
Alternative to #14921
Cherry-picks fc1fec7 from #14851

This pull request seeks to resolve an issue where repeated mounting of the link URLPopover component would cause excessive animations to occur in response to changes of selection or content within the formatted link. In doing so, it updates the behavior of link Popover to anchor to the link element itself, rather than follow the position of the caret.

Implementation Notes:

This reimplements the positioning to behave similar to Autocomplete in passing getAnchorRect to the rendered Popover, computed from the current selection. This replaces use of PositionedAtSelection.

#14872 is partly caused by the fact that to update PositionedAtSelection, we must assign a new key value, which would unmount and remount the component, causing the animation to restart.

It may be simpler to review using GitHub's "Hide whitespace changes" diff options, given the changes in inline.js are largely indentation changes.

Testing instructions:

Repeat Steps to Reproduce from #14872, verifying that the animation only occurs once when within the link formatting.

Avoids need to explicitly support onClickOutside as deprecated pass-through prop, instead leveraging pass-through nature of spread props.

…tion

gziolo

It doesn't work for me as expected. The popover moves on the screen...:

macOS, Chrome 73

gziolo · 2019-04-12T06:57:54Z

packages/block-editor/src/components/url-popover/index.js

 			position = 'bottom center',
 			focusOnMount = 'firstElement',
+			...popoverProps


I think I reviewed it already earlier this week 😃

…ement

aduth · 2019-04-12T15:15:53Z

@gziolo Good catch. It's not reliable to continuously update the caret positioning when link interactions (add, edit) will place selection into the editing popover.

@ellatrix I've pushed some updates which converge a bit more with your #14921, but still opts to render URLPopover with the getAnchorRect than to update PositionedAtSelection. It works well, though I did note the one case of pressing ArrowRight into the anchor node, it will not show the popover when first entering. I noticed this is what you'd accounted for with considering nextElementSibling, but I was a bit worried about the possibility of elements within the link yielding an unexpected value (the bolded element, for example, instead of the anchor). Although, now that I type this, I guess if it eventually uses Element#closest( 'a' ) anyways, it would match the anchor node.

packages/format-library/src/link/inline.js

ellatrix · 2019-04-12T15:21:51Z

I noticed this is what you'd accounted for with considering nextElementSibling, but I was a bit worried about the possibility of elements within the link yielding an unexpected value (the bolded element, for example, instead of the anchor). Although, now that I type this, I guess if it eventually uses Element#closest( 'a' ) anyways, it would match the anchor node.

Yes, it's an edge case. I think it could work reliably though, you'd always end up with the right element.

packages/format-library/src/link/inline.js

…ment Co-Authored-By: Ella van Durpe <ellatrix@users.noreply.github.com>

ellatrix · 2019-04-12T15:43:16Z

packages/components/src/popover/README.md

@@ -123,6 +123,20 @@ Opt-in prop to show popovers fullscreen on mobile, pass `false` in this prop to
 - Required: No
 - Default: `false`

+### getAnchorRect


Are we certain about the future of getAnchorRect before documenting it? Perhaps we could work towards only supporting anchorRect.

Are we certain about the future of getAnchorRect before documenting it? Perhaps we could work towards only supporting anchorRect.

I don't mind to remove the documentation, if even just to keep the changes well-contained. I don't have much a problem with this prop, at least in that it's a useful way to also base the computed anchor rect relative the passed default anchor argument. I'm also not entirely comfortable with the prospect of removing functionality based on the absence of it from documentation either.

ellatrix · 2019-04-12T15:43:54Z

packages/components/src/popover/index.js

+		}
+
+		const hasAnchorRect = this.props.hasOwnProperty( 'anchorRect' );
+		if ( hasAnchorRect !== prevProps.hasOwnProperty( 'anchorRect' ) ) {


Why use hasOwnProperty?

Why use hasOwnProperty?

Because we expect the value could change over time, but scheduling the interval doesn't depend on the value, it just depends on whether a value exists.

i.e. if the alternative is this.props.anchorRect !== prevProps.anchorRect, it would have unintended consequences on the auto-refresh.

ellatrix

This works really well in my testing!

ellatrix · 2019-04-12T15:48:50Z

packages/format-library/src/link/inline.js

@@ -77,6 +77,39 @@ const LinkViewerUrl = ( { url } ) => {
 	);
 };

+const URLPopoverAtLink = ( { isActive, addingLink, value, ...props } ) => {
+	const anchorRect = useMemo( () => {


Just wondering... Does this have a limit on cache size? Sounds like it should have a limit of exactly one.

Just wondering... Does this have a limit on cache size? Sounds like it should have a limit of exactly one.

It's not entirely clear, but it seems like something they would have considered as being the default behavior, but I also recall some similar discussion with @gziolo recently where he was showing a separate project which, by its name, might imply a different behavior 🤷‍♂️

https://reactjs.org/docs/hooks-reference.html#usememo

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

https://github.com/alexreardon/use-memo-one#readme

useMemo and useCallback cache the most recent result. However, this cache can be destroyed by React when it wants to:

Yeah, to me it sounded like it was not the intention of useMemo to limit the cache to one by default. So I would consider swapping it. I'm generally wary of things that create cache without any limit. We're just taking more and more space from memory for no reason.

Yeah, to me it sounded like it was not the intention of useMemo to limit the cache to one by default. So I would consider swapping it. I'm generally wary of things that create cache without any limit. We're just taking more and more space from memory for no reason.

Oh, I was thinking the opposite by my interpretation; that yes, it would only store the one latest result in memory. I'll plan to double-check, though.

From what I can discern, it only maintains a single value.

https://github.com/facebook/react/blob/3438e5ce879883590f68370b9ad6448a8ffdfdc1/packages/react-dom/src/server/ReactPartialRendererHooks.js#L338-L365

Feedback addressed, confirmed via DM to dismiss

WordPress#14938) * Block Editor: Pass spread props from URLPopover to Popover Avoids need to explicitly support onClickOutside as deprecated pass-through prop, instead leveraging pass-through nature of spread props. * Format Library: Assign Popover getAnchorRect to update selection position * Format Library: Compute memoized anchor from caret point or active element * Components: Add anchorRect prop to Popover component * Format Library: Provide direct reference to Popover anchorRect * Format Library: Consider next element sibling for initial caret placement Co-Authored-By: Ella van Durpe <ellatrix@users.noreply.github.com> * Components: Mark PositionedAtSelection as unstable * Components: Remove getAnchorRect mention from Popover README

#14938) * Block Editor: Pass spread props from URLPopover to Popover Avoids need to explicitly support onClickOutside as deprecated pass-through prop, instead leveraging pass-through nature of spread props. * Format Library: Assign Popover getAnchorRect to update selection position * Format Library: Compute memoized anchor from caret point or active element * Components: Add anchorRect prop to Popover component * Format Library: Provide direct reference to Popover anchorRect * Format Library: Consider next element sibling for initial caret placement Co-Authored-By: Ella van Durpe <ellatrix@users.noreply.github.com> * Components: Mark PositionedAtSelection as unstable * Components: Remove getAnchorRect mention from Popover README

aduth · 2020-01-28T21:15:34Z

packages/format-library/src/link/inline.js

+		if ( closest ) {
+			return closest.getBoundingClientRect();
+		}
+	}, [ isActive, addingLink, value.start, value.end ] );


I don't know that isActive needed to be listed as a dependency. It's not ever used in the useMemo callback.

aduth added 2 commits April 11, 2019 16:10

Block Editor: Pass spread props from URLPopover to Popover

a309269

Avoids need to explicitly support onClickOutside as deprecated pass-through prop, instead leveraging pass-through nature of spread props.

Format Library: Assign Popover getAnchorRect to update selection posi…

ad35be2

…tion

aduth added the [Package] Format library /packages/format-library label Apr 11, 2019

aduth requested a review from ellatrix April 11, 2019 20:21

aduth requested review from gziolo, jorgefilipecosta, noisysocks, talldan and youknowriad as code owners April 11, 2019 20:21

gziolo previously requested changes Apr 12, 2019

View reviewed changes

gziolo added the [Type] Bug An existing feature does not function as intended label Apr 12, 2019

Format Library: Compute memoized anchor from caret point or active el…

425900e

…ement

ellatrix reviewed Apr 12, 2019

View reviewed changes

packages/format-library/src/link/inline.js Outdated Show resolved Hide resolved

ellatrix reviewed Apr 12, 2019

View reviewed changes

packages/format-library/src/link/inline.js Show resolved Hide resolved

aduth and others added 3 commits April 12, 2019 11:35

Components: Add anchorRect prop to Popover component

e3f0e9d

Format Library: Provide direct reference to Popover anchorRect

3b85cc9

Format Library: Consider next element sibling for initial caret place…

04b8b1d

…ment Co-Authored-By: Ella van Durpe <ellatrix@users.noreply.github.com>

aduth requested review from ajitbohra and chrisvanpatten as code owners April 12, 2019 15:37

Components: Mark PositionedAtSelection as unstable

c42067e

ellatrix reviewed Apr 12, 2019

View reviewed changes

ellatrix approved these changes Apr 12, 2019

View reviewed changes

ellatrix reviewed Apr 12, 2019

View reviewed changes

Components: Remove getAnchorRect mention from Popover README

28c44d2

aduth changed the title ~~Format Library: Assign Popover getAnchorRect to update selection position~~ Format Library: Assign Popover anchorRect to update selection position Apr 12, 2019

aduth merged commit 6a26e36 into master Apr 12, 2019

aduth deleted the fix/14872-link-url-popover-popover-rect branch April 12, 2019 18:43

aduth mentioned this pull request Apr 12, 2019

Links: do not reposition container on selection change #14921

Closed

5 tasks

youknowriad added this to the 5.5 (Gutenberg) milestone Apr 15, 2019

talldan mentioned this pull request Apr 15, 2019

Bump version number to 5.5.0-rc.1 #14981

Merged

aduth mentioned this pull request Apr 15, 2019

Plugin: Backport bug fixes from Gutenberg 5.5.0 to wp/trunk (WordPress 5.2 RC) #14987

Merged

This was referenced Apr 17, 2019

Bump/5.5 #15011

Closed

Release/5.5 #15012

Closed

Bump plugin version to 5.5 #15013

Merged

This was referenced Apr 18, 2019

Components: remove __unstablePositionedAtSelection #15035

Merged

Three issues when trying to edit links #14461

Closed

ellatrix mentioned this pull request May 1, 2019

Link popover does not scroll with page #15371

Closed

aduth commented Jan 28, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format Library: Assign Popover anchorRect to update selection position #14938

Format Library: Assign Popover anchorRect to update selection position #14938

aduth commented Apr 11, 2019 •

edited

Loading

gziolo left a comment •

edited

Loading

gziolo Apr 12, 2019

aduth commented Apr 12, 2019

ellatrix commented Apr 12, 2019

ellatrix Apr 12, 2019

aduth Apr 12, 2019

ellatrix Apr 12, 2019

aduth Apr 12, 2019 •

edited

Loading

ellatrix left a comment

ellatrix Apr 12, 2019

aduth Apr 12, 2019 •

edited

Loading

ellatrix Apr 12, 2019

aduth Apr 12, 2019

aduth Apr 12, 2019

aduth Jan 28, 2020

Format Library: Assign Popover anchorRect to update selection position #14938

Format Library: Assign Popover anchorRect to update selection position #14938

Conversation

aduth commented Apr 11, 2019 • edited Loading

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 12, 2019

ellatrix commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

ellatrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 11, 2019 •

edited

Loading

gziolo left a comment •

edited

Loading

aduth Apr 12, 2019 •

edited

Loading

aduth Apr 12, 2019 •

edited

Loading