STR: * go to http://everlong.org/mozilla/animation/animation2.html with Firefox OS' browser and with a desktop Firefox * trigger the animation on both Expected : * the animation on Firefox OS is the same than the animation on desktop Firefox Actual : * the animation on Firefox OS is broken. We don't even see the text. And the final result is broken as well. If you pan or zoom in or do something on the page, the red square even disappear. This was used to display the install animation on the homescreen (Bug 822201) but this animation will be removed. However, if you click on "1>" to display the list of tabs, and then open the tab again, then the square is correct. :padenot suggests this could be due to Bug 814921, Bug 780692 or Bug 811157 that landed recently on b2g18.

CCing some people who will be able to help.

Does flipping the preference "layers.offmainthreadcomposition.animate-transform" to false make the problem go away?

dbaron> yes, the problem goes away with this preference.

Jeff, can you try and narrow this down then hand it to Nick in a couple of hours when their day starts?

See also Bug 822236, I don't think it is a dup, but definitely related.

I'm 99% sure this a regression from bug 780692. Setting layers.offmainthreadcomposition.throttleanimations=false fixes this, so that implicates 780692/814921. Which is annoying because this and the test case for 822236 are so close to my test cases whilst developing.

I think there are three distinct problems here: starting an animation at scale = 0 is totally broken (as in bug 822236), scaling on the compositor thread from a small scale looks awful because we scale up from a very small number of pixels; at the end of the animation the scale is being reset to its original value rather than remaining at its final value. I'm not sure how related the first two problems are, but possibly very.

To solve the third issue, we need to flush when animations end, like we do with transitions. I have a hack for that, now to try and work out a nice way of doing it.

