1231619 - CSS Animation not properly timed when using AngularJS animate on Firefox Developer edition and nightly

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[valepu]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151209004008

Steps to reproduce:

This has been discovered on Firefox Developer edition version 44. It is not present on Firefox Standard version 42 (it also doesn't happen in any other browser)
1) have a site using AngularJS, ng-animate, and a CSS animation with a specified delay and opacity, or an animation and a transition-delay property specified
2) Activate the animation

Here's an example: http://plnkr.co/edit/Tr1nd5r0gAyy3eH3Z7Zh?p=preview

This might happen with other combination, if a property not supposed to stay when adding an animation class through ng-animate. But in general i think this might affect any javascript relying on getting a CSS animation duration when said animation has a delay


Actual results:

The animation works fine but once it ends the animated element disappears for exactly half the animation duration, after that it returns to its correct state. This happens because AngularJS's ng-animate removes the animation classes too late (it usually removes the animation classes when the animation ends, but on Firefox Developer version 44 it seems to incorrectly time the animation duration if a delay property is present)

here a more detailed description: http://stackoverflow.com/questions/34181822/animation-with-delay-in-firefox-developer-edition-weird-behavior


Expected results:

ng-animate should remove its animation classes as soon as the animation ends

Comment 1

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 45.0a1 (2015-12-09) and Firefox 44.0a2 (2015-12-09) under Ubuntu 14.04 32-bit and Windows 10 32-bit.
Firefox 43 RC (20151029151421) is not affected.

Last good revision: f8b3d9a3d5771e23cd99f899315831b97c774a92
First bad revision: e13c23f424f664651417fffe914efa7348106249
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f8b3d9a3d5771e23cd99f899315831b97c774a92&tochange=e13c23f424f664651417fffe914efa7348106249


Based on the regression window, this issue was regressed by Bug 1026803

Comment 2

[Boris Zbarsky [:bzbarsky]]

Vasilica, thank you for finding the regression range!

In general, you want to also mark the regression as blocking the bug that caused the regression and request tracking so we don't accidentally ship the problem.  In this case, though, no need to track per se, because this is disabled on release for now.

Brian, can you take a look and see whether this is a bug in our code or an actual web compat problem?

Comment 3

[Boris Zbarsky [:bzbarsky]]

