w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.46k stars 658 forks source link

[css-contain-2] content-visibility should pause css animations in subtree #5611

Closed vmpstr closed 2 years ago

vmpstr commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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?

vmpstr commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

flackr commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

+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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

vmpstr commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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"

flackr commented 3 years ago

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 commented 3 years ago

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

flackr commented 3 years ago

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

Animations would not be created. We would only do this when a style update is developer forced or the content becomes visible.

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

content-visibility is going to have some requirements in order to not be observable. Perhaps if you replace style updates with animation updates in my above statement it sounds less onerous? I.e. the browser can feel free to update style as frequently as it wants, and it should ensure that animations are updated when the developer requests the style of elements.

vmpstr commented 3 years ago

Animations would not be created. We would only do this when a style update is developer forced or the content becomes visible.

For what it's worth, this is kind of the current (accidental) state of implementation in Blink. We create animations on any style update, but I think most of those come from developers. I would also assume that we would want to create them on script forced layout, since that implicitly depends on clean style.

@flackr, @chrishtr Let me know if the following summary sounds reasonable:

flackr commented 3 years ago

Yes, this sounds good! Thanks!

chrishtr commented 3 years ago

Great.@birtles is this ok by you also?

birtles commented 3 years ago

It sounds good except for the second point:

  • We do not tick animations in skipped subtrees (meaning no updates and animations never "end"), even if style is forced

The first part makes sense, but wouldn't we update the computed style (including animations) if style was forced?

flackr commented 3 years ago

I think the intention was that we would update the style to reflect the newly started animations, but that existing animations would not tick forward to the current time.

chrishtr commented 3 years ago

I think the intention was that we would update the style to reflect the newly started animations, but that existing animations would not tick forward to the current time.

Agreed. @birtles does does this sound ok to you?

Agenda+ to confirm this spec change with the WG.

birtles commented 3 years ago

I'm not sure. I'm concerned that implementations will need to preserve the last time of each animation in order to achieve the desired result. Normally their current time would be derived from their document timeline (which does tick) but this introduces an extra state where that is not the case. That extra state probably needs to be added to Web animations.

flackr commented 3 years ago

That's an excellent point - holding the time at which the animation / element was hidden would add non-trivial complexity to the animation timing model.

I must admit I'm still not entirely clear as to why having the real current time when style is explicitly flushed or requested is problematic. Even if this required us to fire animation events they would be entirely predictable.

vmpstr commented 3 years ago

Removed from Agenda, while we're still discussing the right proposal here

vmpstr commented 3 years ago

The goal of this proposal is to not tick animations when they are under a content-visibility hidden subtree in order to avoid doing work in such subtrees. AFAIK the only observable effect of this is that animations would not start/end in a hidden subtree unless the update is somehow forced.

Is it feasible for us to add leeway for the implementations by saying something like "the user-agent may not process animations updates while the contents are under a hidden subtree". I think that would leave room for us to do the updates when updates are forced but also allow the optimization? I know this is a weaker language but it seems hard to specify exact behavior under all situations.

One other issue I ran into is that it's not trivial for us to do these updates when we getAnimations with subtree: true. This is because the subtree may or may not have content-visibility elements in it. Without first finding that out, we can't force the update on those elements easily. This is a bit of an implementation difficulty in Blink, but I think depending on the spec language we decide to use here, it may be necessary.

birtles commented 3 years ago

The goal of this proposal is to not tick animations when they are under a content-visibility hidden subtree in order to avoid doing work in such subtrees. AFAIK the only observable effect of this is that animations would not start/end in a hidden subtree unless the update is somehow forced.

Yes. Also, iteration events would not fire.

Is it feasible for us to add leeway for the implementations by saying something like "the user-agent may not process animations updates while the contents are under a hidden subtree". I think that would leave room for us to do the updates when updates are forced but also allow the optimization? I know this is a weaker language but it seems hard to specify exact behavior under all situations.

I wonder if we need the more vague language. Could we just say the UA doesn't update animations unless forced?

CSS transitions has the following text:

Various things can cause the computed values of properties on an element to change. These include insertion and removal of elements from the document tree (which both changes whether those elements have computed values and can change the styles of other elements through selector matching), changes to the document tree that cause changes to which selectors match elements, changes to style sheets or style attributes, and other things. This specification does not define when computed values are updated, beyond saying that implementations must not use, present, or display something resulting from the CSS cascading, value computation, and inheritance process [CSS3CASCADE] without updating the computed value (which means merely that implementations cannot avoid meeting requirements of this specification by claiming not to have updated the computed value as part of handling a style change). However, when an implementation updates the computed value of a property on an element to reflect one of these changes, or computes the computed value of a property on an element newly added to the document, it must update the computed value for all properties and elements to reflect all of these changes at the same time (or at least it must be undetectable that it was done at a different time). This processing of a set of simultaneous style changes is called a style change event. (Implementations typically have a style change event to correspond with their desired screen refresh rate, and when up-to-date computed style or layout information is needed for a script API that depends on it.)

In keeping with that, we don't need to define when style must be updated. So could we say that animations in content-visibility: hidden subtrees are not updated unless required (e.g. by triggering some API whose result is based on up-to-date style information for some element in a content-visibility: hidden subtree)?

One other issue I ran into is that it's not trivial for us to do these updates when we getAnimations with subtree: true. This is because the subtree may or may not have content-visibility elements in it. Without first finding that out, we can't force the update on those elements easily. This is a bit of an implementation difficulty in Blink, but I think depending on the spec language we decide to use here, it may be necessary.

I'm not sure how this would be implemented in Gecko (@emilio would likely know) so I can't comment on if this is an engine-specific concern or something all engines are likely to encounter.

emilio commented 3 years ago

One other issue I ran into is that it's not trivial for us to do these updates when we getAnimations with subtree: true. This is because the subtree may or may not have content-visibility elements in it. Without first finding that out, we can't force the update on those elements easily. This is a bit of an implementation difficulty in Blink, but I think depending on the spec language we decide to use here, it may be necessary.

So you want to avoid updating style on content-visibility: hidden subtrees, but only if there are animations in there?

It seems like you need to update style on the whole subtree regardless if you want to return the right set of animations. But you should have the same issue with a lot of other APIs right? I feel like I might be missing something.

vmpstr commented 3 years ago

As it stands now, content-visibility: hidden tries to avoid doing style in its subtree if possible. A lot of the APIs that need clean style begin from an element within the subtree. For example, getComputedStyle targets an element. It's much easier for us to check by navigating the ancestor chain whether that element is in a content-visibility: hidden subtree, and if so force the style update by "temporarily not blocking style" from that content-visibility: hidden element (a bit of an implementation detail).

It's harder for us to start from getAnimations({ subtree: true}) and then visit every single descendant to check if it may be blocking style by content-visibility: hidden. Since we may potentially have to do this for every getAnimations({ subtree: call }), I'm worried that this may be a big performance hit as well.

vmpstr commented 3 years ago

To summarize the issue a bit (and revive the discussion):

For the last point, I'd like to not do that work and skip returning animations that are in content-visibility skipped subtrees. That seems a bit inconsistent with refreshing animation state if style is updated. Also, I'm not sure of potential use cases (i.e. is it important for us to return animations in content-visibility skipped subtrees when we ask for getAnimations with subtree: true?)

birtles commented 3 years ago
  • A problem with this is something like getAnimations({ subtree: true }) since that can force us to visit every node looking for nodes that had skipped style.

Why is that a problem?

chrishtr commented 3 years ago

(birtles) Why is that a problem?

I kind of agree with @birtles - there are other APIs that also force various parts of style or layout. This one is particularly expensive, but shouldn't developers just avoid calling it if they want optimal performance? I suggested this also in a comment a while back in this issue:

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

vmpstr commented 3 years ago

Yeah, the only problem is performance.

flackr commented 3 years ago

The performance penalty seems expected to me as asking for all animations seems similar to selecting all text. I don't expect this to be done often for large subtrees.

vmpstr commented 3 years ago

Ok, is it a correct to then say we agree on the rest of the summary with the exception of the last point about getAnimations?

chrishtr commented 3 years ago

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