Repository navigation
fix(live): keep legend toggle above VCR bar - #84
Merged
Merged
Conversation
…-2049-live-toggle-anchor
Kpa-clawbot#1833 round 2 anchored both .feed-show-btn and .legend-toggle-btn to .live-page via position:absolute so a viewport-fixed anchor could no longer drift out of sync with the VCR bar at 641-768px width. That change also introduced a new regression: .feed-show-btn's offset (stacked above .legend-toggle-btn) can rise far enough at short viewport heights to land underneath .top-nav, since .live-page sits flush with the viewport top on /live with .top-nav fixed on top of it rather than pushing it down. A real click on the button then does nothing, and the feed panel can never be un-hidden again at those sizes. A/B/C testing (master, PR #84's original feature-head, and the current master-integrated head, identical fixture + Playwright flow, 10-viewport matrix with 720x300 repeated 10x) confirmed this is a regression introduced by #84, not pre-existing: master passes 19/19 required cases; both #84-derived trees fail ~16/19 with the button geometrically overlapping and hit-testing under .top-nav. Fix: clamp .feed-show-btn's total bottom offset so it can never rise closer than .top-nav's height to .live-page's own top edge, while keeping position:absolute (matching .legend-toggle-btn, avoiding the fixed/absolute containing-block mismatch that an earlier attempt at this fix — reverting only .feed-show-btn to position:fixed — was empirically found to reintroduce as a different collision with .legend-toggle-btn at 641px width). The clamp only ever pulls the offset down from its existing VCR-bar-clearing value, so it cannot reintroduce the VCR-bar overlap Kpa-clawbot#1833 round 2 already prevents. Adds a regression test to the existing #84 E2E file covering the 720x300 case: real hit-test against .top-nav, and a real click verified against the actual liveFeed hidden-state change (not just CSS). Verified clean across the full required viewport matrix, the 641-768px width band at both short and normal heights (no collision with .legend-toggle-btn or #vcrBar), an SPA remount, keyboard Enter/Space activation, and all four corner hit-tests. Confirmed via 4 mutation tests that the new assertions actually catch: the original unclamped offset, legend-toggle reverted to fixed, a z-index-only non-fix, and a broken click handler that passes hit-testing alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fork-adapted port of Kpa-clawbot/CoreScope#2049.
.live-page, the same containing block as the VCR barWhy
The buttons were
position: fixed, while.vcr-baris absolutely positioned inside the shorter.live-page. At tablet widths the two coordinate systems differ by--bottom-nav-reserve; the VCR bar then covers the legend toggle and consumes pointer events.The production change is constant-time CSS positioning only. It adds no runtime loop, API call, dependency, configuration value, or customizer setting.
Validation
Fresh on commit
21c6c774d0e110129f2b242ea7c1aa6d4579632e:node test-issue-1107-live-layout.js— 16 passednode test-issue-1833-legend-toggle-vcr-offset.js— 5 passednode scripts/check-css-vars.js— passedtest-issue-1833-legend-toggle-clickable-e2e.jsagainst a local Go server — 4 passed at 720 px and 1440 px, including real pointer clicksgo test ./...incmd/server— passedgo test ./...incmd/ingestor— passedgit diff --check— passedBrowser validation used a migrated temporary copy of
test-fixtures/e2e-fixture.db; the checked-in fixture was not modified. The test server was shut down afterward.Baseline note
npm testreachestest-frontend-helpers.jsand reports two pre-existingfavStarassertion failures (705 passed, 2 failed). The same two failures reproduce on a clean worktree at base6334c42789bd15b7887f56ffc88446f803ef42c6. They are unrelated to this PR; this PR does not modifynodes.jsor those assertions.No deploy, release, dependency, cache-buster, or customizer changes.
Master sync (
21c6c774→ce496a4e)A separate independent feature-review of
21c6c774gave PASS, no blockers before this sync. Master had since moved ~38 commits ahead (through issue #86, #88, #89, #91), so this branch was brought up to date with an ordinary merge commit — no rebase, no squash, no amend.21c6c774d0e110129f2b242ea7c1aa6d4579632ece496a4e52904eb00676c926968c278cbccd982d21c6c774…(this branch) +4c5efa7b75efa46c0696286a8f800b68bb9935f9(master at sync time)563ca69966d8540bf4fc5ecbb06885bb8b41dff8, computed conflict-free withgit merge-treebefore touching any ref and confirmed identical to the actual merge commit's tree.Filescope, verified byte-for-byte:
public/live.css,test-issue-1107-live-layout.js, bothtest-issue-1833-*files) are byte-identical before and after the sync.public/live.jsandtest-live-multibyte-only-e2e.js(issue fix(live): restore the Multibyte only toggle before init awaits #88's reload-race fix) are byte-identical to current master..github/workflows/deploy.ymlandtest-all.share the only two files both sides touched; the diff against master is purely the two additive lines this PR already had (its own test registrations) layered next to master's newer ones. No trigger, permission, job,needs, or fork-guard changed — verified by a full diff against master (additive-only) and byTestForkGuardOnSideEffectJobs,TestForkGuardOnPublishSteps,TestForkGuardLeavesTestJobsRunning,TestReleaseFastPathWorkflowExists,TestDeployWorkflowNoLongerTriggersOnTags, all passing on the merge result.Fresh verification on an isolated
git archiveexport of the merge result (not the branch tip alone):git diff --check,node --checkon every changed/relevant Live JS file, YAML-parse of all six workflow files — all clean.test-issue-1833-legend-toggle-vcr-offset.js× 10 andtest-issue-1107-live-layout.js× 10 — 10/10 clean.test-issue-1833-legend-toggle-clickable-e2e.js× 5 — 5/5 clean (4 passed, 0 failed each run).test-live-multibyte-only-e2e.js(master's fix(live): restore the Multibyte only toggle before init awaits #88 test) × 5 — 5/5 clean (10/10 assertions each run), including its toggle-before-init and real-reload-persistence checks.test-reach-rank-e2e.js(Back/Forward SPA-navigation control) × 3 — 14/14 clean each run.test-issue-1206-vcr-overlap-e2e.js,test-issue-1244-live-vcr-row-hints-e2e.js,test-issue-1510-live-nav-pin-e2e.js,test-live-fullscreen-1572-e2e.js,test-live-vcr-mode-contrast.js— all clean.elementFromPointhits the button, real click and keyboard Enter both toggle state, no horizontal overflow, no console errors, unaffected by many/long legend entries, and (390×844) the button is genuinely hidden (not just visually) and does not intercept pointer events at its would-be location. 720×300 — see residual risk below.position: absolutedeclarations tofixedin a scratch copy. Both unit tests and the real Playwright hit-test failed on exactly the intended assertion — the E2E failure reproduced the real symptom verbatim (<canvas id="vcrLcdCanvas">from<div id="vcrBar">intercepting the click). Reverted before finishing; never committed.#feedShowBtnunder.top-navat short heights — investigated and fixed (commitf2dd3f0b)The residual risk noted above (feed-show button landing under
.top-navat short viewport heights, e.g. 720×300) was flagged at master-sync time as unclassified. It has since been definitively root-caused with a controlled A/B/C comparison and fixed in this PR.Classification: this was a regression introduced by this PR's own
position: fixed → absolutechange, not a pre-existing bug. A/B/C test — identical fixture data, identical Playwright flow ("hide feed, then click show"), three git trees (current master, this PR's original feature-head21c6c774, and the master-integrated headce496a4e), 10-viewport matrix (720×250/275/300/320/375/420, 641/700/768×300, 812×375) with 720×300 repeated 10× to rule out timing flakes:.top-nav(button center hit-tests to itself, no nav overlap — the click simply times out at an extremely cramped 250px height); it reproduces identically on the fixed code below, so it is not part of this regression's scope.21c6c774(original feature-head) andce496a4e(master-integrated head): both fail ~16/19 cases, all with the same signature —#feedShowBtnoverlaps.top-navby 12–39px andelementFromPointat the button's center resolves to the nav, not the button.Fix:
.feed-show-btnkeepsposition: absolute(unchanged from this PR's own.legend-toggle-btnfix, and deliberately not reverted toposition: fixed— an earlier attempt at exactly that was empirically found to reintroduce a different bug: at 641px width and normal (900px) height, afixed-positioned#feedShowBtncollides with the still-absolute#legendToggleBtnbecause the two positioning schemes drift apart under--bottom-nav-reserve, the same containing-block mismatch this PR's own legend fix exists to avoid). Instead,#feedShowBtn's totalbottomoffset is now clamped so it can never rise closer than.top-nav's own height to.live-page's top edge, regardless of viewport height; the clamp only ever pulls the offset down from its existing VCR-bar-clearing value, so it cannot reintroduce the VCR-bar overlap this PR already fixes for the legend toggle.New regression test (added to the existing
test-issue-1833-legend-toggle-clickable-e2e.js, not a new file): a 720×300 case asserting geometric non-overlap with.top-nav, a realelementFromPointhit-test at the button's center, and the actual functional consequence of a real Playwright click (#liveFeed's hidden state actually toggling) — not just CSS inspection.Verification:
#feedShowBtnand#legendToggleBtn.Tab-equivalent focus +Enterand +Spaceboth correctly toggle the feed.test-issue-1833-legend-toggle-clickable-e2e.js× 13,test-issue-1833-legend-toggle-vcr-offset.js× 5,test-live-multibyte-only-e2e.js(fix(live): restore the Multibyte only toggle before init awaits #88) × 5,test-issue-1107-live-layout.js,test-issue-1206-vcr-overlap-e2e.js,test-issue-1205-live-controls-anchor-e2e.js,test-issue-1244-live-vcr-row-hints-e2e.js,test-issue-1510-live-nav-pin-e2e.js,test-issue-1532-live-fullscreen.js,test-issue-1413-nav-overlap-e2e.js,test-live.js— all clean..legend-toggle-btntoposition: fixed→ red with the original VCR-bar overlap; (3) raising only#feedShowBtn's z-index without fixing its position → still red (geometric overlap assertion, not fooled by a z-index-only non-fix); (4) a broken click handler with geometry left untouched → hit-test-only assertions would pass (false negative), but the real functional-state assertion correctly fails, demonstrating hit-testing alone is not sufficient and the functional check is load-bearing..live-page's actual containing-block behavior, an independent empirical spot-check at 720×300 and 700×900, and the mutation tests' structural validity) — verdict: PASS, no blockers.git diff --checkclean;favStarremains the only (pre-existing, unrelated, confirmed reproducing identically on unmodifiedce496a4e) failure intest-frontend-helpers.js.No deploy, staging, or production contact was made at any point in this fix.