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

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

Open
wants to merge 2 commits into
base: beta
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e2d5bf
fix(web): gracefully disable predictive-text when WebWorkers are unav…
Feb 18, 2025
b6069ce
docs(web): adds doc comment about languageProcessor optionality
Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion web/src/app/browser/src/keymanEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,7 @@ export default class KeymanEngine extends KeymanEngineBase<BrowserConfiguration,
this.pageIntegration.shutdown();
this.contextManager.shutdown();
this.osk?.shutdown();
this.core.languageProcessor.shutdown();
this.core.languageProcessor?.shutdown();
this.hardKeyboard.shutdown();
this.util.shutdown(); // For tracked dom events, stylesheets.

Expand Down
22 changes: 11 additions & 11 deletions web/src/engine/interfaces/src/prediction/predictionContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,19 +93,19 @@ export default class PredictionContext extends EventEmitter<PredictionContextEve
}

private connect() {
this.langProcessor.addListener('invalidatesuggestions', this.invalidateSuggestions);
this.langProcessor.addListener('suggestionsready', this.updateSuggestions);
this.langProcessor.addListener('tryaccept', this.doTryAccept);
this.langProcessor.addListener('tryrevert', this.doTryRevert);
this.langProcessor.addListener('statechange', this.onModelStateChange);
this.langProcessor?.addListener('invalidatesuggestions', this.invalidateSuggestions);
this.langProcessor?.addListener('suggestionsready', this.updateSuggestions);
this.langProcessor?.addListener('tryaccept', this.doTryAccept);
this.langProcessor?.addListener('tryrevert', this.doTryRevert);
this.langProcessor?.addListener('statechange', this.onModelStateChange);
}

public disconnect() {
this.langProcessor.removeListener('invalidatesuggestions', this.invalidateSuggestions);
this.langProcessor.removeListener('suggestionsready', this.updateSuggestions);
this.langProcessor.removeListener('tryaccept', this.doTryAccept);
this.langProcessor.removeListener('tryrevert', this.doTryRevert);
this.langProcessor.removeListener('statechange', this.onModelStateChange);
this.langProcessor?.removeListener('invalidatesuggestions', this.invalidateSuggestions);
this.langProcessor?.removeListener('suggestionsready', this.updateSuggestions);
this.langProcessor?.removeListener('tryaccept', this.doTryAccept);
this.langProcessor?.removeListener('tryrevert', this.doTryRevert);
this.langProcessor?.removeListener('statechange', this.onModelStateChange);
this.clearSuggestions();
}

Expand Down Expand Up @@ -356,7 +356,7 @@ export default class PredictionContext extends EventEmitter<PredictionContextEve
if(target) {
// Note: should be triggered after the corresponding new-context event rule has been processed,
// as that may affect the value of layerId here.
return this.langProcessor.invalidateContext(target, this.getLayerId());
return this.langProcessor?.invalidateContext(target, this.getLayerId()) ?? Promise.resolve([]);
} else {
return Promise.resolve([]);
}
Expand Down
23 changes: 16 additions & 7 deletions web/src/engine/main/src/headless/inputProcessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,18 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jahorton jahorton Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

} catch {
/* Some browsers ("Yeti") do not support WebWorkers - see #13262. */
}
}

/**
* Returns the module directly responsible for communicating with the predictive-text WebWorker.
*
* Note: may be `undefined` if the user's browser does not support WebWorkers. (See #13262.)
*/
public get languageProcessor(): LanguageProcessor {
return this.lngProcessor;
}
Expand All @@ -76,7 +85,7 @@ export class InputProcessor {
}

public get activeModel(): ModelSpec {
return this.languageProcessor.activeModel;
return this.languageProcessor?.activeModel;
}

/**
Expand Down Expand Up @@ -166,7 +175,7 @@ export class InputProcessor {
// If suggestions exist AND space is pressed, accept the suggestion and do not process the keystroke.
// If a suggestion was just accepted AND backspace is pressed, revert the change and do not process the backspace.
// We check the first condition here, while the prediction UI handles the second through the try__() methods below.
if(this.languageProcessor.isActive) {
if(this.languageProcessor?.isActive) {
// The following code relies on JS's logical operator "short-circuit" properties to prevent unwanted triggering of the second condition.

// Can the suggestion UI revert a recent suggestion? If so, do that and swallow the backspace.
Expand Down Expand Up @@ -260,7 +269,7 @@ export class InputProcessor {
}

// Yes, even for ruleBehavior.triggersDefaultCommand. Those tend to change the context.
ruleBehavior.predictionPromise = this.languageProcessor.predict(ruleBehavior.transcription, this.keyboardProcessor.layerId);
ruleBehavior.predictionPromise = this.languageProcessor?.predict(ruleBehavior.transcription, this.keyboardProcessor.layerId);

// Text did not change (thus, no text "input") if we tabbed or merely moved the caret.
if(!ruleBehavior.triggersDefaultCommand) {
Expand All @@ -276,7 +285,7 @@ export class InputProcessor {

// If we're performing a 'default command', it's not a standard 'typing' event - don't do fat-finger stuff.
// Also, don't do fat-finger stuff if predictive text isn't enabled.
if(this.languageProcessor.isActive && !ruleBehavior.triggersDefaultCommand) {
if(this.languageProcessor?.isActive && !ruleBehavior.triggersDefaultCommand) {
let keyDistribution = keyEvent.keyDistribution;

// We don't need to track absolute indexing during alternate-generation;
Expand All @@ -287,7 +296,7 @@ export class InputProcessor {
let windowedMock = contextWindow.toMock();

// Note - we don't yet do fat-fingering with longpress keys.
if(this.languageProcessor.isActive && keyDistribution && keyEvent.kbdLayer) {
if(keyDistribution && keyEvent.kbdLayer) {
// Tracks a 'deadline' for fat-finger ops, just in case both context is long enough
// and device is slow enough that the calculation takes too long.
//
Expand Down Expand Up @@ -381,6 +390,6 @@ export class InputProcessor {
// Also handles new-context events, which may modify the layer
this.keyboardProcessor.resetContext(outputTarget);
// With the layer now set, we trigger new predictions.
this.languageProcessor.invalidateContext(outputTarget, this.keyboardProcessor.layerId);
this.languageProcessor?.invalidateContext(outputTarget, this.keyboardProcessor.layerId);
}
}
12 changes: 6 additions & 6 deletions web/src/engine/main/src/keymanEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export default class KeymanEngine<
return;
}

if(!this.core.languageProcessor.mayCorrect) {
if(!this.core.languageProcessor?.mayCorrect) {
event.keyDistribution = [];
}

Expand Down Expand Up @@ -125,7 +125,7 @@ export default class KeymanEngine<
this.interface = processorConfiguration.keyboardInterface as KeyboardInterface<ContextManager>;
this.core = new InputProcessor(config.hostDevice, worker, processorConfiguration);

this.core.languageProcessor.on('statechange', (state) => {
this.core.languageProcessor?.on('statechange', (state) => {
// The banner controller cannot directly trigger a layout-refresh at this time,
// so we handle that here.
this.osk?.bannerController.selectBanner(state);
Expand Down Expand Up @@ -269,7 +269,7 @@ export default class KeymanEngine<
* This is called after the suggestion is applied but _before_ new
* predictions are requested based on the resulting context.
*/
this.core.languageProcessor.on('suggestionapplied', () => {
this.core.languageProcessor?.on('suggestionapplied', () => {
// Tell the keyboard that the current layer has not changed
keyboardProcessor.newLayerStore.set('');
keyboardProcessor.oldLayerStore.set('');
Expand Down Expand Up @@ -418,12 +418,12 @@ export default class KeymanEngine<

if(this.core.activeModel != model) {
if(this.core.activeModel) {
this.core.languageProcessor.unloadModel();
this.core.languageProcessor?.unloadModel();
}

// Semi-hacky management of banner display state.
if(model) {
return this.core.languageProcessor.loadModel(model).then(() => {
return this.core.languageProcessor?.loadModel(model).then(() => {
return model;
});
}
Expand Down Expand Up @@ -495,7 +495,7 @@ export default class KeymanEngine<

// Is it the active model?
if(this.core.activeModel && this.core.activeModel.id == modelId) {
this.core.languageProcessor.unloadModel();
this.core.languageProcessor?.unloadModel();
}
}

Expand Down