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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 171270: Patch
https://bugs.webkit.org/attachment.cgi?id=171270&action=review

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