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

Projects
None yet
4 participants
@aduth
Member

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.

@youknowriad

LGTM 👍

@aduth

This comment has been minimized.

Member

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 #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

This comment has been minimized.

@mtias

mtias Jun 29, 2018

Contributor

This comment was a bit hard to understand.

This comment has been minimized.

@aduth

aduth Jun 29, 2018

Member

This comment was a bit hard to understand.

Updated in 2ea9199.

@mtias mtias self-requested a review Jun 29, 2018

@mtias

mtias approved these changes Jun 29, 2018

Looks good to me. Thanks for adding comments!

@iseulde iseulde requested a review from jorgefilipecosta Jun 29, 2018

aduth added some commits Jun 22, 2018

Rich Text: Test edge as empty after / before fragment
Accommodates case where childNodes exist, but none which would qualify after filtering. Also preserves existing logic for detecting / handling paste context.
@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

Updates:

  • Rebased to resolve conflicts.
  • Restored respect of the insertBlocksAfter potentially being undefined for a locked template. See also #6587.
  • Refactored RichText split once more: This time simplifying to handle edge detection at once, with a single path toward restoreContentAndSplit with handling of paste context. See extended commit message of 7a8ec43.
@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 29, 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.

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

This comment has been minimized.

Member

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

2 checks passed

codecov/project 47.08% (-0.04%) compared to 8f58e27
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the update/paragraph-split-set-content branch Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment