-
-
Notifications
You must be signed in to change notification settings - Fork 130
feat(web): implement doModifierPress ✨ 🎼
#15343
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
base: epic/web-core
Are you sure you want to change the base?
Conversation
User Test ResultsTest specification and instructions User tests are not required |
doModifierPressdoModifierPress 🎼
1bd3aa5 to
283f43f
Compare
5b3ea22 to
878b8ab
Compare
Fixes: #15287 Test-bot: skip
283f43f to
68de61a
Compare
doModifierPress 🎼doModifierPress ✨ 🎼
mcdurdin
left a comment
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.
I am not sure about some of the modifier-related changes here, we should discuss
| 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; | ||
| } |
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.
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)
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.
Yes, that's from jsKeyboardProcessor:
| if(this.activeKeyboard.isMnemonic && this.stateKeys['K_CAPS']) { |
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.
Worth reading through the PR and comments that originally added this: #5456
| case Codes.keyCodes.K_RCTRL: | ||
| case Codes.keyCodes.K_LALT: | ||
| case Codes.keyCodes.K_RALT: | ||
| case Codes.keyCodes.K_ALTGR: |
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.
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!
| case Codes.keyCodes.K_ALTGR: |
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.
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
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.
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):
keyman/developer/src/kmc-kmn/src/kmw-compiler/keymanweb-key-codes.ts
Lines 808 to 822 in 90f5ab6
| 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:
keyman/developer/docs/help/reference/file-types/keyman-touch-layout.md
Lines 106 to 141 in 929b7d7
| 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> |
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.
I added a comment to virtual-key-constants.ts
| 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: |
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.
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.
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.
Removed.
| { 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 |
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.
.js keyboards do support L/R alt and ctrl, but not L/R shift. We don't support L/R shift anywhere in Keyman.
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.
Removed.
Build-bot: skip Test-bot: skip
Fixes: #15287
Test-bot: skip