Bug 165603

Summary: Implement CSS `display: flow-root` (modern clearfix)
Product: WebKit Reporter: Marat Tanalin <mtanalin>
Component: CSSAssignee: Joonghun Park <jh718.park>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, darin, dino, eoconnor, gyuyoung.kim, jh718.park, Justin, koivisto, m.kurz+webkitbugs, murakami, simon.fraser, webkit-bug-importer, webkit.bugzilla, webkit, zalan
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://drafts.csswg.org/css-display-3/#valdef-display-flow-root
See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=1322191
https://bugs.chromium.org/p/chromium/issues/detail?id=672508
Attachments:
Description Flags
Patch none

Description Marat Tanalin 2016-12-08 08:19:02 PST
`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
Comment 1 Radar WebKit Bug Importer 2017-01-08 10:12:47 PST
<rdar://problem/29918759>
Comment 2 Joonghun Park 2019-05-14 16:49:16 PDT
Created attachment 369909 [details]
Patch
Comment 3 Joonghun Park 2019-05-15 16:26:25 PDT
Could you please review this change?
Comment 4 Simon Fraser (smfr) 2019-05-15 16:57:19 PDT
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.
Comment 5 Joonghun Park 2019-05-16 08:03:42 PDT
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:)
Comment 6 Simon Fraser (smfr) 2019-05-16 11:08:11 PDT
Is the WPT enough to test the behavior of flow and flow-root?
Comment 7 zalan 2019-05-16 19:27:53 PDT
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.
Comment 8 Joonghun Park 2019-05-17 04:24:17 PDT
(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.
Comment 9 Joonghun Park 2019-05-17 04:34:09 PDT
(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:)
Comment 10 zalan 2019-05-17 11:26:40 PDT
(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.
Comment 11 Joonghun Park 2019-05-17 21:00:41 PDT
(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:)
Comment 12 WebKit Commit Bot 2019-05-17 21:29:54 PDT
Comment on attachment 369909 [details]
Patch

Clearing flags on attachment: 369909

Committed r245494: <https://trac.webkit.org/changeset/245494>
Comment 13 WebKit Commit Bot 2019-05-17 21:29:56 PDT
All reviewed patches have been landed.  Closing bug.