fix(table): floor fractional table width so columns don't overflow#10238
fix(table): floor fractional table width so columns don't overflow#10238ardittirana wants to merge 2 commits into
Conversation
calculateColumnSizes relies on cascadeRounding, which assumes the target column sizes sum to an integer. With a fractional table width (e.g. a percentage-based size) that invariant breaks and the rounded widths can exceed the available width by 1px, producing an unexpected horizontal scrollbar. Flooring the available width keeps the sum integral; integer widths are unchanged. Adds a regression test to TableUtils. Fixes adobe#9448.
ardittirana
force-pushed
the
fix-tablelayout-fractional-width
branch
from
7a60738 to
cbe5c8c
Compare
| () => 150, | ||
| () => 50 | ||
| ); | ||
| expect(widths).toStrictEqual([500, 500]); |
There was a problem hiding this comment.
I don't think this is what we actually want, then there's a .5px gap in the last column and the edge of the table, which could lead to odd rendering artifacts on higher resolution monitors.
Did you try assigning the decimals to the last column? in this case, [500, 500.5] so that the sum matches the table width? We should be able to handle it inside cascade rounding.
There was a problem hiding this comment.
Good call — you're right that flooring leaves a sub-pixel gap at the table edge. I've switched to your approach: cascadeRounding now assigns the leftover fraction to the last column so the widths sum exactly to the available width, with no flooring.
For 1000.5 with [1fr, 1fr] it now returns [500, 500.5] (and [334, 333, 333.5] for three 1fr columns) — flush with the edge, no gap and no overflow. Integer widths are unchanged, since fpTotal === intTotal in that case and nothing is added.
Updated the test to assert the widths sum exactly to the table width. Pushed in 8b60847.
There was a problem hiding this comment.
Thanks, it looks like there is a bug though. There are some failing tests that shouldn't be failing, such as react-aria/test/table/tableResizingTests.tsx:498:39
I can't recall if this was the reason it was assumed to be integers a long time ago.
Did you run the tests before committing and pushing?
Address review: instead of flooring the available width (which left a sub-pixel gap between the last column and the table edge), assign the leftover fraction to the last column inside cascadeRounding so the column widths sum exactly to the available width. Integer widths are unchanged.
2 participants
Closes
Closes #9448
📣 Description
calculateColumnSizesdistributes column widths and then callscascadeRounding, whose contract (per its comment) is "given an array of floats that sum to an integer…". When the table width is fractional — e.g. a percentage-based width derived from the page size — that invariant no longer holds: the per-column target sizes sum to a non-integer, and the cascade rounds the total up (e.g.round(1000.5) = 1001). The columns then sum to more than the available width, producing an unexpected horizontal scrollbar even withoverflow-x: clip.🔎 Fix
Floor
availableWidthonce at the start ofcalculateColumnSizesso the rounding invariant holds. Integer table widths are unchanged (floor(n) === n); only fractional widths are corrected, and columns can no longer round past the container.🧪 How verified
Added a regression test to
packages/react-stately/test/table/TableUtils.test.js:with
tableWidth = 1000.5and two1frcolumns, the result is[500, 500](sum 1000 ≤floor(1000.5)). On the current code it returns[500, 501](sum 1001), so the test fails before this change and passes after. ExistingTableUtilscases (integer widths) are unaffected.