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.

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

fix(android/engine): Fix Backspace key to delete without errant subkeys #7156

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

darcywong00
Copy link
Contributor

@darcywong00 darcywong00 commented Aug 30, 2022

Fixes #7069, fixes #7172, and fixes #7155 and follow-on to #6984

When the subkeys window is dismissed, the subKeysList isn't cleared, so holding backspace key would end up displaying the previous longpress menu.

User Testing

We need to re-run the main test from #6984, first, to verify that this does not re-introduce new bugs.

Setup - A physical Android device. Don't use emulator because the scenario involves typing fast on the OSK (e.g. two thumbs)

  • TEST_POPUPS - Verify popup keys don't get stuck on
  1. On the Android device, install the PR build of Keyman for Android
  2. Dismiss the "Get Started" menu
  3. In the Keyman app, start typing with the default sil_euro_latin.
  4. Type on the OSK using the following scenarios and verify expected output:
    • Clicking a suggestion on the suggestion banner - should insert the suggestion
    • short-press a key and release - should insert the base key
    • long-press a key, select a long-press key, and release - should insert the long-press key (note: there should be a 0.5 second delay before the menu appears)
    • long-press a key, while keeping the finger down, move off the long-press options, and release - should not output
    • long-press a key, while keeping the finger down, move off the long-press options, then move back on a long-press option so it's highlighted, and release - should output the long-press key
    • quickly type a long paragraph (e.g. repeat the word "reply") - verify long-press keys don't get stuck (displayed when not touching a key)
  • TEST_UP_FLICK - Verify that the up-flick gesture reliably opens the longpress menu
  1. On the Android device, install the PR build of Keyman for Android
  2. Dismiss the "Get Started" menu
  3. In the Keyman app, start typing with the default sil_euro_latin.
  4. Touch a key such as e and immediately swipe upwards to access the long-press menu, keeping your finger down. This should show long-press options without the 0.5 second delay.
    • You should be able to select an option from the long-press menu.
    • You should be able to move back onto the base key to select the base key letter again.
    • You should also be able to move your finger away from the menu to cancel the input.
  5. Please run this test a number of times to verify that the behaviour remains consistent.
  • TEST_HELD_BACKSPACE - Verify spurious longpress menu doesn't appear while holding backspace key
  1. On the Android device, install the PR build of Keyman for Android
  2. Dismiss the "Get Started" menu
  3. In the Keyman app, start typing with the default sil_euro_latin.
  4. Accept a suggestion
  5. Type another letter
  6. Touch and hold the backspace key
  7. Verify the previous letter is deleted
  8. Verify longpress menus for that letter do not appear
    Note, if the backspace key fails to delete, that's a separate issue bug(android): Pressing Backspace key for a while make the cursor stuck on the text pane  #7069 not fixed on this PR
  • TEST_HELD_GLOBE - Verify spurious longpress menu doesn't appear while holding globe key
  1. On the Android device, install the PR build of Keyman for Android
  2. Dismiss the "Get Started" menu
  3. In the Keyman app, start typing with the default sil_euro_latin.
  4. Touch and hold the globe key
  5. Verify longpress menus for other letters do not appear
  6. Release the globe key and verify the Keyboard menu appears (default globe key action when only 1 Keyman keyboard is installed)

01 Sept -
Adding additional tests to verify if this PR fixes other corresponding issues

  1. On the Android device, install the PR build of Keyman for Android
  2. In the Keyman app start typing a sentence
  3. While holding the backspace key
  1. On the Android device, install the PR build of Keyman for Android
  2. In the Keyman app, start typing the sentence "Here is some text"
  3. Move the cursor to "some" and then back to the end of "text"
  4. While holding the backspace key
  • Verify the context is deleted one character at a time until the entire context is deleted

@darcywong00 darcywong00 added this to the A16S9 milestone Aug 30, 2022
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Aug 30, 2022
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Aug 30, 2022

User Test Results

