1253465 - Stop storing invalid timing parameters

Status: RESOLVED DUPLICATE of bug 1253470

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I met with Google in February to discuss whether or not the AnimationTimingReadOnly interface should preserve the provided timing parameters even if they are invalid. We agreed that it should not and Google seemed in favor of throwing exceptions in this case.

I've yet to edit this into the spec[1] but we have some meeting notes.[2]

[1] https://github.com/w3c/web-animations/issues/143
[2] https://docs.google.com/document/d/1lQbgJ0fZYGn1_qboj59G-MOILK33z4kmBdjgtSHF2gA/edit#heading=h.cioj28lsgoa2

Marking this as blocking shipping Element.animate because we should try to fix our behavior with regards to throwing exceptions before shipping.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

One question, however: if we are given a negative value for duration, is a RangeError the most appropriate exception to throw here? Boris?

There seems to be some advice that TypeError is what we should normally use for invalid arguments.[3][4]

[3] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27283
[4] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27284

Also, I think it would be helpful to produce a console warning in this case so the author can tell *which* timing parameter caused a problem. This might be a little bit inconsistent, however, since I guess we won't be able to do this for any TypeErrors generated by the WebIDL bindings.

Comment 2

[Boris Zbarsky [:bzbarsky]]

It would make a lot of sense to me to throw RangeError for out-of-range values, but other people seem to have different ideas on how this stuff should work...  I suggest checking with Domenic directly on what his thoughts are.

> Also, I think it would be helpful to produce a console warning in this case so the author can tell *which*
> timing parameter caused a problem.

You can indicate that in the .message of the exception, right?  That won't help if the exception is caught, of course.

> since I guess we won't be able to do this for any TypeErrors generated by the WebIDL bindings.

Web IDL binding exceptions try to indicate which thing caused the exception to be thrown in the .message (things like "argument 1 of Foo.bar" or "foo member of Bar dictionary" or whatnot).

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Great. So I think for things like a negative duration we can use:

  aRv.ThrowTypeError<ENFORCE_RANGE_OUT_OF_RANGE>(NS_LITERAL_STRING("duration"));

When it is a string other than "auto", however, there doesn't seem to be an appropriate error in:

  https://dxr.mozilla.org/mozilla-central/source/dom/bindings/Errors.msg

So we might need to work out where/if we should add one.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

It looks like we already add codes for invalid zoomPan, invalid transform etc. so it's probably reasonable to add a code for invalid duration.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Added this to the spec: https://github.com/w3c/web-animations/commit/f6f50d33ab5869b4b0bb4426b6d313b8c9b26e3d

Comment 6

[Boris Zbarsky [:bzbarsky]]

> When it is a string other than "auto", however, there doesn't seem to be an appropriate error in:

Just add things as needed.