-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
feat(web): delayed modipress completion 🐵 #9973
feat(web): delayed modipress completion 🐵 #9973
Conversation
User Test ResultsTest specification and instructions
Test Artifacts
|
…letion-triggering source
deb35e2
to
f47fd0d
Compare
…ed-modipress-completion
// For multitouch gestures, only report the gesture **once**. | ||
const sourceIDs = selection.matcher.allSourceIds; | ||
for(let sequence of this._activeGestures) { | ||
if(!!sequence.allSourceIds.find((id1) => !!sourceIDs.find((id2) => id1 == id2))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O(m*n)? Are these arrays short?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A person only has ten fingers (corresponding to allSourceIds
), and I'd be quite impressed if someone actually managed to use each of them independently while building concurrent gestures (which corresponds to the for... let
part). Technically, completed touches can be included in the allSourceIds
array - it's needed for multitap history.
Secondly, with the way Web currently works, I believe the maximum count for concurrent gestures is 2; a modipress and whatever the current input is on the modipress layer. Starting a third will immediately resolve the previously-current input (simple tap, flick, multitap) or be blocked (longpress). There's a chance of it actually being 3 - I'm not 100% sure that a held backspace will be auto-cancelled on new input. But yeah, that should be it.
This does depend on proper state management of the 'active gestures' array. If errors happen that prevent removal of finished gesture objects from that array, then over time it may become "less short" than intended.
The potential for most growth is in allSourceIds
if someone goes crazy with constant 'rota' key mashing, but even then, it's still practically just O(n) due to the low cap on this._activeGestures
's practical length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I figured but helpful info on the potential edge cases, thanks!
@keymanapp-test-bot retest TEST_10_KEY_DIACRITICS @bharanidharanj That failed test is on me; I forgot that we changed a related behavior with #9944. I've updated the test instructions accordingly. Turns out that I discovered a bug on Also, something probably worth note/making an issue: TEST_APP_10_KEY_DIACRITICS will fail if performed on an iOS device if the As it appears to be due to something iOS-specific and does not occur on Android, I think this issue should be documented and pursued separately. On Android, the context-initial diacritic doesn't seem to get displayed properly in the emulator setup I use most frequently, but it is still present and does still combine properly according to keyboard rules. (Notably, placeholder text disappears when it should be visible, indicating the presence of text... even if it - the isolated diacritic - is not being displayed properly.) That'd be more of a rendering issue within the app and/or on Android, I guess. |
Eh, why not. Did a little investigation: our old enemy, U+200E (https://unicode-explorer.com/c/200E) has returned to haunt us. It's being automatically inserted at the start of the context whenever a diacritic is added in that position or directly after a different diacritic meeting this compound condition (e.g., it can chain). This is occurring via keyman/ios/engine/KMEI/KeymanEngine/resources/Keyman.bundle/Contents/Resources/ios-host.js Lines 292 to 295 in 6b8a06a
This has been verified via breakpoint when performing a repro for the conditions described above - that if-condition will trigger, resulting in, among other things, a deadkey-wipe. The Swift-side interface that calls this: keyman/ios/engine/KMEI/KeymanEngine/Classes/Keyboard/KeymanWebViewController.swift Lines 231 to 251 in 6b8a06a
Well, huh. That should handle it, right? Well... here's the thing... So... there's no match, the byte-header doesn't get filtered, do not pass Go, do not collect $200, etc. Of course, that did work before... so a recent-ish version of iOS must have changed how the byte-header gets encoded. The filter-block was written in #1756 for version 14.0 in May 2019, and there have been quite a few new versions of iOS since then. Also, I'd noticed weirdness with predictive-text even on |
Given the investigation and related behaviors I've noted, it's a bug on the current |
And, of course, fixing the fact that context-resets weren't resetting deadkeys (a la #10039)... caused me to run into a different context-sync issue, at least on iOS. Now checking if it also happens on Android. Core issue there: there are two text updates during a multitap: 'restore original context' and then 'apply new text effects'. If text is updated from the app based on the first and not the second, while the JS side has done both, that causes a context desync. |
…multitap context-reversion
As that last comment is something only relevant for |
Test Results |
This aims to cover a few "fun" edge cases that can easily arise during quick typing that involves modipresses:
The main idea behind point 2 above: during quick typing, it's very natural to lift the modipress finger to prepare to type a new key. This could easily trigger earlier than intended, so we keep the modipress active while the flick is still active.
Point 1 above then provides consistency with point 2 - in both cases, the layer "sticks" until the pending gesture is completed.
In case the user accidentally presses a new key either slightly before or simultaneously with release during this state, we can still allow the flick to complete. Since this is generally during quick typing, and the user did lift the modipress key, they've likely has anticipated the layer to return - and so we adjust for that expectation.
I will say that point 2 above was an interesting niche case to implement - it involved a second coming of the TWo caps problem (#7173 / #9802) from a slightly different angle. While it does add some complexity, it should help make flicks more robust to certain natural 'optimizations' we often subconsciously make while typing quickly.
Notes for future revisitation
❗ ❗ ❗
I ended up throwing in quite a number of other fixes into this one, as it held the big user test suite and was used as the anchor-point for demo builds. Not exactly ideal, but it is what it is at this point. FWIW, I did aim to keep the commits reasonably clean, so going commit by commit might help... though there was a merge from the base (including a master update) that may make certain aspects interesting to follow.
User Testing
Note that most of these tests will be repeats of user tests from previous PRs. Consider this largely a feature-branch regression-test suite... and thus tests worthy of consideration for becoming permanent regression tests once the feature-branch lands.
TEST_10_KEY_ROTA: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
8
key once, then wait for a second.8
key twice in quick succession, then wait for a second.8a
.8
key continuously for at least 2 seconds.8a8
,8aa
,8ab
,8ac
,8aA
,8aB
,8aC
, then back to8a8
(and continue from there)9
key once. AD
should be emitted as the result.9
for at least five seconds, paying attention to the key hints.TEST_10_KEY_DIACRITICS: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
8
key.à
à
.9
key.àê
ê
.ê
.8
key.àềa
à
's accent-mark on top the originalê
, as in the cropped screenshot above.a
.TEST_APP_10_KEY_DIACRITICS: Using the Keyman for Android test artifact...
77
is output.8
key.77à
à
.9
key.77àê
ê
.8
key.77àềa
à
's accent-mark on top the originalê
, as in the cropped screenshot above.a
.TEST_BASIC_SIMPLE_SHIFT: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
TEST_BASIC_MODIPRESS: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
A
key, verifying that it produces anA
.shift
layer'sA
key, then releasing quickly.TEST_BASIC_MODIPRESS_HOLD: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
TEST_NUMERIC_FROM_SHIFT: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
123
) underneath the shift key.%
key and select the‱
subkey.‱
is emitted as text.%
key and select the‱
subkey.TEST_DELAYED_SUBKEY: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
123
) underneath the shift key.%
key, but do not select a subkey yet.‱
subkey.‱
is emitted as text and that the layer has changed back to theshift
layer.TEST_DOUBLETAP_CAPS: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
All
.CAPS
.work
.Well
.All CAPS work Well
TEST_CUSTOM_MULTITAP_MODIFIER: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
◌́
key. (The key between the spacebar [left] and the Enter key [right].)◌́
accent mark.á
and verify that the corresponding letter is output.◌́
key; you should be returned to the default layer.◌́
, then triple-tap it, holding on the third tap.◌̂
mark◌́
-key's hint.â
and verify that the corresponding letter is output.◌́
key; you should be returned to the default layer.TEST_ALTERNATING_SHIFT_AND_KEY: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
H
key and release it.H
is output and that the layer returns todefault
after step 4.h
orH
) is not output each time you press theh
/H
key.123
) key and verify that the layer switches properly to keys with numbers in the top row and symbols in the middle two rows.TEST_FLICKS_BASIC: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
u
key.ù
u
key again and drag it in a northeast direction for at least a key-width before releasing it.ú
(does not replace the previous output)u
again and drag north (straight up) in the same manner.û
.TEST_FLICK_CORRECTION: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
u
key.ù
u
key.u
. The flick animation and key preview should disappear.u
key again.u
.TEST_FLICK_LOCKING: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
u
key.û
key in the preview area (assuming it's not blocked by your finger).ú
as you do so.ú
is visible and in the center, lift your finger and end the gesture.ú
is output.a
key; you should see similar previews and text there. (â
,á
)TEST_MODIPRESS_MULTITAP_FLICK_PREVIEW: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
U
key.U
purely north (straight up) - you should see theÛ
key in the preview area (assuming it's not blocked by your finger).Û
.Û
fully in-view, release the flick.Û
is output.TEST_FLICK_DURING_MODIPRESS: Using the standard "Test unminified KeymanWeb" test page from a mobile device...
U
key.Û
key in the preview area (assuming it's not blocked by your finger).Ú
as you do so.Ú
is visible and in the center, tap on theQ
key.Ú
orÚq
is output.Úq
is ideal, but if youra
tap is quick, you may only get theÚ
.I will note that sometimes, when doing the input quickly, I get only
Ú
(and not theq
). Not sure exactly why that is, but I felt it may be worth noting... and worth a future investigation. It is likely typing-speed related, since there's a fair bit ofawait
-async
there.