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

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

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.

[Javier Fernandez]

Created attachment 242346 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 242346 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242346&action=review

Good catch and yay! for the new tests. But I'm setting r- for the moment due to the amount of required changes.

> Source/WebCore/rendering/RenderBlockFlow.cpp:79
> +        && !block.isGridItem();

Missing guards for GRID_LAYOUT.

> Source/WebCore/rendering/RenderBox.h:533
> +    bool isGridItem() const { return parent() && parent()->isRenderGrid(); }

Ditto.

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:8
> +   height: auto;

This is auto by default so you don't likely need it.

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:39
> +  <div class="cell relative floatLeft firstRowFirstColumn">

Why firstRowFirstColumn?

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:45
> +  <div class="cell floatLeft firstRowSecondColumn">

Why firstRowSecondColumn?

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:51
> +  <div class="cell floatLeft clearLeft secondRowFirstColumn"></div>

Why secondRowFirstColumn?

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item-expected.html:52
> +  <div class="cell floatLeft secondRowSecondColumn"></div>

Why secondRowSecondColumn?

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:7
> +    -webkit-grid-auto-columns: minmax(50px, -webkit-max-content);

Wouldn't be enough with 50px without the minmax?

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:8
> +    -webkit-grid-auto-rows: minmax(50px, -webkit-max-content);

Ditto.

> LayoutTests/fast/css-grid-layout/float-not-protruding-into-next-grid-item.html:14
> +   clear: both;

Not sure why you need the 'clear' here.

> LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse-expected.html:2
> +<link href="resources/grid.css" rel="stylesheet">

I guess you don't need this here.

> LayoutTests/fast/css-grid-layout/grid-item-margins-not-collapse-expected.html:7
> +  <div ><p margin="20px 0px">XXXXX</p></div>

Nit: extra space after div.

[Javier Fernandez]

Created attachment 242413 [details]
Patch

Applied suggested changes.

[Dave Hyatt]

Comment on attachment 242413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242413&action=review

r-

> Source/WebCore/rendering/RenderBlock.cpp:1103
> -bool RenderBlock::expandsToEncloseOverhangingFloats() const
> +bool RenderBlock::createsNewFormattingContext() const
>  {
> -    return isInlineBlockOrInlineTable() || isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || (parent() && parent()->isFlexibleBoxIncludingDeprecated())
> -        || isTableCell() || isTableCaption() || isFieldset() || isWritingModeRoot() || isRoot() || isRenderFlowThread();
> +    return isInlineBlockOrInlineTable() || isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || isFlexItemIncludingDeprecated()
> +        || isTableCell() || isTableCaption() || isFieldset() || isWritingModeRoot() || isRoot() || isRenderFlowThread()
> +#if ENABLE(CSS_GRID_LAYOUT)
> +        || isGridItem()
> +#endif
> +        || style().specifiesColumns() || style().columnSpan();
>  }

Need to add isRenderRegion to this list.

From http://dev.w3.org/csswg/css-regions/ section 3.2:

"CSS Regions establish a new block formatting Context."

