Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
116 changes: 114 additions & 2 deletions web/src/engine/src/core-processor/coreKeyboardProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ import {
DeviceSpec, EventMap, Keyboard, KeyboardMinimalInterface, KeyboardProcessor,
KeyEvent, KMXKeyboard, SyntheticTextStore, MutableSystemStore, TextStore, ProcessorAction,
StateKeyMap,
Deadkey
Deadkey,
Codes
} from "keyman/engine/keyboard";
import { KM_CORE_EVENT_FLAG } from '../core-adapter/KM_Core.js';
import { ModifierKeyConstants } from '@keymanapp/common-types';

export class CoreKeyboardInterface implements KeyboardMinimalInterface {
public activeKeyboard: Keyboard;
Expand Down Expand Up @@ -150,6 +152,12 @@ export class CoreKeyboardProcessor extends EventEmitter<EventMap> implements Key
this._layerStore.set(value);
}

private getLayerId(modifier: number): string {
// TODO-web-core: implement
// return Layouts.getLayerId(modifier);
return 'default'; // TODO-web-core: put into LayerNames enum
}

/**
* Retrieve context including deadkeys from TextStore and apply to Core's context
*
Expand Down Expand Up @@ -302,6 +310,7 @@ export class CoreKeyboardProcessor extends EventEmitter<EventMap> implements Key
return null;
}

// TODO-web-core: this could be shared with JsKeyboardProcessor
/**
* Determines if the given key event is a modifier key press.
* Returns true if the event corresponds to a modifier key, otherwise false.
Expand All @@ -313,10 +322,113 @@ export class CoreKeyboardProcessor extends EventEmitter<EventMap> implements Key
* @returns {boolean} True if the event is a modifier key press, false otherwise.
*/
public doModifierPress(keyEvent: KeyEvent, textStore: TextStore, isKeyDown: boolean): boolean {
// TODO-web-core: Implement this method (#15287)
if(!this.activeKeyboard) {
return false;
}

if(keyEvent.isModifier) {
this.activeKeyboard.notify(keyEvent.Lcode, textStore, isKeyDown);
// For eventual integration - we bypass an OSK update for physical keystrokes when in touch mode.
if(!keyEvent.device.touchable) {
return this._UpdateVKShift(keyEvent); // I2187
} else {
return true;
}
}

if(keyEvent.LmodifierChange) {
this.activeKeyboard.notify(0, textStore, true);
if(!keyEvent.device.touchable) {
this._UpdateVKShift(keyEvent);
}
}

// No modifier keypresses detected.
return false;
}


// TODO-web-core: this could be shared with JsKeyboardProcessor
/**
* Updates the virtual keyboard shift state based on the provided key event.
* Handles modifier key simulation, state key updates, and layer selection for the OSK.
*
* @param {KeyEvent} e - The key event used to update the shift state.
*
* @returns {boolean} True if the update was processed, otherwise true if no active keyboard.
*/
private _UpdateVKShift(e: KeyEvent): boolean {
let keyShiftState=0;

const lockNames = ['CAPS', 'NUM_LOCK', 'SCROLL_LOCK'] as const;
const lockKeys = ['K_CAPS', 'K_NUMLOCK', 'K_SCROLL'] as const;
const lockModifiers = [ModifierKeyConstants.CAPITALFLAG, ModifierKeyConstants.NUMLOCKFLAG, ModifierKeyConstants.SCROLLFLAG] as const;
Copy link
Member

Choose a reason for hiding this comment

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

These, together with LL417-419, should be grouped together at the top of the module rather than declared at the top of the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


if(!this.activeKeyboard) {
return true;
}

if(e) {
// read shift states from event
keyShiftState = e.Lmodifiers;

// Are we simulating AltGr? If it's a simulation and not real, time to un-simulate for the OSK.
if(this.activeKeyboard.isChiral && this.activeKeyboard.emulatesAltGr &&
(this.modStateFlags & Codes.modifierBitmasks['ALT_GR_SIM']) == Codes.modifierBitmasks['ALT_GR_SIM']) {
keyShiftState |= Codes.modifierBitmasks['ALT_GR_SIM'];
keyShiftState &= ~ModifierKeyConstants.RALTFLAG;
}

// Set stateKeys where corresponding value is passed in e.Lstates
let stateMutation = false;
for(let i=0; i < lockNames.length; i++) {
if((e.Lstates & Codes.stateBitmasks[lockNames[i]]) != 0) {
this.stateKeys[lockKeys[i]] = ((e.Lstates & lockModifiers[i]) != 0);
stateMutation = true;
}
}

if(stateMutation) {
this.emit('statekeychange', this.stateKeys);
}
}

this.updateStates();

if (this.activeKeyboard.isMnemonic && this.stateKeys['K_CAPS'] && (!e || !e.isModifier)) {
// Modifier keypresses don't trigger mnemonic manipulation of modifier state.
// Only an output key does; active use of Caps will also flip the SHIFT flag.
// Mnemonic keystrokes manipulate the SHIFT property based on CAPS state.
// We need to unflip them when tracking the OSK layer.
keyShiftState ^= ModifierKeyConstants.K_SHIFTFLAG;
}
Comment on lines +425 to +431
Copy link
Member

Choose a reason for hiding this comment

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

Is this straight from jsKeyboardProcessor? I just don't quite understand the rationale - shift and caps have different effects on keys (e.g. 1 --> ! / 1 vs a --> A / A for shift and caps respectively)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's from jsKeyboardProcessor:

if(this.activeKeyboard.isMnemonic && this.stateKeys['K_CAPS']) {

Copy link
Member

Choose a reason for hiding this comment

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

Worth reading through the PR and comments that originally added this: #5456

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that code might not be correct - see #15439.


this.layerId = this.getLayerId(keyShiftState);
return true;
}

// TODO-web-core: this could be shared with JsKeyboardProcessor
private updateStates(): void {
const lockKeys = ['K_CAPS', 'K_NUMLOCK', 'K_SCROLL'] as const;
const lockModifiers = [ModifierKeyConstants.CAPITALFLAG, ModifierKeyConstants.NUMLOCKFLAG, ModifierKeyConstants.SCROLLFLAG] as const;
const noLockModifers = [ModifierKeyConstants.NOTCAPITALFLAG, ModifierKeyConstants.NOTNUMLOCKFLAG, ModifierKeyConstants.NOTSCROLLFLAG] as const;

for (let i = 0; i < lockKeys.length; i++) {
const key = lockKeys[i];
const flag = this.stateKeys[key];

// Ensures that the current mod-state info properly matches the currently-simulated
// state key states.
if (flag) {
this.modStateFlags |= lockModifiers[i];
this.modStateFlags &= ~noLockModifers[i];
} else {
this.modStateFlags &= ~lockModifiers[i];
this.modStateFlags |= noLockModifers[i];
}
}
}

/**
* Resets the keyboard context, optionally using the provided text store.
* Clears or reinitializes the context for subsequent keyboard processing.
Expand Down
19 changes: 13 additions & 6 deletions web/src/engine/src/keyboard/keyEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,19 @@ export class KeyEvent implements KeyEventSpec {

get isModifier(): boolean {
switch(this.Lcode) {
case 16: //"K_SHIFT":16,"K_CONTROL":17,"K_ALT":18
case 17:
case 18:
case 20: //"K_CAPS":20, "K_NUMLOCK":144,"K_SCROLL":145
case 144:
case 145:
case Codes.keyCodes.K_SHIFT:
case Codes.keyCodes.K_CONTROL:
case Codes.keyCodes.K_ALT:
case Codes.keyCodes.K_CAPS:
case Codes.keyCodes.K_NUMLOCK:
case Codes.keyCodes.K_SCROLL:
case Codes.keyCodes.K_LSHIFT:
case Codes.keyCodes.K_RSHIFT:
case Codes.keyCodes.K_LCTRL:
case Codes.keyCodes.K_RCTRL:
case Codes.keyCodes.K_LALT:
case Codes.keyCodes.K_RALT:
Copy link
Member

Choose a reason for hiding this comment

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

These do not currently return true, so this is a functional change which would need extensive testing. I suggest we do not add them here unless we actually have a need for them, or else we audit all uses to ensure that adding these does not break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

case Codes.keyCodes.K_ALTGR:
Copy link
Member

Choose a reason for hiding this comment

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

This is not a real AltGr. It's a pseudo AltGr that probably should never be used. See its keyCodes value of 50010. May need some further investigation!

Suggested change
case Codes.keyCodes.K_ALTGR:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this apply to all keycodes > 50000? If so, what are those? And we should probably add a comment to https://github.com/keymanapp/keyman/blob/feat/web/15287_doModifierPress/common/web/types/src/consts/virtual-key-constants.ts#L130

Copy link
Member

Choose a reason for hiding this comment

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

Yes, all the keycodes > 50000 are special key codes used only in touch layouts; called TKeymanWebTouchStandardKey in keymanweb-key-codes.ts (in the kmw compiler, translated from Delphi, hence the initial T):

export const enum
TKeymanWebTouchStandardKey {
K_LOPT = 50001,
K_ROPT = 50002,
K_NUMERALS = 50003,
K_SYMBOLS = 50004,
K_CURRENCIES = 50005,
K_UPPER = 50006,
K_LOWER = 50007,
K_ALPHA = 50008,
K_SHIFTED = 50009,
K_ALTGR = 50010,
K_TABBACK = 50011,
K_TABFWD = 50012
};

These are predefined to have specific behaviours on a touch layout but no effect on a standard layout. The documentation could be improved; see:

As noted above, some `K_xxxx` codes emit characters, if no rule is defined.
There are also some codes which have special functions:
<table class="display">
<thead>
<tr>
<th>Identifier</th>
<th>Meaning</th>
</tr>
</thead>
<tbody>
<tr>
<td markdown="1">`K_ENTER`</td>
<td>Submit a form, or add a new line (multi-line); the key action may vary depending on the situation.</td>
</tr>
<tr>
<td markdown="1">`K_BKSP`</td>
<td>Delete back a single character. This key, if held down, will repeat. It is the only key code which triggers
repeat behavior.</td>
</tr>
<tr>
<td markdown="1">`K_LOPT`</td>
<td>Open the language menu (aka Globe key).</td>
</tr>
<tr>
<td markdown="1">`K_ROPT`</td>
<td>Hide the on screen keyboard.</td>
</tr>
<tr>
<td markdown="1">`K_TAB`, `K_TABBACK`, `K_TABFWD`</td>
<td markdown="1">Move to next or previous element in a form. Note that these key functions are normally
implemented outside the touch layout, so should not typically be used. `K_TAB` will go to previous
element if used with the `shift` modifier.</td>
</tr>
</tbody>
</table>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to virtual-key-constants.ts

return true;
default:
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { assert } from 'chai';
import sinon from 'sinon';
import { KM_Core, km_core_context, km_core_keyboard, km_core_state, KM_CORE_CT, KM_CORE_STATUS, km_core_context_items } from 'keyman/engine/core-adapter';
import { coreurl, loadKeyboardBlob } from '../loadKeyboardHelper.js';
import { Codes, Deadkey, KeyEvent, KMXKeyboard, SyntheticTextStore } from 'keyman/engine/keyboard';
import { Codes, Deadkey, DeviceSpec, KeyEvent, KMXKeyboard, SyntheticTextStore } from 'keyman/engine/keyboard';
import { CoreKeyboardProcessor } from 'keyman/engine/core-processor';

describe('CoreKeyboardProcessor', function () {
Expand Down Expand Up @@ -532,4 +532,87 @@ describe('CoreKeyboardProcessor', function () {
});
}
});

describe('doModifierPress', function () {
// const touchable = true;
const nonTouchable = false;

beforeEach(async function () {
coreProcessor = new CoreKeyboardProcessor();
await coreProcessor.init(coreurl);
state = createState('/common/test/resources/keyboards/test_8568_deadkeys.kmx');
context = KM_Core.instance.state_context(state);
sandbox = sinon.createSandbox();
const coreKeyboard = loadKeyboard('/common/test/resources/keyboards/test_8568_deadkeys.kmx');
coreProcessor.activeKeyboard = new KMXKeyboard(coreKeyboard);
});

afterEach(() => {
sandbox.restore();
sandbox = null;
})

for (const key of [
{ code: Codes.keyCodes.K_SHIFT, name: 'Shift' },
{ code: Codes.keyCodes.K_CONTROL, name: 'Control' },
{ code: Codes.keyCodes.K_ALT, name: 'Alt' },
{ code: Codes.keyCodes.K_CAPS, name: 'CapsLock' },
{ code: Codes.keyCodes.K_NUMLOCK, name: 'NumLock' },
{ code: Codes.keyCodes.K_SCROLL, name: 'ScrollLock' },
// TODO-web-core: should LSHIFT/RSHIFT etc also be detected as modifier?
// Currently .js keyboards don't don't support distinguishing
Copy link
Member

Choose a reason for hiding this comment

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

.js keyboards do support L/R alt and ctrl, but not L/R shift. We don't support L/R shift anywhere in Keyman.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

// between left and right keys, but should KMX keyboards in Web?
// { code: Codes.keyCodes.K_LSHIFT, name: 'LeftShift' },
// { code: Codes.keyCodes.K_RSHIFT, name: 'RightShift' },
// { code: Codes.keyCodes.K_LCTRL, name: 'LeftControl' },
// { code: Codes.keyCodes.K_RCTRL, name: 'RightControl' },
// { code: Codes.keyCodes.K_LALT, name: 'LeftAlt' },
// { code: Codes.keyCodes.K_RALT, name: 'RightAlt' },
// { code: Codes.keyCodes.K_ALTGR, name: 'AltGr'},
]) {
it(`recognizes ${key.name} as modifier`, function () {
// Setup
const keyEvent = new KeyEvent({
Lcode: key.code,
Lmodifiers: 0,
Lstates: Codes.modifierCodes.NO_CAPS | Codes.modifierCodes.NO_NUM_LOCK | Codes.modifierCodes.NO_SCROLL_LOCK,
LisVirtualKey: true,
device: new DeviceSpec('chrome', 'desktop', 'windows', nonTouchable),
kName: key.name
});
keyEvent.source = { type: 'keydown' };

// Execute
const result = coreProcessor.doModifierPress(keyEvent, new SyntheticTextStore(), true);

// Verify
assert.isTrue(result);
});
}

for (const key of [
{ modifiers: 0, name: 'a' },
{ modifiers: Codes.modifierCodes.SHIFT, name: 'A' }
]) {
it(`recognizes ${key.name} not as modifier`, function () {
// Setup
const keyEvent = new KeyEvent({
Lcode: Codes.keyCodes.K_A,
Lmodifiers: key.modifiers,
Lstates: Codes.modifierCodes.NO_CAPS | Codes.modifierCodes.NO_NUM_LOCK | Codes.modifierCodes.NO_SCROLL_LOCK,
LisVirtualKey: true,
device: new DeviceSpec('chrome', 'desktop', 'windows', nonTouchable),
kName: 'K_A'
});
keyEvent.source = { type: 'keydown' };

// Execute
const result = coreProcessor.doModifierPress(keyEvent, new SyntheticTextStore(), true);

// Verify
assert.isFalse(result);
});
}

});
});