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

Hitting enter in text block updates the block's client id. #26252

Closed
TimothyBJacobs opened this issue Oct 17, 2020 · 6 comments
Closed

Hitting enter in text block updates the block's client id. #26252

TimothyBJacobs opened this issue Oct 17, 2020 · 6 comments
Labels
[Block] Paragraph Affects the Paragraph Block [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Bug An existing feature does not function as intended

Comments

@TimothyBJacobs
Copy link
Member

Describe the bug
If my selection is in a text block, hitting the enter button to create a new paragraph updates the clientId of the original text block.

To reproduce

  1. Create a new paragraph block.
  2. Open the inspector and look for the data-block attribute. Record it's value.
  3. Position your cursor at the end of the paragraph block.
  4. Press return.
  5. Look for the data-block attribute of your first block. Notice it is now different.

Expected behavior
The client id should stay the same.

Editor version (please complete the following information):

  • WordPress version: 5.5.1
  • Gutenberg plugin
  • master.

Desktop (please complete the following information):

  • OS: MacOS
  • Browser: Safari
  • Version: 14

Additional context

This completely breaks the Widgets screen as it relies on a stable clientId to map widget IDs to block. Otherwise, duplicate blocks end up being created.

https://i.imgur.com/1TJXvG7.gifv

@TimothyBJacobs TimothyBJacobs added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Block] Paragraph Affects the Paragraph Block labels Oct 17, 2020
@TimothyBJacobs TimothyBJacobs added this to To do in WordPress 5.6.x Must Haves via automation Oct 17, 2020
@TimothyBJacobs TimothyBJacobs added this to Inbox in Block-based Widgets Editor via automation Oct 17, 2020
@talldan
Copy link
Contributor

talldan commented Oct 19, 2020

This is also reproducible in the post editor. This seems to be caused by how RichText's onSplit prop works:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/paragraph/edit.js#L172-L181

The callback will be triggered twice once for the first part of the text, and again for the second part. The original block is then replaced with the two new blocks:
https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/rich-text/index.js#L348

So I guess the proposal would be to preserve the first block's clientId, but it seems like a pretty big API change at this stage.

cc @ellatrix.

@talldan
Copy link
Contributor

talldan commented Oct 19, 2020

Just to add as well, this wouldn't be just an issue with the paragraph block, but also any block that implements onSplit, even third party blocks. To solve the problem everywhere, I think the solution would need to be in this bit of code:

if ( ! hasPastedBlocks || ! isEmpty( before ) ) {
blocks.push(
onSplit(
toHTMLString( {
value: before,
multilineTag,
} )
)
);
lastPastedBlockIndex += 1;
}

Probably merging the block created as a result of calling onSplit on before with the current block at that position if they have the same block type. Not sure if that would have any repercussions elsewhere in the codebase.

@TimothyBJacobs
Copy link
Member Author

Is there a way to listen for a split? Maybe in just edit-widgets we could listen for a block spit and update the clientId syncing. We'd need the clientId of the block that split and the new blocks created. That'd localize the change to somewhere potentially less impactful if it does cause the world to fall.

@ellatrix
Copy link
Member

What's the use case for this? Can't it be seen as removing a block and replacing it with two new ones?

@TimothyBJacobs
Copy link
Member Author

The menus and widget screens rely on a stable mapping from persisted entities to block ids. If that persistence breaks, the old widget/menu-item is deleted and recreated. This can cause a couple of issues.

  1. If there is a partial save error, duplicate entities end up being created.
  2. If there was anything attached to that entities' id, like metadata, that won't be there anymore. Or if anything references those ids, that tracking will be lost.
  3. It causes id inflation, there is no reason to create and delete the same entity multiple times in a row.

@noisysocks noisysocks removed the [Priority] High Used to indicate top priority items that need quick attention label Jan 6, 2021
@noisysocks noisysocks moved this from Inbox to Needs dev in Block-based Widgets Editor Jan 6, 2021
@grzim grzim self-assigned this Jan 8, 2021
@grzim grzim removed their assignment Jan 15, 2021
@kevin940726
Copy link
Member

I think we can now close it as the widget screen doesn't rely on clientId anymore, and that #28505 is also merged which should fix the issue in the post editor.

The original issue with the widget screen is not fixed yet though. I opened #30268 to better track it. I think we can close this as duplicated.

Block-based Widgets Editor automation moved this from Needs dev to Done Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Paragraph Affects the Paragraph Block [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

No branches or pull requests

6 participants