RS-22108: Wait for fonts before laying out the heatmap

[JustinCCYap]

Fixes RS-22108, and its duplicate RS-23543: heatmap axis labels are shifted and truncated in a PPT or PDF export when the document uses a custom font, intermittently and not reproducibly.

Diagnosis

The chart sizes its axis bands by measuring throwaway <text> elements in the live DOM (rhtmlLabelUtils getSingleLineLabelDimensions, called from theSrc/scripts/lib/components/parts/labelUtilsWrapper.js:52, consumed at yAxis.js:34-38 and xAxis.js:59-75), and it never re-measures.

In the Displayr export page the custom fonts arrive through an asynchronous CSS @import, so the chart can lay out with fallback font metrics. The widget then reported itself ready synchronously, and rhtmlwidget-status is the only readiness signal the export screenshot waits on. The custom font swaps in after layout, leaving the labels in bands sized for the wrong font, so they shift, truncate with an ellipsis, or are clipped at the SVG edge.

On screen the problem is usually invisible because any resize triggers a full re-render and re-measure (rhtmlHeatmap.factory.js:29-32). An export renders once, which is why only the export is affected, and why identical steps give different results: it is a race against the font load.

Solution

The chart now waits for the fonts it draws with before it measures anything.

1. New theSrc/scripts/lib/fonts.js. fontFamiliesInUse(options) returns the distinct values of every *_font_family option, so no hardcoded list of components is needed. waitForFonts(options) calls document.fonts.load() for each family in normal and bold, then awaits document.fonts.ready, bounded by a 3s timeout so an unloadable font can only delay the chart, never prevent it. The explicit load() matters: fonts.ready alone can resolve before a face that nothing has rendered yet is fetched.
2. heatmapOuter.js gates rendering on Promise.all([loadImage(image), waitForFonts(options)]), so fonts are awaited alongside the image data and cost no extra time when already available.
3. heatmapOuter.js claims rhtmlwidget-status as loading before the asynchronous work starts. It was previously only claimed inside the Heatmap constructor, which runs after the image load, so during the wait an export would see a widget that is not loading and could screenshot an empty SVG.
4. heatmapOuter.js reports ready on the error path too, so a failed render cannot leave an export waiting out its screenshot timeout.

Two hardening changes came out of reviewing the above, both of which matter only because step 3 widened the window between claiming loading and reporting ready:

5. A fonts.load() call is guarded against a synchronous throw. waitForFonts is evaluated as an argument to Promise.all, so a throw would escape before the chain has a catch and leave the status at loading; Blink throws rather than rejects when it cannot parse a font shorthand.
6. Both status writes check that this render's svg is still in the container. A resize re-renders without cancelling the render it interrupts, so an older chain could otherwise settle and mark a newer, unfinished chart ready.

Included refactor

heatmapcore no longer writes rhtmlwidget-status on its container — it was reaching outside itself to set an attribute on its parent, which left the claim in one file and the release in another. All three transitions now sit in heatmapOuter.js, the code that owns the render. Behaviour and timing are unchanged, and heatmapOuter.js is the only caller of heatmapcore anywhere in the Displayr org.

Tests

theSrc/scripts/lib/fonts.jest.test.js covers the option collection and the failure paths that must not stop a chart rendering: an unloadable font, a synchronous throw from load, a font set with no load method, no document.fonts at all, and a font set that never becomes ready. gulp testSpecs is 11 green.

Not unit covered, and worth a reviewer's eye: the status lifecycle and the stale-render guard in heatmapOuter.js, which needs a DOM, Image and canvas to exercise and has no harness in this repo. The visual regression suite waits on div[rhtmlwidget-status=ready], so a broken ready transition would show up there as timeouts.

Warning

Author: before requesting review, attach evidence the bug is fixed.
A screenshot, screen recording, test log, or step-by-step repro showing the
original failure no longer happens. A reviewer cannot tell whether this PR
actually fixes the ticket from the diff alone - make it easy for them.

🤖 Generated with Claude Code

[chschan]

Reviewed origin/master...origin/RS-22108. The shape of the fix is right — claim loading synchronously before the async chain, wait on document.fonts alongside the image load rather than after it, and gate the status write on isCurrentRender() so a stale resize chain can't mark a newer chart ready. fonts.jest.test.js covers fonts.js close to exhaustively, and I verified the rebuilt inst/htmlwidgets/rhtmlHeatmap.js matches source.

Two correctness comments inline, plus two testing points below. The inline ones are the two I'd want fixed before merge, since each leaves the widget in a worse state than pre-PR on its own failure path.

---

Add a heatmapOuter.jest.test.js

