1354879 - stylo: Properly support generated content for display: contents.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

This is needed to pass layout/reftests/css-display/display-contents-generated-content.html

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: 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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(Note that we should respect display: contents in that case, I'll file a spec issue, but that's another matter)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1354879 - Re-enable some tests.

Review commit: https://reviewboard.mozilla.org/r/149368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149368/

Comment 5

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9316add1f17a420d82d19109d2add877591274e

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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).

Comment 7

[Cameron McCormack (:heycam)]

(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.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(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 9

[Cameron McCormack (:heycam)]

Comment on attachment 8877968 [details]
Bug 1354879 - Re-enable some tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149368/diff/1-2/

Comment 10

[Cameron McCormack (:heycam)]

https://github.com/servo/servo/pull/17350

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec746a728e18
Re-enable some tests. r=emilio

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ec746a728e18