[css-grid] refactoring of layout tests

[css-grid] refactoring of layout tests

In order to manage the css values and properties prefixes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.

Committed: https://crrev.com/1314b8fc0bc46b2e7f05095ecd35d34147b20bed
Cr-Commit-Position: refs/heads/master@{#370840}

[jfernandez, 2015-11-30 10:21:48]

Description was changed from

==========
[css-grid] refactoring of layout tests

In order to manage the css value and properties prefiexes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.
==========

to

==========
[css-grid] refactoring of layout tests

In order to manage the css value and properties prefiexes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.
==========

[jfernandez, 2015-11-30 10:21:48]

jfernandez@igalia.com changed reviewers:
+ rego@igalia.com, svillar@igalia.com

[jfernandez, 2015-11-30 10:22:18]

Description was changed from

==========
[css-grid] refactoring of layout tests

In order to manage the css value and properties prefiexes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.
==========

to

==========
[css-grid] refactoring of layout tests

In order to manage the css values and properties prefixes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.
==========

[Manuel Rego, 2015-12-02 09:31:02]

Non-owner LGTM.

Really nice patch, thanks for the effort.
Just a few comments inline.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:12:
}
This change is unrelated to prefixes, it's about different viewport sizes.
Maybe you want to add a line about that in the patch description.
Or do this particular change in a different patch.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:67:
<div class="container">
I think you don't need the container here.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:76:
<div class="container">
Ditto.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:85:
<div class="container">
Ditto.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html:80:
<div class="grid fit-content fitContentOnlyWidth columnsSmallerThanContentBox
rowsBiggerThanMarginBox">
This was not using "-webkit-fit-content" before, why we need to change it?

Probably the classes "fitContentHeight|Width" should be renamed,
as they seem to be only setting start alignment.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html:29:
height: auto;
height is "auto" by default, I don't think you need this class.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css:265:
-webkit-writing-mode: vertical-rl;
Usually you write this the other way around.

First "-webkit-writing-mode" and later "writing-mode".

So I'd change it to:
.verticalRL {
    -webkit-writing-mode: vertical-rl;
    writing-mode: vertical-rl;
}

If the unprefixed one is implemented, we prefer that one instead of the prefixed
one.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css:269:
-webkit-writing-mode: vertical-lr;
Ditto.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css:273:
-webkit-writing-mode: horizontal-tb;
Ditto.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css:277:
-webkit-writing-mode: horizontal-bt;
Ditto.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css:286: 
Nit: Unneeded extra lines.

[jfernandez, 2015-12-03 15:06:11]

Thanks for the review. See my comments inline.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:12:
}
On 2015/12/02 09:31:01, Manuel Rego wrote:
> This change is unrelated to prefixes, it's about different viewport sizes.
> Maybe you want to add a line about that in the patch description.
> Or do this particular change in a different patch.

Umm, this change could indeed be done in a different patch, I agree.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:67:
<div class="container">
On 2015/12/02 09:31:01, Manuel Rego wrote:
> I think you don't need the container here.

umm, I don't agree. the idea of this patch is to verify the shrink-to-fit
behavior, hence we would need to have a container reference. If we don't set it
here, we will us the viewport. I think all the test cases must be identical,
just changing the positioning scheme.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html:80:
<div class="grid fit-content fitContentOnlyWidth columnsSmallerThanContentBox
rowsBiggerThanMarginBox">
On 2015/12/02 09:31:02, Manuel Rego wrote:
> This was not using "-webkit-fit-content" before, why we need to change it?
> 

Yes, it was actually. The fitContentWidth class was defined to set 
-webkit-fit-content for the "width" property only.

> Probably the classes "fitContentHeight|Width" should be renamed,
> as they seem to be only setting start alignment.

The problem is that we use fit-content for both width and height in this tests,
but we need either width or height being controlled. We need to set auto value
for the uncontrolled property and that's what  itContentOnlyWidth actually does.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html:29:
height: auto;
On 2015/12/02 09:31:02, Manuel Rego wrote:
> height is "auto" by default, I don't think you need this class.

Yes, we do need it. The issue is when we setup fit-content for both width and
height, but needing the default value for some specific cases. There is no way
to do it with the current classes defined in the css-sizing style sheet.

[Manuel Rego, 2015-12-04 13:12:43]

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-shrink-to-fit.html:67:
<div class="container">
On 2015/12/03 15:06:11, jfernandez wrote:
> On 2015/12/02 09:31:01, Manuel Rego wrote:
> > I think you don't need the container here.
> 
> umm, I don't agree. the idea of this patch is to verify the shrink-to-fit
> behavior, hence we would need to have a container reference. If we don't set
it
> here, we will us the viewport. I think all the test cases must be identical,
> just changing the positioning scheme. 

