Bug 214292 - WPT test css/css-flexbox/flex-minimum-height-flex-items-023.html fails
Summary: WPT test css/css-flexbox/flex-minimum-height-flex-items-023.html fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks: 136754 240970
  Show dependency treegraph
 
Reported: 2020-07-13 21:36 PDT by Carlos Alberto Lopez Perez
Modified: 2022-05-26 12:59 PDT (History)
14 users (show)

See Also:


Attachments
Patch (4.95 KB, patch)
2021-03-01 07:25 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (5.09 KB, patch)
2021-03-02 01:30 PST, Sergio Villar Senin
jfernandez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2020-07-13 21:36:48 PDT
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
Comment 1 Radar WebKit Bug Importer 2020-07-20 21:37:14 PDT
<rdar://problem/65864835>
Comment 2 Philip Jägenstedt 2020-10-23 03:00:48 PDT
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.
Comment 3 Sergio Villar Senin 2020-10-23 04:37:08 PDT
(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.
Comment 4 Philip Jägenstedt 2020-10-26 01:56:39 PDT
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.
Comment 5 Sergio Villar Senin 2021-03-01 07:25:40 PST
Created attachment 421822 [details]
Patch
Comment 6 Sergio Villar Senin 2021-03-02 01:30:47 PST
Created attachment 421914 [details]
Patch
Comment 7 Javier Fernandez 2021-03-04 06:27:27 PST
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 ?
Comment 8 Sergio Villar Senin 2021-03-04 08:02:35 PST
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
Comment 9 Javier Fernandez 2021-03-04 14:22:05 PST
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;
Comment 10 Sergio Villar Senin 2021-03-05 02:19:12 PST
Committed r273955 (234903@main): <https://commits.webkit.org/234903@main>