1086937 - Overlay navigation menu not animating on http://www.w3.org/conf/2013sf/ due to downloadable font load during animation with an implicit from or to value

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Markus Stange [:mstange]]

The "Sponsors Raffle Schedule Location Videos" navigation overlay in the top left corner of http://www.w3.org/conf/2013sf/ is supposed slide in from the left when the page is loaded. It no longer does that; now it just appears after a delay without any animation.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7037d68868eb&tochange=5839fbd7b8c6

-> bug 1047928

Comment 1

[Markus Stange [:mstange]]

[Tracking Requested - why for this release]: CSS animation regression

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If you shift-reload, though, it starts to animate in, and then jumps.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: partially simplified testcase

I think the problem happens when the font load completes.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And the above testcase works from a file: URL but not http:, because of cross-domain font load restrictions.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: partially simplified testcase

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I thought I knew what the problem was, but my guess was wrong.

In any case, I did at least locally confirm that it was a regression from https://hg.mozilla.org/integration/mozilla-inbound/rev/a1bc385f0ad4 , which is as I suspected.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, my theory was right, but my first fix was wrong.  I have a better fix now, but I think it might depend on some other work that I need to do.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, the dependency is easily avoidable.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I have a patch that I think is ready; I need to write a test, hopefully tomorrow.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

patches are:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/daab14c2ed11/change-animation-phase-descendants
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/daab14c2ed11/use-change-animation-phase

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b476c9224286
https://tbpl.mozilla.org/?tree=Try&rev=b476c9224286

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Add eRestyle_ChangeAnimationPhaseDescendants restyle hint that is like eRestyle_ChangeAnimationPhase, but for a whole subtree

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Use eRestyle_ChangeAnimationPhaseDescendants to get the right style data in RestyleManager::RebuildAllStyleData

Until we get rid of animation phases in bug 960465, we need to ensure
we're producing style data for the correct animation phase.  This makes
this optimization slightly less beneficial until then.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Add test for animations continuing across a user font set update

I confirmed that without patch 2, the third and fourth tests fail
(reporting -1000px), whereas with the patches all 4 tests pass.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 0 - Add missing null check of root element so this patch series doesn't expose a crash in layout/style/crashtests/472237-1.html

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d510eb5366b
https://tbpl.mozilla.org/?tree=Try&rev=7d510eb5366b

Comment 17

[Brian Birtles (:birtles)]

I wonder if you can outline the cause the problem. From what I understand of the test case:

1. We initially trigger an animation running from -1000px to the implicit value of margin-left of 0.
2. While the animation is running we trigger a font load.
3. When the font load completes, we rebuild style data.

   Prior to bug 1047928 we would also do selector matching as part of that, but now we don't. Furthermore, using eRestyle_Subtree (as we did prior to bug 1047928) implied eRestyle_ChangeAnimationPhase so we would also update animation styles at the same time. Now, however, we use eRestyle_ForceDescendants which does *not* cause us to update animation styles and we just keep the animated value we used *before* the font-load trigger rebuilding style data.

That explains why the third test in part 3 fails. What I don't understand is why, after ticking the refresh driver, we don't update animated style at that point. That is, I don't understand why the fourth test fails.

If I get a chance, I'll debug this myself, but any help understanding this would be much appreciated.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the problem is that when we get a restyle, we call nsAnimationManager::CheckAnimationRule, which in turn calls BuildAnimations, which rebuilds the animations.

The problem is that BuildAnimations assumes that the element's current style context is a style without animation.  What the failure to change animation phase causes is that this isn't the case -- the current style context includes the current state of the animation, barely started.

This means that when we recompute the to-value of the animation, which implicitly uses the base value, we use the current state of the animation as the base value.  So we change from animating from -1000px to 0px to instead animating from -1000px to -1000px (or, in the real testcase, change from -400px to 0px to animating from -400px to approximately -385px).

Comment 19

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #18)
> So the problem is that when we get a restyle, we call
> nsAnimationManager::CheckAnimationRule, which in turn calls BuildAnimations,
> which rebuilds the animations.
> 
> The problem is that BuildAnimations assumes that the element's current style
> context is a style without animation.  What the failure to change animation
> phase causes is that this isn't the case -- the current style context
> includes the current state of the animation, barely started.
> 
> This means that when we recompute the to-value of the animation, which
> implicitly uses the base value, we use the current state of the animation as
> the base value.  So we change from animating from -1000px to 0px to instead
> animating from -1000px to -1000px (or, in the real testcase, change from
> -400px to 0px to animating from -400px to approximately -385px).

Thanks, that makes a lot more sense!

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8521762 [details] [diff] [review]
patch 1 - Add eRestyle_ChangeAnimationPhaseDescendants restyle hint that is like eRestyle_ChangeAnimationPhase, but for a whole subtree

>   // If we are restyling this frame with eRestyle_Self, we restyle
>   // children with nsRestyleHint(0).  But we pass the eRestyle_ForceDescendants
>   // flag down too.
>   nsRestyleHint childRestyleHint =
>     nsRestyleHint(aRestyleHint & (eRestyle_Subtree |
>+                                  eRestyle_ChangeAnimationPhaseDescendants |
>                                   eRestyle_ForceDescendants));

The comment here needs to be updated.

>diff --git a/layout/style/nsStyleSet.cpp b/layout/style/nsStyleSet.cpp
>--- a/layout/style/nsStyleSet.cpp
>+++ b/layout/style/nsStyleSet.cpp
>@@ -1355,30 +1355,44 @@ nsStyleSet::RuleNodeWithReplacement(Elem
>                                     nsCSSPseudoElements::Type aPseudoType,
>                                     nsRestyleHint aReplacements)
> {
>   NS_ABORT_IF_FALSE(!(aReplacements & ~(eRestyle_CSSTransitions |
>                                         eRestyle_CSSAnimations |
>                                         eRestyle_SVGAttrAnimations |
>                                         eRestyle_StyleAttribute |
>                                         eRestyle_ChangeAnimationPhase |
>+                                        eRestyle_ChangeAnimationPhaseDescendants |
>                                         eRestyle_Force |
>                                         eRestyle_ForceDescendants)),
>                     // FIXME: Once bug 979133 lands we'll have a better
>                     // way to print these.
>                     nsPrintfCString("unexpected replacement bits 0x%lX",
>                                     uint32_t(aReplacements)).get());
> 
>   // If we're changing animation phase, we have to reconsider what rules
>   // are in these four levels.

Three or four levels?

>-  if (aReplacements & eRestyle_ChangeAnimationPhase) {
>-    aReplacements |= eRestyle_CSSTransitions |
>-                     eRestyle_CSSAnimations |
>-                     eRestyle_SVGAttrAnimations |
>-                     eRestyle_StyleAttribute;
>+  if (aReplacements & (eRestyle_ChangeAnimationPhase |
>+                       eRestyle_ChangeAnimationPhaseDescendants)) {
>+    // Animations are only on elements and on :before and :after
>+    // pseudo-elements.  Style attributes are only on elements and on
>+    // pseudo-elements other than :before and :after, and we assert
>+    // below for pseudo-elements, so we need to avoid adding
>+    // eRestyle_StyleAttribute for pseudo-elements.

This comment seems to suggest that pseudo-elements other than :before and :after can have style attributes but then the following code doesn't permit any pseudo-elements. Maybe I'm parsing the second part "and we assert below for pseudo-elements" incorrectly but I think this could be a little more clear.

The rest looks good.

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8521763 [details] [diff] [review]
patch 2 - Use eRestyle_ChangeAnimationPhaseDescendants to get the right style data in RestyleManager::RebuildAllStyleData

>   // Until we get rid of these phases in bug 960465, we need to skip
>   // animation restyles during the non-animation phase, and post
>   // animation restyles so that we restyle those elements again in the
>   // animation phase.
>   mSkipAnimationRules = true;
>   mPostAnimationRestyles = true;
>+  aRestyleHint |= eRestyle_ChangeAnimationPhaseDescendants;

The comment here explains the first two lines but not the third. If you can think of a suitable comment here for the third line that would be good, but since all this code is going away in the near-ish future I think it's ok without.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles) from comment #20)
> >   // If we're changing animation phase, we have to reconsider what rules
> >   // are in these four levels.
> 
> Three or four levels?

I think it's still four levels.  It's just that in some cases we can guarantee that the before-change and after-change states are the same.

I'll fix up the other comments.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a095f2eaec4d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c54b11c885
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8b31cd8938
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe2aa84c776

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a095f2eaec4d
https://hg.mozilla.org/mozilla-central/rev/e3c54b11c885
https://hg.mozilla.org/mozilla-central/rev/2a8b31cd8938
https://hg.mozilla.org/mozilla-central/rev/dfe2aa84c776

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8521889 [details] [diff] [review]
patch 0 - Add missing null check of root element so this patch series doesn't expose a crash in layout/style/crashtests/472237-1.html

Approval Request Comment
[Feature/regressing bug #]: bug 996796 patch 20
[User impact if declined]: crash
[Describe test coverage new/current, TBPL]: landed on m-c
[Risks and why]: very low risk; it's a null-check that fixes a crash in our test automation once the other patches in this bug land (and probably also a crash in some other real cases)
[String/UUID change made/needed]: no

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8521762 [details] [diff] [review]
patch 1 - Add eRestyle_ChangeAnimationPhaseDescendants restyle hint that is like eRestyle_ChangeAnimationPhase, but for a whole subtree

Approval Request Comment
[Feature/regressing bug #]: bug 1047928
[User impact if declined]: misrendered web pages
[Describe test coverage new/current, TBPL]: landed on m-c; test in automation, confirmed locally to fix problem
[Risks and why]: pretty low risk; it makes a performance optimization that landed be slightly less of an optimization than it was before
[String/UUID change made/needed]: no

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8521763 [details] [diff] [review]
patch 2 - Use eRestyle_ChangeAnimationPhaseDescendants to get the right style data in RestyleManager::RebuildAllStyleData

Approval Request Comment
same as comment 26

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8521889 [details] [diff] [review]
patch 0 - Add missing null check of root element so this patch series doesn't expose a crash in layout/style/crashtests/472237-1.html

dbaron tells me that 34 does need this one fix. Approving this one patch for 34. I'm not marking with Aurora approval as I haven't reviewed the rest of the patches.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ccd3e117f5d

Calling Fx34 fixed for tracking purposes.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0581fc8f02bf
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e5a41c241a27
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/88f4bcea1674
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/700fe4de52c8

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

and https://hg.mozilla.org/releases/mozilla-aurora/rev/751a34f8231f

Comment 32

[Hossain Al Ikram [:ikram] (QA Contact)]

Reproduced with Nightly 2014-10-21 with the instruction from comment 0 and on Windows 7 x64.

Verified as fixed with Firefox 34.0.5 (Build ID: 20141126041045) , Firefox 35 beta 4 (Build ID: 20141216120925) and latest Aurora 36.0a2 (Build ID: 20141217004003) 

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0

[bugday-20141217]