Flexbox: Allow intrinsic aspect ratios to inform main-size calculation

Flexbox: Allow intrinsic aspect ratios to inform main-size calculation

According to the http://dev.w3.org/csswg/css-flexbox/#algo-main-item
point B states that:
If the flex item has
  a) an intrinsic aspect ratio,
  b) a flex basis of 'auto', and
  c) a definite cross size
then the flex base size is calculated from its inner cross size
and the flex item’s intrinsic aspect ratio.

The current implementation does not handle properly this case. This patch
fixes this bug by allowing support for aspect ratio while calculation
of main-size calculation in RenderFlexibleBox::preferredMainAxisContentExtentForChild.

Bug=249112

[harpreet.sk, 2014-06-20 05:55:45]

PTAL. According to the description given in crbug.com/249112
what i understood is to implement point B of calculation of main-size of
flex-item. Please let me know what else has to be added in the existing patch.

[tony, 2014-06-23 16:50:58]

https://codereview.chromium.org/343103003/diff/1/LayoutTests/css3/flexbox/asp...
File LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html (right):

https://codereview.chromium.org/343103003/diff/1/LayoutTests/css3/flexbox/asp...
LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html:11:
-webkit-aspect-ratio: 1 / 2;
We should have a test case where the flex item is an image.  I think that's the
primary use case (although the aspect-ratio property should also work).

https://codereview.chromium.org/343103003/diff/1/LayoutTests/css3/flexbox/asp...
LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html:18: <div
class="flexitem" style="height:100px" data-expected-width="50"
data-expected-height="100"></div>
We should also have a test case where the height is not set on the flex item to
see if the stretched size applies.

Here's an example:
http://jsfiddle.net/6kJTE/

In the example, the aspect ratio of the image is 2:1 and the image has a
flex-basis of 'auto'.  There's a definite cross size of 100px for the image[1].
This means that the flex basis size should be 200px during the flex algorithm. 
In the example, we use 100px incorrectly.

Note that Firefox and IE11 both render the same as us.  Maybe we should bring
this back to the working group and ask if we should instead change the spec.


[1] From the spec: "As a special case for handling stretched flex items, if a
single-line flex container has a definite cross size, the outer cross size of
any stretched flex items is the flex container’s inner cross size (clamped to
the flex item’s min and max cross size) and is considered definite."

[harpreet.sk, 2014-06-26 14:41:49]

I have tried to incorporate the comments mention in previous patch and also
added all the test cases mention earlier in LayoutTest. Please have a look.

https://codereview.chromium.org/343103003/diff/1/LayoutTests/css3/flexbox/asp...
File LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html (right):

https://codereview.chromium.org/343103003/diff/1/LayoutTests/css3/flexbox/asp...
LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html:11:
-webkit-aspect-ratio: 1 / 2;
On 2014/06/23 16:50:57, tony wrote:
> We should have a test case where the flex item is an image.  I think that's
the
> primary use case (although the aspect-ratio property should also work).


I have added the test case where the flex item is an image and also test case
where we are applying the aspect-ratio to the image using css.

https://codereview.chromium.org/343103003/diff/1/LayoutTests/css3/flexbox/asp...
LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html:18: <div
class="flexitem" style="height:100px" data-expected-width="50"
data-expected-height="100"></div>
On 2014/06/23 16:50:57, tony wrote:
> We should also have a test case where the height is not set on the flex item
to
> see if the stretched size applies.
> 
> Here's an example:
> http://jsfiddle.net/6kJTE/
> 
> In the example, the aspect ratio of the image is 2:1 and the image has a
> flex-basis of 'auto'.  There's a definite cross size of 100px for the
image[1].
> This means that the flex basis size should be 200px during the flex algorithm.

> In the example, we use 100px incorrectly.


I have also added this test case in the new patch. New patch handle this test
case properly. It will show flex basis as 200px and not 100px.



> 
> Note that Firefox and IE11 both render the same as us.  Maybe we should bring
> this back to the working group and ask if we should instead change the spec.


I think firefox and IE both render same as us because currently they are also
not using aspect ratio while calculating main-size. Since it is already mention
in the spec that

[1] From the spec: "As a special case for handling stretched flex items, if a
single-line flex container has a definite cross size, the outer cross size of
any stretched flex items is the flex container’s inner cross size (clamped to
the flex item’s min and max cross size) and  <<<< is considered definite. >>>" 

and also it has been mention in the spec

If the flex item has 
a) an intrinsic aspect ratio, 
b) a flex basis of 'auto', and 
c) "a definite cross size" 
then the flex base size is calculated from its inner cross size 
and the flex item’s intrinsic aspect ratio


From the above 2 things we can conclude that for the single line flex containers
we will have definite cross size if container has definite cross size. So while
calculating their main size we have to take care of aspect ratio property.

Firefox and IE currently also not following this point. May be after we
implement they might also add the support for aspect ratio.

[tony, 2014-06-26 16:25:32]

Tab, what are you thoughts on the aspect ratio parts of the flexbox spec. 
AFAICT, all current browsers implement the same behavior, but it's not what the
spec says.  Should we change the spec to match implementations?

[tony, 2014-06-26 16:31:29]

https://codereview.chromium.org/343103003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/343103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:628: void
RenderFlexibleBox::computeAspectRatioOfChild(RenderBox* child, double&
aspectRatio, bool& hasAspectRatio)
The bool seems unnecessary.  Set the initial value of apsectRatio to -1 to mean
no aspect ratio.

