There are at least a couple of cases here: * Invalid timing parameters passed to something like Element.animate() or 'new KeyframeEffect()'. It would be helpful to know *which* timing parameter is at fault and in what way. How this works will depend somewhat on the behavior we decide upon in bug 1253465. * Invalid CSS property values. We currently silently accept these so we really should report them. The tricky bit is shorthands, which, once bug 1245748 lands, we won't parse until we have a style context. Also, I'm not sure if we should report these as CSS errors or Javascript errors. Furthermore, for values passed to the API from CSS we need to be careful not to report errors twice (presumably we will already log invalid values in @keyframes rules?).

Please review.

Comment on attachment 8731096 [details] [diff] [review] Part 1: Produce console warnings for invalid duration Review of attachment 8731096 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/TimingParams.h @@ +59,3 @@ > } > + } else if (!aDuration.GetAsString().EqualsLiteral("auto")) { > + aRv.ThrowTypeError<dom::MSG_INVALID_DURATION_ERROR>(); What actually gets displayed on the console in each of these cases? ::: dom/bindings/Errors.msg @@ +91,5 @@ > MSG_DEF(MSG_SW_INSTALL_ERROR, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} encountered an error during installation.") > MSG_DEF(MSG_SW_SCRIPT_THREW, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} threw an exception during script evaluation.") > MSG_DEF(MSG_TYPEDARRAY_IS_SHARED, 1, JSEXN_TYPEERR, "{0} can't be a typed array on SharedArrayBuffer") > MSG_DEF(MSG_CACHE_ADD_FAILED_RESPONSE, 3, JSEXN_TYPEERR, "Cache got {0} response with bad status {1} while trying to add request {2}") > +MSG_DEF(MSG_INVALID_DURATION_ERROR, 0, JSEXN_TYPEERR, "Invalid duration.") \ No newline at end of file No full-stop (.) needed here.

I have two questions. 1. Should we split the test for invalid keyframe options to another test file? Now, gathered to testing/web-platform/tests/web-animations/keyframe-effect/constructor.html 2. Should we throw TypeError when inputed easing keyword is not found in the following page? https://w3c.github.io/web-animations/#timing-function

Comment on attachment 8731099 [details] [diff] [review] Part 3: Produce console warnings for invalid delay Review of attachment 8731099 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, the spec is wrong here. Negative delay should be allowed.

