Fix print button so fonts are loaded before printing - #2302
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2302 +/- ##
==========================================
+ Coverage 43.20% 43.54% +0.33%
==========================================
Files 601 602 +1
Lines 25757 25759 +2
Branches 8574 7665 -909
==========================================
+ Hits 11128 11216 +88
+ Misses 14580 14494 -86
Partials 49 49 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I'm not particularly worried about the case where it times out because of exceptionally slow connection. That's a rare case and no worse than the current behaviour anyway. I'm also not particularly worried about printing being initiated externally such as via ctrl-p - this is already less feature-rich since it doesn't support our with/without hints menu. It's nice that it works, but it's not our officially supported avenue.
Ultimately, I think this is good and could go in as-is with the improvements in my comments made later - but since we're waiting on a release anyway, and since this wasn't critical priority so it can wait, we should do those improvements now.
| async function printWithFontsReady() { | ||
|
|
||
| const loads = Array.from(document.fonts).map(font => (font.status === "unloaded" ? font.load() : font.loaded ).catch(() => undefined)); |
There was a problem hiding this comment.
| async function printWithFontsReady() { | |
| const loads = Array.from(document.fonts).map(font => (font.status === "unloaded" ? font.load() : font.loaded ).catch(() => undefined)); | |
| async function printWithFontsReady() { | |
| const loads = Array.from(document.fonts).map(font => (font.status === "unloaded" ? font.load() : font.loaded).catch(() => undefined)); |
Not a big deal since its just code-style, but quick note to pay attention to whitespace. We tend to not start functions with an empty line (curiously PrintButton below is a rare exception, since it's untouched 6 year old code), and spacing should be matched on either side of a bracket pair like after font.loaded.
| const loads = Array.from(document.fonts).map(font => (font.status === "unloaded" ? font.load() : font.loaded ).catch(() => undefined)); | ||
|
|
||
| await Promise.race([ | ||
| Promise.all(loads).then(()=> document.fonts.ready), |
There was a problem hiding this comment.
| Promise.all(loads).then(()=> document.fonts.ready), | |
| Promise.all(loads).then(() => document.fonts.ready), |
Another whitespace spot. A => should have spacing either side.
There was a problem hiding this comment.
I see why this happened - intelliJ puts a very helpful type hint next to the =>, and at first glance I couldn't see there wasn't any space. Fixed in the newest commit and will be more careful for next time.
| // Function that goes through all fonts on the page, waits for them to load/fail, and then prints the page | ||
| async function printWithFontsReady() { | ||
|
|
||
| const loads = Array.from(document.fonts).map(font => (font.status === "unloaded" ? font.load() : font.loaded ).catch(() => undefined)); |
There was a problem hiding this comment.
This is definitely the right idea, but I think there is a mild improvement to be made to it.
document.fonts contains the set of ALL fonts that may be used on the page according to our (and our libraries') @font-face rules, not just the ones actually in-use. We therefore load many fonts unnecessarily (you can see this on the dev tools Network tab when pressing the print button).
We can instead query the body of the page for the list of fonts actually used by our elements. Something like the following, where this new fonts list would then take the place of document.fonts in your function.
const fonts = new Set<string>();
for (const element of document.body.querySelectorAll('*')) {
fonts.add(getComputedStyle(element).font);
}(You can check back against the Network tab and see that this now requests the same fonts that are already requested when printing on the live site, but with the benefit you added of awaiting these loads before the print)
| onClick={() => { | ||
| dispatch(printingSettingsSlice.actions.enableHints(true)); | ||
| setTimeout(window.print, 100); | ||
| setTimeout(printWithFontsReady, 100); | ||
| }} |
There was a problem hiding this comment.
We could probably combine this whole structure into a function now that one exists to cut down on code reuse - especially with the two layers of setTimeout making things slightly confusing anyway.
It's a subtle difference, but what we want really want from a timeout before printing is [minimum: 100ms/maximum: 2000ms], rather than [maximum: 2000ms] + 100ms (otherwise we're just needlessly waiting 100ms much of the time).
Since this 100ms wait is specifically to give time for the DOM to update from any hint changes, it makes sense for the to be in the same function too.
This would neatly reduce each function call to something like:
onClick={() => printWithHintsAndLoadedFonts(dispatch, true)}
sjd210
left a comment
There was a problem hiding this comment.
Looks good! I particularly like the use of requestAnimationFrame() to check that hints have been rendered. It makes the code much more intuitive👍
Basically, the problem was that the browser wouldn't load all fonts because they were in an accordion, and it loaded them lazily. When you clicked the print button, it opened the accordions, saw that they are not loaded, and sent a request, but the print function would execute before the request came back, thus sometimes some symbols would be missing, which is also why the second time you click "print" it worked.
What I did was just make a new function, that goes through all fonts, loads them, and then prints. There is a timeout that I have set at 2 seconds, could be less or more. We might need to play with that as when I tested a fully loaded page, opened the network tools tab, set it to 3G, it couldn't load the fonts in time for 2 seconds, so it timed out.
This also won't work if a user presses Control+P or does the in-browser print button - we could be just preloading the fonts at load anyway, if we care about that.