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

Fix pasting content into inner blocks #16717

Merged
merged 3 commits into from
Jul 26, 2019
Merged

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jul 23, 2019

Fixes #16645

Description

Resolves an issue where content pasted into inner blocks is not saved correctly.

The issue was caused by a small caching error introduced in #16407.

When pasting content into an empty paragraph, a block replacement is triggered. When this action was being carried out in an inner block, cached parent blocks were not being invalidated.

This is the code that attempts to invalidate parents of a replaced block:
https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/store/reducer.js#L269-L271

The issue is that getBlocksWithParentsClientIds is called with the incorrect clientId. getBlocksWithParentsClientIds operates on the old state rather than the new state, so it needs to be called with the block that's about to be removed rather than the block that's about to be added: https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/store/reducer.js#L228-L237

This PR resolves the issue.

How has this been tested?

Updated the relevant unit test to ensure the parent block is invalidated.

Manual testing:

  1. Add Columns block.
  2. Copy and paste text from another website into each of the columns.
  3. Save the document and preview it.
  4. Observe that the text isn't showing on the frontend.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention [Type] Regression Related to a regression in the latest release [Package] Block editor /packages/block-editor labels Jul 23, 2019
@talldan talldan self-assigned this Jul 23, 2019
@talldan talldan added the [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P label Jul 23, 2019
getBlocksWithParentsClientIds( keys( flattenBlocks( action.blocks ) ) ),
),
...omit( parentClientIds, action.replacedClientIds ),
...fillKeysWithEmptyObject( keys( flattenBlocks( action.blocks ) ) ),
Copy link
Contributor Author

@talldan talldan Jul 23, 2019

Choose a reason for hiding this comment

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

To explain a bit what I've done here:

  1. The replacedClientIds needs to be used to find parentClientIds, but then replacedClientIds also need to be omitted, hence the second call to omit here.
  2. The new block clientId needs to be included in the cache, and there's a unit test should replace the block even if the new block clientId is the same, so that's why the last object spread is adding the new client id.

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic seems good to me.

@talldan talldan added this to the Gutenberg 6.2 milestone Jul 23, 2019
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Tested with columns and nested columns, by pasting content in, saving and previewing, and it's working fine.
Code looks good, thanks for documenting everything so thoroughly!
A thought about the reducer unit tests in general: I wonder if we shouldn't be more granular with what we're testing. There's a huge amount of assertions inside some of those tests, so if they fail, it's not immediately obvious why. E.g. should we have a test to check caching in block replacement, instead of having that check be part of the block replacement test?

@youknowriad youknowriad merged commit 297396b into master Jul 26, 2019
@youknowriad youknowriad deleted the fix/pasting-into-inner-blocks branch July 26, 2019 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Package] Block editor /packages/block-editor [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting text into columns isn't showing up on frontend
3 participants