Fix percentage min/max on flex items resolving against wrong ancestor#1903
Fix percentage min/max on flex items resolving against wrong ancestor#1903costajohnt wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| </div> | ||
|
|
||
| <div id="percentage_flex_basis_main_min_width" style="width: 200px; height: 200px; flex-direction: row;"> | ||
| <div id="percentage_flex_basis_main_min_width" data-disabled="true" style="width: 200px; height: 200px; flex-direction: row;"> |
There was a problem hiding this comment.
Is Chrome's behavior here wrong? What changes?
|
Change seems sane. Just curious about the case where we used to agree with Chrome, but no longer do. Or any references to spec, for what box we should be using as reference in this situation? |
|
The percentage_flex_basis_main_min_width test is Chrome-generated, and Chrome has a known spec deviation here. The test combines flex-basis: 15% with min-width: 60%. CSS Flexbox §9.2 step 3 says min/max main sizes should be ignored when determining flex base size ("no clamping occurs"). Chrome clamps anyway, so Yoga now diverges from Chrome on this specific case. Same thing already happens with the existing disabled test percentage_flex_basis_cross_min_height. For which box percentages resolve against: CSS Sizing 3 §5.1 says percentage sizes resolve against the containing block. For a flex item, that's the flex container (parent), not the grandparent. This is also what PR #1015 identified, but it couldn't be merged at the time because errata gating didn't exist yet. |
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this in D95014513. |
Oh, really? That part of the spec ("min/max main sizes should be ignored when determining flex base size") is a very annoying special case. I believe this is the only case in any css layout mode where a container needs to measure it's child without clamping, and it makes caching for Flexbox a lot trickier than it would otherwise need to be. If Chrome is clamping anyway, then I'd be very tempted to adopt this behaviour too. Perhaps a discussion to be had in the |
3ee5405 to
46df371
Compare
|
@nicoburns I need to correct my earlier comment. After looking at this more carefully, Chrome's output (120/80 for that test case) is actually the spec-compliant result. Chrome is not clamping during flex base size computation here. If Chrome were clamping the base size by min-width upfront, the numbers would be 128/72 instead. You can verify with a simple test page using the same styles from the fixture (flex-basis 15% with min-width 60% in a 200px row container). The test is disabled because my fix makes percentage min-width actually resolve correctly (before, it couldn't resolve because it was looking at the wrong ancestor). Once it resolves, Yoga applies it too early during the base size step, which gives 128/72 instead of Chrome's 120/80. That early clamping is a separate issue from this PR. So to answer your question directly, I don't have evidence of Chrome clamping because it looks like Chrome follows the spec here. Sorry for the confusion in my earlier comment. A csswg-drafts discussion could still be interesting though, especially if the no-clamping behavior is making caching harder for Taffy. |
|
Just checking in — anything else needed on this one? |
eea8c01 to
bed4504
Compare
|
Friendly ping. The "Import Status" check has been queued since April 11. If the internal diff has stalled, happy to rebase, split the change, or do whatever helps move it along. Otherwise no rush, just wanted to surface it in case it needs a poke. |
Percentage min-width/max-width/min-height/max-height on flex items resolved against the grandparent's owner size instead of the parent container's inner size. This is because boundAxisWithinMinAndMax() received mainAxisOwnerSize (the parent's parent) rather than availableInnerMainDim (the parent's inner content area) at three call sites. The fix is gated behind a new errata flag, FlexItemPercentMinMaxAgainstOwner, so consumers using Classic or All errata (e.g. React Native) automatically preserve the old behavior. Default (Errata::None) now produces correct W3C-conformant results. Fixes facebook#872
Ran yarn gentest -f YGPercentageTest to update the generated test files after rebasing onto upstream/main. The gentest tool now emits style properties in a different order, which accounts for the bulk of the diff.
Upstream fb8e618 (Fix "instrinsic" typo) silently merged into the branch during rebase, invalidating the previous SignedSource hash. Ran yarn gentest -f YGPercentageTest to produce a valid hash against the current main state.
bed4504 to
ea07e09
Compare
Summary
Fixes #872.
Percentage
min-width/max-width/min-height/max-heighton flex items resolved against the grandparent's owner size instead of the parent container's inner size. This is becauseboundAxisWithinMinAndMax()receivedmainAxisOwnerSize(the node's parent, i.e. the flex item's grandparent) rather thanavailableInnerMainDim(the node's own inner content area, i.e. the flex item's actual parent) at three call sites:calculateFlexLine()inFlexLine.cppdistributeFreeSpaceSecondPass()inCalculateLayout.cppdistributeFreeSpaceFirstPass()inCalculateLayout.cppThe fix is gated behind a new errata flag
FlexItemPercentMinMaxAgainstOwner, so consumers usingClassicorAllerrata (e.g. React Native) automatically preserve the old behavior. Default (Errata::None) now produces correct W3C-conformant results.This is a successor to #1015, which identified the correct fix but was not merged because it needed to be gated behind an errata flag (which didn't exist at the time).
Test plan
min-width: 50%andmax-width: 50%(Chrome-verified: child = 5px = 50% of parent's 10px, not 20px = 50% of grandparent's 40px)FlexItemPercentMinMaxAgainstOwnererrata preserves old behavior (20px)Classicerrata includes the new flagmin-height) works correctlypercentage_flex_basis_main_min_widthtest (data-disabled) where Yoga now diverges from Chrome (same pattern as existingpercentage_flex_basis_cross_min_height)gentest-validatepasses for all 75 generated files