-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
[Web, iOS, Android] Undisplayed diacritic fix #1407
Conversation
Looking for test cases for you... Lorna found one on iPad: |
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.
This looks really good.
Given the fairly broad impact this change has, I'd like to make sure we capture before-and-after screenshots in this PR. can we make sure we test against:
- iOS 10, iOS 12
- Android 4.4, 5.0, latest-you-can-get
- Safari, Chrome, Firefox on target platforms
There will need to be a related PR for the popup keys on Android and iOS in the native apps given those are not implemented by KMW.
I understand that this doesn't touch the desktop OSK? Is that listed as a separate piece of work?
web/source/kmwkeyboards.ts
Outdated
@@ -784,6 +784,11 @@ namespace com.keyman { | |||
return ((lg == 'cmn') || (lg == 'jpn') || (lg == 'kor')); | |||
} | |||
|
|||
isRTL(k0?: any|KeyboardStub): boolean { |
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.
Why type any
?
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.
Right. I got a bit lazy and copied isCJK
's type definition, which wasn't quite correct. The surprise here is that any
is the correct type at present. Line 93 of kmwkeyboards.ts:
activeKeyboard: any; // We might can define a more precise def, though...
It never got a fully-precise refactor, as keyboard objects have a decidedly different structure from their stubs. While we'd obviously like to remedy this eventually, I'm pretty sure that's beyond the scope of this PR.
web/source/kmwkeyboards.ts
Outdated
getFont(k0?) { | ||
var k = k0 || this.activeKeyboard; | ||
|
||
if(k['KV']) { |
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.
What if k0
is undefined
and this.activeKeyboard
is null
?
This change does affect the desktop OSK, since it's modifying the new parent class method implemented by #1403. You can see a small change before-and-after with Khmer Angkor with Windows 10, actually. (macOS Mojave doesn't appear to need the fix for this specific case.) Using the current version of Chrome (or Firefox)... Before: After: Obviously, the rendering's not exactly perfect, but it does affect the coeung marker's key. |
Also, good point about the in-app popup keys. |
I can confirm that this fixes this particular case, which also affects iOS 12. Perhaps I don't understand the diacritic correctly, but its resulting layout does look a bit strange. Using the iOS Simulator for "iPad 2 - 9.3": Before After Given how the diacritic displays when used for output, I'm leaning toward saying its display is 'correct', or at least as 'correct' as it can be made, given that it seems to intentionally 'combine' / 'overlay' with the following character. #1421 (the PR actually used for the screenshot - note the 'zwnj' key) doesn't fix this particular case perfectly... at least not yet. Probably something due to the diacritic's nature. |
And found a decent enough example for Android, using the cloud-default Thai keyboard (Thai Kedmanee): Using emulated Android 5.0: Before After While the diacritics did display in the "before" sample, note that they weren't properly aligned within the key - they were effectively being joined to an empty "previous character" of some sort by the renderer, and thus reported text width of zero. While the keyboard doesn't display properly on Android 4.4 due to not using an SVG font, it amusingly does attempt to use the same solution via font fallback: The classic Unicode circle does not appear on |
Unfortunately, I can't use the same test for IE. Unlike the others... it appears that its renderer automatically adds |
static getTextWidth(text: string, font: string) { | ||
// re-use canvas object for better performance | ||
var canvas = OSKKey.getTextWidth['canvas'] || (OSKKey.getTextWidth['canvas'] = document.createElement("canvas")); | ||
var context = canvas.getContext("2d"); |
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.
Is it possible and/or desirable to cache context as well?
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 code example I drew from didn't seem to think so.
[Web] Keyboard text auto-scaling
Addresses the web and web-based mobile app components of #1070.