refactor(web): OSK spacebar-label updates now managed by layer object 🪠 #11175
Conversation
keymanapp-test-bot
bot
added
has-user-test
user-test-required
User Test Results |
jahorton
changed the title
Test Results
..Android 5 
..Android 7.1 
..Android 9.0 
..Android 12 
|
keymanapp-test-bot
bot
added
user-test-failed
and removed
user-test-required
I'm going to call this a pass, but I really can't blame you for reporting it this way. TL,DR: technical limitations with older Chrome versions. I forgot about this aspect and should have mentioned to only test it with "the latest version of Android" and/or "versions of Android 12+" or similar. TEST_ANDROID_SPACEBAR: PASS The reason: the technique we use to ensure that the key height stays within the spacebar's boundary isn't available to us in old versions of Chrome. Per https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics#browser_compatibility, we need a minimum of Chrome 87 in order to scale based on key-height. (See the Re: Android 7.1.1, I found a link suggesting that Android 7 shipped with Chrome 58. For Android 9.0, APKMirror offers Chrome 73. As Android Studio emulators typically start out with the minimum possible Chrome version, this explains why the height scaling wasn't working. So, a couple of things of note:
|
|
Changes in this pull request will be available for download in Keyman version 17.0.305-beta |
| if (typeof (tButton.className) == 'undefined' || tButton.className == '') { | ||
| tButton.className = 'kmw-spacebar'; | ||
| } else if (tButton.className.indexOf('kmw-spacebar') == -1) { | ||
| tButton.className += ' kmw-spacebar'; | ||
| } |
There was a problem hiding this comment.
| if (typeof (tButton.className) == 'undefined' || tButton.className == '') { | |
| tButton.className = 'kmw-spacebar'; | |
| } else if (tButton.className.indexOf('kmw-spacebar') == -1) { | |
| tButton.className += ' kmw-spacebar'; | |
| } | |
| tButton.classList.add('kmw-spacebar'); |
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
| if (spacebarLabel.className != 'kmw-spacebar-caption') { | ||
| spacebarLabel.className = 'kmw-spacebar-caption'; | ||
| } |
There was a problem hiding this comment.
| if (spacebarLabel.className != 'kmw-spacebar-caption') { | |
| spacebarLabel.className = 'kmw-spacebar-caption'; | |
| } | |
| spacebarLabel.className = 'kmw-spacebar-caption'; |
No need to guard this ... I am sure it'll already be optimized.
| const spacebarLabel = this.spaceBarKey.label; | ||
| let tButton = this.spaceBarKey.btn; | ||
|
|
||
| if (typeof (tButton.className) == 'undefined' || tButton.className == '') { | ||
| tButton.className = 'kmw-spacebar'; | ||
| } else if (tButton.className.indexOf('kmw-spacebar') == -1) { | ||
| tButton.className += ' kmw-spacebar'; | ||
| } | ||
|
|
||
| if (spacebarLabel.className != 'kmw-spacebar-caption') { | ||
| spacebarLabel.className = 'kmw-spacebar-caption'; | ||
| } |
There was a problem hiding this comment.
Or how about this ... so much shorter:
| const spacebarLabel = this.spaceBarKey.label; | |
| let tButton = this.spaceBarKey.btn; | |
| if (typeof (tButton.className) == 'undefined' || tButton.className == '') { | |
| tButton.className = 'kmw-spacebar'; | |
| } else if (tButton.className.indexOf('kmw-spacebar') == -1) { | |
| tButton.className += ' kmw-spacebar'; | |
| } | |
| if (spacebarLabel.className != 'kmw-spacebar-caption') { | |
| spacebarLabel.className = 'kmw-spacebar-caption'; | |
| } | |
| this.spaceBarKey.btn.classList.add('kmw-spacebar'); | |
| this.spaceBarKey.label.className = 'kmw-spacebar-caption'; |
| **/ | ||
| showLanguage(displayName: string) { | ||
| if(!this.spaceBarKey) { | ||
| return () => {}; |
There was a problem hiding this comment.
What's this about? Nothing else returns anything?
| spacebarLabel.innerText = displayName; | ||
| } | ||
| } | ||
| catch (ex) { } |
There was a problem hiding this comment.
Why are we catching everything here?
| } | ||
|
|
||
| public refreshLayout(vkbd: VisualKeyboard, layoutParams: LayerLayoutParams) { | ||
| const layerHeight = layoutParams.keyboardHeight; |
To prepare to optimize layout-reflow for Web's OSK, it will help us to rework how spacebar-text and its styling are managed. They'll be far easier to "batch" with other layout updates if managed by the layer containing the key, rather than set outside of the standard flow.
Also, we had a lot of redundant calls to the
showLanguage()function, which is what updated the spacebar. We only need one.This PR has been spun off of an older state of #11140. Certain design decisions may make more sense with the whole picture in mind, though I have tried to recontextualize them to make better sense in isolation for this PR.
User Testing
TEST_ANDROID_SPACEBAR: Using Keyman for Android, verify that the spacebar is always appropriately captioned (based on user settings in the menu) for the active keyboard, with the text properly sized.
balochi_phonetic- it has very high font scaling that will help stress-test part of spacebar-display management.