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
fix(web): font size was not consistently set #5906
Conversation
Fixes #5779. Fixes #5731 (I believe mitigation is sufficient to close this issue). A variety of interrelated font and font size display issues resolved: 1. KVK font was not applied early enough for size calculations, which meant that we were calculating font scaling per key based on a default font when transforming from the KVK data for desktop devices (defaultLayout.ts) 2. Font scaling for non-default layers was calculated when elements were not visible and had no size information, giving incorrect values. To resolve this, font scaling is now calculated when a layer is made visible, which had performance impacts; resolved by reducing unnecessary `layer.refreshLayout()` calls; see performance point 1 below (oskView.ts:layerChangeHandler()) 3. `getViewportScale()` would return an incorrect scaled value when emulating touch devices on a desktop browser (kmwutils.ts) 4. After switching keyboards, the device-specific scaling factor was not maintained (oskView.ts:refreshLayout()) Related performance improvements: 1. Multiple calls to `layer.refreshLayout()` in `refreshLayout()` have been eliminated, and only the currently visible layer is now refreshed. This dramatically reduces the number of calls to `getIdealFontSize()` which was the primary concern of #5731. (visualKeyboards.ts) 2. Unnecessary use of `innerHTML` replaced with `innerText` (oskBaseKey.ts) Minor Keyman Developer performance improvement: 1. The web debugger no longer recalculates the OSK twice (test.js)
User Test ResultsTest specification and instructions ✅ SUITE_DEVELOPER: Test the web debugger in Keyman Developer
✅ SUITE_COMPAT: Test that these changes are compatible across a range of devices
✅ SUITE_APPS: Test that these changes are compatible in Keyman apps on Android and iOS
Test Artifacts |
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.
lgtm
This is a font and renderer issue which is not trivial to solve. See for example how we fixed this for Lao: https://marc.durdin.net/2015/01/how-to-rendering-combining-marks-consistently-across-platforms-a-long-story/ |
Tracking this in #5926. |
This makes two corrections: 1. Removed an obsolete test for screen size based on dpi, which is unreliable, meant that we had been treating many Android tablets as phones (domManager.ts). 2. Font scale for Android tablets had some incorrect assumptions around keyboard height and devicePixelRatio, which I was able to simplify into a fixed ratio, and was much better across all devices I tested (however, all emulated in Chrome).
I have adjusted the font scaling for Android tablet devices. Can we retest Android devices? The issues with scaling around specific two-part characters such as ោះ is a separate thing to resolve. This is really tricky. I have reviewed those tests where there are multiple dotted circles and PASSED them for this issue; feel free to open a new issue for us to track this as a general problem -- and as you noted the behaviour is different on each platform as well! SUITE_COMPAT
SUITE_APPS
|
SUITE_APPS: Test that these changes are compatible in Keyman apps on Android and iOS
|
This is based on research from various websites, including Google documentation and https://screensiz.es/, as well as various older pages.
Latest push should resolve the tablet detection, I hope. Please retest the relevant SUITE_COMPAT
GROUP_ANDROIDTABLET_CHROME
|
SUITE_COMPAT
Note that there is a weird behavior noted before and had been investigated with @darcywong00. I would like to show it here for our record. Screen.Recording.2021-12-06.at.6.00.49.PM.movGROUP_ANDROIDTABLET_CHROME
One thing that stays the same is that the shift layer of Dzongkha keyboard is not shown correctly. No double dotted circle should be shown. |
Changes in this pull request will be available for download in Keyman version 15.0.165-alpha |
See https://community.software.sil.org/t/feedback-on-keyman-15-0-alpha/5405/34 for details on regression introduced by this PR. |
Fixes #5779.
Fixes #5731 (I believe mitigation is sufficient to close this issue).
A variety of interrelated font and font size display issues resolved:
layer.refreshLayout()
calls; see performance point 1 below (oskView.ts:layerChangeHandler())getViewportScale()
would return an incorrect scaled value when emulating touch devices on a desktop browser (kmwutils.ts)Related performance improvements:
layer.refreshLayout()
inrefreshLayout()
have been eliminated, and only the currently visible layer is now refreshed. This dramatically reduces the number of calls togetIdealFontSize()
which was the primary concern of bug(web):getIdealFontSize
is slow #5731. (visualKeyboards.ts)innerHTML
replaced withinnerText
(oskBaseKey.ts)Minor Keyman Developer performance improvement:
User Testing
I am sorry that we need to run these tests across so many devices. However, there have been repeated issues with font scaling and there is special-case code for each platform, so we do need to do these tests.
SUITE_DEVELOPER: Test the web debugger in Keyman Developer
The web debugger in Developer currently runs only on desktop devices (see #5905). We need to test that the presentation of various Keyman keyboards is still working correctly here. You should not need to use Chrome Developer Tools to run these tests.
SUITE_COMPAT: Test that these changes are compatible across a range of devices
Here we want to check that KeymanWeb now displays the on screen keyboard correctly in a variety of contexts. For these groups, use KeymanWeb Unminified test page:
GROUP_WINDOWS_CHROME: KeymanWeb in Chrome on Windows
GROUP_WINDOWS_FIREFOX: Firefox on Windows
GROUP_MACOS_CHROME: Chrome on macOS
GROUP_LINUX_FIREFOX: Firefox on Linux
GROUP_IPAD_SAFARI: Safari on iPad (not Chrome emulation; iOS simulator okay)
GROUP_IPHONE_SAFARI: Safari on iPhone (not Chrome emulation; iOS simulator okay)
GROUP_ANDROIDPHONE_CHROME: Chrome on Android phone
GROUP_ANDROIDTABLET_CHROME: Chrome on Android tablet
The tests:
SUITE_APPS: Test that these changes are compatible in Keyman apps on Android and iOS
For these groups, use the Keyman app on each device:
The tests: