[RNMobile] RichText: Prevent early return in onSelectionChangeFromAztec when content has leading or trailing spaces #42046
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
This is to prevent returning early and the space being unnecessarily stripped out when the user is typing.
|
Size Change: -24 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
SiobhyB
changed the title
SiobhyB
added
Mobile App - i.e. Android or iOS
SiobhyB
added
the
Mobile App - Automation
|
Thank you for opening this PR with such helpful information! 🙇🏻 I spent time debugging this. I do not have a solution, but do have findings to share. Lifecycle Event DependenciesIt would appear as though changes in #41682 may have disrupted dependencies between Rich Text component lifecycle events. Namely, the early return in Log of lifecycle conflicts
I am uncertain which finding or a combination is the reason for the behavior we are observing. However, the new Not Specific to Trailing SpacesI believe the focus on trailing spaces to be a "red herring," one that originates from the fact that our e2e test data happens to have a trailing space in the first paragraph. I was able to recreate the bug on a simulator with other characters. In the below image, you can see how the last character is "pushed" into the subsequent paragraph behind the cursor position. This results in it remaining as the last character of the last paragraph. If the bug occurs multiple times, the editor arrives with multiple characters "stacked" after the cursor position. Log of lifecycle conflicts
Next StepsUnfortunately, I believe these findings mean that the proposed fix in this PR may not resolve the issue in its entirety. I plan to pause investigating this week to return to a competing priority. I may have more time next week to further debug. Do you think we should consider reverting #41682 for now until we have a better understanding of the dependencies between lifecycle events. Or should we continue forward debugging? |
|
Thank you so much for uncovering those core issues @dcalhoun! From the original PR where
I agree that reverting the original change seems to be the best option for now! Although it was really nice to see the performance improvements the changes brought, a fix for the issue seems complex and like it might require some substantial changes. I've gone ahead to create a revert PR at #42089. Thank you again for all your help and time exploring then explaining the problems with the code. 🙇♀️ |
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?
In #41682, the RichText component was update to return early in
RichText::onSelectionChangedFromAztecwhen content changes are detected. This is to avoid unnecessary calls while a user is quickly typing.Returning early, however, can cause leading or trailing spaces to be stripped out. This PR, therefore, iterates on the changes in #41682 to prevent the early return when a leading or trailing empty space is detected in the user's content.
Why?
We were alerted to some failures via the
gutenberg-editor-block-insertion.test.jstests. An example of a failure can be found here:Through manual testing, it was possible to replicate a trailing space being removed when typing quickly into multiple paragraphs. As such, it's been determined that the issue is related to leading/trailing spaces, but more work needs to be done to identify exactly what's happening in the code.
How?
A check has been added to detect whether content has leading or trailing spaces. If it does, then we no longer return early in
onSelectionChangeFromAztec.Although this works to fix the bug described in this issue, more work needs to be done to figure out exactly why this the spaces are stripped out to begin with. Possibly helpful/relevant issues include:
Testing Instructions
Manual testing
Verify E2E failures are resolved
With this branch checked out, run
npm run native test:e2e:ios:local gutenberg-editor-block-insertion.test.jsfrom the Gutenberg directory in the terminal. Verify that all tests pass.