783415 - Make all child elements of a flexbox have their "display" values compute to blockified forms

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

The CSS3 Flexbox ED now says:
 # each child of a flex container becomes a flex item [...]
 # The computed ‘display’ of a flex item is determined by applying
 # the table in CSS 2.1 Chapter 9.7.
http://dev.w3.org/csswg/css3-flexbox/#flex-items

Filing this bug on converting the "display" property along those lines.

(This means we'll need to know the value of our parent's "display" value in order to compute our own "display" value.  dbaron says we should be able to do this efficiently by adding a new node near the top of the rule tree, similar to what we do for :first-line and :first-letter.)

Comment 1

[Daniel Holbert [:dholbert]]

NOTE: After this bug is fixed, we'll be able to simplify some of the nsCSSFrameConstructor code about forming anonymous flex items.  There's a bunch of code there to coalesce runs of contiguous inline content into anonymous flex items. But after this bug is fixed, there should only be one type of inline content that we'll encounter as a child of a flexbox -- raw text -- and we can simplify our checks to just look for that.

Comment 2

[Daniel Holbert [:dholbert]]

Alternately: nsStyleContext::ApplyStyleFixups() may be a better way to go. Looking into that now.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reference case

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: wip fix

Pretty sure this takes care of it.  Still need to fix up tests to expect this behavior, though.

(Also: we'll need to tweak nsCSSFrameConstructor to not wrap placeholder-frames in anonymous flex items -- but that's a separate issue.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix existing tests (needs to be landed with 'fix')

This patch fixes the flexbox reftests to adjust to the new behavior.

The patch deletes a few reftests that are no longer relevant, and tweaks other tests whose behavior has changed so that they still pass & test useful things.

(I'll probably fold this into the main patch before I land, so as not to introduce a new intermediate point in our hg-history where reftests fail.)

Comment 9

[Daniel Holbert [:dholbert]]

Linux/Mac Try run: https://tbpl.mozilla.org/?tree=Try&rev=bc2cfea9168a

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this still supposed to apply if there's an out-of-flow child of a flexbox?  (e.g., float or abs pos)  If so, then what we do to mOriginalDisplay might matter.  If not, are we supposed to mess with the display value in that case?

Comment 11

[Daniel Holbert [:dholbert]]

I think out-of-flow children already would've had EnsureBlockDisplay() called, so our added EnsureBlockDisplay call here won't change anything, right?  (And so we won't end up messing with the display value here in those cases.)

Positioned items get EnsureBlockDisplay call here:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#4888
and floated items get one here:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#4900

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suppose that's true.  But if it's not supposed to happen for those items per the flexbox spec, you should at least have a comment explaining that you don't need to check for that condition because it's guaranteed not to change anything.  And if it is supposed to apply to those items, then we need to think about what (if anything) should happen to mOriginalDisplay.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #12)
> I suppose that's true.  But if it's not supposed to happen for those items
> per the flexbox spec [...]

This is indeed the case (it's not supposed to happen for those items). Quoting the spec:
  # each in-flow child of a flex container becomes a flex item
  # [...]
  # The computed ‘display’ of a flex item is determined by applying
  # the table in CSS 2.1 Chapter 9.7.
http://dev.w3.org/csswg/css3-flexbox/#flex-items

So -- out of flow children don't become flex items, so we shouldn't do any flex-item-specific display-value-tweaking. (Side note: this is the same region of text that I quoted in comment 0, except "in-flow" has now been added to it -- presumably to clarify issues similar to this one.)

> [...], you should at least have a comment explaining that you
> don't need to check for that condition

Good point. I'll add that (and perhaps an assertion).

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Added a comment & assertion.

NOTE: I'm only considering abspos/fixedpos elements in the comment & assertion -- *not* floated elements -- because floating doesn't actually pull things out of flow in a flex container.  Per the flexbox spec:
  #  ‘float’ and ‘clear’ have no effect on a flex item.

(Technically, we do let "float" blockify the display value, but we're going to do that anyway, if we're a flex item.  Though -- we might let "float" blockify table-parts right now, and we shouldn't do that, per current spec... That's an existing separate issue, though, and I'd like to track that separately, and perhaps get w3c clarification, since the spec *used* to say that float does affect the "display" value.)

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Though -- we might let "float"
> blockify table-parts right now, and we shouldn't do that, per current
> spec... That's an existing separate issue, though, and I'd like to track
> that separately, and perhaps get w3c clarification, since the spec *used* to
> say that float does affect the "display" value.

(Just to follow up on this -- on further reflection, I think our current behavior on this is reasonable.  I emailed www-style for clarification[1], and Tab agrees with me.[2]
[1] http://lists.w3.org/Archives/Public/www-style/2012Oct/0675.html
[2] http://lists.w3.org/Archives/Public/www-style/2012Oct/0677.html
)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: add new mochitest

This patch adds a new mochitest, to go across all the possible "display" values, insert an element with that "display" into a flex container, and check that we convert it to its blockified form as-expected.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 674302 [details] [diff] [review]
fix v2

Is there a bug filed on our behavior not matching what the current spec says about floats?  If not, could you file one?

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #17)
> Is there a bug filed on our behavior not matching what the current spec says
> about floats?  If not, could you file one?

If this is RE the parenthetical at the end of comment 14 -- I actually think our current behavior on floats is correct, per comment 15. (Let me know if that's not what you're talking about.)

Comment 19

[Daniel Holbert [:dholbert]]

Landed fix + test-fixes:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/dea90afc3fe5
and mochitest:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/491c14bbbf3e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dea90afc3fe5
https://hg.mozilla.org/mozilla-central/rev/491c14bbbf3e