-
-
Notifications
You must be signed in to change notification settings - Fork 112
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): implementation of modipress, integration with multitap 🐵 #9876
Conversation
The removed block could cause race-condition-like errors due to async input handling
User Test ResultsTest specification and instructions
|
I think we need to make sure our keyboard design guidance notes this. |
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 few more thoughts on this one but LGTM once those are addressed
try { | ||
if(shouldLockLayer) { | ||
this.lockLayer(true); | ||
} | ||
keyResult = this.modelKeyClick(gestureStage.item, coord); | ||
} finally { | ||
this.lockLayer(false); | ||
} |
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.
Why do we need try
here? Are we expecting exceptions to be thrown?
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.
Expecting? No, not during standard use. But that's why they're "exceptions". Note: I don't even try to catch
them - I'm just looking to ensure a critical recovery step happens even if they occur.
this.modelKeyClick
triggers code that links into rule processing, which isn't exactly a narrow area of code. Should something unexpectedly break, we want to at least "unlock" the layer even if an error occurs... otherwise, the OSK will appear to "lock up" to end users, and that's far from ideal.
Sometimes we've seen errors from individual keystrokes that don't break things for other keystrokes. If KMW isn't completely broken, just failing that one keystroke can allow a form of recovery... in which we want to ensure that any layer-lock (modipress) manipulations we do here also recover if possible.
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.
Okay, we just haven't got anywhere near that level of robustness elsewhere in KeymanWeb, so this is a bit of a new pattern.
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 text-search across the repo for } finally {
actually turns up 8 pre-existing instances of this exact pattern. It's not the first time I've used it like this.
Test Results
|
@keymanapp-test-bot retest all While it's nice that some of the tests passed, I've noticed that fixing one aspect sometimes has led me to break others. It's best to do a full retest to ensure nothing is left broken by accident. |
Another goal from #5029: introducing the "modipress". (#5029 (comment) for the original mention.)
Short for "modi-fier long-press", this is a gesture interaction in which the user may hold down any of our recognized modifier keys to temporarily shift the keyboard's active layer... as if it were a longpress key. (Such behavior exists in iOS's default keyboard, for example.) Its layer will then be locked in place until the interaction ends, allowing the user to type with the keys of that layer. Lifting the finger holding the modifier key will then return to the original layer.
Bonus: this also works with layers reached through multitapping on such a key! Tapping once, then holding on a second tap will activate the layer corresponding to the first
multitap
key under the modifier key - the key that'd normally be reached by a second tap.At present, this is locked by the following logic:
keyman/web/src/engine/osk/src/input/gestures/specsForLayout.ts
Lines 82 to 94 in facafe3
The main thing: it's a lot simpler if we don't have to rely on key-event rule processing to determine whether or not a key should be considered for modipresses. If an approach can fit within the method posted above ("... the following logic"), it'll be very easy to integrate. We can even add access to the compiled keyboard object (say, for a "here's all keys for modipress" property) if needed.
As for the keys included above... including
K_NUMERALS
, in particular, gives us numeric-layer modipress forsil_euro_latin
.K_LOWER
(the numeric-layer 'abc' key) is not currently included, but perhaps I should correct that. That said, thePossibly relevant codes for consideration:
keyman/common/web/keyboard-processor/src/text/codes.ts
Lines 71 to 74 in 6aaf58a
As for this part:
if(key.sp > 0 && key.sp != 8 && key.nextlayer)
- if the key is marked for special styling that doesn't match deadkeys and has anextlayer
property, we'll trust that key "in good faith" as a modipressable layer-swap key.Reference:
keyman/common/web/types/src/keyman-touch-layout/keyman-touch-layout-file.ts
Lines 67 to 89 in 6aaf58a
(The version directly above has not yet reached this branch, though.)
It'd be a "heuristic" of sorts, though - there's nothing stopping someone from special-styling a text-output key that would swap the layer too. Hopefully folks don't consider that a "feature" and instead wisely design their layouts accordingly.
One extra-fun interaction:
If a subkey menu has been opened during a modipress and the modipress key is lifted, subkeys still work... but any
nextlayer
attribute on the subkeys will not take effect, as it's from a subkey of the modipressed layer.While we could "just" cancel the subkey-menu operation... I think this scenario is both niche enough and consistent enough to be reasonable. I think it could be possible to maintain the layer-lock until the modipressed longpress is complete, but that will take extra effort to accomplish.
User Testing
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 does not change as a result.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_MISCELLANEOUS: If anything you consider an error or bug occurs that seems unrelated to the tests, please note it here.