Bug 139150 - [CSS Grid Layout] Grid items must set a new formatting context.
Summary: [CSS Grid Layout] Grid items must set a new formatting context.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-01 14:02 PST by Javier Fernandez
Modified: 2014-12-09 07:29 PST (History)
12 users (show)

See Also:


Attachments
Patch (8.87 KB, patch)
2014-12-01 14:49 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (13.62 KB, patch)
2014-12-02 09:06 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
avoids-floats-refactoring (52.74 KB, patch)
2014-12-03 01:39 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (501.55 KB, application/zip)
2014-12-03 02:57 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (514.25 KB, application/zip)
2014-12-03 03:25 PST, Build Bot
no flags Details
avoidsFloats-refactoring. (20.91 KB, patch)
2014-12-07 08:57 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2014-12-01 14:02:54 PST
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.
Comment 1 Javier Fernandez 2014-12-01 14:49:18 PST
Created attachment 242346 [details]
Patch
Comment 2 Sergio Villar Senin 2014-12-02 00:34:25 PST
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.
Comment 3 Javier Fernandez 2014-12-02 09:06:57 PST
Created attachment 242413 [details]
Patch

Applied suggested changes.
Comment 4 Dave Hyatt 2014-12-02 09:26:31 PST
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).
Comment 5 Javier Fernandez 2014-12-03 01:39:52 PST
Created attachment 242480 [details]
avoids-floats-refactoring

The avoidFloats function uses now the new createsNewFormattingContext. Rebaselining the forms/form-hides-table.html test.
Comment 6 Build Bot 2014-12-03 02:57:36 PST
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
Comment 7 Build Bot 2014-12-03 02:57:41 PST
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
Comment 8 Build Bot 2014-12-03 03:25:50 PST
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
Comment 9 Build Bot 2014-12-03 03:25:56 PST
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
Comment 10 Javier Fernandez 2014-12-07 08:57:18 PST
Created attachment 242750 [details]
avoidsFloats-refactoring.

The avoidsFloats() uses now the new createsNewFormattingContext function. Some tests needed a rebaseline.
Comment 11 Dave Hyatt 2014-12-08 10:59:13 PST
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.
Comment 12 WebKit Commit Bot 2014-12-08 11:12:47 PST
Comment on attachment 242750 [details]
avoidsFloats-refactoring.

Clearing flags on attachment: 242750

Committed r176957: <http://trac.webkit.org/changeset/176957>
Comment 13 WebKit Commit Bot 2014-12-08 11:12:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2014-12-08 22:12:08 PST
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.
Comment 15 Javier Fernandez 2014-12-09 07:29:18 PST
(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.