-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
refactor(web): active element management in relation to OSK display control ✂️ #5644
Conversation
User Test Results✅ SUITE_IN_APP: Host app tests.
✅ SUITE_DOM: Site-based tests.
✅ SUITE_CJK: Tests CJK keyboard functionality |
web/source/dom/domManager.ts
Outdated
get lastActiveElement(): HTMLElement { | ||
return DOMEventHandlers.states._lastActiveElement; | ||
} | ||
|
||
clearLastActiveElement() { | ||
DOMEventHandlers.states.lastActiveElement = null; | ||
set lastActiveElement(Pelem: HTMLElement) { | ||
DOMEventHandlers.states._lastActiveElement = Pelem; | ||
|
||
if(this.lastActiveElement == null && this.activeElement == null) { | ||
this.keyman.osk.hideNow(); // originally from a different one, seemed to serve the same role? | ||
} | ||
} | ||
|
||
getActiveElement(): HTMLElement { | ||
return DOMEventHandlers.states.activeElement; | ||
get activeElement(): HTMLElement { | ||
return DOMEventHandlers.states._activeElement; | ||
} | ||
|
||
_setActiveElement(Pelem: HTMLElement) { | ||
DOMEventHandlers.states.activeElement = Pelem; | ||
set activeElement(Pelem: HTMLElement) { | ||
DOMEventHandlers.states._activeElement = Pelem; | ||
|
||
var isActivating = this.keyman.uiManager.isActivating; | ||
|
||
// Hide the OSK when the control is blurred, unless the UI is being temporarily selected | ||
const osk = this.keyman.osk; | ||
const device = this.keyman.util.device; | ||
if(this.keyman.osk) { | ||
if(!Pelem) { | ||
if(this.keyman.osk && !isActivating) { | ||
this.keyman.osk._Hide(false); | ||
} | ||
} else { | ||
// Force display of OSK for touch input device, or if a CJK keyboard, to ensure visibility of pick list | ||
if(device.touchable) { | ||
osk._Enabled = true; | ||
osk._Show(); | ||
} else { | ||
// Conditionally show the OSK when control receives the focus | ||
if(this.keyman.isCJK()) { | ||
osk._Enabled = true; | ||
} | ||
if(osk._Enabled) { | ||
osk._Show(); | ||
} else { | ||
osk._Hide(false); | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
The biggest win: this area now encompasses all the manners of how the web engine's active-element management controls OSK activation and visibility.
OSK activation and visibility have been noted within #5633 to need significant cleanup; by centralizing the related aspects here, we'll be able to move forward more clearly on the OSK activation & visibility subsystems as a whole. Previously, their entanglement was simply too great.
ab80b75
to
a495bf4
Compare
…factor/web/active-element-integrations
…factor/web/active-element-integrations
…factor/web/active-element-integrations
Can probably reformat TEST_SPECIFIC_KEYBOARDS: so GitHub doesn't link the issue/pull request for |
var keyboardID = this.keyman.core.activeKeyboard ? this.keyman.core.activeKeyboard.id : ''; | ||
var langCode = this.keyman.keyboardManager.getActiveLanguage(); | ||
|
||
if(PInternalName !== undefined && PLgCode !== undefined) { | ||
keyboardID = PInternalName; | ||
langCode = PLgCode; | ||
} | ||
|
||
var lastElem = DOMEventHandlers.states.lastActiveElement; | ||
|
||
if(lastElem && lastElem._kmwAttachment.keyboard != null) { | ||
lastElem._kmwAttachment.keyboard = keyboardID; |
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.
Won't this now modify the parameter lastElem
?
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.
To directly answer your question, yes, it does modify (a property of) the parameter.
That said, that parameter references a KMW-managed property of a persistent, global page element. The parameter is passed to this method for exactly this reason - to perform related updates to the element's KMW-managed properties.
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 think this looks ok, pending user testing 😁
@keymanapp/testers
They are applicable for these pairs - you're just referring to the wrong part of the instructions. The |
SUITE_DOM: Site-based tests.
|
OK, so that last failed user test opened a minor can of worms. There was a "fun" conflict between the focus timer and Android-specific page-scroll handling that simply wasn't ever resolved properly. I think I've figured out how to make the two aspects cooperate, though this did require a few more changes than I'd like. The previous two commits (which accomplished this) may be worth their own review. As may be noted, I did take the opportunity this provided as an excuse to polish up this area dramatically, removing the old ugly |
@keymanapp-test-bot retest SUITE_DOM TEST_NORMAL_USE @mcdurdin Looks like that path wasn't quite covered right: Only the first group's test was reset. So... @keymanapp-test-bot retest SUITE_DOM GROUP_ANDROID_DOM TEST_NORMAL_USE Okay then... SUITE_DOM SUITE_DOM |
... maybe it needs an extra comment for this type? SUITE_DOM SUITE_DOM Edit: there we go. Finally. |
SUITE_DOM: Site-based tests.
|
@jahorton you are pushing that poor little test bot too hard. Currently:
So, I expect you could do: @--keymanapp-test-bot retest SUITE_DOM GROUP_ANDROID_DOM TEST_NORMAL_USE SUITE_DOM GROUP_IOS_DOM TEST_NORMAL_USE (identifier deliberately mangled to prevent interference.) Both of these issues should be resolved but as there is a way forward I'll just encourage you to use the recognised incantation for now -- no time this week... |
Okay, re-reviewing. I've got a few things in my queue so hopefully will get to that this morning. |
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.
Changes LGTM
Actually, he's referring to something else here: keyman/web/source/dom/domEventHandlers.ts Lines 30 to 36 in 3eda9dd
When testing touch-device element attachment, KMW uses this timer to prevent de-focusing selected elements too quickly. I believe that it's been there since before we went open-source. keyman/web/source/dom/domManager.ts Lines 1764 to 1772 in a6289f4
My best guess is that it's there to prevent fat-fingering effects from deselecting an element when the touchpoint is lifted, should the off-element part lift after the on-element part... but this is admittedly conjecture and inference on my part. The block above (and the second comment line) includes fixes to use of this timer, based on said guess. I agree with @MakaraSok that the timer's probably a bit too long, but I didn't want to make that call in this PR. |
SUITE_IN_APP |
SUITE_IN_APP: Host app tests.
SUITE_DOM: Site-based tests.
|
Looks like that bug was addressed on @keymanapp-test-bot retest SUITE_IN_APP GROUP_ANDROID_APP TEST_ROTATE_L-TO-P retest |
SUITE_IN_APP: Host app tests.
|
Changes in this pull request will be available for download in Keyman version 15.0.118-alpha |
Addresses aspects of #5026.
Follows #5633.
Fixes #5026. (Finally!) This PR finally centralizes management of KMW's active element logic and fixes a few niche scenarios that went relatively undetected until now.
One of the worst aspects of "spaghetti code" in relation to the OSK is how entangled some of its display aspects have been with the web engine's active-element management. While this PR doesn't get everything cleaned up perfectly for active-element tracking, its biggest success is that all logic where the two subsystems intersect has now been centralized within the setters of two properties.
While not actually refactoring OSK code in this PR, it's prep work necessary to continue along the lines of the other ✂️
PRs.
User Testing
SUITE_IN_APP: Host app tests.
All behavior exhibited here should feel "normal". These tests are solely to ensure that nothing got accidentally broken.
Platform / keyboard mode combinations
Suite tests
TEST_NORMAL_LOAD: Ensure that the keyboard displays and works correctly when first loaded up.
Be sure to test one or two keystrokes; ensure that the key produces the expected output.
TEST_ROTATE_P-TO-L:
TEST_ROTATE_L-TO-P:
SUITE_DOM: Site-based tests.
All behavior exhibited here should feel "normal". These tests are solely to ensure that nothing got accidentally broken.
OS / Browser combinations
For each of these, make use of the "Tests the new Attachment/Enablement API functionality" test page.
Suite tests
TEST_NORMAL_USE: Ensure that the keyboard displays and works correctly when first loaded up.
TEST_ELEMENT_HOPPING:
TEST_SPECIFIC_KEYBOARDS:
Dynamic area #0.
Dynamic area #1
's "Set to Dzongkha button.Dynamic area #1
. The Dzongkha keyboard should be displayed.Dynamic area #2
.Dynamic area #0.
The French keyboard should be displayed.Dynamic area #2.
The Lao keyboard should be displayed.Dynamic area #1.
The Dzongkha keyboard should be displayed.Dynamic area #0
's "Clear Keyboard" button.Dynamic area #0.
The Lao keyboard should be displayed.SUITE_CJK: Tests CJK keyboard functionality
OS / Browser combinations
Use the standard "Test unminified KeymanWeb" page.
Suite tests
TEST_JAPANESE_TYPING: Verify that a CJK keyboard still works.
Test sequence
japanese
, then click Add.a
. A "picker" displaying a few options should display:2
. The second option should replace the context.TEST_JAPANESE_FOCUS: Verify that a CJK keyboard is shown and hidden properly without interfering with a page's UX.
_TYPING
test).So, fun fact... the
TEST_JAPANESE_FOCUS
test above actually fixes an issue that currently exists both onmaster
and onstable_14.0
that had gone unnoticed until this PR's development.