fonts.js is well covered; heatmapOuter.js has no coverage at all, and it holds the three new behaviours — loading claimed before the chain, ready on the catch path, and the isCurrentRender() gating. That last one is the subtlest logic in the PR and nothing asserts either half of it (that the live render does report ready, and that a superseded one doesn't).

This does not need puppeteer. With the require conversion from the inline comment, it's a plain gulp testSpecs test — jest.mock the heatmapcore, stub global.Image, assert against the attribute. I've confirmed this passes as-is on jest 26 / jsdom / Node 22:

jest.mock('./lib/heatmapcore/heatmapcore', () => function Heatmap () {})
const heatmapOuter = require('./heatmapOuter')

class FakeImage { set src (v) { this._src = v } }   // onload never fires
global.Image = FakeImage

test('claims loading synchronously, before any await', () => {
  const el = document.createElement('div')
  document.body.appendChild(el)
  heatmapOuter(el, config())
  expect(el.getAttribute('rhtmlwidget-status')).toBe('loading')
})

Two caveats on the other two behaviours:

• The .catch branch isn't cleanly testable while line 66 does throw error. Nothing above catches it, so in production it only surfaces as an unhandled rejection in the console — and under jest 26 that unhandled rejection fails the test, and leaks into whichever test runs next. Swapping the rethrow for rootLog.error(error) loses nothing and makes the branch assertable.
• With an Image that never fires, the chain never reaches the status write, so asserting loading afterwards is vacuous — it doesn't exercise isCurrentRender(). Once the rethrow is gone, the reject path tests the guard properly: render, el.innerHTML = '', settle, assert the attribute is still null.

The full happy path additionally needs a canvas shim (jest-canvas-mock) — jsdom's getContext('2d') returns null, so loadImage can't get past drawImage. Optional.

CI doesn't run these tests

build-r-package.yaml only builds the R package through the nix flake, so fonts.jest.test.js isn't run by CI at all — right now it only runs if someone remembers to locally. Worth porting the unit job from rhtmlCombinedScatter's js-tests.yaml, with the run: lines adapted: that workflow targets rhtmlBuildUtils 9.0.0, and this repo is on 7.1.1, which ships no rhtml bin. So npx gulp lint / npx gulp testSpecs / npx gulp compileWidgetEntryPoint. I've verified lint and testSpecs are green under node-version: 22 here, npm ci works against the lockfile, and the preinstall npm-force-resolutions hook self-skips when CI=true. PUPPETEER_SKIP_DOWNLOAD is worth keeping — puppeteer 3.3.0 honours it (install.js:193).

The compileWidgetEntryPoint step earns its place here for a reason specific to this repo: the bundle in inst/htmlwidgets/ is committed, so it catches a source change pushed without a rebuild.

I'd leave the visual job out — it's a rhtmlBuildUtils 9.0.0 migration, not a workflow file. At 7.1.1: puppeteer 3.3.0 is Chromium ~83 against an apt list tuned for Chrome 14x, --env is whitelisted to local/travis so --env=ci is rejected, --acceptNewSnapshots defaults true so a missing baseline is silently written and passes, jest-image-snapshot 3.1.0 swallows mismatches at the individual-test level, and the committed baselines were generated on Windows so all ~100 would fail on ubuntu at a 0.0001% threshold.

Separately: npm run localTest doesn't look like it's been run on this branch. Worth doing before merge — every visual test gates on div[rhtmlwidget-status=ready], so that suite is the only existing coverage of the refactored status path.

[chschan]

This claims loading synchronously, but nothing guarantees a terminal status, so there's a path that ends up worse than before this PR.

loadImage (line 92) still has no img.onerror — the TODO on line 93 is untouched — so when img.src = uri fails on an empty, malformed, or corrupt data URI, that promise never settles. The .catch below can't help, because there is no rejection to catch. The div then sits at rhtmlwidget-status=loading forever. Pre-PR the attribute was simply never written, so per your own reasoning in the PR description the export treated the widget as not-loading and screenshotted immediately; now an image failure means waiting out the screenshot timeout instead — the exact outcome the comment on the .catch says it's avoiding.

Same hole for a synchronous throw between here and the Promise.all — getContainerDimensions, or the d3.select(element) array-like path. That escapes before any promise exists, so again there's no rejection and the status stays loading.

Two changes close it: img.onerror = reject in loadImage, and a try/catch around this block that reports ready on a synchronous failure.

[JustinCCYap]

Both closed in 0974d08.

img.onerror = () => reject(...) in loadImage, so an unloadable data URI now rejects and the existing .catch reports ready instead of the chain hanging. That was the real hole, and it removes the TODO too.

One correction on the second half: getContainerDimensions runs at line 21, above the loading claim at line 26, so a throw there leaves the attribute unwritten exactly as it was pre-PR. The window is only d3.select(element).append('svg') and new Image(), both of which are now inside a try/catch that reports ready before rethrowing. That branch writes unconditionally rather than through isCurrentRender() — no newer render can have started while this one is still synchronous, and if the throw came from the append itself there is no svg for the check to find.

[chschan]

No rejection handler on this chain, so a rejecting fontSet.ready skips the entire render rather than degrading.

The rejection propagates out of waitForFonts into the Promise.all in heatmapOuter.js, which means processImageData and new Heatmap are never called and the .catch there marks the div ready with an empty <svg>. That directly contradicts this file's own contract on line 14 — "A font we cannot load must delay the chart, not prevent it". I confirmed the propagation with a throwaway jest probe (ready: Promise.reject(...) → waitForFonts rejects).

The spec says FontFaceSet.ready never rejects, which is why this is a should-fix rather than a must-fix — but a non-conforming or shimmed font set is exactly what the try/catch above already defends against, so it seems worth being consistent. .catch(() => {}) after the .then does it.

Related, same two lines: clearTimeout only runs on the fulfilled path, so the 3s timer leaks whenever the race rejects.

[JustinCCYap]

Valid on both counts, fixed in a follow-up commit.

.then(stopWaiting, stopWaiting) — the same handler as both the fulfilled and rejected callback, so a rejecting font set is swallowed rather than propagated, clearTimeout runs on either settlement, and the resolution value stays undefined. Chose that over .catch(() => {}) plus a separate clear because it keeps the timer handling in one place, and over .finally because core-js 2 is what the bundle polyfills against.

Added a test with ready: Promise.reject(...) asserting waitForFonts still resolves. 12 green.

[chschan]

Unrelated to the fix itself, but it's the thing blocking any test of this file, so it may be worth folding in here.

These three ESM imports (lines 3-5) mean jest can't load this module at all:

require('./heatmapOuter.js')
→ SyntaxError: Cannot use import statement outside a module

There's no babel transform wired for jest in this repo — no .babelrc, no babel.config.js, no jest or babel key in package.json, and neither babel-jest nor @babel/preset-env in devDeps. The one existing spec target (lib/components/utils.js) is plain CommonJS, so it has never come up.

Cheapest fix is to convert the three to require, which the file already mixes anyway two lines below:

const _ = require('lodash')
const d3 = require('d3')
const rootLog = require('loglevel')

I'd prefer that over adding a babel config, since a root config would also feed compileWidgetEntryPoint and could change the shipped bundle; browserify handles both forms, so this way the bundle is untouched.

[JustinCCYap]

Done in e5a3e55, folded in as you suggested — the three imports are now require, and no babel config was added.

That is what unblocked heatmapOuter.jest.test.js, which is green locally and in CI.

Checked your reason for preferring it over a babel config: the only change to inst/htmlwidgets/rhtmlHeatmap.js in that commit is the one line carrying the new code, so the conversion left the shipped bundle untouched.

Scoped to this file only. The ~20 others under theSrc/scripts/lib/ still use import, which browserify handles either way, so converting them would be churn beyond this PR.

[JustinCCYap]

Added theSrc/scripts/heatmapOuter.jest.test.js in e5a3e55 — four tests, gulp testSpecs is 16 green. Also took the require conversion from the inline comment, for the reason you gave: no babel config, so no risk to the shipped bundle.

I kept the rethrow rather than swapping it for rootLog.error. SharedWebUi/modules/src/core/BugCatcher.ts:1136 sets window.onunhandledrejection, so today a failed heatmap render is reported to BugDup; logging it locally would silence that. Instead heatmapOuter now returns the chain, so a test can await expect(rendering).rejects.toThrow(...) while production still ignores the return value and lets the rejection surface. That covers the .catch branch without changing what Displayr sees.

Two notes on the test approach, since they differ from your sketch:

• new Image() throwing does not exercise the synchronous path — the constructor runs inside the Promise executor in loadImage, so a throw there becomes a rejection. The synchronous branch is reached by stubbing element.appendChild to throw, which fails inside d3.select(element).append('svg').
• Both halves of isCurrentRender() are asserted, using img.onerror (now wired) to settle the chain deterministically: the live render reports ready, and a superseded one — render, element.innerHTML = '', render again, then settle the first — leaves the attribute at loading.

Mutation-checked rather than assumed: dropping the early loading claim and the staleness guard fails exactly 2 of the 16.

No canvas shim, so the happy path is still uncovered — getContext('2d') returns null under jsdom. Happy to add jest-canvas-mock if you want that too.