Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In wake() function, when time flush is called "clock now" variable is calculated wrongly. #17

Closed
dhandaweb opened this issue Oct 23, 2016 · 16 comments

Comments

@dhandaweb
Copy link

dhandaweb commented Oct 23, 2016

Issue is:

Open a html page with d3 transition in browser.
Navigate away from page before transition happen.
Say away until file is loaded.
Come back and you will find the transition will take 10-20 sec to start giving impression that chart is broken.

I found the bug in timeflush() function which is not calculating the right clocknow variable.

function timerFlush() {
  now(); // Get the current time, if not already set.
  ++frame; // Pretend we’ve set an alarm, if we haven’t already.
  var t = taskHead, e;
  while (t) {
    if ((e = clockNow - t._time) >= 0) t._call.call(null, e);
    t = t._next;
  }
  --frame;
}

clocknow should be positive when tab wake up but its not causing next transition call to stop.

Please suggest.

@mbostock
Copy link
Member

mbostock commented Oct 24, 2016

This sounds like the expected behavior, although your description is vague, so I could be misinterpreting. Per the API Reference:

The apparent elapsed time may be less than the true elapsed time if the page is backgrounded and requestAnimationFrame is paused; in the background, apparent time is frozen.

If you start a transition with a delay, and then background the page, time may pause until the page returns to the foreground. For example, if the delay is ten seconds, and you move the page to the background after six seconds, then leave the page in the background for sixty seconds, when you come back the transition may not start for another four seconds. The real elapsed time when the page is returned to the foreground is sixty-six seconds, but the apparent elapsed time used by d3-timer is only six seconds, because most of that time the page was in the background.

It’s actually somewhat messier than this because a setTimeout is used to determine whether requestAnimationFrame callbacks have been paused; see poke. Thus, there’s no guarantee that time is frozen in the background exactly when the page is backgrounded—just that at some point, if requestAnimationFrame callbacks are paused while setTimeout continues to fire at a throttled rate, d3-timer will realize that the page is backgrounded and skew the clock appropriately.

For more on this topic, see #6 d3/d3-transition#29 d3/d3#885 d3/d3#2211.

Here’s the test case I used:

http://bl.ocks.org/mbostock/77ba8fd702bab10deb8afd6e2c99cf4a

If I load the page and quickly background it, and then leave it in the background for a long time, when I return the page to the foreground the fade-to-red transition starts after less than ten seconds of being foregrounded.

If you want me to look at this further, please include a link to a live example on bl.ocks.org, or as a pull request with new unit tests, that demonstrates that the described behavior is not the expected behavior. Use only the minimum amount of code necessary to reproduce the unexpected behavior.

Thank you! 🤗

@chrisfrancis27
Copy link

I'm experiencing this same behaviour on v4.4.0. Whilst I work on creating a reduced test case / PR, I thought I'd share my initial observations. The pattern seems to be as follows:

  1. User loads/refreshes the page
  2. User navigates away from the page before any transitions have been scheduled
  3. In the background tab, the page finishes loading and does some work (fetching async data etc) for, let's say 7500ms
  4. D3 then schedules its first transition and begins calling poke() at regular intervals to keep the clockSkew and clockLast values updated
  5. When the user navigates back to the page, D3 calls sleep() with a timeout roughly equal to the 7500ms

It seems to me that when the transition is scheduled in a background tab, the clockSkew value (which I believe represents the time since window.performance.timing.navigationStart) somehow ends up being used in the sleep() call, resulting in a delayed setTimeout instead of immediately requesting the next available frame.

As I said, I'll try and get an example gist as soon as possible. And thanks always @mbostock for the incredible work. 👍

@mbostock
Copy link
Member

The clockSkew variable roughly equates to the amount of time the page has been sleeping. (It should be a negative value.)

Happy to investigate if you can put a test case on bl.ocks.org for me to look at.

@cbbraun
Copy link

cbbraun commented Nov 28, 2016

@chrisfrancis27 I can confirm that I am having the same problem following the same steps (using 4.4.0 in our product).

I traced it down to timers that were created when the tab is blurred, but they are not scheduled. I had believed this is related to requestAnimationFrame not clearing 'now' when the page is not in focus which caches a old clockNow, started digging and found this bug. Similar to mentioned above, if the data took 40s to load in the blurred tab, animation appears to be delayed by 40s once the page is in focus. This leaves initial chart lines along the axis waiting to animate up to their final position.

