fix(web): fix engine initialization when initial keyboard is debug-compiled

[jahorton]

Fixes: #11962

• TODO: Once ready, cherry-pick this for 17.0 stable.

Debug-mode keyboards refer to keyman.osk API points, but we try to load initial keyboards before constructing the OSK for optimization reasons, making those API points unavailable. These changes enforce the "try" aspect, retrying after OSK load once those API points become available.

User Testing

TEST_REPRO: Attempt to reproduce #11962 using Keyman for Android.

• Install the package provided within that issue.
• Force-quit the app.
• Relaunch the app.
  • If the issue is not properly fixed, an error notification will appear on app-relaunch.

TEST_STANDARD_ERROR_ALERTS: Verify that all standard keyboard error alerts still operate normally.

• Use the "Tests keyboard error-handling functionality" Web testing page, which may be found within the "Issue Testing" section of the "manual" Web testing index.
• Try out each of the three.
• Note that the last one - "timeout" - may require waiting for around 10 seconds due to the nature of what it's testing.

TEST_BROWSER_KEYBOARDS: Using the "Tests unminified Keymanweb" test page, verify that refreshing the page always restores the previously used keyboard, never displaying an error message. Do not use mobile-device emulation.

• Test with "English - English", setting the keyboard and then refreshing the page.
• Repeat with "Lao - Lao Basic".
• Repeat with "English - Classic 10-key". (The OSK will be effectively blank, but that's OK.)
  • You may see a very brief "flash" of the alert window, but so long as it's very brief, that's okay.
  • The OSK for each keyboard should appear whenever you click inside of the top-most text area.

Other notes

Within the JS console, we actually still get a copy of the keyboard-load error message for debug-mode keyboards within app/browser. No alerts will be shown and the error will be fully handled, but the call here still occurs:

keyman/web/src/app/browser/src/contextManager.ts

Lines 541 to 545 in c537bcd

	if(err instanceof KeyboardScriptError) {
	// We get signaled about error log messages if the site is connected to our Sentry error reporting
	// system; we want to know if we have a broken keyboard that's been published.
	console.error(err || message);
	} else {

The re-thrown error (see line 559) is caught and handled by keymanEngine.ts, line 246 from this PR's changes.

[keymanapp-test-bot]

User Test Results

Test specification and instructions

• ✅ TEST_REPRO (PASSED) (notes)
• ✅ TEST_STANDARD_ERROR_ALERTS (PASSED) (notes)
• ✅ TEST_BROWSER_KEYBOARDS (PASSED) (notes)

Test Artifacts

• Android
  • Keyman for Android apk
  • FirstVoices for Android 16.0 apk
  • FirstVoices Keyboards for Android apk
  • KeyboardHarness apk
  • Keyman for Android 16.0 apk
  • KMSample1 apk
  • KMSample2 apk
• Developer
  • Keyman Developer
  • Compiler Regression Tests
  • kmcomp.zip
• iOS
  • Keyman for iOS (simulator image)
  • FirstVoices Keyboards for iOS (simulator image)
  • TestFlight internal PR build version - 18.0.71 (0.11968.11448)
• Keyboards
  • Test Keyboards
• Web
  • KeymanWeb Test Home
• Windows
  • Keyman for Windows
  • FirstVoices Keyboards for Windows
  • Text Editor (32 bit)
  • Text Editor (64 bit)

[jahorton]

Addresses a side detail, though that side detail's documentation is within the same base issue. Got lumped together there, so "why not here?"

[jahorton]

Without the added true parameter, the first reload would remember the old keyboard, but the setting wouldn't be preserved for all subsequent reloads; the first-time default keyboard would appear instead.

[dinakaranr]

Test Results

I tested this issue with the attached "Keyman 18.0.71-alpha-test-11968" build on the Windows 10 and Ubuntu Mantic 23.10 OS environments(Chrome browser): Here is my observation.

• TEST_REPRO (Passed):

1. Install the "Keyman 18.0.71-alpha-test-11968" build.
2. Relaunch the app after force quit.
3. No error notification appears when forced to quit the app.

• TEST_STANDARD_ERROR_ALERTS (Passed):

1. Navigate to the "keyboard error-handling functionality" --> "KeymanWeb Sample Page - Error Testing page"
2. a) debugging - wrongfilename b) debugging - non-parsable c) debugging - timeout
3. An error alert message box appears when selecting to above options.
4. An error alert message appears after 10 seconds when selecting the "timeout" option.

