Fix clipboard read/write on regular elements in e2e tests #55030
Conversation
This file contains 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
kevin940726
added
[Type] Bug
kevin940726
removed
the
[Type] Bug
|
Size Change: +184 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 745f8da. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6875697803
|
|
Okay it doesn't work on CI 😅 . I'll keep looking for alternatives. |
|
Thanks for identifying and working on this! It stumped me yesterday 😅 |
kevin940726
force-pushed
the
fix/playwright-clipboard-read-write
branch
3 times, most recently
from
fcd0c36
to
b965e4b
Compare
getdave
reviewed
Oct 12, 2023
There was a problem hiding this comment.
I tested this by applying the changes to press-keys.ts against the work in #54718.
I can confirm that the test that is .skip will pass 🎉 .
As such this feels like an improvement.
I'm not sure I'm best placed to review the technical implementation details to I've cc'd @WunderBart who has done a lot of work in this area in the hope he can assist.
Great work 🙇
|
@kevin940726, can you rebase this on top of the trunk? Let's ensure that changes here work as expected with the latest Playwright. |
kevin940726
force-pushed
the
fix/playwright-clipboard-read-write
branch
from
2db7526
to
55c091d
Compare
Mamaduka
reviewed
Nov 15, 2023
Comment on lines
+94
to
+96
| const timeout = setTimeout( () => { | ||
| resolve( false ); | ||
| }, 50 ); |
There was a problem hiding this comment.
This is just a safeguard to resolve the Promise if the method isn't able to add an event listener, primarily due to activeElement being unavailable. Is my logic/understanding here correct?
There was a problem hiding this comment.
No, this is here in case the event never fires. This could happen for paste events when the element doesn't attach an event listener and it needs to fallback to native "real" event. We need a flag here that basically means hasEventHandled, and it that's false we'll fire a real key press.
I know this is all very confusing and I've been thinking about better ways to document this 😅. Maybe I should make a comparison table or something like that...
There was a problem hiding this comment.
Any additional documentation would be helpful here.
Mamaduka
reviewed
Nov 15, 2023
| @@ -450,7 +450,7 @@ test.describe( 'Copy/cut/paste', () => { | |||
| // back to default browser behaviour, allowing the browser to insert | |||
| // unfiltered HTML. When we swap out the post title in the post editor | |||
| // with the proper block, this test can be removed. | |||
| pageUtils.setClipboardData( { | |||
| await pageUtils.setClipboardData( { | |||
There was a problem hiding this comment.
I'm seeing Unexpected await of a non-Promise (non-"Thenable") value. ESLint errors here.
There was a problem hiding this comment.
This is possibly due to outdated build. Try running npm run dev or npm run build again and see if eslint still errors?
Mamaduka
approved these changes
Nov 15, 2023
There was a problem hiding this comment.
Thank you, @kevin940726!
The changes look good to me (thanks for the inline comments). The original RichText behavior works as before, which big plus ➕
Having test cases for regular elements would have been nice, but not a blocker.
|
I can still see some instances where the |
|
Since the basic clipboard support via shortcuts seems to be there, I was wondering if a (what might be a simpler) ctrl-c ctrl-v would work now. I tried to make a very simple PoC and it seems to be working well (at least for this case): test( 'should turn selected text into a link', async ( { page, editor } ) => {
// Set content to copy (could be turned into the setClipboardContent util?)
const tmpPage = await page.context().newPage();
await tmpPage.setContent( 'https://wordpress.org' );
await tmpPage.keyboard.press( 'Meta+A' );
await tmpPage.keyboard.press( 'Meta+C' );
await tmpPage.close();
await editor.canvas
.getByRole( 'button', { name: 'Add default block' } )
.click();
await page.keyboard.type('WordPress');
await page.keyboard.press( 'Meta+A' );
await page.keyboard.press( 'Meta+V' );
await page.pause(); // see the WordPress link (when headed :D)
} );Please ignore me if this has already been considered and didn't work. 😅 If the current implementation works as expected, let's go with it! 👍 |
Nice catch! These tests were added after this PR so I forgot about them 😅. The tests could pass if it's running fast enough, but still better to be awaited.
I tried something like this but unfortunately it didn't work on Linux (CI) for some reason. Happy to revalidate this if you manage to get it work though! You can try the official docker image to run it in an environment very close to what's on CI. docker run --network host --rm -it -v $(pwd):/work/ -w /work/ mcr.microsoft.com/playwright:v1.39.0 /bin/bash |
Thanks for the snippet - very useful! I was able to make the ctrl-c ctrl-v work there as well, but it does seem to behave a bit differently than on my local machine. Let's go with this one and maybe actually wait for the official support 😅 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Currently, pasting values from clipboard to regular elements with no
pasteevent handlers won't work as expected. This PR fixes that.Why?
See #54718 (comment).
How?
It's actually really tricky. The solution doesn't always work but it works in most cases which is good enough for us.
The key is to fire both the actual key press (
primary+v) and the clipboard emulation event (dispatchEvent) together. The order is different for writing to the clipboard (copyandcut) and reading from the clipboard (paste) though. The actual reasoning can be shown in the inline comments, but it's still fairly complicated 😅 . It still took multiple retries to make it work on CI. Feel free to ask any questions!Testing Instructions
CI should pass, hopefully 🤞 .
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A