Re: Review Report on section 8.5.x (Border property) ~= 260 testcases

On 08/25/2010 06:10 PM, "Gérard Talbot" wrote:
>
> Section 8.5 (partial) review report
> ===================================
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5511-brdr-tw-000.htm
>
> Why a table and cells for such test?

It makes it easier to see the variation if they are arrange side-by-side.

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5511-brdr-tw-001.htm
>
> Ideally, a text assert should state for that testcase that when the
> element's border color is not specified, then the value of the element's
> 'color' property must be used for the border color.
>
> Reviewed and approved

The test doesn't actually test that, though, because it doesn't require
the border to be green. I think the test is testing length values for
border-width, the color thing is just accidental.

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5511-ibrdr-tw-000.htm
>
> 1- class="one" is not defined anywhere in this testcase; class="one" can
> be safely removed from the testcase.

Fixed.

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5517-brdr-s-000.htm
>
> The left and right borders are rather short: so, e.g., it may make
> verifying the type of borders (left border and right border) difficult a
> bit in some sentences. E.g.:<p class="nine">(...) Blue and grooved on
> the left and right. The left and right borders are only 16px tall and
> 3px wide. There is very little chance realistically that a human would
> be able to say that such elft and right borders are groove and not
> ridge.
>
> Suggestion: add p {padding: 1em;} so that left and right borders height
> change from 16px to 48px and increase border-width everywhere, from
> thick to 12px or 16px or more.

Good idea. Added.

> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5519-brdr-r-001.htm
>
> "occurance" ->  occurrence
>
> s/occurance/occurrence/

Fixed.

> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5521-brdr-l-001.htm
>
> "occurance" ->  occurrence
>
> s/occurance/occurrence/

Fixed.

>     span, td { text-align: left; border-left: blue 2px solid; }
>     table { border-collapse: separate; border-spacing: 6px; }
>
> td is by default left-aligned; table border-collapse is separate by
> default. So, the above could be replaced with the more compact code:
>
>     span, td { border-left: blue 2px solid; }
>     table { border-spacing: 6px; }

Yes, but if the test is relying on this, then it's best to call it
out explicitly. The default UA style sheet is not normative, and
tests should avoid making assumptions about it other than those
documented on the test suite cover page.

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5522-brdr-000.htm
>
> Suggestion:
> p { color: blue; }
> changed to
> p { color: blue; padding: 1em;}
> so that border left and border right are 32px taller.

Fixed.

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5522-brdr-002.htm
>
> More compact code suggestion:
>
> Change
>     td, span {border: 2px solid blue;}
>     table { border-collapse: separate; border-spacing: 6px; }
>
> to
>
>     td {border: 2px solid blue;}
>     table { border-spacing: 6px; }

Removed the span; left the border-collapse: separate for reasons described above.

> Reviewed and approved
>
> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5522-ibrdr-000.htm
>
> There is no nested span in the testcase. So,
>
> span span { color: silver; }
> is irrelevant.

Fixed.

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/border-dynamic-001.htm
>
> Proposed modification:
>
> To change the following
>
>    <style type="text/css">
>     .test { background: green; }
>     .test div { margin: 10em 0; background: red; }
>    </style>
>   </head>
>
>   <body>
>    <p>There should be a big green box below.</p>
>    <div id="test" class="test">
>     <div></div>
>    </div>
>    <script type="text/javascript">
>     var x = document.getElementById('test');
>     x.clientHeight; // force reflow
>     x.style.border = "solid lime";
>    </script>
>
> to
>
>    <style type="text/css">
>     #test { background: green; }
>     #test div { margin: 10em 0; background: red; }
>    </style>
>   </head>
>
>   <body>
>    <p>There should be a big green box below with a bright green border
> and there should be no red.</p>
>    <div id="test">
>     <div></div>
>    </div>
>    <script type="text/javascript">
>     var x = document.getElementById('test');
>     x.clientHeight; // force reflow
>     x.style.border = "solid lime";
>    </script>

Fixed.

> Reviewed and approved
>
> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/border-dynamic-002.htm
>
> Proposed modification:
>
> To change the following
>
>    <style type="text/css">
>     .test { background: green; }
>     .test div { margin: 10em 0; background: red; }
>     .style { border: solid lime; }
>    </style>
>   </head>
>
>   <body>
>    <p>There should be a big green box below.</p>
>    <div id="test" class="test">
>     <div></div>
>    </div>
>    <script type="text/javascript">
>     var x = document.getElementById('test');
>     x.clientHeight; // force reflow
>     x.className += " style";
>    </script>
>
> to
>
>    <style type="text/css">
>     #test { background: green; }
>     #test div { margin: 10em 0; background: red; }
>     .limeborder { border: solid lime; }
>    </style>
>   </head>
>
>   <body>
>    <p>There should be a big green box below.</p>
>    <div id="test">
>     <div></div>
>    </div>
>    <script type="text/javascript">
>     var x = document.getElementById('test');
>     x.clientHeight; // force reflow
>     x.className = "limeborder";
>    </script>

Fixed.

~fantasai

Received on Thursday, 16 September 2010 03:00:33 UTC