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

Marat Tanalin
Reported 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
Attachments
Patch (9.82 KB, patch)
2019-05-14 16:49 PDT, Joonghun Park
no flags
Radar WebKit Bug Importer
Comment 1 2017-01-08 10:12:47 PST
Joonghun Park
Comment 2 2019-05-14 16:49:16 PDT
Joonghun Park
Comment 3 2019-05-15 16:26:25 PDT
Could you please review this change?
Simon Fraser (smfr)
Comment 4 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.
Joonghun Park
Comment 5 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:)
Simon Fraser (smfr)
Comment 6 2019-05-16 11:08:11 PDT
Is the WPT enough to test the behavior of flow and flow-root?
zalan
Comment 7 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.
Joonghun Park
Comment 8 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.
Joonghun Park
Comment 9 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:)
zalan
Comment 10 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.
Joonghun Park
Comment 11 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:)
WebKit Commit Bot
Comment 12 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>
WebKit Commit Bot
Comment 13 2019-05-17 21:29:56 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.