fix(web): gracefully disable predictive-text when WebWorkers are unavailable #13270

jahorton · 2025-02-18T03:37:24Z

User Testing

TEST_ANDROID_YETI_WITH_WEB: Using an Android device, verify that our test pages operate normally when using the Yeti browser.

Through the play store, install the Yeti browser.
Launch Yeti.
Navigate to this PR's "Test unminified KeymanWeb" test page.
Verify that the page operates normally, displaying our OSK when editing the usual elements, etc.
Navigate to this PR's "Predictive Text: Robust Testing" test page.
Verify that the page does not display the suggestion banner and that predictive text is disabled.
- The toggles may throw errors, however.

…ailable Fixes: #13262

keymanapp-test-bot · 2025-02-18T03:37:29Z

User Test Results

Test specification and instructions

✅ TEST_ANDROID_YETI_WITH_WEB (PASSED) (notes)

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 18.0.195 (0.13270.12895)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

mcdurdin

I am a little uncomfortable with how lngProcessor has now become optional without any clear maintenance documentation about this -- it feels a bit fragile.

And a question about the worker.

mcdurdin · 2025-02-18T03:40:40Z

web/src/engine/main/src/headless/inputProcessor.ts

@@ -48,7 +48,11 @@ export class InputProcessor {

    this.contextDevice = device;
    this.kbdProcessor = new KeyboardProcessor(device, options);
-    this.lngProcessor = new LanguageProcessor(predictiveTextWorker, this.contextCache);
+    try {
+      this.lngProcessor = new LanguageProcessor(predictiveTextWorker, this.contextCache);


This puzzles me, isn't the issue with Yeti that we cannot construct predictiveTextWorker, so wouldn't this go wrong (much) earlier?

keyman/web/src/engine/predictive-text/worker-main/src/web/sourcemappedWorker.ts

Lines 4 to 10 in 263f918

export default class SourcemappedWorker {

// the only difference to DefaultWorker is that this class uses

// the unminified LM* blobs

static constructInstance(): Worker {

return new Worker(this.asBlobURI(LMLayerWorkerCode));

}

The issue is that we cannot allow line 9 above to run - we can't instantiate the actual Worker. predictiveTextWorker above corresponds to an instance of the class SourcemappedWorker here. I suppose we could add Factory to the variable and parameter names to make the design pattern clearer.

There's also a Node-friendly variant for use in headless testing, which is why the abstraction exists:

keyman/web/src/engine/predictive-text/worker-main/src/node/sourcemappedWorker.ts

Lines 6 to 15 in 263f918

export default class SourcemappedWorker {

static constructInstance(): Worker {

let scriptStr = unwrap(LMLayerWorkerCode);

// If this is definitively set to either true or false, tree-shaking can take effect.

// An imported const variable doesn't seem to do it, though.

// if(false) {

scriptStr += '\n' + LMLayerWorkerSourcemapComment;

// }

let worker = new MappedWorker(scriptStr);

MappedWorker there uses Node workers internally, but their spec differs enough from the Web worker type that we needed special handling for it.

Looking at where constructInstance() is called, it seems to be called at the very top-level, e.g.

keyman/web/src/app/browser/src/release-main.ts

Lines 11 to 16 in 6591c61

var scripts = document.getElementsByTagName('script');

var ss = scripts[scripts.length-1].src;

var sPath = ss.substr(0,ss.lastIndexOf('/')+1);

// @ts-ignore

window['keyman'] = new KeymanEngine(Worker.constructInstance(), sPath);

So I am not sure how this fix handles that?

Huh. Makes me wonder why it seemed to work with the artificial repro attempt I tried. 🤷 Good point.

dinakaranr · 2025-02-18T10:47:20Z

Test Results

I tested this issue with the attached Keyman"18.0.195-beta-test-13270" build(18/02/2025) on Android 14. Here I am sharing my observation.

TEST_ANDROID_YETI_WITH_WEB (Passed):

Installed the Yeti browser.
Launch Yeti.
Navigate to this PR's "Test unminified KeymanWeb" test page.
Verified that the page operates normally.
Verified that the OSK is displayed when clicking in the " text area" box.
Verified that the suggestion banner does not appear.
Navigate to this PR's "Predictive Text: Robust Testing" test page.
Verified that the suggestion banner appeared by default.
Verified that the "Predictions & correction" button is showing by default as a "disable" state.
Click on the "disable predictions" and "disable correction" buttons.
Verify that the page does not display the suggestion banner and that predictive text is disabled.
Verified that no errors appeared when clicking the toggle buttons("Predictions & correction)
Predictive Text: "Robust Testing" test page shows the "suggestion banner" and "predictive text" by default if I click the toggles button then It's disabled.
It works well. Thank you.

jahorton · 2025-02-19T02:33:39Z

I've clarified the test results with @dinakaranr - it sounds like our WebWorker... actually does work with Yeti - at least with its latest version(s). So... that's good to see, but sadly means we don't have a solid repro we can use to actually validate the "no WebWorkers available" codepath. Hmm. Might have to do some research into a better user test.

mcdurdin · 2025-02-19T06:01:18Z

Also wondering if the lngProcessor should always exist and just have a 'disabled' state (unless 'inactive' is sufficient)? Feels less fragile than lots of optional tests and encapsulates its behavior better. And it would be cleaner to keep the try/catch as close as possible around the instance construction.

It also looks like LanguageProcessor already has an inactive state where lmEngine may be undefined if predictiveTextWorker param is falsy, can this be used?

jahorton · 2025-02-19T06:16:44Z

Also wondering if the lngProcessor should always exist and just have a 'disabled' state (unless 'inactive' is sufficient)? Feels less fragile than lots of optional tests and encapsulates its behavior better. And it would be cleaner to keep the try/catch as close as possible around the instance construction.

I opted for the strategy I did because the approach you're suggesting then motivates a question: what should the class's active methods do if called when the engine is disabled? My intuition says "throw errors", and that is prone to its own form of fragility and instability - we'd want to either try-catch them elsewhere or add a lot of if(!lngProcessor.isInactive) checks before references.

I personally felt that relying on the behavior of optionals encapsulated the same logic more cleanly and succinctly while allowing us to get away with a single try-catch.

That said, how would you answer my first paragraph's question? What would be the design for that?

mcdurdin · 2025-02-20T02:58:25Z

What should the class's active methods do if called when the engine is disabled?
That said, how would you answer my first paragraph's question? What would be the design for that?

Given that the way the class is being used is to ignore it if it isn't available, I would design the internals of the class that way -- if it is disabled, active methods become a no-op. Allow the class to be interrogated as to its state (in case we need that somewhere; for completeness); document that when the class is in disabled state, actions are a no-op, not an error.

That makes the design explicit, separates the concerns of availability of the worker, and avoids the fragility of having to nullish-check the class every usage.

jahorton · 2025-02-20T05:08:38Z

I've made a replacement PR as #13290.

jahorton · 2025-02-21T06:05:16Z

I've clarified the test results with @dinakaranr - it sounds like our WebWorker... actually does work with Yeti - at least with its latest version(s). So... that's good to see, but sadly means we don't have a solid repro we can use to actually validate the "no WebWorkers available" codepath. Hmm. Might have to do some research into a better user test.

A bit more on this - it's Yeti 1.1 that shows up in the error reports. Meanwhile, Google's Play Store mentions Yeti being at version 1.5. I'm guessing they added WebWorker support after a while.

fix(web): gracefully disable predictive-text when WebWorkers are unav…

9e2d5bf

…ailable Fixes: #13262

jahorton requested a review from ermshiperete as a code owner February 18, 2025 03:37

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Feb 18, 2025

keymanapp-test-bot bot added this to the B18S2 milestone Feb 18, 2025

github-actions bot added web/ fix labels Feb 18, 2025

jahorton linked an issue Feb 18, 2025 that may be closed by this pull request

bug(web): if web workers are unavailable, then we should gracefully degrade and still make other KeymanWeb resources available #13262

Open

mcdurdin reviewed Feb 18, 2025

View reviewed changes

docs(web): adds doc comment about languageProcessor optionality

b6069ce

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 18, 2025

jahorton mentioned this pull request Feb 20, 2025

fix(web): properly support workerless use of Web engine #13290

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(web): gracefully disable predictive-text when WebWorkers are unavailable #13270

fix(web): gracefully disable predictive-text when WebWorkers are unavailable #13270

jahorton commented Feb 18, 2025

keymanapp-test-bot bot commented Feb 18, 2025 •

edited

Loading

mcdurdin left a comment

mcdurdin Feb 18, 2025

jahorton Feb 18, 2025 •

edited

Loading

mcdurdin Feb 19, 2025

jahorton Feb 20, 2025

dinakaranr commented Feb 18, 2025

jahorton commented Feb 19, 2025

mcdurdin commented Feb 19, 2025

jahorton commented Feb 19, 2025 •

edited

Loading

mcdurdin commented Feb 20, 2025

jahorton commented Feb 20, 2025

jahorton commented Feb 21, 2025

	export default class SourcemappedWorker {
	// the only difference to DefaultWorker is that this class uses
	// the unminified LM* blobs

	static constructInstance(): Worker {
	return new Worker(this.asBlobURI(LMLayerWorkerCode));
	}

	export default class SourcemappedWorker {
	static constructInstance(): Worker {
	let scriptStr = unwrap(LMLayerWorkerCode);

	// If this is definitively set to either true or false, tree-shaking can take effect.
	// An imported const variable doesn't seem to do it, though.
	// if(false) {
	scriptStr += '\n' + LMLayerWorkerSourcemapComment;
	// }
	let worker = new MappedWorker(scriptStr);

	var scripts = document.getElementsByTagName('script');
	var ss = scripts[scripts.length-1].src;
	var sPath = ss.substr(0,ss.lastIndexOf('/')+1);

	// @ts-ignore
	window['keyman'] = new KeymanEngine(Worker.constructInstance(), sPath);

fix(web): gracefully disable predictive-text when WebWorkers are unavailable #13270

Are you sure you want to change the base?

fix(web): gracefully disable predictive-text when WebWorkers are unavailable #13270

Conversation

jahorton commented Feb 18, 2025

User Testing

keymanapp-test-bot bot commented Feb 18, 2025 • edited Loading

User Test Results

Test Artifacts

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Feb 18, 2025

Choose a reason for hiding this comment

jahorton Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

mcdurdin Feb 19, 2025

Choose a reason for hiding this comment

jahorton Feb 20, 2025

Choose a reason for hiding this comment

dinakaranr commented Feb 18, 2025

Test Results

jahorton commented Feb 19, 2025

mcdurdin commented Feb 19, 2025

jahorton commented Feb 19, 2025 • edited Loading

mcdurdin commented Feb 20, 2025

jahorton commented Feb 20, 2025

jahorton commented Feb 21, 2025

keymanapp-test-bot bot commented Feb 18, 2025 •

edited

Loading

jahorton Feb 18, 2025 •

edited

Loading

jahorton commented Feb 19, 2025 •

edited

Loading