[webkit-reviews] review canceled: [Bug 76553] min-width is not implemented on <table> : [Attachment 123140] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Max Vujovic
<mvujovic at adobe.com>'s request for review:
Bug 76553: min-width is not implemented on <table>
https://bugs.webkit.org/show_bug.cgi?id=76553

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

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