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): active element management in relation to OSK display control ✂️ #5644

Closed jahorton closed 2 years ago

jahorton commented 2 years ago

Addresses aspects of #5026.

Follows #5633.

Fixes #5026. (Finally!) This PR finally centralizes management of KMW's active element logic and fixes a few niche scenarios that went relatively undetected until now.

One of the worst aspects of "spaghetti code" in relation to the OSK is how entangled some of its display aspects have been with the web engine's active-element management. While this PR doesn't get everything cleaned up perfectly for active-element tracking, its biggest success is that all logic where the two subsystems intersect has now been centralized within the setters of two properties.

While not actually refactoring OSK code in this PR, it's prep work necessary to continue along the lines of the other ✂️ PRs.

User Testing

SUITE_IN_APP: Host app tests.

All behavior exhibited here should feel "normal". These tests are solely to ensure that nothing got accidentally broken.

Platform / keyboard mode combinations

Suite tests

SUITE_DOM: Site-based tests.

All behavior exhibited here should feel "normal". These tests are solely to ensure that nothing got accidentally broken.

OS / Browser combinations

For each of these, make use of the "Tests the new Attachment/Enablement API functionality" test page.

Suite tests

SUITE_CJK: Tests CJK keyboard functionality

OS / Browser combinations

Use the standard "Test unminified KeymanWeb" page.

Suite tests


So, fun fact... the TEST_JAPANESE_FOCUS test above actually fixes an issue that currently exists both on master and on stable_14.0 that had gone unnoticed until this PR's development.

keymanapp-test-bot[bot] commented 2 years ago

User Test Results

✅ SUITE_IN_APP: Host app tests.

✅ SUITE_DOM: Site-based tests.

✅ SUITE_CJK: Tests CJK keyboard functionality

darcywong00 commented 2 years ago

Can probably reformat TEST_SPECIFIC_KEYBOARDS: so GitHub doesn't link the issue/pull request for #1 and #2.

MakaraSok commented 2 years ago

SUITE_IN_APP: Host app tests.

SUITE_DOM: Site-based tests.

SUITE_CJK: Tests CJK keyboard functionality

jahorton commented 2 years ago

@keymanapp/testers

The instructions provided are not applicable for this pair, as Keyman app does not have the Create Inputs button. This case is marked as BLOCKED for now. Kindly guide me through if the steps taken were not expected/intended.

They are applicable for these pairs - you're just referring to the wrong part of the instructions. The DOM tests explicitly say to use the test pages, outside of the apps. Just like you did for the Android one that's part of the same test suite.

MakaraSok commented 2 years ago

@keymanapp/testers

The instructions provided are not applicable for this pair, as Keyman app does not have the Create Inputs button. This case is marked as BLOCKED for now. Kindly guide me through if the steps taken were not expected/intended.

They are applicable for these pairs - you're just referring to the wrong part of the instructions. The DOM tests explicitly say to use the test pages, outside of the apps. Just like you did for the Android one that's part of the same test suite.

Please specify the browser necessary for this test. Should it be in Chrome or FireFox on Windows?

jahorton commented 2 years ago

Oh, I stand corrected for GROUP_WIN. CTRL+C / CTRL+V strikes again; I should have edited that.

I've now changed GROUP_WIN to actually specify Windows things, and the IOS one to specify using iOS Simulator's Safari. You should be able to visit the test pages through Simulator, though this may require logging into TeamCity within the Simulator instance.

MakaraSok commented 2 years ago

SUITE_DOM: Site-based tests.

jahorton commented 2 years ago
  • On Android 5.0.2, 6.0, 8.1, 10 and 11, there is a fatal error when typing a longpress key on any layer. After that nothing is output from any longpresses, however the normal keys on any layers are working fine after the crash.

Good catch there - it also would have affected hardware keystrokes, actually. Missed a few spots when changing how the last active element is retrieved.

jahorton commented 2 years ago

The mobile-device bugs (aside from spacebar captions) should now be ready for a retest. I'm also going to add notes for the on-device Safari testing: Safari likes to interfere with language-menu access a bit, as the same location on the screen will display its nav bar.

Spacebar captions are handled separately in #5688.

jahorton commented 2 years ago

Resetting some of the tests:

SUITE_IN_APP GROUP_ANDROID_APP

TEST_NORMAL_LOAD OPEN retest


SUITE_DOM OPEN retest

jahorton commented 2 years ago

Okay, so we can't do a full suite that way.

SUITE_DOM

GROUP_ANDROID_DOM OPEN retest GROUP_IOS_DOM OPEN retest

... which doesn't work yet either.

jahorton commented 2 years ago

Well, in that case...

SUITE_DOM

GROUP_WIN

TEST_NORMAL_USE OPEN retest TEST_ELEMENT_HOPPING OPEN retest TEST_SPECIFIC_KEYBOARDS OPEN retest

GROUP_ANDROID_DOM

TEST_NORMAL_USE OPEN retest TEST_ELEMENT_HOPPING OPEN retest TEST_SPECIFIC_KEYBOARDS OPEN retest

GROUP_IOS_DOM

TEST_NORMAL_USE OPEN retest TEST_ELEMENT_HOPPING OPEN retest TEST_SPECIFIC_KEYBOARDS OPEN retest

MakaraSok commented 2 years ago

SUITE_IN_APP: Host app tests.

jahorton commented 2 years ago
* **TEST_ROTATE_L-TO-P: FAILED** Fatal Error in the first attempt to load the keyboard in landscape mode on Android 6.0.
  <img alt="" width="70" src="https://user-images.githubusercontent.com/28331388/133953357-21b144ea-ddb5-49ff-8167-53fbb3f566df.png">
  _Note that the error doesn't occur again on the next attempt._
  There is also a Fatal Error when typing longpress on the default layer in horizontal mode in Android 8.1. After this error, no longpress outputs anything. Normal press works just fine though.

I loaded up an emulator with both versions and could reproduce neither issue mentioned here.

MakaraSok commented 2 years ago

SUITE_DOM: Site-based tests.

jahorton commented 2 years ago

OK, so that last failed user test opened a minor can of worms. There was a "fun" conflict between the focus timer and Android-specific page-scroll handling that simply wasn't ever resolved properly. I think I've figured out how to make the two aspects cooperate, though this did require a few more changes than I'd like. The previous two commits (which accomplished this) may be worth their own review.

As may be noted, I did take the opportunity this provided as an excuse to polish up this area dramatically, removing the old ugly (<any>this.keyman) blocks by providing "proper" definitions where they were previously missing. All changed field and method names were always file- and class-internal, despite where they were being stored. (Things would have been much messier due to the fixes had I not done this.)

jahorton commented 2 years ago

@keymanapp-test-bot retest SUITE_DOM TEST_NORMAL_USE


@mcdurdin Looks like that path wasn't quite covered right:

Result of the command above (Seen via drill-down) ![image](https://user-images.githubusercontent.com/25213402/134116190-5ffb7814-2448-4af8-84f7-f42c82967217.png)

Only the first group's test was reset.

So...

@keymanapp-test-bot retest SUITE_DOM GROUP_ANDROID_DOM TEST_NORMAL_USE @keymanapp-test-bot retest SUITE_DOM GROUP_IOS_DOM TEST_NORMAL_USE


Okay then...

SUITE_DOM GROUP_ANDROID_DOM TEST_NORMAL_USE OPEN retest

SUITE_DOM GROUP_IOS_DOM TEST_NORMAL_USE OPEN retest

jahorton commented 2 years ago

... maybe it needs an extra comment for this type?

SUITE_DOM GROUP_ANDROID_DOM TEST_NORMAL_USE OPEN retest

SUITE_DOM GROUP_IOS_DOM TEST_NORMAL_USE OPEN retest

Edit: there we go. Finally.

MakaraSok commented 2 years ago

SUITE_DOM: Site-based tests.

mcdurdin commented 2 years ago

@jahorton you are pushing that poor little test bot too hard. Currently:

  1. only one bot command allowed per comment.
  2. retest only matches first test, rather than all tests that could match a given string (except in case of 'all' which matches all tests in a suite, group or spec).

So, I expect you could do:

@--keymanapp-test-bot retest SUITE_DOM GROUP_ANDROID_DOM TEST_NORMAL_USE SUITE_DOM GROUP_IOS_DOM TEST_NORMAL_USE

(identifier deliberately mangled to prevent interference.)

Both of these issues should be resolved but as there is a way forward I'll just encourage you to use the recognised incantation for now -- no time this week...

mcdurdin commented 2 years ago
  • unless waiting for a second or so

I've also noted these performance issues. See #5729, #5731 for potential mitigations.

mcdurdin commented 2 years ago

The previous two commits (which accomplished this) may be worth their own review.

Okay, re-reviewing. I've got a few things in my queue so hopefully will get to that this morning.

jahorton commented 2 years ago

When testing on the emulator, the mouse click may be faster than the reality of when using the actual device. Consequently, the OSK may not behave as intended, unless waiting for a second or so before touching to show or hide it. The behavior on web in Windows and macOS is more robust when it comes to show and hide the OSK.

Actually, he's referring to something else here:

https://github.com/keymanapp/keyman/blob/3eda9dd3a10071fcf76a3b7ffab6280902358f14/web/source/dom/domEventHandlers.ts#L30-L36

When testing touch-device element attachment, KMW uses this timer to prevent de-focusing selected elements too quickly. I believe that it's been there since before we went open-source.

https://github.com/keymanapp/keyman/blob/a6289f4f0fcc99fb0a29eb8fc97cf0ddbbce458a/web/source/dom/domManager.ts#L1764-L1772

My best guess is that it's there to prevent fat-fingering effects from deselecting an element when the touchpoint is lifted, should the off-element part lift after the on-element part... but this is admittedly conjecture and inference on my part. The block above (and the second comment line) includes fixes to use of this timer, based on said guess.

I agree with @MakaraSok that the timer's probably a bit too long, but I didn't want to make that call in this PR.

jahorton commented 2 years ago

SUITE_IN_APP GROUP_ANDROID_APP TEST_ROTATE_L-TO-P OPEN retest

MakaraSok commented 2 years ago

SUITE_IN_APP: Host app tests.

SUITE_DOM: Site-based tests.

jahorton commented 2 years ago

Looks like that bug was addressed on master by https://github.com/keymanapp/keyman/pull/5724. It fits the bill near-perfectly, too.

@keymanapp-test-bot retest SUITE_IN_APP GROUP_ANDROID_APP TEST_ROTATE_L-TO-P retest

MakaraSok commented 2 years ago

SUITE_IN_APP: Host app tests.

keyman-server commented 2 years ago

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