feat(web): beginning of gesture-recognizer integration with the OSK 🐵 #9657
Conversation
keymanapp-test-bot
bot
added
the
user-test-missing
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
keymanapp-test-bot
bot
removed
the
user-test-missing
jahorton
force-pushed
the
feat/web/base-recognizer-integration
branch
from
0006296
to
0429a1c
Compare
|
A number of the full-integration KMW tests based upon OSK integration are failing. ... that tracks. I'm ripping out lots of older code and slowly linking the new gesture engine in, but it's incomplete at this stage. |
jahorton
force-pushed
the
feat/web/base-recognizer-integration
branch
from
0429a1c
to
de83fee
Compare
jahorton
changed the base branch from
fix/web/recognizer-polish-and-source-uniqueness
to
refactor/web/common-layout-typing
jahorton
force-pushed
the
feat/web/base-recognizer-integration
branch
from
de83fee
to
86aee99
Compare
…t/web/base-recognizer-integration
| @@ -54,4 +58,129 @@ export default class OSKLayerGroup { | |||
| lDiv.appendChild(layerObj.element); | |||
| } | |||
| } | |||
|
|
|||
| findNearestKey(coord: Omit<InputSample<KeyElement>, 'item'>): KeyElement { | |||
There was a problem hiding this comment.
Relocated from visualKeyboard.ts.
| * @return {Object} nearest key to touch point | ||
| * | ||
| **/ | ||
| findNearestKey(input: InputEventCoordinate, t: HTMLElement): KeyElement { |
There was a problem hiding this comment.
Relocated to oskLayerGroup.ts.
| itemPriority: 0, | ||
| pathResolutionAction: 'resolve', | ||
| timer: { | ||
| duration: 500, |
There was a problem hiding this comment.
Is this the only place this magic is used?
There was a problem hiding this comment.
This gets handled far better within #9698. It... might be the only place within this PR, but I'm unsure of that.
From said PR:
keyman/web/src/engine/osk/src/input/gestures/specsForLayout.ts
Lines 51 to 58 in f67549a
| evaluate: (path) => { | ||
| if(path.isComplete) { | ||
| return 'resolve'; | ||
| } | ||
| } |
There was a problem hiding this comment.
| evaluate: (path) => { | |
| if(path.isComplete) { | |
| return 'resolve'; | |
| } | |
| } | |
| evaluate: path => path.isComplete ? 'resolve' : undefined, |
Is undefined really what we should be returning otherwise?
There was a problem hiding this comment.
Looks like I need to update the doc-comment (TrackedPath => GesturePath), but the rest of this holds.
If you think it feels better, the actual check is "falsy"-based, so null should work as well.
| evaluate: (path) => { | ||
| const stats = path.stats; | ||
| if(stats.rawDistance > LongpressDistanceThreshold) { | ||
| return 'reject'; | ||
| } | ||
|
|
||
| if(path.isComplete) { | ||
| return 'reject'; | ||
| } | ||
| } |
There was a problem hiding this comment.
| evaluate: (path) => { | |
| const stats = path.stats; | |
| if(stats.rawDistance > LongpressDistanceThreshold) { | |
| return 'reject'; | |
| } | |
| if(path.isComplete) { | |
| return 'reject'; | |
| } | |
| } | |
| evaluate: path => | |
| path.stats.rawDistance > LongpressDistanceThreshold ? 'reject' | |
| : path.isComplete ? 'reject' | |
| : undefined, |
| evaluate: (path) => { | ||
| const stats = path.stats; | ||
|
|
||
| // Adds up-flick support! | ||
| if(stats.rawDistance > LongpressFlickDistanceThreshold && stats.cardinalDirection == 'n') { | ||
| return 'resolve'; | ||
| } | ||
|
|
||
| return MainContactLongpressSourceModel.pathModel.evaluate(path); | ||
| } |
There was a problem hiding this comment.
| evaluate: (path) => { | |
| const stats = path.stats; | |
| // Adds up-flick support! | |
| if(stats.rawDistance > LongpressFlickDistanceThreshold && stats.cardinalDirection == 'n') { | |
| return 'resolve'; | |
| } | |
| return MainContactLongpressSourceModel.pathModel.evaluate(path); | |
| } | |
| evaluate: path => | |
| // up-flick support | |
| path.stats.rawDistance > LongpressFlickDistanceThreshold && | |
| path.stats.cardinalDirection == 'n' ? 'resolve' | |
| : MainContactLongpressSourceModel.pathModel.evaluate(path), |
| evaluate: (path) => { | ||
| if(path.isComplete && !path.wasCancelled) { | ||
| return 'resolve'; | ||
| } | ||
| } |
There was a problem hiding this comment.
| evaluate: (path) => { | |
| if(path.isComplete && !path.wasCancelled) { | |
| return 'resolve'; | |
| } | |
| } | |
| evaluate: path => path.isComplete && !path.wasCancelled ? 'resolve' : undefined, |
| evaluate: (path) => { | ||
| if(path.isComplete && !path.wasCancelled) { | ||
| return 'resolve'; | ||
| } | ||
| } |
There was a problem hiding this comment.
| evaluate: (path) => { | |
| if(path.isComplete && !path.wasCancelled) { | |
| return 'resolve'; | |
| } | |
| } | |
| evaluate: path => path.isComplete && !path.wasCancelled ? 'resolve' : undefined, |
| } | ||
| ], | ||
| sustainTimer: { | ||
| duration: 500, |
There was a problem hiding this comment.
I think we need to surface the magic timing constants somewhere together
| // TODO: needs better abstraction, probably. | ||
|
|
||
| // But, to get started... we can just use a simple hardcoded approach. | ||
| const modifierKeyIds = ['K_SHIFT', 'K_ALT', 'K_CTRL']; | ||
| for(const modKeyId of modifierKeyIds) { | ||
| if(baseItem.key.spec.id == modKeyId) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
So modipress is only working for desktop modifier keys? That's going to need significant work still I think.
There was a problem hiding this comment.
Considering I've done little to actually integrate modipress thus far... yes, it will need significant work.
…t/web/base-recognizer-integration
It's still quite rough, but this PR integrates the OSK (but not the banner) with the new gesture-recognition engine... but just for "simple tap" operations. Not for any of the other interaction types, such as: longpresses, multitaps, special keys, flicks, held backspaces on touch OSKs, etc.
One of the primary pushes for this PR was full conversion of the OSK's
findNearestKeyfunction - the original inspiration for, and the intended target for, the gesture-recognizer'sitemIdentifierconfiguration functor field. This is the part responsible for determining which key is pressed (the inspiration for the "item" abstraction) - something obviously critical for actually using a keyboard. It has been converted for compatibility with the gesture-engine and its assumed coordinate system.The other push, naturally, was to actually connect the OSK with the new engine. Basic wiring is now in place; any gestures reporting keys directly will produce corresponding key events. This is even enough to trigger modifier keys and allow language switching, even if not as immediately as they were triggered before. That said, complex gestures - such as a longpress's subkey-selection mode - do not trigger any Web-side UI or integration as of yet.
In this PR's present state, there are a few very large, very obvious sections of commented code. These sections have only been partially converted, and there are relevant bits there yet to be properly integrated with the new engine.
@keymanapp-test-bot skip
I mean, I could start writing user tests at this point... but given how old OSK functionality is broken in this PR due to the low state of integration, I feel it's better to delay in order to confusion between the "intentionally broken" parts and the "unintentionally broken" parts that tests would be looking for at this stage.
At this stage, a build failure is expected. It will be resolved within 2 PRs, as of #9719.