Fair enough.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html:80:
<div class="grid fit-content fitContentOnlyWidth columnsSmallerThanContentBox
rowsBiggerThanMarginBox">
On 2015/12/03 15:06:11, jfernandez wrote:
> On 2015/12/02 09:31:02, Manuel Rego wrote:
> > This was not using "-webkit-fit-content" before, why we need to change it?

Sorry but I don't see it. It was only:
.fitContentWidth { justify-items: start; }

This test was not using at all "-webkit-fit-content",
only that the class name was very confusing.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html:29:
height: auto;
On 2015/12/03 15:06:11, jfernandez wrote:
> On 2015/12/02 09:31:02, Manuel Rego wrote:
> > height is "auto" by default, I don't think you need this class.
> 
> Yes, we do need it. The issue is when we setup fit-content for both width and
> height, but needing the default value for some specific cases. There is no way
> to do it with the current classes defined in the css-sizing style sheet. 

True as you're using both
width-keyword-classes.css and height-keyword-classes.css.

[svillar, 2015-12-04 13:18:31]

lgtm. Awesome job.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/calc-resolution-grid-item.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/calc-resolution-grid-item.html:28:
height: calc(70% + 30%);
I don't see any refactoring here.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-column-repaint.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-column-repaint.html:30:
<div class="grid fit-content">
It was min-content

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html:80:
<div class="grid fit-content fitContentOnlyWidth columnsSmallerThanContentBox
rowsBiggerThanMarginBox">
On 2015/12/03 15:06:11, jfernandez wrote:
> On 2015/12/02 09:31:02, Manuel Rego wrote:
> > This was not using "-webkit-fit-content" before, why we need to change it?
> > 
> 
> Yes, it was actually. The fitContentWidth class was defined to set 
> -webkit-fit-content for the "width" property only.
> 
> > Probably the classes "fitContentHeight|Width" should be renamed,
> > as they seem to be only setting start alignment.
> 
> The problem is that we use fit-content for both width and height in this
tests,
> but we need either width or height being controlled. We need to set auto value
> for the uncontrolled property and that's what  itContentOnlyWidth actually
does.

I also find it terribly confusing. It's pretty weird that something called
fitContentWidth was used to set justify-items.

I'd explicity use style="height:auto" or style="width:auto" whenever needed. I'd
also rename the fitContentX to justifyItemsX or even better use the already
defined classes in grid-align.css if possible.

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
(right):

https://codereview.chromium.org/1488493003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html:29:
height: auto;
On 2015/12/03 15:06:11, jfernandez wrote:
> On 2015/12/02 09:31:02, Manuel Rego wrote:
> > height is "auto" by default, I don't think you need this class.
> 
> Yes, we do need it. The issue is when we setup fit-content for both width and
> height, but needing the default value for some specific cases. There is no way
> to do it with the current classes defined in the css-sizing style sheet. 

In this case I also think is better to explicitly use style="height:auto"
because indefiniteHeight is confusing as fit-content is already an indefinite
height. Do we really need it to be auto?

[svillar, 2016-01-19 12:16:14]

What happened with this?

[jfernandez, 2016-01-19 12:39:52]

On 2016/01/19 12:16:14, svillar wrote:
> What happened with this?

I forgot to land it, so I'll rebase the patch and probably applying similar
changes to the new tests cases or files. If there are important changes, Ill
request a new review.

[jfernandez, 2016-01-21 22:59:05]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-01-21 22:59:05]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com,
rego@igalia.com
Link to the patchset: https://codereview.chromium.org/1488493003/#ps20001
(title: "Patch rebased and refactoring of alignment css rules.")

[commit-bot: I haz the power, 2016-01-21 22:59:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488493003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488493003/20001

[commit-bot: I haz the power, 2016-01-22 00:07:49]

Description was changed from

==========
[css-grid] refactoring of layout tests

In order to manage the css values and properties prefixes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.
==========

to

==========
[css-grid] refactoring of layout tests

In order to manage the css values and properties prefixes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.
==========

[commit-bot: I haz the power, 2016-01-22 00:07:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-22 00:08:54]

Description was changed from

==========
[css-grid] refactoring of layout tests

In order to manage the css values and properties prefixes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.
==========

to

==========
[css-grid] refactoring of layout tests

In order to manage the css values and properties prefixes using by the
different browsers, I've made several changes to reuse some css keywords
and style files already defined in other folders.

No need for additional tests since there is no functional change.

Committed: https://crrev.com/1314b8fc0bc46b2e7f05095ecd35d34147b20bed
Cr-Commit-Position: refs/heads/master@{#370840}
==========

[commit-bot: I haz the power, 2016-01-22 00:08:55]

Patchset 2 (id:??) landed as
https://crrev.com/1314b8fc0bc46b2e7f05095ecd35d34147b20bed
Cr-Commit-Position: refs/heads/master@{#370840}