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 sticky long-press keys #6637

Merged
merged 3 commits into from May 23, 2022

Conversation

darcywong00
Copy link
Contributor

@darcywong00 darcywong00 commented May 20, 2022

Fixes #6636

The logic to clear popup keys depends on GestureDetector triggering MotionEvent.ACTION_UP. This PR pulls the handling out of the if/else because sometimes, the ACTION_UP event goes through the if() block.

if (subKeysWindow != null && suggestionMenuWindow == null) {
// Passes KMKeyboard (subclass of WebView)'s touch events off to our subkey window
// if active, allowing for smooth, integrated gesture control.
subKeysWindow.getContentView().findViewById(R.id.grid).dispatchTouchEvent(event);
} else {

User Testing

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

  • TEST_BETA_STICKY_KEYS - Verify beta build can have long-press keys stuck
  1. On the Android device, install 15.0.249 beta of Keyman for Android
    https://downloads.keyman.com/android/beta/15.0.249/keyman-15.0.249.apk
  2. Dismiss the "Get Started" menu
  3. In the Keyman app, quickly type with the default sil_euro_latin keyboard on the OSK.
  4. After typing a long paragraph, observe if a long-press keys become stuck (displayed while not holding down on a key)
  5. Verify at some point, the long-press key is stuck. If you see this bug, this test is PASS
  6. Uninstall the beta build of Keyman for Android.
  • 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
    • 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)

@darcywong00 darcywong00 requested a review from rc-swag as a code owner May 20, 2022 06:02
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels May 20, 2022
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented May 20, 2022

User Test Results

Test specification and instructions

  • TEST_BETA_STICKY_KEYS (PASSED): Tested this as per the instructions and I was able to reproduce this bug in the Android Mobile device (as it mentioned in the User Testing steps).
  • TEST_POPUPS (PASSED): Tested this in my Android Mobile (Version 11) with Keyman 15.0.250-beta build, as per the instructions and it seems to be working fine in all conditions.

Test Artifacts

@bharanidharanj
Copy link

  • TEST_BETA_STICKY_KEYS (PASSED): Tested this in my Android Mobile device (Version 11) with the Keyman 15.0.249 beta build as per the instructions and it seems that the long press keys did not get stuck after typing a long paragraph. Seems to be working fine.

@bharanidharanj
Copy link

  • TEST_POPUPS (PASSED): Tested this in my Android Mobile (Version 11) with Keyman 15.0.250-beta build, as per the instructions and it seems to be working fine in all conditions.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label May 20, 2022
@darcywong00
Copy link
Contributor Author

Tested this in my Android Mobile device (Version 11) with the Keyman 15.0.249 beta build as per the instructions and it seems that the long press keys did not get stuck after typing a long paragraph. Seems to be working fine.

TEST_BETA_STICKY_KEYS should have shown a long-press key getting stuck. Like quickly typing y multiple times and release sometimes results in the long-press keys being displayed.

@keymanapp-test-bot retest TEST_BETA_STICKY_KEYS

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label May 21, 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.

LGTM. Nice clean update.

@bharanidharanj
Copy link

  • TEST_BETA_STICKY_KEYS (FAILED): Oops! Yes, Darcy! Retested this with Keyman 15.0.249-beta build in my Android Mobile device and this time I was able to see the clicking on any long-press key getting stuck and it displayed for a while.

Long Press Key Problem:

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label May 23, 2022
@darcywong00
Copy link
Contributor Author

@bharanidharanj - TEST_BETA_STICKY_KEYS is expecting the long-press key to be stuck (a bug), but the test itself should PASS if you see the bug. I'll edit the step to expect that.

@keymanapp-test-bot retest TEST_BETA_STICKY_KEYS

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label May 23, 2022
@bharanidharanj
Copy link

@bharanidharanj - TEST_BETA_STICKY_KEYS is expecting the long-press key to be stuck (a bug), but the test itself should PASS if you see the bug. I'll edit the step to expect that.

@keymanapp-test-bot retest TEST_BETA_STICKY_KEYS

@darcywong00 Okay, Thanks for the Clarification.

@bharanidharanj
Copy link

  • TEST_BETA_STICKY_KEYS (PASSED): Tested this as per the instructions and I was able to reproduce this bug in the Android Mobile device (as it mentioned in the User Testing steps).

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label May 23, 2022
@darcywong00 darcywong00 merged commit 124a967 into beta May 23, 2022
@darcywong00 darcywong00 deleted the fix/android/sticky-longpress branch May 23, 2022 13:57
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 15.0.251-beta

mcdurdin added a commit that referenced this pull request Jul 24, 2022
Fixes #6981.

Longpress menus were being triggered on any detected movement on keys on
Android. This would cause rapid typing to periodically fail as the user
would make a single-pixel movement while typing, which would then block
subsequent key events.

This issue was introduced in #6637, which resolved a somewhat related
problem with sticky longpress menus.

The fix is in three parts:

1. Remove the touch movement detection which caused the primary problem
2. Address some logic issues around visibility of `subkeysWindow`
3. Introduce a new 'scroll' gesture handler which detects a negative-y
   movement above a 5px threshold, to open the longpress menu.

The 5px threshold is the minimum default used by Keyman Engine for Web.
However, it may not be ideal, and we should consider moving to using the
0.25 x row height value that Keyman Engine for Web uses in a future
update. However, I do not consider this to be a reason to block this
fix, as it would require significant extra engineering.
mcdurdin added a commit that referenced this pull request Jul 26, 2022
Fixes #6981.

Longpress menus were being triggered on any detected movement on keys on
Android. This would cause rapid typing to periodically fail as the user
would make a single-pixel movement while typing, which would then block
subsequent key events.

This issue was introduced in #6637, which resolved a somewhat related
problem with sticky longpress menus.

The fix is in three parts:

1. Remove the touch movement detection which caused the primary problem
2. Address some logic issues around visibility of `subkeysWindow`
3. Introduce a new 'scroll' gesture handler which detects a negative-y
   movement above a 5px threshold, to open the longpress menu.

The 5px threshold is the minimum default used by Keyman Engine for Web.
However, it may not be ideal, and we should consider moving to using the
0.25 x row height value that Keyman Engine for Web uses in a future
update. However, I do not consider this to be a reason to block this
fix, as it would require significant extra engineering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants