fix(web): allow to set default keyboard to 'off' by ermshiperete · Pull Request #16524 · keymanapp/keyman

ermshiperete · 2026-09-03T16:52:42Z

The parameters for setKeyboardForControl basically have three different modes: - an id of a keyboard/language to set that keyboard, enabling independent keyboard mode - empty string to set the system keyboard, enabling independent keyboard mode - null to disable independent keyboard mode.

Our previous code didn't properly handle the last two modes. This change fixes the problems and also clarifies and updates the documentation.

Also included is an improvement to the guide-examples e2e tests that now wait until all keyboards are loaded (which may fix #16167).

Partially drafted by kilo.ai.

User tests will follow in an upcoming PR for #16522.

Fixes: #16080
Build-bot: skip release:web
Test-bot: skip

keymanapp-test-bot · 2026-09-03T16:52:47Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Web
- KeymanWeb Test Home

ermshiperete · 2026-09-07T17:30:36Z

 }

-test.describe.skip('First example from the guide', function () {
+test.describe('First example from the guide', function () {


The guide-examples e2e tests are re-enabled; if it turns out that they're still not stable then we can disable them again.

ermshiperete · 2026-09-08T09:54:10Z


        languageMenu.lgList.style.display='none'; //still allows blank menu momentarily on selection
        languageMenu.keyman.contextManager.activateKeyboard(entry.kn, entry.kc,true);
-        languageMenu.keyman.contextManager.restoreLastActiveTextStore();


This is already done in activateKeyboard, so there's no need to do it twice (especially since activateKeyboard is async, so this call could possibly work with outdated data...)

The parameters for `setKeyboardForControl` basically have three different modes: - an id of a keyboard/language to set that keyboard, enabling independent keyboard mode - empty string to set the system keyboard, enabling independent keyboard mode - `null` to disable independent keyboard mode. Our previous code didn't properly handle the last two modes. This change fixes the problems and also clarifies and updates the documentation. Also included is an improvement to the guide-examples e2e tests that now wait until all keyboards are loaded (which may fix #16167). Partially drafted by kilo.ai. Fixes: #16080 Build-bot: skip release:web

jahorton · 2026-09-08T13:04:58Z

+    if (!elem.ownerDocument.defaultView) {
+      return;
+    }


What role does this new conditional play?

Just to play safe. defaultView could theoretically be null.

It seems unlikely? But if we want to really play safe, we should:

Suggested change

if (!elem.ownerDocument.defaultView) {

return;

}

if (!elem?.ownerDocument?.defaultView) {

return;

}

That will cover cases where elem is not set.

In some ways, this may be misleading -- perhaps it would be better for an error to be raised, so the consumer knows that this failed. Either by returning false or by throw:

Suggested change

if (!elem.ownerDocument.defaultView) {

return;

}

if (!elem?.ownerDocument?.defaultView) {

return false;

}

Suggested change

if (!elem.ownerDocument.defaultView) {

return;

}

if (!elem?.ownerDocument?.defaultView) {

throw new Error('setKeyboardForControl: elem null or has no default View');

}

However, my primary thought here is that there are so many things we could consider for all the API endpoints -- this is a piecemeal validation step which will probably become inconsistent over time with other API endpoints, so really we should be thinking about parameter validation across the whole API -- types, object shape, return values / exceptions / console.warn / console.error, and implement that consistently. That would be a better outcome than micro patches. We see a similar issue with lines 299-302 below, where we have a console warning raised which is not really visible to the API consumer at runtime -- so becomes an invisible log message in 99% of cases.

ok, I'll undo this check.
Done.

jahorton · 2026-09-08T13:06:55Z

   * @param langId
   */
-  public setKeyboardForTextStore(textStore: AbstractElementTextStore<any>, kbdId: string, langId: string): void {
+  public setKeyboardForTextStore(textStore: AbstractElementTextStore<any>, kbdId: string | null, langId: string | null): void {


Why not use kbdId?: string, langId?: string instead? That way, there's no need for the nullish fallbacksin line 317 of keymanEngine.ts.

Since null is one of the possible values I'd like to keep it in the type to be explicit. But I can make the arguments optional so that the signature is similar to KeymanEngine.setKeyboardForControl.

Done.

jahorton · 2026-09-08T13:07:34Z

    }

-    this.contextManager.setKeyboardForTextStore(elem._kmwAttachment.textStore, keyboard, languageCode);
+    this.contextManager.setKeyboardForTextStore(elem._kmwAttachment.textStore, keyboard ?? null, languageCode ?? null);


I would prefer changes that leave these two parameters completely pass-through. Why change this line when an equally-simple change avoids the need for it?

jahorton · 2026-09-08T13:25:21Z

+  constructInstance: (): null => null
+};
+
+describe('KeymanEngine.getKeyboardForControl', () => {


No explicit .setKeyboardForControl tests here?

While I guess they're kinda tied, you should also be able to verify three things:

A keystroke (either physical or via OSK) results in the correct output character

Swapping to a second, still-global control swaps the current "active keyboard" reported by the engine to the global keyboard setting.

Swapping back to the original control restores its setting and what the "current keyboard" reported by the engine is.

I thought we had some old automated tests that might have already been testing points 2 and 3, but I don't see them upon a search.

They did exist back in stable-16.0, but apparently they got erased at some point by accident during work toward stable-17.0. Here's a permalink to the relevant automated tests from before:

keyman/web/unit_tests/cases/attachmentAPI.js

Line 94 in 88776fb

it("Keyboard Management (active control)", function() {

Added e2e tests

Also add links to guide examples to test page.

mcdurdin · 2026-09-15T06:59:12Z

+    if (!elem.ownerDocument.defaultView) {
+      return;
+    }


It seems unlikely? But if we want to really play safe, we should:

Suggested change

if (!elem.ownerDocument.defaultView) {

return;

}

if (!elem?.ownerDocument?.defaultView) {

return;

}

That will cover cases where elem is not set.

In some ways, this may be misleading -- perhaps it would be better for an error to be raised, so the consumer knows that this failed. Either by returning false or by throw:

Suggested change

if (!elem.ownerDocument.defaultView) {

return;

}

if (!elem?.ownerDocument?.defaultView) {

return false;

}

Suggested change

if (!elem.ownerDocument.defaultView) {

return;

}

if (!elem?.ownerDocument?.defaultView) {

throw new Error('setKeyboardForControl: elem null or has no default View');

}

However, my primary thought here is that there are so many things we could consider for all the API endpoints -- this is a piecemeal validation step which will probably become inconsistent over time with other API endpoints, so really we should be thinking about parameter validation across the whole API -- types, object shape, return values / exceptions / console.warn / console.error, and implement that consistently. That would be a better outcome than micro patches. We see a similar issue with lines 299-302 below, where we have a console warning raised which is not really visible to the API consumer at runtime -- so becomes an invisible log message in 99% of cases.

mcdurdin · 2026-09-15T10:50:24Z

+    if(!elem || !this.contextManager.isElementInIndependentMode(elem)) {
+      return null;
+    }
+    const keyboard = elem._kmwAttachment.keyboard;


What if _kmwAttachment is not defined?

Suggested change

const keyboard = elem._kmwAttachment.keyboard;

const keyboard = elem._kmwAttachment?.keyboard ?? '';

Also suggest that we use keyboardId for consistency. Assuming that is what it is?

If _kmwAttachment is not defined then isElementInIndependentMode will return false, so at this point here we know that it is defined.

mcdurdin · 2026-09-15T10:51:41Z

+    if(!elem || !this.contextManager.isElementInIndependentMode(elem)) {
+      return null;
+    }
+    return elem._kmwAttachment.languageCode;


Suggested change

return elem._kmwAttachment.languageCode;

return elem._kmwAttachment?.languageCode ?? '';

Query: should this be returning '' for system-specified language (matching the shape of getKeyboardForControl) or null (which is simpler to reason on)?

To make it symmetrical to the values we pass to setKeyboardForControl it should return ''.

Done.

mcdurdin · 2026-09-15T10:52:57Z

   * @return      {string|null}         The independently-managed keyboard for the control,
   *                                    or null if it is following the global keyboard setting.


This may be '' to mean 'independently-managed but set to system keyboard', I think according to the intention of the spec. We should make sure that is clear

Requesting re-review

Co-authored-by: Marc Durdin <marc@durdin.net>

keyman-server · 2026-09-17T18:01:30Z

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

ermshiperete requested review from jahorton and mcdurdin September 3, 2026 16:52

github-project-automation Bot moved this to Todo in Keyman Sep 3, 2026

github-project-automation Bot added this to Keyman Sep 3, 2026

keymanapp-test-bot Bot added the user-test-missing User tests have not yet been defined for the PR label Sep 3, 2026

keymanapp-test-bot Bot added this to the B19S1 milestone Sep 3, 2026

github-actions Bot added web/ fix labels Sep 3, 2026

ermshiperete force-pushed the fix/web/16080_kbdoff branch from 5d4a63b to 466b822 Compare September 3, 2026 17:12

ermshiperete mentioned this pull request Sep 4, 2026

bug(web): inconsistency firing keyboardchange events #16530

Open

ermshiperete force-pushed the fix/web/16080_kbdoff branch from 466b822 to 01921a2 Compare September 4, 2026 20:30

ermshiperete changed the base branch from chore/web/typesInTest to refactor/web/funcscope September 4, 2026 20:32

ermshiperete force-pushed the fix/web/16080_kbdoff branch from 01921a2 to a21df46 Compare September 4, 2026 21:25

ermshiperete force-pushed the fix/web/16080_kbdoff branch 2 times, most recently from 96c527c to 9de509a Compare September 7, 2026 17:19

ermshiperete commented Sep 7, 2026

View reviewed changes

ermshiperete force-pushed the fix/web/16080_kbdoff branch from 9de509a to f82c68d Compare September 8, 2026 09:20

ermshiperete commented Sep 8, 2026

View reviewed changes

ermshiperete marked this pull request as ready for review September 8, 2026 09:55

Base automatically changed from refactor/web/funcscope to master September 8, 2026 09:56

ermshiperete force-pushed the fix/web/16080_kbdoff branch from f82c68d to 4d7867e Compare September 8, 2026 09:56

jahorton reviewed Sep 8, 2026

View reviewed changes

chore(web): address code review comments

e50fd2c

Also add links to guide examples to test page.

keyman-server modified the milestones: B19S1, B19S2 Sep 12, 2026

test(web): add e2e tests for independent/global mode

835f226

ermshiperete requested a review from jahorton September 15, 2026 09:03

mcdurdin previously requested changes Sep 15, 2026

View reviewed changes

fix(web): address code review comments

4b7c0d4

ermshiperete requested a review from mcdurdin September 16, 2026 13:45

mcdurdin approved these changes Sep 16, 2026

View reviewed changes

Comment thread web/src/app/browser/src/contextManager.ts Outdated

Comment thread web/src/app/browser/src/contextManager.ts Outdated

Apply batched suggestions from code review

56524f1

Co-authored-by: Marc Durdin <marc@durdin.net>

keymanapp-test-bot Bot removed the user-test-missing User tests have not yet been defined for the PR label Sep 17, 2026

ermshiperete merged commit 7b2f275 into master Sep 17, 2026
8 checks passed

github-project-automation Bot moved this from Todo to Done in Keyman Sep 17, 2026

-    if (!elem.ownerDocument.defaultView) {
-      return;
-    }
+    if (!elem?.ownerDocument?.defaultView) {
+      return false;
+    }

	const keyboard = elem._kmwAttachment.keyboard;
	const keyboard = elem._kmwAttachment?.keyboard ?? '';

	return elem._kmwAttachment.languageCode;
	return elem._kmwAttachment?.languageCode ?? '';

		* @return {string\|null} The independently-managed keyboard for the control,
		* or null if it is following the global keyboard setting.

Uh oh!

Conversation

ermshiperete commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keymanapp-test-bot Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ermshiperete Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keyman-server commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ermshiperete commented Sep 3, 2026 •

edited

Loading

keymanapp-test-bot Bot commented Sep 3, 2026 •

edited

Loading

ermshiperete Sep 16, 2026 •

edited

Loading