921317 - [Gaia] Transform Animation in the clock app is wrong

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Clock, defect)

Description

[Mike Pennisi [:jugglinmike]]

Gaia's Clock application uses CSS animations to transition between application "panels" [1]. When running on Gecko version "1.1.0.0-prerelease", this transition was smooth. After updating to "1.3.0.0-prerelease", it is consistently jittery. Notably, the jitter is 100% reproducible, but only occurs when using the "slide-in-right" keyframe (the corresponding "slide-in-left" keyframe continues to animate smoothly).

I have inspected the log messages with the "log slow animations" setting enabled, but I saw no messages related to animation while switching panels.

Device: Unagi

[1] https://github.com/mozilla-b2g/gaia/blob/49e118a2dd514a4a290b6f11ee567969f09b993e/apps/clock/style/views.css

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Milan can you find someone to investigate?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Jet, is this pointing to layout?

Comment 3

[Corey Frang [:gnarf]]

Attachment: attachment.3gp

video capture from

Comment 5

[Diego Wilson [:diego]]

I can reproduce this on on gecko v1.2 as well.

Comment 6

[Jet Villegas (inactive)]

In the dupe bug 922710, comments indicate that this appears correct on unagi devices and on desktop. Layout bugs tend to break on all platforms. Bug 909336 that added these animations landed recently. Can someone with the known good setup (same hardware, Gecko 1.1, Gaia 1.3) confirm?

Comment 7

[Corey Frang [:gnarf]]

Try installing the code for 1.2 clock on a 1.1 device (we used an unagi during development) and note that the animation is correct.  I would not of landed this code if I had seen how jittery it was on a 1.2+ device.  Not sure what changed, but it seems like this is a gecko on the device problem since it's not reproducible in Nightly either.

Comment 8

[Diego Wilson [:diego]]

> Can someone with the known good setup (same hardware, Gecko 1.1, Gaia 1.3) confirm?

Why are people mixing and matching gecko and gaia versions? v1.2 is on it's way to commercialization so a solution on that branch is particularly time sensitive. Is it a question of hardware availability?

Comment 9

[Corey Frang [:gnarf]]

> Why are people mixing and matching gecko and gaia versions? v1.2 is on it's way to commercialization
> so a solution on that branch is particularly time sensitive. Is it a question of hardware availability?

Yes, lack of devices is the problem.  We are still waiting to get newer devices than unagi to develop on at Bocoup.  We still work on the occasional leo+ bug and my device happened to be running 1.1 gecko when I added these animations.  We've since updated to developing on 1.2 / master gecko, but fear with every update that unagi will break.

We only discovered the animation problem when upgrading our devices to 1.2+ gecko, which is why we are assuming this is a problem with gecko, since the animation is not jittery on the 1.1 device, and also not jittery in Firefox Nightly even today.

Comment 10

[Diego Wilson [:diego]]

Are the left-to-right and right-to-left animations identically mirrored? If not, then given that the right-to-left animation looks good maybe that can also be used for left-to-right. At least that'll leave us in good shape in v1.2 while the gecko investigation goes forward.

Comment 11

[Corey Frang [:gnarf]]

https://github.com/mozilla-b2g/gaia/blob/49e118a2dd514a4a290b6f11ee567969f09b993e/apps/clock/style/views.css#L58 is were all the css for the animation is defined.  We did a little extra testing here, made the content less wide so you could see the effect better, and added a red outline, and made the timing much longer (diff being attached)

The attached video should show what we see on an unagi using the v1-train gecko, firefox nightly, and finally the broken animation in master on the device.

Comment 12

[Corey Frang [:gnarf]]

Attachment: visual-comparison.m4v

Comment 13

[Corey Frang [:gnarf]]

Attachment: debug.diff

The changes applied during the visual-comparison recording

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Looks like the only animation that's misbehaving is the "slide-in-right" that goes from 100% to 0.  The others (0->100%, 0->-100% and -100%->0) are ok.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

By the way, not a jitter, just wrong animation results.  You really see it if you make the duration longer.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Really weird, but layout or app.  Making this change in views.css masks the problem (makes it go away).

Bad:
@keyframes slide-in-right {
  from {
    transform: translateX(100%);
  }
  to {
    transform: translateX(0);
  }
}

Good:
@keyframes slide-in-right {
  from {
    transform: translateX(0);
  }
  1% {
    transform: translateX(100%);
  }
  to {
    transform: translateX(0);
  }
}

Also good:
Good:
@keyframes slide-in-right {
  1% {
    transform: translateX(100%);
  }
  to {
    transform: translateX(0);
  }
}

Combined with the comment 14, if the translation is decreasing, and it has not started at 0, we're in trouble.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Can we land this hack for 1.2 and open a separate issue to track the platform issue (if any) ?

Comment 18

