Re: Review Report on section 8.4 (Padding property) ~= 125 testcases

On 08/21/2010 04:38 PM, "Gérard Talbot" wrote:
>
> - I spent quite some time on
> http://test.csswg.org/suites/css2.1/20100815/html4/c5507-ipadn-r-003.htm
> and someone else besides me will have to assess such testcase. I could
> not come up with a conclusion. Fantasai or someone else familiar with
> line breaking opportunities will have to make a call on that one.

I believe this testcase is correct.

> Section 8.4 (partial) review report
> ===================================
>
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/blocks-018.htm
>
> I do not think this is a good test.
>
> If the whole<span class="test">  is removed or if its padding-bottom
> declaration is removed, how does the test fail? Shouldn't the test show
> red?
> The test is not really testing what it aims at testing (padding-bottom).
>
> Rejected.

The test will show red if the padding on span.test increases the
effective line height, which is what it's trying to test. I'll
add an assertion to that effect. (A separate test would be needed
to show that the padding increases the background area.)

http://test.csswg.org/source/contributors/hixie/submitted/css2.1/box/blocks-018.xht

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/blocks-019.htm
>
> Same thing here. I do not think this is a good test.
>
> If the whole<span class="test">  is removed or if its padding-top
> declaration is removed, how does the test fail? Shouldn't the test show
> red?
> The test is not really testing what it aims at testing (padding-top).
>
> Rejected.

See above.
http://test.csswg.org/source/contributors/hixie/submitted/css2.1/box/blocks-010.xht

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5502-mrgn-r-002.htm
>
> To remove:
> <link rel="help"
> href="http://www.w3.org/TR/CSS21/box.html#padding-properties" title="8.4
> Padding properties: 'padding-top', 'padding-right', 'padding-bottom',
> 'padding-left', and 'padding'">
>
> because that testcase has very little (IMO nothing) to do with padding
> property testing. If you remove all of the padding declarations (and
> only rely on default padding values), then the test still passes.
>
> That testcase should not be removed but its listing in padding section
> should be removed.

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5502-mrgn-r-002.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5504-mrgn-l-002.htm
>
> To remove:
> <link rel="help"
> href="http://www.w3.org/TR/CSS21/box.html#padding-properties" title="8.4
> Padding properties: 'padding-top', 'padding-right', 'padding-bottom',
> 'padding-left', and 'padding'">
>
> That testcase should not be removed but its listing in padding section
> should not be.

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5502-mrgn-l-002.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5507-ipadn-r-003.htm
>
> I spent quite some time on this testcase. I am not sure of my conclusion.
> There is no white space collapsing occuring before rendering the block.
> And
> "
> If a space (U+0020) at the end of a line has 'white-space' set to
> 'normal', 'nowrap', or 'pre-line', it is also removed.
> "
> but
> "CSS 2.1 does not fully define where line breaking opportunities occur."
>
> All browsers (except Firefox 3.6.8) starts writing the first 2 "x"es of
> the span at position (line 1, column 9); Firefox starts such span at
> position (line 2, column 0). By itself, it is not sufficient to explain
> all of the differences in the layout. There are other line wrapping
> occurences generated by Firefox.
>
> Undefined.

While line breaking is not defined in the CSS specs, the test suite
assumes that spaces provide line breaking opportunities when white-space
is 'normal'.

Reviewed and approved: r=fantasai
http://test.csswg.org/source/approved/css2.1/src/css1/c5507-ipadn-r-003.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5507-ipadn-r-004.htm
>
> Suggested correction:
> <meta name="flags" content="ahem image">
>
> should be
>
> <meta name="flags" content="ahem image invalid">
>
> since a negative padding value will be parsed as an error.

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5507-ipadn-r-004.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5507-padn-r-003.htm
>
>    <style type="text/css">
>     div { margin: 1em; }
>     .test { width: 10em; padding-right: 2em; padding-right: -5em; color:
> yellow; background: navy; }
>     .control { width: 12em; color: yellow; background: navy; }
>    </style>
>
> could be rewritten as
>
>    <style type="text/css">
>     div { background: navy; color: yellow; margin: 1em; }
>     .test { width: 10em; padding-right: 2em; padding-right: -5em; }
>     .control { width: 12em; }
>    </style>

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5507-padn-r-003.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5508-ipadn-b-002.htm
>
>     p { margin-bottom: 3em; }
> should be removed

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5508-ipadn-b-002.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5508-ipadn-b-003.htm
>
> <meta name="flags" content="ahem">
>
> should be replaced with
>
> <meta name="flags" content="ahem invalid">

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5508-ipadn-b-003.htm

> A font-size 16px version of this test is available.

?

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5509-ipadn-l-001.htm
>
> Suggested more compact code:
>
>     .two span { padding-left: 4em; color: white; }
>
> can be replaced with
>
>     .two span { padding-left: 4em; }

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5509-ipadn-l-001.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5509-ipadn-l-004.htm
>
> Suggested correction:
> <meta name="flags" content="ahem image">
>
> should be
>
> <meta name="flags" content="ahem image invalid">
>
> since a negative padding value will be parsed as an error.
>
> Reviewed and approved.

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5509-ipadn-l-004.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5509-padn-l-003.htm
>
> Suggested corrections:
>
>    <meta name="flags" content="image">
> should be replaced with
>    <meta name="flags" content="image invalid">
>
> Suggested more compact code:
>
>    <style type="text/css">
>     div { margin: 1em; }
>     .test { width: 10em; padding-left: 2em; padding-left: -5em; color:
> yellow; background: navy; }
>     .control { width: 12em; color: yellow; background: navy; }
>
> can be replaced with
>
>    <style type="text/css">
>     div { margin: 1em; color: yellow; background: navy; }
>     .test { width: 10em; padding-left: 2em; padding-left: -5em; }
>     .control { width: 12em; }

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5509-padn-l-003.htm

> ----------------------
>
> Author: Ian Hickson
>
> .control .orange { border: solid 1em yellow; background: yellow; }
>
> is definitely counter-intuitive!
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5510-padn-001.htm

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5510-padn-001.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/c5510-padn-002.htm
>
> Suggested more compact code:
>
>     div { margin: 1em; }
>     .test { width: 10em; padding: 1em; padding: -5em; color: yellow;
> background: navy; }
>     .control { width: 10em; border: solid 1em navy; color: yellow;
> background: navy; }
>
> can be replaced with
>
>     div { margin: 1em; color: yellow; background: navy; }
>     .test { width: 10em; padding: 1em; padding: -5em; }
>     .control { width: 10em; border: solid 1em navy; }

Fixed.
http://test.csswg.org/source/approved/css2.1/src/css1/c5510-padn-002.htm

> ----------------------
>
> Author: Ian Hickson
>
> http://test.csswg.org/suites/css2.1/20100815/html4/padding-applies-to-016.htm
> and
> http://test.csswg.org/suites/css2.1/20100815/html4/padding-applies-to-017.htm
>
> Rejected.
>
> Required modification:
>
> To replace
>
>    <div class="table">
>     <tr>
>      <td>
>       <span class="test"><span class="control">There should be no red on
> this page.</span></span>
>      </td>
>     </tr>
>
>    </div>
>
> with
>
>    <table>
>     <tr>
>      <td>
>       <span class="test"><span class="control">There should be no red on
> this page.</span></span>
>      </td>
>     </tr>
>
>    </table>

Arron fixed this one.
http://test.csswg.org/source/contributors/hixie/submitted/css2.1/box/padding-applies-to-016.xht
http://test.csswg.org/source/contributors/hixie/submitted/css2.1/box/padding-applies-to-017.xht

~fantasai

Received on Thursday, 9 September 2010 20:50:58 UTC