I'm putting together a work-around in our product to disable animations when data loads while document.hidden indicates that the page is blurred (what good are they when blurred). After that I can try work on putting an example together that has this problem.

Thanks.

@cbbraun
Copy link

cbbraun commented Nov 28, 2016

OK, so I have a working reproduction case.

My apologies for not being familiar with bl.ocks.org tools, I built the example using jsfiddle based on the example from http://bl.ocks.org/mbostock/3883245

Code: https://jsfiddle.net/yqt1Lyuj/7/

To see it in action (in Chrome):

  1. Go to https://fiddle.jshell.net/yqt1Lyuj/7/show/
  2. Before it loads anything, click away to some other tab you have open.
  3. Wait 15 sec (Fake async is setup to draw at 10 sec).
  4. Click back on tab and observe that the animation hasn't actually kicked off yet. Once the tab is active the animation will be delayed by the 10 sec then animate up from the bottom.

@mbostock
Copy link
Member

These quickest way to create a block is to go to https://gist.github.com and create a new gist with a single file, index.html. (If you want a more detailed walk-through, see Let’s Make a Block.)

@mbostock
Copy link
Member

(I made a gist here: http://bl.ocks.org/mbostock/3a3b50f6569f205bde9859250be073d9)

I’m able to reproduce what you describe, but this seems like the expected behavior? Time is paused when the tab is in the background, so if you schedule a transition with a 10-second delay, move the tab to the background before the transition starts, the 10-second countdown doesn’t resume until the tab returns to the foreground.

@cbbraun
Copy link

cbbraun commented Nov 28, 2016

Thanks for the notes for gist.github.com.

Unfortunately I did not delay the transition.
The 'delay' is the time it took to load the data.
I would not expect the time it took for the data to load to be included in the animation time.

@cbbraun
Copy link

cbbraun commented Nov 28, 2016

Actually, looking into it more, the animation isn't delayed the 'async delay' but some other long delay as indicated by the times in the example when going to http://bl.ocks.org/mbostock/raw/3a3b50f6569f205bde9859250be073d9/

I should have added a log for the tab going into focus.

@mbostock
Copy link
Member

Ah, okay, looking more closely at your code now. I expect the setTimeout is triggering while the page is backgrounded. If the transition is scheduled with no delay, then it should start approximately when the page is returned to the foreground. I could see it being off by a second or two based on the heuristic to detect when timers are paused, but I’ll investigate and see if something else wacky is happening.

@mbostock mbostock reopened this Nov 28, 2016
@mbostock
Copy link
Member

Here’s a smaller reproduction of your test case:

https://bl.ocks.org/mbostock/bbe2ab9e9df59b5ef7cc58fbb660df09

@cbbraun
Copy link

cbbraun commented Nov 28, 2016

FWIW, we have some dashboards where the animations are waiting 40+ seconds (which is what started this investigation). Just a bit beyond heuristics :)

Thanks for looking into this so quickly!

@mbostock
Copy link
Member

My guess is that there’s a cumulative error in the clockSkew where time is delayed by approximately twice the intended amount.

mbostock added a commit that referenced this issue Nov 29, 2016
Fixes #17. The poke interval was relying on the wake callback to record the
last-seen clock time, clockLast. If no timers were run before sleeping (such as
when no timers are run immediately on load, but timers are scheduled during a
timeout), clockLast was still its initial value, zero. Now we set clockLast to
clockNow before scheduling the poke interval, so that it detects skew relative
to the time the poke interval was started.
@mbostock
Copy link
Member

Can you test #18 and see if that fixes it for you? I’ve attached a built copy of d3-timer.zip.

@cbbraun
Copy link

cbbraun commented Nov 29, 2016

Confirmed. I made that one line change to my local d3.js binary and it animates as soon as the tab is focused.

I think it is worth discussing weather or not it should animate while not in focus, but either way the problem is gone.

Thanks so much!

@chrisfrancis27
Copy link

Ha, I'm super late to the party, but I made this example when I was offline so here it is for posterity :)

http://bl.ocks.org/chrisfrancis27/9b97432fb70dae8c54c34e86ebdadaa8

Confirmed @mbostock all fixed with #18
🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants