Closed Bug 1086937 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- unaffected
firefox34 --- verified
firefox35 + verified
firefox36 + verified

People

(Reporter: mstange, Assigned: dbaron)

References

()

Details

(Keywords: regression, Whiteboard: [bugday-20141217])

Attachments

(5 files, 1 obsolete file)

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
[Tracking Requested - why for this release]: CSS animation regression
If you shift-reload, though, it starts to animate in, and then jumps.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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.
Flags: needinfo?(dbaron)
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.
Actually, the dependency is easily avoidable.
I have a patch that I think is ready; I need to write a test, hopefully tomorrow.
Flags: needinfo?(dbaron)
Summary: Overlay navigation menu not animating on http://www.w3.org/conf/2013sf/ → 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
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.
Attachment #8521763 - Flags: review?(birtles)
I confirmed that without patch 2, the third and fourth tests fail
(reporting -1000px), whereas with the patches all 4 tests pass.
Attachment #8521764 - Flags: review?(birtles)
Attachment #8521889 - Flags: review?(birtles) → review+
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.
Flags: needinfo?(dbaron)
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).
Flags: needinfo?(dbaron)
(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 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.
Attachment #8521762 - Flags: review?(birtles) → review+
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.
Attachment #8521763 - Flags: review?(birtles) → review+
Attachment #8521764 - Flags: review?(birtles) → review+
(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 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
Attachment #8521889 - Flags: approval-mozilla-beta?
Attachment #8521889 - Flags: approval-mozilla-aurora?
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
Attachment #8521762 - Flags: approval-mozilla-aurora?
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
Attachment #8521763 - Flags: approval-mozilla-aurora?
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.
Attachment #8521889 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8521762 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8521763 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8521889 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [good first verify]
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]
Whiteboard: [bugday-20141217]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.