-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Writing flow: fix paste for input fields #61389
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
const hasSelection = | ||
event.type === 'copy' || event.type === 'cut' | ||
? documentHasUncollapsedSelection( ownerDocument ) | ||
: documentHasSelection( ownerDocument ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this line to documentHasSelection( ownerDocument ) && ! ownerDocument.activeElement.isContentEditable
while also running it for the paste event as we did before #54543.
Size Change: +2 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
4d43223
to
2c38d9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ellatrix!
The fix works when manually testing the changes. The e2e tests are failing due to pasting not working; nothing gets pasted into the input field.
The embed block e2e test isn't a great place to test this regression. Embeddable URLs in paragraphs are auto-embedded. Let's add a separate test in writing flow specs for the RSS block.
// See: https://github.com/WordPress/gutenberg/pull/61389.
test( 'should retain native copy/paste behavior for input fields', async ( {
editor,
pageUtils,
} ) => {
await editor.insertBlock( { name: 'core/rss' } );
pageUtils.setClipboardData( {
plainText: 'https://developer.wordpress.org/news/feed/',
} );
await pageUtils.pressKeys( 'primary+v' );
await expect.poll( editor.getBlocks ).toMatchObject( [
{
name: 'core/rss',
attributes: {
feedURL: 'https://developer.wordpress.org/news/feed/',
},
},
] );
} );
P.S. It seems your local trunk is a little behind. I had to rebase so I could switch between branches.
// Do not use `fill` here, it's not how the user interacts with the | ||
// block. | ||
this.#pageUtils.setClipboardData( { plainText: url } ); | ||
await this.#pageUtils.pressKeys( 'primary+v' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin940726 reminded me that this currently doesn't work on regular elements. See #55030.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, interesting. I fixed it a bit differently here: 3bed7b6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin940726, do you mind having a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid it's not that simple. It's a cross-browser and cross-os compatibility issue too. PR #55030 should work fine but it's too complicated to review. I haven't had the time to refactor it to a simpler form. I think it's okay to omit this change and just add the test.skip
and a comment for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why skip? It actually tests a regression? Could you elaborate on why it's not that simple? Currently all our copy/paste tests are emulated and we manually fill in data, so how is this different?
This is a pretty bad regression, so we should get at least the fix in asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind merging this as is 👍. I just think that the test might not have good coverage after all if we're "designing" the test in a way that it should pass. I guess it's hard to argue against not having the test at all though 😆.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I agree there. We're "designing" all copy/paste tests to pass then? We're actually manually emulating everything, this is no different? For example for copy/cut, we're manually setting a clipboard instead of the browser doing the default behaviour. The test assures that default behaviour is not prevented, and we emulate the default behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I chose the wrong words😅. I couldn't find a better word. What I mean is that for this particular use case, we might not want to intercept what Playwright already does. It's to prevent future breakage of our tests when the bundled browsers update. The more robust way might be to feature-detect it in our utils, but even that is hard to do right. I hope that this convo didn't sound discouraging or blaming! I'm just not sure what the best approach is at the time, but I'm positive that we can find one! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at all! ❤️ Just saying I prefer to err on the side of adding a test, even if it's not perfect, but I'm open to your util changes as a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I'll try to experiment with it some more another time 😅.
2c38d9d
to
3bed7b6
Compare
Flaky tests detected in 3bed7b6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9176157396
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've agreed that this fix should be merged ASAP, and it fixes the issue for me when manually testing. ✅
Screen.Recording.2024-05-24.at.10.18.59.AM.mov
Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: allilevine <firewatch@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: liviopv <liviopv@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: allilevine <firewatch@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: liviopv <liviopv@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: allilevine <firewatch@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: liviopv <liviopv@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
What?
Fixes #61377. I don't remember exactly why I removed this check for the paste event.
Why?
How?
Turns out I shouldn't have removed this condition for the paste event. We still don't want to handle paste for input and textarea elements, but we do want to override it for
contenteditable
.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast