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.

Sign up for GitHub

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

Jump to bottom

change(android): smoother keyboard initialization ✨ #10022

Merged
merged 3 commits into from
Dec 1, 2023
Merged

change(android): smoother keyboard initialization ✨ #10022

merged 3 commits into from
Dec 1, 2023

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Nov 17, 2023 •
edited
Loading

Continuing from #10017, this PR adds a few more tweaks to further smooth certain aspects of keyboard use.

One thing I happened upon while investigating the keyboard-load process:

Sixth:

(System keyboard) is active; the actual desired keyboard hasn't shown yet.

That tidbit above surfaces as a keyboard-initialization flash whenever the host-page is loaded - be it the first time or a reset. It's what happens when Web loads on touch without an available keyboard stub... corresponding to an old warning about how "no keyboard stubs exist". It seems that the flash has become slower / more prominent recently... possibly due to side effects from the Web modularization work?

One way to resolve that issue: make sure that a valid keyboard stub is available before the embedded version of KMW initializes. In essence, "get it right the first time." It turns out there were a few complications to resolve on the way, but they're nothing a little elbow grease can't fix.

So, for many of those complications... the best way forward involved replicating a lot of the keyboard-stub building code for use within the Keyboard class, unbound from the standard setKeyboard methods. This is obviously a bit WET to do, but unavoidable due to entangling side-effects in the originals. The new methods are better modularized; I'd advise refactoring the old methods to rely on the new methods at some point.

User Testing

TEST_SIMPLE_LOAD:

  1. Install the Keyman app test build from this PR.
  2. Open Keyman App.
  3. If any errors are reported during the loading process, FAIL this test.
  4. If you see a flash of some other keyboard that is swapped out before app loading completes, FAIL this test.
    • If it occurs, you might see "(System keyboard)" on the "temporary" keyboard's space bar.
    • That "temp" keyboard may have five rows instead of four, which would also likely be noticeable.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-missing User tests have not yet been defined for the PR labels Nov 17, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Nov 17, 2023 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ TEST_SIMPLE_LOAD (PASSED): Tested with the attached PR build (Keyman 17.0.212-alpha-test-10022) in an Android mobile (ver 13) and here is my observation: 1. Opened the Keyman App. 2. Confirmed that there were no errors while loading. 3. No other keyboards flashed on the screen before the app finished loading. (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S26 milestone Nov 17, 2023
mcdurdin
mcdurdin approved these changes Nov 19, 2023
View reviewed changes
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

@keymanapp-test-bot keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Nov 20, 2023
@bharanidharanj
Copy link

Test Results

  • TEST_SIMPLE_LOAD (PASSED): Tested with the attached PR build (Keyman 17.0.212-alpha-test-10022) in an Android mobile (ver 13) and here is my observation: 1. Opened the Keyman App. 2. Confirmed that there were no errors while loading. 3. No other keyboards flashed on the screen before the app finished loading.

..After loading the Keyman app

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 20, 2023
@bharanidharanj
Copy link

Tested the same in the Android 5.0 (API 21) emulator and here is my observation: 1. Opened the Keyman App. 2. Confirmed that there were no errors while loading. It seems to be working as expected.

jahorton
jahorton commented Nov 22, 2023
View reviewed changes
Copy link
Contributor Author

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

Got curious about if this approach is even possible these days for iOS as well; figured I'd document what I could find in that regard.

android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboardJSHandler.java
@@ -62,6 +64,21 @@ public int getKeyboardWidth() {
return kbWidth;
}

@JavascriptInterface
public String initialKeyboard() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we wanted to adopt something kinda similar for iOS:

https://developer.apple.com/documentation/webkit/wkscriptmessagehandlerwithreply

We could then reply with a stub string like the one this method builds, passing it through that Swift function's 'reply' callback for use and interpretation. (See the other comment in this batch.)

But... this is only available with iOS 14.0+.

android/KMEA/app/src/main/assets/android-host.js
@@ -31,6 +31,10 @@ function init() {
keyman.getOskHeight = getOskHeight;
keyman.getOskWidth = getOskWidth;
keyman.beepKeyboard = beepKeyboard;

// Readies the keyboard stub for instant loading during the init process.
KeymanWeb.registerStub(JSON.parse(jsInterface.initialKeyboard()));
Copy link
Contributor Author

@jahorton jahorton Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Uses the function defined within this PR within the Java KMKeyboardJSHandler class. See other comment (re https://developer.apple.com/documentation/webkit/wkscriptmessagehandlerwithreply) on how this could look for iOS if we wanted to go that route.

That said, I don't see the problem this PR addresses on my personal iOS device. So, adopting this code for use on iOS can probably be low-priority for now.

@jahorton jahorton mentioned this pull request Nov 22, 2023
Open
@darcywong00 darcywong00 modified the milestones: A17S26, A17S27 Nov 27, 2023
Base automatically changed from change/android/flishy-flashy-mitigation to master December 1, 2023 02:12
@jahorton jahorton merged commit 19d3d7c into master Dec 1, 2023
16 checks passed
@jahorton jahorton deleted the change/android/flishy-flashy-mitigation-2 branch December 1, 2023 02:12
@keyman-server
Copy link
Collaborator

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

@jahorton jahorton mentioned this pull request Dec 13, 2023
Merged
@jahorton jahorton mentioned this pull request Dec 22, 2023
Closed
@jahorton jahorton mentioned this pull request Jan 9, 2024
Closed
@darcywong00 darcywong00 mentioned this pull request Jan 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@ermshiperete ermshiperete Awaiting requested review from ermshiperete ermshiperete is a code owner

@darcywong00 darcywong00 Awaiting requested review from darcywong00 darcywong00 is a code owner

@sgschantz sgschantz Awaiting requested review from sgschantz sgschantz is a code owner

Assignees
No one assigned
Labels
android/engine/ android/ has-user-test web/
Projects
None yet
Milestone
A17S27
Development

Successfully merging this pull request may close these issues.
5 participants
@jahorton @bharanidharanj @keyman-server @mcdurdin @darcywong00