Bug 136041

Summary: Safari (WebKit) doesn't wrap element within flex when width comes below min-width
Product: WebKit Reporter: Kenan Sulayman <kenan>
Component: CSSAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: akmjenkins, commit-queue, danpoynor, dbates, denvned, dliebner, ericdphy, esprehn+autocc, glenn, harou2, hyatt, joan, joepeck, johnw377, kenan, kondapallykalyan, lukejacksonn, mael.lavault, m.goleb+bugzilla, mike.hayes10, mikol, m.kurz+webkitbugs, philip, simon.fraser, timothy, wawyed, webkit-bug-importer, webkit, zalan
Priority: P3 Keywords: HasReduction, InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Screenshot: Safari, Chrome, Opera, Firefox.
none
Example
none
Patch
none
Patch
none
Patch
none
Patch none

Description Kenan Sulayman 2014-08-18 09:11:52 PDT
Created attachment 236765 [details]
Screenshot: Safari, Chrome, Opera, Firefox.

Safari doesn't wrap an element when flex is applied (1 0 25%) and the width comes below the set min-width. This is different in Firefox, Chrome (Blink) and Opera (Blink). That is, I assume it was already fixed in that fork and not incorrectly implemented in Gecko at the first place.

Changing

flex: 1 0 25%;

to

flex: 1 0 40%;

Seems to be a bit of a workaround, doesn't fix it, though.

In the attachment (Screenshot), from left to right: Safari, Chrome, Opera, Firefox.

Since I am unable to attach two attachments, see this link for the example used in the screenshot: http://data.sly.mn/text/163h0C2x2f3O/raw.
Comment 1 Kenan Sulayman 2014-08-19 01:28:31 PDT
Created attachment 236805 [details]
Example
Comment 2 Mikol Graves 2015-03-04 09:43:57 PST
See also:

* http://codepen.io/anon/pen/qEKwXX (Does not work as expected.)
* http://codepen.io/anon/pen/yyqLGr (Works around actual behavior.)
* https://github.com/angular/material/issues/1720 (Resulting Angular Material issue.)

Steps to Reproduce:
1. Create a container with CSS declarations `-webkit-flex-direction: row; -webkit-flex-wrap: wrap;` and three child elements.
2. Each child should include the CSS declarations `-webkit-flex: 1; min-width: 320px;`.
3. View the page with Safari on OS X (10.9.5) or iOS (I used 8.x on both a real and simulated iPhone 6).


Expected Results:
Each child element should shrink to its minimum width as the viewport gets narrower and wrap as the viewport becomes narrower than their combined minimum width.

Setting `flex: 1 1 320px` achieves the expected results.

Actual Results:
The child elements shrink to their mnimum width when the viewport width is narrow, but they do not wrap.

User Agent: 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/600.3.18 (KHTML, like Gecko) Version/7.1.3 Safari/537.85.12
Comment 3 Philip Walton 2015-07-25 14:01:18 PDT
I've added this bug to the Flexbugs list I maintain:
https://github.com/philipwalton/flexbugs#11-min-and-max-size-declarations-are-ignored-when-wrapping-flex-items

I'll copy the bug and description here to make it easier to reference:

## Min and max size declarations are ignored in Safari when wrapping flex items

Bug: http://codepen.io/anon/pen/BNrGwN
Workaround: http://codepen.io/anon/pen/RPMqjz

Safari uses min/max width/height declarations for actually rendering the size of flex items, but it ignores those values when calculating how many items should be on a single line of a multi-line flex container. Instead, it simply uses the item's `flex-basis` value, or its width if the flex basis is set to `auto`.

This is problematic when using the `flex: 1` shorthand because that sets the flex basis to `0%`, and an infinite number of flex items could fit on a single line if the browser thinks their widths are all zero. The bug above (http://codepen.io/philipwalton/pen/BNrGwN) show an example of this happening.

This is also problematic when creating fluid layouts where you want your flex items to be no bigger than X but no smaller than Y. Since Safari ignores those values when determining how many items fit on a line, that strategy won't work.
Comment 4 Radar WebKit Bug Importer 2016-04-05 20:04:42 PDT
<rdar://problem/25569370>
Comment 5 Jon Lee 2016-06-02 17:15:11 PDT
*** Bug 145402 has been marked as a duplicate of this bug. ***
Comment 6 wawyed 2016-09-05 07:56:50 PDT
This is flex basic behaviour, works as expected in all browsers except Safari. It's unbeliable that since 2014, no one in the Safari dev team has even bothered to look at this.

Can we get some sorf of update on this please?

Thanks.
Comment 7 Martin Kadlec 2016-09-15 01:27:16 PDT
This is really giving us trouble right now and basically preventing us from using flexbox effectively on production. Any chance for a fix?
Comment 8 Maƫl Lavault 2016-09-20 06:34:28 PDT
It is a shame that this is not fixed, it's taking us so much time working around this. flex-wrap is something we should expect to work in every recent browsers.
Comment 9 johnw377 2016-10-27 17:14:58 PDT
+1 for this still being a problem; I can't get any workaround to work.
Comment 10 Daniel 2016-11-16 01:01:34 PST
This is a problem for me too.

"Safari uses min/max width/height declarations for actually rendering the size of flex items, but it ignores those values when calculating how many items should be on a single line of a multi-line flex container. Instead, it simply uses the item's `flex-basis` value, or its width if the flex basis is set to `auto`."

The resulting behavior in my case is that the flex items overflow their container. Safari *should* use the min- and max-width values when calculating how many items should be on a single line of a multi-line flex container.
Comment 11 Phy 2016-11-19 10:47:02 PST
This is causing problems for UI devs all over; please prioritize it.  It's a shame to have Safari be the exception for web design architecture.
Comment 12 Luke Jackson 2016-11-23 01:48:35 PST
+1 on this issue, have had to use a workaround which is far from ideal
Comment 13 zalan 2016-11-28 15:41:10 PST
Created attachment 295540 [details]
Patch
Comment 14 zalan 2016-11-28 16:54:11 PST
Created attachment 295551 [details]
Patch
Comment 15 Daniel Bates 2016-11-28 19:56:28 PST
Comment on attachment 295551 [details]
Patch

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

I did not review this patch for correctness. I noticed a minor style nit.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:996
> +        LayoutUnit childMainAxisExtentWithMinWidthConstrain = childMainAxisExtent;

Should this local be called childMainAxisExtentWithMinWidthConstraint? (Notice the 't' at the end of the name).
Comment 16 zalan 2016-11-28 19:57:58 PST
(In reply to comment #15)
> Comment on attachment 295551 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295551&action=review
> 
> I did not review this patch for correctness. I noticed a minor style nit.
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:996
> > +        LayoutUnit childMainAxisExtentWithMinWidthConstrain = childMainAxisExtent;
> 
> Should this local be called childMainAxisExtentWithMinWidthConstraint?
> (Notice the 't' at the end of the name).
:( It should. Thanks!
Comment 17 zalan 2016-11-28 20:02:33 PST
Created attachment 295573 [details]
Patch
Comment 18 Darin Adler 2016-11-28 20:02:45 PST
Comment on attachment 295551 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:999
> +            auto minWidthForChild = computeMainAxisExtentForChild(*child, MinSize, child->style().logicalMinWidth());
> +            if (minWidthForChild)

Could define this variable inside the if:

    if (auto minWidthForChild = ...)

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1000
> +                childMainAxisExtentWithMinWidthConstrain = std::max(childMainAxisExtent, minWidthForChild.value());

Seems like there ought to be a function like std::max that ignores optional values; then we would not need an if statement.
Comment 19 Darin Adler 2016-11-28 20:03:48 PST
Comment on attachment 295573 [details]
Patch

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

See my comments from the previous version of this patch.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1002
> +        preferredMainAxisExtentWithMinWidthConstraint += (childMainAxisExtentWithMinWidthConstraint + borderMarginAndPaddingSpace);

Not sure the parentheses here are helpful.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1009
> +        preferredMainAxisExtent += (childMainAxisExtent + borderMarginAndPaddingSpace);

Not sure the parentheses here are helpful.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1014
> +        minMaxAppliedMainAxisExtent += (childMinMaxAppliedMainAxisExtent + borderMarginAndPaddingSpace);

Not sure the parentheses here are helpful.
Comment 20 zalan 2016-11-28 20:07:27 PST
(In reply to comment #18)
> Comment on attachment 295551 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295551&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:999
> > +            auto minWidthForChild = computeMainAxisExtentForChild(*child, MinSize, child->style().logicalMinWidth());
> > +            if (minWidthForChild)
> 
> Could define this variable inside the if:
> 
>     if (auto minWidthForChild = ...)
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:1000
> > +                childMainAxisExtentWithMinWidthConstrain = std::max(childMainAxisExtent, minWidthForChild.value());
> 
> Seems like there ought to be a function like std::max that ignores optional
> values; then we would not need an if statement.
I've seen people doing this instead -> std::max(a, b.value_or(a)) but I find it even worse.
Comment 21 zalan 2016-11-28 20:10:45 PST
Created attachment 295574 [details]
Patch
Comment 22 WebKit Commit Bot 2016-11-29 08:05:00 PST
Comment on attachment 295574 [details]
Patch

Clearing flags on attachment: 295574

Committed r209068: <http://trac.webkit.org/changeset/209068>
Comment 23 WebKit Commit Bot 2016-11-29 08:05:07 PST
All reviewed patches have been landed.  Closing bug.