fix: stop the contents sidebar flashing open on page load - #123
Conversation
|
🎭 Visual regression results
Details
Flaky testsdesktop-chrome › theme.spec.ts › QuantEcon theme — visual regression › sidebar-open Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › launch-colab |
|
Nice piece of debugging — the diagnosis is right and the primary guard demonstrably works. I built each variant and traced the panel frame-by-frame in WebKit (serve
Also green: One thing I'd change before merge — the new sync-list comment in
Neither half quite holds. Resolved widths are 350 / 250 / 350 across the three bands (base A few smaller notes, take or leave:
Two things I hit while reviewing are pre-existing and out of scope here, so I filed them separately: #126 (React hydration failing on every load, which also makes the FOUC control test racy) and #127 (the sidebar toggle icons animate the same first-paint correction). |
|
@DrDrij here is a review pass. Can you let me know if this is helpful or not? Thanks. |
The sync-list comment was wrong on both halves. Resolved widths are 350/250/350 across the base/lg/2xl bands, not 250 — `.w-[350px]` lands after `.w-[250px]` in the compiled base layer — so the rule's width was off at two of three, and the used transform went -250px to -350px rather than staying identical. The rule was nonetheless correct, for a better reason: `translateX(-100%)` resolves against the element's own border box, so the right edge lands at `left + W - W` = 0 for any width. `width` and `height` were doing no work and are dropped; `position:fixed` stays so the panel does not push the article down while it waits. Verified off-screen with `right = 0` at all three bands, with no horizontal overflow now that the box is shrink-to-fit. That same property means the `mounted` gate is not load-bearing: with both states at -100% there is nothing for a transition to interpolate. Measured with the gate removed at 800px and 1280px — zero on-screen frames either way. Reworded from "either alone leaves an artefact" to what it is, insurance that keeps the component correct if the critical rule is later changed. Also: - `sidebarOnScreen` returns null rather than false when the element is absent. false is the value that passes the guard, so a renamed hook slipped past the main test and surfaced as a confusing control failure; both tests now assert presence with a message naming the cause. - Adds the missing CHANGELOG entry. Review by @mmcky on #123.
On the static build every navigation is a full document load, and the first paint can happen before app.css applies. Until it does, `-translate-x-full` means nothing, so the nav panel painted in-flow and fully visible. When the stylesheet finally arrived the transform resolved, and because `transition-all` was already on the element the correction was animated — the menu appeared to open, then slide shut over 300ms. Two guards, since either alone leaves a visible artefact: - Park the panel off-screen in the inlined critical CSS, so it is never visible on the first paint. Width/height/position match the Tailwind classes so the resolved transform is identical before and after app.css lands and nothing animates on arrival. - Withhold the transition classes until after mount, so any remaining correction is applied instantly rather than animated. Also narrows `transition-all` to `transition-transform`: the former animated 15 properties (background, borders, padding, tab-size) where only the slide was wanted. Covered by the existing FOUC guard, which already isolates the inline critical CSS and now asserts the panel starts off-screen.
The sync-list comment was wrong on both halves. Resolved widths are 350/250/350 across the base/lg/2xl bands, not 250 — `.w-[350px]` lands after `.w-[250px]` in the compiled base layer — so the rule's width was off at two of three, and the used transform went -250px to -350px rather than staying identical. The rule was nonetheless correct, for a better reason: `translateX(-100%)` resolves against the element's own border box, so the right edge lands at `left + W - W` = 0 for any width. `width` and `height` were doing no work and are dropped; `position:fixed` stays so the panel does not push the article down while it waits. Verified off-screen with `right = 0` at all three bands, with no horizontal overflow now that the box is shrink-to-fit. That same property means the `mounted` gate is not load-bearing: with both states at -100% there is nothing for a transition to interpolate. Measured with the gate removed at 800px and 1280px — zero on-screen frames either way. Reworded from "either alone leaves an artefact" to what it is, insurance that keeps the component correct if the critical rule is later changed. Also: - `sidebarOnScreen` returns null rather than false when the element is absent. false is the value that passes the guard, so a renamed hook slipped past the main test and surfaced as a confusing control failure; both tests now assert presence with a message naming the cause. - Adds the missing CHANGELOG entry. Review by @mmcky on #123.
The new guard asserted the closed nav panel's right edge was not > 0. That is knife-edge: on the unstyled first paint the panel has no width class yet, so it is shrink-to-fit and lands on a fractional width (135.171875px in WebKit on macOS). WebKit snaps the painted translateX(-100%) to a whole device pixel but leaves the border box fractional, so the measured edge comes back at +0.171875 instead of 0. The remainder depends on the intrinsic width, hence on platform font metrics — which is why the assertion passed on the ubuntu runner and failed locally on macOS against identical, correct markup. Report the measured edge instead of a boolean and compare it against a 1px tolerance. The control case measures ~1272px of a 1280px viewport, so the two states stay three orders of magnitude apart. The missing-element guard is kept and its rationale updated: `null` coerces to 0, which would pass the off-screen comparison, so a renamed hook would still slip past the main test without the explicit check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c023e7a to
f1659ff
Compare
|
Rebased onto RebaseThe only conflict was The fix is soundI checked the mechanism rather than taking the description's measurements on trust. Serving the visual fixture with all external stylesheets aborted, the closed panel's right edge measures:
The zero-specificity reasoning holds, and so does the invariant the block comment now records — every property the critical rule sets ( One defect, fixed in
|
|
thanks @DrDrij -- merging. |
2 participants
The contents sidebar flashes open for a moment on page load, then slides shut. Reported against the Netlify preview of QuantEcon/lecture-python-programming#363.
Cause
Not a state bug — the server-rendered markup is correct and already carries
-translate-x-full.On a static build every navigation is a full document load, and the first paint can happen before
app.cssapplies. This is the same FOUC as quantecon-theme-src#66, which the inlined critical CSS already addresses for the font and the content grid — but that critical CSS never covered the sidebar.So the sequence is:
app.css.-translate-x-full,fixedandw-[250px]all mean nothing, so the panel lays out as a plain in-flow block, full width and fully visible.app.cssarrives andtransformresolves fromnonetotranslateX(-100%).transition-all duration-300had been on the element since that first paint, the browser animates the correction.The menu is never actually opened. It was simply never hidden, and then took 300ms to put itself away — which is why a one-frame glitch reads as a deliberate animation.
Measured
Two builds of this repo, served with
myst starton the visual fixture.app.cssis served empty so the first paint happens unstyled — the field condition — then the real stylesheet is applied and the panel traced every frame.Note that simply delaying
app.cssdoes not reproduce this: Chrome then blocks rendering and never paints, which is the healthy path. The flash needs a paint that beats the stylesheet.Fix
app/root.tsx— a.qe-contents-sidebarrule in the existing critical CSS parks the panel off-screen on the first paint. It deliberately sets no width:translateX(-100%)resolves against the element's own border box, so the right edge lands atleft + W − W= 0 for any width. That holds beforeapp.cssarrives and after, even though the resolved width differs (350/250/350 across the base/lg/2xl bands).position:fixedis kept so the panel does not push the article down while it waits.app/components/ContentsSidebar.tsx— the transition classes are withheld until after mount. This is insurance rather than the primary guard: because both states are a −100% translate there is nothing for a transition to interpolate, so removing it does not by itself reintroduce the flash (measured at 800px and 1280px — zero on-screen frames either way). It is kept because the transition is only ever wanted in response to a click, which keeps the component correct if the critical rule is later changed.transition-allis also narrowed totransition-transform. The former animated 15 properties when only the slide was ever wanted; the latter is compositor-only.Opening and closing the menu is unaffected — it still animates normally once mounted (
-250 -> 0 -> -250, animatingtransformalone).A trap worth recording
The critical CSS uses
:where(), so it carries zero specificity. That means every property set there must also be declared by the real stylesheet, or it can never be overridden. Hiding the panel withvisibility: hiddenwould have pinned it shut permanently, since no Tailwind class setsvisibility. Hence the transform. This constraint is now documented in the block comment.Tests
The existing WebKit FOUC guard covers this, since the cause and the guard are the same. It now also asserts the panel starts off-screen — and its control case asserts the flash does reproduce when the inline rule is removed, so the guard cannot silently rot.
sidebarOnScreenreturnsnullrather thanfalsewhen the element is missing.falseis the value that passes the guard, so a renamed hook would slip past the main test and surface as a confusing control failure instead; both tests now assert presence with a message naming the cause.Review follow-ups
Applied from @mmcky's review:
app.css. Neither held — widths are 350/250/350, and the used transform moved −250px → −350px. The rule was still correct, for the better reason now documented, sowidthandheightare dropped.mountedgate.sidebarOnScreenfallback described above.CHANGELOG.mdentry.One thing worth filing separately: the intended 350/250/350 only holds because Tailwind emits
.w-[350px]just after.w-[250px]in the base layer. Nothing pins that order — if it flips, base and 2xl silently become 250px and no test notices. The element carries duplicate unprefixedw-[350px]andw-[250px], which is the root of it.