Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Port keyframe name fix from ShadyCSS #5038

Merged
merged 3 commits into from
Jan 24, 2018
Merged

Port keyframe name fix from ShadyCSS #5038

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99d0a60
Port keyframe name fix from ShadyCSS
dfreedm Jan 11, 2018
ec19683
animate border instead
dfreedm Jan 11, 2018
ad98b68
Port Edge 16 test fixes from 2.x
dfreedm Jan 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/lib/style-properties.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,10 @@
// Transforms `@keyframes` names to be unique for the current host.
// Example: @keyframes foo-anim -> @keyframes foo-anim-x-foo-0
_scopeKeyframes: function(rule, scopeId) {
rule.keyframesNameRx = new RegExp(rule.keyframesName, 'g');
// Animation names are of the form [\w-], so ensure that the name regex does not partially apply
// to similarly named keyframe names by checking for a word boundary at the beginning and
// a non-word boundary or `-` at the end.
rule.keyframesNameRx = new RegExp('\\b' + rule.keyframesName + '(?!\\B|-)', 'g');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like foo will match in a-foo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the prefix issue, looks like we'll need something more complicated like (?:animation-name\s*:|@(?:-moz|-webkit|-ms)?-?keyframes)\s*a-foo(?!\B|-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing this, it turns out not to matter if foo matches a-foo. We're only looking to add a suffix scoping name to the keyframe, and the regex fix makes sure that that suffix scoping is only added to animations already without a suffix.

The bug here was specifically if the match was found as a prefix for an animation, such that a would match a-foo and incorrectly add the scoping suffix as an infix modification, breaking the naming scheme.

Therefore, if a-foo is matched for foo, it will no longer match for a-foo, and all animations will have the scoping suffix added correctly.

rule.transformedKeyframesName = rule.keyframesName + '-' + scopeId;
rule.transformedSelector = rule.transformedSelector || rule.selector;
rule.selector = rule.transformedSelector.replace(
Expand Down
70 changes: 70 additions & 0 deletions test/unit/styling-cross-scope-var.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,57 @@
</script>
</dom-module>

<dom-module id="prefix-keyframes">
<template>
<style>
:host {
--time: 0.1s;
border: 0px solid rgb(0, 0, 0);
display: block;
/* Prefix required by Safari <= 8 */
-webkit-animation-duration: var(--time);
-webkit-animation-fill-mode: forwards;
animation-duration: var(--time);
animation-fill-mode: forwards;
}

:host([animated]) {
/* Prefix required by Safari <= 8 */
-webkit-animation-name: border-width;
animation-name: border-width;
}

/* Prefix required by Safari <= 8 */
@-webkit-keyframes border {}
@-webkit-keyframes border-width {
to {
border-top-width: 10px;
}
}
@keyframes border {}
@keyframes border-width {
to {
border-top-width: 10px;
}
}
</style>
</template>
<script>
HTMLImports.whenReady(function() {
Polymer({
is: 'prefix-keyframes',
properties: {
animated: {
type: Boolean,
value: false,
reflectToAttribute: true
}
}
});
});
</script>
</dom-module>

<dom-module id="x-scope">
<template>
<style>
Expand Down Expand Up @@ -509,6 +560,7 @@
<div id="me">x-scope</div>
<x-keyframes id="keyframes"></x-keyframes>
<x-keyframes id="keyframes2"></x-keyframes>
<prefix-keyframes id="prefix"></prefix-keyframes>
<x-child-scope id="child"></x-child-scope>
<x-child-scope id="child2"></x-child-scope>
<x-overrides id="overrides1a"></x-overrides>
Expand Down Expand Up @@ -939,6 +991,24 @@
xKeyframes.animated = true;
});

test('keyframes are transformed correctly', function(done) {
var xKeyframes = styled.$.prefix;
var onAnimationEnd = function() {
assertComputed(xKeyframes, '10px');

xKeyframes.removeEventListener('animationend', onAnimationEnd);
xKeyframes.removeEventListener('webkitAnimationEnd', onAnimationEnd);
xKeyframes.animated = false;
done();
};

assertComputed(xKeyframes, '0px');
xKeyframes.addEventListener('animationend', onAnimationEnd);
xKeyframes.addEventListener('webkitAnimationEnd', onAnimationEnd);

xKeyframes.animated = true;
})

test('mutiple elements in document', function() {
var e$ = document.querySelectorAll('simple-element');
assertComputed(e$[0].$.inner, '10px');
Expand Down