Skip to content

fix(android): rework longpress movement trigger 🎁#6984

Merged
mcdurdin merged 1 commit intomasterfrom
fix/android/6981-longpress-movement-threshold
Jul 26, 2022
Merged

fix(android): rework longpress movement trigger 🎁#6984
mcdurdin merged 1 commit intomasterfrom
fix/android/6981-longpress-movement-threshold

Conversation

@mcdurdin
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin commented 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 which meant that the longpress delay trigger was never fired.
  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.

User Testing

We need to re-run the main test from #6637, first, to verify that this does not re-introduce any of the behaviours that that PR was intended to fix.

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.

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.
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Jul 24, 2022

User Test Results

Test specification and instructions

  • TEST_POPUPS (PASSED): Tested this in Keyman 16.0.38-alpha-test-6984 build in my Android Mobile deivce (Ver 11) and it is working as expected in all scenarios. ie., the Popup keys don't get stuck on. (notes)
  • TEST_UP_FLICK (PASSED): Tested this in Keyman 16.0.38-alpha-test-6984 build in my Android Mobile device (Ver 11) and verified that the up-flick gesture reliably opens the longpress menu. It is working as expected when I followed the test steps given in this PR.

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Jul 24, 2022
@mcdurdin mcdurdin modified the milestones: A16S6, A16S7 Jul 24, 2022
@bharanidharanj
Copy link
Copy Markdown

  • TEST_POPUPS (PASSED): Tested this in Keyman 16.0.38-alpha-test-6984 build in my Android Mobile deivce (Ver 11) and it is working as expected in all scenarios. ie., the Popup keys don't get stuck on.
    Note: Not able to re-run the main test from fix(android/engine): Fix sticky long-press keys #6637, since the corresponding PR build was missing.

  • TEST_UP_FLICK (PASSED): Tested this in Keyman 16.0.38-alpha-test-6984 build in my Android Mobile device (Ver 11) and verified that the up-flick gesture reliably opens the longpress menu. It is working as expected when I followed the test steps given in this PR.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jul 25, 2022
Copy link
Copy Markdown
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

While I have to admit that I don't directly see exactly how the changes match with the first point listed in the description, the changes overall do seem reasonable. That, and the fact that the user tests passed, is enough for me to LGTM this.

@mcdurdin
Copy link
Copy Markdown
Member Author

While I have to admit that I don't directly see exactly how the changes match with the first point listed in the description

Longpress menus were being triggered on any detected movement on keys on Android.

This relates to the removal of this code (L296-299 of KMKeyboard.java):

      if (action == MotionEvent.ACTION_MOVE && subKeysList != null && subKeysWindow == null) {
        // Display subkeys during move
        showSubKeys(context);
      }

@mcdurdin mcdurdin merged commit eb8fa4c into master Jul 26, 2022
@mcdurdin mcdurdin deleted the fix/android/6981-longpress-movement-threshold branch July 26, 2022 03:57
@gushmazuko
Copy link
Copy Markdown

Many thanks @mcdurdin

@keyman-server
Copy link
Copy Markdown
Collaborator

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(android): rapid typing sometimes leads to missing letters 🎁

5 participants