Skip to content

[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

Closed
vmpstr opened this issue Oct 13, 2020 · 66 comments
Closed

Comments

@vmpstr
Copy link
Member

vmpstr commented Oct 13, 2020

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

@flackr
Copy link
Contributor

flackr commented Oct 13, 2020

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:

  1. Pause CSS animations. This would be a fairly new behavior, would only apply to elements which had previously been visible (since only those know they have css animations), and is inconsistent with how animations normally continue on otherwise invisible elements (e.g. visibility: hidden, opacity: 0 etc) or how animations are removed on display: none elements.
  2. Continue to tick animations. This would be consistent with visibility: hidden and with how web animations would continue to tick even when the contents of the subtree are skipped. Also with content-visibility: auto I believe this option makes the most sense as normally scrolling elements off screen does not pause running animations.
  3. Remove animations. This would be consistent with display: none, which would likely be what a polyfill of content-visibility would do since it is conceptually closest. It also is consistent with how content which has not yet been visible does not have animations until it becomes visible.

[1] https://jsbin.com/fobafok/edit?html,css,js,output

@flackr
Copy link
Contributor

flackr commented Oct 13, 2020

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?

@frivoal frivoal added the css-contain-2 Current Work label Oct 13, 2020
@vmpstr
Copy link
Member Author

vmpstr commented Oct 13, 2020

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)

@chrishtr
Copy link
Contributor

chrishtr commented Oct 13, 2020

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 display: none content does.

@chrishtr
Copy link
Contributor

Option (1) has an advantage though, in that any existing animation would naturally continue once it came back on-screen.

@flackr
Copy link
Contributor

flackr commented Oct 13, 2020

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)

Whether animations are created or not is developer exposed through animationstart listeners and getAnimations({subtree: true}) as well as the start time of the animation. We don't have to do regular per frame work for non-visible animations except firing event handlers when various times are reached.

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.

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 like the option (3) @flackr mentioned, because it's simple and matches what display: none content does.

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?

@vmpstr
Copy link
Member Author

vmpstr commented Oct 13, 2020

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"?

@chrishtr
Copy link
Contributor

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 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.

I like the option (3) @flackr mentioned, because it's simple and matches what display: none content does.

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.

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.

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?

I think it's ok for this to just be another performance pitfall.

@birtles
Copy link
Contributor

birtles commented Oct 14, 2020

Is it possible to just not tick the animations in a content-visibility: hidden subtree, i.e. without pausing? (And not create them if they don't already exist, unless the author forced style resolution via getAnimations() etc.)

That would mean events / promise callbacks etc. would be delayed indefinitely, or until style resolution was forced, but that seems reasonable?

@chrishtr
Copy link
Contributor

Hi @birtles,

Is it possible to just not tick the animations in a content-visibility: hidden subtree, i.e. without pausing? (And not create them if they don't already exist, unless the author forced style resolution via getAnimations() etc.)

That would mean events / promise callbacks etc. would be delayed indefinitely, or until style resolution was forced, but that seems reasonable?

Wouldn't not ticking be effectively the same as pausing though?

@birtles
Copy link
Contributor

birtles commented Oct 14, 2020

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 getAnimations().

@flackr
Copy link
Contributor

flackr commented Oct 14, 2020

+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.

@flackr
Copy link
Contributor

flackr commented Oct 14, 2020

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 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.

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.

@vmpstr
Copy link
Member Author

vmpstr commented Oct 14, 2020

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.

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.

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.

@flackr
Copy link
Contributor

flackr commented Oct 14, 2020

Option 1 has the same challenge though right? We'll assume the animation is still there even if there is a style change?

@vmpstr
Copy link
Member Author

vmpstr commented Oct 14, 2020

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)

@flackr
Copy link
Contributor

flackr commented Oct 14, 2020

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).

@flackr
Copy link
Contributor

flackr commented Oct 15, 2020

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):

  1. CSS animations and transitions within currently invisible content-visibility subtrees do not update their local times until visible. As such, they do not finish or fire other events.
  2. We defer updating active css animations and transitions on currently invisible content-visibility subtrees. I.e. not only do we defer creating new animations but also removing existing animations when they disappear from the animation list or when the element is in a display: none subtree. (I believe in blink we may be able to do this by deferring calling ElementAnimations::MaybeApplyPendingUpdate).

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 content-visibility: auto elements left the screen, making it difficult for engines to choose different thresholds.

@birtles
Copy link
Contributor

birtles commented Oct 15, 2020

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 getComputedStyle, getAnimations etc.)?

@flackr
Copy link
Contributor

flackr commented Oct 15, 2020

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.

getAnimations({subtree: true}) will be a bit of a performance cliff if we require that it account for animations within hidden content-visibility subtrees but on the other hand it may not be too common and we already have the same issues with select all and find in page.

Do you have a preference?

@birtles
Copy link
Contributor

birtles commented Oct 15, 2020

I don't really know. At a glance making getComputedStyle behave consistently seems good. I don't really understand the mental model for content-visibility. My naive understanding was that it was a hint and hence getComputedStyle should continue to work consistently.

Special-casing getAnimations could make sense but then it opens up the question of which other APIs to special-case. Would it be such a performance cliff if we simply updated all animations in these subtrees that one time? We'd still avoid ticking them beyond that point.

@flackr
Copy link
Contributor

flackr commented Oct 15, 2020

I suspect it won't be too bad. The other cases that come to mind are other API's like offsetLeft, which should probably similarly flush the element, and whether flushing style implicitly flushes ancestor styles (starting animations)? Probably, in order to make sure that inherited properties are correctly inherited.

@vmpstr
Copy link
Member Author

vmpstr commented Oct 19, 2020

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.

@flackr
Copy link
Contributor

flackr commented Oct 19, 2020

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.

@vmpstr
Copy link
Member Author

vmpstr commented Oct 19, 2020

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.

@flackr
Copy link
Contributor

flackr commented Oct 19, 2020

I'm not too sure how easy it is to distinguish these from within the implementations.

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.

For example offsetLeft would cause a style update that is caused by script but that shouldn't flush (IMHO).

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.

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.

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).

@vmpstr
Copy link
Member Author

vmpstr commented Oct 19, 2020

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?

  • Any animation state is up to date as far as script can tell (via getAnimations, getComputedStyle, or observing the animation effects on layout)
  • If script does not access anything that causes a style update, then there are also no animation related events that fire
  • These events are deferred until the animation is created / caught up which happens when style is cleaned (by script only?)

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

  • When content-visibility skips the contents, the list of animations in the subtree does not change even if style changes in a way that would cause this list to change.
  • These animations updates are deferred until the subtree is no longer skipped
  • Existing animations' clocks are frozen so no animationend events can fire while the subtree is skipped. The clocks catch up to "current time" when no longer skipped

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"

@flackr
Copy link
Contributor

flackr commented Oct 19, 2020

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.

@vmpstr
Copy link
Member Author

vmpstr commented Oct 19, 2020

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

and style is not updated in content-visibility: hidden subtrees

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

@chrishtr
Copy link
Contributor

chrishtr commented Jan 7, 2021

I think so. @flackr @birtles is anything missing?

@flackr
Copy link
Contributor

flackr commented Jan 7, 2021

Yes, it looks good to me.

@birtles
Copy link
Contributor

birtles commented Jan 8, 2021

Seems fine to me.

@vmpstr
Copy link
Member Author

vmpstr commented Mar 11, 2021

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'

@vmpstr vmpstr added the Agenda+ label Mar 11, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-contain-2] content-visibility should pause css animations in subtree, and agreed to the following:

  • RESOLVED: Animation work is skipped in content- visibility subtrees when they're not rendered. creating, taking, or ending animations. Animation is refreshed if recalculate
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

@emilio
Copy link
Collaborator

emilio commented Mar 24, 2021

Hmm, so how do we plan to deal with animations that would cause content-visibility: auto to be e.g. moved into the screen?

That seems like a reasonable use case.

@flackr
Copy link
Contributor

flackr commented Mar 24, 2021

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?

@emilio
Copy link
Collaborator

emilio commented Mar 24, 2021

I guess so, yeah, that's a good point, nevermind then :)

@tabatkins
Copy link
Member

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?

@flackr
Copy link
Contributor

flackr commented Apr 2, 2021

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

@tabatkins
Copy link
Member

Ah, excellent, that's a great precedent. Thanks!

@tabatkins
Copy link
Member

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?

@birtles
Copy link
Contributor

birtles commented Apr 3, 2021

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.

@chrishtr
Copy link
Contributor

@tabatkins the only thing blocking closing this issue is @birtles's comment above, can you take a look?

@tabatkins
Copy link
Member

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.)

@birtles
Copy link
Contributor

birtles commented Feb 12, 2022

@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.)

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed CSS Animation and content-visibility, and agreed to the following:

  • RESOLVED: Accept edits, if any issues open a new issue
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

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

No branches or pull requests

8 participants