Bug 14858 – <col> width ignored when not tied to a single cell

• Component: Tables
• Status: RESOLVED FIXED

[And Clover]

When <col>s are declared, a column width is only enforced if there is a single cell somewhere in that column. If the column contains only cells shared with other columns using colspan, the column ends up 'auto' width.

This occurs even in table-layout: fixed mode, and causes layouts that are heavy on colspan to break.

[David Kilzer (:ddkilzer)]

Thanks for the bug report!  Could you attach a test case demonstrating the issue as well?

[Chasen Le Hara]

Created attachment 18980 [details]
Test case

When rendered correctly, the page should be completely blank because the column widths are being respected. However, WebKit nightly r30053 (and below as far as I've seen) will display the word FAIL because the table cells don't span wide enough to cover the text.

[Dmitry Gorbik]

Created attachment 47295 [details]
Proposed fix

Fixes the bug for the fixed values of the <col> width. Otherwise handled as before.

[Dmitry Gorbik]

Table specs for this case: http://www.w3.org/TR/CSS2/tables.html#auto-table-layout

"For each column group element with a 'width' other than 'auto', increase the minimum widths of the columns it spans, so that together they are at least as wide as the column group's 'width'."

[David Kilzer (:ddkilzer)]

Comment on attachment 47295 [details]
Proposed fix

I am not a rendering expert, so I can't review this patch for correctness.  However, I will give it a first-pass review.

Overall, this is a great first patch, Dmitry!  Please consider the following feedback and post another patch for review.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 53776)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> [phone]  Dmitry Gorbik  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed width calculation for cells with span when <col> is defined
> +        https://bugs.webkit.org/show_bug.cgi?id=14858
> +
> +        Test: fast/table/col-width-span-expand.html
> +
> +        * rendering/RenderTableCell.cpp:
> +        (WebCore::RenderTableCell::styleOrColWidth):

A description of what was changed in the styleOrColWidth() method would be useful in the ChangeLog.  It's not strictly required, but often helps future bug fixers to understand why the code changes were made and what their intent was.  It can also help reviewers to understand what the bug fixer was thinking when they wrote the patch.

> Index: WebCore/rendering/RenderTableCell.cpp
> -
> +    

Please don't include gratuitous whitespace changes (unless you're removing whitespace).  See <http://webkit.org/coding/coding-style.html> and the check-webkit-style script.

>  Length RenderTableCell::styleOrColWidth() const
>  {
>      Length w = style()->width();
> -    if (colSpan() > 1 || !w.isAuto())
> +    if (!w.isAuto())
>          return w;
> +    
>      RenderTableCol* tableCol = table()->colElement(col());
>      if (tableCol) {
> -        w = tableCol->style()->width();
> +        int i = 1;

The 'i' variable should be declared in the scope of the loop if possible.

> +        int iColSpan = colSpan();

A better variable name for this might be "colSpanCount".

> +        Length colWidthSum = Length(0, Fixed);
> +        while (i <= iColSpan) {

This could be written as a for() loop:

    for (int i = 1; i <= colSpancount; ++i) {

> +            Length colWidth = tableCol->style()->width();
> +            
> +            // Percentage value should be returned only for colSpan=1
> +            // Otherwise we return original width for the cell

Comments that are sentences should end with a period.

> +            if (!colWidth.isFixed()) {
> +                if (iColSpan > 1)
> +                    return w;
> +                return colWidth;
> +            }
> +            
> +            colWidthSum = Length(colWidthSum.value() + colWidth.value(), Fixed);
> +            
> +            RenderObject * child = tableCol->nextSibling();

The '*' goes next to the type (RenderObject) in C++ code.

> +            // If no next <col> tag found for the span we just return what we have for now

Comment needs a period.

> +            if (child && child->isTableCol())
> +                tableCol = toRenderTableCol(child);  
> +            else 
> +                break;

I think the preferred way to write this would be to use an "early break":

    if (!child || !child->isTableCol())
        break;

    tableCol = toRenderTableCol(child);

> +            i++;

This line of code moved into the for() loop.

> +        }
>          
>          // Column widths specified on <col> apply to the border box of the cell.
>          // Percentages don't need to be handled since they're always treated this way (even when specified on the cells).
>          // See Bugzilla bug 8126 for details.
> -        if (w.isFixed() && w.value() > 0)
> -            w = Length(max(0, w.value() - borderLeft() - borderRight() - paddingLeft() - paddingRight()), Fixed);
> +        if (colWidthSum.isFixed() && colWidthSum.value() > 0)
> +            colWidthSum = Length(max(0, colWidthSum.value() - borderLeft() - borderRight() - paddingLeft() - paddingRight()), Fixed);
> +        return colWidthSum;
>      }
> +    
>      return w;
>  }
> +    

No need for an extra blank line after the method here.

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 53776)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> [phone]  Dmitry Gorbik  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed width calculation for cells with span when <col> is defined
> +        https://bugs.webkit.org/show_bug.cgi?id=14858         
> +
> +        * fast/table/col-width-span-expand.html: Added.

It's great that you included at test case with this change, but the patch is missing the expected results: both "text" results (*-expected.txt) and pixel test results (*-expected.png, *-expected.checksum) since this is not a "text-only" test case.  Using "run-webkit-tests" to generate expected text results (which should be a render tree dump), and add "--pixel" to generate pixel test results.

> Index: LayoutTests/fast/table/col-width-span-expand.html
> ===================================================================
> --- LayoutTests/fast/table/col-width-span-expand.html	(revision 0)
> +++ LayoutTests/fast/table/col-width-span-expand.html	(revision 0)
> @@ -0,0 +1,40 @@
> +<!DOCTYPE html>
> +<html>
> +	<head>
> +		<title>WebKit Bug 14858: col width ignored when not tied to a single cell</title>
> +		<link href="http://www.w3.org/TR/REC-CSS2/tables.html#q4" rel="help" />
> +		<style type="text/css">
> +div, table {
> +	left: 1em;
> +	position: absolute;
> +	top: 1em
> +}
> +div {
> +	color: red;
> +	left: 250px
> +}
> +table {border-spacing: 0}
> +td {
> +	background-color: white;
> +	padding: 0
> +}
> +		</style>
> +	</head>
> +	<body>
> +		<div>FAIL</div>
> +		<table>
> +			<col width="20" />
> +			<col width="40" />
> +			<col width="80" />
> +			<col width="160" />
> +			<tbody>
> +				<tr>
> +					<td>&nbsp;</td>
> +					<td colspan="2">&nbsp;</td>
> +					<td>&nbsp;</td>
> +				</tr>
> +			</tbody>
> +		</table>
> +	</body>
> +</html>

This test from the bug is good, but could be improved by:

- Adding HTML to add a link to the bugs.webkit.org bug.
- Adding text to describe how the test passes (since passing means that you can't see the red "FAIL" text any more, but no green "PASS" text appears).
- Using the CSS style definitions from LayoutTests/fast/js/resources/js-test-style.css is usually preferred for consistency, but not required.
- Instead of the last two items, switch the test to a text-only test case (see other tests that use "layoutTestController.dumpAsText()" and which include LayoutTests/fast/js/resources/js-test-pre.js and js-test-post.js) by testing the computed style width of the actual <td> elements (which should be 50, 250, and 200, I think).  This is preferred because pixel tests can differ between platforms, pixel tests are slower, and they take up more room on disk.  The js-test-* files also give you nice status output without you having to do much besides providing a "console" for them to use.

Also note that the "bot status" on bugs.webkit.org only checks that the patch builds, not that all of the layout tests pass.  You need to run the layout tests with your patch applied to make sure it doesn't cause any regressions:

./WebKitTools/Scripts/run-webkit-tests

Current layout test status for trunk:  <http://build.webkit.org/>

r- to address the above issues.  Thanks for working on this patch!

[Dmitry Gorbik]

Created attachment 47515 [details]
Proposed fix v2

[WebKit Review Bot]

Attachment 47515 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Gorbik]

Created attachment 47516 [details]
Proposed fix v2

[Dmitry Gorbik]

David, I do not think pixel or javascript tests are necessary here. Width of the cell is dumped in the RenderTree.

[David Kilzer (:ddkilzer)]

Comment on attachment 47516 [details]
Proposed fix v2

> Index: WebCore/rendering/RenderTableCell.cpp
> -}
> +}                   

Unneeded whitespace change here.

> Index: LayoutTests/fast/table/col-width-span-expand-expected.txt
> ===================================================================
> --- LayoutTests/fast/table/col-width-span-expand-expected.txt	(revision 0)
> +++ LayoutTests/fast/table/col-width-span-expand-expected.txt	(revision 0)
> @@ -0,0 +1,35 @@
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x8
> +  RenderBlock {HTML} at (0,0) size 800x8
> +    RenderBody {BODY} at (8,8) size 784x0
> +layer at (350,16) size 36x18
> +  RenderBlock (positioned) {DIV} at (350,16) size 36x18 [color=#FF0000]
> +    RenderText {#text} at (0,0) size 36x18
> +      text run at (0,0) width 36: "FAIL"
> +layer at (16,16) size 500x18
> +  RenderTable {TABLE} at (16,16) size 500x18
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableSection {TBODY} at (0,0) size 500x18
> +      RenderTableRow {TR} at (0,0) size 500x18
> +        RenderTableCell {TD} at (0,0) size 50x18 [bgcolor=#FFFFFF] [r=0 c=0 rs=1 cs=1]
> +          RenderText {#text} at (0,0) size 4x18
> +            text run at (0,0) width 4: " "
> +        RenderTableCell {TD} at (50,0) size 250x18 [bgcolor=#FFFFFF] [r=0 c=1 rs=1 cs=2]
> +          RenderText {#text} at (0,0) size 4x18
> +            text run at (0,0) width 4: " "
> +        RenderTableCell {TD} at (300,0) size 200x18 [bgcolor=#FFFFFF] [r=0 c=3 rs=1 cs=1]
> +          RenderText {#text} at (0,0) size 4x18
> +            text run at (0,0) width 4: " "
> +layer at (16,40) size 469x36
> +  RenderBlock (positioned) {DIV} at (16,40) size 469x36
> +    RenderText {#text} at (0,0) size 469x18
> +      text run at (0,0) width 469: "Visual test: if you do not see the red word \"FAIL\" then your have passed. "
> +    RenderBR {BR} at (469,0) size 0x18
> +    RenderInline {A} at (0,0) size 133x18 [color=#0000EE]
> +      RenderText {#text} at (0,18) size 133x18
> +        text run at (0,18) width 133: "WebKit Bug #14858"
> +    RenderText {#text} at (0,0) size 0x0

(In reply to comment #9)
> David, I do not think pixel or javascript tests are necessary here. Width of
> the cell is dumped in the RenderTree.

You are correct about the widths appearing in the render tree dump, but unfortunately this is not the way text-only tests work in WebKit.  If you don't use layoutTestController.dumpAsText(), run-webkit-tests and DumpRenderTree expect that there are pixel test results.

This test should still be turned into a text-only test.  There are many examples of text-only tests in the tree.  Start by searching for "dumpAsText()" in another test that appears to check something similar.

We also still need a domain expert in rendering to review the change, although my opinion is that the WebCore changes look good.

r- to fix the whitespace change above and to make the layout test a text-only test.

[Dmitry Gorbik]

Created attachment 47767 [details]
Proposed fix v2.1

I dont know what is better — to just put test table behind (z-index), or make it hidden by the use of css. I decided to place it behind, because I think that in case of "hidden" it may not be rendered at all in some cases.

[David Kilzer (:ddkilzer)]

Comment on attachment 47767 [details]
Proposed fix v2.1

The layout test is looking much better, but it doesn't quite match WebKit style for a text-only test.  Comments below.

> Index: LayoutTests/fast/table/col-width-span-expand-expected.txt
> ===================================================================
> --- LayoutTests/fast/table/col-width-span-expand-expected.txt	(revision 0)
> +++ LayoutTests/fast/table/col-width-span-expand-expected.txt	(revision 0)
> @@ -0,0 +1,3 @@
> +Test PASSED
> +Â 	Â 	Â 
> +WebKit Bug #14858

Can <br> tags be used in place of non-breaking spaces in the test?  The "Â " characters here are kind of odd.

> Index: LayoutTests/fast/table/col-width-span-expand.html
> ===================================================================
> --- LayoutTests/fast/table/col-width-span-expand.html	(revision 0)
> +++ LayoutTests/fast/table/col-width-span-expand.html	(revision 0)
> @@ -0,0 +1,62 @@
> +<!DOCTYPE html>
> +<html>
> +	<head>
> +		<title>WebKit Bug 14858: col width ignored when not tied to a single cell</title>
> +		<link href="http://www.w3.org/TR/REC-CSS2/tables.html#q4" rel="help" />
> +    <link href="../js/resources/js-test-style.css" rel="stylesheet" type="text/css">

Please don't use tabs in the test case unless absolutely necessary.

Also, there's usually no need for lots of indentation, but if you do indent, please do so consistently (using only spaces).

You also want to include here:

    <script src="../js/resources/js-test-pre.js"></script>
    <script src="../js/resources/js-test-post-function.js"></script>

> +    <script type="text/javascript" charset="utf-8">
> +      if (window.layoutTestController)
> +        layoutTestController.dumpAsText();

Usually we use 4 spaces to indent JavaScript code, but don't indent it relative to the <script> tag.

> +      function runTest()
> +      {

Please add a description method call here (from js-test-pre.js):

    description(
    "This tests <a href='http://webkit.org/b/14858'>Bug 14858: &lt;col&gt; width ignored when not tied to a single cell</a>."
    );

> +        var testCell = document.getElementById('testCell');
> +        var testTable = document.getElementById('testTable');

Good.

> +        var result = document.getElementById('result');
> +        var tdWidth = window.getComputedStyle(testCell).width;
> +        var tableWidth = window.getComputedStyle(testTable).width;
> +        if (tdWidth != "250px" || tableWidth != "500px") {
> +          result.style.color = "red";
> +          result.innerHTML = "Test FAILED";
> +        } else {
> +          result.style.color = "green";
> +          result.innerHTML = "Test PASSED";
> +        }

Instead of doing this, please use the methods provided in js-test-pre.js:

    shouldBe("window.getComputedStyle(testCell).width", "250px");
    shouldBe("window.getComputedStyle(testTable).width", "500px");

Then you need to add this to finish the test:

    isSuccessfullyParsed();

> +      }

Here you need to set the successfullyParsed variable which is used later:

    var successfullyParsed = true;

> +    </script>
> +		<style type="text/css">
> +div#testDiv {
> +  left: 1em;
> +  position: absolute;
> +  top: 1em;
> +  z-index: -1
> +}
> +table {border-spacing: 0}
> +td {
> +  background-color: white;
> +  padding: 0
> +}
> +		</style>

I don't think it's necessary to hide the table in the test.  If someone loads it in the browser, it should be easy to see that it passes.  You might consider making each table cell a different color.  Another best practice is to create a "baseline" table that has the same colors and expected widths as the test table so it's easy to verify that the test passes visually.

> +	</head>
> +	<body onload="runTest()">
> +		<div id="result"></div>

Instead of the "result" div, use a "description" and "console" elements (which are used by js-test-pre.js):

    <p id="description"></p>
    <div id="console"></div>

I would put the "console" div after the table, leaving the description at the top of the page.

> +    <div id="testDiv">
> +      <table id="testTable">
> +        <col width="50" />
> +        <col width="100" />
> +        <col width="150" />
> +        <col width="200" />
> +        <tbody>
> +          <tr>
> +            <td>&nbsp;</td>
> +            <td id="testCell" colspan="2">&nbsp;</td>
> +            <td>&nbsp;</td>
> +          </tr>
> +        </tbody>
> +      </table>
> +    </div>
> +    <div id="info">
> +      <a href="https://bugs.webkit.org/show_bug.cgi?id=14858">WebKit Bug #14858</a>
> +    </div>

The "info" div will be replaced by the "description" paragraph above.

> +	</body>
> +</html>

I'll ask someone to review the WebCore part of the patch since it hasn't changed recently.

r- to fix up the layout test.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> Instead of doing this, please use the methods provided in js-test-pre.js:
> 
>     shouldBe("window.getComputedStyle(testCell).width", "250px");
>     shouldBe("window.getComputedStyle(testTable).width", "500px");

Oops, I think you need to double-quote the expected values since they're also eval-ed:

    shouldBe("window.getComputedStyle(testCell).width", '"250px"');
    shouldBe("window.getComputedStyle(testTable).width", '"500px"');

[Dmitry Gorbik]

Created attachment 47772 [details]
Proposed fix v2.2

[Dmitry Gorbik]

Created attachment 47773 [details]
Proposed fix v2.3

[Dmitry Gorbik]

Created attachment 47775 [details]
Proposed fix v2.4

[Dave Hyatt]

Comment on attachment 47775 [details]
Proposed fix v2.4

RenderObject* child = tableCol->nextSibling();

^^^^ That's not adequate for picking up the next col is it?

[Dmitry Gorbik]

Created attachment 49318 [details]
proposed fix v2.5

[Eric Seidel (no email)]

(In reply to comment #13)
> (In reply to comment #12)
> > Instead of doing this, please use the methods provided in js-test-pre.js:
> > 
> >     shouldBe("window.getComputedStyle(testCell).width", "250px");
> >     shouldBe("window.getComputedStyle(testTable).width", "500px");
> 
> Oops, I think you need to double-quote the expected values since they're also
> eval-ed:
> 
>     shouldBe("window.getComputedStyle(testCell).width", '"250px"');
>     shouldBe("window.getComputedStyle(testTable).width", '"500px"');

There is also shouldBeEqualToString() which solves this problem.

[Eric Seidel (no email)]

Comment on attachment 49318 [details]
proposed fix v2.5

Is it a bug that the text dump makes the rows un-equal?

1 Two rows of cells should look identical.
 2 left	middle	right
 3 left	middle		right

[Dmitry Gorbik]

(In reply to comment #20)
> (From update of attachment 49318 [details])
> Is it a bug that the text dump makes the rows un-equal?
> 
> 1 Two rows of cells should look identical.
>  2 left    middle    right
>  3 left    middle        right

Different number of cells. So the empty cell adds spaces in case of four of them.

[David Kilzer (:ddkilzer)]

Comment on attachment 49318 [details]
proposed fix v2.5

This looks great!  Just a couple nits below:

> Index: WebCore/ChangeLog
> +        Fixed width calculation for cells with span when <col> is defined

You need to include a link to the bug in the ChangeLog entry.

> +        * rendering/RenderTableCell.cpp:
> +        (WebCore::RenderTableCell::styleOrColWidth): added the calculation of cell width
> +        in case of <col> defined and span>1.

Nit:  Change "span>1" to "span > 1".

> Index: WebCore/rendering/RenderTable.cpp
> +    RenderTableCol* colElem = toRenderTableCol(child);
> +    while (colElem) {
> +        int span = colElem->span();
> +        if (!colElem->firstChild()) {
> +            int startCol = cCol;
> +            int endCol = cCol + span - 1;
> +            cCol += span;
> +            if (cCol > col) {
> +                if (startEdge)
> +                    *startEdge = startCol == col;
> +                if (endEdge)
> +                    *endEdge = endCol == col;
> +                return colElem;
> +            }
> +        }
> +        colElem = nextColElement(colElem);
> +        if (!colElem)
>              break;
>      }

You can actually leave off the final "if (!colElem) break;" statements in the while loop since the next iteration through the loop will catch this condition and then return 0.

> Index: WebCore/rendering/RenderTableCell.cpp
> ===================================================================
> --- WebCore/rendering/RenderTableCell.cpp	(revision 53844)
> +++ WebCore/rendering/RenderTableCell.cpp	(working copy)
> @@ -83,18 +83,42 @@ void RenderTableCell::updateFromElement(
>  Length RenderTableCell::styleOrColWidth() const
>  {
>      Length w = style()->width();
> -    if (colSpan() > 1 || !w.isAuto())
> +    if (!w.isAuto())
>          return w;
> +
>      RenderTableCol* tableCol = table()->colElement(col());
> +
>      if (tableCol) {
> -        w = tableCol->style()->width();
> -        
> +        int colSpanCount = colSpan();
> +
> +        Length colWidthSum = Length(0, Fixed);
> +        for (int i = 1; i <= colSpanCount; i++) {
> +            Length colWidth = tableCol->style()->width();
> +
> +            // Percentage value should be returned only for colSpan=1.
> +            // Otherwise we return original width for the cell.

Why is this true?  Is this part of the standard?  Is this covered by an existing test case?  (If not, you need to add a test case for this condition as well.)

Nit:  "colSpan=1" should be "colSpan == 1" to be clearer that it's an equality check.

> +            if (!colWidth.isFixed()) {
> +                if (colSpanCount > 1)
> +                    return w;
> +                return colWidth;
> +            }
> Index: LayoutTests/ChangeLog
> [phone]  Dmitry Gorbik  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed width calculation for cells with span when <col> is defined

You need to include a link to the bug in the ChangeLog entry.

> Index: LayoutTests/fast/table/col-width-span-expand.html
> +	<head>

There is a tab on this line that should be 8 spaces.

r- to answer the question about the new code in RenderTableCell::styleOrColWidth().

Everything else looks good.  An explanation for the code in RenderTableCell::styleOrColWidth() and/or a layout test that covers that case will get an r+.

Thanks!

[Dmitry Gorbik]

David, that piece of code just falls back to previous behavour (before the patch). After it I do calculations based on "fixed" width, so if I dont return, that will crash (and crashes on existing test cases). I couldnt find in specifications what we do if we have to sum different types — fixed and absolute, so I do it the way it does not collide with previous tests.

[David Kilzer (:ddkilzer)]

(In reply to comment #23)
> David, that piece of code just falls back to previous behavour (before the
> patch). After it I do calculations based on "fixed" width, so if I dont return,
> that will crash (and crashes on existing test cases). I couldnt find in
> specifications what we do if we have to sum different types — fixed and
> absolute, so I do it the way it does not collide with previous tests.

Okay, I'll take another look at the old and new logic.  Thanks!

[David Kilzer (:ddkilzer)]

Comment on attachment 49318 [details]
proposed fix v2.5

(In reply to comment #22)
> (From update of attachment 49318 [details])
> This looks great!  Just a couple nits below:
> 
> > Index: WebCore/ChangeLog
> > +        Fixed width calculation for cells with span when <col> is defined
> 
> You need to include a link to the bug in the ChangeLog entry.
> 
> > +        * rendering/RenderTableCell.cpp:
> > +        (WebCore::RenderTableCell::styleOrColWidth): added the calculation of cell width
> > +        in case of <col> defined and span>1.
> 
> Nit:  Change "span>1" to "span > 1".
> 
> > Index: WebCore/rendering/RenderTable.cpp
> > +    RenderTableCol* colElem = toRenderTableCol(child);
> > +    while (colElem) {
> > +        int span = colElem->span();
> > +        if (!colElem->firstChild()) {
> > +            int startCol = cCol;
> > +            int endCol = cCol + span - 1;
> > +            cCol += span;
> > +            if (cCol > col) {
> > +                if (startEdge)
> > +                    *startEdge = startCol == col;
> > +                if (endEdge)
> > +                    *endEdge = endCol == col;
> > +                return colElem;
> > +            }
> > +        }
> > +        colElem = nextColElement(colElem);
> > +        if (!colElem)
> >              break;
> >      }
> 
> You can actually leave off the final "if (!colElem) break;" statements in the
> while loop since the next iteration through the loop will catch this condition
> and then return 0.
> 
> > Index: WebCore/rendering/RenderTableCell.cpp
> > ===================================================================
> > --- WebCore/rendering/RenderTableCell.cpp	(revision 53844)
> > +++ WebCore/rendering/RenderTableCell.cpp	(working copy)
> > @@ -83,18 +83,42 @@ void RenderTableCell::updateFromElement(
> >  Length RenderTableCell::styleOrColWidth() const
> >  {
> >      Length w = style()->width();
> > -    if (colSpan() > 1 || !w.isAuto())
> > +    if (!w.isAuto())
> >          return w;
> > +
> >      RenderTableCol* tableCol = table()->colElement(col());
> > +
> >      if (tableCol) {
> > -        w = tableCol->style()->width();
> > -        
> > +        int colSpanCount = colSpan();
> > +
> > +        Length colWidthSum = Length(0, Fixed);
> > +        for (int i = 1; i <= colSpanCount; i++) {
> > +            Length colWidth = tableCol->style()->width();
> > +
> > +            // Percentage value should be returned only for colSpan=1.
> > +            // Otherwise we return original width for the cell.
> 
> Why is this true?  Is this part of the standard?  Is this covered by an
> existing test case?  (If not, you need to add a test case for this condition as
> well.)

I reviewed the new logic again per Comment #23, and I agree it preserves the previous behavior, so this is no longer an issue.

> Nit:  "colSpan=1" should be "colSpan == 1" to be clearer that it's an equality
> check.
> 
> > +            if (!colWidth.isFixed()) {
> > +                if (colSpanCount > 1)
> > +                    return w;
> > +                return colWidth;
> > +            }
> > Index: LayoutTests/ChangeLog
> > [phone]  Dmitry Gorbik  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Fixed width calculation for cells with span when <col> is defined
> 
> You need to include a link to the bug in the ChangeLog entry.
> 
> > Index: LayoutTests/fast/table/col-width-span-expand.html
> > +	<head>
> 
> There is a tab on this line that should be 8 spaces.

r=me!  I'll clean up the nits above and land this patch.

[David Kilzer (:ddkilzer)]

Committed r56319: <http://trac.webkit.org/changeset/56319>

[Dmitry Gorbik]

Thanks Dave, I was about to fix all issues, just waited for your response about the logic.