-
Notifications
You must be signed in to change notification settings - Fork 715
[css-contain-2] content-visibility should pause css animations in subtree #5611
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
Comments
Does this include css transitions? I think for consistency it perhaps should, although transitions normally don't have the ability to be paused. Does this include web animations? I think it shouldn't since the web animation's lifetime is not tied to the element[1]. This does add a bit of hysteresis about whether content-visibility elements have been visible in the past. I.e. new content will not be styled so the animations won't be created until the content is first visible, and animations will start whenever any style invalidation is requested. Pausing may make the most sense, but I think it's worth considering all of the options:
|
Also what should happen if the subtree has not yet been visible but there is a forced style update? If we use option 1 / 2, should we create the animation at that point in time? |
content-visibility doesn't actually say that we skip styling, it just tries to set itself up in a way that allows style to be skipped (ie it's a "should" language not "must"). Without any explicit mention of what happens with animations, I think the only possible interpretation is that these animations are created in the same way as if content-visibility was not present. In Chromium, this would currently be a bug. And you're right that on forced style, the animation would be created and start ticking. The intent of pausing the animation is that it allows us to not tick the animation whether it has already been created or not. In turn, I believe this means that we can actually skip style (at least from the animations perspective) |
I don't think that continuing animations while skipped is compatible with avoiding style updates for the subtree. Animations can start at times defined by the next update-the-rendering opportunity after modifying style, and if we're avoiding style recalc for performance reasons we can't say when that animation should have started. I like the option (3) @flackr mentioned, because it's simple and matches what |
Option (1) has an advantage though, in that any existing animation would naturally continue once it came back on-screen. |
Whether animations are created or not is developer exposed through
Is this a reason not to do option 2? I think that option 1 and 2 have all of the same difficulties since playing or paused, animations and their start times are observable.
I think option (3) is my preferred option as well as I think it's the most internally consistent and least likely to have interop issues if/when we change when we can skip style recalcs and has no hysteresis about whether the element had been previously styled. One potential wrinkle I just thought of is the interaction with Element.getAnimations. I assume that getAnimations({subtree: true}) should not enter hidden content-visibility subtrees, but presumably if you call Element.getAnimations on an element within the hidden content-visibility subtree it should because it needs to freshen the style of that element which will create an animation? Alternately perhaps getting all animations in a subtree is another performance pitfall similar to selecting all text? |
What is the behavior of getAnimations({subtree: true}) or Element.getAnimations for display none subtrees? If option 3 is the favorite, then presumably we should do "the same thing"? |
I don't think option 2 is feasible at all, because it would mean we would have to compute style every time it changes, even while skipped, just in case it affected an animation.
Agree about those advantages, but the developer ergonomics might be better with option (1). I say only "might" because the hysteresis may be a problem for some developers due to the new race conditions.
I think it's ok for this to just be another performance pitfall. |
Is it possible to just not tick the animations in a That would mean events / promise callbacks etc. would be delayed indefinitely, or until style resolution was forced, but that seems reasonable? |
Hi @birtles,
Wouldn't not ticking be effectively the same as pausing though? |
No, because when it does resume the intervening time would have elapsed (since the animation time is based on wallclock time, regardless of whether it is ticked or not). Furthermore, the paused-state of the animation is observable using |
+1, between (1) and (2) my preference would be to conceptually continue the animation (essentially (2)) making the hiding not directly observable from the animation's point of view. |
I'm not sure I understand this point. We would tick the animation's time, but we could leave the elements with dirty style unless it was queried. As far as I can tell, all properties which could start or stop animations are not animatable, i.e. animation-* and display are not animatable properties. For composited animations we already only tick the time if something else requests a blink main frame so we can skip frame updates in the same way here. |
The problem is that when the element is skipped, we don't process its style. This, in turn, means that if script modifies the style in such a way that some element in the subtree now has an animation, then we don't know about this unless we always keep style clean. This is also true if script modifies an element in such a way that, say, increases its animation duration, or adds animation-play-state: paused, or changes any other animation-affecting style. If we don't have up-to-date style, we can only continue ticking an animation as we knew it at the time we started skipping the element (or, even worse, at the last time style was forced). This is why option 2 seems like a poor fit here. |
Option 1 has the same challenge though right? We'll assume the animation is still there even if there is a style change? |
If the animation is paused then there is nothing that can occur before we process style though, right? For instance, we wouldn't get an animationend event or anything along those lines. If the style is forced (to get the animation object), or if we start rendering (thus unpausing the animation), we would have up-to-date style at that time, so we should act on the latest information (at least that was the intent of my proposal to pause the animation) |
We should chat, I must still be missing something. This seems to me like it is the same regardless of whether we pause the animation or conceptually let time advance. We don't need to freshen style just for ticking animations, but in either case getting the animation would freshen it (processing pending css changes which may stop the animation). |
After chatting with @vmpstr and @chrishtr I've learned that the main goal is ensuring that when engines perform style updates in content-visibility hidden subtrees that we do not have timing dependent side effects. I.e. setting element.style.animationDuration may result in different behavior depending on when a hidden subtree cleans style. However, even if animations are paused changes to display and animation-name would have observable side effects based on when style invalidations happened unless we defer updating runing animations. To this end I believe there are two necessary changes to achieve this behavior (which I think @birtles may have been alluding to):
When the subtree becomes visible again, we would then process animation updates, creating new animations, modifying existing animations and deleting old animations. Note: We should probably update the starting of transitions to define that they should not start at this time, which I believe is consistent with elements which were display: none. Otherwise making a subtree with transition properties visible could cause many unintended transitions to start (and we would have to maintain the pre-content-visibility hidden styles). Orthogonally, we can decide what time animations resume at when they become visible again. I believe we have all agreed that it makes the most sense for them to keep their old start time and jump to the appropriate current time when visible again - as otherwise users would be very aware of at what threshold |
That all makes sense to me, but what is the behavior when the author flushes style on the subtree using script? That is, for the sentence, "CSS animations and transitions within currently invisible content-visibility subtrees do not update their local times until visible." is there an implicit, "unless the author forcibly flushes style" (e.g. with |
When I wrote that I was thinking even for forced flushes from the author, but thinking more about this it could make sense to have an exception for forced style flushes so that getting the computed style doesn't behave differently in and out of these subtrees.
Do you have a preference? |
I don't really know. At a glance making Special-casing |
I suspect it won't be too bad. The other cases that come to mind are other API's like |
My concern with making these changes when style is flushed is the fact that events can be fired when we create an animation, thus revealing exactly when the style is flushed (via a response to script or response to user action or anything else). I think we should avoid this. For things like resize observer, where style and layout changes are observed by events, we specifically say that these events do not happen in a hidden subtree. We either would have to say the same thing for the animations, or not update them on forced styles. The latter would also mean that getAnimations would not get the animations that were deferred because of content-visibility. |
The idea here is that animations would only be created for specific style flush cases (getAnimations, getComputedStyle, etc). We could still flush style internally for other cases without creating animations. |
I'm not too sure how easy it is to distinguish these from within the implementations. For example offsetLeft would cause a style update that is caused by script but that shouldn't flush (IMHO). getComputedStyle, on the other hand, should, but not if it is called from an internal source (which is possible, I think but I don't really know) I still think that we should just spec it as everything is deferred while the element's contents are skipped. But maybe if it's just limited to getAnimations then it could work? It just seems awkward that animation creation events would get fired when there is a getComputedStyle call. |
From an implementation point of view, the API's which are developer requested flushes can for the duration of their execution set some attribute on the state to reflect that style is updating due to a developer requested flush.
offsetLeft requires clean layout in order to compute result which requires flushing style. You could for example have a pending style update (or even animation) which affects the position.
We already do create animations when style is flushed, but events are (as usual) deferred until the next lifecycle update. The advantage of flushing in these cases is that the correct style would be reflected (e.g. the animation's initial position may affect the element's style). |
I'm trying to understand the proposed changes as they relate to the content-visibility spec here. What would be the proposed change roughly speaking (if any)? Is the following a fair summary?
I'll be honest, it sounds a little harder to think about (for me) than other proposals that keep the clocks frozen and prevent any animations from being created or destroyed. Something like the following
Now that I write this out, I think the only difference is that I'm proposing that all these things happen "when the content-visibility no longer skips the contents" vs "when style is cleaned for whatever reason" |
I think the spec could say something like css animations and transitions are not updated and style is not updated in content-visibility: hidden subtrees during the document lifecycle update. Any updates which explicitly force style updates (i.e. getComputedStyle) must already freshen style since they can be run when style is dirty, and any internal freshening of style would not be observable so doesn't need to be spec'd. |
Is this because we wouldn't fire the animation events? If we create animations and whatnot, it would be observable indirectly
I think this is a problematic thing to put in because style needs to be updated in a whole bunch of situations under a content-visibility: hidden. I don't remember off the top of my head if any of them are in document lifecycle (in Blink) though. This would, however, move content-visibility from being a largely a strong hint which can be used for optimization and become a requirement that style optimization occurs in specific scenarios |
Yes, it looks good to me. |
Seems fine to me. |
So, to re-summarize the proposed change is the following: Animation work is skipped in content-visibility subtrees when they are not rendered. Specifically, in the content-visibility unrendered subtree, we avoid creating animations, ticking them, and ending them. If style is updated for any reason, the animation state is 'refreshed' |
The CSS Working Group just discussed
The full IRC log of that discussion<dael> Topic: [css-contain-2] content-visibility should pause css animations in subtree<dael> github: https://github.com//issues/5611 <dael> vmpstr: Hard to talk about this without previous one in mind. This is about skipping css animations in subtrees that are unrendered by content-visibility <dael> vmpstr: Building toward making sure we can skip the animation work if elements are not rendered <dael> vmpstr: Have agreement from a couple people on issue who I believe are animation experts <dael> vmpstr: I think we should still resolve, though we haven't solved the previous issue <dael> astearns: What's the resolution they agree on? <dael> vmpstr: Animation work is skipped in content-visbility subtrees when they're not rendered. creating, taking, or ending animations. Animation is refreshed if recaluclate <dael> astearns: Reasonable to me. Any concerns? <dael> astearns: Objections? <dael> RESOLVED: Animation work is skipped in content- visibility subtrees when they're not rendered. creating, taking, or ending animations. Animation is refreshed if recalculate <emilio> hmm |
Hmm, so how do we plan to deal with animations that would cause That seems like a reasonable use case. |
content-visibility enforces containment, so an animation within a content-visibility: auto subtree wouldnt' be able to move its parent (the element with content-visibility) onscreen would it? |
I guess so, yeah, that's a good point, nevermind then :) |
Rereading the above thread, I'm not 100% clear on whether the animation events should fire when animations are refreshed. The start and end events are obviously relevant, but what about iteration events? Should we fire multiple of them if several iterations occurred between refreshing? |
IMO, we should fire a single animationiteration just as we do if you background a tab running an animation and come back to it, e.g. https://jsbin.com/cacakeq/edit?html,css,js,output |
Ah, excellent, that's a great precedent. Thanks! |
Okay, draft spec text is up; you can check it at 82fc7dd. Does this match up with what y'all are expecting? I did my best to synthesize this thread's consensus into a reasonable whole. Is there anything missing that still needs to be detailed? |
CSS animations level 2 and CSS Transitions level 2 list out the possible events that can be dispatched for each "refresh". It might be better to refer to those definitions since they're a little more thoroughgoing (e.g. they define that if the "refresh" interval entirely covers an active interval then only start and end are fired and all iteration events are skipped). Other than that, it looks good to me. |
@tabatkins the only thing blocking closing this issue is @birtles's comment above, can you take a look? |
Finally updated this, thanks. Heya @birtles, this was delayed on my part because I tried yak-shaving a bunch of broken links. Animations 2 and Transitions 2 were both using anchors block to define a bunch of links as existing in Web Animations, but a lot of those links were busted. Most of them should be updated on a case-by-case basis specifying what they're for (animations or timelines is the main thing needing discrimination), but some were simply busted and I have no idea what they were meant to refer to, such as the term "sampling" which I want to use, but which literally occurs nowhere in the text of Web Anim 1, and once in a header of Web Anim 2, so I can't tell what the term is actually meant to be pointing at. For now I've just deleted the anchors block entirely from both specs: that feature is meant to let you autolink into things that aren't bikeshedded at all. All it was doing was masking errors and making the specs look like they built clean. There's now a bunch of linking errors in both specs which could use your attention. (If all instances of a term do need to go to a specific definition, link-defaults is meant for that; it plays nicely with autolinking rather than masking errors.) |
@tabatkins Thanks. I've had a look and fixed the links in CSS Transitions 2. I'll work on CSS Animations 2 next week. Sampling has been replaced with referring to animations frames. I suspect I didn't understand the difference between anchors and link defaults so that makes sense as to why the broken references were never picked up. (I believe there was also a time when there was a delay in getting the autolinks database updated so I might have used anchors as a workaround for that.) |
The CSS Working Group just discussed
The full IRC log of that discussion<fantasai> Subtopic: CSS Animation and content-visibility<fantasai> github: https://github.com//issues/7576 <florian> github: https://github.com//issues/5611 <fantasai> florian: We had that discussion and had ar esolution, but afterwards discussion because some things weren't clear <fantasai> florian: Tab made edits based on subsequent discussion, so had resolution on fundamentals <fantasai> florian: but commit contains more <fantasai> florian: I suspect it's fine, but precise thing in the spec is beyond what we resolved in <fantasai> florian: suggestion is if you care about it, please review it, and open a new issue if you don't like it <fantasai> RESOLVED: Accept edits, if any issues open a new issue <TabAtkins> original edits: https://github.com/w3c/csswg-drafts/commit/82fc7ddb1abf9ce0c3f616cbd91a0f1d2069a215 <TabAtkins> subsequent edits: https://github.com/w3c/csswg-drafts/commit/1ef126260e59fb486bc3749d731b8ba22556d92a |
content-visibility property, when skipping its contents should also pause css animations, as if animation-play-state: paused has been applied to them.
This allows UA to skip work that would otherwise be necessary to keep the animations going and up to date.
/cc @chrishtr @dvoytenko
The text was updated successfully, but these errors were encountered: