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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(5 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
Alternately: nsStyleContext::ApplyStyleFixups() may be a better way to go. Looking into that now.
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #672496 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
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.)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #672521 -
Attachment is obsolete: true
Attachment #673064 -
Flags: review?(dbaron)
Assignee | ||
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
Linux/Mac Try run: https://tbpl.mozilla.org/?tree=Try&rev=bc2cfea9168a
Comment 10•12 years ago
|
||
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?
Assignee | ||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
(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).
Assignee | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #673064 -
Attachment is obsolete: true
Attachment #673064 -
Flags: review?(dbaron)
Assignee | ||
Comment 15•12 years ago
|
||
(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
)
Assignee | ||
Updated•12 years ago
|
Attachment #673105 -
Attachment description: part 2: fix tests → fix existing tests (needs to be landed with 'fix')
Assignee | ||
Comment 16•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #673105 -
Flags: review?(dbaron) → review+
Comment 17•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #674926 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 18•12 years ago
|
||
(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.)
Assignee | ||
Comment 19•12 years ago
|
||
Landed fix + test-fixes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dea90afc3fe5
and mochitest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/491c14bbbf3e
Flags: in-testsuite+
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dea90afc3fe5
https://hg.mozilla.org/mozilla-central/rev/491c14bbbf3e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•