fix(flex): give justifyContent free space to distribute again - #51
Open
chiefcll wants to merge 1 commit into
Open
fix(flex): give justifyContent free space to distribute again#51chiefcll wants to merge 1 commit into
chiefcll wants to merge 1 commit into
Conversation
A flex row with no explicit width seeds w = 0 and only the flexStart branch of the layout resizes the container to fit its children. Every other justify mode computes against containerSize, so with a shrink-to-fit width they position children out of zero free space — producing negative offsets against a zero-width container rather than simply falling back to flexStart. This regressed in e7310f8, which switched non-fixed flex rows from filling the parent to seeding 0 to avoid a resize flash. Restore the previous default when justifyContent needs free space: that case never triggers the shrink-to-fit resize, so it can't flash. An explicit flexBoundary 'contain' still wins — that combination is a genuine contradiction, so honor the boundary and warn in dev. Replaces the #39 warning, which tested kebab-case strings against a camelCase union and so only ever fired for 'center'. Testing !== 'flexStart' avoids the mismatch entirely and drops the `as string` cast that hid it from the compiler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@pecoram - you may like the last sentence in What section. LMK thoughts before I merge this |
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.
What
A flex row with no explicit
widthseedsw = 0, and only theflexStartbranch of the layout resizes the container to fit its children. Every other justify mode computes positions againstcontainerSize, so with a shrink-to-fit width they have no free space to work with.This restores the pre-regression default: when
justifyContentneeds free space, the container falls back to filling its parent. An explicitflexBoundary: 'contain'still wins.Follow-up to #39, prompted by @konclave's comment there — they were right that #38's direction was the correct one.
Why
Two separate problems.
1. This is a regression, not intended behavior.
git log -S flexFitsWidthpoints at e7310f8 ("for flex rows the grow start with w 0 to avoid flash"), which changed the default fromprops.w = node.flexGrow ? 0 : parentWidth - props.xto seeding0for any non-fixed flex row. Before that commit<view display="flex" justifyContent="center">filled its parent and centering worked.The flash that commit was avoiding doesn't apply here: a container that fills its parent for
center/spaceBetween/etc. never gets resized by layout, so there's no width jump to flash. Only theflexStartpath — the one that does shrink — keeps thew = 0seed.2. The failure mode is worse than "falls back to flexStart." With
containerSize = 0,centercomputes(0 − totalItemSize) / 2, so children get negative offsets against a zero-width container.spaceBetween/spaceAround/spaceEvenlyproduce negative spacing and overlap.3. The #39 warning is ~80% dead code.
justifyContentis a camelCase union ('flexStart' | 'flexEnd' | 'center' | 'spaceBetween' | 'spaceAround' | 'spaceEvenly'), but the warning tests kebab-case strings:Only
'center'matches, so it never fired for the four cases most likely to confuse someone. Theas stringcast is why the compiler didn't catch it.Approach
!== 'flexStart'instead of listing the modes that need space. Correct by construction, no string list to keep in sync, and no casing to get wrong. Theas stringcast is gone, so TypeScript checks these comparisons now.flexBoundary: 'contain'alongside a justify mode that needs space. The developer asked for two incompatible things; honor the boundary and say so.What this changes for existing apps
This is a behavior change, not just a bug fix. Measured before/after on a 1000px-wide parent with two 100px children:
justifyContent="center"on a row positioned atx={500}widthx0-100500150Before, the content ended up visually centered on the container's own x. If anyone reached for
x={centerPoint}+justifyContent="center"and it looked right, it was right by accident — and it moves now. The fix centers withinx → parent right edge, which is what the old pre-e7310f8 behavior did.justifyContent="flexEnd"on a row atx={400}widthx0-100600500Same shape of break: content used to right-align ending at the container's x, now it right-aligns to the parent's right edge.
Containers with a
coloror border. Width goes from0to the parent width, so a background that was silently invisible now paints across the full parent. Worth grepping fordisplay="flex"+justifyContent+colorwith no width.Anything reading
.widthon these containers — customonLayouthandlers, scroll math, measurement code — now reads parent width instead of0.spaceBetween/spaceAround/spaceEvenlywere producing negative spacing and overlapping children, so I'd be surprised if anyone is depending on those.Workaround for all of the above: set an explicit
width, or addflexBoundary="contain"to keep shrink-to-fit (you'll get a dev warning explaining the tradeoff).Known limitation
The width is still chosen once at node creation, so:
justifyContentthat becomes non-flexStartreactively after render won't re-pick the width.parent.wis0at creation time, soparentWidth - xis0and nothing improves. Verified: outerw=0, innerw=0, childx=-100, unchanged by this PR.Fixing either means moving container sizing into the layout functions' non-
flexStartbranches, which is a bigger change than I wanted to fold in here.Tests
New
tests/flex-justify-width.test.tsx— render-level, because the existingtests/flex.spec.tscalls the layout functions directly with explicit container widths and can't reach this code path at all.centerandspaceBetweenwith no width → container fills parent, children land at the right offsetsflexStartand nojustifyContent→ still shrinks to fit (fix is scoped)flexBoundary="contain"+center→ stays contained, dev warning firesI confirmed test 1 is discriminating by stashing the fix and re-running: fails with
expected +0 to equal 600. Test 3 passes against both old and new code — the old kebab-case list happened to contain'center', the one value that matched — so it's guarding theflexBoundaryrefinement, which a plain #38-style fix would break.npm test171 passing,npm run tscclean,npm run lint0 errors.🤖 Generated with Claude Code