Bobby Holley (:bholley) Reporter
	Description • 7 years ago

See the discussion in bug 1323693. I'll write up a bit more here.

	Bobby Holley (:bholley) Reporter
	Comment 1 • 7 years ago

My plan here is as follows:

First, we encode the pseudo-element for all pseudo-element-implementing NAC directly into the DOM element. This is bug 1331045.

Then, we teach Servo how to recognize pseudo-implementing NAC. We can make this check pretty cheap because it can only happen if NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE (in which case we then check the property table). If this happens, we skip the normal style computation, and instead copy the appropriate pseudo style hanging off the parent's ElementData into the primary style of the child. I think we'll probably want to just copy the pseudo map verbatim from the parent to the child, so that we can support nested pseudo-element implementations in nsNumberControlFrame::CreateAnonymousContent. Boris, is there any spec that actually has nested pseudo-elements and says whether the pseudos should inherit from each other in that way, as opposed to all the pseudos inheriting style from the host element? Or do nested PEs only exist for our internal stylesheets?

Finally, we simplify StyleChildrenIterator to get rid of the guesswork surrounding pseudo-element-implementing-NAC, and just traverse everything. We can then remove special handling for these pseudos in ServoRestyleManager::RecreateStyleContexts, and just rely on the normal content tree traversal to find everything. One tricky bit is the generation of ReconstructFrame change hints, which is illegal in NAC subtrees (see bug 1297857). I guess we'll need to forward any ReconstructFrame hints to the root of the NAC subtree?

We'll also want an optimization to avoid style traversal in servo for NAC subtrees once we get ReconstructFrame, since the NAC will get blown away anyway.

We'll still need special handling for ::first-line and ::first-letter, which aren't NAC-backed. Are there any others?

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 7 years ago

I need to think through this a bit, but my first question is this: are there cases in which we can't figure out where the pseudo-element should be inheriting its style from simply based on its position in the (anonymous) DOM?

	Bobby Holley (:bholley) Reporter
	Comment 3 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> I need to think through this a bit, but my first question is this: are there
> cases in which we can't figure out where the pseudo-element should be
> inheriting its style from simply based on its position in the (anonymous)
> DOM?

The only case that I'm aware of where an nsIAnonymousContentCreator creates trees of depth > 1 is in the number frame stuff [1]. That stuff sets things up so that the styles inherit from the DOM parent (rather than from the root).

However, heycam's patch over in bug 1330843 causes us to inherit from the host element in all cases.

From a complexity standpoint, it's probably marginally better to just inherit from the parent, because then we don't need to walk up the tree and find the host.

From an efficiency standpoint, it's probably marginally better to have all the pseudo-styles inherit from the root, because then we don't need to copy the pseudo styles to the ElementStyles of each pseudo-implementing NAC element.

It's probably not a big deal either way, we just need to pick something and stick with it.

[1] http://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_nsIAnonymousContentCreator%3A%3AContentInfo%3E_2&redirect=false

	Bobby Holley (:bholley) Reporter
	Comment 4 • 7 years ago

Looks like this code at least expects to punch through some number of pseudos up to the host: http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/forms/nsTextControlFrame.cpp#355

So maybe we should always inherit from the host, and change the behavior for the NumberControlFrame stuff?

Curious what jwatt thinks about all this, given that he wrote a bunch of the code in question.

	Bobby Holley (:bholley) Reporter
	Comment 5 • 7 years ago

Though I guess that _also_ raises the question of whether we want to go to the root of the NAC, or all the way up to the first non-NAC element. It looks like the text control frame is explicitly trying to do the latter, unless I'm misunderstanding something. Are there situations where we want the former?

	Bobby Holley (:bholley) Reporter
	Comment 6 • 7 years ago

Bug 1330843 comment 11 reveals some implementation difficulties with the walk-us-out-of-the-nac-subtree approach. These difficulties are non-issue with the stylo backend, but we probably need to settle on some mutually compatible behavior between the two backends, at least for the interim.

It would be really helpful to understand if there's a reason why the NumberControlFrame stuff does the inheritance like it does, and if there's a reason why the TextControlFrame wants to punch through the NAC subtree.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 7 years ago

I haven't paged all this back in yet, but note that what the pseudo-element inherits from is totally observable by styling it with "inherit" values for properties that don't normally inherit, and hence should be defined in the relevant spec that defines the pseudo-element.

	Bobby Holley (:bholley) Reporter
	Comment 8 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> I haven't paged all this back in yet, but note that what the pseudo-element
> inherits from is totally observable by styling it with "inherit" values for
> properties that don't normally inherit, and hence should be defined in the
> relevant spec that defines the pseudo-element.

As far as I can tell, none of the Web-exposed pseudo-elements have have any nesting, nor any of the internal pseudo-elements in Blink [1]. So I think we're kind of on our own here.

The two cases in Gecko when we get nesting are:
(1) When we explicitly set up nesting in the ContentInfo, which only happens in nsNumberControlFrame [2].
(2) When a pseudo-element creates an element that has its own NAC (i.e. the <input> element within nsNumberControlFrame's NAC).

None of the pseudos in (1) are web-exposed, so the difference is only observable internally, and only insofar as any properties from the two container divs might be inherited. A quick scan of [3] and [4] suggests that the only inherited property specified in either div is |writing-mode: horizontal-tb| on the ::-moz-number-spin-box. I'm not sure whether that has any observable effect on the ::-moz-number-spin-{down,up}, but we could certainly just copy it over in pinch.

As for (2), the main interesting case I've seen is bug 1004130, where we added a special case to allow the placeholder pseudo on an <input type="number"> to apply to the NAC within the <input> field. The code added for that bug is pretty specific, but my general sense is that developers expect content-addressable pseudo-elements to apply to things within compound elements, and don't really care much about the structure of the NAC.

So in the general case for a given piece of NAC, I think we want to apply pseudo rules from the originating element (not just from the root of the NAC subtree, given the nested NAC case described above). As for what we _inherit_ from, it probably doesn't matter so much - but the least surprising thing is to inherit from the DOM parent in the NAC subtree, because that avoids totally changing the inheritance behavior when giving a piece a NAC a (possibly-internal) pseudo.

The upshot here is that this _may_ solve the implementation issue in bug 1330843 comment 11, because we can just inherit from the passed-in style context (which should correspond to the DOM parent), assuming ResolvePseudoElementStyle knows how to deal with that.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/ComputedStyleConstants.h?sq=package:chromium&dr&rcl=1484526055&l=58
[2] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/forms/nsNumberControlFrame.cpp#367
[3] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/style/res/forms.css#1087
[4] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/style/res/forms.css#1116

	Bobby Holley (:bholley) Reporter
	Comment 9 • 7 years ago

I just realized that we still need to pass the originating element to ResolvePseudoElementStyle (which is what bug 1004130 was all about), so we still have the implementation problem described in comment 6.

	Bobby Holley (:bholley) Reporter
	Comment 10 • 7 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> I just realized that we still need to pass the originating element to
> ResolvePseudoElementStyle (which is what bug 1004130 was all about), so we
> still have the implementation problem described in comment 6.

Or wait! We don't, because the problem there is that we can't find the _frame_. We can still walk up the DOM tree to find the originating element.

(Sorry, it's a bit late).

	Bobby Holley (:bholley) Reporter
	Comment 11 • 7 years ago

Heycam pointed out that the inheritance is in fact observable with:

input[type=number]::placeholder { color: inherit }

Which screws up my whole proposal on how we can just use normal DOM inheritance within NAC trees.

We discussed this for a bit, and concluded the following:
* The originating element for pseudo elements should always been the first non-NAC ancestor
* The styles of every element in a NAC tree (no matter how deep and nested) should always inherit from the style of the originating element

This brings us back to the implementation issue in comment 6. We decided that we can probably solve it by adding the desire content/stylecontext pair to the frame constructor state at the top of ProcessChildren. I'm going to work on that in bug 1331322.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 12 • 7 years ago

I have a question, which is probably kind of dumb, but I'll drop it here anyway in case it isn't...

I guess that some parts of these can be implemented with Shadow DOM? If so, given that the only complex case is the number frame, should we focus in properly implementing Shadow DOM for Stylo, and potentially rewriting <input type="number"> using it?

It's probably a bit more complex than that, but I'm worried about adding a super-uncommon complex path to the style system and then another similar path for Shadow DOM. Ideally we would be able to use Shadow DOM for most if not all the NAC we have.

WDYT, Bobby?

	Bobby Holley (:bholley) Reporter
	Comment 13 • 7 years ago

On our call, Boris pointed out that another advantage of inheriting all NAC (including non-pseudo-element-implementing NAC) from the originating element is that it would fix this discrepancy:

data:text/html,<input type="text" style="text-decoration: underline" value="1234">
data:text/html,<input type="number" style="text-decoration: underline" value="1234">

(need to run for a bit, will respond to emilio's comment when I get back)

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 7 years ago

So a summary of my thoughts.  

1)  https://drafts.csswg.org/css-pseudo-4/ doesn't say much about how pseudo-elements should inherit, nor do the specs that define them.  I filed https://github.com/w3c/csswg-drafts/issues/953 but assuming that inheritance happens from the originating element unless specified otherwise is a good idea.  And yes, this is totally observable.

2)  Specs really are pretty free to define pseudo-elements as inheriting from anything they please.  ::before/::after/::first-line/::first-letter already have all sorts of inheritance weirdness (and note that the first two of those _are_ NAC-based, but still weird because of the interactions with ::first-line).  nothing stops a spec in the future from defining something other than "inherit from the originating element".  So ideally we would not bake that _too_ hard into our design.  But at the moment, I'm not obviously aware of pseudo-elements that inherit from something else.

3)  I haven't looked into whatever the ::cue stuff and related pieces do in terms of inheritance.

4)  As discussed with Bobby, we can solve the problem mentioned in comment 6 by simply passing in a frame, when we have one, to the innermost ResolveStyleContext function in nsCSSFC.  In the NAC-backed pseudo-element case, we always have one.  We can even make it be the "right" one (walked out of NAC, etc) in the NAC-specific code.  At least assuming none of the relevant frames can end up doing display:contents on us....

5)  We do NOT want to gate this on Shadow DOM.  We just don't.  Especially not the unspecified thing that is "native" or "default" or whatever you want to call it Shadow DOM.  The complexity of dealing with that is pretty much guaranteed to be larger than Bobby's proposal above, plus what we discussed on the call today in terms of passing in frames to get the thing we're inheriting from.

6)  You'll probably need to fix nsPlaceholderFrame::GetParentStyleContext and nsFrame::DoGetParentStyleContext to have the new behavior, so when stylo is off we get the same behavior for dynamic (gecko-side) restyling and initial frame construction.

	Bobby Holley (:bholley) Reporter
	Comment 15 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> I guess that some parts of these can be implemented with Shadow DOM?

As bz said, I think builtin shadow DOM is still pretty far off. In general, the first target for replacement for shadow DOM will probably be the stuff we implement with in-content XBL (like video controls and marquee). Replacing NAC is trickier.

> If so,
> given that the only complex case is the number frame

Well, not entirely. Number frame is the only thing where we get these nested pseudos right now. But we still have tons of other pseudo-implementing NAC that we need to crawl. Right now we don't handle most of that with incremental restyle, and the nice thing about treating all NAC the same is that we mostly get it for free.

> , should we focus in
> properly implementing Shadow DOM for Stylo, and potentially rewriting <input
> type="number"> using it?
> 
> It's probably a bit more complex than that, but I'm worried about adding a
> super-uncommon complex path to the style system and then another similar
> path for Shadow DOM.

There's actually not all that much complexity, since StyleChildrenIterator handles most of the nastiness (and it will get simpler when we stop distinguishing regular vs pseudo-implementing NAC). We just need a few lines of code to check the NAC bit and crawl the parent chain when selecting the parent ElementData.

	Bobby Holley (:bholley) Reporter
	Comment 16 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> 2)  Specs really are pretty free to define pseudo-elements as inheriting
> from anything they please.  ::before/::after/::first-line/::first-letter
> already have all sorts of inheritance weirdness (and note that the first two
> of those _are_ NAC-based, but still weird because of the interactions with
> ::first-line).

What is this interaction?

>  nothing stops a spec in the future from defining something
> other than "inherit from the originating element".  So ideally we would not
> bake that _too_ hard into our design.

I don't think it'll be baked too hard. We can always just special-case the pseudo when crawling the parent chain.

> 3)  I haven't looked into whatever the ::cue stuff and related pieces do in
> terms of inheritance.

https://www.w3.org/TR/2011/WD-html5-20110525/rendering.html#the-::cue-pseudo-element doesn't say anything about inheritance, so it may be undefined. I think Cameron is aware of what's going on in that spec, and he's on board with this plan.

> 4)  As discussed with Bobby, we can solve the problem mentioned in comment 6
> by simply passing in a frame, when we have one, to the innermost
> ResolveStyleContext function in nsCSSFC.  In the NAC-backed pseudo-element
> case, we always have one.  We can even make it be the "right" one (walked
> out of NAC, etc) in the NAC-specific code.  At least assuming none of the
> relevant frames can end up doing display:contents on us....

I don't think I follow this hazard. Can you explain it more?

> 6)  You'll probably need to fix nsPlaceholderFrame::GetParentStyleContext
> and nsFrame::DoGetParentStyleContext to have the new behavior, so when stylo
> is off we get the same behavior for dynamic (gecko-side) restyling and
> initial frame construction.

That's a good point. I'll do that.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 7 years ago

> What is this interaction?

The part of ::before/::after that falls on the first line inherits from ::first-line, and the rest inherits from the originating element.  This is not special to ::before, of course; that's just how ::first-line behaves.

> doesn't say anything about inheritance, so it may be undefined.

Sort of, but it does basically say that the whole thing should be treated as follows: "The document tree against which the selectors are matched is the tree of WebVTT Node Objects rooted at the list of WebVTT Node Objects for the cue."

I would expect that this spec expects inheritance in this tree to be inheritance along its tree structure; the "::cue pseudo-element" stuff is just a nasty hack to effectively inject a scoped stylesheet into this tree, a far as I can tell.  So I guess if we don't actually plan to represent these as pseudo-elements in any real sense they're not a problem.

> I don't think I follow this hazard. Can you explain it more?

The hazard would be if we could somehow construct some NAC for an element, then not construct an actual frame for it, but construct frames for the NAC, because the element is display:contents.

Luckily, construction of NAC stuff goes via the frame, so if the element is display:contents the NAC doesn't get constructed at all, more or less.  To a first approximation.  Insert muttering about <svg:use> here.  ;)

	Bobby Holley (:bholley) Reporter
	Comment 18 • 7 years ago

Sorry for the delay here. I have partial patches, just buried with email and reviews.

	Bobby Holley (:bholley) Reporter
	Comment 19 • 7 years ago

Had some time to work on this yesterday - getting close but still a few failures on try.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 21 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 22 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 23 • 7 years ago

Let's see...

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 24 • 7 years ago

So here's the latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38c6738daf37980b297ac5049d384993a377a64d

It seems green, modulo two tests that currently pass, but I think that they expose a bug we had before. The patches build on top of bug 1355351 so until that lands I can try to debug those further. I can start posting patches for review tomorrow as soon as I take time to clean them up.

These patches take a lot of complexity out of the animation stuff.

In particular, here's the diff between the test expectations then and with these patches... I'll try to get these sorted out, but hiro, do you think it's reasonable to take those? Seem definitely similar to the failures we have already, seems we're not updating animations properly for pseudo-elements when getting the computed style? I have no clue why the tests that fail with my patch are passing right now, any pointers in where to look at would be appreciated.

diff --git a/then.txt b/now.txt
index f79838bf628d..8b57087a1d40 100644
--- a/then.txt
+++ b/now.txt
@@ -189,10 +189,15 @@
     check_transition_value@layout/style/test/test_transitions.html:495:5
     check_pseudo_element_tests@layout/style/test/test_transitions.html:768:9
     @layout/style/test/test_transitions.html:777:1
