[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





--- Comment #10 from Max Vujovic <mvujovic at adobe.com>  2012-01-24 15:18:01 PST ---
Created an attachment (id=123823)
 --> (https://bugs.webkit.org/attachment.cgi?id=123823&action=review)
Patch

(In reply to comment #7)
> (From update of attachment 123140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123140&action=review
> > 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?

Added tests for display: inline-table.

> > 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.

Added tests for writing-mode: vertical-rl and direction: rtl.

> 
> > 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.
> 

Agreed. I'm repurposing and renaming this bug for the automatic table layout case of min-width. I've split out the fixed table layout min-width case in Bug 76948, and linked it to the common master bug 12396.

> > 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?

I've removed this change from the patch because it's unrelated to implementing min-width. This change consisted of changing "!remainingPercent" to "remainingPercent <= 0" because remainingPercent is a float. I was worried that the logical negation operator (!) would not function correctly on the float if floating point error was present. However, I have not come up with a real example that causes this to happen yet, so I have not filed a bug.

For context, this check handles the case when percent width columns take up 100% of the table width, but there are also non-percent width columns that require some of the table's width. I've only seen this case described in a W3C editor's draft of table layout algorithms. Note that the draft warns that it is not being actively maintained:

(From http://dev.w3.org/csswg/css3-tables-algorithms/Overview.src.htm)
"c. When percent columns are demanding the full table width (100%) and there are also non-percent columns

Because the algorithm does not allow the sum of specified percentages to be larger than 100%, a special case might arise where the percent columns are demanding the full table width (100%) but there are also non-percent columns which require at least their minimum width. In this case, the tables preferred width could never be satisfied. The algorithm treats tables preferred width as infinitely large

tablePreferredWidth = INFINITE"

> > 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.

Firefox and Opera support both fixed and percent min-width. This patch also supports both fixed and percent min-width. 

Percent min-width is handled in RenderTable::computeLogicalWidth, and is intentionally not used in AutoTableLayout's calculation of preferred min and max width. This is analogous to percent width, which is not used in the AutoTableLayout's calculation of the preferred widths. I've added a note about this in the ChangeLog. Here is the existing code in AutoTableLayout::computePreferredLogicalWidths which only looks at a fixed width, if present:

    if (tableLogicalWidth.isFixed() && tableLogicalWidth.value() > 0) {
        minWidth = max<int>(minWidth, tableLogicalWidth.value());
        maxWidth = minWidth;
    }

In the above code snippet, note that minWidth and maxWidth refer to the preferred min width and the preferred max width as calculated by AutoTableLayout, not the min-width and max-width styles.

The following line actually computes percent widths. This line is in the new method RenderTable::convertStyleLogicalWidthToComputedWidth:

    return styleLogicalWidth.calcMinValue(availableWidth) + borders;

> > 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

Done.

> > 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

Done.

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

Done.

> > 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 && ...)

Done. I originally did this, but then took it out since Dirk didn't think it was appropriate because the boolean is only used once. If it's a matter of opinion rather than official WebKit coding style, I'd rather have the boolean because it's easier to read.

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