[css-grid] "normal" alignment is "start" for replaced elements

[css-grid] "normal" alignment is "start" for replaced elements

"normal" alignment used to be "stretch" for all the elements,
however the CSS Grid Layout spec has been updated to change
this behavior for replaced elements:
https://github.com/w3c/csswg-drafts/issues/523

A few tests need to be updated due to this change.
We also take advantage to make
fast/css-grid-layout/grid-align-stretching-replaced-items.html
a testharness.js test so we can get rid of the expected file.

BUG=666961
TEST=external/csswg-test/css-grid-1/grid-items/grid-items-sizing-alignment-001.html

Review-Url: https://codereview.chromium.org/2722613003
Cr-Commit-Position: refs/heads/master@{#454133}
Committed: https://chromium.googlesource.com/chromium/src/+/fad39c71033eea490d8437ca186529c8e87125c2

[Manuel Rego, 2017-02-28 10:14:37]

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

[Manuel Rego, 2017-02-28 10:14:37]

The new test has been sent directly to csswg-test:
https://github.com/w3c/csswg-test/pull/1219

It should be reviewed there and then imported
before merging this change.

[Manuel Rego, 2017-02-28 10:15:47]

Description was changed from

==========
[css-grid] "normal" alignment is "start" for replaced elements

"normal" alignment used to be "stretch" for all the elements,
however the CSS Grid Layout spec has been updated to change
this behavior for replaced elements:
https://github.com/w3c/csswg-drafts/issues/523

A few tests need to be updated due to this change.
We also take advantage to make
fast/css-grid-layout/grid-align-stretching-replaced-items.html
a testharness.js test so we can get rid of the expected file.

BUG=555027
TEST=external/csswg-test/css-grid-1/grid-items/grid-items-sizing-alignment-001.html
==========

to

==========
[css-grid] "normal" alignment is "start" for replaced elements

"normal" alignment used to be "stretch" for all the elements,
however the CSS Grid Layout spec has been updated to change
this behavior for replaced elements:
https://github.com/w3c/csswg-drafts/issues/523

A few tests need to be updated due to this change.
We also take advantage to make
fast/css-grid-layout/grid-align-stretching-replaced-items.html
a testharness.js test so we can get rid of the expected file.

BUG=666961
TEST=external/csswg-test/css-grid-1/grid-items/grid-items-sizing-alignment-001.html
==========

[jfernandez, 2017-03-01 12:46:49]

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-align-stretching-replaced-items.html
(right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-align-stretching-replaced-items.html:28:
<div class="grid">
This case is based on the fact that 'normal' is the default value, but I think
it'd be better to set explicitly 'normal' as alignment value. 

It'd be even better to have both cases, setting 'normal' as defailt and explicit
value.

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2755:
.resolvedAlignSelf(cb->selfAlignmentNormalBehavior(this),
Try to avoid the new 'child' argument.

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:1386: if (child &&
child->isLayoutReplaced())
I think this logic is grid specific, so it should be implemented in Grid, so
other LayoutBox still use stretch as 'normal' behavior.

Also, I don't like too much the new 'child' argument. I've got a proposal; since
this is a grid specific logic, I'd remove this changes in LayoutBox and
implement the different behavior for replaced grid items as part of
LayoutGrid::alignSelfForChild and LayoutGrid::justifySelfForChild 
methods.

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutFullScreen.h (right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutFullScreen.h:71: const LayoutBox*
child = nullptr) const override {
Ditto.

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1421: if
(!child.isAnonymous()) {
Perhaps it'd be a better idea to solve directly the normal behavior instead of
using selfAlignmentNormalBehavior().

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1434: if
(!child.isAnonymous()) {
ditto.

[Manuel Rego, 2017-03-01 17:57:07]

Thanks for the review, uploaded a new version PTAL.

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-align-stretching-replaced-items.html
(right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-align-stretching-replaced-items.html:28:
<div class="grid">
On 2017/03/01 12:46:49, jfernandez wrote:
> This case is based on the fact that 'normal' is the default value, but I think
> it'd be better to set explicitly 'normal' as alignment value. 
> 
> It'd be even better to have both cases, setting 'normal' as defailt and
explicit
> value.

Done.

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2755:
.resolvedAlignSelf(cb->selfAlignmentNormalBehavior(this),
On 2017/03/01 12:46:49, jfernandez wrote:
> Try to avoid the new 'child' argument.

We need to pass the child in the case of "cb"
is a grid container.

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2722613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:1386: if (child &&
child->isLayoutReplaced())
On 2017/03/01 12:46:49, jfernandez wrote:
> I think this logic is grid specific, so it should be implemented in Grid, so
> other LayoutBox still use stretch as 'normal' behavior.
> 
> Also, I don't like too much the new 'child' argument. I've got a proposal;
since
> this is a grid specific logic, I'd remove this changes in LayoutBox and
> implement the different behavior for replaced grid items as part of
> LayoutGrid::alignSelfForChild and LayoutGrid::justifySelfForChild 
> methods. 

Yeah, good point, it's grid layout specific so I've overridden
this method in LayoutGrid.

The problem is that we still need to have the "child" argument,
as we need to pass the child in LayoutBox::hasStretchedLogicalWidth()
if it's a Grid.

[jfernandez, 2017-03-01 22:58:54]

lgtm

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid-alignment.css
(right):

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid-alignment.css:96:
.itemsStretch {
I guess this should be itemsNormal, shouldn't it ?

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1386: DCHECK(!child);
I'm not sure about this DCHECK; I couldn't say it's incorrect to pass a non-null
pointer as 'child' argument, but useless.

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFullScreen.h (right):

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFullScreen.h:72: DCHECK(!child);
ditto.

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.h:103: if
(child->isLayoutReplaced())
Just a suggestion; wouldn't be these lines the best candidates for a ternary
expression ? 

  return child->isLayoutReplaced() ? ItemPositionStart 
                                   : ItemPositionStretch;

[Manuel Rego, 2017-03-01 23:23:30]

Thank you very much for the review!

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid-alignment.css
(right):

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid-alignment.css:96:
.itemsStretch {
On 2017/03/01 22:58:53, jfernandez wrote:
> I guess this should be itemsNormal, shouldn't it ? 

Oops, true. :-)

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1386: DCHECK(!child);
On 2017/03/01 22:58:53, jfernandez wrote:
> I'm not sure about this DCHECK; I couldn't say it's incorrect to pass a
non-null
> pointer as 'child' argument, but useless.

At this moment we shouldn't pass any child to them,
at least it's not useful. So I prefer an ASSERT here
so we realize ASAP if we're passing a child by mistake.

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/2722613003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.h:103: if
(child->isLayoutReplaced())
On 2017/03/01 22:58:53, jfernandez wrote:
> Just a suggestion; wouldn't be these lines the best candidates for a ternary
> expression ? 
> 
>   return child->isLayoutReplaced() ? ItemPositionStart 
>                                    : ItemPositionStretch;

Indeed.

[Manuel Rego, 2017-03-01 23:23:42]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2017-03-01 23:23:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/2722613003/#ps60001
(title: "Applied suggested changes")

[commit-bot: I haz the power, 2017-03-01 23:24:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 01:19:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488410622028210,
"parent_rev": "880895ed7e23b1d0b920d5c62c361a226a62c50e", "commit_rev":
"fad39c71033eea490d8437ca186529c8e87125c2"}

[commit-bot: I haz the power, 2017-03-02 01:20:55]

Description was changed from

==========
[css-grid] "normal" alignment is "start" for replaced elements

"normal" alignment used to be "stretch" for all the elements,
however the CSS Grid Layout spec has been updated to change
this behavior for replaced elements:
https://github.com/w3c/csswg-drafts/issues/523

A few tests need to be updated due to this change.
We also take advantage to make
fast/css-grid-layout/grid-align-stretching-replaced-items.html
a testharness.js test so we can get rid of the expected file.

BUG=666961
TEST=external/csswg-test/css-grid-1/grid-items/grid-items-sizing-alignment-001.html
==========

to

==========
[css-grid] "normal" alignment is "start" for replaced elements

"normal" alignment used to be "stretch" for all the elements,
however the CSS Grid Layout spec has been updated to change
this behavior for replaced elements:
https://github.com/w3c/csswg-drafts/issues/523

A few tests need to be updated due to this change.
We also take advantage to make
fast/css-grid-layout/grid-align-stretching-replaced-items.html
a testharness.js test so we can get rid of the expected file.

BUG=666961
TEST=external/csswg-test/css-grid-1/grid-items/grid-items-sizing-alignment-001.html

Review-Url: https://codereview.chromium.org/2722613003
Cr-Commit-Position: refs/heads/master@{#454133}
Committed:
https://chromium.googlesource.com/chromium/src/+/fad39c71033eea490d8437ca1865...
==========

[commit-bot: I haz the power, 2017-03-02 01:20:56]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fad39c71033eea490d8437ca1865...