fix(web): OSK resource-loading promise's font-wait ⛲ #10506
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
|
Given this changes significant core functionality in the font loading, I think we need user tests. |
| s=s+'src:url(\''+ttf+'\') format(\'truetype\');'; | ||
| } else { | ||
| return null; | ||
| source = "url('"+ttf+"') format('truetype')"; | ||
| } | ||
| } else { | ||
| var s0 = []; |
There was a problem hiding this comment.
| var s0 = []; |
Is s0 used any more?
There was a problem hiding this comment.
... I thought I did remove it; good catch.
| // with embedded ttf or woff. svg mostly works so is a better initial | ||
| // choice on the Android browser. | ||
| if(svg != '') { | ||
| s0.push("url('"+svg+"') format('svg')"); | ||
| source = "url('"+svg+"') format('svg')"; | ||
| } |
There was a problem hiding this comment.
SVG is dead for fonts. We shouldn't be perpetuating if we are changing anyway.
l.161 references Android 4.2, 4.3 which we no longer support. We need to clean that up too.
And I see a reference to EOT also. Even deaderer
| s=s+'src:url(\''+ttf+'\') format(\'truetype\');'; | ||
| } else { | ||
| return null; | ||
| source = "url('"+ttf+"') format('truetype')"; |
There was a problem hiding this comment.
Since we are fixing this up should we be escaping the ttf string for safety?
| const fontFace = new FontFace(fd.family, source); | ||
| const loadPromise = fontFace.load(); | ||
| this.fontPromises.push(loadPromise); | ||
| loadPromise.then(() => this.fontPromises = this.fontPromises.filter((entry) => entry != loadPromise)); |
There was a problem hiding this comment.
Wouldn't it be cleaner to just clear fontPromises after allSettled() on l.62 or so?
There was a problem hiding this comment.
There's technically a chance for another stylesheet to come in at a later point that would need a Promise not currently included. It's best not to assume that there will only be one call to the allLoadedPromise() method - largely because, in #10507, documentation-keyboards do end up needing a separate call to the method. (Though... I might actually be able to restructure that a bit.)
To me, this is the cleanest way - each Promise ensures that it's only included in the allLoadedPromise when it's actually still loading, bowing out as soon as it's completed. If new ones rotate in and old ones rotate out in the meantime, that all comes out in the wash with the current approach. That is, so long as I add .catch handling too - that part is certainly an important "catch" here.
There was a problem hiding this comment.
Secondary, do Promise.all() (and cousins) handle the scenario of the array changing while they are waiting?
| @@ -197,15 +228,15 @@ export class StylesheetManager { | |||
| * | |||
| * @param {string} href path to stylesheet file | |||
| */ | |||
| linkExternalSheet(href: string): void { | |||
| linkExternalSheet(href: string, force?: boolean): HTMLStyleElement { | |||
There was a problem hiding this comment.
There's a change to this function signature but the change doesn't seem to be used?
There was a problem hiding this comment.
It comes into play in the followup - #10507. Admittedly, the two were part of a single batch of changes at first, with me splitting them into separate chunks later.
If need be, I can undo it here and re-implement in that PR, which is a direct follow-up.
There was a problem hiding this comment.
no that's fine, I see the changes there.
| // We've already linked this stylesheet, don't do it again | ||
| return; | ||
| return null; | ||
| } | ||
| } catch(e) { | ||
| // We've built an invalid href, somehow? |
There was a problem hiding this comment.
As I look at this a bit more, I think it's a bit of hold-over from when we used to always attach kmwosk.css to the document's head. In 17.0, it's actually set within the OSK's element hierarchy... and its link gets cleared when the keyboard is changed.
As such, before, it was used to avoid having a high number of identical kmwosk.css links in the head... but we keep that relatively under control now.
OK... so... how would I define user tests in a way that would be sufficiently clear? That's the main reason I didn't spec any - setup instructions aren't hard, but setting the expectations clearly, in an unambiguous way... that's where the trick lies. |
Agree this is tricky. Need to find an example (or craft one) where the font change is obvious I guess? |
|
Requesting a re-review given that the related user test seen within #10534 has passed. |
|
Changes in this pull request will be available for download in Keyman version 17.0.257-alpha |
Enhances the
Promisesetup used to await OSK resource loading to also properly wait for a keyboard's fonts to load.Rather than use techniques from KMW's past, I've instead utilized a (comparatively) newer Web feature:
FontFace. It's newer than when the setup we previously used was first written, but it's supported on all browser versions KMW currently supports and provides a much cleaner way to achieve the same goals - it even directly provides a Promise for font-loading completion!This changeset includes a tweak to ensure that there's always a layout-refresh once the keyboard's fonts load; what's small in one font can be large in another, after all.
This admittedly corresponds better to the original design/logic behind similar handling in KMW 15.0 and before than what it's replacing from 16.0.
For comparison, in versions of KeymanWeb up to and including 15.0, when a keyboard's stylesheets were added, we'd wait for font loads like this:
keyman/web/source/osk/visualKeyboard.ts
Lines 1365 to 1368 in 2dcc114
That method's comments & signature:
keyman/web/source/osk/visualKeyboard.ts
Lines 1725 to 1731 in 2dcc114
We dropped this technique when we dropped the use of touch-alias elements.
@keymanapp-test-bot skip
See #10534; the best user test I could come up with is implemented therein.