Skip to content

Fix /whatsthat question-box overflow and desync'd reveal animation - #317

Merged
WilfordGrimley merged 3 commits into
masterfrom
fix-whatsthat-question-box-sizing
Jul 22, 2026
Merged

Fix /whatsthat question-box overflow and desync'd reveal animation#317
WilfordGrimley merged 3 commits into
masterfrom
fix-whatsthat-question-box-sizing

Conversation

@WilfordGrimley

@WilfordGrimley WilfordGrimley commented Jul 22, 2026

Copy link
Copy Markdown

Description

Owner blocker fix on the live /whatsthat quiz-reveal hero, post-#310. Two independent issues from live review, both addressed in this PR.

1. Question box too small for its content

Root cause (measured, not assumed): the task's a-priori hypothesis was "width:100% on a shallow-band SVG makes each word enormous" - that's not what's happening. Each word's own rendered box (whatsthat-word-* test ids) was correctly height-driven and intrinsically ~193px wide, exactly matching WhatsThatWords.tsx's own comment about align-items: flex-start keeping each word at its own intrinsic width. The real mechanism: HeroGrid's grid-template-rows: auto minmax(0, 1fr) sizes the auto row to HeroWordsArea's content height (measured 220px total for all three words, vs. wtc-mockup.html's own approved 164px at 1280px wide, itself measured directly off that file with its demo-only transform: scale() removed) - and since #310 bounded the whole hero to one viewport-height row, every pixel the words claimed came straight out of the questions row's 1fr share. At 1400x900 this left even Level 1 (suggested-match card + all four answer controls, no candidate grid to scroll) short by ~140px - forcing an internal scroll that clipped the card mid-view, exactly the reported symptom.

Fix: Word (WhatsThatWords.tsx) now sizes via clamp() of the viewport height, not a flat rem value - smaller than even the mockup's own absolute number, since the mockup was never height-constrained (its hero simply grew as tall as needed, with the page scrolling normally below - a constraint #310 introduced that the mockup never modeled). HeroGrid's row-gap and StarburstBackground's own padding/margin (whatsthat.tsx) were also trimmed at md+ - pure chrome spacing, not approved content, and every pixel of it was coming out of the same budget. The Level 1 reference thumbnail's maxWidth was cut as a separate, smaller lever once the above alone still left only single-digit-px margin.

Mid-PR complication: rebasing onto #313 (navbar/footer redesign, merged mid-task) revealed the new three-tier Footer is substantially taller (its own margin-top + top padding alone add 48px, before its actual three-tier content) - this ate a further ~140px out of the same budget, on top of the first pass's own trims. Re-measured and re-tuned a second time against the new baseline: word-stack clamp shrunk further (from a first-pass clamp(1.75rem, 3.4dvh, 2.25rem) to clamp(1.15rem, 2.1dvh, 1.6rem)), row-gap trimmed again (1rem -> 0.5rem), StarburstBackground's padding-top also trimmed (not just bottom, which is now 0 along with margin-bottom, since Footer's own top spacing makes that fully redundant), and the reference thumbnail cut again (140px -> 95px - a genuinely visible size reduction, ~55% of Level 2's own per-candidate tile width, called out explicitly rather than folded in quietly).

Verification: new hard assertion in QuestionFeedResponsive.spec.ts - at 1400x900, HeroQuestionsArea never overflows (scrollHeight <= clientHeight) and all four Level 1 answer controls are fully within the viewport (not just Playwright's own toBeVisible(), which only requires a non-zero intersection, not full containment). Confirmed to fail on the pre-fix code with the exact numbers reported (545 vs 405 budget pre-#313; 517 vs 394 post-#313), and pass post-fix with a real, measured ~30px margin (not just barely clearing by construction). Before/after screenshots taken this session (not committed, per this repo's own convention for ephemeral Playwright captures):

2. Reveal fade / word pop / card pulse desynced from a still-loading image (owner mid-task addendum)

Root cause: RevealOverlay's fade, WhatsThatWords' pop sequence, and CardPulseWrapper's pulse all started counting the moment their own elements mounted, independent of whether the subject card's <img> had actually finished loading - on a slow connection this could reveal/pop/pulse against a still-loading or half-painted image.

