Bug 26559 – When a block's height is determined by min-height/max-height, children with percentage heights are sized incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

Version is 530.17, Safari 4.0.

If the specified height of an element is less than min-height, the actual height used is the min-height; similarly, if the specified height of an element is more than max-height, the actual height used is the max-height.

When this occurs, Safari uses the originally-specified height value rather than the actually-used height to calculate the heights of children with percentage-based heights.

Created attachment 31563 [details]
Test case

The test case consists of two sets of boxes, the outer with a red border and the inner with blue. The top set has height=100px and min-height=200px on the red box, and the bottom set has height=400px and max-height=200px on the red box; both have height=100% on the blue box. The red and blue boxes should all be 200px high, but Safari draws the upper blue box at 100px and the lower at 400px.

[And Clover]

Same root cause as bug 14762. The percentage unit applied to `height`, `top` and `bottom`, when used on an element with static or relative positioning, is calculated purely from the the containing block's `height` CSS property, and not its computed height (which would take min/max-height into account).

The same does not occur on widths or on elements with absolute or fixed positioning.

Other browsers use the computed height of the containing block (except in the case where the computed height is dependent on the child height, ie `auto`, as per CSS2.1 section 10.5).

[Silas Brill]

*** Bug 76860 has been marked as a duplicate of this bug. ***

[Bem Jones-Bey]

Created attachment 155594 [details]
Bug fix

Make percent height computation respect min/max of parent

[Max Vujovic]

Comment on attachment 155594 [details]
Bug fix

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

Nice patch. Here's my informal review:

> Source/WebCore/rendering/RenderBox.cpp:2166
> +    LayoutUnit minH = computeContentLogicalHeightUsing(MinSize, style()->logicalMinHeight()); // Leave as -1 if unset.

I would avoid abbreviations (minH, maxH).

> LayoutTests/fast/block/min-max-height-percent-height-child.html:10
> +                min-height: 400px;

We should add some more tests. I would check if it works with:
- min-height and max-height as percentages on the parent element
- paddings on the parent element
- vertical writing modes

It might be easier to cover more permutations with a JavaScript test if you want to do that. You could apply different styles or classes on the elements and then verify the computed height. We do this a lot in <table> tests.

> LayoutTests/fast/block/min-max-height-percent-height-child.html:32
> +        These are tests for <a href="https://bugs.webkit.org/show_bug.cgi?id=26559">Bug 26559</a><br/>

Add a line describing what you're testing here like: "This test checks that children with percentage heights compute their height correctly when their parent element's height is determined by min-height or max-height."

> LayoutTests/fast/block/min-max-height-percent-height-child.html:34
> +        <p>Test 1: one tall green rectangle</p>

Describe the case that you're testing in addition to the expected result. For example, "Test 1: Parent's height is determined by min-height..."

> LayoutTests/fast/block/min-max-height-percent-height-child.html:39
> +        <p>Test 2: one short green rectangle</p>

Ditto.

> LayoutTests/fast/block/min-max-height-percent-height-child.html:44
> +        <p>Test 3: small green rectangle on top of larger blue rectangle</p>

Ditto.

[Ojan Vafai]

Comment on attachment 155594 [details]
Bug fix

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

Looks roughly correct. Thanks for the fix! Please address my comment as well as Max's.

> Source/WebCore/rendering/RenderBox.cpp:2138
>          result = cb->computePercentageLogicalHeight(cbstyle->logicalHeight());
> -        if (result != -1)
> -            result = cb->computeContentBoxLogicalHeight(result);
> +        if (result != -1) {
> +            result = cb->computeContentLogicalHeightRespectingMinMaxHeight(result);
> +            if (result != -1)
> +                result = cb->computeContentBoxLogicalHeight(result);
> +        }

Doesn't computePercentageLogicalHeight already adjust for min/max with your above change?

> Source/WebCore/rendering/RenderBox.cpp:2172
> +    return result;

Can you have this call computeContentBoxLogicalHeight? Then you don't need to do it in the two calling locations?

[Tony Chang]

Comment on attachment 155594 [details]
Bug fix

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

> Source/WebCore/rendering/RenderBox.cpp:2171
> +    LayoutUnit result = min<LayoutUnit>(maxH, logicalHeight);
> +    result = max<LayoutUnit>(minH, result);

What happens if min and max height are fixed, but height is auto?  It looks like we would use the value of min-height, which is incorrect. Please add a test case for this too.

>> LayoutTests/fast/block/min-max-height-percent-height-child.html:10
>> +                min-height: 400px;
> 
> We should add some more tests. I would check if it works with:
> - min-height and max-height as percentages on the parent element
> - paddings on the parent element
> - vertical writing modes
> 
> It might be easier to cover more permutations with a JavaScript test if you want to do that. You could apply different styles or classes on the elements and then verify the computed height. We do this a lot in <table> tests.

I would also like to see a test where height is auto, but min-height is fixed (e.g., 400px).  In that case, the percentage height should be treated as auto (I get the same behavior in Firefox).

[Tony Chang]

Comment on attachment 155594 [details]
Bug fix

I wonder if there are other places where we get this wrong (e.g., we might get this wrong in RenderFlexibleBox::computeLogicalClientHeight).

Another possible way to implement this is in RenderStyle::logicalHeight.  If height/width is fixed, we then check min/max and make sure the value is within the bounds.

Also, it would be good to have some test cases where min-height > max-height.

[Ojan Vafai]

(In reply to comment #8)
> (From update of attachment 155594 [details])
> I wonder if there are other places where we get this wrong (e.g., we might get this wrong in RenderFlexibleBox::computeLogicalClientHeight).

This case is special because converting the Length to a LayoutUnit requires grabbing the containingBlock's height and it's the containingBlock's min/max that is getting ignored. We do get this wrong in RenderFlexibleBox::computeLogicalClientHeight, but this patch fixes that case too since that just calls computeContentLogicalHeightUsing, which, in turn calls computePercentageLogicalHeight.

> Another possible way to implement this is in RenderStyle::logicalHeight.  If height/width is fixed, we then check min/max and make sure the value is within the bounds.

I'm wary of making RenderStyle magical like this. Then it needs to know about things like converting percentages to fixed values, etc.

[Tony Chang]

(In reply to comment #9)
> (In reply to comment #8)
> > Another possible way to implement this is in RenderStyle::logicalHeight.  If height/width is fixed, we then check min/max and make sure the value is within the bounds.
> 
> I'm wary of making RenderStyle magical like this. Then it needs to know about things like converting percentages to fixed values, etc.

Yeah, after sleeping on the idea, I realized that this wouldn't work for percentages or calc values.  It'll have to be done in computePercentageLogicalHeight.

[Bem Jones-Bey]

Comment on attachment 155594 [details]
Bug fix

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

>> Source/WebCore/rendering/RenderBox.cpp:2138
>> +        }
> 
> Doesn't computePercentageLogicalHeight already adjust for min/max with your above change?

It doesn't, which is why I added it here as well. But I will have to do more digging to be able to give you a good explanation as to why it doesn't work.

>> Source/WebCore/rendering/RenderBox.cpp:2166
>> +    LayoutUnit minH = computeContentLogicalHeightUsing(MinSize, style()->logicalMinHeight()); // Leave as -1 if unset.
> 
> I would avoid abbreviations (minH, maxH).

Ok.

>> Source/WebCore/rendering/RenderBox.cpp:2171
>> +    result = max<LayoutUnit>(minH, result);
> 
> What happens if min and max height are fixed, but height is auto?  It looks like we would use the value of min-height, which is incorrect. Please add a test case for this too.

I'll add a test case and fix it if it doesn't work properly.

>> Source/WebCore/rendering/RenderBox.cpp:2172
>> +    return result;
> 
> Can you have this call computeContentBoxLogicalHeight? Then you don't need to do it in the two calling locations?

Yes, I can rename the method to computeContentBoxLogicalHeightRespectingMinMaxHeight and do that. (I only didn't do that initially in an attempt to be more consistent with similar methods that were already there)

>>> LayoutTests/fast/block/min-max-height-percent-height-child.html:10
>>> +                min-height: 400px;
>> 
>> We should add some more tests. I would check if it works with:
>> - min-height and max-height as percentages on the parent element
>> - paddings on the parent element
>> - vertical writing modes
>> 
>> It might be easier to cover more permutations with a JavaScript test if you want to do that. You could apply different styles or classes on the elements and then verify the computed height. We do this a lot in <table> tests.
> 
> I would also like to see a test where height is auto, but min-height is fixed (e.g., 400px).  In that case, the percentage height should be treated as auto (I get the same behavior in Firefox).

I will add more tests for this and switch to JavaScript tests.

[Bem Jones-Bey]

Also, sorry about the delay in responding, I was on vacation for the past two weeks. Also, it will probably take me a while to put together a new patch, I have a few other things I need to attend to. But I will update it and greatly appreciate all of the feedback.

[Bem Jones-Bey]

Created attachment 173722 [details]
Updated fix

Updated fix

[Ojan Vafai]

Comment on attachment 173722 [details]
Updated fix

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

Sorry it took so long to get you a review on this.

Instead of a reftest the test a check-layout.js test? They much easier to read than this test I think. For example, instead of 
<div id="simple-min"></div>

you'd have:
<div id="simple-min" class="parent" style="width: 50px; height; 50px; max-height: 25px">
    <div class="child" data-expected-width=50 data-expected-height=25></div>
</div>

It's much easier to see what's going on here than needing to coordinate between three different files.

> Source/WebCore/rendering/RenderBox.cpp:2216
> +        availableHeight = max<LayoutUnit>(0, cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeight - cb->scrollbarLogicalHeight()));

I think we need to call this on contentBoxHeight and then subtract scrollbarLogicalHeight from the result of constrainContentBoxLogicalHeightByMinMax, but I'm not sure. Can you add some test cases with overflow:scroll on the containingBlock.

> Source/WebCore/rendering/RenderBox.cpp:2226
> +            LayoutUnit contentBoxHeight = cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeightWithScrollbar - cb->scrollbarLogicalHeight());

Ditto.

Also, it's still not clear to me why you need to call this here. It seems like computePercentageLogicalHeight should already constrain it with your change above. I'm worried that this is masking a bug. Which test cases break if you don't modify this line? It's very possible I'm just missing something obvious. :)

[Bem Jones-Bey]

Comment on attachment 173722 [details]
Updated fix

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

Thanks for the review, no worries on the time it took, it took me longer to post a new patch after your first round of feedback. :-)

>> Source/WebCore/rendering/RenderBox.cpp:2216
>> +        availableHeight = max<LayoutUnit>(0, cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeight - cb->scrollbarLogicalHeight()));
> 
> I think we need to call this on contentBoxHeight and then subtract scrollbarLogicalHeight from the result of constrainContentBoxLogicalHeightByMinMax, but I'm not sure. Can you add some test cases with overflow:scroll on the containingBlock.

Ok.

>> Source/WebCore/rendering/RenderBox.cpp:2226
>> +            LayoutUnit contentBoxHeight = cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeightWithScrollbar - cb->scrollbarLogicalHeight());
> 
> Ditto.
> 
> Also, it's still not clear to me why you need to call this here. It seems like computePercentageLogicalHeight should already constrain it with your change above. I'm worried that this is masking a bug. Which test cases break if you don't modify this line? It's very possible I'm just missing something obvious. :)

Consider a situation like the following:
<div id="A" style="height: 100px; max-height: 50px;">
    <div id="B" style="height: 80%; max-height: 25px;">
        <div id="C" style="height: 100%; max-height: 20px;"></div>
    </div>
</div>

When we attempt to compute the height of C, we end up in computePercentageLogicalHeight where our containing box is B. Since B has a percentage height, we need to recurse and call computePercentageLogicalHeight for B. So now we're in the recursive call, with our current box is B and our containing box is A. Since A has a fixed height, we can compute the height of A, with the other change I made to takes into account the max-height of A. This is used with the percentage height of B to determine the return value for our recursive call. Note that it never looks at the max-height of B, so this return is 80% of 50px, or 40px. So now we're back in the original call to computePercentageLogicalHeight with C being the current box and B being the containing box. heightWithScrollbar gets set to 40px (the height of B), and then we need to constrain for the max-height of B, which is 25px. After that, we can do the calculation for the percentage height of C, which is 100% of the height of B, so 25px. This gets returned up the call chain (computePercentageLogicalHeight -> computeContentAndScrollbarLogicalHeightUsing -> computeLogicalHeightUsing -> computeLogicalHeight). In computeLogicalHeight, it adjusts for the min/max of the current box ( C ), which then ends up being the actual computed height of 20px.

Does that make sense? It is still possible that there is another bug here, but I do not believe it would be a new one, since the expectation that these height helper methods do not adjust for the max/min height of the box they are computing the height for is the existing behavior.

[Bem Jones-Bey]

(In reply to comment #14)
> (From update of attachment 173722 [details])
>
> Instead of a reftest the test a check-layout.js test? They much easier to read than this test I think. For example, instead of 
> <div id="simple-min"></div>
> 
> you'd have:
> <div id="simple-min" class="parent" style="width: 50px; height; 50px; max-height: 25px">
>     <div class="child" data-expected-width=50 data-expected-height=25></div>
> </div>
> 
> It's much easier to see what's going on here than needing to coordinate between three different files.

That sounds good to me. I'll revise the tests to do that instead.

[Bem Jones-Bey]

Created attachment 177087 [details]
Updated patch

Incorporate review feedback.

[Bem Jones-Bey]

(In reply to comment #14)
> > Source/WebCore/rendering/RenderBox.cpp:2216
> > +        availableHeight = max<LayoutUnit>(0, cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeight - cb->scrollbarLogicalHeight()));
> 
> I think we need to call this on contentBoxHeight and then subtract scrollbarLogicalHeight from the result of constrainContentBoxLogicalHeightByMinMax, but I'm not sure. Can you add some test cases with overflow:scroll on the containingBlock.

I added the test cases, and played around with it a bit, and this does seem correct to me.

[Bem Jones-Bey]

Created attachment 181058 [details]
Updated patch

Rebase since it's been a long time since the patch was originally pushed

[Tony Chang]

Comment on attachment 181058 [details]
Updated patch

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

> Source/WebCore/rendering/RenderBox.cpp:2403
> +            // handle the min/max of the current block, it's caller does. So the

Nit: it's -> its

[Bem Jones-Bey]

Created attachment 181079 [details]
Updated patch

Update for nit

[WebKit Review Bot]

Comment on attachment 181079 [details]
Updated patch

Clearing flags on attachment: 181079

Committed r138668: <http://trac.webkit.org/changeset/138668>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Josh]

Has this fix been released yet? I've been trying to base the height of columns off the min-height of the parent, but it's not working yet in Chrome or Safari.

[Robert Hogan]

(In reply to comment #24)
> Has this fix been released yet? I've been trying to base the height of columns off the min-height of the parent, but it's not working yet in Chrome or Safari.

Try the Chrome Canary build..

[Josh]

(In reply to comment #25)
> (In reply to comment #24)
> > Has this fix been released yet? I've been trying to base the height of columns off the min-height of the parent, but it's not working yet in Chrome or Safari.
> 
> Try the Chrome Canary build..

Yeah, tried that. No dice.

[Bem Jones-Bey]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > Has this fix been released yet? I've been trying to base the height of columns off the min-height of the parent, but it's not working yet in Chrome or Safari.
> > 
> > Try the Chrome Canary build..
> 
> Yeah, tried that. No dice.

I just looked, and Chrome Canary does seem to have the fix. Perhaps what you have is a different bug? Can you file a bug with an example of something that doesn't work, and I'll take a look at it?

Thanks.

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > Has this fix been released yet? I've been trying to base the height of columns off the min-height of the parent, but it's not working yet in Chrome or Safari.
> > > 
> > > Try the Chrome Canary build..
> > 
> > Yeah, tried that. No dice.
> 
> I just looked, and Chrome Canary does seem to have the fix. Perhaps what you have is a different bug? Can you file a bug with an example of something that doesn't work, and I'll take a look at it?

To me it seems if the container element's height is defined as a percentage, the child's height is still not adjusted correctly. Is this the same bug?

[Bem Jones-Bey]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > (In reply to comment #24)
> > > > > Has this fix been released yet? I've been trying to base the height of columns off the min-height of the parent, but it's not working yet in Chrome or Safari.
> > > > 
> > > > Try the Chrome Canary build..
> > > 
> > > Yeah, tried that. No dice.
> > 
> > I just looked, and Chrome Canary does seem to have the fix. Perhaps what you have is a different bug? Can you file a bug with an example of something that doesn't work, and I'll take a look at it?
> 
> To me it seems if the container element's height is defined as a percentage, the child's height is still not adjusted correctly. Is this the same bug?

Not sure. Can you (In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > (In reply to comment #24)
> > > > > Has this fix been released yet? I've been trying to base the height of columns off the min-height of the parent, but it's not working yet in Chrome or Safari.
> > > > 
> > > > Try the Chrome Canary build..
> > > 
> > > Yeah, tried that. No dice.
> > 
> > I just looked, and Chrome Canary does seem to have the fix. Perhaps what you have is a different bug? Can you file a bug with an example of something that doesn't work, and I'll take a look at it?
> 
> To me it seems if the container element's height is defined as a percentage, the child's height is still not adjusted correctly. Is this the same bug?

It is quite possible that it's a different bug. Can you file a new bug with an example attached that reproduces the problem? Then just assign it to me or email me the bug number, and I'll take a look as soon as I can.

[Paul d'Aoust]

I'm still not seeing the corrected behaviour in Chrome 29 or Safari 5/Win. Given a div with a min-width (explicit units; in my example it's 20em) and a child with a height of 100%, the child still only takes up as much as its contents. As soon as I change the container's min-width to width, the child expands to 100% as expected.

http://codepen.io/pdaoust/pen/nxfGw

Now, granted, I don't see the expected behaviour in Firefox 23, IE 10, or Opera 12, so at least all the browsers agree :-)

[Paul d'Aoust]

addendum: CSS 2.1 says that percentage-sized children should only respect containers with an explicit height, and if the container is sized based on its content, percentage-height children's computed height should be auto.

http://www.w3.org/TR/CSS2/visudet.html#the-height-property

This could be interpreted, I suppose, to mean that min-height is an explicit height, but there's the ambiguous case of what happens when the content makes the item bigger than the min-height? Should the percentage-sized child now have its height computed to auto? This could break expectations, as well as layouts. Perhaps the current behaviour needs to stay the way it is.

[Paul d'Aoust]

Sorry for the stream-of-consciousness; I just thought of (and discovered) something. If the container has a min-height and the child has a percentage min-height rather than a percentage height, perhaps then the child should expand to the height of the container.

However, this goes against the CSS spec. Maybe a bug should be filed against the spec.

[Bem Jones-Bey]

(In reply to comment #32)
> Sorry for the stream-of-consciousness; I just thought of (and discovered) something. If the container has a min-height and the child has a percentage min-height rather than a percentage height, perhaps then the child should expand to the height of the container.
> 
> However, this goes against the CSS spec. Maybe a bug should be filed against the spec.

It does sound like you're beyond the scope of this bug. I'd suggest that it's probably best to bring this up on the www-style mailing list (http://lists.w3.org/Archives/Public/www-style/), as the folks there would be better equipped than me to determine if this is a spec bug or not.

If it still looks like there is a bug in WebKit (or whatever else), then I'd suggest filing a new bug for this issue.

[Paul d'Aoust]

Oh, pardon me -- I understood this bug to be about (min|max)-height containers and height: <percentage> children not expanding or shrinking respective to the displayed size of the container. If I did read the intent of the bug report, my suggestion was to leave behaviour as-is, because it seems to be conforming to the CSS spec.

[Bem Jones-Bey]

(In reply to comment #34)
> Oh, pardon me -- I understood this bug to be about (min|max)-height containers and height: <percentage> children not expanding or shrinking respective to the displayed size of the container. If I did read the intent of the bug report, my suggestion was to leave behaviour as-is, because it seems to be conforming to the CSS spec.

Ah. From your last post, I had thought you were still wrestling with if you had a spec issue or not, which is why I suggested www-style. Happy to hear that things have been resolved!

[Alec Larson]

There must have been a regression. I'm getting this issue in Safari on iOS 10.3.2 (14F89).

I have a <div> with `height: 100%` and its parent <div> has `min-height: 36px` and no explicit `height`. The child does not fit its parent. It looks to be having its computed `height` set to `auto`.

[Shravan Rajinikanth]

I still seem to be having this issue on Chrome 64 in 2018. It occurs when the parent doesn't have a height property, but only min-height.

[Sebastian Andil]

Still seeing this on 
- Chrome 71.0.3578.80 (Official Build) (64-bit)
- Firefox Developer Editon 65.0b1 (64-bit)

[Andronachi Marian]

Hen this bug will be repaired? Wtf is from 2009 reported.