[Mike Pennisi [:jugglinmike]]

We could do this to solve the problem in Gaia's Clock application, but since this bug is opened under the "Core::Layout" component, it should remain open until the platform issue is resolved. Ideally, I'd like to see this bug confirmed before we commit to a sub-optimal and application-specific solution, though.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, let's leave it for a couple of days and see if we get the real fix.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

This looks wrong with OMTC animation, but doesn't animate at all (right to left) when OMTC animation is turned off.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

In OMTC case, ignoring scaledOrigin value in AsyncCompositionManager.SampleValue fixes the problem. In the good animations, this is always zero, but in the bad ones, it pushes the panel to the left, which leads to the fight between the two transforms.  I'll see why it shows up as non-zero in the first place.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

At some point, the origin starts being non-zero and causes all these problems.  What's the scenario where we'd expect that to happen, outside of the transform-origin being animated (and it doesn't appear to be explicitly) in the clock app?

Comment 23

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

scaledOrigin looks like it comes from origin, not mozOrigin, where the IPDL fields are:

148 struct TransformData {
149   // the origin of the frame being transformed in app units
150   nsPoint origin;
151   // the -moz-transform-origin property for the transform in css pixels
152   gfxPoint3D mozOrigin;
153   // the -moz-perspective-origin property for the transform in css pixels
154   gfxPoint3D perspectiveOrigin;

(As to that odd naming, I'd have used frameOrigin and transformOrigin instead of origin and mozOrigin.)

So presumably that comes from the |origin| variable in AddAnimationsAndTransitionsToLayer in nsDisplayList.cpp, though that's probably worth checking.  If that's the case, what's the display item for and why does it have an offset to the reference frame?

Comment 24

[Nick Cameron [:nrc]]

I don't know, sorry. I could look into this when I'm back though.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Milan Sreckovic [:milan] from comment #21)
> In OMTC case, ignoring scaledOrigin value in
> AsyncCompositionManager.SampleValue fixes the problem. In the good
> animations, this is always zero, but in the bad ones, it pushes the panel to
> the left, which leads to the fight between the two transforms.  I'll see why
> it shows up as non-zero in the first place.

Why does it push the panel to the left? Is it negative?

Is it possible the element is being laid out so its top-left, without the transform, is not at the top-left of the page?

A display list dump might be useful. MOZ_DUMP_PAINT_LIST=1 in a B2G debug build.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, it's negative, pushing the panel to the left (from 0% to -100%).  It's animating sliding the panel "out of the way", and another one slides from the right to the origin (from 100% to 0%.)  I'll see if I can get the display list dump and check AddAnimationsAndTransitionsToLayer

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mike Pennisi [:jugglinmike] from comment #18)
> We could do this to solve the problem in Gaia's Clock application, but since
> this bug is opened under the "Core::Layout" component, it should remain open
> until the platform issue is resolved. Ideally, I'd like to see this bug
> confirmed before we commit to a sub-optimal and application-specific
> solution, though.

Because of the timing, I would land this workaround and open another bug to find out the actual problem.  Not setup to do gaia returns, if this makes sense, can somebody do it?

app/clock/views.css:

@keyframes slide-in-right {
  1% {
    transform: translateX(100%);
  }
  to {
    transform: translateX(0);
  }
}

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #27)
> (In reply to Mike Pennisi [:jugglinmike] from comment #18)
> > We could do this to solve the problem in Gaia's Clock application, but since
> > this bug is opened under the "Core::Layout" component, it should remain open
> > until the platform issue is resolved. Ideally, I'd like to see this bug
> > confirmed before we commit to a sub-optimal and application-specific
> > solution, though.
> 
> Because of the timing, I would land this workaround and open another bug to
> find out the actual problem.  Not setup to do gaia returns, if this makes
> sense, can somebody do it?
> 
> app/clock/views.css:
> 
> @keyframes slide-in-right {
>   1% {
>     transform: translateX(100%);
>   }
>   to {
>     transform: translateX(0);
>   }
> }

Sounds good to me. Mike or Corey will probably handle that today.

Should they do that in a new bug or should they use this bug?

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

Because this one has enough comments about the real issue, I would use a new bug for this quick fix, and make it block 916966.  We can then koi+ that new bug and leave this one (but not as koi+) for the "real" solution.  We can then also back out this kludge once we have the real solution.

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

The "bad" values for origin get set in nsDisplayTransform::nsDisplayTransform and come from GetOffsetToCrossDoc() call.

Comment 31

[Corey Frang [:gnarf]]

Attachment: workaround v1

I'm not sure that we need another bug number, I can land this patch, uplift it onto 1.2, and put the shas here on this bug to be reverted when gecko fixes the problem.

Is that acceptable?

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Fine with me.

Comment 33

[Mike Pennisi [:jugglinmike]]

Comment on attachment 820366 [details] [diff] [review]
workaround v1

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

Hey Corey,

Could you submit this patch for review on bug 926587 instead? That bug has already been created to track the problem as experienced in Gaia's Clock application. After that, I'll create a new bug to track the removal of this patch (and mark it as dependent on this bug).

Comment 34

[Lucas Adamski [:ladamski]]

Looks like we're close.  This bug blocks partner certification, so lets at least keep this bug updated on progress.

Comment 35

[Dylan Oliver [:doliver]]

Per the plan in comment #29, Corey has landed the workaround fix in bug 926587. 

Milan or m1, I'll leave it to you to remove the koi+ and 916996 dependence on this bug if you're satisfied with that solution for 1.2.

Comment 36

[Michael Vines [:m1] [:evilmachines]]

Hey Diego, can you please follow up with the test folks on this to confirm?   Seems like this should be fine though

Comment 37

[Diego Wilson [:diego]]

The patch in bug 926587 WFM.

Comment 38

[Michael Vines [:m1] [:evilmachines]]

(gr8, - since I koi+'ed it originally)

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Dylan Oliver [:doliver] from comment #35)
> Per the plan in comment #29, Corey has landed the workaround fix in bug
> 926587. 
> 
> Milan or m1, I'll leave it to you to remove the koi+ and 916996 dependence
> on this bug if you're satisfied with that solution for 1.2.

Thanks.  I'm leaving it as needinfo on me on purpose, to make sure it stays flagged for this decision.

Comment 40

[Daniel Holbert [:dholbert]]

:jugglinmike (or others), do you still see this on B2G trunk builds (1.3 prerelease)?

I tried this with my own local B2G build on my Unagi (without custom gaia or gecko; just letting b2g grab what it wants), but I couldn't reproduce.

(Occasionally the animation would miss the first few frames, but I never saw the stuttering "back and forth" type animations shown in Corey's attached video, and the missed-frames that I did see were equally frequent in each direction of the animation.)

Comment 41

[Milan Sreckovic [:milan] (needinfo for best results)]

Right - we landed a workaround in bug 926587, which is masking this problem.  To see it, you need to change https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/style/views.css and replace 0.00001% to 0% at line 69.

Comment 42

[Daniel Holbert [:dholbert]]

Ah, thanks. I can reproduce with that changed.
[re-tagging milan for needinfo, since his comment 41 canceled out his self-request in comment 39]

Comment 43

[Daniel Holbert [:dholbert]]

I debugged this a bit. It turns out the issue is not with the interpolation -- the problem is actually with the animation data's "origin" value, which gives us a supplemental transform that we apply after doing the interpolation.

Specifically, I'm talking about the "origin" value here:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=546d07bdc09c&mark=354-354#337

Some fraction of the time (specifically, when we're experiencing this bug), that origin value has an apparently-bogus negative "x" coordinate. And as a result, we end up translating by that much extra, which leads to the jerkiness reported here.

(I don't know enough about the source of that "origin" value to comment more at this point, but that's what I've got so far.)

Comment 44

[Daniel Holbert [:dholbert]]

(The supplemental transform for "origin" gets applied here, FWIW:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=546d07bdc09c&mark=354-354#378 )

Comment 45

[Daniel Holbert [:dholbert]]

Attachment: gecko hackaround [not intended as actual fix]

If I force origin to be "0,0", as in this patch, I can't reproduce this bug anymore.

(I'm sure that introduces other glitches though -- e.g. the lockscreen thumb-swipe animation renders in the wrong spot at the very end. So, not an actual fix -- just a demonstration of where the problem lies.)

Comment 46

[Daniel Holbert [:dholbert]]

Attachment: (semi-broken version of reduced testcase 1)

This affects desktop Firefox, though it's harder to see in the clock app in desktop b2g since the jiggling is more subtle.

But here's a testcase I reduced -- this makes it super-clear in desktop firefox.

When you click the link in this testcase, the black-bordered div is *supposed* to slide in from the right -- but instead, it mostly stays fixed, and instead the rest of the page slides in from the left.

I think what's basically going on is that we've got two things in play at once:
 (1) We start an animation which (initially) shifts #target offscreen to the right.
...AND SIMULTANEOUSLY:
 (2) We honor the fact that the user just clicked <a href="#target">, which tells us to scroll the viewport such that #target is onscreen.

As a result, we end up effectively pushing the *rest* of the page offscreen to the *left*, so that the offscreen-on-the-right #target is visible.

I think the jiggling behavior in the clock app results from us honoring (1) in the compositor, while we're honoring (2) in the app. (i.e. the app is adjusting its horizontal scroll-position to keep the stopwatch panel in-view, and the compositor thread is simultaneously animating the position of that panel, which produces jerkiness)

Comment 47

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1

(fixed a typo in testcase, so that now you can test this multiple times without reloading)

Comment 48

[Daniel Holbert [:dholbert]]

Attachment: reference case 1a

Here's a reference case, which is identical to the reduced testcase except that I've added a small "animation-delay".  That addition fixes the problem.

Comment 49

[Daniel Holbert [:dholbert]]

Attachment: reference case 1b

Here's an alternate reference case, which is also identical to the reduced testcase, except that in this case I've used the s/from/small-percent/ hackaround that the gaia clock app is currently using (per comment 41 & the patch on bug 926587).

Comment 50

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 2

(here's a slightly simpler variant of the previous reduced testcase)

Comment 51

[Daniel Holbert [:dholbert]]

I actually don't think this is indicative of a gecko bug.

Basically: if a page starts an animation that takes an element offscreen, and simultaneously navigates to #thatElement, it doesn't seem *incorrect* that we immediately scroll the viewport to bring the element back into view.

Opera 12.16 (presto-based) matches Firefox's behavior on the attached reduced testcases, FWIW, and Chrome does something different and more mysterious/odd (after I've added "-webkit" prefixing on all the animation and transform properties).

I think the bottom line here is that the clock app needs to change, so that it either...
 (1) avoids animating elements' position offscreen and simultaneously seeking to those elements
 ...OR, if it can't easily avoid that...
 (2) introduce a small animation-delay to such animations, so that the navigation happens *before* the element is pushed offscreen (so that no scrolling ends up being necessary)

The current "small-percent" hack (currently in use) is a form of option (2), but "animation-delay: 0.01s" would be a more declarative way of representing it.

Comment 52

[Daniel Holbert [:dholbert]]

Also, FWIW: this issue does *not* affect "slide-in-left" because:
 - slide-in-left uses a *negative* transform (i.e. it pushes its target out of the viewport to the left)
 - Content that's transformed into negative territory (outside the viewport to the left) is *not* accessible via scrolling, because we can't scroll "below 0".
 - SO, the navigation to #foo has no effect on the scroll position.

Comment 53

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #51)
> I think the bottom line here is that the clock app needs to change, so that
> it either...
>  (1) avoids animating elements' position offscreen and simultaneously
> seeking to those elements

Actually, (1) is easy to do -- we can just use preventDefault on the click event to prevent the navigation to #foo (and its resulting "scroll-into-view" action) from occurring.

Pull request coming up to do this.

Comment 54

[Daniel Holbert [:dholbert]]

Attachment: pull request: use preventDefault()

(pull request, as described in previous comment)

This fixes the bug for me.

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 8346113 [details] [review]
pull request: use preventDefault()

Looks like mhenretty touched clock last, so I'm tagging him for review. :)

(I'm not sure who can review clock code; Michael, if you don't feel comfortable reviewing and you know someone who'd be more appropriate, could you punt review to them?)

Comment 56

[Daniel Holbert [:dholbert]]

(Reclassifying as a Gaia bug, per comment 51)

Comment 57

[Dylan Oliver [:doliver]]

ni? to jugglinmike to see if he or Corey wants to steal this review -- they might be in a better spot for it than mhenretty.

Comment 58

[Mike Pennisi [:jugglinmike]]

Thanks, Dylan--we talked this over on :dholbert's pull request, and Corey is going to follow up with a slightly different approach.

Comment 59

[Daniel Holbert [:dholbert]]

Comment on attachment 8346113 [details] [review]
pull request: use preventDefault()

As discussed at https://github.com/mozilla-b2g/gaia/pull/14588 , gnarf has an alternate solution that makes the hashchange event responsible for starting the animation. (i.e. preventing the animation from running (and creating an offscreen navigation-target) until we've already handled the navigate)

--> canceling review request and obsoleting this attachment.

Comment 60

[Corey Frang [:gnarf]]

Attachment: github PR

As discussed in the previous pull:

- Allow hashchange to initiate navigation instead of emitting 'selected' event from tabs
- Remove emitter from tabs since it doesn't need to emit anything anymore

Comment 61

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8346810 [details] [review]
github PR

Hey Corey,

I've tested this locally, and everything looks okay. TravisCI is also satisfied, so this is good to land. I'm not sure if you intended for the "revert" commit to be merged separately (don't know if that's an idiom in the Gaia landing process), so I'll wait to merge until I hear from you about that. Or you can feel free to merge it yourself.

Great patch :)

Comment 62

[Corey Frang [:gnarf]]

This is actually one of those times I thought we might want the "revert" commit on its own.  I don't think there is any idiomatic reason, just felt like "the right thing".

revert of bug 926587: https://github.com/mozilla-b2g/gaia/commit/e58b3cdec712e34e28d434caa59238c88b481b2d
master: https://github.com/mozilla-b2g/gaia/commit/87a3467f0a36678f36007ef5b1dc10a6708f21f2