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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f7275f5
Copy handler: only handle paste event once
gwwar Aug 31, 2021
d3c2cbc
Add e2e test to verify we only handle paste once when pasting into ri…
gwwar Sep 8, 2021
d467a4b
Add e2e to verify that we can paste groups onto nontextual elements
gwwar Sep 8, 2021
1a915bc
Add some comments and add a snapshot check
gwwar Sep 9, 2021
7124cec
save default prevented and check to see if paste was already handleed
gwwar Sep 10, 2021
ed69a12
Try handling the case when event.cancellable is false
gwwar Sep 16, 2021
13e353f
E2E: make sure clipboard events are cancellable
gwwar Sep 16, 2021
c8114ad
E2E: also check snapshot after a cut
gwwar Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/copy-handler/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export function useClipboardHandler() {
return;
}

const eventDefaultPrevented = event.defaultPrevented;
event.preventDefault();
gwwar marked this conversation as resolved.
Show resolved Hide resolved

if ( event.type === 'copy' || event.type === 'cut' ) {
Expand All @@ -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

// This was likely already handled in rich-text/use-paste-handler.js
return;
}
const {
__experimentalCanUserUseUnfilteredHTML: canUserUseUnfilteredHTML,
} = getSettings();
Expand Down
1 change: 1 addition & 0 deletions packages/e2e-test-utils/src/press-key-with-modifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

clipboardData: window._clipboardData,
} )
);
Expand Down
28 changes: 28 additions & 0 deletions ...ges/e2e-tests/specs/editor/various/__snapshots__/copy-cut-paste-whole-blocks.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Copy/cut/paste of whole blocks can copy group onto non textual element (image, spacer) 1`] = `""`;

exports[`Copy/cut/paste of whole blocks can copy group onto non textual element (image, spacer) 2`] = `
"<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>P</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->"
`;

exports[`Copy/cut/paste of whole blocks should copy and paste individual blocks 1`] = `
"<!-- wp:paragraph -->
<p>Here is a unique string so we can test copying.</p>
Expand Down Expand Up @@ -56,6 +70,20 @@ exports[`Copy/cut/paste of whole blocks should cut and paste individual blocks 2
<!-- /wp:paragraph -->"
`;

exports[`Copy/cut/paste of whole blocks should handle paste events once 1`] = `""`;

exports[`Copy/cut/paste of whole blocks should handle paste events once 2`] = `
"<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>P</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->"
`;

exports[`Copy/cut/paste of whole blocks should respect inline copy in places like input fields and textareas 1`] = `
"<!-- wp:shortcode -->
[my-shortcode]
Expand Down
92 changes: 92 additions & 0 deletions packages/e2e-tests/specs/editor/various/copy-cut-paste-whole-blocks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,96 @@ describe( 'Copy/cut/paste of whole blocks', () => {
await pressKeyWithModifier( 'primary', 'v' );
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should handle paste events once', async () => {
// Add group block with paragraph
await insertBlock( 'Group' );
await page.click( '.block-editor-button-block-appender' );
await page.click( '.editor-block-list-item-paragraph' );
await page.keyboard.type( 'P' );
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

expect( await getEditedPostContent() ).toMatchSnapshot();

await page.keyboard.press( 'Enter' );

await page.evaluate( () => {
window.e2eTestPasteOnce = [];
let oldBlocks = wp.data.select( 'core/block-editor' ).getBlocks();
wp.data.subscribe( () => {
const blocks = wp.data
.select( 'core/block-editor' )
.getBlocks();
if ( blocks !== oldBlocks ) {
window.e2eTestPasteOnce.push(
blocks.map( ( { clientId, name } ) => ( {
clientId,
name,
} ) )
);
}
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


// Paste
await pressKeyWithModifier( 'primary', 'v' );

// Blocks should only be modified once, not twice with new clientIds on a single paste action
const blocksUpdated = await page.evaluate(
() => window.e2eTestPasteOnce
);

expect( blocksUpdated.length ).toEqual( 1 );
gwwar marked this conversation as resolved.
Show resolved Hide resolved
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'can copy group onto non textual element (image, spacer)', async () => {
// Add group block with paragraph
await insertBlock( 'Group' );
await page.click( '.block-editor-button-block-appender' );
await page.click( '.editor-block-list-item-paragraph' );
await page.keyboard.type( 'P' );
await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.press( 'ArrowLeft' );
// Cut group
await pressKeyWithModifier( 'primary', 'x' );
expect( await getEditedPostContent() ).toMatchSnapshot();

await page.keyboard.press( 'Enter' );

// Insert a non textual element (a spacer)
await insertBlock( 'Spacer' );
// Spacer is focused
await page.evaluate( () => {
window.e2eTestPasteOnce = [];
let oldBlocks = wp.data.select( 'core/block-editor' ).getBlocks();
wp.data.subscribe( () => {
const blocks = wp.data
.select( 'core/block-editor' )
.getBlocks();
if ( blocks !== oldBlocks ) {
window.e2eTestPasteOnce.push(
blocks.map( ( { clientId, name } ) => ( {
clientId,
name,
} ) )
);
}
oldBlocks = blocks;
} );
} );

await pressKeyWithModifier( 'primary', 'v' );

// Paste should be handled on non-textual elements and only handled once.
const blocksUpdated = await page.evaluate(
() => window.e2eTestPasteOnce
);

expect( blocksUpdated.length ).toEqual( 1 );
expect( await getEditedPostContent() ).toMatchSnapshot();
} );
} );