• TEST_BROWSER_KEYBOARDS (Passed):

1. Navigate to the "Tests unminified Keymanweb" --> "KeymanWeb Sample Page - Unminified Source" pages.
2. "English - English": It works well in the text box and OSK keyboard.
3. "Lao - Lao Basic": It works well in the text box and OSK keyboard.
4. "English - Classic 10-key": OSK board appears but it is blank now. An alert window appears for very few seconds.
   It works as per the scenarios points. Thank you.

[mcdurdin]

This screams ugly hack, and I think it is fragile and will cause us long-term grief.

The underlying issue is that the (implicit) API contract that KeymanWeb offered (osk.modifierCodes et al) is no longer being met. Hacking around a script load error masks the problem -- we cannot assume that the keyboard is generated by the compiler -- it may use modifierCodes for other purposes. We need to make keyman.osk.modifierCodes available at all times.

It is implicit because we don't have formal documentation of the API surface. But it's still real, because there are real keyboards using it.

Alternate possibility: is it possible to create a stub osk object with the necessary members when keyman object is constructed -- e.g. osk = {modifierCodes: ..., keyCodes: ...}? We'd just throw the stub away when we rebuild the osk.

[jahorton]

Alternate possibility: is it possible to create a stub osk object with the necessary members when keyman object is constructed -- e.g. osk = {modifierCodes: ..., keyCodes: ...}? We'd just throw the stub away when we rebuild the osk.

That was my first thought, but to me, that screamed "uglier hack" - it's why I decided on this approach. It might look weird to API consumers to have a not-OSK available when no OSK has been set in place.

(Originally noted as a possibliity here: #11962 (comment) - CTRL-F temporarily "mock" the OSK)

It wouldn't be too hard to set it during script-load and then remove it once the script-load is done. That said, it'd also be kind of ugly to have such a core field of the engine subject to this sort of variability.

[jahorton]

Also of note: we only noticed this now because of how late the OSK is being initialized. If we somehow had a state in 16.0 or before where the OSK was left uninitialized while a keyboard loaded, we'd have had the same problem there as well. It's simply that such a state never existed then - mostly due to loading an initial "blank" / "stand-in" keyboard. (This got labeled as (System keyboard) within the mobile apps when it occurred.)

The only difference for 17.0 is by optimizing away that initial blank-keyboard state that was originally part of the OSK lifecycle, we enter a state where we can try to load the initial keyboard before the keyman.osk.* endpoints are available.

[mcdurdin]

The appropriate fix may be to roll back the optimization in v17. Correct behavior is more important than speed.

That was my first thought, but to me, that screamed "uglier hack" - it's why I decided on this approach. It might look weird to API consumers to have a not-OSK available when no OSK has been set in place.

Sorry, I think faking out the global script load error handler is definitely uglier!

I also can't see how it could even work correctly -- surely the keyboard object would then have invalid modifierCodes and keyCodes variables internally, leading to more subtle failures later on? This is what the debug-compiled keyboard script is doing:

KeymanWeb.KR(new Keyboard_kbdal());
function Keyboard_kbdal()
{
  var modCodes = keyman.osk.modifierCodes;
  var keyCodes = keyman.osk.keyCodes;
  ...

Other keyboards may do other things, we should always assume keyman.* is fully valid when a keyboard is loaded, because we cannot control what a keyboard script does.

But I agree it's not ideal either way.

It feels like keyman.osk was designed to be persistent as an API surface but is no longer (or perhaps was never) like that. There's nothing in the documentation to specify the lifecycle of keyman.osk. For example, what happens to event handlers attached to keyman.osk if keyboard view style changes? That seems like it could be a (maybe less important) gotcha too?

We could do something like this for the stub?

• addEventListener -- add a console.warn?
• getRect -- return a nullrect?
• setRect -- add a console.warn?
• setPos -- add a console.warn?
• hide -- noop?
• isEnabled -- return false
• isVisible -- return false
• removeEventListener -- noop?
• restorePosition -- console.warn?
• show -- console.warn?
• userLocated -- return false
• modifierCodes -- return the modifierCodes
• keyCodes -- return the keyCodes
• (any other members?)

So, given the stubbing approach is incomplete, perhaps we should approach this differently.

1. we rollback the performance improvement at least for v17, and
2. reapproach this with a clear picture of how we can separate 'code' (API surface) from 'data' (keyboard script). Can we initialize the osk property without a keyboard, and apply the keyboard data afterwards? We need to document what expectations for osk.* calls are when no keyboard has yet been loaded.

[jahorton]

I also can't see how it could even work correctly -- surely the keyboard object would then have invalid modifierCodes and keyCodes variables internally, leading to more subtle failures later on?

As you've noted, the early-load will be subject to failure. The keyboard doesn't even finish loading due to the error, so we never get the keyboard object during that load. (The keyboard script-object's constructor never completes.)

The workaround is pretty easy, though - just retry once the codes are available. We track if a failure occurred; if so, once the OSK is loaded, retry the load. That's the purpose behind the new deferForOsk field added to the engine's configuration-state object; it exists to facilitate the 'retry' mechanism. In essence, this retry mechanism implements your suggestion - it removes the "optimization" part from the picture when the optimized-load fails.

The whole point of the error-checking + silencing is to say "Let's try to load it early. If that fails, try at the old, unoptimized time."

Of note: the user who reported the error was still able to use the keyboard despite the error + error alert. We kind of lucked out in that existing mobile-app code triggered the retry for us there, admittedly.

[jahorton]

It feels like keyman.osk was designed to be persistent as an API surface but is no longer (or perhaps was never) like that.

I'd say the perspective on this changed once we introduced the "inlined OSK" for KMW 15.0. (See: #5665) Before then, there was only ever the "one true OSK" for a device, but afterward we allowed OSK hotswapping - a feature needed for the Developer test-host page.

There's nothing in the documentation to specify the lifecycle of keyman.osk. For example, what happens to event handlers attached to keyman.osk if keyboard view style changes? That seems like it could be a (maybe less important) gotcha too?

Yeah, I don't think we gave too much thought to this at the time. I think we're kind of operating under a policy of "if you're comfortable manipulating this undocumented OSK-hotswap feature, you should be able to figure this out for yourself too."

[mcdurdin]

The workaround is pretty easy, though - just retry once the codes are available

I don't think we should ever be re-running keyboard scripts. We cannot assume what the script looks like. It might define a global const, which would fail on second run, for example.

Of note: the user who reported the error was still able to use the keyboard despite the error + error alert.

But did the modifier keys work correctly? Unless they were testing with a hardware keyboard, they might never know.

[jahorton]

So, given the stubbing approach is incomplete, perhaps we should approach this differently.

1. we rollback the performance improvement at least for v17, and
2. reapproach this with a clear picture of how we can separate 'code' (API surface) from 'data' (keyboard script). Can we initialize the osk property without a keyboard, and apply the keyboard data afterwards? We need to document what expectations for osk.* calls are when no keyboard has yet been loaded.

So... it sounds like you're asking for a reversion of #11174.

We already know it's possible to initialize osk without a keyboard; it's just kinda expensive, especially on older devices.

For 18.0, we have some additional optimization improvements should significantly mitigate this, as we won't need to build as much of the blank/default OSK as we previously did. That won't really help things for 17.0-stable, though.

[mcdurdin]

So... it sounds like you're asking for a reversion of #11174.

Yes, I think we have to do that.

[jahorton]

Closing in favor of #12015, which was merged earlier today and implements the reversion mentioned above.