-188 INFO TEST-PASS | layout/style/test/test_transitions.html | ::before indent test: computed value 0px should be between 0.000000px and 0.000000px at time between 0s and 0s. 
+188 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions.html | ::before indent test: computed value 100px should be between 0.000000px and 0.000000px at time between 0s and 0s. 
+    check_transition_value@layout/style/test/test_transitions.html:495:5
+    check_pseudo_element_tests@layout/style/test/test_transitions.html:771:9
+    @layout/style/test/test_transitions.html:777:1
 189 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions.html | ::after test: computed value 1258px should be between 0.000000px and 0.000000px at time between 0s and 0s. 
     check_transition_value@layout/style/test/test_transitions.html:495:5
     check_pseudo_element_tests@layout/style/test/test_transitions.html:768:9
     @layout/style/test/test_transitions.html:777:1
-190 INFO TEST-PASS | layout/style/test/test_transitions.html | ::after indent test: computed value 0px should be between 0.000000px and 0.000000px at time between 0s and 0s. 
+190 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions.html | ::after indent test: computed value 100px should be between 0.000000px and 0.000000px at time between 0s and 0s. 
+    check_transition_value@layout/style/test/test_transitions.html:495:5
+    check_pseudo_element_tests@layout/style/test/test_transitions.html:771:9
     @layout/style/test/test_transitions.html:777:1

	Hiroyuki Ikezoe (:hiro)
	Comment 25 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> In particular, here's the diff between the test expectations then and with
> these patches... I'll try to get these sorted out, but hiro, do you think
> it's reasonable to take those? Seem definitely similar to the failures we
> have already, seems we're not updating animations properly for
> pseudo-elements when getting the computed style? I have no clue why the
> tests that fail with my patch are passing right now, any pointers in where
> to look at would be appreciated.
> 
> diff --git a/then.txt b/now.txt
> index f79838bf628d..8b57087a1d40 100644
> --- a/then.txt
> +++ b/now.txt
> @@ -189,10 +189,15 @@
>      check_transition_value@layout/style/test/test_transitions.html:495:5
>      check_pseudo_element_tests@layout/style/test/test_transitions.html:768:9
>      @layout/style/test/test_transitions.html:777:1
> -188 INFO TEST-PASS | layout/style/test/test_transitions.html | ::before
> indent test: computed value 0px should be between 0.000000px and 0.000000px
> at time between 0s and 0s. 
> +188 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions.html |
> ::before indent test: computed value 100px should be between 0.000000px and
> 0.000000px at time between 0s and 0s. 
> +    check_transition_value@layout/style/test/test_transitions.html:495:5
> +    check_pseudo_element_tests@layout/style/test/test_transitions.html:771:9
> +    @layout/style/test/test_transitions.html:777:1
>  189 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions.html |
> ::after test: computed value 1258px should be between 0.000000px and
> 0.000000px at time between 0s and 0s. 
>      check_transition_value@layout/style/test/test_transitions.html:495:5
>      check_pseudo_element_tests@layout/style/test/test_transitions.html:768:9
>      @layout/style/test/test_transitions.html:777:1
> -190 INFO TEST-PASS | layout/style/test/test_transitions.html | ::after
> indent test: computed value 0px should be between 0.000000px and 0.000000px
> at time between 0s and 0s. 
> +190 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions.html |
> ::after indent test: computed value 100px should be between 0.000000px and
> 0.000000px at time between 0s and 0s. 
> +    check_transition_value@layout/style/test/test_transitions.html:495:5
> +    check_pseudo_element_tests@layout/style/test/test_transitions.html:771:9
>      @layout/style/test/test_transitions.html:777:1

Thanks for investigating animation stuff!  This result is acceptable to me for now.  But I'd like to debug what's really going on there.  Where can I find all of patch set to reproduce this issue?  I did try to apply the patch in the try upon patches for bug 1355351, but it failed. There is another patch which has 'implemented_pseudo_element()' method?

Also the log noticed me that transitions on pseudo elements work fine (at least this test case).  Failure test cases there are actually the width of parent element of the pseudo element. And the width is affected by '-moz-fit-content', so now I am suspecting '-moz-fit-content' is something broken.

	Hiroyuki Ikezoe (:hiro)
	Comment 26 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)

> Also the log noticed me that transitions on pseudo elements work fine (at
> least this test case).  Failure test cases there are actually the width of
> parent element of the pseudo element. And the width is affected by
> '-moz-fit-content', so now I am suspecting '-moz-fit-content' is something
> broken.

To be clear, I meant 'Failure test cases' is on current stylo.  The new two failures are for pseudo elements.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 27 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> 
> > Also the log noticed me that transitions on pseudo elements work fine (at
> > least this test case).  Failure test cases there are actually the width of
> > parent element of the pseudo element. And the width is affected by
> > '-moz-fit-content', so now I am suspecting '-moz-fit-content' is something
> > broken.
> 
> To be clear, I meant 'Failure test cases' is on current stylo.  The new two
> failures are for pseudo elements.

The test failures are also for pseudos AFAIK. You're right that -moz-fit-content likely affects the results here (good catch!).

I'll put the patches up in this bug today.

	Hiroyuki Ikezoe (:hiro)
	Comment 33 • 7 years ago
mozreview-review

Comment on attachment 8861376 [details]
Bug 1331047: Fix test_animations_event_order.html so that we actually have pseudo-elements.

https://reviewboard.mozilla.org/r/133366/#review136486

Thank you for fixing this!
From the content property spec [1], the initial value is normal and normal computes to none for ::before and ::after and none means pseudo element is not generated.

I am guessing there are other tests rely on this behavior but we will handle later.

[1] https://www.w3.org/TR/CSS21/generate.html#content

::: layout/style/test/test_animations_event_order.html:171
(Diff revision 1)
>  var extraStyle = document.createElement('style');
>  document.head.appendChild(extraStyle);
>  var sheet = extraStyle.sheet;
>  sheet.insertRule('#div0::after { animation: anim 10s }', 0);
>  sheet.insertRule('#div0::before { animation: anim 10s }', 1);
> +sheet.insertRule('#div0::after, #div0::before { ' +

Nit: I think '' is enough for generating pseudo elements. No?

	Hiroyuki Ikezoe (:hiro)
	Comment 34 • 7 years ago
mozreview-review

Comment on attachment 8861377 [details]
Bug 1331047: Fix another instance of a test relying on animating an non-existing pseudo-element.

https://reviewboard.mozilla.org/r/133368/#review136492

::: dom/animation/test/crashtests/1330190-2.html:10
(Diff revision 1)
>  @keyframes anim {
>  }
>  
>  #o_0:before {
>    animation: anim 10s;
> +  content: " ";

Likewise here.

	Bobby Holley (:bholley) Reporter
	Comment 35 • 7 years ago
mozreview-review

Comment on attachment 8861373 [details]
Bug 1331047: Return the correct flattened tree parent for ::before and ::after of the root element.

https://reviewboard.mozilla.org/r/133360/#review136508

	Hiroyuki Ikezoe (:hiro)
	Comment 36 • 7 years ago
mozreview-review

Comment on attachment 8861375 [details]
Bug 1331047: Implement the new traversal semantics for stylo.

https://reviewboard.mozilla.org/r/133364/#review136526

This looks basically fine to me, but a problem what I found is that we don't call process_animations for pseudo elements at all.  I think there are some cases that we need to call process_animations for pseudo elements. For example, when setting transition-property against a pseudo element which has *already* generated.  I guess this is the reason why some test cases in test_transitions.html fail with this patch.

::: layout/style/ServoBindings.h:192
(Diff revision 1)
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetExtraContentStyleDeclarations(RawGeckoElementBorrowed element);
>  
>  // Animations
>  bool
> -Gecko_GetAnimationRule(RawGeckoElementBorrowed aElement,
> +Gecko_GetAnimationRule(RawGeckoElementBorrowed aElementOrPseudo,

Nice name. Let's use this |aElementOrPseudo| in ServoBindings.cpp.

::: servo/components/style/gecko/wrapper.rs:428
(Diff revision 1)
>                        cascade_level: CascadeLevel)
>                        -> Option<Arc<Locked<PropertyDeclarationBlock>>> {
> -    let atom_ptr = PseudoElement::ns_atom_or_null_from_opt(pseudo);
> +    // FIXME(emilio): This is quite dumb, why an RwLock, it's local to this
> +    // function?
> +    //
> +    // Also, we should try to reuse the PDB, to avoid creating extra rule nodes.

The reason why I did not reuse PDB is that PDB is using Vec<> to store each property values. This hash map repeatedly gets/puts the same property.

::: servo/components/style/gecko/wrapper.rs:753
(Diff revision 1)
> -        if old_values.is_none() {
> -            return false;
> -        }
> +        let old_values = match old_values {
> +            Some(v) => v,
> +            None => return false,

Smart! :-)

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 37 • 7 years ago
mozreview-review-reply

Comment on attachment 8861375 [details]
Bug 1331047: Implement the new traversal semantics for stylo.

https://reviewboard.mozilla.org/r/133364/#review136526

(Just to capture IRC conversation):

```
01:52 <emilio> hiro: ping?
01:52 <hiro> emilio: hi!
01:54 <emilio> hiro: you mention in your reviews in bug 1331047 that we don't call process_animations on a generated pseudo? That's wrong AFAICT.
01:54 <firebot> https://bugzil.la/1331047 — NEW, emilio+bugs@crisal.io — stylo: Teach servo how to restyle pseudo-implementing NAC
01:54 <hiro> emilio: oh?
01:54 <hiro> emilio: where do we call process_animations?
01:54 <emilio> hiro: we'd traverse the generated pseudo as a normal element, then call process_animations on it.
01:56 <emilio> hiro: let me find it real quick... we have if pseudo.is_none() && !context.shared.traversal_flags.for_animation_only() { self.process_animations() }
01:56 <hiro> emilio: oh, during the traversal for pseudo,  Option<&PseudoElement> is none?  it's really confusing..
01:56 <emilio> hiro: it is, I should rename that to eager_pseudo
01:57 <emilio> hiro: note that there's an assertion though: debug_assert!(pseudo.is_none() || self.implemented_pseudo_element().is_none()
01:57 <hiro> emilio: OK, but when I build your patches locally, it seems that transitions on pseudo element are not triggered at all
01:58 <emilio> hiro: I think I definitely tested some cases even manually, I'll double-check though, perhaps I'm messing something up
```

TL;DR: Still some debugging to do, yay

	Bobby Holley (:bholley) Reporter
	Comment 38 • 7 years ago
mozreview-review

Comment on attachment 8861375 [details]
Bug 1331047: Implement the new traversal semantics for stylo.

https://reviewboard.mozilla.org/r/133364/#review136524

r=me with those fixes on the non-animation bits. I didn't really look at the animation stuff.

With this, it seems like we can stop manually resolving pseudo styles from Gecko when we create the NAC. File a followup to do that work from servo and simplify the code?

Also, I assume we still need to handle the degenerate ::before/::after case, and handle ::first-line/::first-letter, right?

::: layout/style/ServoBindings.cpp:422
(Diff revision 1)
> +static nsIAtom*
> +PseudoTagAndCorrectElementFor(const Element*& aElementOrPseudo) {

Can you rename this to make it clearer that it's specific to animations? The fact that it's just for ::before and ::after means that it's not really useful for other stuff.

::: servo/components/style/data.rs:535
(Diff revision 1)
>          debug_assert!(self.get_restyle().map_or(true, |r| r.snapshot.is_none()),
>                        "Traversal should have expanded snapshots");
>          self.styles = Some(styles);
>      }
>  
> +    /// Set the computed element rules, and returns whether the rules changed.

Nit: "Sets".

::: servo/components/style/data.rs:536
(Diff revision 1)
>                        "Traversal should have expanded snapshots");
>          self.styles = Some(styles);
>      }
>  
> +    /// Set the computed element rules, and returns whether the rules changed.
> +    pub fn set_rules(&mut self, rules: StrongRuleNode) -> bool {

Seems like we should call this set_primary_rules, no?

::: servo/components/style/matching.rs:583
(Diff revision 1)
> +            // FIXME(emilio): We have more element-backed stuff, and this is
> +            // redundant for them right now.
> +            if cfg!(feature = "servo") ||
> +               pseudo.map_or(true, |p| !p.is_before_or_after()) {

Make sure there's a bug on file for this?

::: servo/components/style/matching.rs:748
(Diff revision 1)
> +    ///
> +    /// FIXME(emilio): Damage for non-::before and non-::after element-backed
> +    /// pseudo-elements should be refactored to go on themselves (right now they
> +    /// do, but we apply this twice).

And reference that bug here.

::: servo/components/style/matching.rs:879
(Diff revision 1)
> +    ///
> +    /// Returns whether the traversal should stop immediately, because a
> +    /// previous ::before or ::after pseudo-element has stopped matching.

This doesn't make any sense to me (why would the traversal stop immediately if an pseudo started or stopped matching?), and the function doesn't return a bool.

::: servo/components/style/stylist.rs:647
(Diff revision 1)
>                   PresentationalHintsSynthetizer,
>                V: Push<ApplicableDeclarationBlock> + VecLike<ApplicableDeclarationBlock>,
>                F: FnMut(&E, ElementSelectorFlags),
>      {
>          debug_assert!(!self.is_device_dirty);
> -        debug_assert!(style_attribute.is_none() || pseudo_element.is_none(),
> +        // Have you heard about ::-moz-color-swatch? Now you have.

Make this comment a bit clearer. :-)

::: servo/components/style/traversal.rs:272
(Diff revision 1)
> -                // destroyed. In that case, we don't need to traverse the subtree.
> +                // destroyed. In that case, we wouldn't need to traverse the
> +                // subtree...
> +                //
> +                // Except if there could be transitions of pseudo-elements, in which
> +                // case we still need to process them, unfortunately.
>                  if el.is_native_anonymous() {
> +                    if el.implemented_pseudo_element().map_or(true, |p| !p.is_before_or_after()) {

So, this whole setup was designed to solve precisely the problem we have now in recalc_style_at (where we have that special case). Instead of doing that, why don't we move that check here (i.e. if we're native anonymous and before/after, check whether the pseudos have been removed from the originating element).

That will avoid the issue in this patch with using the existence of current_element_info to determine whether to cull the traversal, which I object to below.

If you want to fix this some other way, we should talk about it.

::: servo/components/style/traversal.rs:412
(Diff revision 1)
> +        // We bailed out early, so nothing else to do in this subtree.
> +        if thread_local.borrow_mut().current_element_info.is_none() {
> +            return;
> +        }
> +

I don't think we should use the existence of current_element_info to do this signaling here - it adds too much "where are we in the callgraph" state that makes things hard to follow.

See my suggestion above on how to avoid this.

::: servo/components/style/traversal.rs:576
(Diff revision 1)
> +    if let Some(pseudo) = element.implemented_pseudo_element() {
> +        // We have this special-case for the case a parent is reframed and
> +        // we're a ::before or ::after pseudo.
> +        //
> +        // We need to conservatively continue the traversal to style the
> +        // pseudo-element in order to properly process potentially-new
> +        // transitions that we won't see otherwise.
> +        //
> +        // But it may be that we no longer match, so detect that case and
> +        // act appropriately here, by effectively doing no work.
> +        //
> +        // This means that we effectively do nothing in this case, but
> +        // that's fine, because we're going away anyway.
> +        //
> +        // Note that this is inherent to the traversal, not to the one-off
> +        // restyles for getComputedStyle, so we should be good there.
> +        if pseudo.is_before_or_after() {
> +            let parent = element.parent_element().unwrap();
> +            let parent_data = parent.borrow_data().unwrap();
> +            if parent_data.styles().pseudos.get(&pseudo).is_none() {
> +                debug_assert!(data.restyle().damage_handled.contains(RestyleDamage::reconstruct()));
> +                return;
> +            }
> +        }
> +    }

Per above, I would like to handle this in the current place where we check for doomed nac subtrees.

	Bobby Holley (:bholley) Reporter
	Comment 39 • 7 years ago
mozreview-review

Comment on attachment 8861374 [details]
Bug 1331047: Require a child iterator for elements with ::before and ::after pseudos.

https://reviewboard.mozilla.org/r/133362/#review136544

	Hiroyuki Ikezoe (:hiro)
	Comment 40 • 7 years ago

I am still debugging but I noticed that there is a case that pseudo elements are not traversed.  I think this is the case why test_transitions.html fails. So, animation part in attachment 8861375 [details] seems not a problem.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 41 • 7 years ago

I talked with hiro and he didn't apply all the patches.

Here's a testcase that shows the issue. The pseudo animates fine, but since we get the computed style from the EagerPseudoStyles, which don't contain the animation rules, you can't see the transition advance via getComputedStyle.

	Boris Zbarsky [:bzbarsky]
	Comment 42 • 7 years ago

> From the content property spec [1], the initial value is normal and normal
> computes to none for ::before and ::after and none means pseudo element is not generated.

Yes, but what does that mean for animations on it?  In particular, what do browsers actually do?

Given the state of CSS specs, where they often don't consider the various interactions between different specs very carefully (and where things are often not formalized well enough to consider those interactions even if you wanted to), I would be very careful about assuming that something one CSS spec says means anything at all in terms of things another CSS spec defines.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 43 • 7 years ago

I consider this to be a Gecko bug.

Animations don't apply to elements with display: none, and there's no reason they should apply to non-existing pseudo-elements, at all.

I've seen nothing in the animations spec preventing this from happening, but I'd say it's the most sane behavior. It's also consistent with transition events, where if you have:

<style>
#o_0,
#o_0::before {
  color: green;
  transition: color 1s;
}
#o_0:hover,
#o_0:hover::before {
  color: blue;
}
</style>
<div id="o_0">Inner</div>
<script>
document.addEventListener("transitionend", () => {
  alert("animation!");
}, false)
</script>


It'll only trigger _one_ transition event (for the element), but will trigger two if the pseudo-element would have a content property.

Anyway, probably Brian can confirm.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 51 • 7 years ago

Apparently I didn't submit attachment 8861998 [details] the last time. All the try runs definitely were with it btw, so maybe that explains the differences hiro observed (whoops).

Anyway, with that commit stamped, and the last one, the transition tests failing are the pre-existing ones, and we fix all failures in layout/style/test/test_transitions_and_reframes.html, and back to the original number pre bug 1355351 for test_animations.html, which means we fixed the "propagate the style to the frame" bits for pseudos.

So I think with that, comments addressed, and followups filed, this should be ready to go.

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 7 years ago

> I've seen nothing in the animations spec preventing this from happening,
> but I'd say it's the most sane behavior

I actually agree, but "sane" and "web compat" have a tenuous relationship at best.  Hence my question: what do other browsers do?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 53 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #52)
> > I've seen nothing in the animations spec preventing this from happening,
> > but I'd say it's the most sane behavior
> 
> I actually agree, but "sane" and "web compat" have a tenuous relationship at
> best.  Hence my question: what do other browsers do?

Blink has the behavior of Gecko w/ my patch, the following test-case only alerts one time:

<style>
@keyframes anim {
  from { color: green }
  to { color: red }
}
#o_0,
#o_0::before {
  color: green;
  animation: anim 1s;
  transition: color 1s;
}
</style>
<div id="o_0">Inner</div>
<script>
window.onanimationend = () => {
  alert("animation!");
};
</script>

	Boris Zbarsky [:bzbarsky]
	Comment 54 • 7 years ago

OK.  So I tried testing this in various browsers, using that testcase:

1)  Blink (dev build): animationend events have no .pseudoElement prop, but it does fire
    one for the pseudo (on the element itself, making it look like an animation on
    the element) if and only if the pseudo has a non-none 'content'.
2)  WebKit (nightly): same as Blink, afaict.
3)  Edge: No support for animationend events (at least unprefixed).

