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

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

Merged
merged 8 commits into from
Apr 12, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a309269
Block Editor: Pass spread props from URLPopover to Popover
aduth Apr 11, 2019
ad35be2
Format Library: Assign Popover getAnchorRect to update selection posi…
aduth Apr 11, 2019
425900e
Format Library: Compute memoized anchor from caret point or active el…
aduth Apr 12, 2019
e3f0e9d
Components: Add anchorRect prop to Popover component
aduth Apr 12, 2019
3b85cc9
Format Library: Provide direct reference to Popover anchorRect
aduth Apr 12, 2019
04b8b1d
Format Library: Consider next element sibling for initial caret place…
aduth Apr 12, 2019
c42067e
Components: Mark PositionedAtSelection as unstable
aduth Apr 12, 2019
28c44d2
Components: Remove getAnchorRect mention from Popover README
aduth Apr 12, 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
55 changes: 36 additions & 19 deletions packages/format-library/src/link/inline.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 { __ } from '@wordpress/i18n';
import { Component, createRef } from '@wordpress/element';
import { Component, createRef, useMemo } from '@wordpress/element';
import {
ExternalLink,
IconButton,
Expand Down Expand Up @@ -38,20 +38,6 @@ function isShowingInput( props, state ) {
return props.addingLink || state.editLink;
}

/**
* Returns a DOMRect object representing dimensions of the current selection
* caret.
*
* @return {DOMRect} Selection caret dimensions.
*/
function getCaretRect() {
const range = window.getSelection().getRangeAt( 0 );

if ( range ) {
return getRectangleFromRange( range );
}
}

const LinkEditor = ( { value, onChangeInputValue, onKeyDown, submitLink, autocompleteRef } ) => (
// 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 @@ -91,6 +77,35 @@ const LinkViewerUrl = ( { url } ) => {
);
};

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@aduth aduth Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

const range = window.getSelection().getRangeAt( 0 );
if ( ! range ) {
return;
}

if ( addingLink ) {
return getRectangleFromRange( range );
}

let element = range.startContainer;
while ( element.nodeType !== window.Node.ELEMENT_NODE ) {
element = element.parentNode;
}

const closest = element.closest( 'a' );
if ( closest ) {
return closest.getBoundingClientRect();
}
}, [ isActive, addingLink, value.start, value.end ] );
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 know that isActive needed to be listed as a dependency. It's not ever used in the useMemo callback.


if ( ! anchorRect ) {
return null;
}

return <URLPopover getAnchorRect={ () => anchorRect } { ...props } />;
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
};

const LinkViewer = ( { url, editLink } ) => {
return (
// Disable reason: KeyPress must be suppressed so the block doesn't hide the toolbar
Expand Down Expand Up @@ -225,7 +240,7 @@ class InlineLinkUI extends Component {
}

render() {
const { isActive, activeAttributes: { url }, addingLink } = this.props;
const { isActive, activeAttributes: { url }, addingLink, value } = this.props;

if ( ! isActive && ! addingLink ) {
return null;
Expand All @@ -235,11 +250,13 @@ class InlineLinkUI extends Component {
const showInput = isShowingInput( this.props, this.state );

return (
<URLPopover
<URLPopoverAtLink
value={ value }
isActive={ isActive }
addingLink={ addingLink }
onClickOutside={ this.onClickOutside }
onClose={ this.resetState }
focusOnMount={ showInput ? 'firstElement' : false }
getAnchorRect={ getCaretRect }
renderSettings={ () => (
<ToggleControl
label={ __( 'Open in New Tab' ) }
Expand All @@ -262,7 +279,7 @@ class InlineLinkUI extends Component {
editLink={ this.editLink }
/>
) }
</URLPopover>
</URLPopoverAtLink>
);
}
}
Expand Down