W3C home > Mailing lists > Public > public-css-testsuite@w3.org > November 2014

Re: [css-grid] display-grid, display-inline-grid and parsing-display-grid-inline: review request

From: Gérard Talbot <css21testsuite@gtalbot.org>
Date: Sat, 08 Nov 2014 22:03:50 -0500
To: Manuel Rego Casasnovas <rego@igalia.com>
Cc: Public CSS Test suite mailing list <public-css-testsuite@w3.org>
Message-ID: <1e96982461bbc4c5ada1b1b39698fdc3@gtalbot.org>
Le 2014-11-03 11:51, Manuel Rego Casasnovas a écrit :
> Fixing the subject. Sorry :-(
> 
> On 03/11/14 17:47, Manuel Rego Casasnovas wrote:
>> Hi Gérard,
>> 
>> here are my first 3 tests checking the new values "grid" and
>> "inline-grid" for "display" property.
>> 
>> I've joint them in a single pull request but I can split it in 3
>> different pull-requests as you wish.
>> 
>> Pull request: https://github.com/w3c/csswg-test/pull/626
>> 
>> Tests sources:
>> *
>> https://github.com/Igalia/csswg-test/blob/css-grid-display-tests/css-grid-1/grid-model/display-grid.xht

Filename should be following the file name format test-topic-###.ext :

"
The file name format is test-topic-###.ext where test- topic somewhat 
describes the test and ### is a zero-filled number used to keep the file 
names unique.
"
http://testthewebforward.org/docs/css-naming.html

So here, I would suggest to filename-rename your test as

grid-display-001.xht

or as

display-grid-001.xht

or as

grid-display-grid-001.xht

As a test creator, you want to achieve an unique and somewhat 
descriptive filename. In order to achive this, you may have to prefix 
your filename with the most significant name of the spec.

Eg.
All the tests in
http://test.csswg.org/source/css-multicol-1/
are prefixed with "multicol" so that there can not be a filename 
conflict with any tests of any other spec, including (especially) 
CSS2.1.


-----

line 4:  <meta charset="utf-8" />

Over time, the HTML5 template was created and then there was some mixup 
with the XHTML 1 template. <meta charset="utf-8"> is for HTML5. The 
original, initial XHTML 1 template did not require to declare a 
character set because character encoding of the XHTML document is by 
default UTF-8...which is the character encoding that should be using all 
tests... otherwise you would declare the character encoding into an xml 
declaration.

If you decide to create your tests in XHTML1, then I suggest to install 
HTML Validator 0.9.5.8 (extension available for Firefox only at
http://users.skynet.be/mgueury/mozilla/ ) which can run offline and will 
report any validation error with your tests.

-----

line 5: <title>CSS Grid Layout: 'grid' value for 'display' 
property</title>

I suggest

<title>CSS Grid Layout Test: 'grid' value for 'display' property</title>

-----

line 7:
<link rel="help" href="http://www.w3.org/TR/css-grid-1/#grid-containers" 
/>

If there is more than 1 help link, then it is preferable to specify each 
link with the title attribute with its section description. Like:

<link rel="help" href="http://www.w3.org/TR/css-grid-1/#grid-containers" 
title="3.1 Establishing Grid Containers" />

Some people like myself use a document relation bar extension and we can 
read the title of such link in our browser.

-----

line 10: <style>

If you create a test in HTML5, then <style> is correct.
If you create a test in XHTML1, then use
   <style type="text/css"><![CDATA[
   (...)
   ]]></style>

The <![CDATA[ and ]]> chunks are solely for understanding and managing 
adequately, accordingly <, > and & which could be inside the CSS code 
(child selector) or javascript code (logical AND).

More info:
http://www.w3.org/TR/xhtml1/#h-4.8

-----

line 19: .grid-overlapping-green

If you use "reference" in an identifier, then I would normally expect 
its correspondent counter-part in the test to use "test". So, I suggest

line 10 #reference-overlapped-red

line 19 .test-overlapping-green

What you did was already good.

-----

Nitpicking

"For indentation, spaces are preferred over tabs."
http://testthewebforward.org/docs/review-checklist.html

-----

With GitHub, I can not view the test as rendered in a browser (don't 
know how; I had to copy and save the code into an HTML editor and then 
open it in a browser ... not convenient!) and I can not verify things 
like trailing whitespace (whitespace at the end of lines) or Unicode 
byte order mark (BOM U+FEFF) (Shepherd is able to spot and report this, 
btw) and usage of Unix line endings.

>> *
>> https://github.com/Igalia/csswg-test/blob/css-grid-display-tests/css-grid-1/grid-model/display-inline-grid.xht

On top of issues spotted in your first test,

I suggest to filename-rename this test as

grid-display-002.xht

or

display-grid-002.xht

or

grid-display-inline-grid-001.xht




>> *
>> https://github.com/Igalia/csswg-test/blob/css-grid-display-tests/css-grid-1/grid-model/parsing-display-grid-inline.xht

On top of issues spotted in your first test,...

lines 10, 11 and 34:
  <script src="/resources/testharness.js"></script>
  <script src="/resources/testharnessreport.js"></script>
  <script>
type="text/javascript" is required and should be declared in XHTML1 
document but not in HTML5.


Addendum
********
For a reason I do not understand, locally, I must edit
<script type="text/javascript" 
src="./resources/testharness.js"></script>
<script type="text/javascript" 
src="./resources/testharnessreport.js"></script>
because this linkage
<script type="text/javascript" src="/resources/testharness.js"></script>
<script type="text/javascript" 
src="/resources/testharnessreport.js"></script>
will not work.

-----

line 39:
You may want to do a "search and replace" in your file of the string
testComptuedStyleDisplay
with
testComputedStyleDisplay

Gérard

>> 
>> Thanks,
>>    Rego

-- 
Test Format Guidelines
http://testthewebforward.org/docs/test-format-guidelines.html

Test Style Guidelines
http://testthewebforward.org/docs/test-style-guidelines.html

Test Templates
http://testthewebforward.org/docs/test-templates.html

CSS Naming Guidelines
http://testthewebforward.org/docs/css-naming.html

Test Review Checklist
http://testthewebforward.org/docs/review-checklist.html

CSS Metadata
http://testthewebforward.org/docs/css-metadata.html
Received on Sunday, 9 November 2014 03:04:28 UTC

This archive was generated by hypermail 2.3.1 : Tuesday, 6 January 2015 21:13:27 UTC