Fix Link UI incorrect text replacement in Rich Text #45710
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
|
|
getdave
added
[Type] Bug
|
Size Change: +11.3 kB (+1%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
getdave
commented
Nov 10, 2022
| const [ valBefore, valAfter ] = split( | ||
| value, | ||
| boundary.start, | ||
| boundary.start |
There was a problem hiding this comment.
@ellatrix I don't think passing this should be necessary but it is.
Firstly the split() function only accepts 2 args not 3. The only reason this works is that it uses ...arguments to delegate to the splitAtSelection function which does accept a 3rd parameter.
By default the 3rd param of splitAtSelection defaults to the passed value's end property. In the case of split() we always want to split the string at position X as defined by the 2nd argument. However in some cases the start and end of the value may not match and thus the value is split incorrectly.
I'm having to pass boundary.start as the 3rd ("end") argument to ensure that the value is split in-two at the start of the boundary. Otherwise the second half of the split value often doesj't match what you'd expect.
There was a problem hiding this comment.
Yes, by default all of these functions look at the rich text value's current selection. We allow you to override that, and to do that properly you need to provide both a new selection start and end.
There was a problem hiding this comment.
...but split() doesn't actually have an end argument, it just has string (which can be used to provide a start index):
gutenberg/packages/rich-text/src/split.js
Line 19 in 9e6e8c9
gutenberg/packages/rich-text/src/split.js
Line 15 in 9e6e8c9
The only reason the method I'm using in this PR works is that ...arguments is utilised to pass all of split's arguments to splitAtSelection:
gutenberg/packages/rich-text/src/split.js
Lines 20 to 22 in 9e6e8c9
So it's not obvious from the function signature that you can/should provide an end.
We allow you to override that, and to do that properly you need to provide both a new selection start and end.
Is the intention that you manually update the selection on the value being passed into split before it's passed in?
I still feel it's confusing...
endIndex is set to value.end
gutenberg/packages/rich-text/src/split.js
Lines 57 to 61 in 9e6e8c9
....which may produce unexpected results when (as instructed in the doc block) you've passed an explicit Start index into the split() as the 2nd string argument.
If I pass X as string then I'd expect the value to be split at X. What can actually happen is the value is split at X but the (remaining) second half of the value is not split at X but at whatever value.end is.
gutenberg/packages/rich-text/src/split.js
Lines 71 to 77 in 9e6e8c9
Could / should we instead default endIndex to startIndex so that (by default) if a start index is passed to split as string then it will split where you expect it to?
|
In my testing, this is fixing the issue described in #41771 👍 |
getdave
added
[Package] Format library
getdave
requested review from
scruffian,
MaggieCabrera,
noisysocks,
ajlende and
draganescu
|
@getdave Most important things first, could we add an e2e test please? |
ellatrix
reviewed
Nov 18, 2022
| @@ -148,14 +151,37 @@ function InlineLinkUI( { | |||
| newText.length | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
Commenting on folded code above here. What happens to existing formats? I see that you're creating a new rich text value with the new text, and then applying a link format to it. But by replacing the exiting selection with this new rich text value, any existing formatting will be removed (e.g. bold or italic).
Perhaps this is the wrong way to approach the text replacement? I think we should do the replacement in two steps:
- Replace the link format's URL as we would normally do.
- Replace the active link's text only.
There was a problem hiding this comment.
Yeh clobbering other formats doesn't sound great 😰
@ellatrix This now has an e2e test which emulates the Issue reported and verifies the result is now as expected (i.e. no bug). We should still address your feedback but in a follow up. |
getdave
changed the title
draganescu
approved these changes
Jan 27, 2023
1 task
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?
Fixes a bug whereby editing the text of links with identical text content within a rich text field could result in the wrong link's text being updated.
Closes #41771
Why?
It's important that only the text of the currently active link (active format) is updated.
How?
The code was using the
@wordpress/rich-textreplace()utility to update theRichTextValueobject with the modified text. However,replaceonly operates on the first match it finds. Therefore if the same rich text value contained mulitple links all with the same text then thereplaceoperation would update the text on the first link even if this was not the one being edited.This PR fixes this by splitting the value at the boundary of the active link format. The
replace()is then only run against the second portion of the split value thereby ensuring that the active link format is the first matched byreplace.This PR has to work around what I think may be a bug in
split()whereby the split point for the second portion of the value is determined based on thevalue.end. Asvalues may havestartandendwhich don't match this can result in the string being split incorrectly.Testing Instructions
Screenshots or screencast
Screen.Capture.on.2022-11-10.at.10-13-22.mp4
Screen.Capture.on.2022-11-10.at.10-15-05.mp4