[RichText]: Fix wrong block merging when pressing delete consecutively
#38991
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
ntsekouras
added
[Type] Bug
|
Size Change: +30 B (0%) Total Size: 1.15 MB
鈩癸笍 View Unchanged
|
ntsekouras
requested review from
ellatrix,
cameronvoell,
ntwb,
nerrad and
ajitbohra
as code owners
|
Thanks @ntsekouras! It looks like this patch fixes the issue. Would it be possible to explain a bit further on what's happening here and why we need to Beforebefore.mp4Afterafter.mp4 |
|
PR also fixes #38991. Let's add a test case for this scenario as well. |
@gwwar I updated the PR's description but still waiting for Ella's input, as she has done a ton of stuff in RichText 馃槃 |
8 tasks
This was referenced Feb 23, 2022
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: #37534
Fixes #38991
Currently if you have some
mergeableblocks likeparagraphsand you pressdeleteat the end of one of them multiple times it doesn't work as expected.What is the problem and why this suggested patch
I'm not 100% sure if there is a better fix for this and I've pinged @ellatrix to take a look and share feedback, but from what I understood the reason for this is:
The delete handling happens here which takes its value from
useRichTexthook. After debugging I saw that the value wasn't updated ondeletepress and would stop afterthreeimproper handlings of thedelete, because in the third press would take another stale previous value, but the selection(start,end) would have changed so it wouldn't qualify to merge blocks.At first I thought we didn't check the new/previous value inside
useRichTexthook, but we actually do here. That hook on change updates internalrefs(which do not cause a rerender) and after updating therefsweforceRenderto make consumers take the updated value. So on merge blocks theonChangeis not called butapplyFromPropsis, which updates the usedrefwithout making the hook rerender.Hope that makes sense and I'm not way off with this solution 馃槃
Reproduction steps
There are two scenarios that are affected by the same problem.
Scenario 1:
firstparagraph pressdeletemultiple timesdeletewill merge the blocks and all the others are doing nothingScenario 2:
firstparagraph pressdeletemultiple timesdeletewill merge the two blocks and the second one will merge the third oneTesting instructions
With the reproduction steps everything should work as expected.