Bug 165603 – Implement CSS `display: flow-root` (modern clearfix)

• Component: CSS
• Status: RESOLVED FIXED

[Marat Tanalin]

`display: flow-root` is a modern way to force a block to be a formatting context that floated elements are contained in (aka clearfix).

From the spec [1]:

> The element generates a block container box, and lays out its contents using flow layout. It always establishes a new block formatting context for its contents.

Tab Atkins and Elika Etemad (fantasai) from CSSWG consider the feature stable enough to be implemented [2].

[1] https://drafts.csswg.org/css-display-3/#valdef-display-flow-root
[2] https://discourse.wicg.io/t/1835/6

[Radar WebKit Bug Importer]

<rdar://problem/29918759>

[Joonghun Park]

Created attachment 369909 [details]
Patch

[Joonghun Park]

Could you please review this change?

[Simon Fraser (smfr)]

Comment on attachment 369909 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:699
> +    case DisplayType::FlowRoot:

Is this change sufficient to get the behavior described in the spec? It's unclear to me if this change is complete.

[Joonghun Park]

Comment on attachment 369909 [details]
Patch

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

>> Source/WebCore/css/StyleResolver.cpp:699
>> +    case DisplayType::FlowRoot:
> 
> Is this change sufficient to get the behavior described in the spec? It's unclear to me if this change is complete.

The 'flow-root' spec says that if it is specified it creates a new block formatting context.

https://www.w3.org/TR/CSS2/visuren.html#normal-flow specifies the condition which when BFC is created,
and the conditions are listed in RenderBox::createsNewFormattingContext().

So I added flow-root to the end of the list of the conditions for creating BFC as below.

bool RenderBox::createsNewFormattingContext() const
{
    return isInlineBlockOrInlineTable() || isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || isFlexItemIncludingDeprecated()
        || isTableCell() || isTableCaption() || isFieldset() || isWritingModeRoot() || isDocumentElementRenderer() || isRenderFragmentedFlow() || isRenderFragmentContainer()
        || isGridItem() || style().specifiesColumns() || style().columnSpan() == ColumnSpan::All || style().display() == DisplayType::FlowRoot;
}

BFC creation is addressed above, so I think FlowRoot display type works as expected and this change is sufficient.

But if I missed something, please let me know:)

[Simon Fraser (smfr)]

Is the WPT enough to test the behavior of flow and flow-root?

[alan]

Comment on attachment 369909 [details]
Patch

Unfortunately we can't explicitly say that this block level box should establish a block formatting context, instead the combination of the renderer type (RenderBlock in this case) and its children will imply the type of layout (e.g. inline vs block). ::createsNewFormattingContext() is really mostly used to check against float intruding and avoiding. So while this patch is fine in this context, we really lack some fundamental CSS basics in our codebase.

[Joonghun Park]

(In reply to Simon Fraser (smfr) from comment #6)
> Is the WPT enough to test the behavior of flow and flow-root?

Yes, the WPT test includes 5 sub test and I think they are testing the expected behavior for flow-root and flow well.

[Joonghun Park]

(In reply to zalan from comment #7)
> Comment on attachment 369909 [details]
> Patch
> 
> Unfortunately we can't explicitly say that this block level box should
> establish a block formatting context, instead the combination of the
> renderer type (RenderBlock in this case) and its children will imply the
> type of layout (e.g. inline vs block). ::createsNewFormattingContext() is
> really mostly used to check against float intruding and avoiding. So while
> this patch is fine in this context, we really lack some fundamental CSS
> basics in our codebase.

For supporting more CSS basic things, I will make a bug in jira if needed, or if there are existing bugs, I will check that to try to address it.
Thank you for your review Zalan:)

[alan]

(In reply to Joonghun Park from comment #9)
> (In reply to zalan from comment #7)
> > Comment on attachment 369909 [details]
> > Patch
> > 
> > Unfortunately we can't explicitly say that this block level box should
> > establish a block formatting context, instead the combination of the
> > renderer type (RenderBlock in this case) and its children will imply the
> > type of layout (e.g. inline vs block). ::createsNewFormattingContext() is
> > really mostly used to check against float intruding and avoiding. So while
> > this patch is fine in this context, we really lack some fundamental CSS
> > basics in our codebase.
> 
> For supporting more CSS basic things, I will make a bug in jira if needed,
> or if there are existing bugs, I will check that to try to address it.
> Thank you for your review Zalan:)
No I don't have a bug on this but the LFC code (WebCore/layout) is meant to fix all these visual formatting model fundamentals.

[Joonghun Park]

(In reply to zalan from comment #10)
> (In reply to Joonghun Park from comment #9)
> > (In reply to zalan from comment #7)
> > > Comment on attachment 369909 [details]
> > > Patch
> > > 
> > > Unfortunately we can't explicitly say that this block level box should
> > > establish a block formatting context, instead the combination of the
> > > renderer type (RenderBlock in this case) and its children will imply the
> > > type of layout (e.g. inline vs block). ::createsNewFormattingContext() is
> > > really mostly used to check against float intruding and avoiding. So while
> > > this patch is fine in this context, we really lack some fundamental CSS
> > > basics in our codebase.
> > 
> > For supporting more CSS basic things, I will make a bug in jira if needed,
> > or if there are existing bugs, I will check that to try to address it.
> > Thank you for your review Zalan:)
> No I don't have a bug on this but the LFC code (WebCore/layout) is meant to
> fix all these visual formatting model fundamentals.

I can see the patches with tag [LFC] in log. I will read that patches and follow up the changes. Thank you:)

[WebKit Commit Bot]

Comment on attachment 369909 [details]
Patch

Clearing flags on attachment: 369909

Committed r245494: <https://trac.webkit.org/changeset/245494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.