Title only the live PDF viewer frame during a recompile swap (fixes typst PDF e2e flake)

claude-notes/plans/2026-10-20-pdf-preview-iframe-flake.md

@@ -0,0 +1,149 @@
+---
+title: 'q2-preview-spa e2e: ''an edit recompiles the PDF'' flakes on two PDF preview iframes (bd-e4q7juv0)'
+date: 2026-10-20
+description: 'The typst PDF preview e2e locator matches both iframes of the viewer''s by-design recompile swap; scope it to the live frame (test-side, or by marking the live frame in @quarto/pdf-viewer).'
+status: in-progress
+braid:
+  strand: bd-e4q7juv0
+---
+
+**Branch:** `braid/bd-e4q7juv0-pdf-preview-iframe-flake` (topic branch in the main checkout, based on `main` @ `9758c3a2e`)
+**Go-ahead given 2026-10-20.** Decisions below.
+
+## Decisions (2026-10-20)
+
+- **D1: option B.** The viewer marks the live frame. Carlos asked whether the two frames are ever
+  visible together (flash avoidance). They are not: both are *rendered*, the pending one stacked
+  underneath the opaque live one, and the swap is a single paint. B only changes accessibility
+  attributes and cannot affect painting. The constraint to carry forward: the pending frame must
+  never be hidden with CSS (`display:none`/`visibility:hidden`), or pdf.js never paints it and every
+  recompile hits the watchdog.
+- **D2: drop the title.** A frame gets `title="PDF preview"` only on promotion. Until then it has no
+  title, plus `aria-hidden="true"` and `inert`. This applies to the very first frame as well: a frame
+  that has not drawn a document is not "the PDF preview".
+- **D3: include the embed spec.** `embed-typst-pdf.spec.ts` drops `.first()` and relies on the same
+  single-match selector.
+
+## Triage verdict
+
+**Ready to design.** The cause is clear from the code: the double-iframe swap is intentional, and the
+test's locator doesn't allow for it. The fix is small; what's left to decide is whether it lives only in
+the test or also in the viewer.
+
+## Issue context
+
+Filed 2026-10-20 (today) by Carlos, P3 bug, label `flaky-test`. Seen on PR #848 (KaTeX snyk bump),
+run 39435016486 job 118669637915, chromium: `q2-preview-spa/e2e/typst-pdf-preview.spec.ts:83`
+failed all 3 attempts with
+
+> strict mode violation: `locator('iframe[title="PDF preview"]')` resolved to 2 elements
+
+A rerun passed; other branches pass. The job is `continue-on-error: true`
+(`.github/workflows/q2-preview-spa-e2e.yml:59`), so it does not block merges.
+
+## Dependency graph
+
+- **discovered-from**: bd-7gqr148v (lmd AST desugar contract, in_progress). This only records which
+  session saw the flake while babysitting CI. The two strands share no code and no design.
+- No `blocks`, `related` or parent edges. Nothing depends on this, so the only pressure is CI noise.
+
+## What the code looks like today
+
+- `ts-packages/pdf-viewer/src/pdfViewer.ts:10-13, 162-184`: **by design**, a recompile loads into a
+  *second* `iframe` titled `PDF preview` (z-index 0, underneath). It is promoted (z-index 2), and the old
+  frame removed, only after pdf.js has painted the current page at its final layout (invariant from
+  bd-c72wsugj: the frame on top is never an empty viewer). So **every** recompile has a window with two
+  matching iframes, lasting from pdf.js start-up until first paint: hundreds of ms locally, seconds on a
+  loaded CI runner. This is not a bug in PdfPane. The issue's "make PdfPane remove the old iframe
+  atomically" alternative would bring back the flash the design exists to prevent, so it is out.
+- `q2-preview-spa/e2e/typst-pdf-preview.spec.ts:51-52`: `textLayer()` =
+  `page.frameLocator('iframe[title="PDF preview"]').locator('.textLayer')`, which is strict. Line 83's
+  `toContainText('Beta heading')` polls straight through the swap window. Playwright (1.60) raises a
+  strict-mode violation as an error, not as a "not yet" result to retry, so the test passes only when
+  every poll misses the window. That fits "fails on a slow runner, passes on rerun": the longer the swap,
+  the likelier a poll hits it, and all 3 retries ran on the same slow runner. *(The claim that strict
+  violations aren't retried is from memory of Playwright's behaviour; Phase 0 confirms it.)*
+- Other uses of `textLayer()` in the same spec: lines 74, 89, 95, 116, 127, 128. They are only exposed if
+  something recompiles while they poll (for example a second `contentTick` right after the first load). The
+  422 path in the "broken edit" test never swaps, because nothing new is loaded.
+- `q2-preview-spa/e2e/embed-typst-pdf.spec.ts:60-67` (hub-client embed) already sidesteps this with
+  `frameLocator(...).first()`. During a swap the **live frame is always first in DOM order**: `current`
+  was appended before any `pending`, and a new pending is always appended last. So `.first()` is the
+  correct "live frame" selector today, but only because of an implementation detail nobody wrote down.
+  `pdfText()` in that spec also reads the old frame until the swap completes. That's harmless under its
+  `expect.poll`.
+- `allPdfText()` (spec line 56) uses `page.frames().find(...)`, which takes the first match and is never
+  strict, so it is fine for the same DOM-order reason.
+- hub-client unit tests (`hub-client/src/pandoc/pdfViewer.test.ts:65,114,135`) count iframes by tag and
+  never use the title. A viewer-side change would have to keep them passing (and might extend them).
+
+Not reproduced locally. The window is timing-dependent, and the code reading above explains the error
+message exactly. A deterministic repro is possible, though: slow the pending frame's first paint (for
+example with a route that delays `/pdfjs/web/viewer.mjs` for the second load) and assert on the text layer
+during the window. That is Phase 0's test.
+
+## Proposed phases (draft)
+
+- **Phase 0 — Failing test.** Make the swap window long and deterministic in the e2e (delay the second
+  viewer load via `page.route`, or slow pdf.js's first `pagerendered`). Show that line 83's assertion fails
+  with the strict-mode error, then that it passes after the fix.
+- **Phase 1 — Scope the locator to the live frame.** Option A or B below (design question 1).
+- **Phase 2 — Align the embed spec** (`embed-typst-pdf.spec.ts`) to the same helper or selector, so the
+  two specs don't drift. Optional.
+- **Phase 3 — Docs.** If B: one paragraph in the `pdfViewer.ts` header naming the marker as the contract
+  for "which frame is live". No user-facing docs or changelog.
+
+Options for Phase 1:
+
+- **A. Test-only.** `page.locator('iframe[title="PDF preview"]').first().contentFrame()`, with a comment
+  saying that during a swap the live frame comes first in DOM order. One-line change; no product code
+  touched; relies on the undocumented DOM-order fact.
+- **B. Mark the live frame in the viewer.** The pending frame gets no `title` (or a distinct one) and
+  `aria-hidden="true"`/`inert` until it is promoted; promotion sets `title="PDF preview"`. The test's
+  existing selector then matches exactly one frame with no change. This also fixes a small accessibility
+  issue: screen readers currently see two frames titled "PDF preview", one of them hidden underneath and
+  half-loaded. Cost: touches `@quarto/pdf-viewer`, which both hub-client and the SPA use, so it needs
+  unit tests in `hub-client/src/pandoc/pdfViewer.test.ts`.
+
+## Open design questions for the user
+
+1. **Test-only (A) or viewer-side marker (B)?** B fixes the a11y issue and turns "which frame is live"
+   into an explicit contract. A is the minimal CI fix. My recommendation: B. The swap is a deliberate,
+   long-lived design, and anything that inspects the pane (tests, assistive tech) should see one frame.
+2. **If B: drop the title on the pending frame, or rename it** (e.g. `PDF preview (loading)`)? Dropping
+   it and adding `aria-hidden` hides it from the accessibility tree completely. Renaming keeps it findable
+   by tests that want to watch the pending load (none do today).
+3. **Include the embed spec (Phase 2) in this strand**, or leave it, since `.first()` works there today?
+
+## Risks / tradeoffs (draft)
+
+- With option A, a future change to the swap order (e.g. inserting the pending frame *before* the live
+  one) would silently make `.first()` read the pending frame. Detecting that would take a dedicated test.
+- With option B, setting the title after load means code that waits for `iframe[title="PDF preview"]`
+  to *appear* now waits until first paint instead of until creation. Today that is only the embed spec's
+  `toBeAttached` (90 s timeout), which is fine.
+- The Phase 0 repro depends on being able to slow one of two identical viewer loads through `page.route`.
+  If that turns out to be fiddly, settle for a unit-level assertion on the viewer (B) and rely on the
+  e2e as-is.
+
+## Work log
+
+- [x] **Phase 0.** Unit test `titles only the live frame…` in `hub-client/src/pandoc/pdfViewer.test.ts`
+  failed before the change (the first frame was titled before it drew). E2E repro
+  `the text layer reads the live PDF while a recompile loads underneath it` holds the second viewer's
+  `viewer.mjs` with `page.route`, keeping the swap window open. Against a build with the old viewer, it
+  and the existing `an edit recompiles the PDF` both failed locally with the CI error,
+  `strict mode violation: locator('iframe[title="PDF preview"]') resolved to 2 elements`.
+- [x] **Phase 1.** `pdfViewer.ts`: new frames start untitled, `aria-hidden` and `inert`; promotion sets
+  `title = VIEWER_TITLE` and clears both. The header comment records the never-hide-with-CSS rule.
+- [x] **Phase 2.** `embed-typst-pdf.spec.ts` drops `.first()`.
+- [x] Verified with a fixed build: `typst-pdf-preview.spec.ts` (chromium + firefox) and
+  `embed-typst-pdf.spec.ts` (embed), 20/20 passed. pdfViewer and PdfPane unit tests, and the hub-client
+  and q2-preview-spa typechecks, are clean.
+- [x] hub-client changelog entry for `2b566a68e`.
+
+Local environment notes (not code issues): the earlier pre-flight failure of
+`editor_ui_serves_pdfjs_from_the_viewer_bundle` and the "no live preview" e2e overlay both came from stale
+local front-end artifacts: `q2-preview-spa/dist`, `hub-client/dist-preview-embed`, the WASM `pkg/`, and a
+`node_modules` missing the `@quarto/pdf-viewer` workspace link. `npm install`, `npm run build:wasm` and the
+two front-end builds fixed them.

hub-client/changelog.md

@@ -25,6 +25,7 @@ WASM rebuild is needed for a changelog-only edit.
 
 ### 2026-10-20
 
+- [`2b566a68e`](https://github.com/quarto-dev/q2/commits/2b566a68e): Screen readers now see a single PDF preview frame while a typst document recompiles, instead of the visible one plus the one still loading underneath.
 - [`4540c20c2`](https://github.com/quarto-dev/q2/commits/4540c20c2): Upgrade KaTeX to 0.18.9, which keeps trailing rows in `align` environments.
 
 ### 2026-10-15

hub-client/src/pandoc/pdfViewer.test.ts

@@ -106,6 +106,54 @@ describe('mountPdfViewer', () => {
     container.remove();
   });
 
+  it('titles only the live frame: a frame still loading is hidden from assistive tech and from title lookups', async () => {
+    const container = document.createElement('div');
+    document.body.appendChild(container);
+    const viewer = mountPdfViewer(container, '/');
+    const frames = () => [...container.querySelectorAll('iframe')];
+    const live = () => [...container.querySelectorAll('iframe[title="PDF preview"]')];
+    const isHidden = (f: HTMLIFrameElement) => f.getAttribute('aria-hidden') === 'true' && f.inert;
+
+    const first = viewer.show(new Uint8Array([1]), { key: 'e', fileName: 'e.pdf' });
+    const [f1] = frames();
+    // The first document has drawn nothing yet: there is no PDF preview to name.
+    expect(live()).toEqual([]);
+    expect(isHidden(f1)).toBe(true);
+    const app1 = fakeViewerApp();
+    await boot(f1, app1);
+    app1.emit('pagesloaded');
+    app1.isInitialViewSet = true;
+    app1.renderingState = FINISHED;
+    app1.emit('pagerendered');
+    await first;
+    expect(live()).toEqual([f1]);
+    expect(f1.hasAttribute('aria-hidden')).toBe(false);
+    expect(f1.inert).toBe(false);
+
+    // During a recompile both frames are in the DOM (the new one painting underneath), but only one is the PDF preview.
+    const second = viewer.show(new Uint8Array([2]), { key: 'e', fileName: 'e.pdf' });
+    const f2 = frames()[1];
+    expect(frames()).toEqual([f1, f2]);
+    expect(live()).toEqual([f1]);
+    expect(isHidden(f2)).toBe(true);
+    // Hidden from assistive tech only: pdf.js must still lay out and paint it, or it never becomes ready.
+    expect(f2.style.display).not.toBe('none');
+    expect(f2.style.visibility).not.toBe('hidden');
+    const app2 = fakeViewerApp();
+    await boot(f2, app2);
+    app2.emit('pagesloaded');
+    app2.isInitialViewSet = true;
+    app2.renderingState = FINISHED;
+    app2.emit('pagerendered');
+    await second;
+    expect(live()).toEqual([f2]);
+    expect(f2.hasAttribute('aria-hidden')).toBe(false);
+    expect(f2.inert).toBe(false);
+
+    viewer.dispose();
+    container.remove();
+  });
+
   it('promotes on documentinit when the page drawn before it survives the initial view (no saved view to apply)', async () => {
     const container = document.createElement('div');
     document.body.appendChild(container);

q2-preview-spa/e2e/embed-typst-pdf.spec.ts

@@ -59,9 +59,12 @@ async function startEditor(files: { path: string; content: string }[]): Promise<
 
 const pdfFrame = (page: Page) => page.locator('[data-testid="pdf-preview-pane"] iframe[title="PDF preview"]');
 
-/** The viewer's own page text (pdf.js text layer), once it has drawn. */
+/**
+ * The live viewer's page text (pdf.js text layer), once it has drawn. Only the live frame carries the title, so
+ * the frame locator is a single match even while a recompile loads underneath (bd-e4q7juv0).
+ */
 async function pdfText(page: Page): Promise<string> {
-  const frame = page.frameLocator('[data-testid="pdf-preview-pane"] iframe[title="PDF preview"]').first();
+  const frame = page.frameLocator('[data-testid="pdf-preview-pane"] iframe[title="PDF preview"]');
   await frame.locator('.textLayer span').first().waitFor({ timeout: 60_000 });
   return (await frame.locator('.textLayer').first().innerText()) ?? '';
 }

q2-preview-spa/e2e/typst-pdf-preview.spec.ts

@@ -48,7 +48,13 @@ async function start(fixtureFiles: FixtureFile[]): Promise<PreviewServerHandle>
   return server;
 }
 
+/**
+ * The live viewer. A recompile loads into a second iframe under it (`@quarto/pdf-viewer`); only the live one
+ * carries the title, so this stays a single match through the swap (bd-e4q7juv0).
+ */
 const pdfFrame = (page: Page): FrameLocator => page.frameLocator('iframe[title="PDF preview"]');
+/** Every viewer iframe, live or still loading. */
+const viewerFrames = (page: Page) => page.locator('iframe[src*="pdfjs/web/viewer.html"]');
 const textLayer = (page: Page) => pdfFrame(page).locator('.textLayer');
 
 /** All pages' text via pdf.js, for documents whose interesting page is not the first (text layers exist only for visible pages). */
@@ -83,6 +89,26 @@ test('an edit recompiles the PDF', async ({ page }) => {
   await expect(textLayer(page)).toContainText('Beta heading', { timeout: 60_000 });
 });
 
+test('the text layer reads the live PDF while a recompile loads underneath it', async ({ page }) => {
+  const s = await start([{ path: 'index.qmd', content: doc('Alpha heading') }]);
+  // Hold the second viewer's script, so the recompile's frame stays loading under the live one until released.
+  let viewerLoads = 0;
+  let release!: () => void;
+  const held = new Promise<void>((resolve) => (release = resolve));
+  await page.route(/\/pdfjs\/web\/viewer\.mjs(\?|$)/, async (route) => {
+    if (++viewerLoads === 2) await held;
+    await route.continue();
+  });
+  await page.goto(s.url);
+  await expect(textLayer(page)).toContainText('Alpha heading', { timeout: 60_000 });
+  await writeFile(path.join(s.projectDir, 'index.qmd'), doc('Beta heading'));
+  await expect(viewerFrames(page)).toHaveCount(2, { timeout: 60_000 });
+  await expect(textLayer(page)).toContainText('Alpha heading');
+  release();
+  await expect(textLayer(page)).toContainText('Beta heading', { timeout: 60_000 });
+  await expect(viewerFrames(page)).toHaveCount(1);
+});
+
 test('a broken edit shows the banner and keeps the last PDF', async ({ page }) => {
   const s = await start([{ path: 'index.qmd', content: doc('Alpha heading') }]);
   await page.goto(s.url);

ts-packages/pdf-viewer/src/pdfViewer.ts

@@ -11,6 +11,11 @@
  * reopening a document inside the live viewer empties it for a few frames, a visible flash. The frame on
  * top is never an empty viewer: a new document that fails to draw is dropped (see `VIEWER_WATCHDOG_MS`),
  * and the previous one stays.
+ *
+ * Only the live frame is titled `PDF preview` (`VIEWER_TITLE`); a frame still loading has no title and is
+ * `aria-hidden` and `inert`, so assistive tech and title lookups (the e2e specs) see exactly one viewer
+ * (bd-e4q7juv0). A loading frame is never hidden with CSS: pdf.js must lay it out and paint it underneath,
+ * or it never becomes ready to promote.
  */
 
 /** The slice of pdf.js's `PDFViewerApplication` this module uses. */
@@ -55,6 +60,9 @@ export interface PdfViewerHandle {
 
 export const PDF_HISTORY_KEY = 'pdfjs.history';
 
+/** The live viewer frame's `title`; a frame still loading has none. */
+export const VIEWER_TITLE = 'PDF preview';
+
 /** URL of the stock viewer under the app's base path (`'./'` for the preview SPA, Vite's `BASE_URL` for hub-client). */
 export function viewerUrl(base: string, file?: string, restore = false): string {
   const root = `${base.endsWith('/') ? base : `${base}/`}pdfjs/web/viewer.html`;
@@ -102,7 +110,8 @@ export function mountPdfViewer(container: HTMLElement, base: string, options: Pd
   function load(pdf: Uint8Array, zIndex: number, restore: boolean): { frame: Frame; ready: Promise<void> } {
     const blobUrl = URL.createObjectURL(new Blob([pdf as BlobPart], { type: 'application/pdf' }));
     const iframe = document.createElement('iframe');
-    iframe.title = 'PDF preview';
+    iframe.setAttribute('aria-hidden', 'true');
+    iframe.inert = true;
     iframe.style.cssText = `position:absolute;inset:0;width:100%;height:100%;border:0;display:block;z-index:${zIndex}`;
     const ready = new Promise<void>((resolve, reject) => {
       // `once`: the subscriptions and the watchdog below belong to this one document load.
@@ -179,6 +188,9 @@ export function mountPdfViewer(container: HTMLElement, base: string, options: Pd
     if (pending !== frame || disposed) return; // superseded, or the pane closed
     pending = null;
     frame.iframe.style.zIndex = '2';
+    frame.iframe.title = VIEWER_TITLE;
+    frame.iframe.removeAttribute('aria-hidden');
+    frame.iframe.inert = false;
     if (current) discard(current);
     current = frame;
   }