1376245 - stylo: Attach styles explicitly to generated content

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

I've been mulling the tiny traversal problem for a while, but haven't had enough uninterrupted time to lay out a full and general solution.

However, looking at some profiles this evening, I realized that the vast majority of tiny traversals come from generated content (i.e. ::before and ::after) rather than the long tail of arbitrary NAC generated by the frame constructor. So with some special-casing, we can solve the problem quite nicely for those cases. jryans recent patches make this especially easy.

This shaves ~5ms off wikipedia and ~10ms off youtube. A good omen for the week ahead. :-)

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

Comment 1

[Bobby Holley (:bholley)]

Attachment: Hand the already-resolved style directly to servo when creating generated content. v1

This avoids a lot of expensive machinery when we've already computed the style.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8881198 [details] [diff] [review]
Hand the already-resolved style directly to servo when creating generated content. v1

Review of attachment 8881198 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1921,5 @@
>    }
>  
> +  // Servo has already eagerly computed the style for the container, so we can
> +  // just stick the style on the element and avoid an additional traversal.
> +  if (ServoStyleContext* servoContext = aStyleContext->GetAsServo()) {

I'm 99% sure you wanted to do pseudoStyleContext here instead. That may explain pretty much all the failures. If it doesn't fix the transitions issue, it may be because we don't process transitions on the parent, and if we reframe and the old pseudo still had transitions, we'd lose track of them until the next restyle I think.

Comment 3

[Bobby Holley (:bholley)]

Whoops, yeah. That's what I get for writing the patch too fast :-)

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Hand the already-resolved style directly to servo when creating generated content. v3

Hm, try push in comment 4 is still busted. Hiro, what's going on here?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

MayHaveAnimations flag is set after we traversed the target element and processed a SequentialTask for the element. So, in the failure cases, the pseudo element does not have MayHaveAnimations flag yet unfortunately. I can't immediately figure out another way to know the element *will* have animations.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> MayHaveAnimations flag is set after we traversed the target element and
> processed a SequentialTask for the element. So, in the failure cases, the
> pseudo element does not have MayHaveAnimations flag yet unfortunately. I
> can't immediately figure out another way to know the element *will* have
> animations.

Ohh, so this is just because we traverse the first time, discover that it has animations, then post the task (and set the flag)... Then do the animation-only traversal.

I guess we could check whether the style specifies transitions/animations, but that'd break the web animations API, wouldn't it?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> I guess we could check whether the style specifies transitions/animations,
> but that'd break the web animations API, wouldn't it?

Can you do WebAnimations for pseudo-element? I'd think you wouldn't be able to, since you can't reach the generated content in the DOM.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Can you do WebAnimations for pseudo-element?

There's a CSSPseudoElement interface which implements Animatable.  So you don't get access to the actual generated content, but you do get access to an object that represents the generated content and which can be used to animate it.

That said, I do not see any way Gecko currently provides to get your hands on a CSSPseudoElement instance.  We don't implement the CSSPseudoElementList interface or window.getPseudoElements, which seems like the one entrypoint that would hand out CSSPseudoElement instances...  But maybe we plan to implement those sometime?

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #9)
> > Can you do WebAnimations for pseudo-element?
> 
> There's a CSSPseudoElement interface which implements Animatable.  So you
> don't get access to the actual generated content, but you do get access to
> an object that represents the generated content and which can be used to
> animate it.
> 
> That said, I do not see any way Gecko currently provides to get your hands
> on a CSSPseudoElement instance.  We don't implement the CSSPseudoElementList
> interface or window.getPseudoElements, which seems like the one entrypoint
> that would hand out CSSPseudoElement instances...  But maybe we plan to
> implement those sometime?

Brian said we will eventually, but not now, so we don't need to worry about it for now.

I implemented the rule checking thing and it doesn't work. It seems that the rule node for the eagerly-computed ::before style has the animation rules at author level, but nothing at the animation cascade level.

Debugging this, it looks like animation rules don't come by normal selector matching, but rather via the passed-in AnimationRules [1]. And we pass an empty value for such things when computing pseudo rule nodes [2]. How is this supposed to work?

[1] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/servo/components/style/stylist.rs#1133
[2] http://searchfox.org/mozilla-central/rev/1b7b1ec949497e9fc2a9b9dfaccbe894ee2ad5ef/servo/components/style/matching.rs#1232

Comment 11

[Bobby Holley (:bholley)]

Oh, I see how this works. We really need to check the computed values, not the rule node.

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

Attachment: Hand the already-resolved style directly to servo when creating generated content. v4

This avoids a lot of expensive machinery when we've already computed the style.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Emilio and bz are right. We can check specified animation or transition styles instead. And in case of script animations, we currently get pseudo elements via CSS animations, i.e. generating CSS animations on pseudo elements and get the pseudo elements via getAnimations() [1], so we don't need to worrying about the case for now.

[1] https://w3c.github.io/web-animations/#dom-animatable-getanimations

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I did forget to comment an important thing. The thing what I am concerned is that we have already the animation or transition style when we process CreateGeneratedContentItem because I don't know when we process CreateGeneratedContentItem exactly.

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8881357 [details] [diff] [review]
Hand the already-resolved style directly to servo when creating generated content. v4

Review of attachment 8881357 [details] [diff] [review]:
-----------------------------------------------------------------

Animation part looks good to me.

Comment 17

[Brian Birtles (:birtles)]

As Hiro points out, the only way to get a CSSPseudoElement object at the moment is using Document.getAnimations or Element.getAnimations({subtree: true}), neither of which we currently ship, and even when we do, they will only return a CSSPseudoElement object if the element has an animation defined using CSS animations / transitions so it shouldn't affect this case where we're creating the generated content (since script won't have a chance to get this handle and trigger a new animation yet).

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09c6e1a4a6df
Hand the already-resolved style directly to servo when creating generated content. r=emilio,r=birtles

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09c6e1a4a6df