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.

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

Links: do not reposition container on selection change #14921

Closed
wants to merge 5 commits into from

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Apr 11, 2019

Description

Fixes #14872. It also makes sure that the container is positioned relative to the link element rather than the selection/caret, so it doesn't move when you move selection inside a link.

There are several steps that came to the fix.

  1. I wanted to modify PositionedAtSelection so that it accepts a selector prop, to select the closest matching element to calculate the position from rather than the caret.
  2. In order to not recreate PositionedAtSelection for every change in selection, we need to use the format object reference as an indicator to recreate, as each DOM element is a different object instance.
  3. Unfortunately, React does not accept object references as keys, so we have to derive a simpler state to use as the key. I used getDerivedStateFromProps to bump the key every time the active format object changes.
  4. Something was wrong in setting the activeFormats in onSelectionChange. The formats need to be taken from the current value to maintain references, not from the created value (from DOM) on the selection change.
  5. To ensure object references persist in the RichText component across selection changes and edits, I've taken out recreating the rich text value for every change. This actually results in other positive changes: no more formatToValue call for every input event, and no more need memoize it, and no more need for updateFormats, introduced in RichText: unify active formats, 'selectedFormat' and 'placeholderFormat' #14411.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Format library /packages/format-library labels Apr 11, 2019
@ellatrix ellatrix added this to the 5.5 (Gutenberg) milestone Apr 11, 2019
@ellatrix
Copy link
Member Author

The only remaining issue here is that the toolbar rerenders when you typing in the element. Fixing that now.

@ellatrix ellatrix force-pushed the try/link-container branch 3 times, most recently from b497ba1 to ccaa94d Compare April 11, 2019 15:16
@@ -222,7 +236,12 @@ class InlineLinkUI extends Component {

return (
<PositionedAtSelection
key={ `${ value.start }${ value.end }` /* Used to force rerender on selection change */ }
Copy link
Member

@aduth aduth Apr 11, 2019

Choose a reason for hiding this comment

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

Possible dumb question: Couldn't a minimal solution just be to remove key (and make no other 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.

What if you move the caret from one link to another?

Copy link
Member

@aduth aduth Apr 11, 2019

Choose a reason for hiding this comment

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

What if you move the caret from one link to another?

That's true. I wonder then if it could either act more like the Autocomplete component in rendering Popover with a getAnchorRect that updates with the selection (but keeps the same element so not to unmount and re-trigger animation), or if PositionedAtSelection should update its styling in response to selection changes rather than once at mount. Both of these would still update the position on each selection change, but at least would avoid the repeated animations.

I'm not totally sure the ramifications of either of these alternatives. Just trying to consider in terms of the least potential for impact.

@ellatrix ellatrix force-pushed the try/link-container branch from 15d85e8 to 18f4c57 Compare April 11, 2019 16:34
end,
};
delete newValue.activeFormats;
const activeFormats = getActiveFormats( newValue );
Copy link
Member

Choose a reason for hiding this comment

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

If we're working with a shallow clone, seems we might as well assign activeFormats directly, rather than create another shallow clone below?

const newValue = {
	...value,
	activeFormats: getActiveFormats( newValue ),
	start,
	end,
};

@@ -763,6 +781,7 @@ export class RichText extends Component {
// In all other cases, prevent default behaviour.
event.preventDefault();

const formats = createPrepareEditableTree( this.props )( value );
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this change is here.

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'll comment, it's to make sure the boundaries work for annotations.

@ellatrix
Copy link
Member Author

Updated the description:

To ensure object references persist in the RichText component across selection changes and edits, I've taken out recreating the rich text value for every change. This actually results in other positive changes: no more formatToValue call for every input event, and no more need memoize it, and no more need for updateFormats, introduced in #14411.

@ellatrix ellatrix force-pushed the try/link-container branch from e158037 to f44a316 Compare April 12, 2019 13:05
@ellatrix
Copy link
Member Author

While I think we eventually want all the changes here to RichText, I'm going to make an alternative PR that doesn't touch as much existing code as it does now.

@aduth
Copy link
Member

aduth commented Apr 12, 2019

Needs to be rebased after #14938

@aduth aduth removed this from the 5.5 (Gutenberg) milestone Apr 12, 2019
@aduth
Copy link
Member

aduth commented Apr 12, 2019

(I'm assuming this doesn't need to land for Gutenberg 5.5 and have removed it from the milestone accordingly)

@ellatrix
Copy link
Member Author

Closing for now.

@ellatrix ellatrix closed this Apr 18, 2019
@ellatrix ellatrix deleted the try/link-container branch April 18, 2019 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Format library /packages/format-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Components: URL input popover animates repeatedly on text/selection change
2 participants