New Test for 'vertical-align has no effect on a grid item'

New Test for 'vertical-align has no effect on a grid item'

The spec says that 'vertical-align' has no effect on a grid item.
Currently there are no tests for this, so added a new test case
for verifying the same.

BUG=395788
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181912

[Sunil Ratnu, 2014-09-10 16:41:48]

sunil.ratnu@samsung.com changed reviewers:
+ esprehn@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Sunil Ratnu, 2014-09-10 16:41:49]

Please have a look. I am not exactly sure how to check the vertical-align
property on the grid item because each grid item creates a grid-level box. Do
let me know your suggestions. Thanks!

[Julien - ping for review, 2014-09-10 22:18:37]

I think the way of testing is adequate (if the property applied, the grid items'
content would be shifted by the vertical-align).

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html
(right):

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:6:
background-color:lightgrey;
Nit: I like to put a space after the semi-colon like in English.

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:9:
#item4 {
With that declaration, only one grid item is actually testing what you want.

You can make it match all the grid-items:

.grid > * { ... }

Or even have different values for vertical-align for increased coverage.

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:13:

The test should state what it's testing (aka description) and what the
expectations are (what is a passing / failing output).

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:19:
<!-- grid item: floated element; floating is ignored -->
This seems like it's testing something unrelated to what we want.

[Sunil Ratnu, 2014-09-11 12:38:02]

Thanks Julien for the review. Please take a look at the new patchset. Thanks!

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html
(right):

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:6:
background-color:lightgrey;
On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote:
> Nit: I like to put a space after the semi-colon like in English.

I've removed background-color in the latest patch. Should we keep it, keeping in
mind the option of providing more clarity to the user if someone wants to use
this code to test separately. What do you think?

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:9:
#item4 {
On 2014/09/10 22:18:37, Julien Chaffraix - PST wrote:
> With that declaration, only one grid item is actually testing what you want.
> 
> You can make it match all the grid-items:
> 
> .grid > * { ... }
> 
> Or even have different values for vertical-align for increased coverage.

Done.

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:13:

On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote:
> The test should state what it's testing (aka description) and what the
> expectations are (what is a passing / failing output).

Done.

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:19:
<!-- grid item: floated element; floating is ignored -->
On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote:
> This seems like it's testing something unrelated to what we want.

Removed "floating is ignored" but keeping the style as float so as to increase
the test coverage.

[Sunil Ratnu, 2014-09-11 12:59:44]

sunil.ratnu@samsung.com changed reviewers:
+ cbiesinger@chromium.org

[Julien - ping for review, 2014-09-11 18:24:20]

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html
(right):

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:6:
background-color:lightgrey;
On 2014/09/11 12:38:02, Sunil Ratnu wrote:
> On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote:
> > Nit: I like to put a space after the semi-colon like in English.
> 
> I've removed background-color in the latest patch. Should we keep it, keeping
in
> mind the option of providing more clarity to the user if someone wants to use
> this code to test separately. What do you think?

I am fine with some colors to ease visual debugging of the test.

https://codereview.chromium.org/543493004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html:19:
<!-- grid item: floated element; floating is ignored -->
On 2014/09/11 12:38:02, Sunil Ratnu wrote:
> On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote:
> > This seems like it's testing something unrelated to what we want.
> 
> Removed "floating is ignored" but keeping the style as float so as to increase
> the test coverage.
> 

What do you call increased test coverage? If float is ignored (and it should),
then it is identical to .item1. Furthermore, this test is about testing
vertical-align, not float. Let's just focus on what we are testing here and
remove this case.

[Sunil Ratnu, 2014-09-12 02:52:12]

Thanks for the review Julien. I've made the suggested changes. Please take a
look. Thanks!

[Julien - ping for review, 2014-09-12 14:53:33]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2014-09-12 14:53:34]

Awesome, thanks! LGTM

[commit-bot: I haz the power, 2014-09-12 14:54:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543493004/60001

[commit-bot: I haz the power, 2014-09-12 15:58:17]

Committed patchset #4 (id:60001) as 181912