Paste Handler: Try to fix pasting text with formatting #63779
Merged
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
t-hamano
added
[Type] Bug
|
Size Change: +369 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
@ellatrix It appears to pass all tests. Could removing this early return cause any problems? |
ellatrix
reviewed
Jul 21, 2024
| @@ -27,12 +27,6 @@ export default ( props ) => ( element ) => { | |||
| pastePlainText, | |||
| } = props.current; | |||
|
|
|||
| // The event listener is attached to the window, so we need to check if | |||
| // the target is the element. | |||
There was a problem hiding this comment.
I think this is a valid comment. I probably thought the target would be a focusable element and always the wrapper, but turns out not. Maybe change it to:
! element.contains( event.target )To make sure we only catch paste within the container.
There was a problem hiding this comment.
Btw, there's a similar early return in enter.js. I wonder if that's causing any bugs?
There was a problem hiding this comment.
! element.contains( event.target )
I have confirmed that this approach also works correctly. After a bit of debugging, it appears that element.target is sometimes an inner element, rather than the focusable element itself.
Btw, there's a similar early return in
enter.js. I wonder if that's causing any bugs?
I've tried debugging close to this code, and as far as I've tested, element.target is always the focusable element itself, so it doesn't seem to be causing any kind of bug.
ellatrix
added
[Type] Regression
t-hamano
changed the title
|
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. |
|
Should we add a small e2e test so this doesn't regress in the next refactor? |
t-hamano
force-pushed
the
rich-text/fix-paste-format
branch
from
bdc7ecf to
92d5699
Compare
|
I've added an e2e test. This test is expected to fail on trunk, but for some reason it passes when I run the e2e tests. However, if I run this test manually on trunk, it fails as expected. I don't understand why it passes when I run it as an e2e test 🤔 Debugging e2e test on trunkThe test passes, but this is unexpected. Post Content: <!-- wp:paragraph -->
<p><strong>testtest</strong></p>
<!-- /wp:paragraph -->8c88d2c810e674a68de73da91e8cdd64.mp4Manual test on trunkThe test fails, which is expected. Post Content: <!-- wp:paragraph -->
<p><strong>test</strong>test</p>
<!-- /wp:paragraph -->9f6df1e6d2c36ed54d2079368e407606.mp4 |
ellatrix
reviewed
Jul 22, 2024
| await pageUtils.pressKeys( 'primary+a' ); | ||
| await pageUtils.pressKeys( 'primary+b' ); | ||
| await pageUtils.pressKeys( 'primary+c' ); | ||
| await page.keyboard.press( 'ArrowRight' ); |
There was a problem hiding this comment.
I think you'll need to make sure the caret is within the bold format?
There was a problem hiding this comment.
Strangely, the problem does not occur inside formatted text, but only occurs when you move the caret outside the formatted text and paste:
343f6d41d1e628a05a987ade8f294824.mp4
There was a problem hiding this comment.
To trigger the link behaviour, you'd need to select text within the format?
There was a problem hiding this comment.
I'd for example select a letter within the text format (to make sure the test is stable and selection boundaries are within the format), then paste the link on the uncollapsed selection
There was a problem hiding this comment.
I tried copying letters within the formatted text and the e2e test still passed on trunk 😅 Of course, the test is expected to fail on trunk.
8145cb965a0b29fd21526da281f67a31.mp4
There was a problem hiding this comment.
But you're not pasting a link?
There was a problem hiding this comment.
Ah, I see what you mean.
#63434 reports two issues:
- If the text is formatted, highlighting and adding the link via paste doesn't work
- If the copied text contains formatted text at the end, pasting the copied text outside the formatted text removes the formatting
This PR solves both problems, but the E2E test I added is for the second problem. Does this mean I should write a test for the first problem?
There was a problem hiding this comment.
I didn't see the second problem, I only knew about the first problem from the PR description. Sure, if it's easier to write a PR for.
There was a problem hiding this comment.
There may be slight differences with how paste is emulated that make it pass.
There was a problem hiding this comment.
I updated the e2e test to paste the link into the formatted text. But for some reason, this test still passes on trunk as well 😅
t-hamano
force-pushed
the
rich-text/fix-paste-format
branch
3 times, most recently
from
37a8220 to
765cca1
Compare
t-hamano
force-pushed
the
rich-text/fix-paste-format
branch
from
765cca1 to
098b184
Compare
t-hamano
force-pushed
the
rich-text/fix-paste-format
branch
from
098b184 to
33a2a73
Compare
|
The problem this PR is trying to solve seems to have been fixed as a result of #63671, which allows multiple selections on touch devices. Since #63671 is an enhancement, I don't think it can be backported to WP6.6.2 as is. If we want to backport just the paste fix to WP6.6.2, do we need to manually backport just this code to the WP6.6 branch? Pinging @hellofromtonya @vcanales for visibility |
Also noting that #63671 is from 2019. As it's before 6.6 and is an enhancement, I agree with you @t-hamano that it cannot be included in the 6.6 minor cycle. That said, if there's a specific bug introduced during the 6.6 cycle of which a smaller subset of this PR directly fixes the bug, then it could potentially and cautiously be considered. Why caution? Sometimes an enhancement indirectly fixes bugs. Though it may fix bugs, it's still introducing new functionality or feature. |
|
Sorry for the late reply. Just to be sure we're on the same page, I'd like to provide a brief summary.
If you have any questions, please let me know, and I'd be happy to help with any backports needed. Either way, this PR is no longer needed for the trunk branch so I will close it. |
ellatrix
reviewed
Sep 25, 2024
| } ) => { | ||
| await editor.insertBlock( { | ||
| name: 'core/paragraph', | ||
| attributes: { content: 'test' }, |
There was a problem hiding this comment.
Perhaps it could start with bolded text to reduce the complexity of the test?
There was a problem hiding this comment.
Indeed, I simplified it with 9a56955
ellatrix
approved these changes
Sep 25, 2024
|
This PR has a bit of a complicated history, so here's a summary of why a backport to WP 6.7 is needed:
|
t-hamano
added
Backported to WP Core
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 26, 2024
* Paste Handler: Try to fix pasting text with formatting * Refactor e2e test * Simplify e2e test Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: hellofromtonya <hellofromtonya@git.wordpress.org> Co-authored-by: ndiego <ndiego@git.wordpress.org> Co-authored-by: rfischmann <rfischmann@git.wordpress.org> Co-authored-by: bph <bph@git.wordpress.org>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 7d65e11 |
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.
Fixes #63434
What?
This PR fixes an issue where copying and pasting text that contains inline formatting would strip the formatting from the pasted text.
Why?
I'm not sure what the root cause is, but it seems the problem occurs when the copied text contains formatted text at the end.
Also, when pasting multiple times, the formatted text seems to be pasted correctly either the odd or even number of times.
How?
Removed the early return that was causing the problem.
We'll review what tests fail to ensure this doesn't introduce any unintended regressions.
Testing Instructions
Screenshots or screencast
Before
9a2c3325579aa4d1685bb79e17c29cc5.mp4
After
6069e56a8ffe1bd049208c9a9681c728.mp4