diff --git a/.github/workflows/validate-build.yml b/.github/workflows/validate-build.yml index b656f411..f07e83a9 100644 --- a/.github/workflows/validate-build.yml +++ b/.github/workflows/validate-build.yml @@ -75,6 +75,8 @@ jobs: run: npm run test:kapa-source-groups - name: Test collapsible TOC groups run: npm run test:toc-collapsible + - name: Test the active On-this-page entry after an anchor jump + run: npm run test:toc-active-anchor - name: Test anonymous Ask AI quota run: npm run test:anon-quota - name: Test nav positioning leaves the page scroll alone diff --git a/package.json b/package.json index 9716cda2..f5644b04 100644 --- a/package.json +++ b/package.json @@ -95,10 +95,11 @@ "test:nav-lazy-buckets": "node --test tests/nav-lazy-buckets/*.test.js", "test:has-code-blocks": "node --test tests/has-code-blocks/*.test.js", "test:docs-page-weight": "node --test tests/docs-page-weight/*.test.js", - "test:all": "npm run test:playground && npm run test:interactive && npm run test:negative-cache && npm run test:head-meta && npm run test:property-tooltips && npm run test:signin-nudge && npm run test:chat-panel-navigation && npm run test:toc-collapsible && npm run test:nav-scroll && npm run test:kapa-source-groups && npm run test:tooltip-touch && npm run test:tooltip-single-open && npm run test:lazy-askai && npm run test:nav-lazy-buckets && npm run test:has-code-blocks && npm run test:docs-page-weight && npm run test:anon-quota", + "test:all": "npm run test:playground && npm run test:interactive && npm run test:negative-cache && npm run test:head-meta && npm run test:property-tooltips && npm run test:signin-nudge && npm run test:chat-panel-navigation && npm run test:toc-collapsible && npm run test:toc-active-anchor && npm run test:nav-scroll && npm run test:kapa-source-groups && npm run test:tooltip-touch && npm run test:tooltip-single-open && npm run test:lazy-askai && npm run test:nav-lazy-buckets && npm run test:has-code-blocks && npm run test:docs-page-weight && npm run test:anon-quota", "build:wasm": "cd blobl-editor/wasm && GOOS=js GOARCH=wasm go build -o ../../src/static/blobl.wasm .", "copy:wasm-exec": "cp \"$(go env GOROOT)/lib/wasm/wasm_exec.js\" src/js/vendor/", - "serve:playground": "npx serve ." + "serve:playground": "npx serve .", + "test:toc-active-anchor": "node --test tests/toc-active-anchor/*.test.js" }, "dependencies": { "@kapaai/agent-react": "^1.0.1", diff --git a/src/css/doc-bump.css b/src/css/doc-bump.css index d15eb50a..5e14ef54 100644 --- a/src/css/doc-bump.css +++ b/src/css/doc-bump.css @@ -777,9 +777,12 @@ html[data-theme=dark] { margin-top: 0; } +/* Mirrors the doc.css rule; see the note there for why anywhere and not break-word. */ .aa-DetachedOverlay .doc .tableblock pre, +.aa-DetachedOverlay .doc .tableblock pre code, .aa-DetachedOverlay .doc .listingblock.wrap pre { white-space: pre-wrap; + overflow-wrap: anywhere; } .aa-DetachedOverlay .doc div.no-wrap pre code { diff --git a/src/css/doc.css b/src/css/doc.css index 5c12248e..a648ae26 100644 --- a/src/css/doc.css +++ b/src/css/doc.css @@ -1,5 +1,10 @@ html { - scroll-padding-top: calc(var(--navbar-height) + var(--toolbar-height)); /* 114px */ + /* The one place an anchor jump is offset. The "On this page" scroll-spy reads + this same value as its activation line (see src/js/02-on-this-page.js), so + any second offset elsewhere puts a linked heading below the line and + highlights the entry above it. The trailing 16px is the breathing room that + used to live on the headings as scroll-margin-top. */ + scroll-padding-top: calc(var(--navbar-height) + var(--toolbar-height) + 16px); } .doc { @@ -26,7 +31,7 @@ html { @media screen and (min-width: 1024px) { html { - scroll-padding-top: calc(var(--navbar-height) + 32px); /* 88px */ + scroll-padding-top: calc(var(--navbar-height) + 32px + 24px); } .doc { @@ -59,24 +64,11 @@ html { font-weight: var(--heading-font-weight); } -/* Offset anchor links to account for sticky bar (navbar + component indicator) */ -.doc h2[id], -.doc h3[id], -.doc h4[id], -.doc h5[id], -.doc h6[id] { - scroll-margin-top: calc(var(--navbar-height) + 16px); -} - -@media screen and (min-width: 1024px) { - .doc h2[id], - .doc h3[id], - .doc h4[id], - .doc h5[id], - .doc h6[id] { - scroll-margin-top: calc(var(--navbar-height) + 24px); - } -} +/* Anchor-link offset lives on html as scroll-padding-top, not here. A + scroll-margin-top on the headings stacked on top of it: the browser parks a + linked heading at the sum of the two, which is below the scroll-spy's + activation line, so clicking an in-page link highlighted the preceding + entry. See tests/toc-active-anchor. */ .doc h1 { font-size: calc(36 / var(--rem-base) * 1rem); @@ -1713,10 +1705,19 @@ details[open] > summary { display: block; } -/* NEEDS REVIEW prevent pre in table from causing article to exceed bounds */ +/* A code block in a table cell must not set the table's width. + pre-wrap alone was not enough: it wraps between tokens but cannot break + inside one, so a single long identifier still sets the cell's min-content + width and the table outgrows the page. The property reference pages hit this + with `cloud_storage_inventory_hash_path_directory:` at 44 characters, which + is wider than a phone on its own and squeezed the label column to nothing. + overflow-wrap: anywhere is the keyword that lowers min-content, so the token + breaks only when there is genuinely no room; break-word does not. */ .doc .tableblock pre, +.doc .tableblock pre code, .doc .listingblock.wrap pre { white-space: pre-wrap; + overflow-wrap: anywhere; } .doc pre.highlight code, diff --git a/src/js/02-on-this-page.js b/src/js/02-on-this-page.js index 0d9a3bc3..f1ce87b2 100644 --- a/src/js/02-on-this-page.js +++ b/src/js/02-on-this-page.js @@ -69,27 +69,44 @@ if (collapsible) buildCollapsibleGroups(list) + /** + * Make one fragment the active entry, clearing whatever was active before. + * + * The scroll pass can leave several entries active at the bottom of a page, + * so the previous value is either a fragment or an array of them. + * + * @param {string} fragment - The '#id' to activate. Ignored if no entry has it. + */ + function setActive (fragment) { + if (!links[fragment]) return + if (lastActiveFragment && lastActiveFragment !== fragment) { + var previous = Array.isArray(lastActiveFragment) ? lastActiveFragment : [lastActiveFragment] + previous.forEach(function (f) { + if (links[f]) links[f].classList.remove('is-active') + }) + } + links[fragment].classList.add('is-active') + revealGroup(links[fragment]) + lastActiveFragment = fragment + } + + /** + * Hold off the scroll pass for a moment, so it cannot overwrite an activation + * the user just caused. The browser is still settling on the target when a + * click or a hash change lands. + */ + function holdScrollUpdates () { + skipScrollUpdate = true + setTimeout(function () { + skipScrollUpdate = false + }, 100) + } + // Add click handlers to TOC links to immediately highlight clicked item Object.keys(links).forEach(function (fragment) { links[fragment].addEventListener('click', function () { - // Immediately update active state on click - if (lastActiveFragment && lastActiveFragment !== fragment) { - if (Array.isArray(lastActiveFragment)) { - lastActiveFragment.forEach(function (f) { - links[f].classList.remove('is-active') - }) - } else { - links[lastActiveFragment].classList.remove('is-active') - } - } - links[fragment].classList.add('is-active') - revealGroup(links[fragment]) - lastActiveFragment = fragment - // Skip scroll-based updates briefly to prevent flicker during scroll animation - skipScrollUpdate = true - setTimeout(function () { - skipScrollUpdate = false - }, 100) + setActive(fragment) + holdScrollUpdates() }) }) @@ -144,22 +161,8 @@ // Update sidebar TOC active state immediately var fragment = link.getAttribute('href') if (fragment && links[fragment]) { - if (lastActiveFragment && lastActiveFragment !== fragment) { - if (Array.isArray(lastActiveFragment)) { - lastActiveFragment.forEach(function (f) { - links[f].classList.remove('is-active') - }) - } else { - links[lastActiveFragment].classList.remove('is-active') - } - } - links[fragment].classList.add('is-active') - revealGroup(links[fragment]) - lastActiveFragment = fragment - skipScrollUpdate = true - setTimeout(function () { - skipScrollUpdate = false - }, 100) + setActive(fragment) + holdScrollUpdates() } }) }) @@ -170,10 +173,12 @@ window.addEventListener('load', function () { onScroll() - revealHashTarget() + syncToHashTarget() window.addEventListener('scroll', onScroll, { passive: true }) - // In-page links and back/forward change the hash without a load, so reveal on those too - window.addEventListener('hashchange', revealHashTarget) + // In-page links and back/forward change the hash without a load, so the + // highlight has to follow on those too. onScroll alone cannot: it runs on + // scroll, and a hash change does not always produce one. + window.addEventListener('hashchange', syncToHashTarget) // On initial load, scroll active item into view (e.g., when navigating to a hash) scrollActiveIntoView() }) @@ -323,24 +328,37 @@ } /** - * Open the group that holds the entry the URL hash points at, on load and on every hash change. - * Browsers park a linked heading at scroll-padding-top + scroll-margin-top, below the activation - * line onScroll uses, so the heading above it becomes active instead. For the first entry of a - * group that heading belongs to the previous group, which would leave the target's own group - * collapsed. + * Make the entry the URL hash names the active one, on load and on every hash + * change, and open the group holding it. + * + * The hash is authoritative here rather than the scroll position, because the + * two disagree. A browser parks a linked heading at scroll-padding-top plus + * any scroll-margin-top on the heading, and onScroll's activation line is + * scroll-padding-top alone; whenever the heading lands below that line the + * scroll pass picks the heading above it and highlights the wrong entry. The + * stylesheet now offsets once so the two line up, and this keeps the highlight + * correct even if that ever drifts again. + * + * It also covers the case onScroll cannot see at all: an in-page link fires + * hashchange, and a hash change does not always move the scroll position + * enough to produce a scroll event. */ - function revealHashTarget () { + function syncToHashTarget () { var hash = window.location.hash if (!hash) return - var link = links[hash] - if (!link && ~hash.indexOf('%')) { + var fragment = links[hash] ? hash : null + if (!fragment && ~hash.indexOf('%')) { try { - link = links[decodeURIComponent(hash)] + var decoded = decodeURIComponent(hash) + if (links[decoded]) fragment = decoded } catch (e) { return } } - if (link) revealGroup(link) + if (!fragment) return + setActive(fragment) + holdScrollUpdates() + scrollActiveIntoView() } function find (selector, from) { diff --git a/tests/toc-active-anchor/toc-active-anchor.test.js b/tests/toc-active-anchor/toc-active-anchor.test.js new file mode 100644 index 00000000..209a598e --- /dev/null +++ b/tests/toc-active-anchor/toc-active-anchor.test.js @@ -0,0 +1,265 @@ +/** + * Which "On this page" entry is highlighted after an in-page anchor jump. + * + * The bug this pins: a browser parks a linked heading at scroll-padding-top + + * scroll-margin-top, which is BELOW the activation line onScroll compares + * against, so the scroll pass picks the heading above the target and highlights + * the wrong entry. Clicking any in-page property link showed it. + * + * onScroll's geometry is not the contract. The contract is that a hash the URL + * names is the active entry, so these tests assert it at two landing positions: + * level with the activation line, and below it. The second one fails if the + * script goes back to inferring the active entry from pixels alone. + * + * A separate assertion covers the stylesheet, where the double offset came + * from. + */ +const assert = require('node:assert/strict') +const fs = require('node:fs') +const path = require('node:path') +const test = require('node:test') +const vm = require('node:vm') + +const SCRIPT = path.join(__dirname, '../../src/js/02-on-this-page.js') +const CSS = path.join(__dirname, '../../src/css/doc.css') + +// Minimal DOM: enough of createElement/appendChild/querySelector for the TOC builder. +function matches (el, simple) { + const m = simple.match(/^([a-z0-9]*)((?:\.[\w-]+)*)$/i) + if (!m) return false + const tag = m[1] + const classes = m[2] ? m[2].split('.').filter(Boolean) : [] + if (tag && el.tagName !== tag.toUpperCase()) return false + return classes.every((c) => el.classList.contains(c)) +} + +function collect (el, simple, out) { + el.children.forEach((child) => { + if (matches(child, simple)) out.push(child) + collect(child, simple, out) + }) + return out +} + +function makeEl (tag) { + const classes = new Set() + const el = { + tagName: tag.toUpperCase(), + nodeName: tag.toUpperCase(), + children: [], + parentNode: null, + attrs: {}, + dataset: {}, + handlers: {}, + _text: '', + classList: { + add: (...cs) => cs.forEach((c) => classes.add(c)), + remove: (...cs) => cs.forEach((c) => classes.delete(c)), + contains: (c) => classes.has(c), + toggle (c, force) { + const on = force === undefined ? !classes.has(c) : force + if (on) classes.add(c) + else classes.delete(c) + return on + }, + }, + get className () { return [...classes].join(' ') }, + set className (v) { + classes.clear() + v.split(/\s+/).filter(Boolean).forEach((c) => classes.add(c)) + }, + get textContent () { return el._text || el.children.map((c) => c.textContent).join('') }, + set textContent (v) { + el._text = v + el.children = [] + }, + get firstChild () { return el.children[0] || null }, + appendChild (child) { + if (child.parentNode) child.parentNode.removeChild(child) + child.parentNode = el + el.children.push(child) + return child + }, + insertBefore (node, ref) { + if (node.parentNode) node.parentNode.removeChild(node) + node.parentNode = el + const i = ref ? el.children.indexOf(ref) : -1 + if (i < 0) el.children.push(node) + else el.children.splice(i, 0, node) + return node + }, + removeChild (child) { + const i = el.children.indexOf(child) + if (i >= 0) el.children.splice(i, 1) + child.parentNode = null + return child + }, + setAttribute (k, v) { el.attrs[k] = String(v) }, + getAttribute (k) { return k in el.attrs ? el.attrs[k] : null }, + addEventListener (t, fn) { el.handlers[t] = fn }, + querySelector (sel) { return collect(el, sel, [])[0] || null }, + querySelectorAll (sel) { return collect(el, sel, []) }, + } + return el +} + +function heading (level, id, text) { + return { id, nodeName: 'H' + level, textContent: text } +} + +// A reverse-chronological page: years at level 1 (h2), months at level 2 (h3). +const HEADINGS = [ + heading(2, '2026', '2026'), + heading(3, 'september-2026', 'September 2026'), + heading(3, 'august-2026', 'August 2026'), + heading(2, '2025', '2025'), + heading(3, 'december-2025', 'December 2025'), + heading(3, 'november-2025', 'November 2025'), + heading(3, 'october-2025', 'October 2025'), + heading(2, '2024', '2024'), + heading(3, 'december-2024', 'December 2024'), +] + +// Drive the IIFE against a stub DOM and hand back the built list. Headings are copied so scrollTo() +// can position them per test; scrollY and scrollHeight feed the end-of-page check in onScroll, and +// hash is what window.location.hash reports on load. +function run ({ collapsible, headings = HEADINGS, scrollY = 0, scrollHeight = 5000, hash = '' }) { + headings = headings.map((h) => Object.assign({}, h)) + const sidebar = makeEl('aside') + sidebar.dataset = { levels: '2', title: '', collapsible: collapsible ? 'true' : undefined } + const menu = makeEl('div') + menu.className = 'toc-menu' + sidebar.appendChild(menu) + + const article = makeEl('article') + article.parentNode = { querySelectorAll: () => headings } + + const listeners = {} + const context = { + console, + setTimeout: () => 0, + document: { + addEventListener () {}, + getElementById: () => null, + createElement: makeEl, + documentElement: { scrollHeight }, + querySelector: (sel) => { + if (sel === 'aside.toc.sidebar') return sidebar + if (sel === 'article.doc') return article + return null + }, + }, + window: { + addEventListener: (t, fn) => { listeners[t] = fn }, + location: { hash }, + scrollY, + innerHeight: 800, + // A 16px root font and an 80px sticky header (scroll-padding-top) put the activation line at 80px. + getComputedStyle: () => ({ fontSize: '16px', paddingTop: '0px', scrollPaddingTop: '80px' }), + }, + } + vm.runInNewContext(fs.readFileSync(SCRIPT, 'utf8'), context) + const list = menu.children[0] + return { sidebar, menu, list, listeners, headings, win: context.window } +} + +// Document position of each heading: a year heading sits 60px above its first month, and everything +// else is 400px apart, so the layout has the same adjacency as a real What's New page. +function layout (headings) { + let y = 0 + return headings.map((h, i) => { + const pos = y + const next = headings[i + 1] + y += next && parseInt(next.nodeName.slice(1), 10) > parseInt(h.nodeName.slice(1), 10) ? 60 : 400 + return pos + }) +} + +// Pretend the window is scrolled so the heading with this id sits `landing` px from the top of the +// viewport. 80 is the activation line (scroll-padding-top). A deep-linked heading lands lower, at +// scroll-padding-top + scroll-margin-top, which is 165 here and in the real stylesheet. +function scrollTo (headings, id, landing = 80) { + const index = headings.findIndex((h) => h.id === id) + assert.notEqual(index, -1, 'scrollTo target exists: ' + id) + const ys = layout(headings) + headings.forEach((h, i) => { h.getBoundingClientRect = () => ({ top: landing + ys[i] - ys[index] }) }) +} + +// Sidebar links keyed by fragment, so tests can read active state without walking the tree. +function linksByHref (list) { + const out = {} + list.querySelectorAll('a').forEach((a) => { out[a.href] = a }) + return out +} + + +// Landing positions a real browser produces. 80 is the activation line +// (scroll-padding-top in the harness). 165 is where a deep-linked heading +// actually lands today, because the stylesheet adds scroll-margin-top on top of +// scroll-padding-top. +const ON_THE_LINE = 80 +const BELOW_THE_LINE = 165 + +for (const landing of [ON_THE_LINE, BELOW_THE_LINE]) { + test(`a deep-linked heading is the active entry, landing at ${landing}px`, () => { + const { list, listeners, headings } = run({ collapsible: false, hash: '#october-2025' }) + scrollTo(headings, 'october-2025', landing) + listeners.load() + + const links = linksByHref(list) + assert.ok( + links['#october-2025'].classList.contains('is-active'), + 'the heading the URL names is active' + ) + const others = Object.keys(links).filter((h) => h !== '#october-2025' && links[h].classList.contains('is-active')) + assert.deepEqual(others, [], 'no other entry is left active') + }) + + test(`an in-page link fires hashchange and moves the highlight, landing at ${landing}px`, () => { + // This is the reported case: click a property link in the body. No load, no + // TOC click, just a hashchange. + const { list, listeners, headings, win } = run({ collapsible: false, hash: '#november-2025' }) + scrollTo(headings, 'november-2025', landing) + listeners.load() + + win.location.hash = '#december-2024' + scrollTo(headings, 'december-2024', landing) + listeners.hashchange() + + const links = linksByHref(list) + assert.ok(links['#december-2024'].classList.contains('is-active'), 'the new target is active') + assert.ok(!links['#november-2025'].classList.contains('is-active'), 'the old target is not still active') + }) +} + +test('a percent-encoded hash still activates its entry', () => { + const { list, listeners, headings, win } = run({ collapsible: false }) + scrollTo(headings, 'october-2025', BELOW_THE_LINE) + listeners.load() + win.location.hash = '#october%2D2025' + listeners.hashchange() + const links = linksByHref(list) + assert.ok(links['#october-2025'].classList.contains('is-active'), 'decoded hash matches its entry') +}) + +test('a hash that names nothing on the page leaves the highlight alone', () => { + const { list, listeners, headings, win } = run({ collapsible: false }) + scrollTo(headings, 'october-2025', ON_THE_LINE) + listeners.load() + const before = Object.keys(linksByHref(list)).filter((h) => linksByHref(list)[h].classList.contains('is-active')) + win.location.hash = '#not-a-heading' + listeners.hashchange() + const after = Object.keys(linksByHref(list)).filter((h) => linksByHref(list)[h].classList.contains('is-active')) + assert.deepEqual(after, before, 'an unknown hash is ignored rather than clearing the highlight') +}) + +test('the stylesheet offsets an anchor jump once, not twice', () => { + // scroll-padding-top on html already clears the sticky bar for every anchor + // jump. A scroll-margin-top on the headings adds a second offset, which is + // what put a linked heading below the activation line and highlighted the + // entry above it. One offset, in one place, so the browser and onScroll agree. + const css = fs.readFileSync(CSS, 'utf8') + assert.match(css, /html\s*\{[^}]*scroll-padding-top:/, 'html still carries scroll-padding-top') + const headingMargin = css.match(/\.doc h[2-6]\[id\][^{]*\{[^}]*scroll-margin-top:[^}]*\}/g) + assert.equal(headingMargin, null, 'headings do not add a second offset via scroll-margin-top') +}) diff --git a/tests/toc-collapsible/toc-collapsible.test.js b/tests/toc-collapsible/toc-collapsible.test.js index c4127346..22fce7db 100644 --- a/tests/toc-collapsible/toc-collapsible.test.js +++ b/tests/toc-collapsible/toc-collapsible.test.js @@ -294,26 +294,29 @@ test('the load pass activates the entry on the activation line and opens its gro }) test('arriving on a deep link opens the group of the target entry', () => { - // Browsers park a deep-linked heading at scroll-padding-top + scroll-margin-top, below the - // activation line, so the scroll pass marks the heading above it active. For the first entry of - // a group that heading belongs to the previous group and would leave the target's group - // collapsed. The load handler has to open it from the hash instead. + // A deep-linked heading can land below the activation line onScroll compares + // against, in which case the scroll pass would pick the heading above it. The + // load handler activates from the hash instead, so the target is active and + // its group opens. 165 is a landing below the line; see tests/toc-active-anchor + // for the activation contract on its own. const { list, listeners, headings } = run({ collapsible: true, hash: '#december-2025' }) const [y2026, y2025, y2024] = list.children scrollTo(headings, 'december-2025', 165) listeners.load() const links = linksByHref(list) - assert.equal(links['#august-2026'].classList.contains('is-active'), true, 'the scroll pass picks the heading above') - assert.equal(y2025.classList.contains('is-expanded'), true, 'the hash opens the target group anyway') + assert.equal(links['#december-2025'].classList.contains('is-active'), true, 'the hash target is active, not the heading above it') + assert.equal(links['#august-2026'].classList.contains('is-active'), false, 'the heading above the target is not active') + assert.equal(y2025.classList.contains('is-expanded'), true, 'the target group is open') assert.equal(y2025.children[1].getAttribute('aria-expanded'), 'true') assert.equal(y2026.classList.contains('is-expanded'), true) assert.equal(y2024.classList.contains('is-expanded'), false) }) test('changing the hash after load opens the group of the new target', () => { - // An in-page link or back/forward fires hashchange, not load. The scroll pass still picks the - // heading above the target, so the group has to be opened from the new hash. + // An in-page link or back/forward fires hashchange, not load, and the scroll + // pass may not run at all. The highlight and the group both come from the new + // hash. const { list, listeners, headings, win } = run({ collapsible: true }) const [, y2025, y2024] = list.children scrollTo(headings, '2026') @@ -327,8 +330,9 @@ test('changing the hash after load opens the group of the new target', () => { listeners.hashchange() const links = linksByHref(list) - assert.equal(links['#october-2025'].classList.contains('is-active'), true, 'the scroll pass picks the heading above') - assert.equal(y2024.classList.contains('is-expanded'), true, 'the hash opens the target group anyway') + assert.equal(links['#december-2024'].classList.contains('is-active'), true, 'the new hash target is active') + assert.equal(links['#october-2025'].classList.contains('is-active'), false, 'the heading above the target is not active') + assert.equal(y2024.classList.contains('is-expanded'), true, 'the target group is open') assert.equal(y2025.classList.contains('is-expanded'), true) })