Fix: owner's redesign of the choreography - the card slot shows the blue cover and holds it until the image's load event fires, then the entire sequence (cover fade off + word pops + card pulse, still frame-for-frame in sync with each other) runs as one queue anchored to that single moment. Implemented via a shared imageLoaded boolean threaded into each animated component's own $playing/playing prop, gating each one's CSS animation-play-state (paused until told otherwise, running once imageLoaded flips) - the timeline (delay included) doesn't advance at all while paused, so flipping all three at once genuinely starts them in lockstep. A failed load keeps the cover up permanently with no animation (no legitimate "reveal" moment to sync to), while revealed still flips true so the rest of the question UI isn't stranded behind it. Reduced motion skips the whole animated queue and jumps straight to revealed on load, per the owner's "swap to the image without pops, immediately on load" instruction.

A genuinely empty configured URL (this test suite's own fixture convention - real cards always carry a real CDN URL) is treated as trivially settled, not a failure - resolved synchronously in the fetch handler, not via any browser event. This turned out to matter: an <img src=""> resolves its empty src against the current page's own URL (confirmed empirically, the URL spec's "empty string" case) and predictably fails to decode that as an image - relying on the resulting real onError event as the only settle signal for this case was flaky under dev-server load, which is exactly the failure mode a Playwright run surfaced while building this fix.

Real bug found and fixed along the way, not just the intended one: the settle logic's own catch-up effect was initially keyed on the card's identifier - but two consecutive feed items can legitimately share an identifier (documented already, for different state, in the fetch handler's own comment), and dev-mode React Strict Mode's double effect-invocation makes a duplicate resolution routine even outside that case. A duplicate resolution resets imageLoaded/revealed unconditionally but doesn't change the identifier, so the identifier-keyed effect silently skipped re-running, permanently stranding the UI on "Loading...". Fixed by keying that effect on a generation counter (imageGeneration) bumped unconditionally in the same reset block instead - full symptom/cause/fix writeup in docs/troubleshooting.md (new addendum to the existing, related "stuck loading spinner" entry, since this recurred on a genuinely fresh, isolated dev server and was a real code bug, not the stale-server artifact that entry originally described).

Verification: new test in WhatsThatWordsAnimation.spec.ts holds a real (route-intercepted) image response and asserts every one of the three animations is genuinely animation-play-state: paused before the response resolves and running once released - confirmed to fail on the pre-fix code ("running" instead of "paused"). The race-condition fix was verified via a 15-iteration --workers=1 loop with zero source edits mid-run (a single clean pass doesn't prove an intermittent race is fixed) - 15/15 passed.

Deviations from the task's literal scope

  • Structural chrome trims (HeroGrid row-gap, StarburstBackground padding/margin) and the Level 1 reference-thumbnail size cut were not explicitly authorized by the original task - the word-stack fix alone left the hard assertion short (~140px pre-Redesign navbar + add three-tier footer, cut nav to five surfaces #313; a further ~90px post-Redesign navbar + add three-tier footer, cut nav to five surfaces #313). Chosen as the lowest-risk levers (spacing/sizing, not approved question content) before touching anything else; each is commented in place with its own before/after numbers.
  • The animation-sync fix (item 2) was added mid-task per an explicit instruction from the coordinator, folded into this same PR as directed.
  • docs/troubleshooting.md and docs/features/printing-tags.md updated in place per this repo's own docs convention (symptom-first troubleshooting entry for a blocker that cost real debugging time; feature doc's quiz-reveal-hero section extended with this fix round, matching Fix round: /whatsthat quiz-reveal hero owner-review issues #310's own precedent).
  • CI follow-up fix, unrelated to /whatsthat: Frontend tests (2/4) failed identically on two consecutive CI runs on getByTestId('display-sheet-position-indicator') stuck at "2/3" instead of "3/3" (DisplayPage.spec.ts's D17 test). Traced to a real, pre-existing bug in DisplayPage.tsx's sheet-position IntersectionObserver: its fixed vertical center-band heuristic can never select the FIRST or LAST sheet once the scrollable container is already at its true scroll extreme and that boundary sheet is too short to move its own center through the band (confirmed on origin/master directly, pre-dating this PR's own diff - introduced with the D17 test itself in Redesign navbar + add three-tier footer, cut nav to five surfaces #313, where it was merely flagged "flaky" rather than caught as broken, since Redesign navbar + add three-tier footer, cut nav to five surfaces #313's own CI happened to get a lucky pass on retry). Not tied to Footer/Navbar sizing (/display renders no Footer) or to this PR's own /whatsthat changes. Fixed in DisplayPage.tsx by checking the container's real scroll-extreme (scrollTop/scrollHeight/clientHeight) first, inside the same observer callback, before falling through to the existing center-band logic - see docs/troubleshooting.md's new entry for the full measurement and fix writeup. Verified 5/5 clean D17 repeats, the full DisplayPage.spec.ts suite (28/28), and CI's own shard-2/4 test set (76/76) locally.

Checklist

  • I have installed pre-commit and installed the hooks with pre-commit install before creating any commits.
  • I have updated any related tests for code I modified or added new tests where appropriate.
  • I have manually tested my changes as follows:
    • npx tsc --noEmit - clean.
    • npx prettier@2.7.1 --check on every touched file - clean.
    • Full npx jest - 511/511 pass.
    • Full npx playwright test --project=chromium (302 tests) - 299 passed; the 3 failures (ChangeQueryModal, DisplayPage phone-viewport D17-family, ImportCSV) were unrelated to any file this PR's diff touches and were assumed pre-existing/flaky at the time. Correction (CI follow-up): the DisplayPage D17 failure was NOT flaky - it reproduces 100% of the time in isolation, both locally and in CI's own shard-2/4 - see the new "CI follow-up fix" deviation bullet above and docs/troubleshooting.md's new entry. Now fixed in DisplayPage.tsx, re-verified 5/5 clean plus the full 28-test DisplayPage.spec.ts suite and shard-2/4 (76/76) locally.
    • Targeted suites (WhatsThatWordsAnimation.spec.ts, QuestionFeed.spec.ts, QuestionFeedResponsive.spec.ts, WhatsThatPWA.spec.ts) - 36/36 pass, run clean multiple times.
    • New hard assertion (box-sizing) and new gated-animation test (sync fix) both independently confirmed to fail on pre-fix code and pass post-fix.
    • Real Playwright screenshots at 1400x900 (before pre-Redesign navbar + add three-tier footer, cut nav to five surfaces #313, before post-Redesign navbar + add three-tier footer, cut nav to five surfaces #313, after) and 390px (order/sticky-bar check) - described above, not committed (ephemeral captures, per this repo's own convention).
  • I have updated any relevant documentation or created new documentation where appropriate.
    • docs/features/printing-tags.md's quiz-reveal hero section extended in place with this fix round's sub-bullets.
    • docs/troubleshooting.md's existing "stuck loading spinner" entry extended with an addendum for the real (non-stale-server) cause found this session.

Word stack rendered ~1.4x the mockup's proportion and, since #310 bounded
the hero to one viewport row, ate directly into the question box's
budget - even Level 1 didn't fit without an internal scroll. Shrinks the
word stack via a viewport-relative clamp(), trims chrome-only spacing
(HeroGrid row-gap, StarburstBackground padding/margin), and trims the
Level 1 reference thumbnail, with a new hard Playwright assertion
(no overflow + all four L1 controls fully on-screen at 1400x900).

Also syncs the reveal fade / word pop / card pulse to the subject card's
actual image-load event (via a shared imageLoaded gate + each animation's
own animation-play-state), instead of firing independently at mount time.
Along the way, fixed a real dependency-array bug this surfaced: an
identifier-keyed catch-up effect silently skipped re-running on a
duplicate fetch resolution (legitimate per the existing chipStates
comment, and routine under dev-mode Strict Mode), permanently stranding
the UI on "Loading..." - fixed by keying it on a generation counter
bumped unconditionally in the reset block instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ailure)

Boundary-clamp the scroll-position IntersectionObserver in DisplayPage.tsx
so it can correctly report the first/last sheet at true scroll extremes;
the center-band heuristic alone could never do so once a boundary sheet
was too short to reach the band. Real, reproducible bug (not flaky),
pre-existing since #313, unrelated to this PR's own /whatsthat changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion-box-sizing

# Conflicts:
#	docs/troubleshooting.md
@WilfordGrimley
WilfordGrimley merged commit 711e132 into master Jul 22, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant