Non-destructive `@keyframes` rule transformation. #3163

cdata · 2015-12-05T01:16:29Z

Previously, the transformer did not disambiguate selectors in @media
blocks and keyframes in @keyframes blocks. Now, the transformer can
safely transform @keyframes blocks. Before a selector is transformed,
if the selector has a parent, it is checked. If the checked parent is a
@keyframes rule, the selector transformation is skipped.

Element-specific @keyframes are suffixed with the scoped element name.
For example, @keyframes foo in an element scoped with x-el-0 will by
transformed to @keyframes foo-x-el-0. References to that animation in
the element's local styles will be updated as well.

Added tests for the new keyframes transformation.

This change also fixes the accidental introduction of a vestigial
semicolon at the end of some generated styles.

Feedback on design
Unit / regression tests
Test @apply in keyframes
Ensure that scopeId doesn't get stale

Fixes #2399

cdata · 2015-12-05T01:37:34Z

@sorvell I have a lingering uncertainty about the semantics of custom property value propagation. Since the "selector" within a @keyframes block isn't actually a selector, it seems that it may be necessary to inform the style system that the effective scope is not what it typically expects. For an example of what I mean, consider the following arrangement:

<style is="custom-style">
  :root {
    --left: 0px;
  }

  /* A selector: */
  from {
    --left: 10px;
  }
</style>
<style is="custom-style">
  @keyframes foo {
    /* Not a selector: */
    from {
      left: var(--left);
    }
  }
</style>
<!-- A pathologically named element: -->
<from></from>

Ideally, the property would be transformed to left: 0px. However, it may be the case that the value will end up being 10px due to selector matching.

WDYT?

cdata · 2015-12-09T23:54:50Z

I uncommented some ShadowDOM tests since the relevant Chrome bug has been fixed.

Note: I did a lot of cleanup whitespace changes (moving <style> tags inside of templates), so viewing the diff with ?w=1 in the query params is recommended.

cdata · 2015-12-10T01:41:42Z

test/runner.html

-      // 'unit/styling-cross-scope-var.html?dom=shadow',
-      // 'unit/styling-cross-scope-apply.html?dom=shadow',
+      'unit/styling-cross-scope-var.html?dom=shadow',
+      'unit/styling-cross-scope-apply.html?dom=shadow',


The related Chrome bug was apparently fixed in M46, so I have reactivated these.

sorvell · 2016-01-08T21:17:26Z

src/lib/style-properties.html

+        rule.keyframesNameRx = new RegExp(rule.keyframesName, 'g');
+        rule.transformedKeyframesName = rule.keyframesName + '-' + scopeId;
+        rule.transformedSelector = rule.transformedSelector || rule.selector;
+        rule.selector = rule.transformedSelector.replace(


They scopeId for a given element may change over time. In this case won't the selector fail to update?

sorvell · 2016-01-08T21:45:10Z

Let's add a test to cover using @apply in keyframes. wdyt?

cdata · 2016-02-02T18:56:24Z

Extra tests added for @mixin and style-scope changing. PTAL!

sorvell · 2016-02-04T21:08:54Z

src/lib/style-properties.html

+      applyKeyframeTransforms: function(rule, keyframeTransforms) {
+        var input = rule.cssText;
+        var output = rule.cssText;
+


Let's remove these empty lines. We generally don't space out function contents like this and prefer factoring instead.

sorvell · 2016-02-04T21:09:41Z

src/lib/style-properties.html

@@ -256,15 +307,55 @@
          hostSelector;
        var hostRx = new RegExp(this.rx.HOST_PREFIX + rxHostSelector +
          this.rx.HOST_SUFFIX);
+
+        var keyframesRules = element._styles._keyframes;


Seems like this keyframes specific handling should be factored into a separate function

Previously, the transformer did not disambiguate selectors in `@media` blocks and keyframes in `@keyframes` blocks. Now, the transformer can safely transform `@keyframes` blocks. Before a selector is transformed, if the selector has a parent, it is checked. If the checked parent is a `@keyframes` rule, the selector transformation is skipped. Element-specific `@keyframes` are suffixed with the scoped element name. For example, `@keyframes foo` in an element scoped with `x-el-0` will by transformed to `@keyframes foo-x-el-0`. References to that animation in the element's local styles will be updated as well. Added tests for the new keyframes transformation.

cdata · 2016-02-04T22:53:38Z

src/lib/style-properties.html

+              !Polymer.StyleUtil.isKeyframesSelector(rule) &&
+              rule.cssText) {
+            // NOTE: keyframe transforms only scope munge animation names, so it
+            // is not necessary to apply them in ShadowDOM.


I left a reminder to hopefully lessen confusion in the future.

sorvell · 2016-02-04T23:20:40Z

LGTM. Thanks for the contribution!

…tion Non-destructive `@keyframes` rule transformation.

cdata assigned sorvell Dec 5, 2015

googlebot added the cla: yes label Dec 5, 2015

cdata mentioned this pull request Dec 5, 2015

Rewrite paper-progress as paper-linear-progress googlearchive/paper-linear-progress#1

Open

cdata force-pushed the allow-keyframe-style-transformation branch from efbb351 to 404335b Compare December 5, 2015 01:19

cdata force-pushed the allow-keyframe-style-transformation branch 4 times, most recently from 5698e63 to 7ca41d5 Compare December 9, 2015 22:39

cdata force-pushed the allow-keyframe-style-transformation branch from b775213 to 3cc2d63 Compare December 10, 2015 01:39

cdata reviewed Dec 10, 2015
View reviewed changes

cdata force-pushed the allow-keyframe-style-transformation branch from 3cc2d63 to d41cf2d Compare December 10, 2015 02:06

cdata force-pushed the allow-keyframe-style-transformation branch 2 times, most recently from 1d7fd4b to 3adeade Compare January 5, 2016 22:40

sorvell reviewed Jan 8, 2016
View reviewed changes

cdata force-pushed the allow-keyframe-style-transformation branch from 3adeade to 34614da Compare January 19, 2016 19:26

cdata force-pushed the allow-keyframe-style-transformation branch 4 times, most recently from 621397c to 5152f11 Compare February 2, 2016 18:55

kevinpschaaf added the p0 label Feb 4, 2016

sorvell reviewed Feb 4, 2016
View reviewed changes

cdata force-pushed the allow-keyframe-style-transformation branch from 5152f11 to b9f2482 Compare February 4, 2016 22:52

cdata reviewed Feb 4, 2016
View reviewed changes

sorvell pushed a commit that referenced this pull request Feb 4, 2016

Merge pull request #3163 from Polymer/allow-keyframe-style-transforma…

9968266

…tion Non-destructive `@keyframes` rule transformation.

sorvell merged commit 9968266 into master Feb 4, 2016

sorvell deleted the allow-keyframe-style-transformation branch February 4, 2016 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-destructive `@keyframes` rule transformation. #3163

Non-destructive `@keyframes` rule transformation. #3163

cdata commented Dec 5, 2015

cdata commented Dec 5, 2015

cdata commented Dec 9, 2015

cdata Dec 10, 2015

sorvell Jan 8, 2016

cdata Jan 19, 2016

sorvell commented Jan 8, 2016

cdata commented Feb 2, 2016

sorvell Feb 4, 2016

cdata Feb 4, 2016

sorvell Feb 4, 2016

cdata Feb 4, 2016

cdata Feb 4, 2016

sorvell commented Feb 4, 2016

Non-destructive @keyframes rule transformation. #3163

Non-destructive @keyframes rule transformation. #3163

Conversation

cdata commented Dec 5, 2015

cdata commented Dec 5, 2015

cdata commented Dec 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorvell commented Jan 8, 2016

cdata commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorvell commented Feb 4, 2016

Non-destructive `@keyframes` rule transformation. #3163

Non-destructive `@keyframes` rule transformation. #3163