keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

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

Closed jahorton closed 2 years ago

jahorton commented 2 years ago

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:

https://github.com/keymanapp/keyman/blob/6034f252e70c2218a1983059324f32d2bdf15daa/web/source/osk/pendingGesture.interface.ts#L2-L8

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

https://github.com/keymanapp/keyman/blob/6034f252e70c2218a1983059324f32d2bdf15daa/web/source/osk/realizedGesture.interface.ts#L2-L9

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.

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:

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:

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.

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:

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.

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.)

MakaraSok commented 2 years ago

On Android 10 Emulator (Pixel 2 API 29), this build was tested:

1. sil_cameroon_qwerty

  • Do a similar test for each, but move your finger away from the base key, then release. Nothing should be emitted.

Up to this point, a fatal error occurs right after rolling the finger away from the subkey to any direction (avoiding highlighting the base key). Press and hold on the base with a subkey and then roll the finger away (either to the left/right or down) also triggers the error. This behavior is experienced across layers and keyboards.

Press and hold "w" key and roll the finger away to see this error: Screenshot_1625023017

1. basic_khmr Move your finger away from the shift key before releasing to see this error: Screenshot_1625025373

3. sil_euro_latin No issue is observed when testing the subkey on this keyboard. The predictive text still suggest relevant words by ignoring the accent/diacritical marks.



jahorton commented 2 years ago

Re: reported Android error...

Thanks for catching that. I've gone ahead and pushed up a fix for it; it was just missing a null guard. That area was hit kind of badly by some Git shenanigans I did during development, so it looks like I missed that detail at the time.

Glad to see things were otherwise solid within Android.

jahorton commented 2 years ago

We definitely don't expect an error to occur, as I'd assert we have full control over the element hierarchy, etc. But, that is an assertion, and an assumption that we'd never make a mistake during maintenance or further implementation.

Those kinds of errors should be reported to Sentry. That's what Sentry is for.

Keep in mind that the KeymanWeb engine itself doesn't have any Sentry integration. So far, we're relying near-completely on console.error statements for that.

We may be able to integrate @sentry/minimal to eliminate this problem, but I'll have to make sure it doesn't require... well... require or import statements. Either way, that's also a separate issue.

mcdurdin commented 2 years ago

Keep in mind that the KeymanWeb engine itself doesn't have any Sentry integration. So far, we're relying near-completely on console.error statements for that.

We may be able to integrate @sentry/minimal to eliminate this problem, but I'll have to make sure it doesn't require... well... require or import statements. Either way, that's also a separate issue.

Error reporting through to Sentry is a job for the library consumer (or final app, in the Keyman Engine for Web embedded case). That's a general principle for all our projects (and I think we might need to tweak some libraries where we've put this on the wrong side of the boundary, thinking KMEA; see #5396).

keymanweb.com has sentry reporting for errors. As do the apps. And Keyman Developer's web test. So that covers our usage pretty well.

jahorton commented 2 years ago

Since I can't find the spot to comment on the original thread...

Secondly, when embedded in Android, failing to catch the error would trigger the Keyman app to say the keyboard had a "fatal error" and thus would boot the user out of their active keyboard. This latter point is definitely not ideal.

If there is a behaviour in Keyman for Android that is not ideal, then we should be fixing that rather than building patches around it. (FWIW, I agree that that behaviour is not ideal and we should be trying to fail more gracefully.)

I've long agreed with that thought in parentheses. Of course, the issue is that right now, it is there, so for now, it must be worked around. I'll be perfectly happy to undo the workaround once the behavior's gone.

Though, if we are saying it'll be gone in short order, and we're willing to risk fatal-error resets in the meantime, I'm fine dropping it now.

jahorton commented 2 years ago

Re: @sentry/minimal - this would allow KMW to "assist" the apps and keymanweb.com with enhanced breadcrumbs and event tagging.

It doesn't add Sentry handling to our code. It adds the ability to integrate with any that might be provided by the library consumer with extra events and breadcrumbing, passively going silent if the library consumer doesn't provide Sentry.

