[css-grid] Clearing the override height before layout

[css-grid] Clearing the override height before layout

The stretching logic sets an override height to the grid item so it's
used instead of the regular one, computed in the LayoutBox class. This
override height is computed using the 'grid area' abstraction as item's
containing block.

It's expectable that a style update where item's Self Alginment CSS
properties change from 'stretch' to any other value implies recomputing
the item's size.

The currently implemented stretching logic clears the override flags
before determining whether to assign an stretched override height. We
expect that the layout performed later will adjust the grid item's
height.

However, if the grid item has been laid out already it doesn't adjust
its height, even though the override has been cleared already. This is
precisely the situation detected in the bug, because the Baseline
Alignment logic performs a layout of the grid item before computing the
baseline offsets. This layout causes that the grid item's size is not
computed again after clearing the override flag.

There may be other cases like the one detected in the bug, so with this
patch we are clearing the override flag at the beginning of the layout.
This is a change we wanted to do ling time ago, since the override flags
should be more consistent and not affecting intermediate operations,
even repeating the layout operation.


BUG=709902
Review-Url: https://codereview.chromium.org/2832783003
Cr-Commit-Position: refs/heads/master@{#468672}
Committed: https://chromium.googlesource.com/chromium/src/+/00833a5660df5bddeb6dabc8a2dfe9fbb94662ff

[jfernandez, 2017-04-20 23:44:10]

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

[jfernandez, 2017-04-20 23:44:10]

Patch sent out for review.

[Manuel Rego, 2017-04-21 05:21:11]

Don't we miss a test in this CL?

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

https://codereview.chromium.org/2832783003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:238: // computed again in
the updateLogicalWidth call bellow.
This comment needs to be updated now.

[svillar, 2017-04-21 07:45:06]

Wouldn't we be forcing a layout of every single grid item by clearing the
overrides unconditionally?

[jfernandez, 2017-04-21 08:31:16]

On 2017/04/21 07:45:06, svillar wrote:
> Wouldn't we be forcing a layout of every single grid item by clearing the
> overrides unconditionally?

I don't see why clearing the overrides will imply forcing a layout. It just
clear it so that IF item's need to be laid out again, it doesn't use an outdated
override size. If the items need a layout or not is defined by the Grid layout
logic.

[jfernandez, 2017-04-21 08:31:47]

Yes, I forgot to add it to the commit, but I've got it already. I'll provide the
test in the next patch.

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

https://codereview.chromium.org/2832783003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:238: // computed again in
the updateLogicalWidth call bellow.
On 2017/04/21 05:21:11, Manuel Rego wrote:
> This comment needs to be updated now.

Well, the comment still applies, IMO. It's just that we clear the item's
overrideSize in any case. However, in the case of content-sized
grid container we still need to cleare the containingBlockOverride size and
force a layout, as the comment states.

[Manuel Rego, 2017-04-21 08:51:51]

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

https://codereview.chromium.org/2832783003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:238: // computed again in
the updateLogicalWidth call bellow.
On 2017/04/21 08:31:47, jfernandez wrote:
> On 2017/04/21 05:21:11, Manuel Rego wrote:
> > This comment needs to be updated now.
> 
> Well, the comment still applies, IMO. It's just that we clear the item's
> overrideSize in any case. However, in the case of content-sized
> grid container we still need to cleare the containingBlockOverride size and
> force a layout, as the comment states.

But the comment says:
// We need to clear both **own** and containingBlock override sizes

Before we clear both if that condition applies, now we clear it always.
That's why I say the comment needs to be updated.

[Manuel Rego, 2017-04-21 10:45:46]

Some comments on the test.

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html
(right):

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:15:
<p>Test to verify that grid item's stretched size is reset when changing to a
different alignment value.</p>
Could you make this test under LayoutTests/external/wpt/css/css-grid-1/?
And we'll test the auto-sync stuff with WPT. :-)

You'll just need to add a <title> and <meta author>
and use this paragraph as <meta assert>.
Also the usual <meta utf-8> stuff.

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:21:
values = ['self-start', 'self-end', 'start', 'end', 'center', 'right', 'left',
'baseline'];
Nit: Use "var" or even "const" to define "values".

Also in theory 'use strict'; is recommended too:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t...

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:26:
assert_equals(child.offsetWidth, 100, "The width is not what it should.");
I'll change the string to something like:
"Check item's width after setting 'stretch' alignment."

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:27:
assert_equals(child.offsetHeight, 200, "The height is not what it should. " +
value);
Ditto.

Also remove the "+ value" at the end.

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:33:
assert_equals(child.offsetWidth, 50, "The width is not what it should.");
And here something like:
"Check item's width after setting '" + value + "' alignment."

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:34:
assert_equals(child.offsetHeight, 10, "The height is not what it should. " +
value);
Ditto.

[jfernandez, 2017-04-21 11:15:33]

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html
(right):

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:15:
<p>Test to verify that grid item's stretched size is reset when changing to a
different alignment value.</p>
On 2017/04/21 10:45:46, Manuel Rego wrote:
> Could you make this test under LayoutTests/external/wpt/css/css-grid-1/?
> And we'll test the auto-sync stuff with WPT. :-)
> 
> You'll just need to add a <title> and <meta author>
> and use this paragraph as <meta assert>.
> Also the usual <meta utf-8> stuff.

Yes, I'll do it.

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:21:
values = ['self-start', 'self-end', 'start', 'end', 'center', 'right', 'left',
'baseline'];
On 2017/04/21 10:45:46, Manuel Rego wrote:
> Nit: Use "var" or even "const" to define "values".
> 
> Also in theory 'use strict'; is recommended too:
>
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t...

Acknowledged.

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:26:
assert_equals(child.offsetWidth, 100, "The width is not what it should.");
On 2017/04/21 10:45:46, Manuel Rego wrote:
> I'll change the string to something like:
> "Check item's width after setting 'stretch' alignment."

This assert message is only shown when it fails. The test id 
is defined below, as "Checking stretched size before changing to " + value)"

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:27:
assert_equals(child.offsetHeight, 200, "The height is not what it should. " +
value);
On 2017/04/21 10:45:46, Manuel Rego wrote:
>
> Also remove the "+ value" at the end.

This is a mistake, thanks for finding it.

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:33:
assert_equals(child.offsetWidth, 50, "The width is not what it should.");
On 2017/04/21 10:45:46, Manuel Rego wrote:
> And here something like:
> "Check item's width after setting '" + value + "' alignment."

Already commented about it above.

https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:34:
assert_equals(child.offsetHeight, 10, "The height is not what it should. " +
value);
On 2017/04/21 10:45:46, Manuel Rego wrote:
> Ditto.

Already commented about it above.

[jfernandez, 2017-04-24 10:33:44]

Patchset #3 (id:40001) has been deleted

[jfernandez, 2017-04-24 10:34:11]

Please reviewers, take another look.

[Manuel Rego, 2017-04-28 09:51:16]

lgtm

https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:238: // interfere in
current layout execution.
Thanks for the new comment!

Nit: the two "so" sound weird to me.

[svillar, 2017-04-28 15:33:31]

https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:251: child->ForceLayout();
I know this forced layout was already there in order to mimic the pre-layout
done in FrameView. The thing is that we no longer to that for grid items, so
perhaps it'd be enough to call SetNeedsLayout() but we can do it in a separate
patch.

[jfernandez, 2017-05-02 14:17:33]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2017-05-02 14:17:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com
Link to the patchset: https://codereview.chromium.org/2832783003/#ps120001
(title: "Patch rebased.")

[commit-bot: I haz the power, 2017-05-02 14:18:04]

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-05-02 16:27:04]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493734653411330,
"parent_rev": "bba3b46df2d77ad3718ec13a5aa48865580a1fc4", "commit_rev":
"00833a5660df5bddeb6dabc8a2dfe9fbb94662ff"}

[commit-bot: I haz the power, 2017-05-02 16:27:16]

Description was changed from

==========
[css-grid] Clearing the override height before layout

The stretching logic sets an override height to the grid item so it's
used instead of the regular one, computed in the LayoutBox class. This
override height is computed using the 'grid area' abstraction as item's
containing block.

It's expectable that a style update where item's Self Alginment CSS
properties change from 'stretch' to any other value implies recomputing
the item's size.

The currently implemented stretching logic clears the override flags
before determining whether to assign an stretched override height. We
expect that the layout performed later will adjust the grid item's
height.

However, if the grid item has been laid out already it doesn't adjust
its height, even though the override has been cleared already. This is
precisely the situation detected in the bug, because the Baseline
Alignment logic performs a layout of the grid item before computing the
baseline offsets. This layout causes that the grid item's size is not
computed again after clearing the override flag.

There may be other cases like the one detected in the bug, so with this
patch we are clearing the override flag at the beginning of the layout.
This is a change we wanted to do ling time ago, since the override flags
should be more consistent and not affecting intermediate operations,
even repeating the layout operation.


BUG=709902
==========

to

==========
[css-grid] Clearing the override height before layout

The stretching logic sets an override height to the grid item so it's
used instead of the regular one, computed in the LayoutBox class. This
override height is computed using the 'grid area' abstraction as item's
containing block.

It's expectable that a style update where item's Self Alginment CSS
properties change from 'stretch' to any other value implies recomputing
the item's size.

The currently implemented stretching logic clears the override flags
before determining whether to assign an stretched override height. We
expect that the layout performed later will adjust the grid item's
height.

However, if the grid item has been laid out already it doesn't adjust
its height, even though the override has been cleared already. This is
precisely the situation detected in the bug, because the Baseline
Alignment logic performs a layout of the grid item before computing the
baseline offsets. This layout causes that the grid item's size is not
computed again after clearing the override flag.

There may be other cases like the one detected in the bug, so with this
patch we are clearing the override flag at the beginning of the layout.
This is a change we wanted to do ling time ago, since the override flags
should be more consistent and not affecting intermediate operations,
even repeating the layout operation.


BUG=709902

Review-Url: https://codereview.chromium.org/2832783003
Cr-Commit-Position: refs/heads/master@{#468672}
Committed:
https://chromium.googlesource.com/chromium/src/+/00833a5660df5bddeb6dabc8a2df...
==========

[commit-bot: I haz the power, 2017-05-02 16:27:18]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/00833a5660df5bddeb6dabc8a2df...