refactor: rebuild the contents drawer on the Popover API - #144
Conversation
|
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
|
Reviewed the diff in full and ran a Playwright probe against the preview (opens on click, closes on outside click and Escape, drawer rect 1. Browser-support trade — accepted, but document it. Safari < 17 / iOS 16 and Firefox < 125 get no drawer and no toggle. The reasoning (a polyfill reintroduces the flash) is sound and I accept the trade, but please add a line to the 2. Please confirm drawer state across client-side navigation. In the probe, clicking a link inside the drawer lands on the new page with the drawer closed. On 3. Minor wording. The comments say the browser "supplies Follow-ups — agree with both suggestions. #135 is fully obsolete and I am closing it. #141 is being rescoped to its |
There was a problem hiding this comment.
Pull request overview
This PR rebuilds the contents drawer as a native HTML Popover (popover="auto") instead of a React-state-driven panel toggled via -translate-x-full. The browser now owns the open/closed state, which eliminates the first-paint FOUC that #123 patched with a zero-specificity critical-CSS rule, and comes with Escape, light-dismiss, focus management, aria-expanded, and removal-from-tab-order "for free". A closed popover is hidden by the UA stylesheet, so the drawer no longer depends on author CSS to stay off-screen — and it works on server-rendered HTML before hydration. Browsers lacking the Popover API get no drawer (both panel and toggle are hidden via @supports), a deliberate tradeoff. Two pre-existing toolbar layout bugs at narrow desktop widths are also fixed.
Changes:
- Replace React
useNavOpen/transform-class drawer with a Popover: new shared-ID module (contentsDrawer.ts),popover/popovertargetwiring, and all presentation moved to.qe-tocinstyles/app.css(widths, slide via@starting-style/allow-discrete, icon swap off:popover-open,@supportsfallback). - Remove the now-unnecessary
.qe-contents-sidebarcritical-CSS rule and theuseSidebarHeight/insetplumbing; addpopover.d.tstypings for React 18. - Toolbar fixes:
shrink-0on the logo and shifting gap/separator/padding frommdtolg; tests updated for the popover (new no-JS test, rewritten FOUC assertions, re-anchored no-thebe check).
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
app/components/contentsDrawer.ts |
New module holding the shared popover id and heading id so panel and toggle agree without importing from each other. |
app/components/ContentsSidebar.tsx |
Drawer becomes a popover="auto" div keyed on .qe-toc; drops React state and sidebar-height hooks; heading is now an <h2> labelling the <nav>. |
app/components/toolbar/SidebarToggle.tsx |
Button uses popovertarget (no onClick/state); icons are aria-hidden and swapped via CSS from the drawer's :popover-open state. |
app/components/NavigationAndArticleWrapper.tsx |
Removes useSidebarHeight, the container ref, and the inset prop now that height is CSS-driven. |
styles/app.css |
Adds the full .qe-toc component layer: fixed positioning, responsive widths via theme(screens.*), slide transition, reduced-motion handling, focus outline, icon swap, and Popover-unsupported fallback. |
app/root.tsx |
Drops the .qe-contents-sidebar critical-CSS rule and updates the explanatory comment (UA stylesheet now hides the closed popover). |
app/popover.d.ts |
New ambient module augmentation adding popover/popovertarget/popovertargetaction to React 18 JSX types. |
app/components/toolbar/Toolbar.tsx |
shrink-0 on the logo item and moves gap/separator/container padding from md to lg to fix distortion/overflow in the ~770–856px band. |
tests/visual/theme.spec.ts |
Re-targets the toggle by accessible name, adds a no-JavaScript drawer test, and re-anchors the no-thebe check to the heading. |
tests/visual/fouc.spec.ts |
Rewrites the drawer assertions to expect a hidden (UA-display:none) popover in both the guard and control cases; removes the off-screen epsilon logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Every bug this panel has produced — the flash on load, the animated correction, the platform-dependent guard assertion, the icon fade in #127, the width trap in #130 — came from one premise: the panel was always rendered and always painted, with visibility expressed as a transform that only exists once app.css has loaded. That guarantees an unstyled frame where it is visible and a correction afterwards that a transition will animate. Each fix so far has patched a symptom of it. A popover is hidden by the UA stylesheet while closed, so it is already invisible on the very first paint with no author CSS at all. There is no unstyled frame to correct, and so nothing for a transition to animate. The failure mode stops existing rather than being guarded against. What this removes: - `useNavOpen` from both components. There is no open/closed state in React at all now, so the drawer works before hydration and the button cannot desync from the panel. - The `useMounted` transition gate — nothing left to gate. - The `.qe-contents-sidebar` critical-CSS rule and its sync-list entry. - `useSidebarHeight` from both call sites. It bails unless one call site holds both its refs; ours each attached one, so both instances early-returned on every scroll event and did nothing. - The duplicate width utilities, so #130 cannot recur — the three widths are expressed once in CSS with no dependence on emission order. - The icon cross-fade, replaced by a `display` swap driven off `:popover-open` via `:has()`, so it cannot animate on first paint. Behaviour the browser now supplies for free: Escape, click-outside dismissal, the closed panel kept out of the tab order and the accessibility tree, and the invoker's expanded state (verified in the AX tree as false -> true). Both icons are aria-hidden and the button holds the single accessible name; previously each icon carried its own label, so a screen reader could announce both "Show" and "Hide" for one control. Verified against a real build in Chromium and WebKit: hidden on an unstyled first paint (0 boxes, display none), nothing animates when app.css arrives, slides 18-19 frames to x=0 on open, sized 250x750 in both, Escape and light dismiss both close it. The existing visual baselines pass untouched in both projects, so the rendered result is pixel-identical to what it replaces; test:fouc passes 2/2. Two details worth keeping in mind. The UA stylesheet sizes popovers `fit-content` in both axes, so height has to be reset to `auto` or the panel stops at its content instead of stretching to `inset-block`. And React 18's JSX types predate the API, so `popover`/`popovertarget` need the declarations in app/popover.d.ts (react-dom renders them correctly — both survive renderToString on 18.3.1). Non-supporting browsers (iOS <= 16, Firefox < 125) treat the attribute as inert and would render the panel permanently open, so it is hidden outright there via @supports; loading a popover polyfill would restore it if that tail matters.
Closing the drawer with Escape returns focus to the toggle, and the browser matches :focus-visible because the interaction was a keypress. The UA default outline is a tight 1px square that reads as a rendering glitch against the toolbar, so restyle it in the theme's blue with an offset and a small radius. Removing it was the alternative, but that leaves keyboard users on an unmarked control (WCAG 2.4.7); pointer users never see it, since a click does not match :focus-visible.
Two related layout bugs in the band just above the md breakpoint. Every desktop control switches on at 768px, but at 20px spacing the row's intrinsic width is about 856px, so between 768 and 856 the flex row had no slack. The only item that could absorb it was the logo: an img with a pinned height and an auto width, which Tailwind's preflight also gives max-width: 100%. It shrank with its flex item and the fixed height then distorted the aspect ratio, down to 0.93 against a natural 3.5 at 770px. Pin the logo with shrink-0 so it can never distort, and widen the item gap, the actions-cluster separator and the container padding at lg rather than md, which returns about 148px to the narrow band. Without that second half, shrink-0 alone just moves the deficit to the end of the row and pushes the last icons off the right edge. Measured 360-1400px: aspect holds at 3.5 throughout, no overflow at any width, and 58px spare at the tightest point — enough for the 32px the live-compute toggle adds on notebook pages.
The check that the notebook page actually loaded used
getByText("Notebook outputs").first(), which resolved to the contents
drawer's link to that page rather than its heading. It passed only
because the closed drawer used to be rendered and translated off-screen,
which Playwright counts as visible; as a popover the closed drawer is
display: none, so the link is correctly hidden and the assertion failed.
Match the heading instead, which is what the check was always after.
Review follow-ups on the popover drawer. The @supports fallback hid the panel on browsers without the Popover API but left the toggle visible, so those users got a button with an accessible name that did nothing when pressed. Hide the toggle with the panel: an absent control is better than a dead one. Its comment also claimed a polyfill in entry.client.tsx restored the drawer there — no such polyfill exists, and adding one is not a drop-in, since :popover-open is a pseudo-class and cannot be polyfilled. The comment now states the trade-off and what reversing it would cost, instead of describing work that was never done. The affected set is Firefox < 125 and Safari < 17, which includes every device that cannot be upgraded past iOS 16. Those browsers keep the in-page outline and the site navigation, so content stays reachable. Also: - the panel no longer imports its id from the button that opens it; both take it from a shared module - the drawer's <nav> is labelled by its own heading rather than repeating the button's "Table of contents", which screen readers announced twice - the lg/2xl breakpoints come from theme(screens.*) rather than being transcribed as 1024px/1536px, which would desync silently - reduced motion drops the transition entirely instead of shortening it to 1ms; display/overlay only need transitioning so the slide can outlast display:none, and under the preference there is no slide
The reason the drawer is a popover is that the browser owns the toggle, so it works on the server-rendered HTML before or without hydration — that is what removes the open/closed flash. Every other test runs with JavaScript on, where a React-state implementation passes identically, so the property was undefended: adding an onClick or a client-only wrapper would have lost it silently. Verified the test can fail: with the markup as shipped it passes, and with popovertarget stripped from the served HTML the drawer stays shut and the assertion goes red.
The comments explained the change rather than the code — how the old implementation behaved, what was measured, why one approach was rejected. That belongs in commit messages and PR history, not in files someone reads to make the next change. Cut to the constraints that still bind: what must stay in sync, what must not be added, what breaks if a rule is dropped. Roughly 70 lines removed, and the drawer's CSS commentary is about half its previous size. Also drops the `inset` prop from NavigationAndArticleWrapper. It existed only to feed useSidebarHeight, no caller ever passed it, and with that hook gone it was dead plumbing kept alive by the comment above it.
21710d1 to
75c2312
Compare
|
Detailed review of the updated PR (multi-agent pass over the full diff, with the load-bearing claims verified against the built Tailwind output, the installed First: this is a genuinely well-engineered change. The popover design is the right altitude for the FOUC class of bug, the trade-off commentary in the code and PR body is unusually honest, and the The one blockerThe open drawer paints above the search dialog. As a popover the drawer lives in the browser top layer, which paints above every z-indexed element. The search hotkey is a global keydown ( Two trades that deserve an explicit maintainer sign-offLight dismiss is broader than the PR body states. The body owns "clicking outside the drawer now closes it", but pointerdown-based dismissal also means selecting text, clicking a code cell, or clicking to focus before keyboard-scrolling closes the drawer. That ends its old role as a persistent side map while reading — including on 2xl viewports where the 350px panel overlaps nothing and persistence had no cost. If persistent-while-reading is worth keeping, The no-Popover-API fallback removes the TOC entirely for iOS 16 Safari and Firefox <125. The reasoning in the Small fixes worth folding in
Follow-up material, not blockingThe toolbar height 50px now exists in three comment-synced copies ( On the PR's suggestions for open PRsAgreed on both — #135 was already closed as superseded on 2026-08-22, and the #141 reduction plan (keep the Outline/BackToTop piece and the FOUC-control hardening, drop the drawer half) matches what we'd concluded independently — the plan is to rebase #141 onto this PR after it merges, re-applying its fouc.spec.ts init-script change on top of this PR's rewrite of that file. Posted by Claude on mmcky's behalf; review method: 8 independent analysis angles + adversarial verification of surviving candidates. |
…nt, harden tests Follow-up to the 2026-08-25 review of #144, applying the items that need no design decision. Light dismiss (`popover="auto"`) is accepted as-is. - Close the drawer when a modal dialog mounts. A popover paints in the top layer above the search dialog's backdrop and panel, and light dismiss cannot cover the Cmd/Ctrl+K route because it fires on pointerdown. A shallow childList observer on <body> catches Radix's portal container and calls hidePopover(). New test `drawer-closes-when-search-opens` pins it. - Hide `.qe-toc-toggle__close` in CRITICAL_CSS so both icons no longer paint side by side on the pre-app.css frame, and make the "needs no rule here" comment true. The FOUC guard now samples the icon: hidden in the main test, rendered in the control, which proves the inline rule does work. - `drawer-opens-without-javascript` moves into a `test.use({ javaScriptEnabled: false })` describe: the ordinary page fixture with the project's own baseURL, and a `toHaveCount(1)` presence guard before `toBeHidden`, which otherwise passes on zero matches. - The drawer title is a styled div again, not an <h2> that precedes every page's <h1> in DOM order while the drawer is open. - Reword the toggle comment: the browser exposes the expanded state in the accessibility tree; the DOM `aria-expanded` attribute reads null by design. - Note in app.css that the rem widths scale with the A+/A- control on purpose. - Changelog entries under [Unreleased] for the rebuild, its behaviour and browser-support changes, and the three toolbar/a11y fixes. Also merges main so the branch carries #155's critical CSS and changelog. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Pushed a follow-up commit onto this branch (with a merge of DecisionsLight dismiss: Drawer state across client-side navigation is not a regression. I probed a build of current Browser-support trade stands as accepted on 2026-08-22, and is now documented in the changelog. What the commit changes
Verified locally on the pushed tree
Not done here, for follow-upThe 08-25 review's non-blocking items are untouched: the three comment-synced copies of the 50px toolbar height, the now-dead @DrDrij please shout if any of this cuts across something you had in progress; otherwise, once CI is green on this push, I intend to merge and then rebase #141 on top as planned. Posted by Claude on mmcky's behalf. |
|
@DrDrij let me know when ready if you're happy with these changes and I'll merge. |
|
This is good to merge @mmcky. Thanks for the final tidy and hardening. |
|
thanks @DrDrij |
…econditions (#169) * fix: pin "back to top" to opacity 0 in the critical CSS BackToTop is hidden by opacity-0 and carries transition-opacity, so on any frame where the stylesheet is absent it paints at full opacity and fades out once the sheet lands. That frame is produced by the React #423 hydration recovery (#126), which re-renders the head and briefly drops the stylesheet after the component has mounted — so gating the transition on mount, as #141 tried, cannot prevent it. Measured in WebKit with the stylesheet delayed: 17-18 animating frames without this rule, none with it. The FOUC guard asserts the rule: the main test expects opacity 0, the control expects 1 with the inline block stripped. Supersedes the Outline.tsx change in #141. Relates to #126. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(fouc): sample first paint from an init script, assert control preconditions The React #423 hydration recovery restores the inlined critical <style> 150-240ms after DOMContentLoaded (#126). The control strips that block from the served HTML, so a post-domcontentloaded sample can see it restored and every control assertion flips at once. Sample in-page on DOMContentLoaded instead, before hydration is scheduled, and have both cases assert their own preconditions so a reshaped CRITICAL_CSS reports as a stale strip pattern. Carries the test half of #141 onto the post-#144 probes. Relates to #126. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Rebuilds the contents drawer on the Popover API, replacing the React state, the transform classes, and the critical-CSS rule that kept the panel off-screen.
Why
The drawer's open/closed state lived in React (
useNavOpen) while its visibility came from-translate-x-full. Neither exists on the first paint of a static-build navigation, so the panel rendered in flow and visible, then corrected itself onceapp.cssarrived. #123 patched that with a zero-specificity rule inCRITICAL_CSS; this removes the failure mode instead.A closed popover is
display: noneby the UA stylesheet. That holds on the very first paint with no author CSS at all, and it comes with the closed panel's links out of the tab order and the accessibility tree, plus Escape, light dismiss and the invoker'saria-expanded— none of which we now maintain.Because the browser owns the toggle, the drawer also works on the server-rendered HTML before hydration. There is a test for that specifically (see below).
Behaviour change worth flagging
popover="auto"means clicking outside the drawer now closes it, and Escape closes it. Previously it stayed open until the button was pressed again. This reads as an improvement on mobile, but it is a change, not a like-for-like port.Browser support — a deliberate trade
Browsers without the Popover API (Firefox < 125, Safari < 17, which includes anything that cannot upgrade past iOS 16) get no drawer: the
@supportsblock hides the panel and the toggle, so there is no button that looks operable but is not.A polyfill would restore it and is not a drop-in —
:popover-openis a pseudo-class and cannot be polyfilled, so every rule keyed on it needs a class-based stand-in, and the polyfill only runs after first paint, which reintroduces the flash this removes. The in-page outline and site navigation still reach every page on those browsers. Happy to revisit if that share matters more than assumed.Firefox 125–128 has popover but not
@starting-style: the drawer opens without sliding, which degrades cleanly.Also included
Two toolbar fixes found while testing this at narrow widths, both pre-existing on
main:max-width: 100%let it clamp to a shrinking flex item.shrink-0pins it.md(768px) but its intrinsic width is ~856px, so the last icons were pushed off the right edge. Gap, separator and container padding now widen atlgrather thanmd, returning ~148px to that band.Measured 360–1400px afterwards: aspect holds at 3.5 throughout, no overflow at any width, 58px spare at the tightest point — enough for the 32px the live-compute toggle adds on notebook pages.
Tests
Full suite green locally on macOS (20 passed, 4 skipped). CI renders against the linux baselines, which may need
/update-snapshots; no new screenshot baselines are introduced by this PR.drawer-opens-without-javascriptis new and guards the reason for the rewrite. Every other test runs with JS on and would pass against a React-state implementation, so without it anonClickor a client-only wrapper could take the property back silently. Verified it can fail: withpopovertargetstripped from the served HTML the drawer stays shut and the assertion goes red.fouc.spec.tsnow asserts the drawer paints nothing in both cases, including the control with every stylesheet stripped — it is the UA stylesheet doing the hiding, not anything we ship.Suggested follow-up on open PRs
#135 — obsolete, suggest closing. It removes a stray unprefixed
w-[250px]so the width bands are decided by intent rather than Tailwind's emission order (#130). This deletes that class string entirely; the widths are declared once instyles/app.css, so the ordering dependency is gone rather than disambiguated.#141 — partly obsolete, suggest reducing rather than closing. Its #127 half (the toggle icons animating their own correction) no longer applies: the icons swap with
displayoff the drawer's:popover-openstate, there is notransition-alland nouseMounted. But the other two pieces are not covered here and are still wanted:Outline.tsx/BackToTophas the same fade-on-first-paint shape and is untouched by this PR.<style>around 195ms in — is a real CI-flake fix and is independent of the drawer.If #141 lands first this will conflict, since it adds
app/hooks/useMounted.tsxfor a hook this removes the last drawer use of.