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

refactor(web): longpresses, groundwork for additional gestures 🍕 #5387

Merged
merged 49 commits into from
Jul 8, 2021

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Jun 29, 2021

This accomplishes the same goals as the "subkey abstraction" series, just in a single PR instead of multiple. The goal: to modularize how KMW handles longpresses in order to display subkeys. Since it's accomplishing the same work as that series, much of #5294's description is also relevant here.

This modularization actually gives rise to two nice interfaces that should prove useful when we look to add support for additional input gesture types, such as flick and multitap from the LDML spec. We aren't implementing those now, but this groundwork will facilitate their development once we're ready.


A few pieces especially worth highlighting:

export interface PendingGesture {
readonly baseKey: KeyElement;
readonly promise: Promise<RealizedGesture>;
cancel(): void;
resolve?(): void;
}

PendingGesture - can model a longpress gesture that has not yet been confirmed, thus does not block base-key interactions / change of highlighted key

export interface RealizedGesture {
readonly baseKey: KeyElement;
readonly promise: Promise<text.KeyEvent>;
clear(): void;
isVisible(): boolean;
updateTouch(touch: Touch): void;
}

RealizedGesture - can model a confirmed longpress gesture that has displayed subkeys but not yet completed (as in, hasn't yet returned a subkey)


Toward new input gestures in the future

The abstractions from the previous block also provide a starting point to, in the future, model LDML's flick and the multitap attribute.

  • There's nothing saying a gesture can't also be automatically 'realized' / resolved.
    • I would imagine this to be useful for flick events. Promises can be built pre-resolved, after all.
  • Once one potential gesture is 'realized' for a touch event, the others ought be cancelled.
    • multitap may make this a bit more complex, but it'll be manageable via some sort of state machine if so.
  • multitap doesn't clear for new touches (on the same base key) when others would, as it is a gesture across multiple touches.
    • Still, it can simply be left 'pending' until the confirming second tap is released, which would then auto-cancel flicks, longpresses, and (new, non-multitap) output from the base key while taps continue to maintain the new 'realized' / 'active' state.
    • Simillarly, a confirmed flick could auto-cancel potential multitaps, which (approximately) maintain the same touch coordinate.
    • Trickiest part: if the output should "rotate" as multitaps continue, the RealizedGesture.promise typing may need a minor rework, as Promises may only resolve once. That's a bridge we can cross when we come to it, though.
  • So, having a common abstraction for all gestures simplifies tracking all gestures for a touch event & for mass-cancellation of the gestures that didn't "win".

We'll probably want some sort of formalized "gesture engine" before implementing either. Design pending, of course, though at least the new abstractions will greatly facilitate that process.


Unit testing

@MakaraSok

As this PR is a focused rework of the longpress gesture, we'll want a fairly extensive round of tests that ensure functionality has been maintained. While I've done so myself, it's always wise to get extra perspectives; it's possible I may have missed something.

There are two test suites I can specify that focus well on certain niches of KMW longpress behavior. The more platforms we can test against, the better. There's also a third suite to check for possible cross-effects with predictive text.

The most important test device configurations:

  • Within the Keyman for Android app
  • or its system keyboard

All other configurations use 'in-browser' implementations rather than 'embedded', so pick one or two from the "exhaustive" list below.

If we want exhaustive testing:

1. sil_cameroon_qwerty

Of particular interest: the W and E keys on the shift layer have subkeys set with layer = default and nextlayer = default. (Most of the keys on the 'shift' layer are set with the same nextlayer property.) Of course, there are a lot of other subkeys as well - but be especially sure to test W and E - they're the most niche and therefore most prone to breakage. Test some subkeys on the default layer as well - w and e should be fine.

Naturally, the subkeys on the default layer do not layer shift.

All tested subkeys should output the text seen on their keycap. For any keys on the shift layer that automatically change layer to the 'default' layer (including W and E), their subkeys should do the same. (Specified by the keyboard's developer; this isn't a general, hard and fast rule for all keyboards.)

For some extra rigor:

The symbolic layer also has a few keys with subkeys that layer-shift:

  • the angle-bracket keys (<< and >>)
  • the quote ' and double-quote "" keys

It also has keys with subkeys that don't layer shift:

  • (, )
  • ?, !

The keyboard's unique layers (using the tricolored key) do not possess any subkeys.

Finally, some other patterns to test with this keyboard: use a longpress gesture on shift's W, but 'cancel' it by moving your finger back to the main key, then release. It should result in a standard keypress for that key.

  • Do a similar test for default-layer w and a couple of other subkeys.
  • Do a similar test for each, but move your finger away from the base key, then release. Nothing should be emitted.
  • Key highlighting should update to match and disappear upon release of the base key or moving away from it.
  • You should also be able to simply release the longpress immediately without moving your finger and emit the base keystroke. (The key will not highlight in this case.)

2. basic_khmr

As the distributed version of its package doesn't include the JS, I've handmade a version that includes it. This is only needed for tests within the Keyman apps: basic_kbdkhmr.kmp.zip

For tests against 'in-browser' KMW, simply load the basic_khmr (Khmer Basic) keyboard. The standard unminified testing page should suffice.

This keyboard does not specify a touch layout, so it uses the default one constructed by KMW. This results in a keyboard with subkeys on SHIFT that exist solely to change the current layer - no output. The embedded-mode's subkey-processing code path has received a few simplifications that could affect such keys; please ensure that the keyboard shifts layers as expected when using SHIFT's subkeys.

These layer-shifting subkeys should only exist on the default layer; other layers should directly return to the default layer when the layer-shift key is pressed.

So, try testing the following layer-shift subkeys:

  • rightalt
  • shift+rightalt
  • leftalt+leftctrl

Finally, one last bit of testing with this keyboard: use a longpress gesture on shift, but 'cancel' it by moving your finger back to the main key, then release. It should layer-shift to the "shift" layer.

  • Also rerun the test, but move your finger away from the shift key before releasing. You should remain on the default layer.

3. sil_euro_latin (English)

While most of the subkey handling is well-handled by the tests established above, there's one last thing that's important to consider: subkeys can be used in predictive text.

When typing words and using subkeys, do unreasonable predictions arise? Do predictions arise when expected? (Note that subkeys should be considered 'replaceable' with their base key. For example, when using the ŝ subkey you should expect corrections that replace it with a plain s.)

jahorton added 30 commits June 16, 2021 15:17
… into refactor/web/osk-centralized-gestures
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no remaining comments, just awaiting final re-test from @MakaraSok for approval.

web/source/osk/browser/pendingLongpress.ts Show resolved Hide resolved
@MakaraSok
Copy link
Collaborator

I'm trying to setup and test KMW, but no success so far. Stuck at step .7 of https://github.com/sillsdev/keyman/wiki/How-to-set-up-a-local-web-server-for-the-Keyman-web-pages. Physical path specified cannot be found. When trying `...\keyman\keymanweb' this pops up:
image

@MakaraSok
Copy link
Collaborator

I think I got the keymanweb install using the instructions here https://github.com/sillsdev/keyman/wiki/How-to-set-up-a-local-web-server-for-the-Keyman-web-pages.

Here is what I see now:
image

Can you guide me further on what to do next to test this PR?

@jahorton
Copy link
Contributor Author

jahorton commented Jul 5, 2021

I think I got the keymanweb install using the instructions here https://github.com/sillsdev/keyman/wiki/How-to-set-up-a-local-web-server-for-the-Keyman-web-pages.

Here is what I see now:
image

Can you guide me further on what to do next to test this PR?

  • View KeymanWeb manual test pages

Screen Shot 2021-07-05 at 11 40 01 AM

  • Test unminified Keymanweb

That'll get you to the standard, "unminified" test page I suggested in the testing notes.

Assuming you're using either Chrome or Firefox, be sure to use Developer mode to emulate a touch device. Though, I think you're familiar with what that entails.

@MakaraSok
Copy link
Collaborator

'in-browser' KMW on iPhone X / iPad Pro / Pixel 2 - sil_cameroon_qwerty

Finally, some other patterns to test with this keyboard: use a longpress gesture on shift's W, but 'cancel' it by moving your finger back to the main key, then release. It should result in a standard keypress for that key.

On iPhone X and Pixel 2, when cancelling by moving your finger back on either the default or shift layer, the output is not that of the standard keypress, but that of the subkey, i.e.

  • press and hold w on the default layer, move your finger to the subkey then move back to the main key to see "ẅ" output.
  • press and hold W on the shift layer, move your finger to the subkey then move back to the main key to see "Ẅ" output.
  • press and hold ) on the symbol layer, move your finger to the subkey then move back to the main key to see "]" output -- the first on the list of the two subkeys on the this main key.

