Closed
Bug 1354879
Opened 8 years ago
Closed 8 years ago
stylo: Properly support generated content for display: contents.
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: emilio, Assigned: heycam)
References
Details
Attachments
(2 files)
This is needed to pass layout/reftests/css-display/display-contents-generated-content.html
Reporter | ||
Comment 1•8 years ago
|
||
Also, we don't force inline display for display: contents pseudo-elements, which means we get to this assertion[1] pretty easily. Testcase attached.
[1]: http://searchfox.org/mozilla-central/rev/c4fdb67bca7889e59af9dd99c604651a49c4aafa/layout/base/nsCSSFrameConstructor.cpp#4880
Reporter | ||
Comment 2•8 years ago
|
||
(Note that we should respect display: contents in that case, I'll file a spec issue, but that's another matter)
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Note that we should respect display: contents in that case
* IMO
Here's the display fixup that makes Gecko not hit the assert: http://searchfox.org/mozilla-central/rev/c4fdb67bca7889e59af9dd99c604651a49c4aafa/layout/style/nsRuleNode.cpp#6361
Updated•8 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Reporter | ||
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8877968 [details]
Bug 1354879 - Re-enable some tests.
https://reviewboard.mozilla.org/r/149368/#review153934
r=me, with all those comments around :)
::: servo/components/style/matching.rs:44
(Diff revision 1)
> /// closest non-Native Anonymous element in case this is Native Anonymous
> - /// Content.
> + /// Content. display:contents is allowed.
> Normal,
> /// Inherit from the primary style, this is used while computing eager
> /// pseudos, like ::before and ::after when we're traversing the parent.
> - FromPrimaryStyle,
> + /// Also prohibits display:contents from having an effect.
This makes me somewhat sad, but I guess it's ok, and we have better things to worry about than display: contents right now.
Could you add a `TODO` here (feel free to make it `TODO(emilio)`, here and below) saying that display: contents should really apply to `::before` and `::after`?
Presumably with a link to https://github.com/w3c/csswg-drafts/issues/1345
::: servo/components/style/style_adjuster.rs:282
(Diff revision 1)
> }
> }
>
> + /// Native anonymous content converts display:contents into display:inline.
> + #[cfg(feature = "gecko")]
> + fn adjust_for_prohibited_display_contents(&mut self, flags: CascadeFlags) {
Could you add also a `TODO` here saying that presumably we should convert `display: contents` to `display: none` in a bunch of elements (or at least getting the following sorted out), linking to https://drafts.csswg.org/css-display/#unbox?
The Spec says that it computes to display: contents, but acts as display: none, but Safari implemented it changing the computed value to display: none instead.
We do the same for ::before and ::after, which is questionable, but at least we should keep it consistent.
::: servo/components/style/style_adjuster.rs:328
(Diff revision 1)
> - skip_root_and_element_display_fixup);
> self.adjust_for_position();
> self.adjust_for_overflow();
> #[cfg(feature = "gecko")]
> {
> + self.adjust_for_prohibited_display_contents(flags);
I'm assuming you checked that the order matches the one in `ApplyStyleFixups` in Gecko. If it doesn't, please ensure it does (or at least some where the dependencies between properties can't affect the results of style computation).
Attachment #8877968 -
Flags: review?(emilio+bugs) → review+
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> I'm assuming you checked that the order matches the one in
> `ApplyStyleFixups` in Gecko. If it doesn't, please ensure it does (or at
> least some where the dependencies between properties can't affect the
> results of style computation).
The fixup happens earlier than ApplyStyleFixups, in nsRuleNode::ComputeDisplayData. So really it should be first. I checked that the position I added it is OK, although I'd be happy to move it explicitly to be first before everything else.
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #7)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> > I'm assuming you checked that the order matches the one in
> > `ApplyStyleFixups` in Gecko. If it doesn't, please ensure it does (or at
> > least some where the dependencies between properties can't affect the
> > results of style computation).
>
> The fixup happens earlier than ApplyStyleFixups, in
> nsRuleNode::ComputeDisplayData. So really it should be first. I checked
> that the position I added it is OK, although I'd be happy to move it
> explicitly to be first before everything else.
Sounds fine there then :)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years ago
|
||
Comment 11•8 years ago
|
||
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec746a728e18
Re-enable some tests. r=emilio
![]() |
||
Comment 12•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•