Bug 210478 – [css-flexbox] percent children don't resolve against the flex basis on a fully inflexible item with fixed flex-basis

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Carlos Alberto Lopez Perez]

In a column flexbox, the height of a fully inflexible item with fixed flex-basis is definite, and percent children resolve against the flex basis, not the specified height.

See:
https://drafts.csswg.org/css-flexbox-1/#definite-sizes
https://drafts.csswg.org/css-flexbox/#change-2016-inflexible-definite

This is tested by WPT test http://wpt.live/css/css-flexbox/percentage-heights-006.html which fails on WebKit

[Sergio Villar Senin]

Created attachment 426701 [details]
Patch

[Darin Adler]

Comment on attachment 426701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426701&action=review

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1244
> +// This method is only called whenever a descendant of a flex item wants to resolve a percentage in its

I’m not a huge fan of the word "method" to mean "member function". Can we just say "function" instead?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1255
> +    if (child.style().flexGrow() == 0.0 && child.style().flexShrink() == 0.0 && childMainSizeIsDefinite(child, flexBasisForChild(child)))

Is this really needed only for the values of "exactly zero"? Are other very small values covered sufficiently by test cases?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1261
> -    auto updateDescendants = m_inLayout ? UpdatePercentageHeightDescendants::Yes : UpdatePercentageHeightDescendants::No;
> -    if (!canComputePercentageFlexBasis(child, Length(0, LengthType::Percent), updateDescendants))
> +    if (!canComputePercentageFlexBasis(child, Length(0, LengthType::Percent), UpdatePercentageHeightDescendants::Yes))

What makes the m_inLayout check no longer necessary?

[Sergio Villar Senin]

Comment on attachment 426701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426701&action=review

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1244
>> +// This method is only called whenever a descendant of a flex item wants to resolve a percentage in its
> 
> I’m not a huge fan of the word "method" to mean "member function". Can we just say "function" instead?

Sure

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1255
>> +    if (child.style().flexGrow() == 0.0 && child.style().flexShrink() == 0.0 && childMainSizeIsDefinite(child, flexBasisForChild(child)))
> 
> Is this really needed only for the values of "exactly zero"? Are other very small values covered sufficiently by test cases?

Yes the specs define "inflexible item" as an item whose flex-growth and flex-shrink are zero https://drafts.csswg.org/css-flexbox/#fully-inflexible

The main doubt I had was whether to express this as !child.style().flexGrow() or as in the patch. Being a float I decided to use the other syntax but not strong opinion here.

Regarding testing, using arbitrary low values are in practice the same as using arbitrary big ones in the sense that we'd end up distributing big chunks of space. There is one test checking that a huge value for shrink/grow does not crash, but apart from that I haven't seen others.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1261
>> +    if (!canComputePercentageFlexBasis(child, Length(0, LengthType::Percent), UpdatePercentageHeightDescendants::Yes))
> 
> What makes the m_inLayout check no longer necessary?

Because it was actually wrong. We do always want to update the hasPercentageHeightDescendants flag whenever this function is called. It used to work because in most of the cases this is called as part of layout.

[Sergio Villar Senin]

Committed r276634 (237062@main): <https://commits.webkit.org/237062@main>

[Radar WebKit Bug Importer]

<rdar://problem/77201507>