[webkit-reviews] review granted: [Bug 98633] max-height property is does not overriding the height properties for css tables(display:table) : [Attachment 173828] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Pravin D
<pravind.2k4 at gmail.com>'s request for review:
Bug 98633: max-height property is does not overriding the height properties for
css tables(display:table)
https://bugs.webkit.org/show_bug.cgi?id=98633

Attachment 173828: Patch
https://bugs.webkit.org/attachment.cgi?id=173828&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.