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)

enhancement

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
Attached file test.html
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
(Note that we should respect display: contents in that case, I'll file a spec issue, but that's another matter)
(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
Priority: -- → P2
Assignee: nobody → cam
Status: NEW → ASSIGNED
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+
(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.
(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 :)
Blocks: 1329119
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: