Marat Tanalin Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161205004004

Steps to reproduce:

`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

	Mats Palmgren (inactive) Assignee
	Comment 1 • 7 years ago

This feature is trivial so a pref seems like overkill, but I guess
it doesn't hurt to have just in case...

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 5 • 7 years ago

Could you send an intent to implement and ship to dev-platform, per https://wiki.mozilla.org/WebAPI/ExposureGuidelines

	Mats Palmgren (inactive) Assignee
	Comment 6 • 7 years ago

https://groups.google.com/forum/#!topic/mozilla.dev.platform/bG9Kpgr5LC4
https://lists.w3.org/Archives/Public/www-style/2016Dec/0074.html

	Marat Tanalin Reporter
	Comment 7 • 7 years ago

Fwiw, here is the corresponding feature request for Edge (Bugzilla doesn’t allow to add it to the “See Also” field):

https://wpdev.uservoice.com/forums/257854/suggestions/17420707

	Daniel Holbert [:dholbert]
	Comment 8 • 7 years ago

Comment on attachment 8820791 [details] [diff] [review]
part 4 - Regenerate devtools properties-db.js after adding display:flow-root.

Review of attachment 8820791 [details] [diff] [review]:
-----------------------------------------------------------------

Reviewing part 4, the most trivial part, first. Just one observation/question (which you might not have an answer to) -- r=me regardless.

::: devtools/shared/css/generated/properties-db.js
@@ -3259,4 @@
>        "forwards",
>        "full-width",
>        "geometricprecision",
> -      "grayscale",

Do you know what's up with this "grayscale" removal? That value looks wholly unrelated to this patch. I wonder why it's in there (and why the autogeneration tool decided that now is a good time to remove it).  At first I thought maybe this was a value that was recently-unsupported, but I don't see any recent csets that mention "grayscale" in their diff.  Maybe it's a platform-specific value (e.g. only meaningful on mac) and it's not supported on your platform?

Anyway, if we had a better process for maintaining this autogenerated file, I'd be more concerned with auditing this change & perhaps splitting this into its own cleanup patch. But we don't have a great story for this file right now... so, *shrug* meh.

	Daniel Holbert [:dholbert]
	Comment 9 • 7 years ago

(You might consider explicitly mentioning the command that you used to regenerate the properties-db.js file, in the extended commit message of part 4.  It's always nice to provide the command that was used, in autogenerated patches, for auditing/posterity/people-who-need-to-generate-similar-patches-and-are-cribbing-from-yours.)

	Daniel Holbert [:dholbert]
	Comment 10 • 7 years ago

Comment on attachment 8820788 [details] [diff] [review]
part 1 - [css-display] Add style system support for display:flow-root.  Enable it by default.

Review of attachment 8820788 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed as you see fit

::: layout/base/nsLayoutUtils.cpp
@@ +326,5 @@
> +  bool isDisplayFlowRootEnabled =
> +    Preferences::GetBool(DISPLAY_FLOW_ROOT_ENABLED_PREF_NAME, false);
> +
> +  if (!sIsDisplayFlowRootKeywordIndexInitialized) {
> +    // First run: find the position of "flow_root" in kDisplayKTable.

"flow_root" is not a thing. I think this means to say "flow-root"? (with a hyphen)

(I'm guessing this is just a search-and-replace error, when fixing up eCSSKeyword_*** naming).

@@ +333,5 @@
> +                                     nsCSSProps::kDisplayKTable);
> +    sIsDisplayFlowRootKeywordIndexInitialized = true;
> +  }
> +
> +  // OK -- now, stomp on or restore the "flow_root" entry in kDisplayKTable,

Same here -- hyphenate "flow-root" please.

::: layout/style/nsStyleStruct.h
@@ +2885,5 @@
>      return mozilla::StyleDisplay::Block == mDisplay ||
>             mozilla::StyleDisplay::ListItem == mDisplay ||
>             mozilla::StyleDisplay::InlineBlock == mDisplay ||
> +           mozilla::StyleDisplay::TableCaption == mDisplay ||
> +           mozilla::StyleDisplay::FlowRoot == mDisplay;

Since "FlowRoot" is so closely tied to "Block", maybe it'd be best to insert it *directly* after "Block" in lists such as this one?  (rather than just inserting at the end of the list, as this patch seems to generally do right now)

(The bulk of this patch is insertion into such lists.)

	Daniel Holbert [:dholbert]
	Comment 11 • 7 years ago

Comment on attachment 8820789 [details] [diff] [review]
part 2 - [css-display] Implement layout for display:flow-root.

Review of attachment 8820789 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBlockFrame.cpp
@@ +300,5 @@
>  {
> +  auto blockFrame = new (aPresShell) nsBlockFrame(aContext);
> +  if (aContext->StyleDisplay()->mDisplay == StyleDisplay::FlowRoot) {
> +    blockFrame->AddStateBits(NS_BLOCK_FLOAT_MGR | NS_BLOCK_MARGIN_ROOT);
> +  }

Looks like this new special-case (in NS_NewBlockFrame) matches what we already do in NS_NewBlockFormattingContext.  (And NS_NewBlockFormattingContext itself does also call into NS_NewBlockFrame -- so it may end up setting these bits twice now, which is probably cheap albeit a little messy/redundant.)

Would it be possible/cleaner to just wire up "display:flow-root" to invoke NS_NewBlockFormattingContext directly?  That would make the connection between the spec & the code a bit clearer, I think.  (In isolation, it's unclear why these specific bits are set here. But NS_NewBlockFormattingContext is more self-documenting [via its name] about what this constellation of bits is associated with.

	Mats Palmgren (inactive) Assignee
	Comment 12 • 7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #8)
Dunno, it's just the result that "./mach devtools-css-db" gave me.
It appears to be a value associated with 'font-smoothing' and 'filter'?
I have no clue why the tool decided to remove it.  I'll leave it in since
it looks like it's still a valid value.  It's hard to know because people
often forget to run that command leaving the next guy to pick up the changes.

Anyway, filed bug 1325472.  And I've already complained that this stuff
should be automated so humans don't have to deal with it, because... mistakes,
in bug 1320607, but apparently it's too complicated :-(

	Daniel Holbert [:dholbert]
	Comment 13 • 7 years ago

Comment on attachment 8820790 [details] [diff] [review]
part 3 - [css-display] Tests for display:flow-root.

Review of attachment 8820790 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 3, with nits addressed as you see fit:

::: layout/reftests/css-display/display-flow-root-001.html
@@ +26,5 @@
> +<body>
> +
> +<div style="border:1px solid">
> +  <span style="display:flow-root; margin: 20px 0">
> +    <div style="margin: 20px 0">x</div>

If you could, it'd be helpful to add some brief HTML comments to hint at what you're testing in each section of this test.  (In particular: flow-root affects floats & margins, and this test has 2 margin-only chunks, 1 float-only chunk, and 1 float+margins chunk.  And it's not superficially clear what expectation each of these different chunks is aiming to test, & how they differ from one another.)

(e.g. I think this part is testing that vertical margins outside the flow-root don't collapse with those inside?  Maybe I'm slow, but it took me a few minutes of staring at the test/reference renderings in current Firefox to figure that out, & realize why it's supposed to hold. :))

@@ +40,5 @@
> +  <span style="display:flow-root; border:1px solid">x</span>
> +</div>
> +
> +<span>
> +  <span style="display:flow-root"><div style="margin:20px">x</div></span>

(And it's not immediately clear what this chunk is testing.  Superficially, looking at it in isolation, it looks like it's testing an interaction between "flow-root" & "margin".  But in fact, I think it's *really* testing some after-effect from the floats above it.)

::: layout/reftests/css-display/display-flow-root-disabled-001-ref.html
@@ +38,5 @@
> +  <span style="border:1px solid">x</span>
> +</div>
> +
> +<span>
> +  <span style="display:inline-block; vertical-align:bottom"><div style="margin-top:20px">x</div></span>

In the interests of simplicity: do you really need this extra "display:inline-block;vertical-align:bottom" styling here? I'd think you could just use an unstyled <span>, since that's what the testcase's <span style="display:flow-root"> is really equivalent to, in a build without flow-root support.

(I tried removing these styles locally, too, it didn't seem to change the rendering.)

	Mats Palmgren (inactive) Assignee
	Comment 14 • 7 years ago

> Would it be possible/cleaner to just wire up "display:flow-root" to invoke NS_NewBlockFormattingContext directly?

Well, I started out doing that, but I thought it was too error prone to have to deal
with this in every place that calls NS_NewBlockFrame in nsCSSFrameConstructor.
This seems like a more robust solution.  I'm worried that people will forget to deal
with it when they add new NS_NewBlockFrame calls.

	Daniel Holbert [:dholbert]
	Comment 15 • 7 years ago

(In reply to Mats Palmgren (:mats) from comment #14)
> Well, I started out doing that, but I thought it was too error prone to have
> to deal
> with this in every place that calls NS_NewBlockFrame in
> nsCSSFrameConstructor.
> This seems like a more robust solution.  I'm worried that people will forget
> to deal
> with it when they add new NS_NewBlockFrame calls.

Fair enough. I guess what worries me about the current solution is that "NS_BLOCK_FLOAT_MGR | NS_BLOCK_MARGIN_ROOT" is just the "magic word" for Block Formatting Context, without it being super-clear that that's what we're doing.  (This is already the case in a number of other places in the code, as I discovered when doing a DXR search.)

Perhaps as a helper-patch (either before or after this one), you could add a convenience #define called e.g. NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS in nsBlockFrame.h, and replace all current instances of this 2-bit bitmask with that clearer name?

	Mats Palmgren (inactive) Assignee
	Comment 16 • 7 years ago

We already have this sort of thing in nsBlockFrame::Init already,
so I decided to move it there instead to avoid having similar
code in two places.

	Mats Palmgren (inactive) Assignee
	Comment 17 • 7 years ago

(also removed an unnecessary null-check in nsLegendFrame.cpp because
allocating a frame is infallible these days)

	Daniel Holbert [:dholbert]
	Comment 18 • 7 years ago

Comment on attachment 8821331 [details] [diff] [review]
part 2 - [css-display] Implement layout for display:flow-root.

Review of attachment 8821331 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with a few nits:

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4835,5 @@
>      FCDATA_FOR_DISPLAY(StyleDisplay::WebkitBox,
>        FCDATA_DECL(FCDATA_MAY_NEED_SCROLLFRAME, NS_NewFlexContainerFrame)),
>      FCDATA_FOR_DISPLAY(StyleDisplay::WebkitInlineBox,
>        FCDATA_DECL(FCDATA_MAY_NEED_SCROLLFRAME, NS_NewFlexContainerFrame)),
> +    FCDATA_FOR_DISPLAY(StyleDisplay::FlowRoot, UNREACHABLE_FCDATA()),

Per end of comment 10, it seems more logical to group this FlowRoot line up higher with the Block line, here:
https://dxr.mozilla.org/mozilla-central/rev/f179934df0c1bab590c558485d419c7910e41325/layout/base/nsCSSFrameConstructor.cpp#4760-4761

...particularly since they both have the same UNREACHABLE_FCDATA behavior.

@@ +4840,2 @@
>    };
> +  static_assert(ArrayLength(sDisplayData) == size_t(StyleDisplay::FlowRoot) + 1,

And similarly: if you reorder nsStyleConsts in part 1 to insert FlowRoot alongside Block (instead of at the end), as I think you probably should, then you won't have to touch this line in this patch.

	Daniel Holbert [:dholbert]
	Comment 19 • 7 years ago

Comment on attachment 8821332 [details] [diff] [review]
part 2b - Replace "NS_BLOCK_FLOAT_MGR | NS_BLOCK_MARGIN_ROOT" with NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS.

Review of attachment 8821332 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLParts.h
@@ +29,5 @@
>  
>  // These are all the block specific frame bits, they are copied from
>  // the prev-in-flow to a newly created next-in-flow, except for the
>  // NS_BLOCK_FLAGS_NON_INHERITED_MASK bits below.
> +#define NS_BLOCK_FLAGS_MASK (NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS | \

Perhaps better to leave this part unchanged (& leave it listing the specific individual bits), since this seems to be wanting* to exhaustively list a collection of all bits in this category?

(*I say it's "wanting" to exhaustively list the bits, per the code-comment "These are all the block specific frame bits".  But it doesn't actually succeed -- there are only 7 bits listed here, whereas there are 15 NS_BLOCK_* bits listed in nsFrameStateBits.h. I wonder if that's a problem? I guess it only gets used when copying bits from prev-in-flow, so the random other block bits don't get copied to NIFs I guess.)

	Mats Palmgren (inactive) Assignee
	Comment 20 • 7 years ago

> Perhaps better to leave this part unchanged...

I think it's better this way, because if we add a third BFC bit
at some point I think we definitely want to pick it up here.

> there are only 7 bits listed here, whereas there are 15 NS_BLOCK_* bits listed in nsFrameStateBits.h

Yeah, this setup is a bit odd.  Ideally, each bit in nsFrameStateBits.h should
declare if it should be copied to a next-in-flow or not.

	Mats Palmgren (inactive) Assignee
	Comment 21 • 7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #18)
> And similarly: if you reorder nsStyleConsts in part 1 to insert FlowRoot
> alongside Block (instead of at the end), as I think you probably should,
> then you won't have to touch this line in this patch.

Fair enough.  I was a bit concerned that the list might be iterated
in order by some code, but it looks like the frame ctor code at least
use array access for sDisplayData.  I think I'll keep it at the end
of IsBlockInsideStyle/IsBlockOutsideStyle though, so that the values
are tested in most-commonly-used order for performance.

	Pulsebot
	Comment 22 • 7 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9ef8b75095
part 1 - [css-display] Add style system support for display:flow-root.  Enable it by default.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6da68ef786
part 2 - [css-display] Implement layout for display:flow-root.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/14b27f1d4157
part 2b - Replace "NS_BLOCK_FLOAT_MGR | NS_BLOCK_MARGIN_ROOT" with NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae695cada69
part 3 - [css-display] Tests for display:flow-root.  r=dholbert

	Phil Ringnalda (:philor)
	Comment 23 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0c9ef8b75095
https://hg.mozilla.org/mozilla-central/rev/bf6da68ef786
https://hg.mozilla.org/mozilla-central/rev/14b27f1d4157
https://hg.mozilla.org/mozilla-central/rev/fae695cada69

	Marat Tanalin Reporter
	Comment 24 • 7 years ago

Mats, thanks for so fast implementation. Unfortunately, I've tested the feature in Firefox Nightly (20161225030206), and it behaves strangely:

while clearing floats really works (that's great), width of the element that has `display: flow-root` is somehow not 100% of its parent block, but depends on actual-content width instead (like a table).

If this is intended according to the spec (I'm not a guru of reading specs), then the width issue looks like an undesired and harmful side effect since _clearing floats_ is the _only_ thing web developers actually need from such a feature, so this should probably be solved on spec-level _before_ shipping the implemented feature, otherwise it's far less useful than it has been expected.

	Marat Tanalin Reporter
	Comment 25 • 7 years ago

If I read the spec [1] correctly, the “short” value of `flow-root` corresponds to the “full” value of `block flow-root` (the latter itself does not currently work in Firefox, fwiw), so this is probably a Firefox-implementation bug.

[1] https://drafts.csswg.org/css-display-3/#display-value-summary

	Mats Palmgren (inactive) Assignee
	Comment 26 • 7 years ago

(In reply to Marat Tanalin | tanalin.com from comment #24)
> while clearing floats really works (that's great), width of the element that
> has `display: flow-root` is somehow not 100% of its parent block, but
> depends on actual-content width instead (like a table).

Can you file a new bug with a testcase please?

	Mats Palmgren (inactive) Assignee
	Comment 27 • 7 years ago

(In reply to Marat Tanalin | tanalin.com from comment #25)
> If I read the spec [1] correctly, the “short” value of `flow-root`
> corresponds to the “full” value of `block flow-root` (the latter itself does
> not currently work in Firefox, fwiw), so this is probably a
> Firefox-implementation bug.
> 
> [1] https://drafts.csswg.org/css-display-3/#display-value-summary

We're only implementing the single-keyword form of 'display' for now.
But yes, 'display:flow-root' is equivalent to 'display:block flow-root'
so that's the layout we want here.

	Marat Tanalin Reporter
	Comment 28 • 7 years ago

Mats, please see the bug 1325970.

	Sebastian Zartner [:sebo]
	Comment 29 • 7 years ago

Documented the new value at https://developer.mozilla.org/en-US/docs/Web/CSS/display and mentioned it in the developer release notes at https://developer.mozilla.org/en-US/Firefox/Releases/53#CSS.

Mats, can you please review the documentation?

Sebastian

	Mats Palmgren (inactive) Assignee
	Comment 30 • 7 years ago

One nit on the documentation:  the 'flow-root' (and 'flow') values are missing
in the Syntax and Formal Syntax sections.  And the 'flow' value is missing altogether.
https://drafts.csswg.org/css-display-3/#inner-model
(we (and I suspect other UAs) don't implement 'flow' yet)

BTW, there is a word "display-value" in bold as the first word in the Values section -
I don't quite understand what purpose that serves.  Maybe just remove it?

I'm not sure what the relation is between Syntax and Formal Syntax, is the former
just an overview of the syntax we support (although some of the values are still
unsupported)? and the latter is the syntax from the spec?  If so, the latter
should probably be updated to what the spec says:
https://drafts.csswg.org/css-display/#the-display-properties

Other than it looks good to me.

	Mats Palmgren (inactive) Assignee
	Comment 31 • 7 years ago

... where "the syntax we support" means single-keyword display values only.

	yisibl
	Comment 32 • 7 years ago

Are you planning to support `display: inline flow-root`?

	Mats Palmgren (inactive) Assignee
	Comment 33 • 7 years ago

I think that's the same as 'display:inline-block', per the table at:
https://drafts.csswg.org/css-display/#propdef-display

I guess you're asking: will we implement the general 'display' syntax?
I don't see any urgent need to do so, since most of the combinations that
authors want are already available as single keywords.  Also, the syntax
for 'display' has changed back and forth a few times so I don't know how
stable the current version is.

	yisibl
	Comment 34 • 7 years ago

So there is really no need to implement the new display syntax, the specification is too unstable.

Thanks and happy new year!

	Sebastian Zartner [:sebo]
	Comment 35 • 7 years ago

(In reply to Mats Palmgren (:mats) from comment #30)
> One nit on the documentation:  the 'flow-root' (and 'flow') values are missing
> in the Syntax and Formal Syntax sections.  And the 'flow' value is missing altogether.
> https://drafts.csswg.org/css-display-3/#inner-model
> (we (and I suspect other UAs) don't implement 'flow' yet)

I've added it in https://github.com/mdn/data/pull/30.

> BTW, there is a word "display-value" in bold as the first word in the Values
> section -
> I don't quite understand what purpose that serves.  Maybe just remove it?

The "display-value" was obviously introduced as a variable for a syntax definition, which got replaced in the meantime.
I've reworked the "Values" section now and grouped the keywords by the value types defined in the spec.

> I'm not sure what the relation is between Syntax and Formal Syntax, is the former
> just an overview of the syntax we support (although some of the values are still
> unsupported)? and the latter is the syntax from the spec?  If so, the latter
> should probably be updated to what the spec says:
> https://drafts.csswg.org/css-display/#the-display-properties

The code block under "Syntax" are syntax examples. I've updated them according to the spec
The "Formal syntax", as you correctly assumed, reflects the definition in the specifications, which gets updated by the PR above.

> Other than it looks good to me.

Ok, thank you for the review! I assume the merge of the PR may still take a bit, because most people are in vacations. Though I already mark this as dev-doc-complete due to your feedback.

I wish you a Happy New Year!

Sebastian