[Webkit-unassigned] [Bug 14858] <col> width ignored when not tied to a single cell

Sat Jan 30 09:29:45 PST 2010

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

--- Comment #12 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2010-01-30 09:29:44 PST ---
(From update of attachment 47767)
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: <col>
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> </td>
> +            <td id="testCell" colspan="2"> </td>
> +            <td> </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.

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