Skip to content

fix(android/engine): Display longpress keys during a Move gesture#6138

Merged
darcywong00 merged 5 commits intobetafrom
fix/android/subkey-move
Mar 23, 2022
Merged

fix(android/engine): Display longpress keys during a Move gesture#6138
darcywong00 merged 5 commits intobetafrom
fix/android/subkey-move

Conversation

@darcywong00
Copy link
Copy Markdown
Contributor

@darcywong00 darcywong00 commented Jan 21, 2022 •
edited
Loading

Addresses #5301

This PR is initially in Draft for @MakaraSok to characterize the behavior of 14.0 compared to this one.
Currently, if a user starts a longpress gesture but moves off the initial base key, the longpress (subkeys) never show.
This is because the Keyman Engine for Android's use of GestureDetector isn't handling ACTION_MOVE when it interrupts ACTION_LONGPRESS.

This PR handles ACTION_MOVE during a longpress gesture to display subkeys (if available).

In talking with @mcdurdin , he said once longpress keys are displayed, it's ok to keep displaying the initial keys while the longpress is held. (We currently don't need to refresh to other longpress keys as the user drags to other keys)

KeymanWeb is also changed so the base key isn't updated if there's subkeys are being displayed.
Previously, moving during the longpress would cause to longpress keys to update for the the final base key.
With this change, the longpress keys are locked into the original base key.

User Testing

This needs to be tested on more than one device type

Device Types

  • GROUP_WEB: - Tests KeymanWeb desktop in mobile view
  1. Load the PR build of KeymanWeb Test Home --> Manual Test pages --> Unminified KeymanWeb
  2. Set the browser page to mobile viewport and hard refresh the page
  3. Click the globe button on the keyboard and switch to Swedish (uses sil_euro_latin)
  • GROUP_ANDROID: - Tests Android app
  • GROUP_IOS: - Tests iOS app

Tests

  • TEST_TOUCH_KEYS - Tests the behavior of popup and longpress keys
  1. Load the corresponding Keyman PR build of the group being tested
  2. Enable Keyman as a system keyboard
  3. Launch a separate app (e.g. Chrome or Safari) and select a field to start typing
  4. With the default sil_euro_latin keyboard, select the symbolic layer
  5. Short press a normal key (and release - verify ( is output
  6. Longpress a normal key (release - verify ( is output
  7. Longpress a key that contains longpress keys, and when the longpress keys display, move to an available longpress key and release - verify the selected longpress key is output
  8. Longpress and move - Case 1:
    a. Longpress a key, but immediately move off the base key before the longpress registers.
    b. While holding the longpress, verify longpress keys for the original base key are displayed
    c. Move to an available longpress key and release - verify the selected longpress key is output
  9. Longpress and move - Case 2:
    a. Longpress a key, but immediately move off the base key before the longpress registers.
    b. While holding the longpress, hold on a separate base key for a few seconds - verify longpress keys for the original base key are displayed
    c. Move to an available longpress key and release - verify the selected longpress key is output
  10. Longpress and move - Case 3:
    a. Longpress a key, but immediately move off the base key before the longpress registers.
    b. While holding the longpress, hold on a separate base key for a few seconds - verify longpress keys for the original base key are displayed
    c. Hold on the separate base key and release - verify nothing is output (only the longpress keys are eligible selections)

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Jan 21, 2022
@darcywong00 darcywong00 added this to the A15S22 milestone Jan 21, 2022
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Jan 21, 2022 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ GROUP_WEB: - Tests KeymanWeb desktop in mobile view

    1 tests PASSED
    • ✅ TEST_TOUCH_KEYS (PASSED): Re-tested this as per Darcy's instructions, Case 3 (Hold on the separate base key and release - verify nothing is output). ie., Hold on the separate base key then quickly move to some other key then release would show nothing as output in the text area.

  • ✅ GROUP_ANDROID: - Tests Android app

    1 tests PASSED
    • ✅ TEST_TOUCH_KEYS (PASSED): Tested this issue in Android emulator API 30 and it is working as expected. (notes)

  • 🟥 GROUP_IOS: - Tests iOS app

    • 🟥 TEST_TOUCH_KEYS (FAILED): ): Re-tested this on iOS 15.2 and I noticed that after long press a key, the Multiple rows are appearing and they hide the base key. (notes)

@github-actions github-actions bot added the fix label Jan 21, 2022
@darcywong00
Copy link
Copy Markdown
Contributor Author

darcywong00 commented Jan 21, 2022

Initial feedback from @MakaraSok is that a key should immediately show preview before waiting a slight delay for longpress

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Jan 23, 2022 •
edited
Loading

Initial feedback from @MakaraSok is that a key should immediately show preview before waiting a slight delay for longpress

Correct. The key preview should always be instantaneous.

@mcdurdin mcdurdin modified the milestones: A15S22, B15S1 Jan 24, 2022
@mcdurdin mcdurdin modified the milestones: B15S1, B15S2 Feb 6, 2022
@darcywong00 darcywong00 force-pushed the fix/android/subkey-move branch from 912166f to 63da0fe Compare February 7, 2022 02:43
@MayuraVerma
Copy link
Copy Markdown
Contributor

MayuraVerma commented Feb 16, 2022

do we have longpress gesture in v15?
can output a character with longpress gesture without selecting the popup key?

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Feb 19, 2022

do we have longpress gesture in v15?

What do you mean by a longpress gesture? Just a longpress on a key without movement? At present that is still the base key. We will be doing a lot more with gestures in 16.0, and that will be the appropriate time to review this.

@MayuraVerma
Copy link
Copy Markdown
Contributor

MayuraVerma commented Feb 19, 2022

With longpress, we are seeing the popup keys. Is it possible for longpress to output shift layer as output instead of popup keys?

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Feb 19, 2022 •
edited
Loading

With longpress, we are seeing the popup keys. Is it possible for longpress to output shift layer as output instead of popup keys?

This can be part of our 16.0 gesture work; see #5029; I will make note of this there.

@mcdurdin mcdurdin mentioned this pull request Feb 19, 2022
Closed
6 tasks
@mcdurdin mcdurdin modified the milestones: B15S2, B15S3 Feb 19, 2022
@darcywong00 darcywong00 force-pushed the fix/android/subkey-move branch from 63da0fe to 39cb6da Compare March 2, 2022 05:57
@darcywong00 darcywong00 changed the base branch from master to beta March 2, 2022 05:58
@mcdurdin mcdurdin modified the milestones: B15S3, B15S4 Mar 4, 2022
@darcywong00 darcywong00 linked an issue Mar 16, 2022 that may be closed by this pull request
bug(android): longpress doesn't reveal the popped up keys all the time #5301
Closed
@darcywong00 darcywong00 marked this pull request as ready for review March 17, 2022 04:52
@darcywong00
Copy link
Copy Markdown
Contributor Author

darcywong00 commented Mar 17, 2022

I think this is ready to test and review

mcdurdin
mcdurdin approved these changes Mar 17, 2022
View reviewed changes
Copy link
Copy Markdown
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!

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Mar 18, 2022

GROUP_WEB: - Tests KeymanWeb desktop in mobile view

  • TEST_TOUCH_KEYS (FAILED): Tested this on Chrome Mobile emulator in Unminified Web page and this is working is fine except Case 3. Because, Hold on the Separate Key then release, Outputs the letter on the Text Area.

Case 1:

Case1C:

Case2C:

Case2LongPress:

Case3:

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Mar 18, 2022

GROUP_IOS: - Tests iOS app

  • TEST_TOUCH_KEYS (FAILED): Tested this on iPhone 12 Pro (iOS 15.2) (on Safari browser) and it is failed due to the Original base key is missing while holding the key for few seconds. (longpress). Also, Hold on the base key then release it would show the Output on the text area.

Case1B:

Case2B:

Case3C: Hold on the separate key then release would outputs the letter on the text area

@darcywong00
Copy link
Copy Markdown
Contributor Author

darcywong00 commented Mar 21, 2022 •
edited
Loading

@bharanidharanj Regarding GROUP_IOS, this doesn't address any iOS popup issues. Those are handled separately in #6383.

Do the iOS steps work as written?
Holding on a base key and releasing should output the base key.

For GROUP_WEB, I cannot reproduce your Case 3
If I longpress y and quickly move to t and hold on t.
Release on t and no character is output.

@mcdurdin mcdurdin modified the milestones: B15S4, B15S5 Mar 21, 2022
@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Mar 21, 2022

GROUP_ANDROID: - Tests Android app

  • TEST_TOUCH_KEYS (PASSED): Tested this issue in Android emulator API 30 and it is working as expected.

Longpress and move - Case 1:

Longpress and move - Case 2:

Longpress and move - Case 3:

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Mar 21, 2022
@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Mar 21, 2022

@bharanidharanj Regarding GROUP_IOS, this doesn't address any iOS popup issues. Those are handled separately in #6383.

Do the iOS steps work as written? Holding on a base key and releasing should output the base key.

For GROUP_WEB, I cannot reproduce your Case 3 If I longpress y and quickly move to t and hold on t. Release on t and no character is output.

@darcywong00 Here, What I did is Hold on a separate base key for some time then release would show the base key letter on the text area. Okay, I will re-test this as per your suggestion in both Group_Web and Group-iOS and will post my result.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Mar 21, 2022

@bharanidharanj Regarding GROUP_IOS, this doesn't address any iOS popup issues. Those are handled separately in #6383.

Do the iOS steps work as written? Holding on a base key and releasing should output the base key.

For GROUP_WEB, I cannot reproduce your Case 3 If I longpress y and quickly move to t and hold on t. Release on t and no character is output.

@darcywong00 On iOS, Holding on a base key then quickly move to some other key will not show any output.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Mar 21, 2022

GROUP_IOS: - Tests iOS app

  • **TEST_TOUCH_KEYS (FAILED): Re-tested this on iOS 15.2 and I noticed that after long press a key, the Multiple rows are appearing and they hide the base key.

Case 1 : While holding the longpress, verify longpress keys for the original base key is missing.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Mar 21, 2022

GROUP_WEB: - Tests KeymanWeb desktop in mobile view

  • TEST_TOUCH_KEYS (PASSED): Re-tested this as per Darcy's instructions, Case 3 (Hold on the separate base key and release - verify nothing is output). ie., Hold on the separate base key then quickly move to some other key then release would show nothing as output in the text area.

@darcywong00
Copy link
Copy Markdown
Contributor Author

darcywong00 commented Mar 23, 2022

I'll go ahead and merge this. The iOS popup key alignment is handled in separate PR.

@darcywong00 darcywong00 merged commit b89a590 into beta Mar 23, 2022
@darcywong00 darcywong00 deleted the fix/android/subkey-move branch March 23, 2022 06:07
@keyman-server
Copy link
Copy Markdown
Collaborator

keyman-server commented Mar 23, 2022

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

@darcywong00 darcywong00 mentioned this pull request Mar 28, 2022
Closed
@jahorton
Copy link
Copy Markdown
Contributor

jahorton commented Jun 6, 2022

In post-mortem, this broke our "roaming touch" behavior on all mobile platforms - the behavior where key previews are shown for the key underneath the user's finger as they move it about the keyboard. (So long as they wouldn't have triggered a longpress by that point, anyway.)

@jahorton jahorton mentioned this pull request Jun 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@jahorton jahorton Awaiting requested review from jahorton

@ermshiperete ermshiperete Awaiting requested review from ermshiperete

@rc-swag rc-swag Awaiting requested review from rc-swag

Assignees

No one assigned

Labels

android/engine/ android/ fix has-user-test web/engine/ web/

Projects

None yet

Milestone

B15S5

Development

Successfully merging this pull request may close these issues.

bug(android): longpress doesn't reveal the popped up keys all the time

6 participants

@darcywong00 @mcdurdin @MayuraVerma @bharanidharanj @keyman-server @jahorton