`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
<rdar://problem/29918759>
Created attachment 369909 [details] Patch
Could you please review this change?
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 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:)
Is the WPT enough to test the behavior of flow and flow-root?
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.
(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.
(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:)
(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.
(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 on attachment 369909 [details] Patch Clearing flags on attachment: 369909 Committed r245494: <https://trac.webkit.org/changeset/245494>
All reviewed patches have been landed. Closing bug.