Test specification and instructions

  • TEST_POPUPS (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 build in my Android Mobile device Version 11.0 and verified that the POPUP keys does not stuck up while testing it.
  • TEST_UP_FLICK (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 build in my Android Mobile device Version 11.0 and verified that the UP FLICK gestures reliably opens the long press menu.
  • TEST_HELD_BACKSPACE (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 build in my Android Mobile device Version 11.0 and verified that the spurious long press menu does not appear while holding the backspace key. seems to be working fine.
  • TEST_HELD_GLOBE (PASSED): Tested this with the attached PR build (16.0.54-alpha-test-7156) in my Android Mobile device Version 11.0 and verified longpress menus for other letters do not appear and releasing the globe key, the keyboard menu appears on the screen. Seems to be working as expected.
  • TEST_HELD_BACKSPACE_DELETES (PASSED): Tested this with the attached PR build (16.0.54-alpha-test-7156) in my Android Mobile device Version 11.0 and verified long press the Backspace key does not make the cursor stuck on the text pane. Instead of that it removes all the characters while long pressing the Backsapce key. Seems to be fixed and working fine with the recent build.
  • TEST_NO_BACKSPACE_DESYNC (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 PR build and followed the steps as it mentioned above User Testing. Verified that the context is deleted one character at a time until the entire context is deleted while holding the backsapce key.

Test Artifacts

@bharanidharanj
Copy link

  • TEST_POPUPS (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 build in my Android Mobile device Version 11.0 and verified that the POPUP keys does not stuck up while testing it.
  • TEST_UP_FLICK (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 build in my Android Mobile device Version 11.0 and verified that the UP FLICK gestures reliably opens the long press menu.
  • TEST_HELD_BACKSPACE (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 build in my Android Mobile device Version 11.0 and verified that the spurious long press menu does not appear while holding the backspace key. seems to be working fine.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Aug 30, 2022
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

Minor cleanup, one related question, but LGTM. Change makes sense. Did you audit the rest of the code for similar patterns?

@darcywong00
Copy link
Contributor Author

Did you audit the rest of the code for similar patterns?

Of the four PopupWindows, only subKeysWindow involves an ArrayList of data that needs to be cleared on dismiss.

  • subKeysWindow
  • keyPreviewWindow
  • helpBubbleWindow
  • suggestionMenuWindow

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Aug 31, 2022
@darcywong00
Copy link
Contributor Author

@keymanapp-test-bot retest TEST_GLOBE

@bharanidharanj
Copy link

  • TEST_HELD_GLOBE (PASSED): Tested this with the attached PR build (16.0.54-alpha-test-7156) in my Android Mobile device Version 11.0 and verified longpress menus for other letters do not appear and releasing the globe key, the keyboard menu appears on the screen. Seems to be working as expected.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Aug 31, 2022
@mcdurdin
Copy link
Member

mcdurdin commented Sep 1, 2022

In my tests of this update, this also appears to fix #7172.

@mcdurdin
Copy link
Member

mcdurdin commented Sep 1, 2022

Does this also fix #7169? I can no longer repro that bug on this build.

@mcdurdin
Copy link
Member

mcdurdin commented Sep 1, 2022

DONT-MERGE: need to add tests for other presumed-fixed issues here before merge.

OK to merge now.

@darcywong00
Copy link
Contributor Author

@keymanapp-test-bot retest TEST_HELD_BACKSPACE_DELETES

@bharanidharanj
Copy link

  • TEST_HELD_BACKSPACE_DELETES (PASSED): Tested this with the attached PR build (16.0.54-alpha-test-7156) in my Android Mobile device Version 11.0 and verified long press the Backspace key does not make the cursor stuck on the text pane. Instead of that it removes all the characters while long pressing the Backsapce key. Seems to be fixed and working fine with the recent build.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Sep 1, 2022
@mcdurdin mcdurdin modified the milestones: A16S9, A16S10 Sep 5, 2022
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Sep 5, 2022
@darcywong00
Copy link
Contributor Author

@keyamnapp-test-bot retest TEST_NO_BACKSPACE_DESYNC

@bharanidharanj
Copy link

  • TEST_NO_BACKSPACE_DESYNC (PASSED): Tested this with Keyman 16.0.54-alpha-test-7156 PR build and followed the steps as it mentioned above User Testing. Verified that the context is deleted one character at a time until the entire context is deleted while holding the backsapce key.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Sep 5, 2022
@darcywong00 darcywong00 changed the title fix(android/engine): Cleanup list of subkeys when dismissing window fix(android/engine): Fix Backspace key to delete without errant subkeys Sep 6, 2022
@mcdurdin
Copy link
Member

mcdurdin commented Sep 6, 2022

Does this also fix #7169? I can no longer repro that bug on this build.

Note: #7169 was not fixed by this PR; see that issue for more details of resolution.

@darcywong00 darcywong00 merged commit bad8806 into master Sep 6, 2022
@darcywong00 darcywong00 deleted the fix/android/longpress-backspace branch September 6, 2022 23:26
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 16.0.57-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment