[Webkit-unassigned] [Bug 76553] min-width is not implemented on <table> for table-layout: auto

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76553


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123850|review?, commit-queue?      |
               Flag|                            |




--- Comment #13 from Julien Chaffraix <jchaffraix at webkit.org>  2012-01-27 10:16:37 PST ---
(From update of attachment 123850)
View in context: https://bugs.webkit.org/attachment.cgi?id=123850&action=review

> LayoutTests/fast/table/min-width-expected.txt:246
> +PASS successfullyParsed is true

The output is huge - which is a good thing as you are covering most of the cases - but I would love for the test to be split in 2 to help maintenance and review.

> LayoutTests/fast/table/script-tests/min-width.js:93
> +    var propertyValue = table[propertyName];

I don't think this is sound. I don't see where you store the style value on the object (I tried this way and I did not always get the right value). I may be missing something but it looks like you want the computed value here and not the raw CSS value. If that's the case, you should use window.getComputedStyle(table, null)[propertyName].

> Source/WebCore/ChangeLog:31
> +            In FixedTableLayout.cpp, the computePreferredLogicalWidths method looks like it has
> +            issues based on the comment: "FIXME: This entire calculation is incorrect for both
> +            minwidth and maxwidth." (minwidth and maxwidth refer to the preferred widths, not the
> +            min-width and max-width styles). I have not implemented min-width for FixedTableLayout
> +            in this patch since it requires some more research around that comment.
> +
> +            min-width for the table-layout: fixed case has been split out into this bug:
> +            https://bugs.webkit.org/show_bug.cgi?id=76948

This is a great comment, this is preferably put at the beginning of the CL to underline it.

> Source/WebCore/rendering/AutoTableLayout.cpp:274
> +    if (tableLogicalMinWidth.isFixed() && tableLogicalMinWidth.value() > 0) {

You can use Length::isPositive here.

> Source/WebCore/rendering/RenderTable.cpp:231
> +    if ((styleLogicalWidth.isFixed() || styleLogicalWidth.isPercent()) && styleLogicalWidth.isPositive())

styleLogicalWidth.isFixed() || styleLogicalWidth.isPercent() can be replaced by styleLogicalWidth.isSpecified()

> Source/WebCore/rendering/RenderTable.cpp:253
> +    if ((styleMinLogicalWidth.isFixed() || styleMinLogicalWidth.isPercent()) && styleMinLogicalWidth.isPositive())

Ditto here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.