[CSS Grid Layout] Grid items must set a new formatting context.

[CSS Grid Layout] Grid items must set a new formatting context.

According to the spec, "A grid item establishes a new formatting context
for its contents." Hence, margins should not collapse even when they may
be adjoining to its content's margins. It also prevents any 'float' protruding
content on the adjoining grid items.

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

[jfernandez, 2014-11-08 01:08:45]

jfernandez@igalia.com changed reviewers:
+ esprehn@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Manuel Rego, 2014-11-11 13:54:22]

https://codereview.chromium.org/697653004/diff/1/LayoutTests/fast/css-grid-la...
File LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse.html
(right):

https://codereview.chromium.org/697653004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse.html:3: 
Probably it would be nice to add some kind of description
text to the test.

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1525: || isDocumentElement() ||
style()->columnSpan() || containingBlock()->isRenderGrid();
It think that you could define something similar to:
RenderBox::isFlexItemIncludingDeprecated()

It might be useful in other places in the future.

[Julien - ping for review, 2014-11-11 16:27:01]

lgtm with a description in the test (kudos to rego for catching that).

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1521: bool
RenderBlock::createsBlockFormattingContext() const
This function is deeply misnamed as most elements listed in it don't establish
*block* formatting contexts. Let's rename it to createNewFormattingContext for
clarity.

That would also help explain that we can have other impact from this change
(like float not protruding into the next grid item, which we should test btw).

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1525: || isDocumentElement() ||
style()->columnSpan() || containingBlock()->isRenderGrid();
On 2014/11/11 at 13:54:22, Manuel Rego wrote:
> It think that you could define something similar to:
> RenderBox::isFlexItemIncludingDeprecated()
> 
> It might be useful in other places in the future.

I don't have a preference about adding a helper function.

However I think we will want to use parent() instead of containingBlock() as
positioned elements create a new formatting context anyway. Also parent() is a
lot cheaper.

[Julien - ping for review, 2014-11-11 16:27:52]

https://codereview.chromium.org/697653004/diff/1/LayoutTests/fast/css-grid-la...
File LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse.html
(right):

https://codereview.chromium.org/697653004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse.html:3: 
On 2014/11/11 at 13:54:22, Manuel Rego wrote:
> Probably it would be nice to add some kind of description
> text to the test.

And a description of the expected output (what is the condition for passing /
failing?)

[jfernandez, 2014-11-12 13:22:45]

I've applied all the suggested changes, so I'll try the CQ now.

https://codereview.chromium.org/697653004/diff/1/LayoutTests/fast/css-grid-la...
File LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse.html
(right):

https://codereview.chromium.org/697653004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse.html:3: 
On 2014/11/11 13:54:22, Manuel Rego wrote:
> Probably it would be nice to add some kind of description
> text to the test.

Done.

https://codereview.chromium.org/697653004/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse.html:3: 
On 2014/11/11 16:27:52, Julien Chaffraix - PST wrote:
> On 2014/11/11 at 13:54:22, Manuel Rego wrote:
> > Probably it would be nice to add some kind of description
> > text to the test.
> 
> And a description of the expected output (what is the condition for passing /
> failing?)

Done.

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1521: bool
RenderBlock::createsBlockFormattingContext() const
On 2014/11/11 16:27:01, Julien Chaffraix - PST wrote:
> This function is deeply misnamed as most elements listed in it don't establish
> *block* formatting contexts. Let's rename it to createNewFormattingContext for
> clarity.
> 

Done.

> That would also help explain that we can have other impact from this change
> (like float not protruding into the next grid item, which we should test btw).

Added a new layout tests for that issue.

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1525: || isDocumentElement() ||
style()->columnSpan() || containingBlock()->isRenderGrid();
On 2014/11/11 16:27:01, Julien Chaffraix - PST wrote:
> On 2014/11/11 at 13:54:22, Manuel Rego wrote:
> > It think that you could define something similar to:
> > RenderBox::isFlexItemIncludingDeprecated()
> > 
> > It might be useful in other places in the future.
> 
> I don't have a preference about adding a helper function.
> 
> However I think we will want to use parent() instead of containingBlock() as
> positioned elements create a new formatting context anyway. Also parent() is a
> lot cheaper.

Done.

https://codereview.chromium.org/697653004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1525: || isDocumentElement() ||
style()->columnSpan() || containingBlock()->isRenderGrid();
On 2014/11/11 13:54:22, Manuel Rego wrote:
> It think that you could define something similar to:
> RenderBox::isFlexItemIncludingDeprecated()
> 
> It might be useful in other places in the future.

Done.

[jfernandez, 2014-11-12 13:23:15]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-11-12 13:23:41]

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

[jfernandez, 2014-11-12 14:06:49]

The CQ bit was unchecked by jfernandez@igalia.com

[Manuel Rego, 2014-11-12 14:11:53]

Just a few nits before it lands :-)

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html
(right):

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:24:
.floatChild {
This is not used in the this file, so it should be removed.

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html
(right):

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:23:
<div>This test checks that grid item sets a new formatting context for its
content, preventing any 'float' protruding content on the adjoining grid
item.</div>
This is ok as description but I think you're missing the explanation
of the expected output as Julien said.
For example: Float text doesn't enter in the second row.

[jfernandez, 2014-11-12 14:23:40]

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html
(right):

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:24:
.floatChild {
On 2014/11/12 14:11:53, Manuel Rego wrote:
> This is not used in the this file, so it should be removed.

Done.

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html
(right):

https://codereview.chromium.org/697653004/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:23:
<div>This test checks that grid item sets a new formatting context for its
content, preventing any 'float' protruding content on the adjoining grid
item.</div>
On 2014/11/12 14:11:53, Manuel Rego wrote:
> This is ok as description but I think you're missing the explanation
> of the expected output as Julien said.
> For example: Float text doesn't enter in the second row.

Done.

[jfernandez, 2014-11-12 14:36:22]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-11-12 14:37:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/697653004/40001

[commit-bot: I haz the power, 2014-11-12 16:05:09]

Committed patchset #3 (id:40001) as 185214