Flexbox flex: 1 incorrectly sets flex-basis to 0px

Flexbox flex: 1 incorrectly sets flex-basis to 0px

Flexbox incorrectly sets the flex-basis to 0px when specified
flex: 1. According to the spec http://www.w3.org/TR/css3-flexbox/#propdef-flex
whenever the flex basis is omitted from the flex shorthand its specified
value is 0%.

Also there are few issues which are seen for column flexbox here:

1. A flex-basis of 0px is incorrectly considered to behave like
   flex-basis of auto (taking intrinsic height). Correct behaviour
   is to set height to 0.

2. A flex-basis of 0% is incorrectly compute to 0 height when the
   containing block has no intrinsic size. Correct behaviour is
   to set height equal to min-content of flex-item.


This patch fixes this bug by first setting flex-basis to 0%
and not 0px when omitted in flex shorthand while parsing flex
value.

Also the reason why flex-basis of 0px is taking min-content
as height and 0% flex-basis is computing height to 0 is because
while computing the preferredMainAxisContentExtentForChild the
condition which checks if preferredMainAxisExtentDependsOnLayout
return true for 0px case (flex-basis fixed and value 0) and
false for 0% case (flex-basis percent and 0 value). Ideally
it should do vice versa returning true (ie main axis extent
depends on layout) for percent case and false (not depends
on layout) for pixel case. This patch fixes this check so
that it behaves correctly.

BUG=404337
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181012

[harpreet.sk, 2014-08-21 13:47:23]

ptal....

[cbiesinger, 2014-08-21 23:54:23]

Thanks for working on this!

https://codereview.chromium.org/478233004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (left):

https://codereview.chromium.org/478233004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:605: return flexBasis.isAuto() ||
(flexBasis.isFixed() && !flexBasis.value() && hasInfiniteLineLength);
So, the reason this code exists is a bit complicated.
https://chromium.googlesource.com/chromium/blink/+/4e1f04846b7b0c605dbe7688c2...
has the description -- 0px plus hasInfiniteLength means we need to layout,
because we made that basically behave like 0%.
So with this bug fixed, we should remove all the hasInfiniteLineLength code
(that can happen in a separate patch), but either way, you should remove
hasInfinteLineLength here.

Now, the other thing here - all percentages depend on layout (not just 0),
unless we have a definite specified size, right? So this should be
flexBasis.isAuto() || flexBasis.isPercent() at a minimum. There's probably a way
to optimize that further.

[harpreet.sk, 2014-08-22 15:13:35]

Made changes. PTAL.

https://codereview.chromium.org/478233004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (left):

https://codereview.chromium.org/478233004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:605: return flexBasis.isAuto() ||
(flexBasis.isFixed() && !flexBasis.value() && hasInfiniteLineLength);
On 2014/08/21 23:54:23, cbiesinger wrote:
> So, the reason this code exists is a bit complicated.
>
https://chromium.googlesource.com/chromium/blink/+/4e1f04846b7b0c605dbe7688c2...
> has the description -- 0px plus hasInfiniteLength means we need to layout,
> because we made that basically behave like 0%.
> So with this bug fixed, we should remove all the hasInfiniteLineLength code
> (that can happen in a separate patch), but either way, you should remove
> hasInfinteLineLength here.


Thanks Chris for the review. We cannot simply remove hasFiniteLineLength here. I
tried to remove it but because of it many layout tests are getting failed. We
need to do a lot of refactoring as well as need to add many things like will
have take flex-basis into account while computing computeIntrinsicLogicalWidths
(A FIXME was added in the patch which you have mentioned above by ojan). I will
look into this when i'll upload a sepearate patch for removing all the
hasInfiniteLineLength code. As for this patch we can keep hasInfiniteLineLength
for time being.


> 
> Now, the other thing here - all percentages depend on layout (not just 0),
> unless we have a definite specified size, right? So this should be
> flexBasis.isAuto() || flexBasis.isPercent() at a minimum. There's probably a
way
> to optimize that further.


Yes you are correct all percentages depends on layout and not just 0. Removed
the !flexBasis.value() check.

[cbiesinger, 2014-08-27 00:48:59]

On 2014/08/22 15:13:35, harpreet.sk wrote:
> Made changes. PTAL.
> 
>
https://codereview.chromium.org/478233004/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderFlexibleBox.cpp (left):
> 
>
https://codereview.chromium.org/478233004/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderFlexibleBox.cpp:605: return flexBasis.isAuto() ||
> (flexBasis.isFixed() && !flexBasis.value() && hasInfiniteLineLength);
> On 2014/08/21 23:54:23, cbiesinger wrote:
> > So, the reason this code exists is a bit complicated.
> >
>
https://chromium.googlesource.com/chromium/blink/+/4e1f04846b7b0c605dbe7688c2...
> > has the description -- 0px plus hasInfiniteLength means we need to layout,
> > because we made that basically behave like 0%.
> > So with this bug fixed, we should remove all the hasInfiniteLineLength code
> > (that can happen in a separate patch), but either way, you should remove
> > hasInfinteLineLength here.
> 
> 
> Thanks Chris for the review. We cannot simply remove hasFiniteLineLength here.
I
> tried to remove it but because of it many layout tests are getting failed. We
> need to do a lot of refactoring as well as need to add many things like will
> have take flex-basis into account while computing
computeIntrinsicLogicalWidths
> (A FIXME was added in the patch which you have mentioned above by ojan). I
will
> look into this when i'll upload a sepearate patch for removing all the
> hasInfiniteLineLength code. As for this patch we can keep
hasInfiniteLineLength
> for time being.

I'm sorry, I don't understand why hasInfiniteLineLength can't be removed here.
Which tests are failing? It is not immediately clear to me if the FIXME needs to
be resolved in the same patch.

> Yes you are correct all percentages depends on layout and not just 0. Removed
> the !flexBasis.value() check.

After looking some more at the code, I'm actually not sure anymore that this is
correct. I think we need to be careful to return true here *only if* the
percentage resolves to auto (ie. the flexbox has a definite size). If you change
the code to do that, will the tests still fail?

[harpreet.sk, 2014-08-27 03:35:42]

On 2014/08/27 00:48:59, cbiesinger wrote:
> 
> I'm sorry, I don't understand why hasInfiniteLineLength can't be removed here.
> Which tests are failing? It is not immediately clear to me if the FIXME needs
to
> be resolved in the same patch.
> 
> 
> After looking some more at the code, I'm actually not sure anymore that this
is
> correct. I think we need to be careful to return true here *only if* the
> percentage resolves to auto (ie. the flexbox has a definite size). If you
change
> the code to do that, will the tests still fail?


Solution is correct because here we are returning true only if percentages
should resolves to auto ie when flexbox has 'indefinite size'. for eg as in  
https://codereview.chromium.org/60633008

That's the reason here we cannot remove hasInfiniteLineLength also because this
bool tells us if flexbox has indefinite size or not and hence finally tells that
percentages should resolves to auto or not.

[cbiesinger, 2014-08-28 02:28:06]

On 2014/08/27 03:35:42, harpreet.sk wrote:
> On 2014/08/27 00:48:59, cbiesinger wrote:
> > 
> > I'm sorry, I don't understand why hasInfiniteLineLength can't be removed
here.
> > Which tests are failing? It is not immediately clear to me if the FIXME
needs
> to
> > be resolved in the same patch.
> > 
> > 
> > After looking some more at the code, I'm actually not sure anymore that this
> is
> > correct. I think we need to be careful to return true here *only if* the
> > percentage resolves to auto (ie. the flexbox has a definite size). If you
> change
> > the code to do that, will the tests still fail?
> 
> 
> Solution is correct because here we are returning true only if percentages
> should resolves to auto ie when flexbox has 'indefinite size'. for eg as in  
> https://codereview.chromium.org/60633008
> 
> That's the reason here we cannot remove hasInfiniteLineLength also because
this
> bool tells us if flexbox has indefinite size or not and hence finally tells
that
> percentages should resolves to auto or not.

Oh! Right! Thanks for explaining. You're right that this function needs to keep
a check like that, and this is definitely good enough for now.

I think longer-term though this check is not 100% correct -- if a (vertical)
flexbox has no explicit height but does have a max-height: 10px, then
hasInfiniteLineLength will be false but a percentage height of a child should be
treated as auto, if I'm reading the code/spec correctly.

[cbiesinger, 2014-08-28 02:28:15]

lgtm

[harpreet.sk, 2014-08-28 03:33:05]

Owner's please give lgtm

[ojan, 2014-08-28 03:36:18]

lgtm

[harpreet.sk, 2014-08-28 04:26:32]

The CQ bit was checked by harpreet.sk@samsung.com

[commit-bot: I haz the power, 2014-08-28 04:26:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/478233004/80001

[commit-bot: I haz the power, 2014-08-28 05:02:55]

Committed patchset #5 (id:80001) as 181012