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

RichText: remove Editable & allow React to diff editable element #17779

Merged
merged 9 commits into from Nov 12, 2019

Conversation

@ellatrix
Copy link
Member

ellatrix commented Oct 4, 2019

Description

This PR eliminates our own diffing for attributes on the content editable element.

Benefits:

  • No more expensive diffing of style and aria- attributes on every render.
  • We normally have to manually add new attributes, e.g. reversed or start etc..
  • No more Editable component.
  • Gets rid of the shouldComponentUpdate hack.

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 marked this pull request as ready for review Oct 4, 2019
@ellatrix ellatrix force-pushed the try/editable-react-diff branch from c7c4b11 to 931615c Oct 7, 2019
@ellatrix ellatrix changed the title RichText: allow React to diff editable element RichText: remove Editable & allow React to diff editable element Oct 28, 2019
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 7, 2019

It seems really promising. What's the status of this? Just needs a review?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 7, 2019

Yeah, this is ready for review :D

Copy link
Member

aduth left a comment

If I understand correctly, the changes here aren't actually leveraging React to do the rendering of the RichText record?

Even still, I think the benefits here are pretty great, and serve as a nice overall simplification.

I would be curious as well what the measurable performance difference is here, if any (seems there ought to be some improvement to key presses?).

Most of my review comments are minor nit-picks. Functionally, it works great. The only thing I'd consider really blocking is the priority with which we're applying ARIA props seems to have changed.

packages/rich-text/src/component/index.js Outdated Show resolved Hide resolved
valueToEditableHTML={ this.valueToEditableHTML }
aria-label={ placeholder }
{ ...pickAriaProps( this.props ) }
style={ { ...style, whiteSpace } }

This comment has been minimized.

Copy link
@aduth

aduth Nov 11, 2019

Member

While it existed previously as well, I wonder if we're incurring unnecessary work here in rendering since React will always consider props.style !== nextProps.style on every render. I guess in real-world usage we'd be doing so anyways, since the paragraph block always passes a new object as well (though that's something on its own to consider refactoring), but even then there's the (albeit trivial) work of merging the objects.

Was it ever considered: Can we apply this style via CSS ? It avoids the whole need to merge the objects at all. I'd even be generally okay with the problem was specificity and we found ourselves needing to !important.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 11, 2019

Author Member

Can we apply this style via CSS ? It avoids the whole need to merge the objects at all.

Oh you mean the white space rule? Yes, I considered using a stylesheet, but then this would be the only rule in the stylesheet, so I picked adding it to the style object instead of creating a whole new stylesheet for this package.

I'll adjust to check the style prop.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 11, 2019

Author Member

I wonder if React has some internal optimisations for the style prop on an element. It's a bit strange to expect React users to optimise these style objects that are so commonly used.

This comment has been minimized.

Copy link
@aduth

aduth Nov 12, 2019

Member

I wonder if React has some internal optimisations for the style prop on an element. It's a bit strange to expect React users to optimise these style objects that are so commonly used.

Yeah, I'm not sure if they do, but as a general principle this is also why I tend to avoid objects as props in the first place. ariaProps is another good example of where this becomes needlessly difficult to manage. In retrospect, I'm not sure why we didn't just expect people to pass the specific prop they wanted to apply (aria-foo). I seem to recall some limitation that led us down that path, but certainly made things more difficult.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 12, 2019

Author Member

Right, I try to avoid them as much as possible too. Worth noting that ariaProps is picked from props though, there's no ariaProps prop. The picking seems to be there to avoid passing all RichText props to the element.

This comment has been minimized.

Copy link
@aduth

aduth Nov 12, 2019

Member

Right, I try to avoid them as much as possible too. Worth noting that ariaProps is picked from props though, there's no ariaProps prop. The picking seems to be there to avoid passing all RichText props to the element.

Oh! You're right, I definitely mis-recalled the behavior.

So maybe the opposite is true, that we have headaches because we chose to pick props than to accept an object 😄 Maybe in the future we could just be passing through all the props, so we don't need to be doing the explicit picking?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 12, 2019

Author Member

I think we either need to pick the props we want to pass on, or delete the props we don't want to pass on. Both of these are explicit in a way I guess, but I'm fine with trying to delete the props we don't want to pass on. :) Either way there will be some sort of picking. We can't pass any unknown props to an element.

packages/rich-text/src/component/index.js Outdated Show resolved Hide resolved
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 11, 2019

If I understand correctly, the changes here aren't actually leveraging React to do the rendering of the RichText record?

No, it's just letting React manage the editable element, not its contents. I'd like to explore that in the future, but I ran into some problems with it. React can't seem to update a DOM tree if the current tree is not in a shape that it's expecting. Anyway, that's a different problem.

I would be curious as well what the measurable performance difference is here, if any (seems there ought to be some improvement to key presses?).

I think that, while there is a bit less work being done, the difference is negligible, especially compared to all the work that's being done outside of RichText after onChange.

@ellatrix ellatrix force-pushed the try/editable-react-diff branch from 567599a to ba7fb6a Nov 11, 2019
@ellatrix ellatrix requested a review from aduth Nov 12, 2019
@aduth
aduth approved these changes Nov 12, 2019
Copy link
Member

aduth left a comment

Nice 👍

@@ -1024,6 +1041,8 @@ class RichText extends Component {
onKeyUp={ this.onSelectionChange }
onMouseUp={ this.onSelectionChange }
onTouchEnd={ this.onSelectionChange }
contentEditable

This comment has been minimized.

Copy link
@aduth

aduth Nov 12, 2019

Member

Technically we previously applied props such that someone could contentEditable. I don't think that's a use case we want to support though 😄

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 12, 2019

Author Member

Right. I don't think we should support that for now. We might in the future, maybe through a disabled or readonly prop. But probably not by directly setting contentEditable.

packages/rich-text/src/component/index.js Outdated Show resolved Hide resolved
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 12, 2019

Thanks a lot for the review @aduth!

@ellatrix ellatrix merged commit d9de027 into master Nov 12, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix deleted the try/editable-react-diff branch Nov 12, 2019
@aduth aduth mentioned this pull request Nov 13, 2019
5 of 5 tasks complete
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.