https://codereview.chromium.org/343103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:633: } else if
(child->isRenderImage() && child->intrinsicLogicalHeight()) {
Is checking for a RenderImage really the right thing? For example, <video>s also
have aspect ratios.

[harpreet.sk, 2014-06-30 09:26:07]

Uploaded new patch addressing comments of previous patch. Please have a look.

https://codereview.chromium.org/343103003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/343103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:628: void
RenderFlexibleBox::computeAspectRatioOfChild(RenderBox* child, double&
aspectRatio, bool& hasAspectRatio)
On 2014/06/26 16:31:28, tony wrote:
> The bool seems unnecessary.  Set the initial value of apsectRatio to -1 to
mean
> no aspect ratio.

Done.

https://codereview.chromium.org/343103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:633: } else if
(child->isRenderImage() && child->intrinsicLogicalHeight()) {
On 2014/06/26 16:31:28, tony wrote:
> Is checking for a RenderImage really the right thing? For example, <video>s
also
> have aspect ratios.

Sorry only checking for RenderImage is not the right thing. Aspect ratio can be
for image, video or canvas. Added new check for the same.

[harpreet.sk, 2014-07-01 15:02:07]

PING

[harpreet.sk, 2014-07-07 13:45:35]

PING

[tony, 2014-07-07 16:52:11]

Sorry, I was on vacation last week.

Please contact the www-style mailing list about this issue.  I'm not sure if we
want to change the browser behavior (which all seems to match) or change the
spec.  It would be helpful if we knew how many websites this would break (if
any).  You might be able to add a use counter to determine how common it is to
use an image as a flex item.

[harpreet.sk, 2014-08-01 13:43:55]

On 2014/07/07 16:52:11, tony wrote:
> Sorry, I was on vacation last week.
> 
> Please contact the www-style mailing list about this issue.  I'm not sure if
we
> want to change the browser behavior (which all seems to match) or change the
> spec.  It would be helpful if we knew how many websites this would break (if
> any).  You might be able to add a use counter to determine how common it is to
> use an image as a flex item.


@tony: I contacted the www-style regarding this issue and got the following
reply from Greg Whitworth:

> Hi,
> According to the spec http://dev.w3.org/csswg/css-flexbox/#algo-main-item  we
should take intrinsic aspect ratios  into account while doing main-size
calculation. 
> I was working on the implementation of the same. Currently none of the browser
is taking  intrinsic aspect ratios into account while doing main-size
calculation.
> Should we change the specification or  implement the same (in that case will
break the consistency with the other browser) ?
 
Reply:
" I would suggest no, that was added fairly recently[1] and we (IE) are working
on this currently."

Greg


Since IE has started working on this so shall we continue with this patch ?

[harpreet.sk, 2014-08-05 08:57:25]

On 2014/07/07 16:52:11, tony wrote:
> Sorry, I was on vacation last week.
> 
> Please contact the www-style mailing list about this issue.  I'm not sure if
we
> want to change the browser behavior (which all seems to match) or change the
> spec.  It would be helpful if we knew how many websites this would break (if
> any).  You might be able to add a use counter to determine how common it is to
> use an image as a flex item.


Posted the following question on www-style mailing list:

> Hi,
> According to the spec http://dev.w3.org/csswg/css-flexbox/#algo-main-item  we
should take intrinsic aspect ratios  into account while doing main-size
calculation. 
> I was working on the implementation of the same. Currently none of the browser
is taking  intrinsic aspect ratios into account while doing main-size
calculation.
> Should we change the specification or  implement the same (in that case will
break the consistency with the other browser) ?


Reply from Greg Whitworth:  " I would suggest no, that was added fairly
recently[1] and we (IE) are working on this currently."

Reply from Daniel Holbert:  "Firefox Nightly[1] should be taking the intrinsic
ratio into account
                             when doing main-size computations now. (if there's
a definite cross
                             size, and an indefinite main-size, and a flex-basis
of "auto"/(aka
                             "main-size"), and an intrinsic aspect ratio.)

                             I added that while implementing the new
min-[width|height]:auto
                             behavior, as noted here:
                            
https://bugzilla.mozilla.org/show_bug.cgi?id=1015474#c2

                             If you notice it not behaving as you'd expect,
please file bugs or let
                             me know. "


Since IE has started implementing it and firefox has implemented it so shall we
continue with this patch?

[tony, 2014-08-06 16:37:40]

I'll review the code after you upload some more test cases and have passing
tests on the try bots.

https://codereview.chromium.org/343103003/diff/40001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html (right):

https://codereview.chromium.org/343103003/diff/40001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/aspect-ratios-main-size-calculation.html:17: <div
class="flexitem" style="height:100px" data-expected-width="50"
data-expected-height="100"></div>
Shouldn't some of the test cases have the flex property set?  Without a flex
value, I'm not sure what this is testing.  Can you copy some of the test cases
from Firefox?

[harpreet.sk, 2014-09-29 08:29:56]

harpreet.sk@samsung.com changed reviewers:
+ tabatkins@google.com
- a.bah@chromium.org, tabatkins@chromium.org

[rwlbuis, 2016-11-16 14:48:18]

@cbiesinger I think this can be closed since the issue is marked as WontFix?

[cbiesinger, 2016-11-19 01:51:49]

On 2016/11/16 14:48:18, rwlbuis wrote:
> @cbiesinger I think this can be closed since the issue is marked as WontFix?

Yep. I verified that the testcase passes, except for two cases where the spec
has changed. Closing.