1002332 - document.timeline.currentTime produces null for hidden (display:none) subframes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Test case to demonstrate problem

Bug 998246 adds the AnimationTimeline interface and its currentTime member. The implementation, however, is based on the refresh driver which doesn't fire for hidden subframes.

There is nothing in the spec that allows this. For document timelines, currentTime can only be null if the timeline is not attached to a document, or was called before navigationStart etc.

We could, for example, simply return 0 always in this case. That seems somewhat undesirable since animations created to line up with the "current time" will jump well into their duration once the subframe becomes unhidden. That should probably be fixed in the spec but I'm not sure what a good solution would be.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

For the case where the iframe is display:block and then becomes display:none, I thought the best thing to do would be to store the most recent refresh driver time on the timeline and return that while we're display:none and don't have a refresh driver.

However, in implementing this I notice that when we toggle display:none on an iframe we sometimes regenerate the contentDocument.

Specifically, I notice the following behavior:

A)
  Initial: display:none
  Read iframe.contentDocument
  Set display:block
  rAF:
    Read iframe.contentDocument
    Set display:none
    rAF:
      Read iframe.contentDocument (object has changed)

B)
  Initial: display:none
  Read iframe.contentDocument
  Set display:block
  setTimeout(100ms):
    Read iframe.contentDocument (object has changed)
    Set display:none
    setTimeout(100ms):
      Read iframe.contentDocument

Since the contentDocument changes, the attached document timeline also changes and any cached refresh driver time is lost.

So I'm wondering:

a) Have I understood this correctly that we shouldn't expect the identity of contentDocument to be preserved?

b) How about contentWindow? It appears to be preserved.

c) If so, should we actually be defining the animation timeline to be a property of the window? i.e. window.timeline?

We could, alternatively, just leave it as is and not try to keep the most recent refresh driver time but let it reset back to 0 while display:none is true.

Or we could even define document.timeline.currentTime as NaN when the iframe is display:none. We're probably going to make currentTime be NaN for a variety of other circumstances anyway.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> when we toggle display:none on an iframe we sometimes regenerate the contentDocument

That has nothing to do with display:none.  Presumably a load is happening in the frame.  Possibly of the about:blank we kick off by default.  It's hard to say more without seeing your testcase.

> we shouldn't expect the identity of contentDocument to be preserved?

Not across navigations.

> b) How about contentWindow? It appears to be preserved.

contentWindow is the outer window, so it's preserved across navigations, yes.  But the inner window may or may not be preserved across navigations, depending.  And the outer window shouldn't be storing any state, from a JS point of view.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> > when we toggle display:none on an iframe we sometimes regenerate the contentDocument
> 
> That has nothing to do with display:none.  Presumably a load is happening in
> the frame.  Possibly of the about:blank we kick off by default.  It's hard
> to say more without seeing your testcase.

That makes sense. Thanks.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Make AnimationTimeline record the last refresh time and use that when there is no refresh driver

WIP patch

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Make AnimationTimeline record the last refresh time and use that when there is no refresh driver

A document that belongs to an iframe that is display:none as no associated pres
context from which to get a refresh driver. However, in this case
document.timeline.currentTime should still never go backwards (since document
timelines are supposed to be monotonically increasing).

This patch makes AnimationTimeline record the last value returned for the
current time so that if the document becomes display:none we can still return
the most recently used time.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8453509 [details] [diff] [review]
Make AnimationTimeline record the last refresh time and use that when there is no refresh driver

Cancelling the review request for now since I'm really not sure how this should work. Sorry about that.

Initially I decided to leave document.timeline.currentTime as null for an initially display:none iframe to reflect the fact that the animations have never been sampled and the timeline is "not started" in Web Animations terms. However, I'm not entirely sure if this is the correct behavior.

Web Animations doesn't define this yet. It just says times are updated on samples and animated styles are consistent with the current time.

For CSS Animations / Transitions it doesn't matter since we're not going to generate any animations while the document is display:none. However, once we support generating animations by script, if the author forces the start time of the animation player to 0 should it take effect? Perhaps it should. That's more or less the behavior I'm suggesting if the iframe later becomes display:none.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Make AnimationTimeline record the last refresh time and use that when there is no refresh driver

I've rewritten this patch to make document.timeline.currentTime 0 even when the
iframe is initially display:none. I *think* this is the better behavior since
it's closer to what you'd get if the display:none was applied following an
external stylesheet load.

What do you think? The spec doesn't define this either way yet (but I'll fix
that if you agree this is reasonable behavior here).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460026 [details] [diff] [review]
Make AnimationTimeline record the last refresh time and use that when there is no refresh driver

r=me on doing this for now.

In spec terms, I'm not sure what the right behavior is for animation timelines in display:none documents.  It's possible that the answer is "they run as normal"...  We'd have to do some work to implement that, of course.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I'm going to land this without the assertion about the time not going backwards since the refresh driver can currently go backwards when we go in and out of test mode (see bug 1043078).

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9345f9481462

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9345f9481462