Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RNMobile] Android Only - Fix Gboard enter detection in EnterPressedWatcher #32471

Merged
merged 5 commits into from
Jun 16, 2021
Merged

[RNMobile] Android Only - Fix Gboard enter detection in EnterPressedWatcher #32471

merged 5 commits into from
Jun 16, 2021

Conversation

AmandaRiu
Copy link
Contributor

@AmandaRiu AmandaRiu commented Jun 4, 2021 •
edited
Loading

Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#3590

Fixes: wordpress-mobile/gutenberg-mobile#3316. This PR is an updated version of this older draft PR by @mkevins and uses a new branch created by @hypest. I tested the fixes on the following physical devices:

Phone OS
Pixel 2 Android 10
Pixel 4 Android 11

Ran through all these writing flow tests first using the default Gboard. I then installed the Microsoft Swift Keyboard and ran through the tests again. Both keyboard's failed TC002 in the Splitting and merging group of test cases. The test that failed was the one where you first split a paragraph block, then add text to the beginning of the second block and then select that new text and delete it. Tapping delete again will not merge the two blocks back into a single block.

I also tested all the known issues with the two keyboards. Only one existing issue was fixed for both keyboards:

However, all known issues were not an issue for the Swift keyboard.

Description

The approach used in this PR adds logic to detect when we've encountered the unusual behavior exhibited by Gboard which leads to a failure to detect onEnter keypresses. When such behavior is detected, it calculates new offsets to be used in string comparison and processing to conform to the original intention of this TextWatcher.

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. Some consideration should be made for those other conditions as well.

How has this been tested

The Writing Flow Tests in a comment below should be completed using a physical device with a and without a GBoard keyboard:

  • Physical device using the GBoard keyboard
  • Physical device using a non-Gboard keyboard

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

mkevins added 5 commits June 4, 2021 18:47
@mkevins @hypest
Add logic to address Gboard's unusual invocation of TextWatcher methods
b8aa541 
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 @hypest
Revert "Add logic to address Gboard's unusual invocation of TextWatch…
0516ff1 
…er methods"

This reverts commit 5581743.
@mkevins @hypest
Search for newline in newTextCopy
9b49e3f
@mkevins @hypest
Revert "Search for newline in newTextCopy"
1638c1c 
This reverts commit 93bbd1f.
@mkevins @hypest
Revert "Revert "Add logic to address Gboard's unusual invocation of T…
f64da2b 
…extWatcher methods""

This reverts commit 5da76c7.
@AmandaRiu
Copy link
Contributor Author

AmandaRiu commented Jun 4, 2021 •
edited
Loading

Writing flow tests for [ENTER] behavior

These 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 Steps

Test the next steps on:

  • Quote block
  • Verse block
  • Preformatted block
  • Code block (DEV only)
  • Pullquote block
✅ TC001

Known Issues

  • On Android, tapping Enter to split a quote or pullquote citation both splits the block and adds a newline to the citation (#2498)
  • On Android, pressing enter multiple times in the content of a Pullquote or Quote block causes unintended behavior (#27690)
  • On Android, there is sometimes a loss of lines when entering multiple lines. (#29861)
  • There is sometimes inconsistent HTML surrounding newlines in the Pullquote and Quote Block. (#1396)

New line on multiline components

  • Start typing and press enter to create a new line.
  • The new line should be created, without splitting the block.
  • On citations, pressing Enter at the end of a citation splits the block, but pressing enter in the middle of a citation creates a new line.
  • Check on HTML mode that the resulting HTML code is correct:
    • Quote: <p> tags per paragraph.
    • Quote citation: <br> ending lines.
    • Verse: <br> ending lines.
    • Preformatted: <br> ending lines.
    • Code: (Invisible \n) new line character.
    • Pullquote: <p> tags per non-wrapping "line"
    • Pullquote citation: <br> ending lines.

✅ Rich Text Formatting - Test Cases

Testing Steps

Precondition

Have a rich-text based component with content (Paragraph, Heading, Quote, Media Caption, etc...)

✅ TC003

Alignment Split

  • Write some lines on a paragraph block.
  • Set an alignment on the block.
  • Move the caret by the half of the text and press enter to split it.
  • Check that the new block has the same alignment.
  • Check that the text alignment corresponds to the setting.

❌ Splitting and merging - Test Cases

Testing Steps

Known Issues

Precondition

Start from an empty post.

Initial steps

  • Repeat the next steps on a Paragraph and Heading block
  • Write (or paste) a paragraph with a few lines
  • Set the caret by the half of the paragraph
  • Press Enter (Intro) to split the block into two.
✅ TC001

Known Issues

Merge after writing

  • Follow the initial steps.
  • Write some more text where the caret is placed (beginning of new block).
  • Delete all new text using the delete button until the blocks are merged again.
  • Check that the blocks were merged.
❌ TC002

Known Issues

Merge after selection

  • Follow the initial steps.
  • Write some more text where the caret is placed (beginning of new block)
  • Select all the newly written text,
  • Press delete to remove all selected text.
  • Press delete once again to merge the blocks. - Unable to merge after selection was deleted
  • Check that the blocks were merged.
✅ TC003

Known Issues

Merge after deleting text

  • Follow the initial steps.
  • Move the caret a few words.
  • Delete all those words until the blocks merge.
  • Check that the blocks were merged.
✅ TC004

Known Issues

Merge after deleting all

  • Follow the initial steps.
  • Select and remove all content of the new block
  • Press delete to remove the empty block.
  • Check that the previous block was selected.
✅ TC005

Merge multiple blocks

  • Follow the initial steps.
  • Press enter two times to create an empty block in between
  • Press back two times to merge everything again.
  • Check that the blocks were merged.
✅ TC006

Splitting/merge list block

  • Write some items on a list block
  • Put the caret by the half of them (at the end of the item) and press Enter to create a new item.
  • Press Enter again to split the block into two, with an empty paragraph block in the middle. The paragraph block at the middle should be focused.
  • Write something on that newly created paragraph block.
  • Move the cursor to the beginning of the paragraph block
  • Press delete to merge the paragraph block with the upper list block.
  • Check that the new word should be a new list item.
  • Move the caret to the beginning of the second list block.
  • Press delete once to merge it back into the first list block.
  • Check that everything was merged and there’s just one list block.

With non-Gboard keyboard

✅ Multiline components - Test Cases

Testing Steps

Test the next steps on:

  • Quote block
  • Verse block
  • Preformatted block
  • Code block (DEV only)
  • Pullquote block
TC001

Known Issues

  • ☑️ On Android, tapping Enter to split a quote or pullquote citation both splits the block and adds a newline to the citation (#2498) - Not an issue using the swift keyboard
  • ☑️ On Android, pressing enter multiple times in the content of a Pullquote or Quote block causes unintended behavior (#27690) - not an issue with the swift keyboard
  • ☑️ On Android, there is sometimes a loss of lines when entering multiple lines. (#29861) - not an issue with the swift keyboard
  • ☑️ There is sometimes inconsistent HTML surrounding newlines in the Pullquote and Quote Block. (#1396) - not an issue with the swift keyboard

New line on multiline components

  • Start typing and press enter to create a new line.
  • The new line should be created, without splitting the block.
  • On citations, pressing Enter at the end of a citation splits the block, but pressing enter in the middle of a citation creates a new line.
  • Check on HTML mode that the resulting HTML code is correct:
    • Quote: <p> tags per paragraph.
    • Quote citation: <br> ending lines.
    • Verse: <br> ending lines.
    • Preformatted: <br> ending lines.
    • Code: (Invisible \n) new line character.
    • Pullquote: <p> tags per non-wrapping "line"
    • Pullquote citation: <br> ending lines.

✅ Rich Text Formatting - Test Cases

Testing Steps

Precondition

Have a rich-text based component with content (Paragraph, Heading, Quote, Media Caption, etc...)

✅ TC003

Alignment Split

  • Write some lines on a paragraph block.
  • Set an alignment on the block.
  • Move the caret by the half of the text and press enter to split it.
  • Check that the new block has the same alignment.
  • Check that the text alignment corresponds to the setting.

❌ Splitting and merging - Test Cases

Testing Steps

Known Issues

Precondition

Start from an empty post.

Initial steps

  • Repeat the next steps on a Paragraph and Heading block
  • Write (or paste) a paragraph with a few lines
  • Set the caret by the half of the paragraph
  • Press Enter (Intro) to split the block into two.
✅ TC001

Known Issues

Merge after writing

  • Follow the initial steps.
  • Write some more text where the caret is placed (beginning of new block).
  • Delete all new text using the delete button until the blocks are merged again.
  • Check that the blocks were merged.
❌ TC002

Known Issues

Merge after selection

  • Follow the initial steps.
  • Write some more text where the caret is placed (beginning of new block)
  • Select all the newly written text,
  • Press delete to remove all selected text.
  • Press delete once again to merge the blocks.
  • Check that the blocks were merged.
✅ TC003

Known Issues

Merge after deleting text

  • Follow the initial steps.
  • Move the caret a few words.
  • Delete all those words until the blocks merge.
  • Check that the blocks were merged.
✅ TC004

Known Issues

Merge after deleting all

  • Follow the initial steps.
  • Select and remove all content of the new block
  • Press delete to remove the empty block.
  • Check that the previous block was selected.
✅ TC005

Merge multiple blocks

  • Follow the initial steps.
  • Press enter two times to create an empty block in between
  • Press back two times to merge everything again.
  • Check that the blocks were merged.
✅ TC006

Splitting/merge list block

  • Write some items on a list block
  • Put the caret by the half of them (at the end of the item) and press Enter to create a new item.
  • Press Enter again to split the block into two, with an empty paragraph block in the middle. The paragraph block at the middle should be focused.
  • Write something on that newly created paragraph block.
  • Move the cursor to the beginning of the paragraph block
  • Press delete to merge the paragraph block with the upper list block.
  • Check that the new word should be a new list item.
  • Move the caret to the beginning of the second list block.
  • Press delete once to merge it back into the first list block.
  • Check that everything was merged and there’s just one list block.

@AmandaRiu AmandaRiu requested review from mkevins and hypest June 5, 2021 00:50
@AmandaRiu AmandaRiu added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Jun 5, 2021
@AmandaRiu AmandaRiu marked this pull request as ready for review June 5, 2021 00:58
@AmandaRiu
Copy link
Contributor Author

This is ready for review, but hold off on merging. I'm waiting to hear if we are okay with fixing this bug at the cost of test TC002 in the Splitting and merging suite of tests failing. (cc: @mchowning @hypest )

@AmandaRiu AmandaRiu mentioned this pull request Jun 5, 2021
Merged
2 tasks
@mkevins
Copy link
Contributor

mkevins commented Jun 7, 2021

Hi @AmandaRiu 👋 😄

I noticed that these changes appear to come from the first approach attempted on the original draft PR, but I may have left the description of that PR in the state from the second attempted approach there, even though those changes had been reverted. I think I should have changed the description back when I pushed the reverts, which would be:

Description

The approach used in this PR adds logic to detect when we've encountered the unusual behavior exhibited by Gboard which leads to a failure to detect onEnter keypresses. When such behavior is detected, it calculates new offsets to be used in string comparison and processing to conform to the original intention of this TextWatcher.

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. Some consideration should be made for those other conditions as well.

In my tests there, this more "targeted" approach had not introduced any new regressions, and resolved the single issue of pressing enter at the end of a suggested ("underlined") word.

I also observed at that time that a few of the known issues had been resolved on trunk, so it is quite possible that the writing flow tests may exhibit different behavior now since the changeset is now rebased. I just wanted to clarify, since while in it's draft state that older PR had two different (though similar) approaches attempted, and it looks like the changeset here corresponds with the original approach used there.

@hypest
Copy link
Contributor

hypest commented Jun 7, 2021

The test that failed was the one where you first split a paragraph block, then add text to the beginning of the second block and then select that new text and delete it. Tapping delete again will not merge the two blocks back into a single block.

👋 @AmandaRiu , thanks for the thorough testing! Let me ask, will the user eventually be able to merge the blocks? Like, doing another backspace or maybe moving the caret somewhere and then the block would allow merging? If yes, then it feel that's a lesser issue than the newline-on-enter and I think it'd be useful to go ahead and land the fix for the newline issue anyway. We can then work on the merge issue separately.

@AmandaRiu
Copy link
Contributor Author

Hi @AmandaRiu 👋 😄

I noticed that these changes appear to come from the first approach attempted on the original draft PR, but I may have left the description of that PR in the state from the second attempted approach there, even though those changes had been reverted. I think I should have changed the description back when I pushed the reverts, which would be:

Description

The approach used in this PR adds logic to detect when we've encountered the unusual behavior exhibited by Gboard which leads to a failure to detect onEnter keypresses. When such behavior is detected, it calculates new offsets to be used in string comparison and processing to conform to the original intention of this TextWatcher.
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. Some consideration should be made for those other conditions as well.

In my tests there, this more "targeted" approach had not introduced any new regressions, and resolved the single issue of pressing enter at the end of a suggested ("underlined") word.

I also observed at that time that a few of the known issues had been resolved on trunk, so it is quite possible that the writing flow tests may exhibit different behavior now since the changeset is now rebased. I just wanted to clarify, since while in it's draft state that older PR had two different (though similar) approaches attempted, and it looks like the changeset here corresponds with the original approach used there.

Thanks @mkevins, I've updated the description in the PR. 👍

@AmandaRiu
Copy link
Contributor Author

The test that failed was the one where you first split a paragraph block, then add text to the beginning of the second block and then select that new text and delete it. Tapping delete again will not merge the two blocks back into a single block.

👋 @AmandaRiu , thanks for the thorough testing! Let me ask, will the user eventually be able to merge the blocks? Like, doing another backspace or maybe moving the caret somewhere and then the block would allow merging? If yes, then it feel that's a lesser issue than the newline-on-enter and I think it'd be useful to go ahead and land the fix for the newline issue anyway. We can then work on the merge issue separately.

that's a good question @hypest . I'll test it out and post the results here.

@AmandaRiu
Copy link
Contributor Author

@hypest

Let me ask, will the user eventually be able to merge the blocks? Like, doing another backspace or maybe moving the caret somewhere and then the block would allow merging?

Hitting backspace again doesn't fix it, but if I click off the block and then click back into it, I'm able to backspace and merge the blocks back together.

@mkevins
Copy link
Contributor

mkevins commented Jun 8, 2021 •
edited
Loading

@AmandaRiu 👋 😄 thank you for testing these flows so thoroughly!

Regarding TC002, I was wondering if you are observing that as a new regression (i.e. that it was introduced by these changes). When I tested this, it seemed to be an issue on trunk as well, and just now tested the same flow on 17.5-rc-1 and observed the same bug with merging being prevented:

tc002-split-and-merge-failing-17.5-rc-1.mp4

Are you able to reproduce this on a current release as well?

@AmandaRiu
Copy link
Contributor Author

@mkevins

Regarding TC002, I was wondering if you are observing that as a new regression (i.e. that it was introduced by these changes). When I tested this, it seemed to be an issue on trunk as well, and just now tested the same flow on 17.5-rc-1 and observed the same bug with merging being prevented:

The video you provided in you comment shows you were testing with "swiping" words, which is a known issue logged in this ticket wordpress-mobile/gutenberg-mobile#2375. The issue I had uncovered was just tapping out the word one letter at a time which wasn't logged as a known issue which is why I flagged it as a regression. I just tried the same scenario on the current production version of WPAndroid, and it IS happening there so this isn't a regression. I didn't think to test that before. With that in mind I think this PR is good to go 👍

Unfortunately, since I'm the PR author, I'm unable to approve this PR so I will need someone else to do it so it may be merged.

@AmandaRiu
Copy link
Contributor Author

I'll update the current ticket (wordpress-mobile/gutenberg-mobile#2375) with notes that it happens with swiping and tapping out words.

@mkevins
Copy link
Contributor

mkevins commented Jun 16, 2021

The issue I had uncovered was just tapping out the word one letter at a time

Ah, good distinction! And thanks for confirming the behavior also exists for tapping!

mkevins
mkevins approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving via @AmandaRiu 's testing and review 😄

@AmandaRiu AmandaRiu merged commit 8d986c9 into trunk Jun 16, 2021
@AmandaRiu AmandaRiu deleted the rnmobile/try/fix-gboard-enter-detection-hypest branch June 16, 2021 14:53
@github-actions github-actions bot added this to the Gutenberg 10.9 milestone Jun 16, 2021
mokagio added a commit to wordpress-mobile/WordPress-iOS that referenced this pull request Jun 29, 2021
@mokagio
Remove release notes entry for Gutenberg change affecting Android only
92c549c 
See title in the PR that was linked to it
(wordpress-mobile/gutenberg-mobile#3590):

> [Android] Fix Gboard enter detection in EnterPressedWatcher

If you then follow up on the Gutenberg PR
(WordPress/gutenberg#32471)
that actually made the change, you'll see confirmation it's Android
only:

> [RNMobile] Android Only - Fix Gboard enter detection in
> EnterPressedWatcher
@mkevins mkevins mentioned this pull request Jul 5, 2021
Closed
7 tasks
@jd-alexander jd-alexander mentioned this pull request Nov 5, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkevins mkevins mkevins approved these changes

@hypest hypest Awaiting requested review from hypest

Assignees
No one assigned
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Milestone
Gutenberg 10.9
Development

Successfully merging this pull request may close these issues.

Enter detection fails in EnterPressedWatcher for Gboard keyboard
3 participants
@AmandaRiu @mkevins @hypest