diff --git a/claude-notes/plans/2026-10-20-pdf-preview-iframe-flake.md b/claude-notes/plans/2026-10-20-pdf-preview-iframe-flake.md new file mode 100644 index 000000000..71fe485b9 --- /dev/null +++ b/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. diff --git a/hub-client/changelog.md b/hub-client/changelog.md index 0c64cc038..e2a5fa8a7 100644 --- a/hub-client/changelog.md +++ b/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 diff --git a/hub-client/src/pandoc/pdfViewer.test.ts b/hub-client/src/pandoc/pdfViewer.test.ts index 854d58c96..02973b628 100644 --- a/hub-client/src/pandoc/pdfViewer.test.ts +++ b/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); diff --git a/q2-preview-spa/e2e/embed-typst-pdf.spec.ts b/q2-preview-spa/e2e/embed-typst-pdf.spec.ts index 8a7d6dbfc..80f04e3fe 100644 --- a/q2-preview-spa/e2e/embed-typst-pdf.spec.ts +++ b/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 { - 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()) ?? ''; } diff --git a/q2-preview-spa/e2e/typst-pdf-preview.spec.ts b/q2-preview-spa/e2e/typst-pdf-preview.spec.ts index 20018e29a..8bd38b9e3 100644 --- a/q2-preview-spa/e2e/typst-pdf-preview.spec.ts +++ b/q2-preview-spa/e2e/typst-pdf-preview.spec.ts @@ -48,7 +48,13 @@ async function start(fixtureFiles: FixtureFile[]): Promise 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((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); diff --git a/ts-packages/pdf-viewer/src/pdfViewer.ts b/ts-packages/pdf-viewer/src/pdfViewer.ts index fd6da2637..1ede1d983 100644 --- a/ts-packages/pdf-viewer/src/pdfViewer.ts +++ b/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 } { 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((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; }