Open Bug 1501530 Opened 1 year ago Updated 1 year ago

Don't be extra-smart in our animation code about serializing getKeyframes with var references.

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

Flags: needinfo?(emilio)
I might need to do this for bug 1253476 but hopefully I can find another way in that bug.
Do the same that CSSOM does for variable references from shorthands in specified
values, which is the empty string, and don't do our own variable substitution
stuff.

Relevant spec issues are:

 * https://github.com/w3c/csswg-drafts/issues/2515
 * https://github.com/w3c/csswg-drafts/issues/3244

If we want to do substitution and such, then the spec would need to define what
happens when there's no style, like what happens when you call getKeyframes() on
a target that is outside of the flat tree, or in a display: none subtree.
I'll have a look at this tomorrow but I was actually hoping to fix bug 1253476 first before doing this since I'm concerned we might need to re-introduce this code for that bug (the currently proposed behavior in that bug is to have getAnimations() resolve and return computed style for filling animations--that might change though, it needs more work to determine if we can avoid it). Maybe we can just re-introduce it then if needed though.

Re: the spec issue, for this particular bug the behavior would need to be specified in CSS Animations Level 2 where we spec the integration with Web Animations which is on my list of things to do after fixing issues in Web Animations proper.
(In reply to Brian Birtles (:birtles) from comment #3)
> I'll have a look at this tomorrow but I was actually hoping to fix bug
> 1253476 first before doing this since I'm concerned we might need to
> re-introduce this code for that bug (the currently proposed behavior in that
> bug is to have getAnimations() resolve and return computed style for filling
> animations--that might change though, it needs more work to determine if we
> can avoid it). Maybe we can just re-introduce it then if needed though.

I'm fine reintroducing this code as long as the behavior is sane when there's no style. What's the proposed behavior then? Not resolving var() and env()? That seems fishy.
Flags: needinfo?(emilio)
I'm still not really comfortable with this change. It means that for any CSS animation using variables `effect.setKeyframes(effect.getKeyframes())` will no longer work and it's not possible to detect this case to work around it.

At the moment, that code will already produce side effects--converting variable references into fixed values, but that's much less destructive (and there are already other such side-effects due to expanding shorthands).

Given that we're talking about a situation that only applies to CSS animations, I don't think the case where there's no style is important--presumably we won't have CSS animations in that case anyway.

I'm also not sure that the seemingly related CSS spec issues are going to help with this case because they relate to CSSOM declaration blocks which have an ordered set of declarations whereas keyframes don't--there is an ordering between shorthands and longhands but that's about all. It will take quite a bit of work to preserve CSS shorthands from @keyframes rules in Web Animations keyframes and last time we discussed it everyone seemed to agree it wasn't worth it. I'm happy to revisit that though if it seems worthwhile.
(In reply to Brian Birtles (:birtles) from comment #5)
> I'm still not really comfortable with this change. It means that for any CSS
> animation using variables `effect.setKeyframes(effect.getKeyframes())` will
> no longer work and it's not possible to detect this case to work around it.

This is the same situation as div.setAttribute("style", div.style.cssText)

> Given that we're talking about a situation that only applies to CSS
> animations, I don't think the case where there's no style is
> important--presumably we won't have CSS animations in that case anyway.

Then this should instead assert there's a pres shell, and use it.

I still think that having special-cased CSS declaration blocks serialization code is a bad idea though.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > I'm still not really comfortable with this change. It means that for any CSS
> > animation using variables `effect.setKeyframes(effect.getKeyframes())` will
> > no longer work and it's not possible to detect this case to work around it.
> 
> This is the same situation as div.setAttribute("style", div.style.cssText)

That seems unfortunate but less critical since you can add still override individual properties using `div.style`.

There's no way to do that with this API though. If you want to override or add a property, you _have_ to use getKeyframes() and setKeyframes(). This change would mean there's no way to do that anymore.

> > Given that we're talking about a situation that only applies to CSS
> > animations, I don't think the case where there's no style is
> > important--presumably we won't have CSS animations in that case anyway.
> 
> Then this should instead assert there's a pres shell, and use it.
> 
> I still think that having special-cased CSS declaration blocks serialization
> code is a bad idea though.

Why is that?
(In reply to Brian Birtles (:birtles) from comment #7)
> Why is that?

This whole code path feels really hacked up... That's pretty much it.
You need to log in before you can comment on or make changes to this bug.