> Source/WebCore/rendering/RenderBlockFlow.cpp:223
> +    while (prev && (!is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats() || downcast<RenderBlock>(prev)->createsNewFormattingContext())) {

Anything that creates a new formatting context also avoids floats, so I'd rather see you fix avoidsFloats to be implemented in terms of createsNewFormattingContext (plus anything additional that avoidsFloats also checks). Then the only change you'd need to make here is the removal of prev->isFloatingOrOutOfFlowPositioned().

> Source/WebCore/rendering/RenderBlockFlow.cpp:2587
> +    if (!child.containsFloats() || child.isRoot() || child.isRenderRegion() || child.createsNewFormattingContext())

No need for child.isRoot() here or child.isRenderRegion() here (once you add isRenderRegion to createsNewFormattingContext).

[Javier Fernandez]

Created attachment 242480 [details]
avoids-floats-refactoring

The avoidFloats function uses now the new createsNewFormattingContext. Rebaselining the forms/form-hides-table.html test.

[Build Bot]

Comment on attachment 242480 [details]
avoids-floats-refactoring

Attachment 242480 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6712604277866496

New failing tests:
fast/forms/form-hides-table.html

[Build Bot]

Created attachment 242485 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 242480 [details]
avoids-floats-refactoring

Attachment 242480 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5678057452994560

New failing tests:
fast/forms/form-hides-table.html

[Build Bot]

Created attachment 242486 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 242750 [details]
avoidsFloats-refactoring.

The avoidsFloats() uses now the new createsNewFormattingContext function. Some tests needed a rebaseline.

[Dave Hyatt]

Comment on attachment 242750 [details]
avoidsFloats-refactoring.

r=me, although we should get the test rebaselined ASAP so the behavior change for table captions is recorded in a test.

[WebKit Commit Bot]

Comment on attachment 242750 [details]
avoidsFloats-refactoring.

Clearing flags on attachment: 242750

Committed r176957: <http://trac.webkit.org/changeset/176957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 242750 [details]
avoidsFloats-refactoring.

View in context: https://bugs.webkit.org/attachment.cgi?id=242750&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2378
>      return RenderBox::avoidsFloats()
> -        || !style().hasAutoColumnCount()
> -        || !style().hasAutoColumnWidth()
>          || style().hasFlowFrom();

Should probably be changed into a 1-liner; style is now peculiar.

> Source/WebCore/rendering/RenderBlockFlow.cpp:226
>      RenderObject* prev = previousSibling();
> -    while (prev && (prev->isFloatingOrOutOfFlowPositioned() || !is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats())) {
> +    while (prev && (!is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats() || downcast<RenderBlock>(prev)->createsNewFormattingContext())) {
>          if (prev->isFloating())
>              parentHasFloats = true;
>          prev = prev->previousSibling();

This is a confusing way to write this condition. The RenderBox check is not useful since it’s redundant with the RenderBlockFlow check, and the new check is written in a different style than the old. And there is no reason to cast to RenderBlock instead of RenderBlockFlow since we know the object is a RenderBlockFlow. Ideally we would use a helper function, and also rename “prev” to some kind of word rather than an abbreviation, but at least we should write it this way:

    RenderBlockFlow* previousBlock = nullptr;
    for (RenderObject* sibling = previousSibling(); sibling; sibling = sibling->previousSibling()) {
        if (is<RenderBlockFlow>(*sibling)) {
            auto& siblingBlock = downcast<RenderBlockFlow>(*sibling);
            if (!siblingBlock.avoidsFloats() && !siblingBlock.createsNewFormattingContext())
                previousBlock = &siblingBlock;
                break;
        }
        if (sibling->isFloating())
            parentHasFloats = true;
    }

Then, below, we can use previousBlock without having to cast it, since we know it’s a RenderBlockFlow.

But also, why do we need to check both avoidsFloats and createsNewFormattingContext? Doesn’t one call the other?

> Source/WebCore/rendering/RenderBox.h:535
> +#if ENABLE(CSS_GRID_LAYOUT)
> +    bool isGridItem() const { return parent() && parent()->isRenderGrid(); }
> +#endif

Does this really need to be public? Lets have it be private until we find someone who needs to call it.

[Javier Fernandez]

(In reply to comment #14)

Suggested changes will be added in bug #139445.

> Comment on attachment 242750 [details]
> avoidsFloats-refactoring.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242750&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2378
> >      return RenderBox::avoidsFloats()
> > -        || !style().hasAutoColumnCount()
> > -        || !style().hasAutoColumnWidth()
> >          || style().hasFlowFrom();
> 
> Should probably be changed into a 1-liner; style is now peculiar.
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:226
> >      RenderObject* prev = previousSibling();
> > -    while (prev && (prev->isFloatingOrOutOfFlowPositioned() || !is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats())) {
> > +    while (prev && (!is<RenderBox>(*prev) || !is<RenderBlockFlow>(*prev) || downcast<RenderBlockFlow>(*prev).avoidsFloats() || downcast<RenderBlock>(prev)->createsNewFormattingContext())) {
> >          if (prev->isFloating())
> >              parentHasFloats = true;
> >          prev = prev->previousSibling();
> 
> This is a confusing way to write this condition. The RenderBox check is not
> useful since it’s redundant with the RenderBlockFlow check, and the new
> check is written in a different style than the old. And there is no reason
> to cast to RenderBlock instead of RenderBlockFlow since we know the object
> is a RenderBlockFlow. Ideally we would use a helper function, and also
> rename “prev” to some kind of word rather than an abbreviation, but at least
> we should write it this way:
> 
>     RenderBlockFlow* previousBlock = nullptr;
>     for (RenderObject* sibling = previousSibling(); sibling; sibling =
> sibling->previousSibling()) {
>         if (is<RenderBlockFlow>(*sibling)) {
>             auto& siblingBlock = downcast<RenderBlockFlow>(*sibling);
>             if (!siblingBlock.avoidsFloats() &&
> !siblingBlock.createsNewFormattingContext())
>                 previousBlock = &siblingBlock;
>                 break;
>         }
>         if (sibling->isFloating())
>             parentHasFloats = true;
>     }
> 
> Then, below, we can use previousBlock without having to cast it, since we
> know it’s a RenderBlockFlow.
> 
> But also, why do we need to check both avoidsFloats and
> createsNewFormattingContext? Doesn’t one call the other?
> 
> > Source/WebCore/rendering/RenderBox.h:535
> > +#if ENABLE(CSS_GRID_LAYOUT)
> > +    bool isGridItem() const { return parent() && parent()->isRenderGrid(); }
> > +#endif
> 
> Does this really need to be public? Lets have it be private until we find
> someone who needs to call it.