Table: stop shifting every column when a table gains a scrollbar - #1210
Draft
JeanMarcMilletScality wants to merge 3 commits into
Draft
JeanMarcMilletScality wants to merge 3 commits into
JeanMarcMilletScality wants to merge 3 commits into
Conversation
…ng it The head row sits outside the body's scroll container, so its tracks came out wider than the body's by the width of the scrollbar. That was closed by measuring the bar in JS -- injecting a throwaway overflow:scroll div, reading offsetWidth minus clientWidth -- and subtracting the result from the head row's width with !important. The arithmetic was right, so nothing looked wrong. The mechanism was the problem. Every column shifted sideways the moment a table gained its first scrollbar, which is the reflow the form's scroll gutter rejected padding-right to avoid. The probe measured a synthetic element rather than the table's own scroller, so it agreed only while both resolved to the same rules. It ran from a callback ref with no dependencies, so anything that changed the bar's width afterwards -- zoom, a theme with different scrollbar rules, the platform switching to overlay bars -- left the stored number stale. And !important put the head row's width beyond the reach of any consumer. Reserve the same gutter on both elements instead. The head row needs `stable` rather than `auto` because it is overflow hidden and so never shows a bar of its own. This removes useTableScrollbar's measurement, both props on HeadRow, and the !important. The visible trade is the one already accepted for the form: a table short enough not to scroll now reserves the gutter anyway, rather than reflowing when it crosses into scrollable. hasScrollbar stays on the table context. Nothing reads it now, but it is reachable through a supported deep import, so removing it is an API decision rather than cleanup.
…ow lists Both selectable-content components still destructured setHasScrollbar from useTableScrollbar and never used it: the one call site that reports scrollability lives in TableRows. Nothing read the value here even before the gutter change.
…ead row The comment explaining why the gutter is stable rather than auto keeps its why; the sentence describing the measurement approach it replaced belongs in the PR body, not beside the CSS.
Contributor
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
This branch has not been deployed
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.
TL;DR — Table: every column in the header jumped sideways the moment a table grew long enough to scroll; the header and the body now reserve the scrollbar's space up front, so nothing moves.
Context / Why
The head row sits outside the body's scroll container, so its tracks come out wider than the body's by the width of the scrollbar. That was closed by measuring the bar in JavaScript and subtracting it from the header's width. The arithmetic was correct and nothing looked misaligned — this PR is about the mechanism, which had four failure modes and needed a hundred lines of props, state and a callback ref to hold up.
🧩 Approach
Reserve the same gutter on both elements and let the engine supply the width.
stablerather thanautoon the head row because that element isoverflow: hiddenand so never shows a bar of its own;autowould reserve nothing there. Measured on a live page with the library's own global scrollbar styling in effect, using 200px probes and reading backclientWidth:overflow-y: auto, scrollable — what the body scroller does todayoverflow-y: hidden+scrollbar-gutter: stable— what the head row now doesoverflow-y: auto+stable, content not overflowingoverflow-y: hidden, no gutter — the head row before this changeThe first three agreeing is the fix: header and body reserve the same width, and they keep reserving it when the table is too short to scroll. Two things the table no longer has to know — the number is the engine's own, so it follows a
thinbar (11px), a consumer'sauto(15px) ornone(0px) with no code change; and it is per-element, so a themed or zoomed bar cannot leave a stored value stale.Moving off the JavaScript measurement was the point rather than a side effect: it shifted every column the instant a table gained a scrollbar, which is the same reflow that
padding-rightwas rejected for in #1196, in the same release. The probe also measured a syntheticoverflow: scrolldiv rather than the table's own scroller, agreeing only for as long as both resolved to the same rules; it ran from a dependency-free callback ref, so zoom, a theme with different scrollbar rules or a switch to overlay bars left the stored number stale; and!importanton a runtime pixel value put the header's width beyond the reach of any consumer stylesheet.The visible trade is the one already accepted for the form scroll gutter in #1196: a table short enough not to scroll now reserves the gutter anyway, instead of reflowing when it crosses into scrollable.
🔧 Usage
HeadRowanduseTableScrollbarare reachable through the package's./dist/*subpath, so their shapes are public API even though the table renders them itself. Both call sites in this repo, before and after:hasScrollbarandsetHasScrollbarstay on the table context. Nothing readshasScrollbarnow, but removing it would be an API decision rather than cleanup, so it is left alone.🔍 Review focus
tablev2/Tablestyle.tsx › HeadRowandtablev2/TableCommon.tsx › useTableScrollbar— two props and two hook return fields are removed. A consumer deep-importing either gets a type error at build (the good case) or, if it rendersHeadRowfrom JavaScript, a header that silently stops compensating. Worth a search of consumer code before this merges.tablev2/TableCommon.tsx › SmoothScrollDiv— the gutter is applied via the inlinestyleprop alongsidescrollBehavior, so a consumer passing their ownstylecannot override it and cannot lose it either. Check that is the intended precedence.useTableScrollbar()call left over with nothing reading its result; the one call site that reports scrollability lives inTableRows.🧪 How to test
tablev2story in Storybook with a scrollable body.Follow-up
🔗 References
scrollbar-gutter: stableon the form's scroll area, rejectingpadding-rightfor the same reflow reason. This applies the same treatment to the table, and was split out of it deliberately.What changed
useTableScrollbarkeeps its name and its context passthrough but no longer measures anything: the probe div it injected, read and removed is gone with the two fields it fed. The head row'swidthdrops its!importantand is nowcalc(100% - 4px)unconditionally, where the 4px is the row's own border.🤖 Generated with Claude Code