Closed Bug 1002332 Opened 6 years ago Closed 5 years ago
.timeline .current Time produces null for hidden (display:none) subframes
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.
Summary: document.timeline.currentTime produces null for hidden subframes → document.timeline.currentTime produces null for hidden (display:none) subframes
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.
> 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.
(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.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
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.
Attachment #8452934 - Attachment is obsolete: true
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.
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).
Attachment #8460026 - Flags: review?(bzbarsky)
Attachment #8453509 - Attachment is obsolete: true
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.
Attachment #8460026 - Flags: review?(bzbarsky) → review+
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).
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.