Animations: extract keyframes from styles #9623
Conversation
| class CSSKeyframesRuleDef { | ||
| constructor() { | ||
| /** @type {string} */ | ||
| this.name; |
There was a problem hiding this comment.
You need to set these values, declaring them like this doesn't help the JS engine's internal structure.
There was a problem hiding this comment.
This is just the definition for closure, in place of extern. Per closure it's fine. The class itself is not used - only its type.
There was a problem hiding this comment.
Can you mark as an interface, then?
There was a problem hiding this comment.
I tried, but unfortunately can't. The underlying CSSRule is declared as a class, not interface. See this. This is kind of what our Def suffix means at this point. I'm going to contribute the same declarations to them, but we are slightly behind their externs version.
There was a problem hiding this comment.
Actually, to remove this inconsistency, I just moved this declaration to our closure externs. So this should be now cleaner.
| keyframeRule.keyText == 'from' ? 0 : | ||
| keyframeRule.keyText == 'to' ? 1 : | ||
| parseFloat(keyframeRule.keyText) / 100; | ||
| for (let j = 0; j < keyframeRule.style.length; j++) { |
There was a problem hiding this comment.
Nit: store style in a variable. Some engines do weird things (like re-initialize) when accessing native objects.
| return null; | ||
| } | ||
| // Exlcude AMP's own styles. | ||
| if (styleNode.hasAttribute('amp-boilerplate') || |
There was a problem hiding this comment.
could this be if (!styleNode.hasAttribute('amp-custom'))?
| if (rule.name == name && isEnabled(win, rule)) { | ||
| return buildKeyframes(keyframesRule); | ||
| } | ||
| } else if (rule.type == /* CSSMediaRule */ 4) { |
There was a problem hiding this comment.
checking media match before the recursion should be a decent optimization
There was a problem hiding this comment.
The reason I didn't do this is because there are potentially a lot more @media definitions than @keyframes. And checking in each case is somewhat expensive. So instead I first find a likely candidate @keyframes name and then check if its parents are enabled recursively.
| for (let j = 0; j < keyframeRule.style.length; j++) { | ||
| const styleName = keyframeRule.style[j]; | ||
| let propName = styleName; | ||
| if (styleName == 'animation-timing-function') { |
There was a problem hiding this comment.
end with to account for vendor prefixes?
There was a problem hiding this comment.
(TIL you could specify animation-timing-function in keyframe def, totally did not know this is possible)
| const array = []; | ||
| for (let i = 0; i < keyframesRule.cssRules.length; i++) { | ||
| const keyframeRule = keyframesRule.cssRules[i]; | ||
| const keyframe = {}; |
There was a problem hiding this comment.
cssRules are of type CSSRuleList, so it's not an array :(
There was a problem hiding this comment.
I think @aghassemi meant map on all, e.g. const array = keyframesRule.cssRules.map(rule => {}). Unfortunately cssRules is not an array and I thought we decided to avoid simple toArray conversions just for better for-loops?
Partial for #9129.