Given that, sounds like changing the behavior is fine.  Thank you for the testcase!

	Bobby Holley (:bholley) Reporter
	Comment 55 • 7 years ago
mozreview-review

Comment on attachment 8861998 [details]
Bug 1331047: Also traverse native anonymous content in the style system.

https://reviewboard.mozilla.org/r/133978/#review136938

	Hiroyuki Ikezoe (:hiro)
	Comment 56 • 7 years ago
mozreview-review

Comment on attachment 8861375 [details]
Bug 1331047: Implement the new traversal semantics for stylo.

https://reviewboard.mozilla.org/r/133364/#review137024

	Hiroyuki Ikezoe (:hiro)
	Comment 57 • 7 years ago
mozreview-review

Comment on attachment 8861999 [details]
Bug 1331047: Look at the style with animations in ResolveStyleLazily.

https://reviewboard.mozilla.org/r/133980/#review137028

Nice!

	Hiroyuki Ikezoe (:hiro)
	Comment 58 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #54)
> OK.  So I tried testing this in various browsers, using that testcase:
> 
> 1)  Blink (dev build): animationend events have no .pseudoElement prop, but
> it does fire
>     one for the pseudo (on the element itself, making it look like an
> animation on
>     the element) if and only if the pseudo has a non-none 'content'.
> 2)  WebKit (nightly): same as Blink, afaict.
> 3)  Edge: No support for animationend events (at least unprefixed).
> 
> Given that, sounds like changing the behavior is fine.  Thank you for the
> testcase!

