-
-
Notifications
You must be signed in to change notification settings - Fork 143
fix(web): allow to set default keyboard to 'off' #16524
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -292,7 +292,10 @@ export class KeymanEngine extends KeymanEngineBase<BrowserConfiguration, Context | |||||||||||||||||||||||||||||||||||||
| * @param {string|null=} languageCode A BCP47 language code which was used when | ||||||||||||||||||||||||||||||||||||||
| * registering the keyboard stub. | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| public setKeyboardForControl(elem: HTMLElement, keyboard?: string, languageCode?: string): void { | ||||||||||||||||||||||||||||||||||||||
| public setKeyboardForControl(elem: HTMLElement, keyboard?: string | null, languageCode?: string | null): void { | ||||||||||||||||||||||||||||||||||||||
| if (!elem.ownerDocument.defaultView) { | ||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+300
to
+302
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What role does this new conditional play?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to play safe.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems unlikely? But if we want to really play safe, we should:
Suggested change
That will cover cases where 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
Suggested change
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. |
||||||||||||||||||||||||||||||||||||||
| if(elem instanceof elem.ownerDocument.defaultView.HTMLIFrameElement) { | ||||||||||||||||||||||||||||||||||||||
| console.warn("'keymanweb.setKeyboardForControl' cannot set keyboard on iframes."); | ||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -311,7 +314,7 @@ export class KeymanEngine extends KeymanEngineBase<BrowserConfiguration, Context | |||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| this.contextManager.setKeyboardForTextStore(elem._kmwAttachment.textStore, keyboard, languageCode); | ||||||||||||||||||||||||||||||||||||||
| this.contextManager.setKeyboardForTextStore(elem._kmwAttachment.textStore, keyboard ?? null, languageCode ?? null); | ||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -320,13 +323,20 @@ export class KeymanEngine extends KeymanEngineBase<BrowserConfiguration, Context | |||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||
| * See https://help.keyman.com/developer/engine/web/current-version/reference/core/getKeyboardForControl | ||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||
| * @param {Element} Pelem Control element | ||||||||||||||||||||||||||||||||||||||
| * @param {Element} elem Control element | ||||||||||||||||||||||||||||||||||||||
| * @return {string|null} The independently-managed keyboard for the control, | ||||||||||||||||||||||||||||||||||||||
| * or null if it is following the global keyboard setting. | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
331
to
332
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be |
||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| public getKeyboardForControl(Pelem: HTMLElement): string | null{ | ||||||||||||||||||||||||||||||||||||||
| const textStore = textStoreForElement(Pelem); | ||||||||||||||||||||||||||||||||||||||
| return this.contextManager.getKeyboardStubForTextStore(textStore).id; | ||||||||||||||||||||||||||||||||||||||
| public getKeyboardForControl(elem: HTMLElement): string | null{ | ||||||||||||||||||||||||||||||||||||||
| if(!elem || !this.contextManager.isElementInIndependentMode(elem)) { | ||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| const keyboard = elem._kmwAttachment.keyboard; | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also suggest that we use |
||||||||||||||||||||||||||||||||||||||
| if(keyboard === '') { | ||||||||||||||||||||||||||||||||||||||
| return ''; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| const stub = this.keyboardRequisitioner.cache.getStub(keyboard, elem._kmwAttachment.languageCode); | ||||||||||||||||||||||||||||||||||||||
| return stub?.KI ?? keyboard; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Is not currently published API... but it exists. | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -335,13 +345,15 @@ export class KeymanEngine extends KeymanEngineBase<BrowserConfiguration, Context | |||||||||||||||||||||||||||||||||||||
| * for this control. If it is currently following the global keyboard setting, | ||||||||||||||||||||||||||||||||||||||
| * returns null instead. | ||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||
| * @param {Element} Pelem Control element | ||||||||||||||||||||||||||||||||||||||
| * @return {string|null} The independently-managed keyboard for the control, | ||||||||||||||||||||||||||||||||||||||
| * or null if it is following the global keyboard setting. | ||||||||||||||||||||||||||||||||||||||
| * @param {Element} elem Control element | ||||||||||||||||||||||||||||||||||||||
| * @return {string|null} The independently-managed keyboard for the control, | ||||||||||||||||||||||||||||||||||||||
| * or null if it is following the global keyboard setting. | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| public getLanguageForControl(Pelem: HTMLElement): string | null { | ||||||||||||||||||||||||||||||||||||||
| const textStore = textStoreForElement(Pelem); | ||||||||||||||||||||||||||||||||||||||
| return this.contextManager.getKeyboardStubForTextStore(textStore).langId; | ||||||||||||||||||||||||||||||||||||||
| public getLanguageForControl(elem: HTMLElement): string | null { | ||||||||||||||||||||||||||||||||||||||
| if(!elem || !this.contextManager.isElementInIndependentMode(elem)) { | ||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| return elem._kmwAttachment.languageCode; | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Query: should this be returning |
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| public isAttached(x: HTMLElement): boolean { | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -422,7 +422,7 @@ export class LanguageMenu { | |
| } | ||
|
|
||
| /** | ||
| * Add a keyboard entry to the language menu * | ||
| * Add a keyboard entry to the language menu | ||
| * | ||
| * @param {Object} kbd keyboard object | ||
| * @param {Object} kb element being added and styled | ||
|
|
@@ -558,7 +558,6 @@ export class LanguageMenu { | |
|
|
||
| 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(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is already done in |
||
| languageMenu.hide(); | ||
| } | ||
|
|
||
|
|
@@ -598,4 +597,4 @@ export class LanguageMenu { | |
|
|
||
| this.keyman.touchLanguageMenu = null; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,58 @@ | ||||
| /* | ||||
| * Keyman is copyright (C) SIL Global. MIT License. | ||||
| */ | ||||
| import { KeymanEngine } from 'keyman/app/browser'; | ||||
| import { StubAndKeyboardCache } from 'keyman/engine/keyboard-storage'; | ||||
| import { assert } from 'chai'; | ||||
|
|
||||
| const mockWorkerFactory = { | ||||
| constructInstance: (): null => null | ||||
| }; | ||||
|
|
||||
| describe('KeymanEngine.getKeyboardForControl', () => { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No explicit While I guess they're kinda tied, you should also be able to verify three things:
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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added e2e tests |
||||
| let engine: KeymanEngine; | ||||
| let keyboardCache: StubAndKeyboardCache; | ||||
|
|
||||
| beforeEach(() => { | ||||
| engine = new KeymanEngine(mockWorkerFactory, ''); | ||||
| keyboardCache = new StubAndKeyboardCache(); | ||||
| (engine as any).keyboardRequisitioner = { | ||||
| cache: keyboardCache | ||||
| }; | ||||
| }); | ||||
|
|
||||
| it('returns null for controls in global mode', () => { | ||||
| const input = document.createElement('input'); | ||||
| document.body.appendChild(input); | ||||
| engine.attachToControl(input); | ||||
|
|
||||
| assert.isNull(engine.getKeyboardForControl(input)); | ||||
| }); | ||||
|
|
||||
| it('returns empty string for explicit system-keyboard mode', () => { | ||||
| const input = document.createElement('input'); | ||||
| document.body.appendChild(input); | ||||
| engine.attachToControl(input); | ||||
|
|
||||
| engine.setKeyboardForControl(input, '', ''); | ||||
| assert.equal(engine.getKeyboardForControl(input), ''); | ||||
| }); | ||||
|
|
||||
| it('returns canonical prefixed ID after setting an unprefixed ID', () => { | ||||
| const stub = { | ||||
| KI: 'Keyboard_lao_2008_basic', | ||||
| KN: 'Lao 2008 Basic', | ||||
| KL: 'Lao', | ||||
| KLC: 'lo', | ||||
| KF: 'resources/keyboards/lao_2008_basic.js', | ||||
| } as any; | ||||
| keyboardCache.addStub(stub); | ||||
|
|
||||
| const input = document.createElement('input'); | ||||
| document.body.appendChild(input); | ||||
| engine.attachToControl(input); | ||||
|
|
||||
| engine.setKeyboardForControl(input, 'lao_2008_basic', 'lo'); | ||||
| assert.equal(engine.getKeyboardForControl(input), 'Keyboard_lao_2008_basic'); | ||||
| }); | ||||
| }); | ||||
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.
Why not use
kbdId?: string, langId?: stringinstead? That way, there's no need for the nullish fallbacksin line 317 of keymanEngine.ts.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.
Since
nullis 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 toKeymanEngine.setKeyboardForControl.Done.