[css-grid] Fix simplified layout of positioned grid items

[css-grid] Fix simplified layout of positioned grid items

When a simplifiedLayout() is performed the method, for example
when a positioned item is moved, LayoutGrid::layoutPositionedObjects()
was not called. So the positioned items were not properly painted.
To fix this we're properly overriding layoutPositionedObjects().

However we've to be careful and protect simplifiedLayout()
so we don't perform it when we've positioned items and a dirty grid,
otherwise we'll have troubles as layoutPositionedObjects() uses m_grid.

On top of that, a new condition has been added into
LayoutBox::updateGridPositionAfterStyleChange(), as for positioned items
we don't need to mark the grid as dirty if they change the position.

BUG=662049
TEST=fast/css-grid-layout/grid-positioned-item-dynamic-change-expected.html

Committed: https://crrev.com/3c5effd4207aa48645264de4eaf3bca6c222ce71
Cr-Commit-Position: refs/heads/master@{#430401}

[Manuel Rego, 2016-11-03 16:47:53]

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

[cbiesinger, 2016-11-03 18:55:05]

I'm confused -- why does simplifiedLayout get called? The caller is in
LayoutBlockFlow, but grid inherits from LayoutBlock...?

[Manuel Rego, 2016-11-03 20:16:55]

On 2016/11/03 18:55:05, cbiesinger wrote:
> I'm confused -- why does simplifiedLayout get called? The caller is in
> LayoutBlockFlow, but grid inherits from LayoutBlock...?

It's called at the beginning of LayoutGrid::layoutBlock() method:
https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKi...

Actually I can avid call it in this case (it has positioned items but it's
dirty),
but I thought it would be better to have a generic way just in case it gets
called from somewhere else (maybe it's not needed).

[cbiesinger, 2016-11-03 20:25:49]

On 2016/11/03 20:16:55, Manuel Rego wrote:
> On 2016/11/03 18:55:05, cbiesinger wrote:
> > I'm confused -- why does simplifiedLayout get called? The caller is in
> > LayoutBlockFlow, but grid inherits from LayoutBlock...?
> 
> It's called at the beginning of LayoutGrid::layoutBlock() method:
>
https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKi...
> 
> Actually I can avid call it in this case (it has positioned items but it's
> dirty),
> but I thought it would be better to have a generic way just in case it gets
> called from somewhere else (maybe it's not needed).

Sorry, I missed that. But even so, simplifiedLayout shouldn't have to be
virtual. It shouldn't be called from anywhere else, it is too much of an
implementation detail.

[mstensho (USE GERRIT), 2016-11-04 08:49:12]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-11-04 08:49:13]

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

https://codereview.chromium.org/2471283004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.h:361: virtual void
layoutPositionedObjects(
This shouldn't be necessary.

https://codereview.chromium.org/2471283004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.h:425: virtual bool
simplifiedLayout();
Ditto.

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

https://codereview.chromium.org/2471283004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:409: if
(oldStyle->hasOutOfFlowPosition() == style()->hasOutOfFlowPosition() &&
I.e. oldStyle->hasOutOfFlowPosition() && style()->hasOutOfFlowPosition()

[Manuel Rego, 2016-11-04 09:29:17]

Thanks for the reviews! I'm uploading a new version of the patch.

On 2016/11/03 20:25:49, cbiesinger wrote:
> Sorry, I missed that. But even so, simplifiedLayout shouldn't have to be
> virtual. It shouldn't be called from anywhere else, it is too much of an
> implementation detail.

Ok, I've just protected the call to simplifiedLayout() on
LayoutGrid::layoutBlock() to avoid issues in the case of a dirty grid.

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

https://codereview.chromium.org/2471283004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.h:361: virtual void
layoutPositionedObjects(
On 2016/11/04 08:49:12, mstensho wrote:
> This shouldn't be necessary.

Mmmm, we need that LayoutGrid::layoutPositionedObjects() is called
from LayoutBlock::simplifiedLayout().
If we don't mark the function as virtual, this won't happen, only
LayoutBlock::layoutPositionedObjects() will be called
(and the positioned objects won't be properly placed inside the grid).

Am I missing something?

https://codereview.chromium.org/2471283004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.h:425: virtual bool
simplifiedLayout();
On 2016/11/04 08:49:12, mstensho wrote:
> Ditto.

True for this case, actually as @cbiesinger suggested
I'm no longer overriding this function in the new version of the patch.

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

https://codereview.chromium.org/2471283004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:409: if
(oldStyle->hasOutOfFlowPosition() == style()->hasOutOfFlowPosition() &&
On 2016/11/04 08:49:12, mstensho wrote:
> I.e. oldStyle->hasOutOfFlowPosition() && style()->hasOutOfFlowPosition()

Acknowledged.

[mstensho (USE GERRIT), 2016-11-04 09:47:34]

Don't know if you want to wait for cbiesinger or others, but this lgtm now.

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

https://codereview.chromium.org/2471283004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.h:361: virtual void
layoutPositionedObjects(
On 2016/11/04 09:29:17, Manuel Rego wrote:
> On 2016/11/04 08:49:12, mstensho wrote:
> > This shouldn't be necessary.
> 
> Mmmm, we need that LayoutGrid::layoutPositionedObjects() is called
> from LayoutBlock::simplifiedLayout().
> If we don't mark the function as virtual, this won't happen, only
> LayoutBlock::layoutPositionedObjects() will be called
> (and the positioned objects won't be properly placed inside the grid).
> 
> Am I missing something?

Duh, no. *I* was missing something. Sorry. This needs to remain virtual.

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

https://codereview.chromium.org/2471283004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:479: // has positioned
items to be layout.
"to be laid out"

https://codereview.chromium.org/2471283004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:480: if (!relayoutChildren
&& !(m_gridIsDirty && posChildNeedsLayout()) &&
Nit: Augustus de Morgan might write that middle expression as (!m_gridIsDirty ||
!posChildNeedsLayout()), and so would I. :)

[Manuel Rego, 2016-11-04 09:57:56]

On 2016/11/04 09:47:34, mstensho wrote:
> Don't know if you want to wait for cbiesinger or others, but this lgtm now.

Thanks! Uploaded new version fixing the nits.

BTW, I can wait for him as we're not in a hurry.

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

https://codereview.chromium.org/2471283004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:479: // has positioned
items to be layout.
On 2016/11/04 09:47:34, mstensho wrote:
> "to be laid out"

Acknowledged.

https://codereview.chromium.org/2471283004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:480: if (!relayoutChildren
&& !(m_gridIsDirty && posChildNeedsLayout()) &&
On 2016/11/04 09:47:34, mstensho wrote:
> Nit: Augustus de Morgan might write that middle expression as (!m_gridIsDirty
||
> !posChildNeedsLayout()), and so would I. :)

Done.

[cbiesinger, 2016-11-07 21:25:17]

lgtm

[Manuel Rego, 2016-11-07 21:28:51]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-11-07 21:28:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2471283004/#ps60001
(title: "Remove unneeded line in test.")

[commit-bot: I haz the power, 2016-11-07 21:29:28]

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, 2016-11-07 22:36:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-07 22:40:19]

Description was changed from

==========
[css-grid] Fix simplified layout of positioned grid items

When a simplifiedLayout() is performed the method, for example
when a positioned item is moved, LayoutGrid::layoutPositionedObjects()
was not called. So the positioned items were not properly painted.
To fix this we're properly overriding layoutPositionedObjects().

However we've to be careful and protect simplifiedLayout()
so we don't perform it when we've positioned items and a dirty grid,
otherwise we'll have troubles as layoutPositionedObjects() uses m_grid.

On top of that, a new condition has been added into
LayoutBox::updateGridPositionAfterStyleChange(), as for positioned items
we don't need to mark the grid as dirty if they change the position.

BUG=662049
TEST=fast/css-grid-layout/grid-positioned-item-dynamic-change-expected.html
==========

to

==========
[css-grid] Fix simplified layout of positioned grid items

When a simplifiedLayout() is performed the method, for example
when a positioned item is moved, LayoutGrid::layoutPositionedObjects()
was not called. So the positioned items were not properly painted.
To fix this we're properly overriding layoutPositionedObjects().

However we've to be careful and protect simplifiedLayout()
so we don't perform it when we've positioned items and a dirty grid,
otherwise we'll have troubles as layoutPositionedObjects() uses m_grid.

On top of that, a new condition has been added into
LayoutBox::updateGridPositionAfterStyleChange(), as for positioned items
we don't need to mark the grid as dirty if they change the position.

BUG=662049
TEST=fast/css-grid-layout/grid-positioned-item-dynamic-change-expected.html

Committed: https://crrev.com/3c5effd4207aa48645264de4eaf3bca6c222ce71
Cr-Commit-Position: refs/heads/master@{#430401}
==========

[commit-bot: I haz the power, 2016-11-07 22:40:20]

Patchset 4 (id:??) landed as
https://crrev.com/3c5effd4207aa48645264de4eaf3bca6c222ce71
Cr-Commit-Position: refs/heads/master@{#430401}