Skip to content

The "On this page" outline scrolls off the page and never marks the current section #182

Description

@mmcky

From DrDrij's review of the v2.5.0 deployment (the preview build of QuantEcon/lecture-python-programming#363, whose lectures/myst.yml:96 pins the v2.5.0 template release): the right-hand "On this page" menu has no scroll-tracking and no active highlighting. Confirmed on that exact deployment. Three independent omissions in app/components/Outline.tsx cause it, and all three have been there since the component was first written — git show d1ac95aac:app/components/Outline.tsx (2025-02-14) already reads useHeaders('main h2', 3), relative self-start and absolute inset-0. This is a parity gap, not a regression from the recent typography and link work.

Measured with Playwright, Chromium at 1440×900, on /matplotlib/ of the reviewed deployment and on python-programming.quantecon.org/matplotlib.html.

Property This theme (v2.5.0 deploy) Sphinx build
nav position absolute fixed
nav top 0px 126px (7rem at that build's 18px root)
nav max-height / overflow-y none / visible 756px (calc(100vh - 8rem)) / hidden, auto on hover
getBoundingClientRect().top after scrolling 1500px −1378px (entirely out of the viewport) 126px (unchanged)
elements carrying an active state 0 2 (li.active and a.active)
entries rendered 5 — h2 only, on a page with 5 h2 and 9 h3 14 (5 toc-h2 + 9 toc-h3; sub-lists collapsed, 3 h3 visible at that offset)
entry font-size 18px 16.2px (0.9rem)
resting link colour rgb(68,64,60) #44403c, opacity 1 rgb(72,86,107) #48566b, opacity 0.8
active link colour / weight rgb(10,125,145) #0a7d91 / 600, plus a 3.375px inset left rule

The three omissions

1. The active id is computed and thrown away. app/components/Outline.tsx:46 is const { headings } = useHeaders('main h2', 3);. Upstream's hook returns { activeId, headings } (node_modules/@myst-theme/site/src/components/DocumentOutline.tsx:310) and derives activeId from a MutationObserver plus an IntersectionObserver pass that picks the heading nearest the navbar line. That machinery already runs on every scroll in this theme; the component destructures headings and discards the rest. The offset is correctly wired: app/root.tsx:214 passes top={50}, @myst-theme/site/src/pages/Root.tsx:83 forwards it into ThemeProvider, and @myst-theme/providers/dist/theme.js:88 returns it, so OFFSET_PX resolves to 40. Only the consumer is missing.

2. Nothing can highlight, in the markup or the CSS. Outline.tsx:60-67 renders a bare <li> and <Link> with one static class set — no conditional class, no aria-current. Upstream's Headings component (DocumentOutline.tsx:55-77) toggles five classes off activeId. The deployed stylesheet /build/_assets/app-QEU53Y65.css (176,488 bytes) has zero aria-current rules and no .active selector at all — only Tailwind's :active state variants .active\:text-green-700:active and .active\:opacity-100:active.

3. The panel is not pinned. Outline.tsx:48-53 is classNames('relative self-start', containerClassName) wrapping <nav className="absolute inset-0 not-prose space-y-6 …">. That same stylesheet declares position in exactly three places — .qe-toc{position:fixed;…} (the left drawer), .fixed{position:fixed} and .sticky{position:sticky} — and the outline element carries none of them.

A fourth, smaller one worth cleaning up in the same pass: app/components/Page.tsx:15 is const { container } = useOutlineHeight();. The hook's second ref, outline, is never attached, and its setHeight opens if (!container.current || !outline.current) return; (DocumentOutline.tsx:320), so every page registers a scroll listener that does nothing.

Do not implement the pinning with position: sticky

The obvious fix — put sticky on the nav — ships nothing, and it will look like it worked in code review. self-start is align-self: start, which sizes the grid item to its content in the block axis, so a sticky nav inside a parent exactly as tall as itself has zero travel and scrolls away precisely as it does today. Dropping self-start does not rescue it: a grid item stretches to its own auto-sized row, not to the grid, and .simple-center-grid declares only columns (verified in the deployed CSS: display:grid;grid-template-columns:[screen-start] 1fr 200px 20px [body-start] 800px [body-end] 20px [margin-start] 200px [margin-end] 1fr [screen-end];align-content:flex-start). row-span-full does not either — Tailwind compiles it to grid-row: 1 / -1, and with no explicit rows line -1 resolves back to line 1, giving a one-row span.

The pattern that works is the one Sphinx ships, whose class name is actively misleading: the element is <div class="inner sticky"> but the declaration is position: fixed.qe-page__toc .inner.sticky{mask-image:linear-gradient(180deg,#000 calc(100% - 2rem),transparent);max-height:calc(100vh - 8rem);overflow-x:hidden;overflow-y:hidden;position:fixed;top:7rem;width:200px}. Keep the existing relative wrapper, make the nav position: fixed with an explicit top and width, and leave left and right at auto so horizontal placement falls back to the static position and the panel stays in the 200px margin track (tailwind.config.js:14).

Do not copy top: 7rem as a value. It computes to 126px on the Sphinx site because that build's root is 18px; this theme's root is 16px and its geometry differs anyway — the navbar is 50px (root.tsx:214) and main carries pt-[72px] (Page.tsx:41). Measure this theme's own first-content line and set top from that.

Ship max-height and overflow-y in the same change. Adding h3 roughly triples the panel — 5 entries become 14 on the matplotlib page — which is exactly why Sphinx caps it at calc(100vh - 8rem) behind a mask fade, with overflow-y: auto only on :hover, :focus-within.

The Sphinx colours you would copy are not the ones Sphinx intends

Worth knowing before anyone lifts values out of quantecon-book-theme.css. That file says .qe-page__toc-nav ul li a.active{color:#0072bc;opacity:1} and .qe-page__toc-nav ul li.active>a{color:#0072bc;font-weight:700;opacity:1} — QuantEcon blue, bold. Neither reaches the page. pydata-sphinx-theme's .toc-entry a.nav-link.active (three classes) outranks the book theme's .qe-page__toc-nav ul li.active > a (two classes, three elements), so what the reader actually sees is --pst-color-primary.

State book-theme intends deployed Sphinx renders contrast on white
resting #444 at opacity .8 #48566b at opacity .8, compositing to #6d7889 4.47:1 ✗
active #0072bc, weight 700 #0a7d91, weight 600, 3.375px inset left rule 4.82:1 ✓

Take the intent, not the accident. QuantEcon blue #0072bc is 5.08:1 on white and is already this theme's link colour (styles/quantecon.css:347, --qe-link-color), so an active outline entry in that blue reads as one system rather than introducing a third blue. Keep a non-colour cue alongside it — weight, or Sphinx's left rule — so the active state is not colour-only (WCAG 1.4.1); this matters more here than in Sphinx because the same review is separately proposing colour-only treatments elsewhere. Do not copy the opacity: .8 resting treatment: composited it lands at 4.47:1, under AA. The theme's current inherited #44403c is 10.27:1; if a quieter resting state is wanted, #44403c at .8 composites to #696663 and still clears AA at 5.70:1. For dark mode, Sphinx uses --qe-dark-link: #6cb6ff, which is 7.40:1 on this theme's #222 ground (tailwind.config.js:34, qepage-dark) — but Sphinx's own dark ground is #1a1a2e, so re-check rather than transplant.

Where the rule goes: there is no qe-outline class today — grep -rn "qe-outline" app/ styles/ returns nothing — so add one to the nav in Outline.tsx, the same hook BackToTop already uses in that file (Outline.tsx:14 carries qe-back-to-top, styled at quantecon.css:452), and a plain .qe-outline a[aria-current] block in styles/quantecon.css then wins uncontested. The outline's anchors are bare Remix <Link>s with no class, the nav is not-prose, #167's link colouring is scoped to .link and .hover-link (quantecon.css:344-365), and the shipped CSS has no competing .active or aria-current rule. There is no specificity contest to lose. Set aria-current="location" on the active link and aria-label="On this page" on the nav — the Sphinx build has no ARIA here, so this is a deliberate improvement rather than a parity item.

While rewriting the resting state, delete text-opacity-80 on the nav (Outline.tsx:51) and text-opacity-100 on the heading (Outline.tsx:57). Both are inert: they compile to custom properties (.text-opacity-80{--tw-text-opacity:.8}, confirmed in the deployed CSS) that do nothing unless a text-<color> utility on the same element consumes them, and neither element carries one. Measured opacity on the deploy is 1.

h3 entries, and the numbering at line 64

The selector excludes h3 — 'main h2' at line 46, against upstream's main h1, main h2, main h3, main h4 default (DocumentOutline.tsx:18). That is 5 entries where Sphinx shows 14 on the same lecture. If Decision 2 goes to h3s, the selector widens to 'main h2, main h3'; useHeaders normalises depth relative to the shallowest heading found, so h2 becomes level 1 and h3 level 2 and the render can indent on h.level (Sphinx indents with .qe-page__toc-nav ul ul{padding-left:1rem}).

Once h3s are in, Outline.tsx:64 breaks. It is pageEnumerator ? \${pageEnumerator}.${i + 1}. ${h.title}` : h.title— numbering by list index, which is only ever right for a flat list of h2s. With h3s included, the fourth entry on the matplotlib page would be labelled "11.4." where the heading itself reads "11.2.1.". The companion issue (#NN) about the in-content{tableofcontents}enumerators cites this same line as evidence that theme-rendered enumerators do carry a trailing period. That is correct and nothing here disputes it: the period is right, the number is what breaks. The real number is already in the DOM —myst-to-reactemits{enumerator}beside.heading-text on every heading (node_modules/myst-to-react/dist/heading.js) — so h.element.querySelector('span.select-none')?.textContentyields exactly "11.1." and "11.2.1.". It needs a fallback: the reviewed build setsnumbering: {titles: true, headings: true, heading_1: false} (lectures/myst.yml:70-73`), so the span exists there, but this repo's own fixture sets no numbering at all and other lecture repos may not either. With no span, fall back to the bare title rather than emitting a wrong number.

#96 is part of this, not a follow-up

The QuantEcon logo and the "Powered by MyST Markdown" block live inside the nav being pinned (Outline.tsx:71-87). Once that element is fixed-positioned with its own max-height and internal scroll, both change behaviour whether or not anyone touches them — on a long lecture the logo will simply be pushed out of the pinned box. #96 asks to shrink the logo and lift it to the top of the menu; do it in this PR. Sphinx keeps its equivalent block outside the scrolling area (.qe-page__toc-footer{font-size:.9rem;margin:2rem 0 0;position:sticky;top:6rem}), which is one workable arrangement; lifting the logo above the list per #96 is another. It is one decision about one element and should not be split across two PRs.

Decisions

1. What is "lost" measured against? The review's wording implies a regression, and there isn't one here. git log origin/main -- app/components/Outline.tsx returns 5 commits; the newest, 0842d0dbc (#155), is 9 insertions and 2 deletions all inside BackToTop, and the 2025-02-14 original already has every one of the three omissions. @DrDrij — is the comparison the Sphinx lectures, an earlier preview built with the archived QuantEcon/quantecon-theme bundle, or a stock jupyter-book build (where upstream's DocumentOutline, which does highlight, would have rendered)? It does not change what gets built, but it decides whether something else regressed alongside it and is worth ruling out.

2. h3 sub-entries — always visible, or collapsed to the active branch? Sphinx collapses them: .inner.sticky[data-autoexpand=true] .qe-page__toc-nav ul ul{display:none;max-height:0;opacity:0;…} with li.active>ul, li.expanded>ul{display:block;max-height:500px;opacity:1}. Measured, 3 of 9 h3s were visible at 1500px scroll. Options: (a) reproduce the collapse — keeps the panel short and gives the active state a second job, but is the largest piece of new behaviour here; (b) show the full nested list always — simpler, always scannable, leans on max-height plus internal scroll; (c) stay h2-only and fix pinning and highlighting alone — smallest change, but leaves the panel materially less useful than Sphinx's on a long lecture. Recommend (b), with (a) as a follow-up if the panel proves too tall in review.

3. Entry type size. Entries render at 18px here against Sphinx's 16.2px, in a 200px column, so this theme's list wraps considerably more. 18px is not wrong for prose, but an outline is navigation chrome, not reading copy, and Sphinx's smaller size is the better call in that width. Either take it in this PR or leave the size alone deliberately and record why.

4. Per-entry copy-link. Sphinx gives each entry a hover copy-link (.toc-copy-link, an SVG that copies the section URL). Proposed out of scope here — it is a separate affordance, not part of the scroll behaviour — but say so explicitly rather than losing it.

Three records that say this is already at parity

All three assert parity that does not exist, and between them they are why this was never scheduled. Correcting them is part of this work, not a follow-up.

  • PLAN.md:57 — the Already at parity row lists "On this page" TOC + back-to-top with ✅ / ✅.
  • Issue Technical Review & Feature Comparison — February 2026 #33 (closed), two rows of its comparison table: | **Right Sidebar (On This Page)** | ✅ Within-page ToC with toc-hN classes, sticky option with scroll highlighting | ✅ Outline component in margin column with useHeaders hook | ✅ Parity | and | **Sticky ToC** | ✅ sticky_contents with back-to-top and scroll spy | ✅ Outline is absolute positioned, BackToTop appears on scroll | ✅ Parity |. The second row states the defect and then grades it parity.
  • Book-theme parity cutover — tracking #147's parity table should record the correction so the cutover gate is not understated.

Testing

The full-page snapshots cannot verify this. theme.spec.ts:33 takes them with fullPage: true, and the file already records why that is a problem for pinned elements — the comment above the sidebar-open test at lines 196-198 says "stitching a scrolled page with a fixed overlay produces artifacts". Add a behavioural assertion instead: scroll to a known section, assert [aria-current="location"] is the expected entry. And give the fixture an h3 and a page enumerator so nesting and numbering are exercised at all — grep -rn '^###\? ' tests/visual/fixture/ currently returns only ## headings (intro.md:6, features.md:35/52/63/76/84, lists.md:43), and tests/visual/fixture/myst.yml.in sets no numbering, so pageEnumerator is undefined throughout the suite and line 64's numbering path has never run under test.

Baselines and sequencing

Every desktop-chrome snapshot moves — features, history-open, intro, lists, notebook, sidebar-open, in both desktop-chrome-darwin/ and desktop-chrome-linux/. That is 12 PNGs today, 14 once #174 lands and adds rtl.png. The desktop project is exactly 1280px (playwright.config.ts:55), which meets the @media (min-width:1280px) unlock, so the margin column is in those baselines. mobile-chrome should not move — the deployed CSS gates the column with .simple-center-grid>.lg\:col-margin{display:none} and the mobile project is Pixel 5 at 393px (playwright.config.ts:60) — but verify rather than assume.

The change may fall under maxDiffPixelRatio: 0.01 (theme.spec.ts:34, :125, :210), because the outline is a narrow strip of a 1280px full-page shot. That is exactly the failure mode #113 describes. Refresh locally with --update-snapshots=all and eyeball the PNGs; sub-threshold changes are not picked up otherwise. The -linux set goes through an /update-snapshots PR comment.

Land after #171 and #174. None of #171, #174 or #175 touches app/components/Outline.tsx (checked with gh pr diff --name-only), so there is no source conflict, but both rewrite the desktop baselines this work moves, so it queues behind them on the same binary-conflict sequencing the combined CSS PR sets out once. #171 also edits PLAN.md in hunks covering old lines 48–55 and 59–65, either side of line 57; a change to line 57 falls inside the three-line context window of both and will conflict.

Own branch, own PR. This is the only item from the review that changes component behaviour rather than CSS values, so it should not be folded into the combined CSS PR the other review items are heading for.

Where it fits

A parity gap against the deployed lectures, and the surface #33 and PLAN.md:57 both record as already done. Not part of Phase 3 (#89) and deliberately not #92. It absorbs #96, which should be closed by the same PR. Cross-links #129: a pinned, self-highlighting "On this page" panel helps the same reader who cannot easily get back to the contents mid-lecture, but it does not answer #129, which is about the left-hand site TOC and proposes breadcrumbs.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething is wrong or broken in a lecture or builddesign-reviewdiscussOpen-ended team deliberation or a decision to be made

    Type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions