stylo: Properly support generated content for display: contents.

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: emilio, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
This is needed to pass layout/reftests/css-display/display-contents-generated-content.html
(Reporter)

Comment 1

4 months ago
Created attachment 8856220 [details]
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
(Reporter)

Comment 2

4 months 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

4 months 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
Priority: -- → P2
Blocks: 1243581
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9316add1f17a420d82d19109d2add877591274e
(Assignee)

Updated

a month ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
(Reporter)

Comment 6

a month 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

a month 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

a month 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 :)
(Assignee)

Updated

a month ago
Blocks: 1329119
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a month ago
https://github.com/servo/servo/pull/17350

Comment 11

a month ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec746a728e18
Re-enable some tests. r=emilio
https://hg.mozilla.org/mozilla-central/rev/ec746a728e18
Status: ASSIGNED → RESOLVED
Last Resolved: a month 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.