I kind of opted for a similar scheme with the iOS application, but I think I may have slightly missed the mark there - the SDK bit is set within the KeymanEngine's code, not from a parameter by the host app. I'm pretty sure it's otherwise closer than KMEA, though.

mcdurdin commented 2 years ago

Though, if we are saying it'll be gone in short order, and we're willing to risk fatal-error resets in the meantime, I'm fine dropping it now.

Definitely. This is alpha. Errors are to be expected. If anything, it helps us to prioritise the change for stable.

MakaraSok commented 2 years ago

On iOS 14.5 emulator, this was built in Xcode and tested.

1. 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.

This turns out that nothing is output when rolling the finger back down on the base key.

  • 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.)

This does output the character on the base key in iOS, but it causes an error (and nothing is output) on Android when press and hold then release without moving your finger.

Screenshot_1625044755

2. basic_khmr

  • 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.

This does not happen as expected on iOS. The keyboard does not switch its layer to shift when moving the finger back on the base key.

3. sil_euro_latin

The predictive text is working as normal ignoring the accent/diacritical marks.

mcdurdin commented 2 years ago

I'll wait until you have followed-up on @MakaraSok's test results before I re-review?

jahorton commented 2 years ago

Already followed-up. I'd missed a ! when refactoring out one of the redundant found variables noted earlier in the review.

mcdurdin commented 2 years ago

Already followed-up

So ready for re-review then? Given the follow-up was posted 30 minutes after I posted my query, I would like to suggest that 'already' might be a bit strong 🤣

jahorton commented 2 years ago

Already followed-up

So ready for re-review then? Given the follow-up was posted 30 minutes after I posted my query, I would like to suggest that 'already' might be a bit strong 🤣

I mean, if you want to wait for a re-test from Makara, be my guest. Both noted issues were very simple and straightforward once I looked at the right spot.

MakaraSok commented 2 years ago

I'll do another round of test on Android and iOS this afternoon.

@jahorton, if you have time later today, could you help me with setting up KMW for the test?

MakaraSok commented 2 years ago

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.

The standard keypress is triggered and output on iOS. And, there is a weird behavior after releasing the key -- the base key on the default layer is highlighted blue. On Android, however, nothing is output after the key is released and the base key is not highlighted blue either.

image

This behavior does not only happen on the default layer, but shift layer which does not automatically switch to default after the release of the key.

image


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.)

This is true in iOS, but not in Android. Nothing would emit after releasing a longpress without moving your finger.

MakaraSok commented 2 years ago

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

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

mcdurdin commented 2 years ago

If that's a simulator, check the menus -- there's an option to show/hide the touch keyboard.

On Thu, 1 Jul 2021, 8:08 pm Makara, @.***> wrote:

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

[image: Simulator Screen Shot - iPhone 11 Pro - 2021-07-01 at 17 06 12] https://user-images.githubusercontent.com/28331388/124106874-dba27380-da8e-11eb-9b1d-737d1f714a43.png

[image: Simulator Screen Shot - iPhone 11 Pro - 2021-07-01 at 17 06 53] https://user-images.githubusercontent.com/28331388/124106904-e3621800-da8e-11eb-896a-52d92baceb14.png

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keymanapp/keyman/pull/5387#issuecomment-872109709, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCKHPNIJO5CQZZAQKNSKC3TVQ5BLANCNFSM47PDLFGA .

MakaraSok commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?

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

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 commented 2 years ago

'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.

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.

jahorton commented 2 years ago

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

MakaraSok commented 2 years ago

'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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/keymanapp/keyman/pull/5387#issuecomment-872092726 and https://github.com/keymanapp/keyman/pull/5387#issuecomment-874415848 have also been quickly fixed on the fly. No highlighted key will be left after being released on Android/iOS.

This issue https://github.com/keymanapp/keyman/pull/5387#issuecomment-874418935 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 commented 2 years ago

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

MakaraSok commented 2 years ago

Indeed.

keyman-server commented 2 years ago

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