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(web): blocks nextLayer for keys quickly typed when multitapping to new layer when final tap is held #11189

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Apr 8, 2024

Fixes #11187.

This fixes an issue where a held layer-swap was being cancelled incorrectly upon use of an output key. Conditions needed for the error to trigger:

  • the output key's layer was reached by multitap + hold, not a single tap + hold
  • the output key was tapped before the current layer would otherwise be "locked in" due to time held. (That is, tapping the layer-change key again, rather than the output key, would continue the layer-changing multitap.)

To be clear, if there's a pause before keys are tapped (enough to wait out that "lock in" timer) but the scenario is otherwise the same, that is working already.

Turns out that I missed something small in #10838, with its changes being why this specific behavior broke. The efforts to avoid an asynchronous infinite loop, as implemented, caused the ongoing gesture to self-cancel at the exact point when it should have transitioned to the "locked-in modipress" state. With a small tweak to the logic, we can allow post-init model rejection to still trigger replacement while ensuring init-time model rejection still auto-cancels, which preserves #10838's fix while fixing #11187 as well.

User Testing

TEST_NUMERIC_MULTITAP: Using Keyman for Android and sil_euro_latin...

  1. Triple-tap the numeric key, holding it down on the final tap.
  2. Tap any key that produces text, still holding down the finger that tapped the numeric key.
  3. Verify that the layer does not change when text is output.
  4. End all ongoing touches and gestures.
  5. Verify that the layer changes afterward.

@jahorton jahorton requested a review from mcdurdin as a code owner April 8, 2024 07:56
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Apr 8, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Apr 8, 2024

User Test Results

Test specification and instructions

  • TEST_NUMERIC_MULTITAP (PASSED): Retested with the attached PR build (Keyman 17.0.305-beta-test-11189) on an Android 13 Mobile device and here is my observation: 1. Tripple tapped the numeric key, holding it down on the final tap. 2. Tapped some other keys that produce text and verified that the tapped keys showed the outputs on the screen. 3. Completed all ongoing touches and gestures, then released the Number key instead of holding it. Verified that the layer changed afterward.

Copy link
Contributor

@darcywong00 darcywong00 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

bharanidharanj commented Apr 9, 2024

Test Results

  • TEST_NUMERIC_MULTITAP (FAILED): Tested with the attached PR build (17.0.303-beta-test-11189) on an Android 13 mobile device and here is my observation: 1. Triple tapped the number key then holding the key on the final tap. 2. Tapped some other key from the keyboard leads to Keyboard error message. I have attached the video file for reference.
keyboarderr.mp4

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Apr 9, 2024
@github-actions github-actions bot added web/ and removed web/ labels Apr 10, 2024
@jahorton
Copy link
Contributor Author

jahorton commented Apr 10, 2024

The keyboard error detected corresponds to #11165, the fix for which (#11178) only just merged in.

The Sentry event: https://keyman.sentry.io/share/issue/799c39d76ad541c9b9ed7d65aac52ce1/ (Confirmed by matching release: [email protected])

While I could override... this is also based directly on beta, so how about I just merge that fix in and we do a quick retest, eh? (And... merge done.) I couldn't help but notice that the sequence actually executed correctly... aside from signalling that error.

@keymanapp-test-bot retest all

@keymanapp-test-bot keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels Apr 10, 2024
@bharanidharanj
Copy link

Test Results

  • TEST_NUMERIC_MULTITAP (PASSED): Retested with the attached PR build (Keyman 17.0.305-beta-test-11189) on an Android 13 Mobile device and here is my observation: 1. Tripple tapped the numeric key, holding it down on the final tap. 2. Tapped some other keys that produce text and verified that the tapped keys showed the outputs on the screen. 3. Completed all ongoing touches and gestures, then released the Number key instead of holding it. Verified that the layer changed afterward.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Apr 10, 2024
@jahorton jahorton merged commit d075c8f into beta Apr 10, 2024
19 checks passed
@jahorton jahorton deleted the fix/web/quick-multitap-modipress-use branch April 10, 2024 07:33
@keyman-server
Copy link
Collaborator

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

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(web): modipress held from multitapping releases early if first key from new layer is pressed too early
4 participants