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

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

Closed
Tracked by #7161
mcdurdin opened this issue Aug 30, 2022 · 7 comments · Fixed by #9803
Closed
Tracked by #7161
Assignees
Milestone

Comments

@mcdurdin
Copy link
Member

Describe the bug

Repro: Press Shift, type two letters very rapidly. They will both get capitalized. (Two thumb typing)

Per @jahorton:

  • [JH] Does repro on iOS.
  • [JH] I suspect it’s due to how the Web OSK operates - for this scenario, the user does press both shift-layer keys before the display-layer is updated.

Screenshots

Screen_Recording_20220831-061413_Keyman.mp4

Keyman for Windows/macOS/Linux/iPhone/iPad/Android:

  • Device: Samsung A90 5G
  • OS: Android 12
  • Keyman version: 15.0.269-stable
@sentry-io
Copy link

sentry-io bot commented Oct 6, 2022

I (@jahorton) have been able to confirm that this Sentry event is highly related.

Sentry issue: KEYMAN-WEB-4J

Repro: using sil_euro_latin...

  1. With your left hand, repeatedly mash between the "numeric" and "shift" layer shifting keys. You're going for a rapid layer rotation.
  2. Simultaneously, with your right hand, repeatedly mash text-outputting keys.

Corresponding events to the one linked above should appear on Sentry.

In essence, this happens when two touches are near-simultaneous. If touch 1 is for a switch key, it takes a little time for JS and the browser's renderer to complete the OSK layer-switching operation; it's quite possible for touch 2 to arrive while the original layer is still in place. In such a case, touch 2 will register against the corresponding key on the original layer, leading to this error - the selected key is not on the current layer at the time that touch 2 gets processed.

@jahorton
Copy link
Contributor

I (@jahorton) have been able to confirm that this Sentry event is highly related.

Sentry issue: KEYMAN-WEB-4J

A bit of further thought here: a similar (Sentry) issue arose with #6898, which has been mitigated by #7543. In that case, the issue arises from the wholesale swapping of the active keyboard - an operation that is particularly asynchronous if the keyboard being swapped in has not yet been loaded. In this case, the issue is that we're swapping the layer during JS event processing, rather than the keyboard - but the overall pattern is the same - a swap in active state occurs during JS event processing before the second event has a chance to start, especially since JS is explicitly single-threaded and doesn't do interrupts.

Noting the mitigation strategy taken by #7543, a similar strategy (using closure-based handlers that lock in the touch's original active layer) could be used to nullify the Sentry error report linked above. That said, the base issue posted here - the "TWo caps letters" issue - would remain.

@jahorton
Copy link
Contributor

jahorton commented Nov 10, 2022

A possible strategy to resolve the "TWo caps" issue instead would require a bigger rework, but it's something that the feature-gestures branch is decently poised to do come integration time. Even then, it's less a "big rework" than a "big validation user test suite" - the shift would be large enough to raise concerns for possible new bugs, even if the code changes themselves aren't likely to be very substantial.

The core of this strategy: part of our fat-finger algorithm (see ActiveLayout.keyTouchDistances) is quite well-suited to detecting the closest key to an incoming touchpoint. If we ignored the TouchEvent's target-element metadata and instead relied entirely on this key-touch distances function (though, a modified version that doesn't mask non-output keys!), that would dynamically adjust correctly (to user expectations) after a layer swap. That said, this is almost definitely not a change we should "throw in" this late in the release cycle.

@mcdurdin
Copy link
Member Author

Seems like a good possibility.

That said, this is almost definitely not a change we should "throw in" this late in the release cycle.

Agreed.

@mcdurdin mcdurdin changed the title bug(android): Rapid typing on first caps letter sometimes gives TWo cap letters bug(web): Rapid typing on first caps letter sometimes gives TWo cap letters (touch) Dec 9, 2022
@mcdurdin mcdurdin modified the milestones: 17.0, A17S19 Jul 7, 2023
@mcdurdin mcdurdin modified the milestones: A17S19, A17S20 Aug 18, 2023
@mcdurdin mcdurdin modified the milestones: A17S20, A17S21 Sep 1, 2023
@mcdurdin mcdurdin modified the milestones: A17S21, A17S22 Sep 15, 2023
@mcdurdin mcdurdin modified the milestones: A17S22, A17S23 Oct 1, 2023
@jahorton jahorton changed the title bug(web): Rapid typing on first caps letter sometimes gives TWo cap letters (touch) bug(web): Rapid typing on first caps letter sometimes gives TWo cap letters (touch) 🐵 Oct 2, 2023
@mcdurdin mcdurdin modified the milestones: A17S23, A17S24 Oct 15, 2023
@mcdurdin mcdurdin modified the milestones: A17S24, A17S25 Oct 27, 2023
@bharanidharanj
Copy link

@mcdurdin I am able to reproduce this issue with the latest Keyman 17.0.205 alpha build in Android OS . (Version 5, 9 and 12)

@jahorton
Copy link
Contributor

That sounds correct; the fixes are currently only on the feature-gestures feature branch.

@mcdurdin
Copy link
Member Author

mcdurdin commented Feb 1, 2024

@jahorton this apparently failed with 17.0.238-alpha (see #10598), but this was apparently fixed with 17.0.230-alpha with the merge of #7324. Are you able to follow up with @bharanidharanj?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants