Bug 81809 - percentage height/width values in quirks mode are incorrectly resolved in flexbox children
: percentage height/width values in quirks mode are incorrectly resolved in fle...
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://plexode.com/eval3/#ht=%3Cifram...
:
:
: 62048
  Show dependency treegraph
 
Reported: 2012-03-21 11:35 PST by
Modified: 2012-08-08 11:29 PST (History)


Attachments
testcase (1.42 KB, text/html)
2012-08-06 13:48 PST, Ojan Vafai
no flags Details
Patch (17.26 KB, patch)
2012-08-07 14:23 PST, Ojan Vafai
tony: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-21 11:35:19 PST
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.
------- Comment #1 From 2012-03-22 07:29:36 PST -------
Hi Tony, can you please elaborate?.
Are you speaking about RenderFlexibleBox?
------- Comment #2 From 2012-03-22 09:51:17 PST -------
(In reply to comment #1)
> Hi Tony, can you please elaborate?.
> Are you speaking about RenderFlexibleBox?

Yes, this is in RenderFlexibleBox.cpp.
------- Comment #3 From 2012-03-22 10:48:38 PST -------
Demo in the URL.

This may actually just work (the above test case with min-height seems to).  We should add tests for this.
------- Comment #5 From 2012-08-06 13:48:08 PST -------
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.
------- Comment #6 From 2012-08-06 13:48:20 PST -------
Created an attachment (id=156758) [details]
testcase
------- Comment #7 From 2012-08-06 14:07:05 PST -------
(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.
------- Comment #8 From 2012-08-06 14:15:21 PST -------
(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.
------- Comment #9 From 2012-08-06 17:07:51 PST -------
I've found a few bugs and will post a patch soon. Having some weird issues with my fix. :(
------- Comment #10 From 2012-08-07 14:23:49 PST -------
Created an attachment (id=157004) [details]
Patch
------- Comment #11 From 2012-08-07 14:41:08 PST -------
(From update of attachment 157004 [details])
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?
------- Comment #12 From 2012-08-07 15:02:29 PST -------
(From update of attachment 157004 [details])
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).
------- Comment #13 From 2012-08-07 18:27:18 PST -------
(From update of attachment 157004 [details])
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.
------- Comment #14 From 2012-08-08 10:58:41 PST -------
(From update of attachment 157004 [details])
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
------- Comment #15 From 2012-08-08 11:17:51 PST -------
(From update of attachment 157004 [details])
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.
------- Comment #16 From 2012-08-08 11:29:11 PST -------
Committed r125055: <http://trac.webkit.org/changeset/125055>