Bug 98633 – max-height property not respected in case of tables

• Component: Tables
• Status: RESOLVED FIXED

[Pravin D]

The max-height property does not override the height property in case of a container having display:table.

Have attached a reduced test case for the same(from bug 25016).
courtesy: [e-mail]

[Pravin D]

Created attachment 167515 [details]
TestCase

[Pravin D]

Created attachment 169033 [details]
Proposed Patch

[Build Bot]

Comment on attachment 169033 [details]
Proposed Patch

Attachment 169033 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14389368

New failing tests:
fast/replaced/table-percent-height-text-controls.html
editing/pasteboard/5780697-2.html
http/tests/misc/acid2-pixel.html
http/tests/misc/acid2.html
fast/css/acid2-pixel.html
fast/replaced/table-percent-height.html
fast/dynamic/subtree-boundary-percent-height.html
css3/flexbox/percentage-sizes.html
fast/css/acid2.html

[WebKit Review Bot]

Comment on attachment 169033 [details]
Proposed Patch

Attachment 169033 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14387506

New failing tests:
editing/pasteboard/5780697-2.html
http/tests/misc/acid2-pixel.html
http/tests/misc/acid2.html
fast/css/acid2-pixel.html
fast/replaced/table-percent-height.html
fast/dynamic/subtree-boundary-percent-height.html
css3/flexbox/percentage-sizes.html
fast/css/acid2.html

[Pravin D]

Created attachment 170387 [details]
Proposed patch

[Pravin D]

Did some testing on the behavior of various browsers with regards to height distribution when max-height,min-height < computed content height. 
FF and IE do not respect max-height when the content height is greater than the max-height(for tables and css tables). 
The fix is that we simply take max-height, min-height in account in RenderTable::layout() while computing the height of the table.

distributeExtraLogicalHeight() (already present) will take care of distributing extra positive height. If the extra height is negative, it simply ignores it, as other browsers do.

Attachment
https://bugs.webkit.org/attachment.cgi?id=170387
does the above.


Attachment
https://bugs.webkit.org/attachment.cgi?id=169033
was actually with a little efficient version of attachment 170387 [details] in terms of code reuse and container height calculation. However it's a little bug at the moment.

@Julin, @Tony 
Would like ur opinions both the patches...

[Tony Chang]

Comment on attachment 170387 [details]
Proposed patch

The code in 170387 is more what I expect. I'm not sure what 169033 is trying to do.

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

> Source/WebCore/rendering/RenderTable.cpp:444
> +            // FIXME: We cannot apply box-sizing: content-box on <table> which other browsers allow.
> +            if ((node() && node()->hasTagName(tableTag)) || style()->boxSizing() == BORDER_BOX)

We should cache this expression in a bool and just write the comment once.

> Source/WebCore/rendering/RenderTable.cpp:451
> +    

Nit: Extra blank line.

> Source/WebCore/rendering/RenderTable.cpp:468
> +    

Nit: Extra blank line.

[Julien Chaffraix]

Comment on attachment 170387 [details]
Proposed patch

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

> Source/WebCore/rendering/RenderTable.cpp:463
> +        if (logicalMinHeightLength.isFixed()) {
> +            // HTML tables size as though CSS height includes border/padding, CSS tables do not.
> +            LayoutUnit borders = ZERO_LAYOUT_UNIT;
> +            // FIXME: We cannot apply box-sizing: content-box on <table> which other browsers allow.
> +            if ((node() && node()->hasTagName(tableTag)) || style()->boxSizing() == BORDER_BOX)
> +                borders = borderAndPaddingBefore + borderAndPaddingAfter;
> +            computedMinLogicalHeight = logicalMinHeightLength.value() - borders;

I really think this code should be some helper function as it is not be the only place where we have to do that (copying and pasting some code is bad!). We should also have a call to RenderBox::adjustBorderBoxLogicalHeightForBoxSizing for the CSS case as it would give us support for other box-sizing value for free (e.g. padding-box - see bug 23658) but that's more a nit.

> LayoutTests/fast/table/css-table-max-height.html:96
> +shouldBe("maxGreatThanMinHeightAutoLayout.getBoundingClientRect().height","202");

You should consider removing the padding on your cell and the border-spacing on your table so that this is 200px!

> LayoutTests/fast/table/css-table-max-height.html:108
> +onlyMaxHeightFixedLayout = document.getElementById("onlyMaxHeightFixedLayout");
> +shouldBe("onlyMaxHeightFixedLayout.getBoundingClientRect().height","202");

This looks like it's failing: max-height is 200px not 202px.

> LayoutTests/fast/table/css-table-max-height.html:112
> +document.body.removeChild(document.getElementById('container'));

This could be a check-layout.js test.

[Pravin D]

Created attachment 171270 [details]
Patch

[Pravin D]

(In reply to comment #8)
> (From update of attachment 170387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170387&action=review
> 
> > Source/WebCore/rendering/RenderTable.cpp:463
> > +        if (logicalMinHeightLength.isFixed()) {
> > +            // HTML tables size as though CSS height includes border/padding, CSS tables do not.
> > +            LayoutUnit borders = ZERO_LAYOUT_UNIT;
> > +            // FIXME: We cannot apply box-sizing: content-box on <table> which other browsers allow.
> > +            if ((node() && node()->hasTagName(tableTag)) || style()->boxSizing() == BORDER_BOX)
> > +                borders = borderAndPaddingBefore + borderAndPaddingAfter;
> > +            computedMinLogicalHeight = logicalMinHeightLength.value() - borders;
> 
Have moved the logic to a helper function
> I really think this code should be some helper function as it is not be the only place where we have to do that (copying and pasting some code is bad!). We should also have a call to RenderBox::adjustBorderBoxLogicalHeightForBoxSizing for the CSS case as it would give us support for other box-sizing value for free (e.g. padding-box - see bug 23658) but that's more a nit.
> 

I was not sure about this. 
In tables the padding is ignored if border collapsing set. adjustBorderBoxLogicalHeightForBoxSizing() uses border + padding to adjust the logical height. So using adjustBorderBoxLogicalHeightForBoxSizing() means we have to re-adjust the border values when border collaspse is set(border-collapse:collapse). 
So currently I have not used adjustBorderBoxLogicalHeightForBoxSizing() to calculate the border values.

Had a doubt(Off-topic)
adjustBorderBoxLogicalHeightForBoxSizing() add/subtract border+padding if the 
style()->boxSizing() == CONTENT_BOX and 

adjustContentBoxLogicalHeightForBoxSizing() add/subtract border+padding if the 
style()->boxSizing() == BORDER_BOX

Function name and logic inside the function seem to different. Is it intentional ? (I don not hv much idea about box sizing :( ).

> > LayoutTests/fast/table/css-table-max-height.html:96
> > +shouldBe("maxGreatThanMinHeightAutoLayout.getBoundingClientRect().height","202");
> 
> You should consider removing the padding on your cell and the border-spacing on your table so that this is 200px!
> 
> > LayoutTests/fast/table/css-table-max-height.html:108
> > +onlyMaxHeightFixedLayout = document.getElementById("onlyMaxHeightFixedLayout");
> > +shouldBe("onlyMaxHeightFixedLayout.getBoundingClientRect().height","202");
> 
> This looks like it's failing: max-height is 200px not 202px.
> 
Done...

> > LayoutTests/fast/table/css-table-max-height.html:112
> > +document.body.removeChild(document.getElementById('container'));
> 
> This could be a check-layout.js test.
> 
Done. 

However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be.

[Julien Chaffraix]

> > I really think this code should be some helper function as it is not be the only place where we have to do that (copying and pasting some code is bad!). We should also have a call to RenderBox::adjustBorderBoxLogicalHeightForBoxSizing for the CSS case as it would give us support for other box-sizing value for free (e.g. padding-box - see bug 23658) but that's more a nit.
> > 
> 
> I was not sure about this. 
> In tables the padding is ignored if border collapsing set. adjustBorderBoxLogicalHeightForBoxSizing() uses border + padding to adjust the logical height. So using adjustBorderBoxLogicalHeightForBoxSizing() means we have to re-adjust the border values when border collaspse is set(border-collapse:collapse). 
> So currently I have not used adjustBorderBoxLogicalHeightForBoxSizing() to calculate the border values.

Good point, we would need some solution to handle collapsing borders unfortunately. You can ignore this comment (which was a nit and orthogonal to your change).

> Had a doubt(Off-topic)
> adjustBorderBoxLogicalHeightForBoxSizing() add/subtract border+padding if the 
> style()->boxSizing() == CONTENT_BOX and 
> 
> adjustContentBoxLogicalHeightForBoxSizing() add/subtract border+padding if the 
> style()->boxSizing() == BORDER_BOX
> 
> Function name and logic inside the function seem to different. Is it intentional ? (I don not hv much idea about box sizing :( ).

Those 2 functions work as expected. The passed in logical height is supposed to be from the style and the functions checks box-sizing to adjust it accordingly. Check CSS 2.1 box model and the 'box-sizing' property if you are not convinced.

> However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be.

You need to read the source to understand the test at some point so it's not really an issue. However I do agree that it's unfortunate that check-layout.js doesn't dump what is tested nor what are the expected values. I don't know the decision behind that choice though so Tony may have some background.

[Julien Chaffraix]

Comment on attachment 171270 [details]
Patch

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

The test matches FF behavior (Opera has the same bug we have), haven't been able to check against IE.

Another round will be needed for the viewport relative inconsistency.

> Source/WebCore/rendering/RenderTable.cpp:329
> +LayoutUnit RenderTable::convertStyleLogicalHeightToComputedHeight(const Length& styleLogicalHeight, bool collapseBorders)

I wouldn't pass collapseBorders in the new method. Querying the value from the style should be cheap enough.

> Source/WebCore/rendering/RenderTable.cpp:467
> +    if (logicalMaxHeightLength.isSpecified() && !logicalMaxHeightLength.isNegative()) {

This will not work if max-height or min-height are view-port relative. Actually it will badly break: the code will compute the 2 values to be 0.

Using isSpecified() was a good idea as it is consistent with the existing logical width code but this means you have to fix convertStyleLogicalHeightToComputedHeight to properly handle view-port relative units. I don't mind either way but this will need to be tested or tracked.

> LayoutTests/fast/table/css-table-max-height-expected.txt:4
> +Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
> +PASS

This placeholder text isn't needed as you force the table logical height and could be dropped.

If you really think it's needed, I would put some explanations instead of a placeholder as it would make the output more readable. For example:

This sub-test checks that max-height is applied to a table.
PASS

> LayoutTests/fast/table/css-table-max-height.html:9
> +    border:1px solid green;

Not sure the border adds much here as we are 100% height.

> LayoutTests/fast/table/css-table-max-height.html:70
> +    <div iclass="child fixed-table" style="max-height:200px;" data-expected-height=200>

typo 'iclass', that's why you don't have a PASS in the output.

[Tony Chang]

(In reply to comment #11)
> > However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be.
> 
> You need to read the source to understand the test at some point so it's not really an issue. However I do agree that it's unfortunate that check-layout.js doesn't dump what is tested nor what are the expected values. I don't know the decision behind that choice though so Tony may have some background.

If a check fails, it dumps the details of the failure.  For this test, I think either js-test-pre.js or check-layout.js would be OK.  check-layout.js is probably most useful if you want to test lots of properties (width, height, offsets, etc for dozens of elements).

Maybe we should add an optional second param to checkLayout that emits the test condition and value (as a separate patch).

[Pravin D]

(In reply to comment #13)
> (In reply to comment #11)
> > > However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be.
> > 
> > You need to read the source to understand the test at some point so it's not really an issue. However I do agree that it's unfortunate that check-layout.js doesn't dump what is tested nor what are the expected values. I don't know the decision behind that choice though so Tony may have some background.
> 
> If a check fails, it dumps the details of the failure.  For this test, I think either js-test-pre.js or check-layout.js would be OK.  check-layout.js is probably most useful if you want to test lots of properties (width, height, offsets, etc for dozens of elements).
> 

Given a choice i'd prefer js-test-pre.js over check-layout.js(atleast in this case). However I'd leave the decision to either one of u(Julien or Tony).
 
> Maybe we should add an optional second param to checkLayout that emits the test condition and value (as a separate patch).
> 
There is a drawback in using check-layout.js over js-test-pre.js.
Inequality comparisons are not possible when using checkLayout such as ">" or "<" as these have special meaning in html and will lead to bad consequences.

For example the case in the test case using
data-expected-height=144

Actually what I wanted was data-expected-height > 100 (showing that height of the table can be more than max-height value when the content height is greater). However this is not possible when using checkLayout

[Pravin D]

Created attachment 173081 [details]
Patch

[Pravin D]

(In reply to comment #12)
> (From update of attachment 171270 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171270&action=review
> 


> > Source/WebCore/rendering/RenderTable.cpp:329
> > +LayoutUnit RenderTable::convertStyleLogicalHeightToComputedHeight(const Length& styleLogicalHeight, bool collapseBorders)
> 
> I wouldn't pass collapseBorders in the new method. Querying the value from the style should be cheap enough.
> 

Done.

> > Source/WebCore/rendering/RenderTable.cpp:467
> > +    if (logicalMaxHeightLength.isSpecified() && !logicalMaxHeightLength.isNegative()) {
> 
> This will not work if max-height or min-height are view-port relative. Actually it will badly break: the code will compute the 2 values to be 0.
> 
> Using isSpecified() was a good idea as it is consistent with the existing logical width code but this means you have to fix convertStyleLogicalHeightToComputedHeight to properly handle view-port relative units. I don't mind either way but this will need to be tested or tracked.
> 
Added support for handling viewport percent height values.
Also added a set of sub-tests to check the same.

> > LayoutTests/fast/table/css-table-max-height-expected.txt:4
> > +Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
> > +PASS
> 
> This placeholder text isn't needed as you force the table logical height and could be dropped.
> 
> If you really think it's needed, I would put some explanations instead of a placeholder as it would make the output more readable. For example:
> 
> This sub-test checks that max-height is applied to a table.
> PASS
> 

The height of the table is governed by it's content height in some sub tests. To keep the test case more informative and maintain consistency across sub tests have used short description text as placeholders.

> > LayoutTests/fast/table/css-table-max-height.html:9
> > +    border:1px solid green;
> 
> Not sure the border adds much here as we are 100% height.
> 
Done.
> > LayoutTests/fast/table/css-table-max-height.html:70
> > +    <div iclass="child fixed-table" style="max-height:200px;" data-expected-height=200>
> 
> typo 'iclass', that's why you don't have a PASS in the output.
> 
thanks for the catch. Done

[Julien Chaffraix]

Comment on attachment 173081 [details]
Patch

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

> Source/WebCore/rendering/RenderTable.cpp:345
> +        computedLogicalHeight = minimumValueForLength(styleLogicalHeight, 0, view());

I would add this to your code to ensure that we didn't forget a case (like calculated which should hopefully fall back on one of the two above):

else
    ASSERT_NOT_REACHED();

> LayoutTests/fast/table/css-table-max-height.html:65
> +    <div class="child" style="max-height:100px;" data-expected-height=128>
> +        This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout.
> +    </div>

I don't really understand the final height. 'max-height' should override 'height' and the computed height should be 100px (not 128px).

Also your wording is confusing as it's the max-height who should be applied.

[Pravin D]

Created attachment 173828 [details]
Patch

[Pravin D]

(In reply to comment #17)
> (From update of attachment 173081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173081&action=review
> 
> > Source/WebCore/rendering/RenderTable.cpp:345
> > +        computedLogicalHeight = minimumValueForLength(styleLogicalHeight, 0, view());
> 
> I would add this to your code to ensure that we didn't forget a case (like calculated which should hopefully fall back on one of the two above):
> 
> else
>     ASSERT_NOT_REACHED();
>  
Done.

> > LayoutTests/fast/table/css-table-max-height.html:65
> > +    <div class="child" style="max-height:100px;" data-expected-height=128>
> > +        This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout.
> > +    </div>
> 
> I don't really understand the final height. 'max-height' should override 'height' and the computed height should be 100px (not 128px).
> 
> Also your wording is confusing as it's the max-height who should be applied.
> 
Sorry for not explaining this case before. 

The height, max-height when used with tables have a slightly different behavior .
The content height always gets the highest priority , i.e Priority wise  -- Content Height > max-height > height.
This is true across all other browsers(verified the case on FF).  This is avoid the negative height distribution I guess. 
In the testcase we check that the above behavior is not affected by the code added to handle max-height, min-height.

[Julien Chaffraix]

Comment on attachment 173828 [details]
Patch

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

I don't need to see the updated change.

> Source/WebCore/ChangeLog:8
> +        max-height property is does not overriding the height properties for css tables(display:table)
> +        https://bugs.webkit.org/show_bug.cgi?id=98633
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The max-height property determines the maximum computed height an element can have. In case of css tables(display:table)

Note that it was all tables and not just CSS tables that were impacted. Please update the bug title and this line to reflect that (fixing also the extra 'is' in the title)

> LayoutTests/fast/table/css-table-max-height.html:65
> +    <div class="child" style="max-height:100px;" data-expected-height=128>
> +        This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout.
> +    </div>

This doesn't match any other browsers (even FF returns 100px here) and the div case so I don't agree with your analysis. As you pointed out this is a bug in our extra logical height spreading algorithm (see bug 81824 for some context) so it is orthogonal to this change.

My take would be to land with the real expected result (ie 100px and failing that) along with a comment in the test instead of accepting our broken behavior in this case.

> LayoutTests/fast/table/css-table-max-height.html:100
> +<div class="container">
> +    <div class="child fixed-table" style="max-height:100px;" data-expected-height=128>
> +        This sub-test checks that when content height is greater than max-height, content height is applied to a table with fixed layout.
> +    </div>

Same comment here.

[Pravin D]

Created attachment 175167 [details]
TestCase with content height > max-height

In the testcase used in the previous patch when rendered on FF did not have enough content to satisfy the condition Content Height > max-height.
This testcase can be used to check the behavior on FF and Webkit.

[Pravin D]

Created attachment 175179 [details]
Patch

[Pravin D]

(In reply to comment #20)
> (From update of attachment 173828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173828&action=review
> 
> I don't need to see the updated change.
> 
> > Source/WebCore/ChangeLog:8
> > +        max-height property is does not overriding the height properties for css tables(display:table)
> > +        https://bugs.webkit.org/show_bug.cgi?id=98633
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        The max-height property determines the maximum computed height an element can have. In case of css tables(display:table)
> 
> Note that it was all tables and not just CSS tables that were impacted. Please update the bug title and this line to reflect that (fixing also the extra 'is' in the title)
> 
Done.

> > LayoutTests/fast/table/css-table-max-height.html:65
> > +    <div class="child" style="max-height:100px;" data-expected-height=128>
> > +        This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout.
> > +    </div>
> 
> This doesn't match any other browsers (even FF returns 100px here) and the div case so I don't agree with your analysis. As you pointed out this is a bug in our extra logical height spreading algorithm (see bug 81824 for some context) so it is orthogonal to this change.
> 
> My take would be to land with the real expected result (ie 100px and failing that) along with a comment in the test instead of accepting our broken behavior in this case.
> 
> > LayoutTests/fast/table/css-table-max-height.html:100
> > +<div class="container">
> > +    <div class="child fixed-table" style="max-height:100px;" data-expected-height=128>
> > +        This sub-test checks that when content height is greater than max-height, content height is applied to a table with fixed layout.
> > +    </div>
> 
> Same comment here.
> 

Have made some changes to the testcase. The issue was that in the previous testcase the content height did not exceed the max-height on FF. Also the computed height when content height is different on FF, Chrome(or Safari) and DumpRenderTree. Also run-webkit-tests gives a different set of values for computed height in case when content height > max-height from the ones found when testcase is fetched using FF or Chrome. The expected test result is in accordance to values found in dumpRenderTree.

Also check attachment https://bugs.webkit.org/attachment.cgi?id=175167 for comparing the behavior of 
FF and Webkit based browser.
In conclusion have changed the testcase such that all the sub tests pass(no expected failures)

Please let me know if your opinion on the same. Maybe if you feel that the height of table should not exceed max-height in any case, I can upload another patch reflecting the same.

[Julien Chaffraix]

Comment on attachment 175179 [details]
Patch

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

> Please let me know if your opinion on the same. Maybe if you feel that the height of table should not exceed max-height in any case

OK, FF and IE match this behavior. Opera has a weird behavior.

> LayoutTests/fast/table/css-table-max-height.html:101
> +    <div class="child fixed-table" style="max-height:100px;" data-expected-height=192>
> +        This sub-test checks that when content height is greater than max-height, content height is applied to a table with fixed layout.
> +        <br><br>FILLER TEXT TO INCREASE CONTENT HEIGHT.

Note that this makes the test dependent on the text size. Hopefully this shouldn't make the test platform-dependent.

[WebKit Review Bot]

Comment on attachment 175179 [details]
Patch

Clearing flags on attachment: 175179

Committed r135549: <http://trac.webkit.org/changeset/135549>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It made zillions tests crash in debug mode on all bot.

[Pravin D]

(In reply to comment #27)
> It made zillions tests crash in debug mode on all bot.
> 

Have created a bug for the same. 
See Bug 103100

[WebKit Review Bot]

Re-opened since this is blocked by bug 103102

[Pravin D]

Created attachment 175823 [details]
Patch

[Pravin D]

(In reply to comment #30)
> Created an attachment (id=175823) [details]
> Patch
> 

The asserts caused by r135549 where due the fact that the patch did not handle the case when logical height was not specified(default auto). 
Fixed the same.
Also added a sub-test to check the same.
Sorry for the trouble.

[Julien Chaffraix]

Comment on attachment 175823 [details]
Patch

r=me again, assuming no ASSERT triggering.

[WebKit Review Bot]

Comment on attachment 175823 [details]
Patch

Clearing flags on attachment: 175823

Committed r135891: <http://trac.webkit.org/changeset/135891>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.