Copy Handler: only handle paste event once#34430
Merged
Conversation
|
Size Change: +288 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
Contributor
Author
|
If this change is needed, let me know if there's a good E2E suite to add a test to. I see ``packages/e2e-tests/specs/editor/various/copy-cut-paste-whole-blocks.test.js` but open to other suggestions. |
Member
|
With this change, it doesn't seem possible to paste in a block without a rich text field. Would it be possible to use We don't have so many paste tests in place, and the ones we have are spread over: |
Contributor
Author
Happy to take a look and add tests for the two cases if I can find a good spot. |
29b8803 to
8010a59
Compare
Contributor
Author
|
It looks like we already prevent default for both cases, but I think it's reasonable to check and see if the event.target is a rich text instance. 8010a59 I'll finish adding tests next week, but let me know if I'm missing any other cases. |
8010a59 to
0d1c61f
Compare
Contributor
Author
|
This one is ready for another look @ellatrix. From testing I found that The E2E implementation wasn't the cleanest either, but I did add cases to catch only inserting blocks once/ensuring we handle paste on non-text elements. I'm also open to other ideas if we can think of ways to test this without using a store subscribe. These test cases are a bit of a mix of needing an E2E for the paste behavior but still wanting some white box testing. I'm more in favor of sticking with E2Es instead of unit tests with too many mocks for this particular scenario. |
ellatrix
reviewed
Sep 10, 2021
gwwar
commented
Sep 10, 2021
Contributor
Author
|
Looks like there are some unit test failures, will investigate in a bit. |
Member
It should be fixed once #34745 is merged. |
Contributor
Author
|
Thanks for the heads up @Mamaduka |
Contributor
All done |
d0d16cf to
c752a91
Compare
c752a91 to
7124cec
Compare
gwwar
commented
Sep 16, 2021
packages/e2e-tests/specs/editor/various/copy-cut-paste-whole-blocks.test.js
Show resolved
Hide resolved
gwwar
commented
Sep 16, 2021
| document.activeElement.dispatchEvent( | ||
| new ClipboardEvent( _type, { | ||
| bubbles: true, | ||
| cancelable: true, |
Contributor
Author
There was a problem hiding this comment.
When the event option is not specified cancelable defaults to false.
ellatrix
reviewed
Sep 17, 2021
| await page.keyboard.press( 'ArrowLeft' ); | ||
| await page.keyboard.press( 'ArrowLeft' ); | ||
| // Cut group | ||
| await pressKeyWithModifier( 'primary', 'x' ); |
Member
There was a problem hiding this comment.
Generally, after cutting, it's good to test setup with getEditedPostContent to make sure that something happened. If we copy/paste and check the content only after that, it's possible that nothing at all has happened.
ellatrix
reviewed
Sep 17, 2021
| } | ||
| oldBlocks = blocks; | ||
| } ); | ||
| } ); |
Member
There was a problem hiding this comment.
I wonder how useful testing for this is, if there's no consequences for the content, it doesn't really matter how many times blocks are replaced. It's mostly a purity thing that doesn't really justify all the additional test code to maintain.
Contributor
Author
There was a problem hiding this comment.
Yes, this is a bit fragile, but I did want some tests to document the behavior since it's a bit of an odd side effect to consider. I'll see if I can get the E2Es to be stable and we can revisit if its flaky.
Overall, fixing this is more minor though I do see that this branch also fixes #34177
ellatrix
reviewed
Sep 17, 2021
| if ( event.type === 'cut' ) { | ||
| removeBlocks( selectedBlockClientIds ); | ||
| } else if ( event.type === 'paste' ) { | ||
| if ( eventDefaultPrevented ) { |
Member
There was a problem hiding this comment.
Curious: why not directly use event.defaultPrevented here?
Contributor
Author
There was a problem hiding this comment.
On line 116 in this file we call event.preventDefault, so I store what this value is before we do so. Alternatively we can call event.preventDefault() before each action, but it may be easy to forget to call it when there are multiple exit points
ellatrix
approved these changes
Sep 17, 2021
2 tasks
Contributor
Author
|
Looks like this branch should fix #34177. Here's what I see:
|
Contributor
Author
|
Thanks for the review @ellatrix! |
This was referenced Nov 27, 2023
This was referenced Nov 27, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When copy and pasting blocks I noticed that we were dispatching two
REPLACE_BLOCKSactions, causing state to be updated two times. I'm relatively sure that the second handling is unintentional when we only have one block selected. In the copy handler the event.target points to the old paragraph block instead of the newly inserted blocks.before.mp4
after.mp4
The replace block actions are coming from:
gutenberg/packages/block-editor/src/components/rich-text/use-paste-handler.js
Line 208 in 01040b8
gutenberg/packages/block-editor/src/components/copy-handler/index.js
Lines 144 to 149 in 01040b8
Testing Instructions