So poking at https://code.angularjs.org/1.4.3/angular-animate.js (used by that testcase), it has code like so:

  function onAnimationProgress(event) {
...
    var ev = event.originalEvent || event;
    var timeStamp = ev.$manualTimeStamp || ev.timeStamp || Date.now();
...
    if (Math.max(timeStamp - startTime, 0) >= maxDelayTime && elapsedTime >= maxDuration) {
...
      animationCompleted = true;
...
  }

Note that this is kind of assuming ev.timeStamp and Date.now() have the same timebase.

Even worse, startTime, the thing timeStamp is being compared to, is set like so:

          startTime = Date.now();

over in the "triggerAnimationStart" function.

So this library definitely depends on event timestamps being in epoch time for this particular event.

onAnimationProgress in this file is used to listen for the following events: webkitTransitionEnd, transitionend, webkitAnimationEnd, animationend.

Looks to me like the AnimationEvent constructor sets mEvent->time to PR_Now, which is in fact epoch time more or less comparable to Date.now().

So this is an actual web compat problem.

Comment 4

[Majid Valipour]

Interesting case. This is also broken on Chrome Canary where we have high-res timestamps.

Based on my tests on FF, the dispatched animation event's timestamp is actually 0 meaning that the logic falls back to setting the timestamp to Date.now().

The are at least two fixes that the application can do:
1- Use Date.now() always and never event timestamp (i.e., what already happens for FF). For Chrome/Safari the current event.timeStamp is pretty much the same as Date.now() so it will not make any difference on the functionality.
2- Use performance.now() instead of Date.now() but that means having a different path for browsers that provide epoch time vs ones that provide high-res time for sometime.

Might be a good idea to reach out to Angular and see if they can do any of these fixes in their future versions. Not sure if anything can be done for older versions in the wild though.

Comment 5

[Rick Byers]

Note that the time bases are different enough that I think it's not unreasonable to feature detect the different here and support both types of timestamps.  Eg. see my code in http://rbyers.github.io/scroll-latency.html.

Personally I don't think this example alone should block our plans to ship DOM highRes event timestamps.  But yes someone should reach out to angular, perhaps submit a pull request with a fix?

Comment 6

[Brian Birtles (:birtles)]

Clearing needinfo since it's clearly a Web compat problem.

Comment 7

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

[Tracking Requested - why for this release]: Web compatibility issue with a well-known library that we need to avoid shipping.

Comment 8

[Brian Birtles (:birtles)]

Note that this is only turned on for non-release channels, and only on Windows and Linux. We have no plans to ship this to release until we do the necessary work for OSX and Android.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Personally I don't think this example alone should block our plans to ship DOM highRes
> event timestamps.

The question is whether it should affect what timebase gets used...  Everyone ships a high-res navigationStart, with a Unix epoch timebase.

Comment 10

[Rick Byers]

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #9)
> > Personally I don't think this example alone should block our plans to ship DOM highRes
> > event timestamps.
> 
> The question is whether it should affect what timebase gets used... 
> Everyone ships a high-res navigationStart, with a Unix epoch timebase.

Right, I meant with the same timebase as Performance.now().  

I'd like to avoid using wall time here (due to issues with NTP skew etc.).  I suppose we could define it as navigationStart + a monotonic timestamp (apparently what NavigationTiming does: http://www.w3.org/TR/2012/REC-navigation-timing-20121217/#mono-clock), but then in cases of NTP skew it would drift away from Date.now(), possibly just leading to more confusion.  But if NavigationTiming is already a precedent here, maybe it's not so bad?

Comment 11

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

(In reply to Rick Byers from comment #10)
> I'd like to avoid using wall time here (due to issues with NTP skew etc.). 
> I suppose we could define it as navigationStart + a monotonic timestamp
> (apparently what NavigationTiming does:
> http://www.w3.org/TR/2012/REC-navigation-timing-20121217/#mono-clock), but
> then in cases of NTP skew it would drift away from Date.now(), possibly just
> leading to more confusion.  But if NavigationTiming is already a precedent
> here, maybe it's not so bad?

Shipping a feature where a developer getting it wrong means their page is intermittently broken on a small percentage of machines seems pretty bad.  Seems like people are likely to get that wrong forever.  It seems better to actually go through a more-breaking change once.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Well, to be fair, Gecko's event.timeStamp handling has been very wrong forever.

Comment 13

[Kohei Yoshino]

Added a note to the relevant site compat doc: https://www.fxsitecompat.com/en-US/docs/2015/event-timestamp-now-returns-domhighrestimestamp-on-nightly-aurora-for-linux/

Comment 14

[Majid Valipour]

FYI, Angular has fixed the issue[1] and it is expected to be in their upcoming 1.5 release.

As far as I understand this issue only affects angular css animations that contain a delay. A possible simple workaround is to remove any delay or add 'animation-fill-mode: forwards' to help the animation bridge the gap. Both of these fixes work fine on the example page.

Also I am trying to better understand how large is the angular css animations usage. My initial attempt was to query for any ranked page that includes 'angular-animate.js' on http-archive (data from 2014/08/15) which returns only a few hits. I will try to see if I can find a better way to get a more accurate read on the usage but so far it does not seem to be that high.


[1] https://github.com/angular/angular.js/pull/13495

Comment 15

[godfreykfc]

Cross posting from https://code.google.com/p/chromium/issues/detail?id=574249#c3. I believe there is a more general form of this problem that is likely to break other sites:

In general, this will be a problem for code that tries to compare event.timeStamp with a Date.now() (or similar) value. Something like this:

```
var delta = Date.now() - event.timeStamp;

if (delta < SOME_CONSTANT) {
  // do something 
} else {
  // event was too old, do something else
}
```

With this change, event.timeStamp will get much, much smaller, so you will always end up in the "else" branch. This is likely to manifest itself when the developer tries to store off the e.timeStamp value for some future comparison.

Again, once we identified the problem, it was an easy fix, but I'm just quite worried about web-compat given the subtle nature of this difference.

One possible fix is to add a fixed base "Date.now()" value (captured on page load) to all high resolution event timestamps. In theory, time-drifts since page load will cause other problems, but that seems much less likely to be a problem in comparison.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> One possible fix is to add a fixed base "Date.now()" value

A better fix on the page side is to just compare to performance.now(), right?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  You meant a fix on the spec/browser side, not on the page side.

Comment 18

[Majid Valipour]

> One possible fix is to add a fixed base "Date.now()" value (captured on page load) to all high resolution
> event timestamps. In theory, time-drifts since page load will cause other problems, but that seems much 
> less likely to be a problem in comparison.

The is essentially the same as #10 and I agree with dbaron that this trades on compat issue with another one which is more subtle and harder to detect. I prefer that we try to use monotonic time first. If the compat issues turn out to be significant, which I doubt, we can always fallback to using a different field.

Comment 19

[godfreykfc]

To be honest, I am a bit surprised that doesn't feel like a significant web-compat issue, but maybe I need to provide enough context to illustrate why this is important.

I'll discuss our code in a bit. However, I think that is a much more generic problem than you think, so I'll start from the general "shape" of the problem and get to the specifics in our usage later. (I also think breaking a modern framework like Angular is in-and-of-itself a pretty big red flag, even if it's a niche feature – usage aside, it at minimum shows that it is a code that developers would reasonably attempt to write.)

* * *

We have a provided a `timeStamp` property on the `Event` object. Since it's just a number, there isn't much you can do with it on its own. The *absolute* time which an event occurred isn't a very interesting piece of information, so you probably aren't echoing that value in the UI.

So what else are you going to use the numeric timestamp for? Probably for some sort of *relative* comparison or computation. But what value are you comparing that against? There are two cases. Either (1) you are comparing that against another event timestamp, or (2) you are comparing that against the wall-clock time (i.e. `Date.now`).

In the first case, as long as you are only comparing against other events, this change doesn't really affect you negatively (it indeed makes it "safer" due to the monotonic nature of the high-res timestamp). There is one case that matters: if you are working around some sort of browser/library bug using `e1.timeStamp === e2.timeStamp`, then the increased precision might screw you, but that would be a separate issue than the one I am discussing here.

In the second case though, as I mentioned above in Comment 15 (and in our code), your code would be completely wrong after this change, because the two numbers you are comparing are now unexpectedly off by a very large "constant".

Why would you want to compare an event timestamp with the wall clock time? Well, there are many reasons. In our case [1], we are drawing a decaying mouse trail, where we have a "mousemove" event listener that draws a circle centered at the coordinates of the event, then increase its radius and decrease its opacity exponentially **over time** to create the "water ripples" effect. You are probably aware that plenty of unmaintained websites built in the 90s have something similar :)

I don't think the algorithm I described here is very specify to our implementation at all. The key here is that you need to compute the "time elapsed" since the original event: it seems impossible to compute this without comparing the timestamp with the "current time".

While it is true that the spec technically did guarantee the value of the epoch, the fact that all browsers uses the same epoch ("UNIX epoch"/`Date.now` epoch) for event timestamps, and that there is no "proper" API to "get me the current value of `e.timeStamp` as if an event has just fired at this moment", it means that everyone have so far been hardcoding the right-hand-side of the comparison (one way or another, directly or indirectly) to `Date.now`. It should then be pretty obvious why shifting the left-hand-side value (`event.timeStamp`) by a big "constant" will break all of these code.

Of course, this is not at all just about silly mouse-trail effects. Any code that needs to compute a "time elapsed since the event" will be broken. This includes things like input debouncing and "manual" animations – as seen in the case of Angular, which is probably orders of magnitude more popular than 90s mouse-trail effects.

In fact, I would go a step further and say that I can hardly imagine any use cases of `event.timeStamp` that does not fall into category (2). In any case, it definitely doesn't feel like a minority use case of `event.timeStamp` to me. If we can verify that usage empirically, that's of course the best. However, I don't really know of a good way to do that, given that it will not produce an error, and just silently break certain UI features that might not even be caught in automated test cases (like auto-complete boxes not working because it is always debounced).

By the way, if you are thinking that code that compares against wall-clock time is fundamentally "incorrect", I totally agree with you, and I am personally looking forward to using this feature. However, correctness is irrelevant here because event timestamps have always been tied to wall-clock time, and there are no better way to do this today. The fact is it basically work "good enough" for the majority cases that people have been able to build useful things on top of it today, and we should be careful not to break them. (For example, popular libraries like underscore uses Date.now for debouncing/throttling – while "incorrect", it obviously works "good enough" to be useful in the wild.)

I also agree that the proposed fix in Comment 15 is not bullet-proof. However, given that the legacy code we are trying to support are already vulnerable to time-drift issues today – and that it's only relevant for said legacy code and does not affect new code at all – I don't know if that's a good reason to not do it. Alternatively, as Majid proposed, using a different field also seems perfectly reasonable as well.

* * *

[1] An old version of the code was deployed here: http://tildeio.github.io/htmlbars/demos/ripples.html, which should exhibit the same issue. The code intermixes `Date.now` and `performance.now`, but that is purely coincidental and unrelated to the core of the problem.

Comment 20

[godfreykfc]

> While it is true that the spec technically did guarantee the value of the epoch

I meant to say "while the spec technically did *not* guarantee :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fwiw, I generally agree with comment 19, with one minor correction: existing events don't all use the UNIX epoch time for .timeStamp.  Even just in Firefox there are some events that do, some events that use a random process-start-like time (the ones setting timeStamp from PR_IntervalNow(), though maybe we've finally killed those off) and the ones always setting 0...  It's a complete mess that really only works sometimes and by accident.  :(

Comment 22

[godfreykfc]

Hey Boris, thanks for the correction!

You are correct that there are some inconsistencies between browsers/platforms/event types, so perhaps it is fundamentally "incorrect" to use the event timestamp for the purposes I described. Perhaps it points to this feature being under-speced, or that the spec could be more clear about its intended uses. (Perhaps the spec can be stricter this time around, since we are revisiting this feature now? Anyhow – that would be beyond the scope of the current discussion.)

But, just like other "incorrect" uses of Date.now() mentioned above, it probably still "works" good enough to be used in the wild – even if it's working "by accident". So I still think we should give the web-compat aspect more consideration. (Again, if there is a way to actually measure it, that seems best, but I'm still not sure how it can be detected.)

FWIW, jQuery tries to "normalizes" some of these differences by assigning events without a timeStamp (i.e. e.timeStamp === 0) to... you guessed it, [`Date.now()`](https://github.com/jquery/jquery/blob/c9935b6d2db9e1be4bed12f7419e98cdca45763e/src/event.js#L574-L575).

Comment 23

[godfreykfc]

Ah, I came across this yesterday and forgot to mention it: both the jQuery API docs and w3schools (unfortunately, still one of the top search engine results) "incorrectly" defines `event.timeStamp` as "The difference in milliseconds between the time the browser created the event and January 1, 1970", so it's probably a pretty widespread misconception.

The MDN definition took only the "Returns the time (in milliseconds since the epoch) at which the event was created." line from the spec without further qualification on what "the epoch" means, so most readers would probably assume it means **The** UNIX epoch.

Stef and I made the same mistake when reading the spec for the first time. It requires some pretty careful reading to notice "the epoch" is actually allowed to be any implementation-defined value.

Comment 24

[godfreykfc]

Forgot the links :)

jQuery API doc: http://api.jquery.com/event.timestamp/
w3school (jQuery): http://www.w3schools.com/jquery/event_timestamp.asp
w3school (JS): http://www.w3schools.com/jsref/event_timestamp.asp
MDN: https://developer.mozilla.org/en-US/docs/Web/API/Event/timeStamp

Comment 25

[Brian Birtles (:birtles)]

As per comment 14, Chrome shows the same behavior for this test case. However, this appears to be fixed from Angular JS 1.4.9 onwards (current stable 1.x version is 1.6.2). So I would say this is the expected behavior if we accept the change to timeStamp.

I'd rather not discuss if such a spec change is Web compatible or not here since I think that discussion belongs in [1] or in the "intent to ship" mail I intend to send for bug 1026804 shortly.

As a result, I'm going to resolve this as invalid since, assuming the spec change goes ahead, this is the intended behavior.

[1] https://github.com/whatwg/dom/issues/23