requestAnimationFrame is passed incorrect argument

[CreativeTechGuy]

• FakeTimers version : 10.0.0
• Environment : Jest/JSDOM
• Example URL : N/A
• Other libraries you are using: N/A

What did you expect to happen?

The callback passed to requestAnimationFrame should receive an argument which is the number of seconds since time origin (same as performance.now()). MDN Docs

What actually happens

The callback instead receives clock.now which when compared to performance.now() is almost always orders of magnitude higher.

How to reproduce

// Sometime before:
clock.setSystemTime(new Date(2000, 10, 2));
// Test code
let lastFrameTime = performance.now();
window.requestAnimationFrame((time) => {
    console.log("Delta Time:", time - lastFrameTime); // This will log something like 973152000000
});

Suggestion

I know that performance.now() may not always be available. So I propose that this line be changed from:

args: [clock.now + getTimeToNextFrame()],

to

args: [(performancePresent ? clock.performance.now() : clock.now) + getTimeToNextFrame()],

(I am happy to create a PR if you'd like.)

[fatso83]

There might be a bug here, but I think the description here is wrong. You say

The callback passed to requestAnimationFrame should receive an argument which is the number of seconds since time origin

This is not in line with the docs you refer to:

"The callback function is passed one single argument, a DOMHighResTimeStamp similar to the one returned by performance.now(), indicating the point in time when requestAnimationFrame() starts to execute callback functions.

A DOMHighResTimeStamp is not "the number of seconds since time origin", but a millisecond value with high precision.

The DOMHighResTimeStamp type is a double and is used to store a time value in milliseconds.

So it is a high-resolution value in milliseconds denoting the number of secons since time origin, which for most reasons, is the start of the page load.

How would you define time origin in the context of our tests? Should we perhaps just set it to 0 when installing the clock and tick it along with the system clock (or make it a synthetic getter that deduces its value from the system clock)?

[CreativeTechGuy]

Sorry, I used some loose language. To keep it simple, the current performance.now() should be passed to the callback when it is called. All of the handling of DOMHisResTimeStamp is already implemented in this library. It is currently used for hrtime() and performance.now(). So nothing fundamentally new needs to be added to make this work. Time origin is already being tracked and updated properly by the library. The only missing piece is that it isn't being passed as the argument and instead a different time is.

[CreativeTechGuy]

I think the approach of determining the args ahead of time won't work when considering #452. The current implementation of requestAnimationFrame pre-calculates what the time to be passed will be. But if more time has elapsed than 16ms then the wrong value will be passed to the callback. Instead of pre-calculating the args, when calling the callback, at that point it should pass the current value of performance.now(), whatever it happens to be at that time.

So similar to how doTick sets clock.now here it should also set performance.now's value. Then in callTimer here it'd have a conditional to check if the timer is an animation like the following:

if (timer.animation) {
    timer.func.apply(null, [performancePresent ? clock.performance.now() : clock.now]);
} else {
    timer.func.apply(null, timer.args);
}

[CreativeTechGuy]

I believe this is the necessary diff to resolve this issue. Let me know if you'd like me to create a PR! 😄

diff --git a/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js b/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js
index a8f69fc..c7afe20 100644
--- a/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js
+++ b/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js
@@ -742,7 +742,11 @@ function withGlobal(_global) {
         }

         if (typeof timer.func === "function") {
-            timer.func.apply(null, timer.args);
+            if (timer.animation) {
+                timer.func.apply(null, [performancePresent ? clock.performance.now() : clock.now]);
+            } else {
+                timer.func.apply(null, timer.args);
+            }
         } else {
             /* eslint no-eval: "off" */
             const eval2 = eval;
@@ -1226,7 +1230,6 @@ function withGlobal(_global) {
             const result = addTimer(clock, {
                 func: func,
                 delay: getTimeToNextFrame(),
-                args: [clock.now + getTimeToNextFrame()],
                 animation: true,
             });

[fatso83]

That seems great. Please push a PR 👍

[CreativeTechGuy]

@fatso83 Should I include the changes from the related issue (#452) here in this PR too since they go together?

[fatso83]

Do this one first. The other one is a type of breaking, unexpected API change and I am not sure how we like that to look. While this change can be merged now independently

[CreativeTechGuy]

Okay! This PR is out!

(And for the record the other one is a pure addition which has no impact on anything that already exists so it isn't breaking. But yeah let's continue the discussion on that other thread.)