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

[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 #123140|review?                     |
               Flag|                            |




--- Comment #7 from Julien Chaffraix <jchaffraix at webkit.org>  2012-01-23 09:19:20 PST ---
(From update of attachment 123140)
View in context: https://bugs.webkit.org/attachment.cgi?id=123140&action=review

The change looks OK. Several questions and rough edges that I would like answered.

> LayoutTests/fast/table/script-tests/min-width.js:10
> +    table.setAttribute("style", "display: table; border-spacing: 0; background-color: #0f0; border: solid 0px #00f;" + extraTableStyle);

Shouldn't we consider the 'display: inline-table' case too?

> LayoutTests/fast/table/script-tests/min-width.js:121
> +shouldEvaluateTo("computeTableOffsetWidth(/*isCSSTable*/ true, /*min-intrinsic*/ 100, /*max-intrinsic*/ 250, /*extraTableStyle*/ 'min-width: auto;' + paddingsAndBorders30px)", 250+30);

You should have some testing for vertical writing-modes as your code seems to support this.

> Source/WebCore/ChangeLog:16
> +        * rendering/AutoTableLayout.cpp:
> +        (WebCore::AutoTableLayout::computePreferredLogicalWidths):
> +
> +            If the min or max preferred logical widths is less than a fixed min width style, it is
> +            set to the fixed min width style.

This only applies to automatic table layout which is only a subset of the goal of this bug. I am fine with repurposing the bug to support this subset. See the discussion on the www-style mailing list:

http://lists.w3.org/Archives/Public/www-style/2012Jan/0361.html

However we need to track the greater goal of also having fixed table layout on board.

> Source/WebCore/rendering/AutoTableLayout.cpp:268
> +    } else if (remainingPercent <= 0 && maxNonPercent) {

This change does not seem to be covered by your tests or am I missing something?

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

Does other browser supports percent min-width? This is comment-worthy in any case as you did not mention that on the www-style mailing list.

> Source/WebCore/rendering/AutoTableLayout.cpp:276
> +        minWidth = max<int>(minWidth, tableLogicalMinWidth.value());
> +        maxWidth = max<int>(minWidth, maxWidth);

Should be max<LayoutUnit>

> Source/WebCore/rendering/RenderTable.cpp:232
> +    Length styleWidth = style()->logicalWidth();
> +    if ((styleWidth.isFixed() || styleWidth.isPercent()) && styleWidth.isPositive())
> +        setLogicalWidth(convertStyleWidthToComputedWidth(styleWidth, containerWidthInInlineDirection));        

s/styleWidth/styleLogicalWidth/g

> Source/WebCore/rendering/RenderTable.cpp:254
> +    Length styleMinWidth = style()->logicalMinWidth();
> +    if ((styleMinWidth.isFixed() || styleMinWidth.isPercent()) && styleMinWidth.isPositive())
> +        setLogicalWidth(max(logicalWidth(), convertStyleWidthToComputedWidth(styleMinWidth, availableLogicalWidth)));

s/styleMinWidth/styleMinLogicalWidth/g

> Source/WebCore/rendering/RenderTable.cpp:268
> +LayoutUnit RenderTable::convertStyleWidthToComputedWidth(const Length& styleWidth, LayoutUnit availableWidth)

s/styleWidth/styleLogicalWidth/g

> Source/WebCore/rendering/RenderTable.cpp:272
> +    if ((!node() || !node()->hasTagName(tableTag)) && styleWidth.isFixed() && styleWidth.isPositive()) {

While at it, it would be neat to add a boolean for part of the logic:

bool isCSSTable = !node() || !node()->hasTagName(tableTag);
if (isCSSTable && ...)

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