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

Throw TypeErrors for invalid keyframe inputs #471

Merged
merged 10 commits into from
Jul 22, 2016
Merged

Throw TypeErrors for invalid keyframe inputs #471

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
71647e7
Throw appropriate TypeErrors
alancutter Jun 14, 2016
495bd65
Throw moar type errors
alancutter Jun 14, 2016
e62cf1e
Remove debugging statements
alancutter Jun 14, 2016
45d7aac
Update unit test
alancutter Jun 14, 2016
ba12393
Use Symbol
alancutter Jun 29, 2016
589c40a
Review changes
alancutter Jul 11, 2016
17382c3
Skip _easingFunction
alancutter Jul 11, 2016
1382cc6
Make everything better
alancutter Jul 22, 2016
0ff392f
Add early linear check
alancutter Jul 22, 2016
f8d8bf7
Update comment
alancutter Jul 22, 2016
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: 2 additions & 3 deletions src/keyframe-interpolations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
}).forEach(function(interpolation) {
var offsetFraction = fraction - interpolation.startOffset;
var localDuration = interpolation.endOffset - interpolation.startOffset;
var scaledLocalTime = localDuration == 0 ? 0 : interpolation.easing(offsetFraction / localDuration);
var scaledLocalTime = localDuration == 0 ? 0 : interpolation.easingFunction(offsetFraction / localDuration);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the "property != 'easing'" test in the else clause below (line 33) now be "property != 'easingFunction'"? Or "property != 'easing' && property != 'easingFunction'"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That check is redundant, should I remove it in this patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why it is redundant? Do you mean just the "property != 'easing'" test or the whole condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole condition. This is due to the special handling of these particular members in makePropertySpecificKeyframeGroups().

scope.apply(target, interpolation.property, interpolation.interpolation(scaledLocalTime));
});
} else {
Expand Down Expand Up @@ -95,13 +95,12 @@
}
}

var easing = keyframes[startIndex].easing;
interpolations.push({
applyFrom: applyFrom,
applyTo: applyTo,
startOffset: keyframes[startIndex].offset,
endOffset: keyframes[endIndex].offset,
easing: shared.toTimingFunction(easing ? easing : 'linear'),
easingFunction: shared.parseEasingFunction(keyframes[startIndex].easing),
property: groupName,
interpolation: scope.propertyInterpolation(groupName,
keyframes[startIndex].value,
Expand Down
28 changes: 17 additions & 11 deletions src/normalize-keyframes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,21 +231,31 @@
if (memberValue != null) {
memberValue = Number(memberValue);
if (!isFinite(memberValue))
throw new TypeError('keyframe offsets must be numbers.');
throw new TypeError('Keyframe offsets must be numbers.');
if (memberValue < 0 || memberValue > 1)
throw new TypeError('Keyframe offsets must be between 0 and 1.');
}
} else if (member == 'composite') {
throw {
type: DOMException.NOT_SUPPORTED_ERR,
name: 'NotSupportedError',
message: 'add compositing is not supported'
};
if (memberValue == 'add' || memberValue == 'accumulate') {
throw {
type: DOMException.NOT_SUPPORTED_ERR,
name: 'NotSupportedError',
message: 'add compositing is not supported'
};
} else if (memberValue != 'replace') {
throw new TypeError('Invalid composite mode ' + memberValue + '.');
}
} else if (member == 'easing') {
memberValue = shared.normalizeEasing(memberValue);
} else {
memberValue = '' + memberValue;
}
expandShorthandAndAntiAlias(member, memberValue, keyframe);
}
if (keyframe.offset == undefined)
keyframe.offset = null;
if (keyframe.easing == undefined)
keyframe.easing = 'linear';
return keyframe;
});

