-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
change(web): initializes OSK with keyboard if available during init 🪠 #11174
Conversation
User Test ResultsTest specification and instructions
Test Artifacts
|
da7ea66
to
c595303
Compare
This prevents unnecessary construction of a stand-in touch layout and related DOM initialization on touch devices when fully leveraged.
56f28bb
to
eebf3e5
Compare
Just noticed an issue with when the OSK is in desktop-keyboard mode; needs a bit more refinement first. |
Test Results
|
Sentry Issue: KEYMAN-WEB-JH From the iOS user-test failure |
@keymanapp-test-bot retest TEST_IOS_PHONE TEST_ANDROID_TABLET I've added code that should ensure the banner displays consistently now; the OSK now proactively checks what mode the banner should be in when it's initialized. (Previously, it always waited to be told.) |
Test Results
|
Test Results
|
try { | ||
KeymanWeb.registerStub(JSON.parse(jsInterface.initialKeyboard())); | ||
} catch {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be silently throwing away exceptions. If there is a specific exception that we need to handle and ignore, then that needs to be documented -- at the very least with a comment. But better to avoid the exception altogether if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is location that, if an error triggers, is guaranteed to cause a blank keyboard. It's before the keyman.init
call.
Here's a Sentry event this is meant to silence: https://keyman.sentry.io/share/issue/d9d9bd46c7404254891b606b87570bec/
I'm... not sure how that error looks on the JS side. I can attempt to reproduce it and see if there's a simple-enough way to filter it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inspecting that Sentry event, though... it hasn't really shown up in several builds, so perhaps it is safe to drop. (Last case was from 17.0.293-beta.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the best solution for now is log all exceptions to sentry, and then we can filter out known 'good' exceptions as we encounter them. So we should never have an empty catch {}
-- it should at the very least have a tellSentryAboutMyPain()
even if we then want to continue on as 'normal'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and dropped the try-catch entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and restored, now with a console.error(<error>)
in it.
Changes in this pull request will be available for download in Keyman version 17.0.311-beta |
change(web): revert #11174, which loads keyboards before initializing the OSK
…-revert-11174 change(web): revert #11174, which loads keyboards before initializing the OSK 🍒 🏠
By delaying the end of KeymanWeb's
init
method in order to check if any keyboards are pre-registered, we can initialize the OSK correctly "the first time". This matters by far the most on touch devices, where we have been building a stand-in default touch-layout the first time first due to not waiting long enough for pre-registered keyboards to fully load. (On desktop devices, we just build a very simple, empty stand-in.) Preparing the stand-in is surprisingly expensive/slow on some older touch devices.By preventing unnecessary construction of a stand-in touch layout (and thus, related DOM initialization + layout reflow) on touch devices when fully leveraged, we can eliminate a notable source of Web engine startup lag that has long been affecting our mobile apps.
I have verified that this allows us to fully bypass the empty touch-layout construction step within the Android Engine, notably cutting the host-page startup time. It currently appears that the iOS app is unable to leverage this as-is, though.
With a little rework of Web's test pages, it can be demonstrated to work for standalone KeymanWeb as well. Their current structures generally do not register keyboards until engine-initialization is complete, which is a necessary prerequisite.
User Testing
TEST_WINDOWS_DESKTOP: Test general use of KeymanWeb on a Windows desktop or laptop. Report any unexpected behavior.
TEST_ANDROID_TABLET: Test general use of KeymanWeb on an Android tablet. Report any unexpected behavior.
TEST_IOS_PHONE: Test general use of KeymanWeb on an iPhone. Report any unexpected behavior.