Bug 81809 – percentage height/width values in quirks mode are incorrectly resolved in flexbox children

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tony Chang]

When computing min/max-height/width and height/width of flex children, we resolve percentage values by only looking at the flexbox size.  In quirks mode, if the flexbox size is auto, we should check parents for a definite value.

[SravanKumar S(:sravan)]

Hi Tony, can you please elaborate?.
Are you speaking about RenderFlexibleBox?

[Tony Chang]

(In reply to comment #1)
> Hi Tony, can you please elaborate?.
> Are you speaking about RenderFlexibleBox?

Yes, this is in RenderFlexibleBox.cpp.

[Tony Chang]

Demo in the URL.

This may actually just work (the above test case with min-height seems to).  We should add tests for this.

[Ojan Vafai]

Added a few more cases to test: http://plexode.com/u/#R!*!style%3DAzframe2q%3Eb_%3C%2FdivqfZ0pxHSK-8blueOrYXR!columnTK-8grerXP)NHVL-directionrWNTmargin-bottomRrVSdisplayRLbox%3B6T%3B!ES!!(%272R%3AQPJow93)!!)bO9)G8red92)G*bN%0A(%27*50pxfL!-webkit-flexrKIlid!orangeHGminrJ0pxTmax-8yellrIborderR!3px!soQH%27q%0AMG!!SU9%27qtestr84round-colorR!r4*100%25TbackgU*heightR!n)_%0An(%3Cdiv!*2!%20~http://plexode.com/eval3/#ht=%3Ciz!*%22heightR350px%22!src%3D%22dataRtext%2Fhtml%2CW!11Z10YenO10P)%22q%3C%2Fizq%0AW!6Z6YenO6P_&jt=zs%5B0%5D.document.compatMode

I agree that this is working correctly, but looking at the code I don't understand why. :( Makes me a little worried that there may be cases where it doesn't work.

[Ojan Vafai]

Ugh. All of these are invalid due to renaming flexbox to flex. Uploading a valid test case. Still, we seem to get the height right.

[Ojan Vafai]

Created attachment 156758 [details]
testcase

[Tony Chang]

(In reply to comment #5)
> Ugh. All of these are invalid due to renaming flexbox to flex. Uploading a valid test case. Still, we seem to get the height right.

Yeah, I think we just need to add some layout tests.

[Ojan Vafai]

(In reply to comment #7)
> (In reply to comment #5)
> > Ugh. All of these are invalid due to renaming flexbox to flex. Uploading a valid test case. Still, we seem to get the height right.
> 
> Yeah, I think we just need to add some layout tests.

I've found at least one bug as I've been trying to understand this. I'll post an update once I feel that I've found all the relevant bugs.

[Ojan Vafai]

I've found a few bugs and will post a patch soon. Having some weird issues with my fix. :(

[Ojan Vafai]

Created attachment 157004 [details]
Patch

[Elliott Sprehn]

Comment on attachment 157004 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2064
> +LayoutUnit RenderBox::computeLogicalClientHeight(SizeType heightType, const Length& height)

Tony originally said this didn't make sense here because it's not useful for things outside new Flexbox. What's different now?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:817
> +            return minSize;

The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug?

[Ojan Vafai]

Comment on attachment 157004 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:2064
>> +LayoutUnit RenderBox::computeLogicalClientHeight(SizeType heightType, const Length& height)
> 
> Tony originally said this didn't make sense here because it's not useful for things outside new Flexbox. What's different now?

I'm thinking ahead to Grid, which will need this same logic for restricting to min/max sizes. The alternative for this patch would have been to pass in the RenderBox to use, which is fine (we'd pass in "this" everwhere except in the three cases where we'd pass in the flex item). But given the Grid need, I went with the simpler patch.

I'm a little wary of all these methods with very similar sounding names, but I couldn't think of a great solution.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817
>> +            return minSize;
> 
> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug?

computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto).

[Tony Chang]

Comment on attachment 157004 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:407
> +    if (isColumnFlow())
> +        return child->computeLogicalClientHeight(sizeType, size);
> +    return child->computeContentBoxLogicalWidth(valueForLength(size, availableSize, view()));

This looks wrong for orthogonal flows, but I guess it's what we had before.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817
>>> +            return minSize;
>> 
>> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug?
> 
> computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto).

I wonder if we can remove the isSpecified() check.  I don't have a strong preference one way or another.

> Source/WebCore/rendering/RenderFlexibleBox.h:88
> +    LayoutUnit computeMainAxisSizeForChild(RenderBox* child, SizeType, const Length& size, LayoutUnit availableSize);

Nit: I think we normally use Extent instead of Size for lengths.  I don't feel strongly about this.

> LayoutTests/css3/flexbox/percentage-sizes-quirks.html:37
> +<script>
> +if (window.testRunner)
> +    testRunner.dumpAsText();
> +</script>

Nit: I would probably print document.compatMode somewhere just to be sure.

[Ojan Vafai]

Comment on attachment 157004 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:407
>> +    return child->computeContentBoxLogicalWidth(valueForLength(size, availableSize, view()));
> 
> This looks wrong for orthogonal flows, but I guess it's what we had before.

Good catch. Adding a FIXME for now.

>>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817
>>>> +            return minSize;
>>> 
>>> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug?
>> 
>> computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto).
> 
> I wonder if we can remove the isSpecified() check.  I don't have a strong preference one way or another.

Seems fine to me. I suppose the minSize != -1 is redundant with the childSize<minSize check since childSize is at least 0 (but the -1 check is definitely needed for maxSize).

This just means we'll need to check of the auto case first since we want that to go down a special code path for flexitems.

>> Source/WebCore/rendering/RenderFlexibleBox.h:88
>> +    LayoutUnit computeMainAxisSizeForChild(RenderBox* child, SizeType, const Length& size, LayoutUnit availableSize);
> 
> Nit: I think we normally use Extent instead of Size for lengths.  I don't feel strongly about this.

Yeah, I thought about it after the fact and was too lazy to change it. :) But you're right, I should be consistent.

>> LayoutTests/css3/flexbox/percentage-sizes-quirks.html:37
>> +</script>
> 
> Nit: I would probably print document.compatMode somewhere just to be sure.

makes sense

[Ojan Vafai]

Comment on attachment 157004 [details]
Patch

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

>>>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817
>>>>> +            return minSize;
>>>> 
>>>> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug?
>>> 
>>> computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto).
>> 
>> I wonder if we can remove the isSpecified() check.  I don't have a strong preference one way or another.
> 
> Seems fine to me. I suppose the minSize != -1 is redundant with the childSize<minSize check since childSize is at least 0 (but the -1 check is definitely needed for maxSize).
> 
> This just means we'll need to check of the auto case first since we want that to go down a special code path for flexitems.

Actually, on closer look, the valueForLength functions all ASSERT_NOT_REACHED for intrinsic values other than FillAvailable and Auto. So, we should probably keep the checks in place.

[Ojan Vafai]

Committed r125055: <http://trac.webkit.org/changeset/125055>