It is also noticed that the main key is not highlighted when the finger is moved back on it.

On iPad, things are working as expected in this regard.

'in-browser' KMW on iPhone X / iPad Pro / Pixel 2 - basic_khmr

basic_khmr cannot be loaded on any of the three emulated devices. Khmer Basic keyboard is not on the globe key. Here is what happens when an attempt has been made to add this keyboard.

iPhone X
image

iPad Pro
image

Pixel 2
image

'in-browser' KMW on iPhone X / iPad Pro / Pixel 2 - sil_euro_latin

Press and hold on s and then choose "ŝ" from the list of the subkeys, then press e twice. The expected prediction are "seem, seems" and the like, but the current prediction gives "week, weeks" instead. Pressing a space after it does not autocorrect either even though the correction option is currently enable. See the screenshot below.

Keys: ŝee, expected to be correct to "see" or something alike.
image

Keys: seė, expected to be corrected to "see" or something alike.
image

No other misbehavior noticed.

@mcdurdin mcdurdin changed the title refactor(web): longpresses, groundwork for additional gestures refactor(web): longpresses, groundwork for additional gestures 🕷 Jul 6, 2021
@jahorton
Copy link
Contributor Author

jahorton commented Jul 6, 2021

Dang it... I put the wrong ID: it's basic_kbdkhmr, not basic_khmr.

@mcdurdin mcdurdin changed the title refactor(web): longpresses, groundwork for additional gestures 🕷 refactor(web): longpresses, groundwork for additional gestures 🍕 Jul 6, 2021
@MakaraSok
Copy link
Collaborator

'in-browser' KMW on iPhone X / iPad Pro / Pixel 2 - basic_kbdkhmr

Finally, one last bit of testing with this keyboard: use a longpress gesture on shift, but 'cancel' it by moving your finger back to the main key, then release. It should layer-shift to the "shift" layer.

On iPad, this works as expected. The keyboard does switch to the shift layer after moving the finger back to the main and release.

On iPhone and Android, however, the keyboard switch to LCtrl layer instead of the expected shift layer.

@MakaraSok
Copy link
Collaborator

It seems like the keyboard is not working when set as a system keyboard.

Simulator Screen Shot - iPhone 11 Pro - 2021-07-01 at 17 06 12

I've looked at this again and they keyboard has properly been set as a system-wide keyboard, but when using to use it in Message the keyboard doesn't appear.
image

@jahorton
Copy link
Contributor Author

jahorton commented Jul 6, 2021

It seems like the keyboard is not working when set as a system keyboard.
[screenshot]

I've looked at this again and they keyboard has properly been set as a system-wide keyboard, but when using to use it in Message the keyboard doesn't appear.
[screenshot]

Have you tried loading it via the globe key yet? iOS doesn't auto-swap out the default keyboard - that'll be different than how (I think) Android does it. Via a longpress on the default system keyboard:

Image from iOS (2)

Select Keyman to get it loaded.


As for your other recent messages, we need to chat through other channels. I can't reproduce any of the errors in your most recent postings.

@jahorton
Copy link
Contributor Author

jahorton commented Jul 6, 2021

Looks like much of that was, indeed, a classic scenario of the old version arising. Not sure if it was because of Chrome caching or an old build being used by accident.

There were three things that we noted that are worth addressing in some form:

  1. When subkeys appear, if the user doesn't move their finger at all, no key is emitted. If they move their finger even a pixel, the key may emit. Not exactly ideal; we just need to decide what the behavior should be and enforce that.

  2. If the base key were reselected while subkeys were active, then deselected by moving the touch over base keys, not subkeys, the highlighting would be sticky afterward. This one's a clear bug and is fixed by the latest commit.

  3. Somehow, for his build in iOS, the build wasn't sending the most current version of the embedded KMW file to the iOS build, which was causing sticky highlighting in more general situations. Super-weird.

@jahorton
Copy link
Contributor Author

jahorton commented Jul 6, 2021

And... may as well go ahead and (for non-embedded cases) auto-select & highlight the base key until the user moves their finger, triggering initial subkey selection. No reason to maintain such an arbitrary condition of "if not moved, don't emit, but if moved even one pixel, emit.)

@MakaraSok
Copy link
Collaborator

After a remote session with Joshua, it turns out that the tests done on KMW was not prep thoroughly.

./build.sh should have been run from the terminal in /keyman/web/source folder.

On iPhone X and Pixel 2, when cancelling by moving your finger back on either the default or shift layer, the output is not that of the standard keypress, but that of the subkey, i.e.

press and hold w on the default layer, move your finger to the subkey then move back to the main key to see "ẅ" output.
press and hold W on the shift layer, move your finger to the subkey then move back to the main key to see "Ẅ" output.
press and hold ) on the symbol layer, move your finger to the subkey then move back to the main key to see "]" output -- the first on the list of the two subkeys on the this main key.

These are no longer an issue. Trigger a longpress gesture on shift's W, but 'cancel' it by moving your finger back to the main key, then release. It results in a standard keypress for that key. The key is also nicely highlighted to notify what is being pressed and triggered the output.

These issues here #5387 (comment) and #5387 (comment) have also been quickly fixed on the fly. No highlighted key will be left after being released on Android/iOS.

This issue #5387 (comment) was purely technical as there was a step which can be done to show the OSK keyboard from the I/O menu on the Simulator. This is not Keyman's issue.

@mcdurdin
Copy link
Member

mcdurdin commented Jul 7, 2021

@MakaraSok @jahorton What is the current status of testing on this PR? Have all tests passed to your satisfaction?

@MakaraSok
Copy link
Collaborator

Indeed.

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jahorton jahorton merged commit 75e47c1 into master Jul 8, 2021
@jahorton jahorton deleted the refactor/web/osk-centralized-gestures branch July 8, 2021 01:18
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 15.0.83-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants