RE: TestTWF Pull requests needing review

Le Mer 6 novembre 2013 21:46, Zhang, Zhiqiang a écrit :
> From: Rebecca Hauck [mailto:rhauck@adobe.com]
> Sent: Tuesday, October 29, 2013 6:49 AM
>
> CSS UI:
> -------
> https://github.com/w3c/csswg-test/pull/95
> https://github.com/w3c/csswg-test/pull/97
>
> I've reviewed the 2 PRs on GitHub and made an improvement:
>
> http://zqzhang.github.io/review/box-sizing-001-review.html
> http://zqzhang.github.io/review/box-sizing-002-review.html
> http://zqzhang.github.io/review/box-sizing-001-ref-review.html
>
> Any comment is welcome.

[Resending this email as my webhost SMTP server seemed to have failed
sending it.]

Zhiqiang,

Here's some feedback on these tests.

http://zqzhang.github.io/review/box-sizing-001-review.html

line 4:
<title>CSS Basic User Interface Test: box-sizing - padding-box</title>

Suggestion:
<title>CSS Basic User Interface Test: box-sizing - padding-box
(basic)</title>

or

<title>CSS Basic User Interface Test: box-sizing - padding-box
(simple)</title>


line 6: <link rel="help" href="http://www.w3.org/TR/css3-ui/#box-sizing">

Suggestion:
<link rel="help" href="http://www.w3.org/TR/css3-ui/#box-sizing"
title="6.1. 'box-sizing' property">


line 11 to 17:
        div {
            height: 200px;
            left: 10px;
            position: absolute;
            top: 50px;
            width: 200px;
        }

setting left and top to an arbitrary value is not needed by the test and
not necessary in the test.

We already have a reftest which is very frequently used and reused for
tests which would fit perfectly here:

http://test.csswg.org/source/approved/css2.1/src/reference/ref-filled-green-100px-square.xht

Shepherd indicates that such reftest is referenced by 151 tests already

http://test.csswg.org/shepherd/reference/ref-filled-green-100px-square/

So, I would change 200px (height and width) for 100px and then just
reuse that reftest. In the long term, reducing the number of reftests
brings many benefits: reusability, reduce intricability (N tests to 1
reftests instead of multiple 1 test to 1 reftest), more efficient memory
management.


line 21:
            padding: 20px;

In other to maximize noticeability of test failure, I would set padding
to 50px.



line 30-31:
    <div class="test-overlapped-red"></div>
    <div class="ref-overlapping-green"></div>

Semantically speaking, class is for logical grouping of several
elements. Id is for linking to a document-unique element. So, here, I
would use id instead of class.

Gérard
-- 
Contributions to the CSS 2.1 test suite:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/

CSS 2.1 Test suite RC6, March 23rd 2011:
http://test.csswg.org/suites/css2.1/20110323/html4/toc.html

CSS 2.1 test suite harness:
http://test.csswg.org/harness/

Contributing to to CSS 2.1 test suite:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/web-authors-contributions-css21-testsuite.html

Received on Thursday, 7 November 2013 20:18:16 UTC