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

Blocks: Avoid setAttributes on end-of-paragraph split #7482

Merged
merged 4 commits into from
Jun 29, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 22, 2018

This pull request is part of a performance audit of the paragraph block. It seeks to resolve an issue where we needlessly call setAttributes on a paragraph block when splitting into a new paragraph, and avoids potentially heavy processing within RichText to determine before and after fragments when it can be inferred by the position of the selection at the end/beginning of the RichText field whether the existing value can be used verbatim; thus also giving opportunity for the paragraph block to do a strict comparison against its own content attribute to determine whether setAttributes needs to be called.

This is one of two setAttributes calls which are currently invoked on the original paragraph when pressing enter at the end of its text. The other is explained at #4956 (comment) .

Testing instructions:

Verify there are no regressions in the behavior of paragraph splitting.

Optional: Validate that UPDATE_BLOCK_ATTRIBUTES is dispatched only once (erroneously, vs. twice on master) when pressing enter at the end of a paragraph block using Redux DevTools Extension.

@aduth aduth added [Feature] Blocks Overall functionality of blocks [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Jun 22, 2018
@aduth aduth requested a review from ellatrix June 22, 2018 15:08
@mtias mtias added this to the 3.2 milestone Jun 27, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aduth
Copy link
Member Author

aduth commented Jun 28, 2018

I've a few things I'm looking into before merging this:

  • Ensuring that there are no problems with calling onSplit directly rather than restoreContentAndSplit. My current conclusion is that we don't need restoreContentAndSplit at all, and it's proposed to be removed in Rich Text: Remove restoreContentAndSplit #7618.
  • Ensuring that there's no issue in not reaching the part of splitContent which considers context.paste. I'm still not entirely clear what it's responsible for doing, and am continue to explore further.

} = this.props;

if ( after ) {
// After content should be appended to the set of spread blocks as
Copy link
Member

Choose a reason for hiding this comment

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

This comment was a bit hard to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment was a bit hard to understand.

Updated in 2ea9199.

@mtias mtias self-requested a review June 29, 2018 12:04
Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding comments!

Accommodates case where childNodes exist, but none which would qualify after filtering. Also preserves existing logic for detecting / handling paste context.
@aduth aduth force-pushed the update/paragraph-split-set-content branch from 785bea9 to 7a8ec43 Compare June 29, 2018 14:19
@aduth
Copy link
Member Author

aduth commented Jun 29, 2018

Updates:

@jorgefilipecosta
Copy link
Member

This pull request is part of a performance audit of the paragraph block. It seeks to resolve an issue where we needlessly call setAttributes on a paragraph block when splitting into a new paragraph.

I think we are stilling making a non required setAttributes call.

I added this paragraph as the content of the post:

<!-- wp:paragraph -->
<p>123456789</p>
<!-- /wp:paragraph -->

I opened redux dev tools in core/editor store.
I put the cursor between 5 and 6 and pressed enter.
I checked that we dispatched UPDATE_BLOCK_ATTRIBUTES with content equal to "123456789" (not needed). Then we inserted a block and then we updated the content of the original block.

It is possible to check that we are doing a non required UPDATE_BLOCK_ATTRIBUTES also by inserting many paragraphs without any content. To undo each paragraph creation we need to use undo two times.

@aduth
Copy link
Member Author

aduth commented Jun 29, 2018

@jorgefilipecosta Nice catch. I can reproduce this. However, I think it's a totally separate issue. Namely, we're causing UPDATE_BLOCK_ATTRIBUTES to be called because even though we're explicitly handling the split on enter press, the event binding for input will cause onChange to be called with the editor content. We should probably prevent this for the enter key (or when otherwise knowing that there is other explicit handling for the input).

@aduth aduth merged commit 51c6ca7 into master Jun 29, 2018
@aduth aduth deleted the update/paragraph-split-set-content branch June 29, 2018 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants