Re: [css-grid] grid-vertical-align-001 and grid-inline-vertical-align-001: review request

Le 2014-11-20 03:14, Manuel Rego Casasnovas a écrit :
> Hi Gérard,
> 
> here are 2 tests checking that "vertical-align" property doesn't apply
> to grid items.
> 
> * grid-vertical-align-001:
>   * Source:
> https://github.com/mrego/csswg-test/blob/css-grid-vertical-align-tests/css-grid-1/grid-model/grid-vertical-align-001.xht
>   * View:
> https://rawgit.com/mrego/csswg-test/css-grid-vertical-align-tests/css-grid-1/grid-model/grid-vertical-align-001.xht

Rego,

Please consider the following as suggestions; in all fairness, I have 
just quickly/superficially read the grid spec and I'm definitely not the 
reviewer you deserve.

I would be tempted to replace some of your code with the following:

         <meta content="ahem" name="flags" />
(...)
             div#test-overlapping-green-grid {
                 display: grid;
                 font: 3.125em/1 Ahem; /* computes to 50px/50px */
             }

             span {
                 background-color: green;
             }

             span#vertical-align {
                 vertical-align: 125px;
             }
         ]]></style>
     </head>
     <body>
         <p>Test passes if there is a filled green square and <strong>no 
red</strong>.</p>

         <div id="reference-overlapped-red"></div>

         <div id="test-overlapping-green-grid">
             <span>1s</span>
             <span id="vertical-align">2n</span>
         </div>

I did not understand why you needed to use empty inline-blocks in your 
code. Maybe what you coded was okay... I don't know for sure.

If 'display: grid' elements form a containing block for their contents 
exactly like block containers do, then non-empty span elements could be 
more appropriately for testing.

---------------

> 
> * grid-inline-vertical-align-001:
>   * Source:
> https://github.com/mrego/csswg-test/blob/css-grid-vertical-align-tests/css-grid-1/grid-model/grid-inline-vertical-align-001.xht
>   * View:
> https://rawgit.com/mrego/csswg-test/css-grid-vertical-align-tests/css-grid-1/grid-model/grid-inline-vertical-align-001.xht
> 

         <meta content="ahem" name="flags" />
(...)

             div#test-overlapping-green-inline-grid {
                 display: inline-grid;
                 font: 3.125em/1 Ahem; /* computes to 50px/50px */
             }

             span {
                 background-color: green;
             }

             span#vertical-align {
                 vertical-align: 125px;
             }
         ]]></style>
     </head>
     <body>
         <p>Test passes if there is a filled green square and <strong>no 
red</strong>.</p>

         <div id="reference-overlapped-red"></div>

         <div id="test-overlapping-green-inline-grid">
             <span>1s</span>
             <span id="vertical-align">2n</span>
         </div>

---------------

http://test.csswg.org/source/css-grid-1/grid-model/grid-display-grid-001.xht

I revisited this test. Firefox 34.0.5 passes it but...

it's because Firefox 34.0.5  does not support 'display: grid' and so it 
is ignored and the default user agent style sheet display value for div 
elements (which is 'display: block') takes precedence. (And when I set 
the display to -moz-grid, then the test fails; it could be that 
-moz-grid is buggy or not implemented in Firefox stable release... I 
don't know. Firefox 37.0a1 build 20141202 also fails the test when 
setting 'display' to '-moz-grid'.) The same phenomenon occurs with 
Chrome 39.0.2171.71: it does not support 'display: grid' and so the 
default user agent style sheet 'display' value for div elements takes 
precedence.

So, I'd say right now that the test is not able (or smart enough, 
smartly tailored, crafted) to fail when the user agent does not support 
'display: grid'. So, this is a test which could be or would be 
eventually labelled as "NeedsWork" and as "false positive" by a 
stringent reviewer.

Ideally, we always want test to fail when a property or property value 
is not supported or not implemented.

Gérard
-- 
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 Monday, 8 December 2014 02:02:54 UTC