Skip to content

fix(web): Fix null error with legacy keyboards#10141

Merged
ermshiperete merged 2 commits intomasterfrom
fix/web/10099_null-property
Dec 7, 2023
Merged

fix(web): Fix null error with legacy keyboards#10141
ermshiperete merged 2 commits intomasterfrom
fix/web/10099_null-property

Conversation

@ermshiperete
Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete commented Dec 5, 2023 •
edited
Loading

Fixes #10099.

User Testing

TEST_FIXED:

  • Open the Unminified Source test page
  • Load the Armenian keyboard by typing "armenian" in the "Add a keyboard by keyboard name" field and pressing the "Add" button
  • click in the "Type here" field and switch to the Armenian keyboard
  • Verify that the Armenian OSK is shown, but no error

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Dec 5, 2023
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Dec 5, 2023 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ TEST_FIXED (PASSED): Tested with the attached "Unminified Source Test" page and verified that the armenian OSK appear without showing any error message. (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S27 milestone Dec 5, 2023
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Dec 5, 2023
mcdurdin
mcdurdin reviewed Dec 5, 2023
View reviewed changes
common/web/keyboard-processor/src/keyboards/activeLayout.ts Outdated
K_NUMLOCK: false,
K_SCROLL: false
}));
Lkc.Lcode = KeyMapping._USKeyCodeToCharCode(keyboard.constructNullKeyEvent(null, null));
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin Dec 5, 2023

Choose a reason for hiding this comment

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

Can you explain the reason for this change? I don't understand it

Copy link
Copy Markdown
Contributor

@jahorton jahorton Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm... it is worth noting that the original line doesn't reference an actual key. Personally, when running a trace on what's going on, this block is extremely illuminating:

keyman/common/web/keyboard-processor/src/keyboards/keyboard.ts

Lines 471 to 473 in 4c7b799

constructKeyEvent(key: ActiveKey, device: DeviceSpec, stateKeys: StateKeyMap): KeyEvent {
// Make a deep copy of our preconstructed key event, filling it out from there.
const Lkc = key.baseKeyEvent;

Note that we're within the constructBaseKeyEvent method (for this comment). L473 above is relying upon the results of said method. This is an obvious problem.

Since this hasn't exactly been a section of code I've actually dived into much, perhaps a comparison against stable-10.0 is in order. I've moved the code a few times since then, so I'll highlight the corresponding section to move things along:

keyman/web/source/kmwdomevents.ts

Lines 722 to 734 in f16872a

if(typeof(activeKeyboard['KM'])=='undefined' && !(Levent.Lmodifiers & 0x60)) {
// Support version 1.0 KeymanWeb keyboards that do not define positional vs mnemonic
var Levent2: com.keyman.LegacyKeyEvent = {
Lcode:this.keyman.keyMapManager._USKeyCodeToCharCode(Levent),
Ltarg:Levent.Ltarg,
Lmodifiers:0,
LisVirtualKey:0
};
if(kbdInterface.processKeystroke(util.physicalDevice, Levent2.Ltarg, Levent2)) {
LeventMatched=1;
}
}

(Note that this is constructing the event in full from nothing, rather than the partial construction intended for constructBaseKeyEvent.)

So... KMW 1.0 used different key-code values, and we just need to map the key code we use now to the one that was used then. It's not even like we're replacing the other key-event values... just .Lcode here and .LisVirtualKey on the next line.

Something far closer to the original would likely be best here. I'm confident that we don't even need to create an intermediary event object... and that'll let us bypass the need for the other change - the optional-based null guard.

Copy link
Copy Markdown
Member

@mcdurdin mcdurdin Dec 6, 2023

Choose a reason for hiding this comment

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

this block is extremely illuminating

Not to me! Perhaps you could explain it more clearly for me?

Copy link
Copy Markdown
Contributor

@jahorton jahorton Dec 6, 2023

Choose a reason for hiding this comment

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

Note that we're within the constructBaseKeyEvent method (for this comment). L473 above is relying upon the results of said method. This is an obvious problem.

In other words, we have an impossible-to-meet precondition here. We're within a method that must complete to satisfy the precondition of a method it's calling.

Copy link
Copy Markdown
Contributor

@jahorton jahorton Dec 6, 2023

Choose a reason for hiding this comment

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

Fortunately, we can simplify things greatly here... that's what the rest of the analysis is about.

jahorton
jahorton requested changes Dec 6, 2023
View reviewed changes
common/web/keyboard-processor/src/keyboards/activeLayout.ts Outdated
K_NUMLOCK: false,
K_SCROLL: false
}));
Lkc.Lcode = KeyMapping._USKeyCodeToCharCode(keyboard.constructNullKeyEvent(null, null));
Copy link
Copy Markdown
Contributor

@jahorton jahorton Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm... it is worth noting that the original line doesn't reference an actual key. Personally, when running a trace on what's going on, this block is extremely illuminating:

keyman/common/web/keyboard-processor/src/keyboards/keyboard.ts

Lines 471 to 473 in 4c7b799

constructKeyEvent(key: ActiveKey, device: DeviceSpec, stateKeys: StateKeyMap): KeyEvent {
// Make a deep copy of our preconstructed key event, filling it out from there.
const Lkc = key.baseKeyEvent;

Note that we're within the constructBaseKeyEvent method (for this comment). L473 above is relying upon the results of said method. This is an obvious problem.

Since this hasn't exactly been a section of code I've actually dived into much, perhaps a comparison against stable-10.0 is in order. I've moved the code a few times since then, so I'll highlight the corresponding section to move things along:

keyman/web/source/kmwdomevents.ts

Lines 722 to 734 in f16872a

if(typeof(activeKeyboard['KM'])=='undefined' && !(Levent.Lmodifiers & 0x60)) {
// Support version 1.0 KeymanWeb keyboards that do not define positional vs mnemonic
var Levent2: com.keyman.LegacyKeyEvent = {
Lcode:this.keyman.keyMapManager._USKeyCodeToCharCode(Levent),
Ltarg:Levent.Ltarg,
Lmodifiers:0,
LisVirtualKey:0
};
if(kbdInterface.processKeystroke(util.physicalDevice, Levent2.Ltarg, Levent2)) {
LeventMatched=1;
}
}

(Note that this is constructing the event in full from nothing, rather than the partial construction intended for constructBaseKeyEvent.)

So... KMW 1.0 used different key-code values, and we just need to map the key code we use now to the one that was used then. It's not even like we're replacing the other key-event values... just .Lcode here and .LisVirtualKey on the next line.

Something far closer to the original would likely be best here. I'm confident that we don't even need to create an intermediary event object... and that'll let us bypass the need for the other change - the optional-based null guard.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Dec 6, 2023

Test Results

  • TEST_FIXED (PASSED): Tested with the attached "Unminified Source Test" page and verified that the armenian OSK appear without showing any error message.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Dec 6, 2023
@github-actions github-actions bot added web/ and removed web/ labels Dec 6, 2023
@ermshiperete
fix(web): Fix null error with legacy keyboards
38c7839 
This uses a different approach in response to code review.

Fixes #10099.
@ermshiperete ermshiperete force-pushed the fix/web/10099_null-property branch from 2c5eb6e to 38c7839 Compare December 6, 2023 15:45
@github-actions github-actions bot added web/ and removed web/ labels Dec 6, 2023
mcdurdin
mcdurdin reviewed Dec 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

I will leave this to @jahorton to review because there's a whole lot of detail I'm not understanding in the previous thread here

@darcywong00
Copy link
Copy Markdown
Contributor

darcywong00 commented Dec 7, 2023

When this is resolved, we'll want to 🍒 pick to stable-16.0 so keymanweb.com gets the fix.

@ermshiperete ermshiperete merged commit 289e075 into master Dec 7, 2023
@ermshiperete ermshiperete deleted the fix/web/10099_null-property branch December 7, 2023 07:13
@ermshiperete ermshiperete mentioned this pull request Dec 7, 2023
Merged
@keyman-server
Copy link
Copy Markdown
Collaborator

keyman-server commented Dec 7, 2023

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

@ermshiperete ermshiperete mentioned this pull request Dec 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcdurdin mcdurdin mcdurdin left review comments

@jahorton jahorton jahorton approved these changes

Assignees

No one assigned

Labels

common/web/ common/ fix has-user-test web/

Projects

None yet

Milestone

A17S27

Development

Successfully merging this pull request may close these issues.

bug(web): Cannot read properties of null (reading 'baseKeyEvent')

6 participants

@ermshiperete @bharanidharanj @darcywong00 @keyman-server @mcdurdin @jahorton