fix(web): proper handling of key previews, longpresses near-synchronous with layer change 🐵 by jahorton · Pull Request #9803 · keymanapp/keyman

jahorton · 2023-10-19T07:27:17Z

This PR ensures that key highlighting and subkey menus can be properly triggered for keys pressed near-simultaneously with others... even after said 'others' have triggered a layer change.

Warning: might need a 🧼🚿 after this one. The solution works, but it's not super-pretty.

From #9802:

While this fix, as is, does ensure that the second tap acts properly for basic text output... there are still a few lingering complications to resolve - key previews / key highlighting, in particular. It's definitely possible to fix, but I have... reasons to push that off into a follow-up PR.

This is based on #9802. Given the concerns and complications noted there, we need to actively rewrite the path for sources that trigger a state change for a previously-existing gesture sequence (when not becoming part of it) so that KeymanWeb knows the correct layer even before it's time to emit a key. In fact... it's best if it's "immediately" available to KMW.

Basically, this PR "bumps up" when correct information is accessible... in part by deliberately delaying the event that provides the new GestureSource to KeymanWeb. There's also the matter of the rewrite itself, but that's comparatively simpler. The key detail is that the rewrite itself needs the delay, as we shouldn't perform the rewrite if it does .

User Testing

TEST_LAYER_MASH: Using the Android Keyman app and the default keyboard (SIL EuroLatin)...

With your left hand, repeatedly mash between the "numeric" and "shift" layer shifting keys. You're going for a rapid layer rotation.
Simultaneously, with your right hand, repeatedly mash text-outputting keys.
Continue this for five to ten seconds.
FAIL this test if any error notifications pop up and/or the keyboard reloads while following the previous steps. Otherwise, PASS.

TEST_SENTENCE_START: Using the Android Keyman app and the default keyboard (SIL EuroLatin)...

Set the device down on a flat surface.
Hover a left hand finger over the capital W and a right-hand finger over the capital T.
Press both keys simultaneously, then lift.
- If the output text matches either WT or TW, FAIL this test.
- If only one of the letters is emitted, FAIL this test.
- Continue if the output matches either Wt or Tw.
Clear the text, ensuring that the keyboard returns to the Shift layer.
- If the layer doesn't return to Shift when expected, do not fail this test; that's out of scope here. (Do feel free to make a separate issue for it, though.)
Repeat the previous steps at least ten times in total.
- Any failure at step 3 FAILs this test. If there were no failures, PASS.

Re: step 5 - yes, touching simultaneously is hard. That's why the repetitions - you'll near-certainly get it right at least one of the ten tries. (And certainly close enough to repro the error in 16.0-stable.)

keymanapp-test-bot · 2023-10-19T07:27:23Z

User Test Results

Test specification and instructions

✅ TEST_LAYER_MASH (PASSED): Retested as per your suggestion in the Android Mobile (Samsung Galaxy A23 5g) and here is my observation: 1. Installed the attached PR build (Keyman 17.0.200-alpha-test-9803) in the Mobile. 2. In the Keyman In-App, I followed the steps as mentioned in the User Testing area. 3. I was not able to reproduce the error message that I previously posted. Seems to be a random issue. (notes)
✅ TEST_SENTENCE_START (PASSED): 1. Verified that it outputs Wt or Tw after pressing both keys simultaneously. 2. Cleared the text and noticed that the keyboard returned to the Shift layer. 3. Pressed both keys 'W' and 'T' simultaneously more than ten times and I did not see any keys stuck during the process. Also, it outputs either 'wt' or 'tw' on the text input screen. (notes)

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 17.0.200 (0.9803.9592)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

…/web/state-change-source-rewrite

common/web/gesture-recognizer/src/engine/headless/touchpointCoordinator.ts

bharanidharanj · 2023-10-30T10:31:30Z

Test Results

TEST_LAYER_MASH (FAILED): Tested with the attached PR build (Keyman 17.0.190-alpha-test-9803) on an Android Mobile device (Samsung Galaxy A23 5g) and here is my observation: 1. I got an error message when I repeatedly pressed the shifting keys between the "numeric" and "shift" layers.
TEST_SENTENCE_START (FAILED): Tested with the attached PR build (Keyman 17.0.190-alpha-test-9803) on an Android Mobile device (Samsung Galaxy A23 5g) and here is my observation: 1. Upto 3rd step, it is working as expected. ie., I was able to see the letters Wt appear on the screen. 2. I cleared the text as it mentioned in the step 4. Able to see the texts were cleared in the text input screen. 3. I observed that the character key became consistently stuck at one point when I repeated step (3) at least 10 times. Seems to be an issue.

bharanidharanj · 2023-10-30T10:34:36Z

Test Results

TEST_LAYER_MASH (FAILED): Tested with the attached PR build (Keyman 17.0.190-alpha-test-9803) on an Android Mobile device (Samsung Galaxy A23 5g) and here is my observation: 1. I got an error message when I repeatedly pressed the shifting keys between the "numeric" and "shift" layers.
TEST_SENTENCE_START (FAILED): Tested with the attached PR build (Keyman 17.0.190-alpha-test-9803) on an Android Mobile device (Samsung Galaxy A23 5g) and here is my observation: 1. Upto 3rd step, it is working as expected. ie., I was able to see the letters Wt appear on the screen. 2. I cleared the text as it mentioned in the step 4. Able to see the texts were cleared in the text input screen. 3. I observed that the character key became consistently stuck at one point when I repeated step (3) at least 10 times. Seems to be an issue.

jahorton · 2023-10-31T02:12:54Z

I wonder if the errors I fixed in #9893 might be the related to the second test's failure; it does help recover from some 'stuck' scenarios.

As for the first... I can't seem to find any Sentry error reports for that error. Weird. I did manage to get a couple of stuck-highlighting states when attempting a repro on my personal iPhone, though, so maybe I can get something from that.

... and nope - doesn't show anything for iOS either. And the WebView within the mobile app isn't set as inspectable. Looks like I may have to do a local build to investigate that angle further.

…/web/state-change-source-rewrite

jahorton · 2023-10-31T09:10:15Z

@keymanapp-test-bot retest all

@bharanidharanj I've merged in a PR that may fix related details; could you try again, please?

bharanidharanj · 2023-10-31T12:13:51Z

@keymanapp-test-bot retest all

@bharanidharanj I've merged in a PR that may fix related details; could you try again, please?

@jahorton Sure, I will do it.

bharanidharanj · 2023-10-31T13:24:34Z

Test Results

TEST_LAYER_MASH (FAILED): Tested with the attached PR build (Keyman 17.0.200-alpha-test-9803) on an Android Mobile device (Samsung Galaxy A23 5g) and here is my observation: 1. Still, I am getting an error message while repeatedly pressed the 'shift' and 'Num' in the left hand, and randomly pressed the letter keys.

..Installed attached Keyman Version 19.0.200-alpha

..an error message appear on the screen

TEST_SENTENCE_START (PASSED): 1. Verified that it outputs Wt or Tw after pressing both keys simultaneously. 2. Cleared the text and noticed that the keyboard returned to the Shift layer. 3. Pressed both keys 'W' and 'T' simultaneously more than ten times and I did not see any keys stuck during the process. Also, it outputs either 'wt' or 'tw' on the text input screen.

..Sentence appeared correctly on the screen

jahorton · 2023-11-01T01:03:57Z

OK, good. I had a suspicion that the last test, at least, would be fixed by that. Not completely surprised about the first, since we can't seem to get specific error reports out, and it could be a cross-effect.

jahorton · 2023-11-01T01:26:28Z

