[Android] Fix Gboard enter detection in EnterPressedWatcher #30368
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
Note: This is probably not enough, since this only addresses when the caret is at the end of the word, but Gboard may exhibit the unusual behavior also when the caret is in the middle or start of the word.
mkevins
added
[Status] In Progress
|
Size Change: +5.26 kB (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
Thanks for the draft fix @mkevins ! I got curious about the fix and looked at the code. We've indeed have noticed in the past that GBoard's autosuggestion system performs these "weird" text chance events where a single char is is actually added after the whole word is first removed from the text buffer. I'm not yet sure about trying to fix the issue by essentially mimicking the logic GBoard uses. I feel it's a bit complex and prone to breakages if GBoard updates it's logic. I took a stab at it and tried a different idea: to just search for the Enter character instead of using the change event's positioning variables. Check out the gist here for a diff against the production code, where I basically rely on Anyway, just a proposal, let me know what you think! Thanks! |
…er methods" This reverts commit 5581743.
|
Thanks for taking a look at this one Stefanos! 😄
To clarify, the earlier approach wasn't really "mimicking" gboard, but rather trying to "detect" it. I agree, though, that in some sense, it relies on specific behavior of Gboard that may change at some point in the future. Also, in some sense, that's how we got here 😆 . I think to address the other cases that my approach hadn't yet handled, we'd inevitable need to do a search for the newline, so your solution is actually more general, and covers these cases, as well as the usual non-Gboard keyboards, at least in my understanding. I tested your patch, and included it in this PR (reverting the "segmented" approach). I tested this with several scenarios, and it seems to solve the issue and I did not encounter any regressions either. I tried a few different things, such as having text selected when pressing enter (which does not work, but is not a regression either - think of that one as an easter egg in inserting a newline in lieu of shift-enter 😃 ). Regarding paste, the good news is that we actually bypass paste when we have a paste handler on the Gutenberg side, so that is all handled in JS. Was there a particular scenario you had concerns with in paste where perhaps we don't have a gb handler? |
|
Oh, good to see you like and incorporated the gist Matthew! And thanks for putting it through more testing. I can't hide my feeling that it could introduce other bugs so, your testing helps alleviate the fear :)
🎉 awesome!
Not really. Just looking at the code and how it only searches for >1< Enter char, just makes me wonder what would happen if the text had more, and that's when the "what happens when pasting multiple paragraphs" came to mind, nothing more specific. It's good that the paste is handled elsewhere anyway so, we can leave it at that for the time being 👍 |
Writing flow tests for [ENTER] behaviorThese tests should be performed both with and without a Gboard keyboard. It may be worth paying close attention to the known issues with enter key behavior to make sure there aren't any new regressions, and if possible, whether some other issues are fixed by these changes. With Gboard keyboard❌ Multiline components - Test Cases Testing StepsTest the next steps on:
TC001Known Issues
New line on multiline components
Testing StepsPreconditionHave a rich-text based component with content (Paragraph, Heading, Quote, Media Caption, etc...) TC003Alignment Split
❌ Splitting and merging - Test Cases Testing StepsKnown Issues
PreconditionStart from an empty post. Initial steps
TC001Known Issues
Merge after writing
❌ TC002Known Issues
Merge after selection
TC003Known Issues
Merge after deleting text
TC004Known Issues
Merge after deleting all
TC005Merge multiple blocks
❌ TC006Splitting/merge list block - cannot add new items to list, can only split into new lists with one item on entering
With non-Gboard keyboard
Testing StepsTest the next steps on:
TC001Known Issues
New line on multiline components
Testing StepsPreconditionHave a rich-text based component with content (Paragraph, Heading, Quote, Media Caption, etc...) TC003Alignment Split
Testing StepsKnown Issues
PreconditionStart from an empty post. Initial steps
TC001Known Issues
Merge after writing
TC002Known Issues
Merge after selection
TC003Known Issues
Merge after deleting text
TC004Known Issues
Merge after deleting all
TC005Merge multiple blocks
TC006Splitting/merge list block
|
|
Unfortunately, it seems that the current change (2ea1654) set may introduce other regressions in writing flow tests, although it does seem to fix some known issues as well. Fixes:
Regressions:
More details in the testing steps listed here. |
|
Thanks for running more detailed tests @mkevins ! Will have a look at the regressions from my side too 👍 Sorry, read your comment above too fast Matthew. Are you trying to check the solution in this PR, or perhaps do more testing on the merged PR with the dropping selection event solution? 🤔 |
Awesome, thanks Stefanos!
I'm checking the current changeset (2ea1654) in this PR by running through the writing flow tests that involved [ENTER] keypresses. My intention is to check for any possible regressions we may face by altering the enter-detection mechanism. So far, I've encountered a few, mostly to do with lists and (pull)quote blocks. |
Writing flow tests for [ENTER] behaviorSince the approach searching for newlines seems to introduce some regressions, I also tried the tests w/ the earlier approach, since it was written with the intent to minimize side effects by attempting to detect and isolate Gboard specific behavior. I performed these tests both with and without a Gboard keyboard with this changeset: 7363bb5. It seems that most of the behavior is unchanged (i.e. I did not encounter any new regressions), and the specific case of pressing With Gboard keyboard
Testing StepsTest the next steps on:
TC001Known Issues
New line on multiline components
Testing StepsPreconditionHave a rich-text based component with content (Paragraph, Heading, Quote, Media Caption, etc...) TC003Alignment Split
Testing StepsKnown Issues
PreconditionStart from an empty post. Initial steps
TC001Known Issues
Merge after writing
TC002Known Issues
Merge after selection
TC003Known Issues
Merge after deleting text
TC004Known Issues
Merge after deleting all
TC005Merge multiple blocks
TC006Splitting/merge list block
With non-Gboard keyboard
Testing StepsTest the next steps on:
TC001Known Issues
Testing StepsPreconditionHave a rich-text based component with content (Paragraph, Heading, Quote, Media Caption, etc...) TC003Alignment Split
Testing StepsKnown Issues
PreconditionStart from an empty post. Initial steps
TC001Known Issues
Merge after writing
TC002Known Issues
Merge after selection
TC003Known Issues
Merge after deleting text
TC004Known Issues
Merge after deleting all
TC005Merge multiple blocks
TC006Splitting/merge list block
|
9 tasks
|
Closing as these changes were brought in with: #32471 |
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: wordpress-mobile/gutenberg-mobile#3316
Description
This PR addresses the Gboard issue by searching the
CharSequencefor a newline, instead of only checking the first character of the text inonTextChanged. If it finds a newline, its offset is then used for further processing. If the newline is the first character, the offset will be0, so previous (non-gboard) behavior will be the same as before. This is a more general solution.How has this been tested?
This should be tested both with and without gboard keyboard via this suite of writing flow tests.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).