I should have commented about other browsers when I reviewed.  Actually I had tested on chrome as well, and send a question to Brian. Once I got an answer from Brian, I will open a new bug for firefox for this pseudo issue. Thank you for taking time.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 59 • 7 years ago

What you're proposing makes sense to me. I suppose we should cancel any running animations if the content property becomes none (and fire a corresponding animationcancel event). Do you mind filing an issue on the spec?

We still need to be able to run animations on pseudos with non-none content, but we don't need to trigger them from style resolution. (That's because it's possible, using the Web Animations API, to grab the CSSAnimation running on a pseudo element and restart it, even after the content property changes. Alternatively, you can take its CSSPseudoElement target and then call animate() on it.)

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #54)
> OK.  So I tried testing this in various browsers, using that testcase:
> 
> 1)  Blink (dev build): animationend events have no .pseudoElement prop, but
> it does fire
>     one for the pseudo (on the element itself, making it look like an
> animation on
>     the element) if and only if the pseudo has a non-none 'content'.
> 2)  WebKit (nightly): same as Blink, afaict.
> 3)  Edge: No support for animationend events (at least unprefixed).

Edge does support unprefixed animationend events, just not for pseudo elements (which I guess means this is a Web-compatible change).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #58)
> I should have commented about other browsers when I reviewed.  Actually I
> had tested on chrome as well, and send a question to Brian. Once I got an
> answer from Brian, I will open a new bug for firefox for this pseudo issue.
> Thank you for taking time.

(If I'm offline, please send me a needinfo or mail. If you just leave a message in an IRC channel I'm likely to miss it.)

	Hiroyuki Ikezoe (:hiro)
	Comment 60 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #59)
> What you're proposing makes sense to me. I suppose we should cancel any
> running animations if the content property becomes none (and fire a
> corresponding animationcancel event). Do you mind filing an issue on the
> spec?

Filed spec issue; https://github.com/w3c/csswg-drafts/issues/1300 ,and our bug 1360059.

> (In reply to Hiroyuki Ikezoe (:hiro) from comment #58)
> > I should have commented about other browsers when I reviewed.  Actually I
> > had tested on chrome as well, and send a question to Brian. Once I got an
> > answer from Brian, I will open a new bug for firefox for this pseudo issue.
> > Thank you for taking time.
> 
> (If I'm offline, please send me a needinfo or mail. If you just leave a
> message in an IRC channel I'm likely to miss it.)

Sure. I will do it next time.

	Boris Zbarsky [:bzbarsky]
	Comment 61 • 7 years ago

> Edge does support unprefixed animationend events, just not for pseudo elements

Hmm.  I thought I tested elements too, but maybe not...

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 62 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #61)
> > Edge does support unprefixed animationend events, just not for pseudo elements
> 
> Hmm.  I thought I tested elements too, but maybe not...

I did verify this locally, but perhaps it's because I'm on Windows Creator's Update. (I thought they supported this from a while back though.)

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 63 • 7 years ago
mozreview-review-reply

Comment on attachment 8861375 [details]
Bug 1331047: Implement the new traversal semantics for stylo.

https://reviewboard.mozilla.org/r/133364/#review136526

> Nice name. Let's use this |aElementOrPseudo| in ServoBindings.cpp.

I'd rather don't (yet at least), because the PseudoTagAndCorrectElementForAnimation uses `aElement` as an inout argument, so once it's called it'd be confused. Happy to fix as a followup returning a std::pair or something if you prefer that.

> The reason why I did not reuse PDB is that PDB is using Vec<> to store each property values. This hash map repeatedly gets/puts the same property.

A property declaration block stores the properties it contains in a bitfield, so putting duplicated properties is linear, but putting non-duplicated ones is faster.

I suggest to reuse the same declaration block, because you'd avoid creating new child rule nodes in the rule tree.

	Pulsebot
	Comment 64 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d7d98905fbb3
Implement the new traversal semantics for stylo. r=bholley,hiro
https://hg.mozilla.org/integration/autoland/rev/f515d9eabe96
Fix test_animations_event_order.html so that we actually have pseudo-elements. r=hiro
https://hg.mozilla.org/integration/autoland/rev/15168855dd54
Look at the style with animations in ResolveStyleLazily. r=hiro

	Pulsebot
	Comment 65 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/99b6054246cd
Fix another instance of a test relying on animating an non-existing pseudo-element. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b3a8acdd984c
Also traverse native anonymous content in the style system. r=bholley
https://hg.mozilla.org/integration/autoland/rev/2cc50d133bbc
Return the correct flattened tree parent for ::before and ::after of the root element. r=bholley
https://hg.mozilla.org/integration/autoland/rev/9fc933936c10
Require a child iterator for elements with ::before and ::after pseudos. r=bholley

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 66 • 7 years ago

Err, that's a messy landing, whoops... I use to split only changes in layout/, but I forgot this bug had also changes in dom/, shrug.

	Pulsebot
	Comment 67 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/472c91312420
Update test expectations. r=emilio

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 68 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d7d98905fbb3
https://hg.mozilla.org/mozilla-central/rev/f515d9eabe96
https://hg.mozilla.org/mozilla-central/rev/15168855dd54
https://hg.mozilla.org/mozilla-central/rev/99b6054246cd
https://hg.mozilla.org/mozilla-central/rev/b3a8acdd984c
https://hg.mozilla.org/mozilla-central/rev/2cc50d133bbc
https://hg.mozilla.org/mozilla-central/rev/9fc933936c10
https://hg.mozilla.org/mozilla-central/rev/472c91312420

	Xidorn Quan [:xidorn] UTC+11
	Comment 69 • 7 years ago
mozreview-review

Comment on attachment 8861375 [details]
Bug 1331047: Implement the new traversal semantics for stylo.

https://reviewboard.mozilla.org/r/133364/#review137574

::: servo/components/style/restyle_hints.rs:117
(Diff revision 2)
>  }
>  
>  #[cfg(feature = "gecko")]
>  impl From<nsRestyleHint> for RestyleHint {
>      fn from(raw: nsRestyleHint) -> Self {
> -        use std::mem;
> +        let raw_bits: u32 = raw.0;

This change seems to break Windows build, because enum uses `c_int` by default for MSVC.

I'm going to submit a patch to mark `nsRestyleHint` based on `uint32_t` explicitly to avoid this kind of issue.