Bug 214292 – WPT test css/css-flexbox/flex-minimum-height-flex-items-023.html fails

• Component: CSS
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

WPT ref-test css/css-flexbox/flex-minimum-height-flex-items-023.html which has been imported in bug 214284 fails on WebKit.

The failure on wpt.fyi: 

https://wpt.fyi/results/css/css-flexbox/flex-minimum-height-flex-items-023.html?label=master&label=experimental&product=chrome&product=edge&product=firefox&product=safari&product=webkitgtk&aligned&q=css%2Fcss-flexbox%2Fflex-minimum-height-flex-items-023.html

[Radar WebKit Bug Importer]

<rdar://problem/65864835>

[Philip Jägenstedt]

The origin of this test is https://bugs.chromium.org/p/chromium/issues/detail?id=1088223, and that might provide clues about what's wrong. There's a very detailed comment in the test itself as well.

[Sergio Villar Senin]

(In reply to Philip Jägenstedt from comment #2)
> The origin of this test is
> https://bugs.chromium.org/p/chromium/issues/detail?id=1088223, and that
> might provide clues about what's wrong. There's a very detailed comment in
> the test itself as well.

Do you think this particular one is a good target from the browsers interoperability POV? Sometimes it's quite difficult to know what's important for web authors and how to priorize bug fixing.

[Philip Jägenstedt]

Because this was reported as a regression by someone I assume to be a web developer, this situation is something we can be pretty sure has affected at least one person on the real web.

Beyond that, it involves the sizing of image as a flex item (although it's also a flex container?) and that's something that came up multiple times in https://mdn-web-dna.s3-us-west-2.amazonaws.com/MDN-Browser-Compatibility-Report-2020.pdf

So without going out to look for this case in the wild, I'd say it seems worth investigation at least.

More generally with Flexbox, since the signals that it's a pain for web developers are so strong, I'd just err on the side of aligning implementations, and treating as low priority mostly tests that originated from browser vendors or standards folks as an edge case which seems exceedingly unlikely to happen in the wild.

[Sergio Villar Senin]

Created attachment 421822 [details]
Patch

[Sergio Villar Senin]

Created attachment 421914 [details]
Patch

[Javier Fernandez]

Comment on attachment 421914 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:432
> +    bool childBlockSizeIsEquivalentToAutomaticSize  = !mainAxisIsChildInlineAxis(child) && (minSize.isMinContent() || minSize.isMaxContent() || minSize.isFitContent());

I don't understand why we are chceking the block-size in case of an item orthogonal to the main-axis. Is this function meant to evaluate the item's main-axis size ?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1202
> +    if (min.isSpecified() || (min.isIntrinsic() && mainAxisIsChildInlineAxis(child)))

Again could to elaborate on why we are not considering the case of flex items orthogonal to the main-axis ?

[Sergio Villar Senin]

Comment on attachment 421914 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:432
>> +    bool childBlockSizeIsEquivalentToAutomaticSize  = !mainAxisIsChildInlineAxis(child) && (minSize.isMinContent() || minSize.isMaxContent() || minSize.isFitContent());
> 
> I don't understand why we are chceking the block-size in case of an item orthogonal to the main-axis. Is this function meant to evaluate the item's main-axis size ?

Yes, min-size auto special handling in flexbox only affects the main axis

[Javier Fernandez]

Comment on attachment 421914 [details]
Patch

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

r=me

> Source/WebCore/rendering/RenderFlexibleBox.cpp:433
> +    if (!minSize.isAuto() && !childBlockSizeIsEquivalentToAutomaticSize)

Just a suggestion, not a strong opinion; what about this ways ?

return minSize.IsAuto() || childBlockSizeIsEquivalentToAutomaticSize() ? mainAxisOverflowForChild(child) == Overflow::Visible : false;

I wouldn't mind to use an if instead, but using the an affirmative clause, like this:

if (minSize.IsAuto() || childBlockSizeIsEquivalentToAutomaticSize())
  return mainAxisOverflowForChild(child) == Overflow::Visible;

return false;

Or even better, like this

return (minSize.IsAuto() || childBlockSizeIsEquivalentToAutomaticSize()) && mainAxisOverflowForChild(child) == Overflow::Visible;

[Sergio Villar Senin]

Committed r273955 (234903@main): <https://commits.webkit.org/234903@main>