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

Copy Handler: only handle paste event once #34430

Merged
merged 8 commits into from
Sep 17, 2021
Merged

Copy Handler: only handle paste event once #34430

merged 8 commits into from
Sep 17, 2021

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Aug 31, 2021

When copy and pasting blocks I noticed that we were dispatching two REPLACE_BLOCKS actions, 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:

onReplace( content, content.length - 1, -1 );

replaceBlocks(
selectedBlockClientIds,
blocks,
blocks.length - 1,
-1
);

Testing Instructions

  • Copy blocks that have innerBlocks. A quick way of doing this is dragging a block pattern and copying it.
  • Start with a fresh empty post and paste the blocks
  • Verify that blocks are inserted as expected, and only insert once. (the block clientIds do not change).
  • Verify that no regressions are present (multi-select paste should work as expected).

@gwwar gwwar added the [Package] Block editor /packages/block-editor label Aug 31, 2021
@gwwar gwwar self-assigned this Aug 31, 2021
@github-actions
Copy link

github-actions bot commented Aug 31, 2021

Size Change: +288 B (0%)

Total Size: 1.06 MB

Filename Size Change
build/block-editor/index.min.js 128 kB +4 B (0%)
build/block-editor/style-rtl.css 13.9 kB +40 B (0%)
build/block-editor/style.css 13.8 kB +41 B (0%)
build/block-library/blocks/navigation/view.min.js 2.74 kB +16 B (+1%)
build/block-library/blocks/query-pagination/editor-rtl.css 262 B -39 B (-13%) 👏
build/block-library/blocks/query-pagination/editor.css 255 B -37 B (-13%) 👏
build/block-library/blocks/query-pagination/style-rtl.css 234 B -25 B (-10%) 👏
build/block-library/blocks/query-pagination/style.css 231 B -26 B (-10%) 👏
build/block-library/editor-rtl.css 9.71 kB -15 B (0%)
build/block-library/editor.css 9.7 kB -10 B (0%)
build/block-library/index.min.js 153 kB +68 B (0%)
build/block-library/style-rtl.css 10.3 kB -7 B (0%)
build/block-library/style.css 10.3 kB -7 B (0%)
build/components/index.min.js 209 kB +23 B (0%)
build/edit-navigation/index.min.js 16.3 kB +35 B (0%)
build/edit-navigation/style-rtl.css 3.7 kB +55 B (+2%)
build/edit-navigation/style.css 3.7 kB +53 B (+1%)
build/edit-site/index.min.js 26.4 kB +120 B (0%)
build/rich-text/index.min.js 10.6 kB -1 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 931 B
build/admin-manifest/index.min.js 1.09 kB
build/annotations/index.min.js 2.7 kB
build/api-fetch/index.min.js 2.19 kB
build/autop/index.min.js 2.08 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.2 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 474 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 600 B
build/block-library/blocks/button/style.css 600 B
build/block-library/blocks/buttons/editor-rtl.css 315 B
build/block-library/blocks/buttons/editor.css 315 B
build/block-library/blocks/buttons/style-rtl.css 370 B
build/block-library/blocks/buttons/style.css 370 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 206 B
build/block-library/blocks/columns/editor.css 205 B
build/block-library/blocks/columns/style-rtl.css 497 B
build/block-library/blocks/columns/style.css 496 B
build/block-library/blocks/cover/editor-rtl.css 666 B
build/block-library/blocks/cover/editor.css 670 B
build/block-library/blocks/cover/style-rtl.css 1.23 kB
build/block-library/blocks/cover/style.css 1.23 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 983 B
build/block-library/blocks/gallery/editor.css 988 B
build/block-library/blocks/gallery/style-rtl.css 1.6 kB
build/block-library/blocks/gallery/style.css 1.59 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 70 B
build/block-library/blocks/group/theme.css 70 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 728 B
build/block-library/blocks/image/editor.css 728 B
build/block-library/blocks/image/style-rtl.css 482 B
build/block-library/blocks/image/style.css 487 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 488 B
build/block-library/blocks/media-text/style.css 485 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 489 B
build/block-library/blocks/navigation-link/editor.css 488 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 300 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/style-rtl.css 195 B
build/block-library/blocks/navigation-submenu/style.css 195 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.72 kB
build/block-library/blocks/navigation/editor.css 1.72 kB
build/block-library/blocks/navigation/style-rtl.css 1.5 kB
build/block-library/blocks/navigation/style.css 1.49 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 202 B
build/block-library/blocks/page-list/style.css 202 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/editor-rtl.css 210 B
build/block-library/blocks/post-author/editor.css 210 B
build/block-library/blocks/post-author/style-rtl.css 182 B
build/block-library/blocks/post-author/style.css 181 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B
build/block-library/blocks/post-comments-form/style.css 140 B
build/block-library/blocks/post-comments/style-rtl.css 360 B
build/block-library/blocks/post-comments/style.css 359 B
build/block-library/blocks/post-content/editor-rtl.css 138 B
build/block-library/blocks/post-content/editor.css 138 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 398 B
build/block-library/blocks/post-featured-image/editor.css 398 B
build/block-library/blocks/post-featured-image/style-rtl.css 143 B
build/block-library/blocks/post-featured-image/style.css 143 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 378 B
build/block-library/blocks/post-template/style.css 379 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 60 B
build/block-library/blocks/post-title/style.css 60 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-title/editor-rtl.css 85 B
build/block-library/blocks/query-title/editor.css 85 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 220 B
build/block-library/blocks/quote/theme.css 222 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 374 B
build/block-library/blocks/search/style.css 375 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 250 B
build/block-library/blocks/separator/style.css 250 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 462 B
build/block-library/blocks/site-logo/editor.css 464 B
build/block-library/blocks/site-logo/style-rtl.css 153 B
build/block-library/blocks/site-logo/style.css 153 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 165 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 812 B
build/block-library/blocks/social-links/editor.css 811 B
build/block-library/blocks/social-links/style-rtl.css 1.3 kB
build/block-library/blocks/social-links/style.css 1.3 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 636 B
build/block-library/blocks/template-part/editor.css 635 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/editor-rtl.css 90 B
build/block-library/blocks/term-description/editor.css 90 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 853 B
build/block-library/common.css 849 B
build/block-library/reset-rtl.css 527 B
build/block-library/reset.css 527 B
build/block-library/theme-rtl.css 658 B
build/block-library/theme.css 663 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46.9 kB
build/components/style-rtl.css 15.8 kB
build/components/style.css 15.8 kB
build/compose/index.min.js 10.3 kB
build/core-data/index.min.js 12.3 kB
build/customize-widgets/index.min.js 11.1 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 614 B
build/data/index.min.js 7.1 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 428 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.45 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 28.9 kB
build/edit-post/style-rtl.css 7.23 kB
build/edit-post/style.css 7.22 kB
build/edit-site/style-rtl.css 5.07 kB
build/edit-site/style.css 5.07 kB
build/edit-widgets/index.min.js 16.1 kB
build/edit-widgets/style-rtl.css 4.09 kB
build/edit-widgets/style.css 4.09 kB
build/editor/index.min.js 37.7 kB
build/editor/style-rtl.css 3.74 kB
build/editor/style.css 3.73 kB
build/element/index.min.js 3.17 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.34 kB
build/format-library/style-rtl.css 668 B
build/format-library/style.css 670 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.6 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.72 kB
build/keycodes/index.min.js 1.3 kB
build/list-reusable-blocks/index.min.js 1.85 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.88 kB
build/notices/index.min.js 845 B
build/nux/index.min.js 2.03 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.83 kB
build/primitives/index.min.js 921 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.63 kB
build/reusable-blocks/index.min.js 2.28 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/server-side-render/index.min.js 1.32 kB
build/shortcode/index.min.js 1.48 kB
build/token-list/index.min.js 562 B
build/url/index.min.js 1.74 kB
build/viewport/index.min.js 1.02 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.27 kB
build/widgets/style-rtl.css 1.17 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@gwwar gwwar marked this pull request as ready for review August 31, 2021 21:20
@gwwar gwwar requested a review from ellatrix as a code owner August 31, 2021 21:20
@gwwar gwwar changed the title Clipboard Handler: only replace blocks on paste with multi-select Copy Handler: only replace blocks on paste with multi-select Aug 31, 2021
@gwwar gwwar added the [Type] Bug An existing feature does not function as intended label Aug 31, 2021
@gwwar
Copy link
Contributor Author

gwwar commented Aug 31, 2021

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.

@ellatrix
Copy link
Member

ellatrix commented Sep 1, 2021

With this change, it doesn't seem possible to paste in a block without a rich text field. Would it be possible to use event.preventDefault() and then check event.defaultPrevented in the second call to check if the event has already been handled?

We don't have so many paste tests in place, and the ones we have are spread over: multi-block-selection.test.js, rich-text.test.js and copy-cut-paste-whole-blocks.test.js. We could either add to those files or create a new one dedicated to copy/paste.

@gwwar
Copy link
Contributor Author

gwwar commented Sep 1, 2021

Would it be possible to use event.preventDefault() and then check event.defaultPrevented in the second call to check if the event has already been handled?

Happy to take a look and add tests for the two cases if I can find a good spot.

@gwwar gwwar force-pushed the fix/handle-paste-once branch from 29b8803 to 8010a59 Compare September 2, 2021 22:44
@gwwar gwwar changed the title Copy Handler: only replace blocks on paste with multi-select Copy Handler: only handle paste event once Sep 2, 2021
@gwwar
Copy link
Contributor Author

gwwar commented Sep 2, 2021

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.

@gwwar gwwar force-pushed the fix/handle-paste-once branch from 8010a59 to 0d1c61f Compare September 8, 2021 22:43
@gwwar
Copy link
Contributor Author

