Skip to content

fix(web): Don't throw errors after detach#10086

Merged
ermshiperete merged 2 commits intomasterfrom
fix/web/detachFromControl
Dec 1, 2023
Merged

fix(web): Don't throw errors after detach#10086
ermshiperete merged 2 commits intomasterfrom
fix/web/detachFromControl

Conversation

@ermshiperete
Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete commented Nov 27, 2023 •
edited
Loading

Fixes #10031.

User Testing

Preparations

TEST_FIXED: follow the repro instructions in #10031 to test if this is fixed. You can use the "KeymanWeb Sample Page - Tests the new Attachment/Enablement API functionality" (in manual mode) instead of the code listed in #10031.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Nov 27, 2023
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Nov 27, 2023 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ TEST_FIXED (PASSED): Tested with the attached KeymanWeb Sample page in the Chrome browser and here is my observation: 1. Followed the steps as mentioned in bug(web): detachFromControl causes subsequent hardware keystrokes to throw an error #10031. 2. When typing in the text area, the Keyman is not active. 3. Then, clicked the Attach button. Again typed in the text area, this time I could see Keyman is active. 4. Clicked the Detach button. Typed again into the text area, this time Keyman was not active. But, I did not see any error message during this process. Seems to be working as expected. (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S27 milestone Nov 27, 2023
@ermshiperete ermshiperete marked this pull request as draft November 27, 2023 17:10
Base automatically changed from fix/web/tests to master November 29, 2023 08:40
@ermshiperete ermshiperete force-pushed the fix/web/detachFromControl branch from c8b1b90 to 2023360 Compare November 29, 2023 11:22
@ermshiperete ermshiperete marked this pull request as ready for review November 29, 2023 15:28
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Nov 29, 2023

These changes look like they address the symptom and are good failsafes to include, but not sure if they are addressing the root cause -- we shouldn't be receiving events on the elements at all once we detach.

jahorton
jahorton reviewed Nov 30, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

IMO, this only masks #10031 - it does not actually fix it. I'd prefer something that actually fixes the root of the problem (in addition to warning about there being a problem if it arises at the point of the current would-be fix).

web/src/app/browser/src/hardwareEventKeyboard.ts
const target = eventOutputTarget(e);
var Levent = preprocessKeyboardEvent(e, this.processor, this.hardDevice);
if(Levent == null) {
if(Levent == null || target == null) {
Copy link
Copy Markdown
Contributor

@jahorton jahorton Nov 30, 2023

Choose a reason for hiding this comment

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

If target == null here, that in itself is an error we should probably log in some manner. The keystroke handler should only activate for elements that are currently in an attached & enabled state.

The attachment engine is supposed to ensure that the handler is only set for such elements, removing an element's event listeners when not attached. I feel like this change only masks the actual error.

Copy link
Copy Markdown
Contributor Author

@ermshiperete ermshiperete Nov 30, 2023

Choose a reason for hiding this comment

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

So should I remove the target == null check again so that we see the error on the console? Or how should we log?

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Nov 30, 2023

IMO, this only masks #10031 - it does not actually fix it

In other words, you are agreeing with my comment 😁

@jahorton
Copy link
Copy Markdown
Contributor

jahorton commented Nov 30, 2023

Thought I'd look for a pointer for you.

This codeblock should be highly-relevant:

keyman/web/src/app/browser/src/hardwareEventKeyboard.ts

Lines 254 to 283 in d084538

page.on('enabled', (Pelem) => {
const target = outputTargetForElement(Pelem);
if(!(target instanceof DesignIFrame)) {
// These need to be on the actual input element, as otherwise the keyboard will disappear on touch.
eventTracker.attachDOMEvent(Pelem, 'keypress', this._KeyPress);
eventTracker.attachDOMEvent(Pelem, 'keydown', this._KeyDown);
eventTracker.attachDOMEvent(Pelem, 'keyup', this._KeyUp);
} else {
const Lelem = target.getElement().contentDocument;
eventTracker.attachDOMEvent(Lelem.body,'keydown', this._KeyDown);
eventTracker.attachDOMEvent(Lelem.body,'keypress', this._KeyPress);
eventTracker.attachDOMEvent(Lelem.body,'keyup', this._KeyUp);
}
});
page.off('disabled', (Pelem) => {
const target = outputTargetForElement(Pelem);
if(!(target instanceof DesignIFrame)) {
eventTracker.detachDOMEvent(Pelem, 'keypress', this._KeyPress);
eventTracker.detachDOMEvent(Pelem, 'keydown', this._KeyDown);
eventTracker.detachDOMEvent(Pelem, 'keyup', this._KeyUp);
} else {
const Lelem = target.getElement().contentDocument;
eventTracker.detachDOMEvent(Lelem.body,'keydown', this._KeyDown);
eventTracker.detachDOMEvent(Lelem.body,'keypress', this._KeyPress);
eventTracker.detachDOMEvent(Lelem.body,'keyup', this._KeyUp);
}
});

For "some reason," the 'disabled' event handler isn't getting called when the 'detach' button from the repro page is clicked. That's the core of the bug.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Nov 30, 2023

Test Results

  • TEST_FIXED (PASSED): Tested with the attached KeymanWeb Sample page in the Chrome browser and here is my observation: 1. Followed the steps as mentioned in bug(web): detachFromControl causes subsequent hardware keystrokes to throw an error #10031. 2. When typing in the text area, the Keyman is not active. 3. Then, clicked the Attach button. Again typed in the text area, this time I could see Keyman is active. 4. Clicked the Detach button. Typed again into the text area, this time Keyman was not active. But, I did not see any error message during this process. Seems to be working as expected.

..Text Area 1

..text area after clicking the Attach button

..text area after clicking the Detach button

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 30, 2023
@ermshiperete ermshiperete merged commit 00ab53e into master Dec 1, 2023
@ermshiperete ermshiperete deleted the fix/web/detachFromControl branch December 1, 2023 14:23
@keyman-server
Copy link
Copy Markdown
Collaborator

keyman-server commented Dec 1, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jahorton jahorton jahorton approved these changes

Assignees

No one assigned

Labels

fix has-user-test web/

Projects

None yet

Milestone

A17S27

Development

Successfully merging this pull request may close these issues.

bug(web): detachFromControl causes subsequent hardware keystrokes to throw an error

5 participants

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