Expand All @@ -256,11 +266,7 @@
var offset = keyframes[i].offset;
if (offset != null) {
if (offset < previousOffset) {
throw {
code: DOMException.INVALID_MODIFICATION_ERR,
name: 'InvalidModificationError',
message: 'Keyframes are not loosely sorted by offset. Sort or specify offsets.'
};
throw new TypeError('Keyframes are not loosely sorted by offset. Sort or specify offsets.');
}
previousOffset = offset;
} else {
Expand Down
31 changes: 20 additions & 11 deletions src/timing-utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
return this._direction;
},
set easing(value) {
this._easingFunction = toTimingFunction(value);
this._easingFunction = parseEasingFunction(normalizeEasing(value));
this._setMember('easing', value);
},
get easing() {
Expand Down Expand Up @@ -224,32 +224,39 @@
var cubicBezierRe = new RegExp('cubic-bezier\\(' + numberString + ',' + numberString + ',' + numberString + ',' + numberString + '\\)');
var stepRe = /steps\(\s*(\d+)\s*,\s*(start|middle|end)\s*\)/;

function toTimingFunction(easing) {
function normalizeEasing(easing) {
if (!styleForCleaning) {
styleForCleaning = document.createElement('div').style;
}
styleForCleaning.animationTimingFunction = '';
styleForCleaning.animationTimingFunction = easing;
var validatedEasing = styleForCleaning.animationTimingFunction;

if (validatedEasing == '' && isInvalidTimingDeprecated()) {
var normalizedEasing = styleForCleaning.animationTimingFunction;
if (normalizedEasing == '' && isInvalidTimingDeprecated()) {
throw new TypeError(easing + ' is not a valid value for easing');
}
return normalizedEasing;
}

var cubicData = cubicBezierRe.exec(validatedEasing);
function parseEasingFunction(normalizedEasing) {
if (normalizedEasing == 'linear') {
return linear;
}
var cubicData = cubicBezierRe.exec(normalizedEasing);
if (cubicData) {
return cubic.apply(this, cubicData.slice(1).map(Number));
}
var stepData = stepRe.exec(validatedEasing);
var stepData = stepRe.exec(normalizedEasing);
if (stepData) {
return step(Number(stepData[1]), {'start': Start, 'middle': Middle, 'end': End}[stepData[2]]);
}
var preset = presets[validatedEasing];
var preset = presets[normalizedEasing];
if (preset) {
return preset;
}
// Easing is invalid, this should never be reached.
Copy link
Contributor

@suzyh suzyh Jul 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Elaborate this comment slightly (or add a comment to the start of the function) to explain that this is because parseEasingFunction has as a precondition that it be passed a valid, normalized easing string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment.

// Fall back to linear in the interest of not crashing the page.
return linear;
};
}

function calculateActiveDuration(timing) {
return Math.abs(repeatedDuration(timing) / timing.playbackRate);
Expand Down Expand Up @@ -345,11 +352,13 @@
shared.calculateActiveDuration = calculateActiveDuration;
shared.calculateTimeFraction = calculateTimeFraction;
shared.calculatePhase = calculatePhase;
shared.toTimingFunction = toTimingFunction;
shared.normalizeEasing = normalizeEasing;
shared.parseEasingFunction = parseEasingFunction;

if (WEB_ANIMATIONS_TESTING) {
testing.normalizeTimingInput = normalizeTimingInput;
testing.toTimingFunction = toTimingFunction;
testing.normalizeEasing = normalizeEasing;
testing.parseEasingFunction = parseEasingFunction;
testing.calculateActiveDuration = calculateActiveDuration;
testing.calculatePhase = calculatePhase;
testing.PhaseNone = PhaseNone;
Expand Down
10 changes: 2 additions & 8 deletions test/js/keyframes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ suite('keyframes', function() {

test('Normalize keyframes with some offsets not specified, but sorted by offset where specified. Some offsets are out of [0, 1] range.', function() {
var normalizedKeyframes;
assert.doesNotThrow(function() {
assert.throws(function() {
normalizedKeyframes = normalizeKeyframes([
{offset: -1},
{offset: 0},
Expand All @@ -55,11 +55,6 @@ suite('keyframes', function() {
{offset: 2}
]);
});
assert.equal(normalizedKeyframes.length, 4);
assert.closeTo(normalizedKeyframes[0].offset, 0, 0.001);
assert.closeTo(normalizedKeyframes[1].offset, 0.5, 0.001);
assert.closeTo(normalizedKeyframes[2].offset, 0.75, 0.001);
assert.closeTo(normalizedKeyframes[3].offset, 1, 0.001);
});

test('Normalize keyframes with some offsets not specified, but sorted by offset where specified. All specified offsets in [0, 1] range.', function() {
Expand Down Expand Up @@ -151,14 +146,13 @@ suite('keyframes', function() {

test('Normalize keyframes with invalid specified easing.', function() {
var normalizedKeyframes;
assert.doesNotThrow(function() {
assert.throws(function() {
normalizedKeyframes = normalizeKeyframes([
{left: '0px', easing: 'easy-peasy'},
{left: '10px'},
{left: '0px'}
]);
});
assert.equal(normalizedKeyframes[0].easing, 'easy-peasy');
});

test('Normalize keyframes where some properties are given non-string, non-number values.', function() {
Expand Down
19 changes: 12 additions & 7 deletions test/js/timing-utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ suite('timing-utilities', function() {
assert.equal(calculateActiveDuration({duration: 1000, playbackRate: 4, iterations: 20}), 5000);
assert.equal(calculateActiveDuration({duration: 500, playbackRate: 0.1, iterations: 300}), 1500000);
});

function convertEasing(easing) {
return parseEasingFunction(normalizeEasing(easing));
}

test('conversion of timing functions', function() {
function assertTimingFunctionsEqual(tf1, tf2, message) {
for (var i = 0; i <= 1; i += 0.1) {
Expand All @@ -16,12 +21,12 @@ suite('timing-utilities', function() {
}

assertTimingFunctionsEqual(
toTimingFunction('ease-in-out'),
toTimingFunction('eAse\\2d iN-ouT'),
convertEasing('ease-in-out'),
convertEasing('eAse\\2d iN-ouT'),
'Should accept arbitrary casing and escape chararcters');

var f = toTimingFunction('ease');
var g = toTimingFunction('cubic-bezier(.25, 0.1, 0.25, 1.0)');
var f = convertEasing('ease');
var g = convertEasing('cubic-bezier(.25, 0.1, 0.25, 1.0)');
assertTimingFunctionsEqual(f, g, 'ease should map onto preset cubic-bezier');
assert.closeTo(f(0.1844), 0.2599, 0.001);
assert.closeTo(g(0.1844), 0.2599, 0.001);
Expand All @@ -30,12 +35,12 @@ suite('timing-utilities', function() {
assert.equal(g(0), 0);
assert.equal(g(1), 1);

f = toTimingFunction('cubic-bezier(0, 1, 1, 0)');
f = convertEasing('cubic-bezier(0, 1, 1, 0)');
assert.closeTo(f(0.104), 0.3920, 0.001);

function assertInvalidEasingThrows(easing) {
assert.throws(function() {
toTimingFunction(easing);
convertEasing(easing);
}, easing);
}

Expand All @@ -45,7 +50,7 @@ suite('timing-utilities', function() {
assertInvalidEasingThrows('cubic-bezier(-1, 1, 1, 1)');
assertInvalidEasingThrows('cubic-bezier(1, 1, 1)');

f = toTimingFunction('steps(10, end)');
f = convertEasing('steps(10, end)');
assert.equal(f(0), 0);
assert.equal(f(0.09), 0);
assert.equal(f(0.1), 0.1);
Expand Down
13 changes: 1 addition & 12 deletions test/web-platform-tests-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,7 @@ module.exports = {
'Element.animate() creates an Animation object':
'assert_equals: Returned object is an Animation expected "[object Animation]" but got "[object Object]"',

'Element.animate() does not accept keyframes not loosely sorted by offset':
'assert_throws: function "function () {\n"use strict";\n\n div.animate(subtest.input, 2000);\n }" threw object "[object Object]" ("InvalidModificationError") expected object "[object Object]" ("TypeError")',

'Element.animate() does not accept keyframes with an invalid composite value':
'assert_throws: function "function () {\n"use strict";\n\n div.animate(subtest.input, 2000);\n }" threw object "[object Object]" ("NotSupportedError") expected object "[object Object]" ("TypeError")',

'Element.animate() does not accept keyframes with an out-of-bounded negative offset':
'assert_throws: function "function () {\n"use strict";\n\n div.animate(subtest.input, 2000);\n }" did not throw',

'Element.animate() does not accept keyframes with an out-of-bounded positive offset':
'assert_throws: function "function () {\n"use strict";\n\n div.animate(subtest.input, 2000);\n }" did not throw',

// Seems to be a bug in Firefox 47? The TypeError is thrown but disappears by the time it bubbles up to assert_throws().
Copy link
Contributor

Choose a reason for hiding this comment

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

Has a bug been filed against Firefox for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit convoluted to reproduce in a bug report, given that Firefox is going to ship element.animate() in the next version I'm not sure it's worth the effort.

'Element.animate() does not accept property-indexed keyframes with an invalid easing value':
'assert_throws: function "function () {\n"use strict";\n\n div.animate(subtest.input, 2000);\n }" did not throw',
},
Expand Down