gwwar commented Sep 9, 2021

This one is ready for another look @ellatrix. From testing I found that event.defaultPrevented was already set in dev mode, but wasn't in the e2e environment. So, I ended up trying to gate an early exit by seeing if the event.target originated from a rich text component. I'm not sure if that's too wp specific, but open to other ideas if folks had any.

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.

@gwwar
Copy link
Contributor Author

gwwar commented Sep 10, 2021

Looks like there are some unit test failures, will investigate in a bit.

@Mamaduka
Copy link
Member

Looks like there are some unit test failures, will investigate in a bit.

It should be fixed once #34745 is merged.

@gwwar
Copy link
Contributor Author

gwwar commented Sep 10, 2021

Thanks for the heads up @Mamaduka

@getdave
Copy link
Contributor

getdave commented Sep 10, 2021

Looks like there are some unit test failures, will investigate in a bit.

It should be fixed once #34745 is merged.

All done

@gwwar gwwar force-pushed the fix/handle-paste-once branch from d0d16cf to c752a91 Compare September 11, 2021 02:49
@gwwar gwwar force-pushed the fix/handle-paste-once branch from c752a91 to 7124cec Compare September 16, 2021 16:19
@@ -126,6 +126,7 @@ async function emulateClipboard( type ) {
document.activeElement.dispatchEvent(
new ClipboardEvent( _type, {
bubbles: true,
cancelable: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the event option is not specified cancelable defaults to false.

await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.press( 'ArrowLeft' );
// Cut group
await pressKeyWithModifier( 'primary', 'x' );
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, updated in c8114ad

}
oldBlocks = blocks;
} );
} );
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -130,6 +131,10 @@ export function useClipboardHandler() {
if ( event.type === 'cut' ) {
removeBlocks( selectedBlockClientIds );
} else if ( event.type === 'paste' ) {
if ( eventDefaultPrevented ) {
Copy link
Member

Choose a reason for hiding this comment

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

Curious: why not directly use event.defaultPrevented here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@mcsf mcsf linked an issue Sep 17, 2021 that may be closed by this pull request
2 tasks
@gwwar
Copy link
Contributor Author

gwwar commented Sep 17, 2021

Looks like this branch should fix #34177. Here's what I see:

Before After
CleanShot 2021-09-17 at 09 28 55 CleanShot 2021-09-17 at 09 26 12

@gwwar
Copy link
Contributor Author

gwwar commented Sep 17, 2021

Thanks for the review @ellatrix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copied blocks get duplicated when pasted inside empty page
4 participants