From 96d2a14cdcf2a15dcd0df343c3142d11dace7968 Mon Sep 17 00:00:00 2001 From: Art Pai Date: Fri, 11 Sep 2026 22:24:00 +1000 Subject: [PATCH] Expose native review permalinks and align them after rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A copy-only control could not be inspected or opened as a link, and centering a target taller than the viewport pushed its title off-screen. Section, step, and change titles now carry real same-document anchors beside a separate copy button, and the deferred reveal aligns the target's leading edge. Co-Authored-By: ことね --- src/report/client.ts | 13 ++- src/report/shell.ts | 26 ++++-- test/report-dom.test.ts | 183 ++++++++++++++++++++++++++++++++++++++++ test/report.test.ts | 79 ++++++++++++++++- worker/index.test.ts | 2 + 5 files changed, 289 insertions(+), 14 deletions(-) diff --git a/src/report/client.ts b/src/report/client.ts index e98a89f..ace7c1f 100644 --- a/src/report/client.ts +++ b/src/report/client.ts @@ -222,7 +222,9 @@ function revealFragment(final: boolean) { } if (!final) return - target.scrollIntoView?.({ block: 'center' }) + // Align the target's leading edge (its title or marker). Centering a target + // taller than the viewport would push that edge far off-screen. + target.scrollIntoView?.({ block: 'start' }) const targets = new Set(generatedTargets()) const focusTarget = [...document.querySelectorAll('[data-copy-fragment]')] .find((button) => { @@ -239,7 +241,7 @@ const feedbackTimers = new WeakMap('[data-copy-label]') - const original = label?.dataset.originalLabel ?? label?.textContent ?? 'Link' + const original = label?.dataset.originalLabel ?? label?.textContent ?? 'Copy' if (label) { label.dataset.originalLabel = original label.textContent = success ? 'Copied' : 'Failed' @@ -319,7 +321,12 @@ function wireFragments(initialRender: Promise) { if (!(origin instanceof Element)) return const link = origin.closest('a[href^="#"]') - if (link && new URL(link.href).hash === window.location.hash) navigate() + if (link && new URL(link.href).hash === window.location.hash) { + // A section permalink lives inside its ; the browser's default + // action can toggle that fold after the click dispatch. Let it settle, then + // reopen ancestors and realign the title on the next task. + setTimeout(navigate, 0) + } }) navigate() void initialRender.then(() => { diff --git a/src/report/shell.ts b/src/report/shell.ts index 53625c9..7a1e8ce 100644 --- a/src/report/shell.ts +++ b/src/report/shell.ts @@ -154,11 +154,11 @@ function renderSection( .filter((change) => change.canonical) .map( (change) => - `${renderCopyLink(change.fragment, `Copy link to change ${change.id}`, change.id)}`, + `${renderPermalink(change.fragment, `Permalink to change ${change.id}`, change.id)}${renderCopyLink(change.fragment, `Copy link to change ${change.id}`)}`, ) .join('') const actions = `
- ${renderCopyLink(stepTarget.fragment, `Copy link to step ${stepIndex + 1} in ${section.title}`)}${changeTargets} + ${renderPermalink(stepTarget.fragment, `Permalink to step ${stepIndex + 1} in ${section.title}`, 'Link')}${renderCopyLink(stepTarget.fragment, `Copy link to step ${stepIndex + 1} in ${section.title}`)}${changeTargets}
` if (step.diff === undefined) { return `
${actions}${textMarkup}
` @@ -198,7 +198,7 @@ function renderSection( const markup = `
- ${sectionIndex(index)}${escapeHtml(section.title)}${renderCopyLink(target.fragment, `Copy link to section ${section.title}`)} + ${sectionIndex(index)}${escapeHtml(section.title)}${renderPermalink(target.fragment, `Permalink to section ${section.title}`, 'Link')}${renderCopyLink(target.fragment, `Copy link to section ${section.title}`)} ${steps.join('\n')}
` @@ -229,7 +229,11 @@ ${links} ` } -function renderCopyLink(fragment: string, label: string, text = 'Link'): string { +function renderPermalink(fragment: string, label: string, text: string): string { + return `` +} + +function renderCopyLink(fragment: string, label: string, text = 'Copy'): string { return `` } @@ -456,7 +460,7 @@ main { max-width: none; min-width: 0; margin: 0; padding: 22px 28px 72px; } .step { scroll-margin-top: 18px; } .step-actions { display: flex; flex-wrap: wrap; justify-content: flex-end; gap: 5px; padding: 8px 12px 0; } .step-text { max-width: 900px; padding: 8px 20px; font-size: 17px; } -.copy-link { +.copy-link, .permalink { padding: 3px 7px; border: 1px solid #c4d1c6; border-radius: 5px; @@ -465,11 +469,12 @@ main { max-width: none; min-width: 0; margin: 0; padding: 22px 28px 72px; } font: 600 10px/1.35 ui-monospace, SFMono-Regular, Menlo, monospace; cursor: pointer; } -.copy-link:hover { color: var(--accent); border-color: #8eaa95; background: #eef5ef; } -.copy-link:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; } +.permalink { text-decoration: none; } +.copy-link:hover, .permalink:hover { color: var(--accent); border-color: #8eaa95; background: #eef5ef; } +.copy-link:focus-visible, .permalink:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; } .copy-link[data-copy-state="success"] { color: var(--accent); border-color: #8eaa95; } .copy-link[data-copy-state="failure"] { color: #a1262f; border-color: #d7a4a8; } -.change-target { scroll-margin-top: 18px; } +.change-target { display: inline-flex; align-items: center; gap: 5px; scroll-margin-top: 18px; } .copy-status { position: fixed; width: 1px; @@ -590,6 +595,9 @@ main { max-width: none; min-width: 0; margin: 0; padding: 22px 28px 72px; } .review-controls { display: flex; gap: 6px; justify-content: flex-end; margin: 0; } .layout-form { flex: 1 1 auto; max-width: 220px; margin: 0 0 0 auto; } .fold-all { flex: none; padding: 5px 9px; font-size: 12px; } + /* The aligned fragment target must clear the sticky strip, so push its + scroll-margin past the strip plus breathing room. */ + .section, .step, .change-target { scroll-margin-top: 64px; } main { padding: 14px 10px 50px; } } @media (max-width: 520px) { @@ -600,7 +608,7 @@ main { max-width: none; min-width: 0; margin: 0; padding: 22px 28px 72px; } .section-fold > summary { font-size: 17px; } } @media print { - .layout-form, .copy-link { display: none; } + .layout-form, .copy-link, .permalink { display: none; } .review-map { display: none; } .review-workspace { display: block; } .report-cover { box-shadow: none; break-inside: avoid; } diff --git a/test/report-dom.test.ts b/test/report-dom.test.ts index 896c980..09661a6 100644 --- a/test/report-dom.test.ts +++ b/test/report-dom.test.ts @@ -372,6 +372,189 @@ describe('report browser client', () => { expect(doc.activeElement).toBe(target.querySelector('[data-copy-fragment="change-001"]')) }) + test('a direct load aligns a viewport-taller section title instead of centering the section', async () => { + const value = document([section(simplePatch(), 'Long section')]) + const fragment = reportTargets(value)[0]!.fragment + const dom = loadReport(renderReport(value, clientBundle), { + url: `https://reports.example/r/report-id#${fragment}`, + }) + const doc = dom.document as unknown as Document + const sectionTarget = doc.getElementById(fragment) as HTMLElement + + // Model the browser scroll: the section starts far down the document and is + // taller than the viewport, exactly the shape reported in the ticket. + const viewportHeight = 577 + const sectionTop = 124_086 + const sectionHeight = 58_144 + const optionsSeen: ScrollIntoViewOptions[] = [] + let scrollTop = 0 + sectionTarget.scrollIntoView = ((options?: ScrollIntoViewOptions) => { + optionsSeen.push(options ?? {}) + const block = options?.block ?? 'start' + scrollTop = + block === 'center' + ? sectionTop + sectionHeight / 2 - viewportHeight / 2 + : block === 'end' + ? sectionTop + sectionHeight - viewportHeight + : sectionTop + }) as typeof sectionTarget.scrollIntoView + + runReportClient() + await waitFor(() => optionsSeen.length > 0) + + expect(optionsSeen).toEqual([{ block: 'start' }]) + // The section title is the section's leading edge; after alignment it sits at + // the top of the viewport instead of thousands of pixels above it. + const titleTopInViewport = sectionTop - scrollTop + expect(titleTopInViewport).toBeGreaterThanOrEqual(0) + expect(titleTopInViewport).toBeLessThan(viewportHeight) + }) + + test('clicking a permalink for the current fragment repositions it without centering', async () => { + const value = document([section(simplePatch(), 'Reveal me')]) + const fragment = reportTargets(value)[0]!.fragment + const dom = loadReport(renderReport(value, clientBundle), { + url: `https://reports.example/r/report-id#${fragment}`, + }) + const doc = dom.document as unknown as Document + const sectionTarget = doc.getElementById(fragment) as HTMLElement + const sectionFold = sectionTarget.querySelector('.section-fold')! + const calls: ScrollIntoViewOptions[] = [] + sectionTarget.scrollIntoView = ((options?: ScrollIntoViewOptions) => { + calls.push(options ?? {}) + }) as typeof sectionTarget.scrollIntoView + + runReportClient() + await waitFor(() => calls.length === 1) + + const anchor = doc.querySelector(`a.permalink[href="#${fragment}"]`) + expect(anchor).not.toBeNull() + expect(anchor!.tagName).toBe('A') + expect(anchor!.getAttribute('href')).toBe(`#${fragment}`) + + // Simulate the settled result of any summary toggle before the correction. + sectionFold.open = false + calls.length = 0 + anchor!.dispatchEvent( + new dom.window.MouseEvent('click', { bubbles: true, cancelable: true }) as unknown as Event, + ) + await waitFor(() => calls.length === 1) + + expect(sectionFold.open).toBe(true) + expect(calls[0]).toEqual({ block: 'start' }) + }) + + test('clicking a folded step permalink opens its ancestors and aligns the step', async () => { + const value = document([ + { + title: 'Navigate by link', + steps: [{ text: 'Target this step.', diff: simplePatch() }], + }, + ]) + const fragment = reportTargets(value)[0]!.steps[0]!.fragment + const dom = loadReport(renderReport(value, clientBundle), { + url: 'https://reports.example/r/report-id', + }) + const doc = dom.document as unknown as Document + runReportClient() + const step = doc.getElementById(fragment) as HTMLElement + const sectionFold = step.closest('.section-fold')! + const fileFold = step.querySelector('details.file')! + sectionFold.open = false + fileFold.open = false + const calls: ScrollIntoViewOptions[] = [] + step.scrollIntoView = ((options?: ScrollIntoViewOptions) => { + calls.push(options ?? {}) + }) as typeof step.scrollIntoView + + const anchor = doc.querySelector(`a.permalink[href="#${fragment}"]`)! + expect(anchor.tagName).toBe('A') + anchor.dispatchEvent( + new dom.window.MouseEvent('click', { bubbles: true, cancelable: true }) as unknown as Event, + ) + + await waitFor(() => calls.length === 1) + expect(sectionFold.open).toBe(true) + expect(fileFold.open).toBe(true) + expect(step.scrollIntoView).toBeDefined() + expect(calls[0]).toEqual({ block: 'start' }) + }) + + test('a target after a large async diff is aligned only after every render settles', async () => { + const large = Array.from({ length: 400 }, (_, index) => `-line ${index}\n+LINE ${index}`).join( + '\n', + ) + const bigPatch = [ + 'diff --git a/big.ts b/big.ts', + '--- a/big.ts', + '+++ b/big.ts', + '@@ -1,400 +1,400 @@', + large, + '', + ].join('\n') + const value = document([ + section(bigPatch, 'Large first section'), + { title: 'Later target', steps: [{ text: 'The real target.', diff: simplePatch() }] }, + ]) + const targets = reportTargets(value) + const fragment = targets[1]!.steps[0]!.fragment + const dom = loadReport(renderReport(value, clientBundle), { + url: `https://reports.example/r/report-id#${fragment}`, + }) + const doc = dom.document as unknown as Document + const first = doc.getElementById(targets[0]!.fragment) as HTMLElement + const second = doc.getElementById(fragment) as HTMLElement + const calls: { element: Element; options: ScrollIntoViewOptions }[] = [] + first.scrollIntoView = ((options?: ScrollIntoViewOptions) => { + calls.push({ element: first, options: options ?? {} }) + }) as typeof first.scrollIntoView + second.scrollIntoView = ((options?: ScrollIntoViewOptions) => { + calls.push({ element: second, options: options ?? {} }) + }) as typeof second.scrollIntoView + + runReportClient() + await waitFor(() => calls.length > 0) + + expect(calls).toHaveLength(1) + expect(calls[0]!.element).toBe(second) + expect(calls[0]!.options).toEqual({ block: 'start' }) + }) + + test('focusing either title action leaves the corrected target position alone', async () => { + const value = document([section(simplePatch(), 'Focus target')]) + const fragment = reportTargets(value)[0]!.fragment + const dom = loadReport(renderReport(value, clientBundle), { + url: `https://reports.example/r/report-id#${fragment}`, + }) + const doc = dom.document as unknown as Document + const sectionTarget = doc.getElementById(fragment) as HTMLElement + const calls: ScrollIntoViewOptions[] = [] + sectionTarget.scrollIntoView = ((options?: ScrollIntoViewOptions) => { + calls.push(options ?? {}) + }) as typeof sectionTarget.scrollIntoView + + const button = doc.querySelector(`[data-copy-fragment="${fragment}"]`)! + const focusOptions: (FocusOptions | undefined)[] = [] + const originalFocus = button.focus.bind(button) + button.focus = ((options?: FocusOptions) => { + focusOptions.push(options) + originalFocus(options) + }) as typeof button.focus + + runReportClient() + await waitFor(() => calls.length === 1) + + // The client's own correction focuses the copy action without scrolling. + expect(focusOptions).toEqual([{ preventScroll: true }]) + expect(doc.activeElement).toBe(button) + + // Focusing the native permalink must not add a competing scroll either. + const anchor = doc.querySelector(`a.permalink[href="#${fragment}"]`)! + anchor.focus() + expect(doc.activeElement).toBe(anchor) + expect(calls).toHaveLength(1) + }) + test('waits for every initial render and honors a hashchange while mounting', async () => { const value = document([ section(simplePatch('one', 'one!'), 'Delayed first'), diff --git a/test/report.test.ts b/test/report.test.ts index eb19c2f..0346e69 100644 --- a/test/report.test.ts +++ b/test/report.test.ts @@ -456,7 +456,7 @@ describe('renderReport shell', () => { const html = renderReport(document([section(simplePatch(), 'Distinct')]), stubClient) expect(html).toContain( - '01Distinct`, + ) + expect(html).toContain('aria-label="Copy link to step 1 in Linkable"') + expect(html).toContain('aria-label="Copy link to change change-001"') + expect(html).not.toContain('