(In reply to Brian Birtles (:birtles) from comment #9) > Comment on attachment 8731099 [details] [diff] [review] > Part 3: Produce console warnings for invalid delay > > Review of attachment 8731099 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry, the spec is wrong here. Negative delay should be allowed. Pushed spec fix: https://github.com/w3c/web-animations/commit/20b33c3b5a342b889f3816a2a76a88fc5c6f3715

Comment on attachment 8731100 [details] [diff] [review] Part 4: Produce console warnings for invalid iterations Review of attachment 8731100 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html @@ +571,5 @@ > + { desc: "a negative iterations", > + input: { iterations: -1 }, > + expected: { name: "TypeError" } }, > + { desc: "a string iterations", > + input: { iterations: "cabbage" }, I don't think we need the last test (for "cabbage")? It probably doesn't hurt to add it, however.

Comment on attachment 8731107 [details] [diff] [review] Part 5: Produce console warnings for invalid easing Review of attachment 8731107 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Daisuke Akatsuka (:daisuke) from comment #8) > I have two questions. > 1. Should we split the test for invalid keyframe options to another test > file? > Now, gathered to > testing/web-platform/tests/web-animations/keyframe-effect/constructor.html Probably. For now this is ok, however. > 2. Should we throw TypeError when inputed easing keyword is not found in the > following page? > https://w3c.github.io/web-animations/#timing-function Yes, I think that's what this patch does, right? ::: dom/animation/KeyframeEffect.cpp @@ +1285,5 @@ > float offset = float(keyframe.mKeyframeDict.mOffset.Value()); > Maybe<ComputedTimingFunction> easing = > + TimingParams::ParseEasing(keyframe.mKeyframeDict.mEasing, > + aTarget->OwnerDoc(), > + aRv); We should check the result of aRv here and return early if it failed. Also, we should have a test for this. ::: dom/animation/TimingParams.h @@ +6,5 @@ > > #ifndef mozilla_TimingParams_h > #define mozilla_TimingParams_h > > +#include "nsCSSParser.h" // For nsCSSParser This belongs in the .cpp file. @@ +7,5 @@ > #ifndef mozilla_TimingParams_h > #define mozilla_TimingParams_h > > +#include "nsCSSParser.h" // For nsCSSParser > +#include "nsIAtom.h" Why is this needed? @@ +8,5 @@ > #define mozilla_TimingParams_h > > +#include "nsCSSParser.h" // For nsCSSParser > +#include "nsIAtom.h" > +#include "nsString.h" We can use nsStringFwd.h here. In general we try to reduce includes in header files and use forward-declarations where possible. @@ +92,5 @@ > } > > + static Maybe<ComputedTimingFunction> ParseEasing(const nsAString& aEasing, > + nsIDocument* aDocument, > + ErrorResult& aRv); Why are we moving this here? I'm not saying it's wrong, but I don't understand why it is necessary/better. ::: dom/bindings/Errors.msg @@ +92,5 @@ > MSG_DEF(MSG_SW_SCRIPT_THREW, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} threw an exception during script evaluation.") > MSG_DEF(MSG_TYPEDARRAY_IS_SHARED, 1, JSEXN_TYPEERR, "{0} can't be a typed array on SharedArrayBuffer") > MSG_DEF(MSG_CACHE_ADD_FAILED_RESPONSE, 3, JSEXN_TYPEERR, "Cache got {0} response with bad status {1} while trying to add request {2}") > +MSG_DEF(MSG_INVALID_DURATION_ERROR, 0, JSEXN_TYPEERR, "Invalid duration.") > +MSG_DEF(MSG_INVALID_EASING_ERROR, 0, JSEXN_TYPEERR, "Invalid easing.") \ No newline at end of file No full-stop at the end here. ::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html @@ +571,5 @@ > + expected: { name: "TypeError" } }, > + { desc: "a blank easing", > + input: { easing: "" }, > + expected: { name: "TypeError" } }, > + { desc: "a uncognised easing", unrecognized @@ +574,5 @@ > + expected: { name: "TypeError" } }, > + { desc: "a uncognised easing", > + input: { easing: "unrecognised" }, > + expected: { name: "TypeError" } }, > + { desc: "a initial easing", 'initial' easing @@ +577,5 @@ > + expected: { name: "TypeError" } }, > + { desc: "a initial easing", > + input: { easing: "initial" }, > + expected: { name: "TypeError" } }, > + { desc: "a inherit easing", 'inherit' easing @@ +580,5 @@ > + expected: { name: "TypeError" } }, > + { desc: "a inherit easing", > + input: { easing: "inherit" }, > + expected: { name: "TypeError" } }, > + { desc: "a token stream easing", a variable easing @@ +583,5 @@ > + expected: { name: "TypeError" } }, > + { desc: "a token stream easing", > + input: { easing: "var(--x)" }, > + expected: { name: "TypeError" } }, > + { desc: "a multi easing", a multi-value easing

(In reply to Brian Birtles (:birtles) from comment #7) > Comment on attachment 8731096 [details] [diff] [review] > Part 1: Produce console warnings for invalid duration > > Review of attachment 8731096 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/animation/TimingParams.h > @@ +59,3 @@ > > } > > + } else if (!aDuration.GetAsString().EqualsLiteral("auto")) { > > + aRv.ThrowTypeError<dom::MSG_INVALID_DURATION_ERROR>(); > > What actually gets displayed on the console in each of these cases? Thank you very much! I'll attach the screenshot later. > ::: dom/bindings/Errors.msg > @@ +91,5 @@ > > MSG_DEF(MSG_SW_INSTALL_ERROR, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} encountered an error during installation.") > > MSG_DEF(MSG_SW_SCRIPT_THREW, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} threw an exception during script evaluation.") > > MSG_DEF(MSG_TYPEDARRAY_IS_SHARED, 1, JSEXN_TYPEERR, "{0} can't be a typed array on SharedArrayBuffer") > > MSG_DEF(MSG_CACHE_ADD_FAILED_RESPONSE, 3, JSEXN_TYPEERR, "Cache got {0} response with bad status {1} while trying to add request {2}") > > +MSG_DEF(MSG_INVALID_DURATION_ERROR, 0, JSEXN_TYPEERR, "Invalid duration.") > \ No newline at end of file Okay. > No full-stop (.) needed here. Other messages such as "Invalid zoom and pan value." have full-stop. Even so, no need?

(In reply to Brian Birtles (:birtles) from comment #9) > Comment on attachment 8731099 [details] [diff] [review] > Part 3: Produce console warnings for invalid delay > > Review of attachment 8731099 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry, the spec is wrong here. Negative delay should be allowed. OK! I'll remove this patch since we don't need to check more than WebIDL.

(In reply to Brian Birtles (:birtles) from comment #11) > Comment on attachment 8731100 [details] [diff] [review] > Part 4: Produce console warnings for invalid iterations > > Review of attachment 8731100 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > testing/web-platform/tests/web-animations/keyframe-effect/constructor.html > @@ +571,5 @@ > > + { desc: "a negative iterations", > > + input: { iterations: -1 }, > > + expected: { name: "TypeError" } }, > > + { desc: "a string iterations", > > + input: { iterations: "cabbage" }, > > I don't think we need the last test (for "cabbage")? It probably doesn't > hurt to add it, however. I removed following test by this patch. https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/web-animations/keyframe-effect/getComputedTiming.html#158 So, I brought same test to here. But ok! I remove the test.

(In reply to Brian Birtles (:birtles) from comment #12) > Comment on attachment 8731107 [details] [diff] [review] > Part 5: Produce console warnings for invalid easing > > Review of attachment 8731107 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Daisuke Akatsuka (:daisuke) from comment #8) > > I have two questions. > > 1. Should we split the test for invalid keyframe options to another test > > file? > > Now, gathered to > > testing/web-platform/tests/web-animations/keyframe-effect/constructor.html > > Probably. For now this is ok, however. OK. > > 2. Should we throw TypeError when inputed easing keyword is not found in the > > following page? > > https://w3c.github.io/web-animations/#timing-function > > Yes, I think that's what this patch does, right? Yes, I'd liked to just re-confirm since some tests case were different. https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/web-animations/keyframe-effect/constructor.html#68 > ::: dom/animation/KeyframeEffect.cpp > @@ +1285,5 @@ > > float offset = float(keyframe.mKeyframeDict.mOffset.Value()); > > Maybe<ComputedTimingFunction> easing = > > + TimingParams::ParseEasing(keyframe.mKeyframeDict.mEasing, > > + aTarget->OwnerDoc(), > > + aRv); > > We should check the result of aRv here and return early if it failed. > > Also, we should have a test for this. OK > ::: dom/animation/TimingParams.h > @@ +6,5 @@ > > > > #ifndef mozilla_TimingParams_h > > #define mozilla_TimingParams_h > > > > +#include "nsCSSParser.h" // For nsCSSParser > > This belongs in the .cpp file. Okay! ( headers relationship ) > @@ +92,5 @@ > > } > > > > + static Maybe<ComputedTimingFunction> ParseEasing(const nsAString& aEasing, > > + nsIDocument* aDocument, > > + ErrorResult& aRv); > > Why are we moving this here? > > I'm not saying it's wrong, but I don't understand why it is necessary/better. The reason why I moved to TimingParams are: 1. Already TimingParams has similar function "ParseDuration" that parse and check invalid option. 2. We can gather all logic that checks invalid options into TimingParam. I thought that we can clarify the roll of TimingParam. > ::: dom/bindings/Errors.msg > @@ +92,5 @@ > > MSG_DEF(MSG_SW_SCRIPT_THREW, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} threw an exception during script evaluation.") > > MSG_DEF(MSG_TYPEDARRAY_IS_SHARED, 1, JSEXN_TYPEERR, "{0} can't be a typed array on SharedArrayBuffer") > > MSG_DEF(MSG_CACHE_ADD_FAILED_RESPONSE, 3, JSEXN_TYPEERR, "Cache got {0} response with bad status {1} while trying to add request {2}") > > +MSG_DEF(MSG_INVALID_DURATION_ERROR, 0, JSEXN_TYPEERR, "Invalid duration.") > > +MSG_DEF(MSG_INVALID_EASING_ERROR, 0, JSEXN_TYPEERR, "Invalid easing.") > \ No newline at end of file > > No full-stop at the end here. OK. > ::: > testing/web-platform/tests/web-animations/keyframe-effect/constructor.html > @@ +571,5 @@ > > + expected: { name: "TypeError" } }, > > + { desc: "a blank easing", > > + input: { easing: "" }, > > + expected: { name: "TypeError" } }, > > + { desc: "a uncognised easing", > > unrecognized > > @@ +574,5 @@ > > + expected: { name: "TypeError" } }, > > + { desc: "a uncognised easing", > > + input: { easing: "unrecognised" }, > > + expected: { name: "TypeError" } }, > > + { desc: "a initial easing", > > 'initial' easing > > @@ +577,5 @@ > > + expected: { name: "TypeError" } }, > > + { desc: "a initial easing", > > + input: { easing: "initial" }, > > + expected: { name: "TypeError" } }, > > + { desc: "a inherit easing", > > 'inherit' easing > > @@ +580,5 @@ > > + expected: { name: "TypeError" } }, > > + { desc: "a inherit easing", > > + input: { easing: "inherit" }, > > + expected: { name: "TypeError" } }, > > + { desc: "a token stream easing", > > a variable easing > > @@ +583,5 @@ > > + expected: { name: "TypeError" } }, > > + { desc: "a token stream easing", > > + input: { easing: "var(--x)" }, > > + expected: { name: "TypeError" } }, > > + { desc: "a multi easing", > > a multi-value easing OK!

This is screen shot when inputted duration is invalid.

(In reply to Daisuke Akatsuka (:daisuke) from comment #13) > (In reply to Brian Birtles (:birtles) from comment #7) > > ::: dom/animation/TimingParams.h > > @@ +59,3 @@ > > > } > > > + } else if (!aDuration.GetAsString().EqualsLiteral("auto")) { > > > + aRv.ThrowTypeError<dom::MSG_INVALID_DURATION_ERROR>(); > > > > What actually gets displayed on the console in each of these cases? > > Thank you very much! > I'll attach the screenshot later. Thank you! I'd like to see the screenshot for *both* cases -- the invalid duration error and the out of range error. I want to see if they look similar or not. > > No full-stop (.) needed here. > > Other messages such as "Invalid zoom and pan value." have full-stop. Even > so, no need? Oh, you're right. Full-stop is more common! Sorry, please add it back!

(In reply to Daisuke Akatsuka (:daisuke) from comment #16) > (In reply to Brian Birtles (:birtles) from comment #12) > > Why are we moving this here? > > > > I'm not saying it's wrong, but I don't understand why it is necessary/better. > > The reason why I moved to TimingParams are: > 1. Already TimingParams has similar function "ParseDuration" that parse and > check invalid option. > 2. We can gather all logic that checks invalid options into TimingParam. > I thought that we can clarify the roll of TimingParam. Ok, sounds good.

This is the screenshot for out of range error.

Comment on attachment 8732011 [details] [diff] [review] Part 5: Produce console warnings for invalid easing Review of attachment 8732011 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/AnimationUtils.cpp @@ -13,5 @@ > #include "nsIDocument.h" > #include "nsGlobalWindow.h" > #include "nsString.h" > #include "mozilla/Attributes.h" > #include "mozilla/ComputedTimingFunction.h" // ComputedTimingFunction Can we get rid of this line too? Does something else use it? ::: dom/animation/KeyframeEffect.cpp @@ +1284,5 @@ > for (OffsetIndexedKeyframe& keyframe : aKeyframes) { > + Maybe<ComputedTimingFunction> easing = > + TimingParams::ParseEasing(keyframe.mKeyframeDict.mEasing, > + aTarget->OwnerDoc(), > + aRv); Minor nit: Put aRv on the same line as aTarget->OwnerDoc(). (The only place we encourage one line per parameter is when defining/declaring a function and it doesn't fit on one line.) ::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html @@ +446,5 @@ > > +var gInvalidEasingInKeyframeSequenceTests = [ > + { desc: "a blank easing", > + input: [{ easing: "" }] }, > + { desc: "a unrecognized easing", Nit: an unrecognized easing @@ +448,5 @@ > + { desc: "a blank easing", > + input: [{ easing: "" }] }, > + { desc: "a unrecognized easing", > + input: [{ easing: "unrecognized" }] }, > + { desc: "a 'initial' easing", an 'initial' easing @@ +450,5 @@ > + { desc: "a unrecognized easing", > + input: [{ easing: "unrecognized" }] }, > + { desc: "a 'initial' easing", > + input: [{ easing: "initial" }] }, > + { desc: "a 'inherit' easing", an 'inherit' easing

Revised: 1. Ridded extra header. 2. Revised some comments for tests.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdab04704f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad28fd60184e https://hg.mozilla.org/integration/mozilla-inbound/rev/b798c62e4365 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c0ea60eaef

https://hg.mozilla.org/mozilla-central/rev/5cdab04704f1 https://hg.mozilla.org/mozilla-central/rev/ad28fd60184e https://hg.mozilla.org/mozilla-central/rev/b798c62e4365 https://hg.mozilla.org/mozilla-central/rev/a0c0ea60eaef