[RNMobile] Try fixing split/merge issue on native mobile by dropping selection update event when late #29683
Conversation
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hi @hypest thanks for these changes. I tested the behavior and it works most of the time but similar to my solution, the bug can still occur even though the solution resolves the issue. Upon observing the content changes of the editor more closely I am realizing that after around 10 splits/merges the bug only occurs if a space is not in front of the word the caret is positioned at before enter is pressed. I am wondering if this has something to do with how the newline marker is being processed in the EnterPressedWatcher I am trying to determine a mechanism for proper diagnosis.
jd-alexander
added
the
Mobile App - i.e. Android or iOS
|
Thanks for giving this a test @jd-alexander !
Hmm, can you try capturing a video perhaps of the situation? Also, just to cover various cases, does that happen to you both on physical devices and on emulators? I've tried to split+merge in the middle of a word (so to not have spaces around the caret) many times in a row and I can't reproduce on my Pixel 2XL Android 10. |
|
Noticed that the solution as is introduces a bug:
Need to work some more on this. |
Understood! I am going to attempt to reproduce and share a video from my device. IIRC, I observed the behavior on my emulator so my reproduction attempt now will give me a solid idea of the bug's occurrence. So the steps I have been using are.
|
This way, when the JS bumps the counter, the native side picks it up and increments from there.
Added b20609c to sync the two sides (JS, native) to avoid dropping all those events that started be marked as late. Generally this can be a risky change, leading to potential race conditions or bugs in places not really foreseen... this will need good testing. |
Ah, thanks for the steps Joel! That step of putting the caret deeper in the second paragraph is one I wasn't doing before. |
Actually @jd-alexander , can you share the emulator details too? Device frame, API level, CPU/ABI, with Play Store or not? Thanks! In the meantime, I tried on a "Pixel 2 XL, API 29, x86, no Play Store" emulator and couldn't reproduce. |
I was using a Pixel 3a, API 30, x86. I am going to attempt to reproduce from my side now. I am syncing up with the latest changes from this PR. |
|
OK, I think I can consistently replicate the issue on the Pixel 3a API 11 emulator (but not at all on the physical device), here are the steps:
Video capture of the steps and issue: screencapture-1615373097537.mp4 |
|
Didn't have the chance to work on debugging/fixing not that there seems to be a consistent way to replicate. I did try to use more |
So to drop the Aztec onChange events too, if late.
If the update was due to content change from the JS side of things, the counter would have already been bumped in shouldComponentUpdate (hopefully), so bumping it again in DidUpdate introduces a counter bump that gets out-of-sync with Aztec since (the bump inside DidUpdate doesn't cause a render and thus no new counter sent to Aztec).
|
Added a couple more commits to fix the issue. The bit that kinda worries me and am not super sure it won't introduce some regression is the removal of the counter manipulation in In the meantime, I'll resort to the testuites and manual testing to verify if some regression is introduced. Edit: the mobile testsuites here and in the gutenberg-mobile PR are green 🎉. |
Thanks! I will check this out in more detail tomorrow!
Exactly! Every time I try to find some history, the mono repo super squash gets in the way, so I have to utilize the debugger or search for previous PRs to create some form of history.
I appreciate the easier reproduction steps mentioned above. 🙏🏾 |
|
I created a test PR that generates an APK to test these changes. |
Concern
Understood! I will investigate this! Investigation
True. I did a bit of git sleuthing to find the introduction of the
TestingDevice: Google Pixel 4 XL
This is a good testing strategy. In addition to this, I used the testing strategy of the I also did the splitting and merging writing flow tests. Splitting and merging
IssuesSplitting/merge list block 🟡When testing, I noticed odd behavior while trying to split the list at times. I was able to reproduce this in other versions of the app like How to reproduce it:
Examplesplitting.mp4Next steps
|
Thanks for digging Joel, good find. Yeah, it's not clear why the set-to-undefined was exactly needed there. I'd say let's assume that it's no longer needed if all our testing works fine, especially since there's no crash introduced here (that original PR of Tug's was fixing a crash).
Nice to see the split/merge manual tests pass, thanks Joel!
Possibly a regression yea, we should look at it separately from this PR. Haven't tried myself but looks like not a blocker for the PR. |
Android Writing Flow checklistDevice: Pixel 2XL General
Rich Text Format
Splitting and merging
Undo / Redo - Test Cases
|
|
Note: won't update the branches until we finish with the manual testing, to avoid waiting again for CI everywhere (the gutenberg-mobile side is fully green on CI atm). We can update the branches when we're ready to start merging. |
|
Noting that I'm seeing strange behaviour when attempting to create more than one new line in the quote block. You'll see from the recording that when I attempt to create a third line, by tapping enter for the second time, the second line is lost and a new block is created: quote.movThere are some existing issues surrounding new lines and the quote block (wordpress-mobile/gutenberg-mobile#2498 and #27690) but this seems significantly different and isn't something I was able to replicate on my main device (running alpha-280). I tested this on a Pixel 2 emulator. Edited to add: I'm seeing some similarly concerning behaviour in the block's citation section, with lines actually being removed when I press enter in my testing: citation.mov |
|
I was able to successfully complete all of the manual tests except for the multiline components checks. If I attempt to create more than two new lines with any of the listed blocks, apart from the code block, then text disappears as explained here for the quote block. I'm going to do some more testing so that I can verify things work as expected on the main branch for me, just in case the issue is with my local environment, and will update accordingly. General
Rich Text Format
Splitting and merging
Undo / Redo - Test Cases
|
|
After some further testing, I found that I was able to replicate the issues I've described on the latest version of the WordPress app, so they're not specific to this branch. The issue with lines sometimes being lost is tricky to reliably replicate, which is what led to the confusion. Sorry for that! I've now created separate issues for these:
I've updated the multiline components test case with these two new known issues. After running through the writing flow test cases again, I confirm that I wasn't able to find any issues specific to this branch. |
👋 @mkevins, thanks for adding info here! I'm a bit confused though, Are you referring to the issue (regression actually) at hand for this PR, or some other issue? If other issue, may I suggest moving to a ticket or a different PR and we can discuss there? There are some interesting points you raise in your comment, ultimately related to GBoard suggestion service that has bitten us in the past too 😢 , and probably deserve it's own space. If your comment is related to this PR, then I'm not sure which split/merge issue you are referring to. Is there some case this fix is missing? Just for context, the underlying issue (previously split text duplicates after merging) is caused by internal data model corruption in the RichText component, itself caused by a selectionChange event that holds obsolete data. I.e. doesn't seem to have something to do with enter-key detection. Let me know if I'm missing something 🤔. |
|
👋 @cameronvoell , added you as a reviewer as well as you've worked with Aztec too. @jd-alexander has already given a deep look but it would be nice if you had a look as well to contribute to the code-level confidence of this PR. Let me know if y'all have any thoughts about the review. Thanks! EDIT: Noticed now that Cameron is away this week so, will remove the review request for the time being. |
Yes, I'm referring to the splitting and merging issue, and that I believe it could be related. Step 4 in the steps to reproduce is:
I noticed that @jd-alexander was also observing strange behavior in the underlying
I agree, the gboard suggestion issues probably deserve their own space. I think it may have been useful here as well since these are inherently related due to how we are handling keypresses and text processing in our Aztec integration, and I wanted this discussion to benefit from what I'd recently discovered, in case it helped resolve the differences observed in some testing scenarios.
From the backscroll, it seems that for some cases (perhaps due to different emulators or physical devices being used) the issue was not fully resolved (though I'm not sure whether this is still the case 🤔 ). I provided the patch with the intention of demonstrating an approach for the gboard problem, which might address the remaining cases, if there are any. |
|
Ah, thanks for elaborating @mkevins , I see, much appreciated. My perception at this point is that we can go ahead with the fix unless new issues are found, and the Gboard suggestions proposal can spawn out to a dedicated PR. 👍 |
...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@hypest I tested this PR again by splitting/merging content in the Paragraph and Heading block and it behaved as expected. I also noticed the WARN and LOG outputs from the event counter logic in the console when the onSelectionChange event is dropped and when lastEventCount bumps doesn't occur.
LGTM 🚢
-
We can keep an eye out for anything suspicious during our next release testing session where we will be publishing the keep block id on split revert we have been doing.
-
Also, could you please add a description of the changes to the PR since this is the solution we are going with. Thanks much :)
-
Add the respective release notes to this PR and the
gb-mobilePR as well.
|
Thanks for the extra review pass @jd-alexander !
Sure thing and good reminder! Updated the PR description with a detailed list of the changes and context about them.
Added the lines! 👍 Will merge all PRs when CI gets green across the board! |
|
All CI is green, including the extended UI tests in the gutenberg-mobile PR and the WPAndroid PR. Will do the merge domino now. |
hypest
deleted the
rnmobile/try-fix-split-issue-drop-selection-update-event
branch
Description
This is another attempt to fix #29478. Opening this set of PRs to have a view of what CI thinks.
There's this prior PR already but since @jd-alexander noticed that the issue still happens sometimes, I gave it a different attempt. We'll collaborate on which one to keep if any.
gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#3239
How has this been tested?
Manual tests of splitting and merging paragraph blocks. Also, we ran the "Writing flow" section of the manual testsuite for native mobile.
Types of changes
lastEventCount(only for Android, the iOS case still usesundefinedto pass the same "message") and the native Aztec wrapper component uses that number to synchronize the counter it keeps internally. Note, the JS side doesn't directly change the native counter; the sync happens in native to ensure that there's no race condition happening by two entities trying to change the same counter variable.Individual changes in the native Aztec wrapper
Individual changes in the RichText component on the JS side
this.lastEventCount = undefinedwiththis.manipulateEventCounterToForceNativeToRefresh()as the way to force the native Android component to update. It was needed to change this for all occurrences otherwise there were issues in various writing flow situations.onChangeFromAztecevent if it's late.handleDeleteevent if late.shouldDropEventFromAztecfunction that checks if the event counter of an event is late compared to the counter kept locally in RichText. Only for Android though. For iOS the function always returns false as the counter based dropping mechanism doesn't exist on iOS, doesn't seem to be needed anyway, yet at least.onSelectionChangeFromAztecevent if late. This is the main fix for the issue at hand actually, since that event is holding a version of the text before the split, and we don't want to use that text.manipulateEventCounterToForceNativeToRefresh()function, that bumps the event counter for Android. Using this function simplifies the callsites trying to trigger the force update of the native side.Checklist: