fix(web): better element inputmode management 📴#7395
Conversation
keymanapp-test-bot
Bot
added
the
user-test-missing
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
mcdurdin
left a comment
mcdurdin
left a comment
There was a problem hiding this comment.
I appreciate this design. You are trying to prevent inputMode from being overwritten by other Javascript code, which would stop Keyman from working.
My only real concern is that while we are preventing the symptoms of a problem here, the other code is not going to expect that setting inputMode can ever "fail" (from their perspective), and so web site developers may still end up with potentially strange behaviours.
So ... whether we go with an observer (which is more complex and more potential for maintenance pain...), or with the naive save/restore approach which was in my mind originally, we're going to need to document this interaction for website devs.
Hope that makes sense! All that said, I'm happy with whatever you decide. Just wanted to flag the potential for weirdness.
Not just that - long-term perspective, it'll store updates to the field's intended I wonder if setting |
Yes. We can't develop in a vacuum. Better to give site developers the tools to connect the dots between their frameworks and Keyman input method than to try and 'magically' make it work ... magic will only get us so far, and then it breaks badly! |
mcdurdin
left a comment
mcdurdin
left a comment
There was a problem hiding this comment.
So, a bunch of minor bit and pieces. At first I thought it was all fine, but I think there is potentially an issue on line 330 of domManager.ts; everything else is just cleanup.
| } | ||
| }.bind(this); | ||
|
|
||
| _InputModeObserverCore = function(this: DOMManager, mutations: MutationRecord[]) { |
There was a problem hiding this comment.
Can you add a comment explaining the purpose of this function? Also I think we should be naming it _inputModeObserverCore?
There was a problem hiding this comment.
The other observer core functions have similar capitalization:
_AutoAttachObserverCore_EnablementMutationObserverCore
Should I change them, too?
There was a problem hiding this comment.
Should I change them, too?
It's probably good sanitisation to iteratively cleanup to javaScript camelCasing preference, so yeah, if it's not too big a yak shave, sure!
| @@ -233,7 +241,6 @@ namespace com.keyman.dom { | |||
|
|
|||
| if(!this.isAttached(Pelem)) { | |||
| this.setupElementAttachment(Pelem); | |||
There was a problem hiding this comment.
This seems to be done again in enableInputElement(), called on line 247, albeit in a more nuanced manner (handling iframes)
keymanapp-test-bot
Bot
removed
the
user-test-missing
|
Changes in this pull request will be available for download in Keyman version 16.0.82-alpha |
3 participants
Refer to #7343 (comment).
If a website designer would normally be specifying input mode for elements that KMW will be managing, we should put forth a good-faith effort to remember what the page's original settings were. At some point, we might even go so far as to support 'intent' behavior in KMW... but not this release cycle.
As proper user-testing would be a lot of work to facilitate here, for now...
@keymanapp-test-bot skip