Closed Bug 783415 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(5 files, 3 obsolete files)

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.)
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.
Alternately: nsStyleContext::ApplyStyleFixups() may be a better way to go. Looking into that now.
Attached file testcase (obsolete) (deleted) —
Attached file testcase (deleted) —
Attachment #672496 - Attachment is obsolete: true
Attached file reference case (deleted) —
Attached patch wip fix (obsolete) (deleted) — Splinter Review
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.)
Attached patch fix (obsolete) (deleted) — Splinter Review
Attachment #672521 - Attachment is obsolete: true
Attachment #673064 - Flags: review?(dbaron)
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.)
Attachment #673105 - Flags: review?(dbaron)
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?
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
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.
(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).
Attached patch fix v2 (deleted) — Splinter Review
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.)
Attachment #674302 - Flags: review?(dbaron)
Attachment #673064 - Attachment is obsolete: true
Attachment #673064 - Flags: review?(dbaron)
(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 )
Attachment #673105 - Attachment description: part 2: fix tests → fix existing tests (needs to be landed with 'fix')
Attached patch add new mochitest (deleted) — Splinter Review
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.
Attachment #674926 - Flags: review?(dbaron)
Attachment #673105 - Flags: review?(dbaron) → review+
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?
Attachment #674302 - Flags: review?(dbaron) → review+
Attachment #674926 - Flags: review?(dbaron) → review+
(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.)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 812822
Blocks: 824297
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: