fix(web): correctly handle cross-origin stylesheets when calculating keyboard size and key cap font size #11472
Conversation
keymanapp-test-bot
bot
added
has-user-test
user-test-required
User Test ResultsTest specification and instructions
Test Artifacts
|
Test Results
Navigate to the "https://help.keyman.com/keyboard/basic_kbdhe220/1.1/basic_kbdhe220" URL on another tab. |
keymanapp-test-bot
bot
removed
the
user-test-required
|
Note: once this lands in 17.0-stable, then we should remove the temporary patch applied in keymanapp/help.keyman.com#1278 |
mcdurdin
changed the title
Well, it hasn't been merged in yet, so it naturally failed - the changes don't exist for the help site.
|
| // For help.keyman.com, sometimes we aren't given a stub for the keyboard. | ||
| // We can't get the keyboard's fonts correct in that case, but we can | ||
| // at least proceed safely. |
There was a problem hiding this comment.
Putting a fix into KeymanWeb specifically for help.keyman.com indicates that we need to:
- Extend the support for different patterns of use such as on help.keyman.com, or
- Fix help.keyman.com to match supported documentation.
The one thing we really shouldn't be doing is adding site-specific patches to Keyman Engine for Web itself. Either it's generally supported or it is not supported.
Or is this a more general comment that just happens to reference help.keyman.com?
There was a problem hiding this comment.
It's citing a specific case that has occurred that can cause KMW to throw errors... when it doesn't really have to do that. We can relax and proceed if we don't worry about having perfect font info.
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
3 similar comments
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
Fixes #11467.
This is intended to be merged together with keymanapp/help.keyman.com#1272, which fixes another facet of the same set of issues.
The issue was arising due to cross-origin stylesheet security. So, a different pattern to detect stylesheet loading is needed. Upon re-examining of the sources that led to the prior solution... much of it was StackOverflow from about a decade back, oriented toward much older browsers than we have today. At the time,
<link>elements didn't consistently supportonloadon all browsers... but that has since been rectified. It appears that we can do this more simply than I previously thought.Additionally, this fixes a follow-up issue that occurred once #11467 was fixed. The way that help.keyman.com loads keyboard-documentation pages (up until now) bypasses use of any keyboard stub; the OSK conditions on that stub and certain properties of it in order to provide font information. In fact, in our initial 17.0 release, it also conditioned application of the SpecialOSK font styling upon this as well! This has been fixed, though keymanapp/help.keyman.com#1272 will also ensure that the information actually is available when run on help.keyman.com keyboard-documentation pages.
User Testing
TEST_DOC_RENDERING: Using the "Tests keyboard documentation rendering" test page, use Developer mode (via F12) and verify that the page loads without displaying any error messages.
We'd also like to verify that this fixes the page mentioned in the base issue (#11467) - https://help.keyman.com/keyboard/basic_kbdhe220/1.1/basic_kbdhe220 - but that will require this PR, keymanapp/help.keyman.com#1271, and keymanapp/help.keyman.com#1272 to be merged in for a proper test.