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

Multiple block select cut and paste failing #53774

Open
annezazu opened this issue Aug 17, 2023 · 15 comments
Open

Multiple block select cut and paste failing #53774

annezazu opened this issue Aug 17, 2023 · 15 comments
Assignees
Labels
[Feature] Paste [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@annezazu
Copy link
Contributor

annezazu commented Aug 17, 2023

Description

Using both GB 16.4 and just 6.3, cut and paste fails when selecting multiple blocks by removing one of the items trying to be pasted. Tested thus far on paragraph, headings, and quote with various results.

Step-by-step reproduction instructions

  1. Open any site using 6.3.
  2. Create a new post.
  3. Create multiple paragraph or heading blocks.
  4. Try to cut and paste.
  5. Notice content is missing.

Screenshots, screen recording, code snippet

Here's a video where I attempt to show all three scenarios with paragraph, heading (which is impacted by #48040), quote, and list:

heading.crash.mov
  • Paragraph: content loss.
  • Heading: pasting causes editor to crash.

Here's the error:

Error: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
at $r (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:75182)
at jr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74867)
at $r (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:75624)
at jr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74867)
at $r (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:75624)
at jr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74867)
at $r (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:75624)
at jr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74867)
at $r (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:75624)
at jr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74867)

In another attempt, it failed when pasting List items with this error:

Error: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74697)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74802)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)
at Qr (https://patarantula-risu.instawp.xyz/wp-includes/js/dist/vendor/react-dom.min.js?ver=18.2.0:10:74770)

Environment info

  • WP 6.3
  • Chrome
  • MacOS

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@annezazu annezazu added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention [Feature] Paste labels Aug 17, 2023
@mcsf
Copy link
Contributor

mcsf commented Aug 17, 2023

cc @ellatrix, in case this is related to any of the recent contentEditable or other flow-related fixes.

@annezazu annezazu added the [Type] Regression Related to a regression in the latest release label Aug 17, 2023
@ellatrix
Copy link
Member

Seeing that error, I’m guessing the browser is cutting the blocks instead of us, and React can no longer reconcile. The lack of snackbar notification supports that.

Can't reproduce though. Tried Chrome, Firefox and Safari. Which browser is this?

@annezazu
Copy link
Contributor Author

annezazu commented Aug 17, 2023

Noting that keyboard shortcuts are fine but, if you right click > cut, something goes wrong. Able to replicate in Chrome.

@ellatrix
Copy link
Member

ellatrix commented Aug 17, 2023

Ok, here's my report after debugging:

This bug is fixed by #53440, but it's an accidental fix and I believe not a full fix.

The bug was introduced by #51201, more specifically by 006b2a7, which actually prevents native browser behaviour on cut so we do it ourselves. That is necessary so we can set the clipboard with HTML not just on copy but on cut too (fixing a bug pre-#51201). This meant to be for non multi select only, and then it's OK.

The problem is that for some reason the browser moves focus to the block's rich text wrapper (from the multi select wrapper) on click-click (opening the context menu). That's why it's triggering that rich text copy event handler instead of the global copy handler. And that's why #53440 also fixes the bug, because the browser is no longer moving focus away from the multi select handler. Somehow tabIndex within contentEditable seems to confuse the browser, thinking the element should receive focus, and this is not just Safari.

This is also why, btw, only one block was copied, because one of the block's rich text copy event handler was triggered instead of the global one.

@azaozz
Copy link
Contributor

azaozz commented Aug 17, 2023

Just to add that I can reproduce in Chromium in the block editor in 6.3 by using the right-click menu to cut and paste. Doesn't happen in Firefox as it seems there multiple blocks cannot be cut from the right-click menu. The menu item is disabled.

@ellatrix
Copy link
Member

@azaozz Yes, it's a bug in 6.3, but fixed by #53440.

I'm actually starting to think it is a good fix, because it prevents the rich text from gaining focus by removing the tab index. But it's still allowing tabindex higher than 0 through.

To me this seems like an issue that should ideally be fixed at the browser level. An element with tabindex should never gain focus when wrapped in content editable. Content editable should alway disable all interactivity except select (no link following, no focus etc.)

It should actually be safe for us to remove the tabIndex attribute altogether from all block except for a single selected block. Having this attribute on all other blocks is completely useless since these blocks cannot be tabbed to anyway.

@ellatrix
Copy link
Member

Ok, here's proof that it's not entirely caused by #51201, it just aggravated the issue for cut. But pre-#53440, copying through the context menu was also broken: it will just copy only one block instead of the whole multi selection, and there would be no snackbar notification. Same bug where focus is moved because of tabindex. #51201 only touched cut.

@annezazu
Copy link
Contributor Author

Where are we in addressing this? It's unclear to me what's left to do to get this into a point release (6.3.2).

@ellatrix
Copy link
Member

I had commented in the #6-3-release-leads channel: https://wordpress.slack.com/archives/C051Z1SKBDZ/p1692380073185779?thread_ts=1692287531.089819&cid=C051Z1SKBDZ

I think that fix is indeed good to backport. This may also be good to have, but not sure if it’s needed for backport: Multi select: prevent blocks from gaining focus

@draganescu
Copy link
Contributor

Is this problem solved? I tested and the problem seems to be gone.

@ellatrix
Copy link
Member

ellatrix commented Oct 3, 2023

@draganescu I explained this in my comment #53774 (comment)

@draganescu
Copy link
Contributor

@ellatrix I don't get it 😂 I understand it's fixed, but I don't understand why this needs to remain open.

@ellatrix
Copy link
Member

ellatrix commented Oct 4, 2023

It was an accidental fix. If it's good enough, we should probably add an e2e test so it doesn't accidentally regress in the future.

@annezazu
Copy link
Contributor Author

@ellatrix can we loop back and add an e2e test to close this out? I'm going to remove the high priority label from this issue for now either way as it's no longer something we need to fix urgently and quickly per this sweep of the label.

@annezazu annezazu removed the [Priority] High Used to indicate top priority items that need quick attention label Dec 19, 2023
@ellatrix ellatrix self-assigned this Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Paste [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
No open projects
Status: 🏗️ In Progress
Development

Successfully merging a pull request may close this issue.

5 participants