(In reply to Nick Cameron [:nrc] from comment #8) > To solve the third issue, we need to flush when animations end, like we do > with transitions. I have a hack for that, now to try and work out a nice way > of doing it. To clarify, since there is no fill mode for the animation, when it ends we expect the animation to disappear completely, but it is being set back to the time=0 value.

This patch fixes the third issue described above. There was some discussion of this in Bug 780692 (from dbaron): In ElementAnimations::EnsureStyleRuleFor: >+ // This animation is being handled on the compositor thread, >+ // so we shouldn't interpolate here. >+ if (aIsThrottled) { >+ continue; >+ } So this is an overly complicated way of doing something much simpler. When aIsThrottled is true, instead of starting each iteration of the loop, getting the position, and then continuing, you should just return before the loop (but after setting mStyleRuleRefreshTime, mStyleRule, and mNeedsRefreshes). I think there's something a little subtle broken here, though, which is that you're setting mNeedsRefreshes to false the first time through when you're throttled, which means that the second time through when you're throttled you won't even get to this code because mNeedsRefreshes is false. That suggests that maybe you should be doing even less work, and just taking the early return at the very top of the function when you're throttled. That, in turn, affects whether you set canThrottleTick in FlushAnimations. My gut feeling is that you want: * to take the earliest return in EnsureStyleRuleFor, updating mStyleRuleRefreshTime and not touching mStyleRule or mNeedsRefreshes * to remove the ea->mNeedsRefreshes part of this condition in nsAnimationManager::FlushAnimations >+ bool canThrottleTick = ea->mNeedsRefreshes && aFlags == Can_Throttle && >+ ea->CanPerformOnCompositorThread(CommonElementAnimationData::Flags_None) && >+ CanThrottleAnimation(ea, now); * to remove the "&& !canThrottleTick" addition a few lines below But if this messes things up that work now, we should discuss further. I think the initial patch was broken as pointed out in the above review comment, but in doing that fix we broke the end of animation case.

Comment on attachment 693217 [details] [diff] [review] part 1: fix animation end behaviour >+ IsForElement() && >+ anim.mLastNotification != ElementAnimation::LAST_NOTIFICATION_END) { These two lines aren't quite right. Otherwise the patch is fine. In particular, the && IsForElement() means that you'll be leaving this bug for animations on pseudo-elements. On the other hand, if you remove it, you'll end up executing this code every time for a pseudo-element, once one animation has completed (there might still be others running that could be throttled). Not the worst bug. However, it would be better not to overload mLastNotification here, since that's supposed to be used for events (which we have a bunch of bugs in that we need to fix, so it's likely to change). So r=dbaron with the IsForElement() check removed, if you also add a comment explaining why using anim.mLastNotification here isn't really right. Also, I just realized that OMTA completely breaks animationiteration events. We should probably get a bug filed on that.

(In reply to David Baron [:dbaron] from comment #12) > > However, it would be better not to overload mLastNotification here, since > that's supposed to be used for events (which we have a bunch of bugs in that > we need to fix, so it's likely to change). > > So r=dbaron with the IsForElement() check removed, if you also add a comment > explaining why using anim.mLastNotification here isn't really right. > I'm not clear on what the problem with mLastNotfication is, so I'm finding it hard to write the comment. mLastNotfication is already being used to test for the end of an iteration (line 59/60), is that what is wrong? Or is it wrong to use it here in particular? Thanks!

(In reply to David Baron [:dbaron] from comment #12) > Also, I just realized that OMTA completely breaks animationiteration events. > We should probably get a bug filed on that. Filed bug 822891.

mLastNotification is specifically about what events we have sent. The spec may change (or may already have changed) the rules on what happens when the animation duration or iteration-count changes midway through the animation, in which case we'll want to change mLastNotification to reflect whatever the rules for when we send events are -- maybe changing what it contains, maybe replacing it with something else, etc. Having users of mLastNotification that aren't about the spec's rules for when to send events complicates that.

Comment on attachment 694073 [details] [diff] [review] part 2: use the maximum scale for the container layer Review of attachment 694073 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +2770,5 @@ > } > + // If the scale factors are too small, just use 1.0. The content is being > + // scaled out of sight anyway. > + if (fabs(scale.width) < 1e-8 || fabs(scale.height) < 1e-8) { > + scale = gfxSize(1.0, 1.0); I think this should apply to the GetMaximumAnimatedScale path as well. ::: layout/base/nsLayoutUtils.cpp @@ +238,5 @@ > + aValue.GetCSSValueListValue(), > + aFrame->GetStyleContext(), > + aFrame->PresContext(), dontCare, frameBounds, > + aFrame->PresContext()->AppUnitsPerDevPixel()); > + return gfxSize(transform._11, transform._22); If there's a rotation, this will be wrong. You should do something like what we do in ChooseScaleAndSetTransforms (or something better). There we call CanDraw2D and if so call ScaleFactors on the 2D matrix, otherwise we assume 1,1. (I guess for 3D we could do something smart to calculate the max scale factor anywhere in the quad, but it would be tricky.)

Hey, is this ready to land? I can push it if you want. It's blocking two smoketest-blocking bugs of Gaia.

Tim> I think there must be some changes in the part 1 patch so better wait for :nrc :)

(In reply to Julien Wajsberg [:julienw] from comment #20) > Tim> I think there must be some changes in the part 1 patch so better wait > for :nrc :) Ah, I didn't see that. That's why I asked before doing anything :)

Upgrading to P1/critical as this is blocking a smoketest blocker.

FYI I pinged Nick by mail.

Addressed reviewer's comments, carrying r=dbaron

Addressed reviewer's comments, carrying r=dbaron

I'm having trouble sorting out my SSH, could someone check this in for me please?

could you upload the revised version of part 2 instead of 2 copies of the revised part 1?

er, sorry, I guess there was already a revised part 2; you just obsoleted the wrong patch

(In reply to David Baron [:dbaron] from comment #28) > er, sorry, I guess there was already a revised part 2; you just obsoleted > the wrong patch Thanks for fixing that, sorry for the mess up.

remote: https://hg.mozilla.org/mozilla-central/rev/aad19a7636f3 remote: https://hg.mozilla.org/mozilla-central/rev/ae6237161b6c

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/319694b15e89 https://hg.mozilla.org/releases/mozilla-aurora/rev/ea2b37997426

B2G18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/fe64846a0cd7 https://hg.mozilla.org/releases/mozilla-b2g18/rev/368d0c521410