OK, finally got something out of testing a local build with iOS, once I remembered to rebuild the dependencies (common/web/types, web's build:app/webview).

Unhandled Promise Rejection:  Error: GestureMatcher may not be built against already-completed contact points.

Fortunately... that's a check I've since disabled later in the main PR chain, with no noticeable ill-effects from doing so. Will push a fix on that shortly.

jahorton · 2023-11-01T01:32:18Z

common/web/gesture-recognizer/src/engine/headless/gestures/matchers/gestureMatcher.ts

-      if(baseContact.isPathComplete) {
-        throw new Error("GestureMatcher may not be built against already-completed contact points");
-      }


To be clear, this was never in place due to errors that occurred elsewhere; this was enforcing an early assumption about how the system should work. I've since realized that said assumption is safe to relax.

jahorton · 2023-11-01T01:33:45Z

... and yep, no more errors after rebuilding when testing that change, at least on the local iPhone. All gesture-engine unit tests pass locally, too.

@keymanapp-test-bot retest TEST_LAYER_MASH

@bharanidharanj

bharanidharanj · 2023-11-01T12:05:29Z

Test Results

TEST_LAYER_MASH (PASSED): Retested as per your suggestion in the Android Mobile (Samsung Galaxy A23 5g) and here is my observation: 1. Installed the attached PR build (Keyman 17.0.200-alpha-test-9803) in the Mobile. 2. In the Keyman In-App, I followed the steps as mentioned in the User Testing area. 3. I was not able to reproduce the error message that I previously posted. Seems to be a random issue.

fix(web): rewrites source on triggered state change before emission

01829bc

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Oct 19, 2023

keymanapp-test-bot bot added the epic-gestures label Oct 19, 2023

keymanapp-test-bot bot added this to the A17S24 milestone Oct 19, 2023

github-actions bot added common/ common/web/ fix web/ labels Oct 19, 2023

jahorton mentioned this pull request Oct 19, 2023

fix(web): interactions from layer-shifting during gesture-event processing 🐵 #9802

Merged

fix(web): right, multitaps don't instant-resolve

23d3aa1

github-actions bot added web/ and removed web/ labels Oct 19, 2023

chore(web): Merge branch 'fix/web/state-tokens-and-two-caps' into fix…

83a65f0

…/web/state-change-source-rewrite

github-actions bot added web/ and removed web/ labels Oct 23, 2023

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Oct 24, 2023

jahorton marked this pull request as ready for review October 24, 2023 02:15

jahorton requested a review from mcdurdin as a code owner October 24, 2023 02:15

mcdurdin approved these changes Oct 24, 2023

View reviewed changes

common/web/gesture-recognizer/src/engine/headless/touchpointCoordinator.ts Outdated Show resolved Hide resolved

docs(web): completes partial comment

99131ed

github-actions bot added web/ and removed web/ labels Oct 24, 2023

mcdurdin modified the milestones: A17S24, A17S25 Oct 27, 2023

jahorton linked an issue Oct 30, 2023 that may be closed by this pull request

bug(web): Rapid typing on first caps letter sometimes gives TWo cap letters (touch) 🐵 #7173

Closed

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Oct 30, 2023

keymanapp-test-bot bot added the user-test-failed label Oct 30, 2023

chore(web): Merge branch 'fix/web/state-tokens-and-two-caps' into fix…

4d615b3

…/web/state-change-source-rewrite

github-actions bot added web/ and removed web/ labels Oct 31, 2023

keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels Oct 31, 2023

keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Oct 31, 2023

fix(web): removes unneeded, problematic GestureMatcher check

b003e1b

github-actions bot added web/ and removed web/ labels Nov 1, 2023

jahorton commented Nov 1, 2023

View reviewed changes

keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels Nov 1, 2023

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 1, 2023

Base automatically changed from fix/web/state-tokens-and-two-caps to feature-gestures November 6, 2023 01:27

jahorton merged commit 8e13ca4 into feature-gestures Nov 6, 2023

jahorton deleted the fix/web/state-change-source-rewrite branch November 6, 2023 01:27

Uh oh!

Conversation

jahorton commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Testing

Uh oh!

keymanapp-test-bot bot commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

Uh oh!

bharanidharanj commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

bharanidharanj commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

jahorton commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jahorton commented Oct 31, 2023

Uh oh!

bharanidharanj commented Oct 31, 2023

Uh oh!

bharanidharanj commented Oct 31, 2023

Test Results

Uh oh!

jahorton commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jahorton commented Nov 1, 2023

Uh oh!

jahorton Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahorton commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bharanidharanj commented Nov 1, 2023

Test Results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jahorton commented Oct 19, 2023 •

edited

Loading

keymanapp-test-bot bot commented Oct 19, 2023 •

edited

Loading

bharanidharanj commented Oct 30, 2023 •

edited

Loading

bharanidharanj commented Oct 30, 2023 •

edited

Loading

jahorton commented Oct 31, 2023 •

edited

Loading

jahorton commented Nov 1, 2023 •

edited

Loading

jahorton Nov 1, 2023 •

edited

Loading

jahorton commented Nov 1, 2023 •

edited

Loading