refactor: single scrollbar add and named slack in getHeadersWidth - #1274
Merged
Conversation
The vertical-scrollbar width was added to the active header band twice: once in the conditional gutter-attribution block and once more, unconditionally, in the band finalization - two generations of code expressing the same intent, with the second add also giving autoHeight grids a gutter for a scrollbar that does not exist. The band widths already carry the 1000px slack that guarantees the scroll-range floor, so the duplicate term contributed nothing. Exactly one conditional add remains, and the slack literal is promoted to HEADER_WIDTH_SLACK. The new headers-width-scroll-sync spec pins the observable contract (range floor, sync at full right scroll, last-column alignment) across plain, frozen-column and autoHeight grids; it passes on both the pre-change and refactored builds. Co-Authored-By: Claude Fable 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
2 participants
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.
Resolves the Q16 investigation item from the quirks triage (discussion #1247):
getHeadersWidth()added the vertical-scrollbar width to the active band twice — once in the conditional gutter-attribution block (from #1260, gated on!autoHeight), and once more, unconditionally, in the band-finalization block. Two generations of code expressing the same intent.Why the duplicate add contributes nothing
The band widths already carry the classic
+1000fixed slack (applied to the left band and again to the return value), which is what actually guarantees the two invariants the header width serves:A second ≤17px scrollbar term riding on a 1000px slack cannot be load-bearing. It was also wrong-shaped: the conditional add correctly skips
autoHeightgrids (which have no vertical scrollbar), while the unconditional one gave them a gutter for a scrollbar that doesn't exist.The change
1000literals are promoted to a named module constant,HEADER_WIDTH_SLACK, with a comment stating what the slack is for — the intent is now explicit rather than folklore.Net effect on computed widths: the header container gets ≤17px narrower (and autoHeight grids lose the phantom gutter). The container is clipped, and both scroll-range floors retain the full 1000px slack, so nothing is user-visible.
This also simplifies the #1238 rework: the ViewportMgr's
computeHeaderWidthscurrently mirrors the double-add bit-for-bit; after this lands it mirrors one sane formula.Test
cypress/e2e/headers-width-scroll-sync.cy.ts— a regression pin, not a bug repro (there is no fail-on-unfixed leg, and per that, no temporary example page). Self-hosting harness with three grids — plain,frozenColumn: 1(right band scrolls), andautoHeight— asserting for each: header scroll range ≥ body scroll range, header scroller reaches the body'sscrollLeftat full right scroll (no clamping), and the last column's header stays pixel-aligned with its body cells there. Verified to pass on the pre-change build (baseline) and on the refactored build, so any future width-formula change that breaks scroll sync fails this spec.Full cypress suite: all specs passed — 652 tests, 650 passing, 2 pre-existing skips (4m51s).
(Quirks-triage follow-up: with this, every actionable item from discussion #1247 is resolved on master — remaining entries are the documented keep-as-is decisions and the Q32 product call.)
🤖 Generated with Claude Code