search

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): delayed OSK initialization 🍕 #5412

Closed jahorton closed 2 years ago

jahorton commented 2 years ago

After some experimentation, I've cleaned things up enough to delay initialization of the keyman.osk field until KeymanWeb is fully ready to initialize (so, within keyman.init()). This did require some mild tinkering within the various desktop-UI modules for compatibility, but it was fortunately quite light for all four of them.

Delaying until KeymanWeb is actively initializing will allow us to simplify OSK initialization and to configure it, at construction time, based on the detected device and any specified KMW initialization options.

Breaking changes:

As keyman.osk will be null until keyman.init()'s Promise resolves, any calls requiring keyman.osk should wait on the initialization Promise.

User Testing

@keymanapp/testers

Instructions

As this PR is centered around how the OSK is constructed within KMW, simply make sure that the keyboard loads as expected on each different device type.

There's nothing particular to specific types of keyboards for this one, so just pick one or two. Test a few keystrokes for good measure; I don't expect any collateral effects here, but it's good to be sure.

Desktop instructions The important part: pay extra attention to in-browser desktop form-factor tests, as those are the most likely to break. Make sure the UI modules load properly and allow keyboard-swapping as expected. There is a new set of test pages designed to facilitate testing the UI modules: image The pages may be found through this link on the testing index page: image In order to test compatibility with the different UI module types, you'll need to mildly edit the unminified test page's source: Here's the UI you should expect to see for each test: - Button UI ![image](https://user-images.githubusercontent.com/25213402/126131066-fa59a6f1-5365-42f2-9cae-64c02e9e4e2b.png) - Note: the button will likely display above the first input element, rather than beside the current text input receiver. This is fine. - Float UI ![image](https://user-images.githubusercontent.com/25213402/126131199-ce14429f-5ae4-4316-9f0c-af640fbd0003.png) - Toggle UI ![image](https://user-images.githubusercontent.com/25213402/126131230-9e2409ac-010e-4a2e-9bc3-7afa6d14b80b.png) - Toolbar UI ![image](https://user-images.githubusercontent.com/25213402/126131258-64f6e98c-566a-479c-be0c-6651a41015b8.png)
Mobile instructions As the OSK will be loading at a later time than it used to, we need to ensure that this doesn't cause obvious blank-keyboard bugs. It would be wise to ensure that the keyboard loads correctly and consistently in both portrait and landscape orientations (issue #5252 not withstanding) within the apps and as system keyboard.

Explicit test list

Run this test suite against the following device/browser pairs:

- Using `web/testing/desktop-ui` and its subpages:
    - `DESKTOP.LOAD.BUTTON`
        - [ ] `.1`:  Confirm that KMW loads properly and displays the Button UI (`kmwuibutton.ts`).
            ![image](https://user-images.githubusercontent.com/25213402/126131066-fa59a6f1-5365-42f2-9cae-64c02e9e4e2b.png)
        - [ ] `.2`:  Use the Button UI to select `sil_euro_latin` and confirm the keyboard swaps properly.
    - `DESKTOP.LOAD.FLOAT`
        - [ ] `.1`:  Confirm that KMW loads properly and displays the Float UI (`kmwuifloat.ts`) when the OSK is visible.
            ![image](https://user-images.githubusercontent.com/25213402/126131199-ce14429f-5ae4-4316-9f0c-af640fbd0003.png)
        - [ ] `.2`:  Use the Float UI to select `sil_euro_latin` and confirm the keyboard swaps properly.
    - `DESKTOP.LOAD.TOGGLE`
        - [ ] `.1`:  Confirm that KMW loads properly and displays the Toggle UI (`kmwuitoggle.ts`) when the OSK is visible.
            ![image](https://user-images.githubusercontent.com/25213402/126131230-9e2409ac-010e-4a2e-9bc3-7afa6d14b80b.png)
        - [ ] `.2`:  Use the Toggle UI to select `sil_euro_latin` and confirm the keyboard swaps properly.
    - `DESKTOP.LOAD.TOOLBAR`
        - [ ] `.1`:  Confirm that KMW loads properly and displays the Toolbar UI (`kmwuitoolbar.ts`).
            ![image](https://user-images.githubusercontent.com/25213402/126131066-fa59a6f1-5365-42f2-9cae-64c02e9e4e2b.png)
        - [ ] `.2`:  Use the Toolbar UI to select `sil_euro_latin` and confirm the keyboard swaps properly.
            - Most easily done through the "Europe" region, then the "English" language.

Run this test suite against the following embedding-app/keyboard-mode pairs:

Per-platform suite and template (mobile form-factor, in-app) ``` - `MOBILE.PORTRAIT`: - [ ] `.1`: Verify that the keyboard loads correctly on the first attempt. - [ ] `.2`: Swap to a different keyboard and verify that it, also, works correctly. - `MOBILE.LANDSCAPE`: - [ ] `.1`: Verify that the keyboard loads correctly on the first attempt. - [ ] `.2`: Swap to a different keyboard and verify that it, also, works correctly. - `MOBILE.APP.SETTINGS`: - [ ] Enter the settings menu and install a new keyboard, then return to the main app. Verify that the new keyboard has loaded properly. ```

Finally, run this test suite for the Android system keyboard:

Mobile form-factor, system keyboard test suite and template ``` - `MOBILE.PORTRAIT`: - [ ] `.1`: Verify that the keyboard loads correctly on the first attempt. - [ ] `.2`: Swap to a different keyboard and verify that it, also, works correctly. - `MOBILE.LANDSCAPE`: - [ ] `.1`: Verify that the keyboard loads correctly on the first attempt. - [ ] `.2`: Swap to a different keyboard and verify that it, also, works correctly. ``` It's mostly the same as the previous suite, just without the in-app-only test.
mcdurdin commented 2 years ago

This did require some mild tinkering within the various desktop-UI modules for compatibility, but it was fortunately quite light for all four of them.

This may be a breaking change for other users who have created their own modules? Can you document any potential breakage?

MakaraSok commented 2 years ago

I need a step by step walk through to properly test this PR.

For the time being, I've checked this branch out and test using this http://localhost/keymanweb/testing/unminified.html. image

No issue stood out to me when loading any of the Cameroon keyboard.

jahorton commented 2 years ago

I need a step by step walk through to properly test this PR.

For the time being, I've checked this branch out and test using this http://localhost/keymanweb/testing/unminified.html. image

No issue stood out to me when loading any of the Cameroon keyboard.

This is the most important part from above:

As this PR is centered around how the OSK is constructed within KMW, simply make sure that the keyboard loads as expected on each different device type.

We want to be sure that KMW and the mobile apps load the keyboard properly after these changes. We're concerned with "keyboard startup" far more than keystroke sequences for this one.

MakaraSok commented 2 years ago

Tested on Emulator Pixel_2_API_29 (Android 10).

There is an error right after the installation of the apk finished.

Screenshot_1625735688

Screenshot_1625735534

jahorton commented 2 years ago

I couldn't get a repro at first, but then I swapped keyboards - that's when I got a repro of what you saw. Looks like I accidentally left a loose end (internal log: this.getHeight is undefined), so the fix was pretty simple - especially since I could tell the keyboard actually did successfully load otherwise.

Rotating the device would also have triggered the affected method.

mcdurdin commented 2 years ago

@keymanapp/testers please re-test 😁

mcdurdin commented 2 years ago

Simply swap out the filename at the line above within unminified.html and reload in order to test a different UI module.

We need to avoid requiring testers to make changes to source in order to complete the test. (It's bad enough that we are in the position where they have to do builds themselves in so many scenarios.)

jahorton commented 2 years ago

We need to avoid requiring testers to make changes to source in order to complete the test. (It's bad enough that we are in the position where they have to do builds themselves in so many scenarios.)

If the test pages had access to PHP, I'd swap this out in a heartbeat - it'd be really easy to configure with a URL parameter. I'd really like to avoid making this tidbit WETter than it needs to be.

I think there may be a way to make it work... but it'll takesome brainstorming around the constraints to find a path forward. The main issue is that I don't see an easy way to force KMW to pause its init until a dynamically-loaded script (for the UI module) is loaded due to the auto-init within kmwinit.ts.

mcdurdin commented 2 years ago

If the test pages had access to PHP, I'd swap this out in a heartbeat - it'd be really easy to configure with a URL parameter. I'd really like to avoid making this tidbit WETter than it needs to be.

There's a significant setup cost to adding PHP for future devs which I'd like to avoid. It's bad enough that the tests need to run on local http rather than filesystem (but we could have a script spin up a temp node.js instance for manual testing, which would be worth considering...?)

Here, I reckon just do the four test pages. It also makes it easier in the future for pointing to specific tests if we have plain html pages. Sometimes duplication is okay. 😉 It's a bit like the move from PHP to markdown in the documentation: we make the content a bit WETter, but we open up editing for less-PHP-aware devs, and it makes possible tool-assisted changes which are otherwise impossible.

I think there may be a way to make it work... but it'll takesome brainstorming around the constraints to find a path forward. The main issue is that I don't see an easy way to force KMW to pause its init until a dynamically-loaded script (for the UI module) is loaded due to the auto-init within kmwinit.ts.

I'd prefer static loading for the test pages in any case -- reducing the complexity of what we are testing in this place. If we want to test dynamic loading of UI, it should really be a separate test harness.

jahorton commented 2 years ago

Latest two commits: those split-out test pages:

image
MakaraSok commented 2 years ago

Is this ready for testing?

jahorton commented 2 years ago

It is now, though please note that this PR technically comes after #5430. (I just merged in its updates here, which should fix the Android issue noted above.)

MakaraSok commented 2 years ago

Explicit test list - RESULTS

Click on the arrow to see the details of each test.

Windows 10 Pro 21H1 / Chrome Version 92.0.4515.107 (x64)

Per-platform suite and template (desktop form-factor) - Using `~/keymanweb/testing/desktop-ui/` and click on the corresponding UI module to test: - `DESKTOP.LOAD.BUTTON` - **Button UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Button UI - [x] `.2`: **PASSED** - A `sil_euro_latin` keyboard can be selected from the list, _but it is a pain to scroll and eyeball for a language and/or keyboard with the name, EuroLatin (SIL). The sorting order of the list is not predictable. Is it sorted by the keyboard name or language or keyboard id?_ - `DESKTOP.LOAD.FLOAT` - **Float UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Float UI when the OSK is visible, _but the text in the dropdown menu is not nicely aligned._ ![image](https://user-images.githubusercontent.com/28331388/126429192-ef17419a-30fd-406d-9c37-f76fa41bd4d8.png) - [x] `.2`: **PASSED** - The keyboard swaps properly, but a little pain to scroll and find the one. - `DESKTOP.LOAD.TOGGLE` - **Toggle UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toggle UI when the OSK is visible. - [x] `.2`: **PASSED** - confirm the keyboard swaps properly to a EuroLatin (SIL) keyboard, _but when pressing on the same OSK key twice the OSK disappears._ - `DESKTOP.LOAD.TOOLBAR` - **Toolbar UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toolbar UI. - [x] `.2`: **PASSED** - the keyboard swaps properly.

Windows 10 Pro 21H1/ Firefox 90.0.1 (x64)

Per-platform suite and template (desktop form-factor) - Using `~/keymanweb/testing/desktop-ui/` and click on the corresponding UI module to test: - `DESKTOP.LOAD.BUTTON` - **Button UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Button UI. - [ ] `.2`: **FAILED** - The keyboard does not swaps properly in the first attempt. The default keyboard is shown instead: image - `DESKTOP.LOAD.FLOAT` - **Float UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Float UI when the OSK is visible. - [x] `.2`: **PASSED** - The keyboard swaps properly. - `DESKTOP.LOAD.TOGGLE` - **Toggle UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toggle UI when the OSK is visible. - [x] `.2`: **PASSED** - The keyboard swaps properly, _but pressing on the same OSK key twice makes the OSK disappear_. - `DESKTOP.LOAD.TOOLBAR` - **Toolbar UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toolbar UI - [x] `.2`: **PASSED** - The keyboard swaps properly.

iOS 14.5, iPhone 11 / in-app

Per-platform suite and template (mobile form-factor, in-app) - **`MOBILE.PORTRAIT`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [ ] `.2`: **FAILED** - Cannot swap to a different keyboard. After installing a new keyboard (Bunong (SIL)), the keyboard stays in Bunong (SIL) when trying to swap to EuroLatin (SIL). Note that the Predictive text is shown in EuroLatin (SIL), while the keyboard is Bunong (SIL). Each keypress outputs Bunong characters. image - **`MOBILE.LANDSCAPE`:** - [x] `.1`: **PASSED** - The keyboard loads on the first attempt, _but it looks weird -- it might be by design_. image - [ ] `.2`: **FAILED** - Cannot swap to a different keyboard. After installing a new keyboard (Bunong (SIL)), the keyboard stays in Bunong (SIL) when trying to swap to EuroLatin (SIL). Note that the Predictive text is shown in EuroLatin (SIL), while the keyboard is Bunong (SIL). Each keypress outputs Bunong characters. image - **`MOBILE.APP.SETTINGS`:** - [x] **PASSED** - Enter the settings menu and install a new keyboard, then return to the main app. The new keyboard has loaded, _but the keyboard cannot be swapped to any other. It stays with the newly installed one_ .

Android 10, Pixel 2 / in-app

Per-platform suite and template (mobile form-factor, in-app) - **`MOBILE.PORTRAIT`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [x] `.2`: **PASSED** - Swap to a different keyboard correctly. - **`MOBILE.LANDSCAPE`:** - [x] `.1`: **FAILED** - The keyboard loads with a black bar above the keyboard. image - [x] `.2`: **PASSED** - Swap to a different keyboard and it loads and works correctly. image - **`MOBILE.APP.SETTINGS`:** - [x] **PASSED** - Install a new keyboard from settings, then return to the main app. The new keyboard has loaded properly.

Android 10, Pixel 2 / system keyboard

Mobile form-factor, system keyboard test suite and template - **`MOBILE.PORTRAIT`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [x] `.2`: **PASSED** - Swap to a different keyboard and it works correctly. _However, the loading process (not a new issue) bothers me a bit. It was not smooth a process as switching between keyboard in Gboard/Samsung Keyboard._ - **`MOBILE.LANDSCAPE`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [ ] `.2`: **FAILED** - Swap to a different keyboard and the keyboard disappears. Tap in the text input area to bring the selected keyboard up.
jahorton commented 2 years ago

Oh, I completely forgot to disable the extra keyboards on the non-Toolbar UI test pages. That probably did not exactly help with the tests.

(iOS in-app) FAILED - Cannot swap to a different keyboard. After installing a new keyboard (Bunong (SIL)), the keyboard stays in Bunong (SIL) when trying to swap to EuroLatin (SIL). Note that the Predictive text is shown in EuroLatin (SIL), while the keyboard is Bunong (SIL). Each keypress outputs Bunong characters.

You know, I thought I noticed something weird in that direction earlier when investigating an unrelated user issue. Looks like this is actually a break on master - so I should probably divert toward that!

I believe that the noted Android issues are pre-existing as well.

jahorton commented 2 years ago

And, I've figured out what happened to iOS. Fix for that tidbit's up at #5475; will merge through once that lands.

In the back of my mind, I did think it odd that I never noticed it while developing this PR chain. It was originally based on a version of master before the bug was introduced, so the bug only showed with today's master merge.

MakaraSok commented 2 years ago

Let me know if you'd like me to retest the iOS / in-app.

jahorton commented 2 years ago

Now that its base (#5430) has passed, we should be good for another round of testing here.

MakaraSok commented 2 years ago

Explicit test list - RESULTS - ROUND 2

Click on the arrow to see the details of each test.

Windows 10 Pro 21H1 / Chrome Version 92.0.4515.107 (x64)

Per-platform suite and template (desktop form-factor) - Using `~/keymanweb/testing/desktop-ui/` and click on the corresponding UI module to test: - `DESKTOP.LOAD.BUTTON` - **Button UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Button UI. - [x] `.2`: **PASSED** - A `sil_euro_latin` keyboard can be selected from the list and the keyboard swaps properly. - `DESKTOP.LOAD.FLOAT` - **Float UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Float UI when the OSK is visible. - [x] `.2`: **PASSED** - The keyboard swaps properly. - `DESKTOP.LOAD.TOGGLE` - **Toggle UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toggle UI when the OSK is visible. - [x] `.2`: **PASSED** - confirm the keyboard swaps properly to a EuroLatin (SIL) keyboard, _but when pressing on the same OSK key twice the OSK disappears._ - `DESKTOP.LOAD.TOOLBAR` - **Toolbar UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toolbar UI. - [x] `.2`: **PASSED** - the keyboard swaps properly.

Windows 10 Pro 21H1/ Firefox 90.0.1 (x64)

Per-platform suite and template (desktop form-factor) - Using `~/keymanweb/testing/desktop-ui/` and click on the corresponding UI module to test: - `DESKTOP.LOAD.BUTTON` - **Button UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Button UI. - [x] `.2`: **PASSED** - The keyboard swaps properly. - `DESKTOP.LOAD.FLOAT` - **Float UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Float UI when the OSK is visible. - [x] `.2`: **PASSED** - The keyboard swaps properly. - `DESKTOP.LOAD.TOGGLE` - **Toggle UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toggle UI when the OSK is visible. - [x] `.2`: **PASSED** - The keyboard swaps properly to a EuroLatin (SIL) keyboard, _but pressing on the same OSK key twice makes the OSK disappear_. - `DESKTOP.LOAD.TOOLBAR` - **Toolbar UI** - [x] `.1`: **PASSED** - KMW loads properly and displays the Toolbar UI - [x] `.2`: **PASSED** - The keyboard swaps properly.

iOS 14.5, iPhone 11 / in-app

Per-platform suite and template (mobile form-factor, in-app) - **`MOBILE.PORTRAIT`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [x] `.2`: **PASSED** - Swap to a different keyboard correctly. - **`MOBILE.LANDSCAPE`:** - [x] `.1`: **PASSED** - The keyboard loads on the first attempt, _but it looks weird -- it might be by design_. image - [x] `.2`: **PASSED** - Swap to a different keyboard correctly. - **`MOBILE.APP.SETTINGS`:** - [x] **PASSED** - Enter the settings menu and install a new keyboard, then return to the main app. The new keyboard has loaded and worked correctly.

Android 10, Pixel 2 / in-app

Per-platform suite and template (mobile form-factor, in-app) - **`MOBILE.PORTRAIT`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [x] `.2`: **PASSED** - Swap to a different keyboard correctly. - **`MOBILE.LANDSCAPE`:** - [x] `.1`: **FAILED** - The keyboard loads with a black bar above the keyboard. This only happens on phone emulator (running Android 10), but not on the tablet one (running Android 5.1.1) Landscape without predictive text: image Landscape with predictive text: image - [x] `.2`: **PASSED** - Swap to a different keyboard and it loads and works correctly in landscape mode, _but when turning back to portrait mode, the keyboard is one row shorter than usual._ Turn to portrait while on `khmer_angkor`: image Turn to portrait while on `sil_euro_latin`: image - **`MOBILE.APP.SETTINGS`:** - [x] **PASSED** - Install a new keyboard from settings, then return to the main app. The new keyboard has loaded properly.

Android 10, Pixel 2 / system keyboard

Mobile form-factor, system keyboard test suite and template - **`MOBILE.PORTRAIT`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [x] `.2`: **FAILED** - when swapping to a different keyboard the keyboard disappear right after a different keyboard is selected. Only when the text input field is tapped on that the selected keyboard appears and work correctly. This behavior happens on Chrome and Messaging apps, but not when testing in Google omnibar. - **`MOBILE.LANDSCAPE`:** - [x] `.1`: **PASSED** - The keyboard loads correctly on the first attempt. - [x] `.2`: **FAILED** - Swap to a different keyboard and the keyboard disappears. Tap in the text input area to bring the selected keyboard up.
jahorton commented 2 years ago

The Android issues noted in your results above are, I believe, noted pre-existing issues. So... while they're certainly not ideal, they aren't exactly failures for this PR.

For the Android in-app black-bar "fail": see #5252. While I don't believe that I see a corresponding issue for the other issue... it's reproducible on master. Not sure when that started, but it's been around for a little while now. On master, the system keyboard doesn't display in my tests after a keyboard swap until I select something that can receive input.

Given these details... everything we can expect to pass did pass; the remaining issues aren't the fault of this PR.

keyman-server commented 2 years ago

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

keyman-server commented 2 years ago

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

keyman-server commented 2 years ago

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