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.

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

[web-animations-1] Default timeline parameter in Animation constructor is confusing #2078

Closed
birtles opened this issue Dec 5, 2017 · 6 comments

Comments

@birtles
Copy link
Contributor

birtles commented Dec 5, 2017

From @graouts on November 22, 2017 13:46

I'd like to reopen this issue. While working on implementing Web Animations in WebKit, I've found the way the optional timeline parameter is defined in the Animation constructor not to be intuitive.

Specifically, I find that calling new Animation(new KeyframeEffect(…)) and new Animation(new KeyframeEffect(…), undefined) and having the first one set timeline to document.timeline and the other to undefined is not intuitive to a JS developer, where omitted values are usually handled as undefined. I understand the reason here is that the 99% case is that authors want document.timeline as their animation's timeline, at least in the first iteration of the spec where the only concrete timeline is a DocumentTimeline. But I would argue that element.animate() is the convenience layer in the Web Animations API and that we should not add convenience features to the Animation API, or at least not in a confusing as is the case right now.

My proposed solution is to either:

  1. Require an explicit value for the timeline parameter to the Animation constructor
  2. Remove the timeline parameter from the Animation constructor

I have a preference for 1 but have no objection to 2.

Copied from original issue: w3c/web-animations#208

@birtles
Copy link
Contributor Author

birtles commented Dec 5, 2017

Yeah, I agree that (1) makes most sense here.

Although the Animation() constructor is shipped in both Firefox and Chrome (I believe) there would be a compatibility concern except that neither browser ships either the KeyframeEffect() or DocumentTimeline() constructor so the Animation() constructor is basically useless at this point so I can't imagine there being a compatibility issue.

@birtles
Copy link
Contributor Author

birtles commented Dec 19, 2017

I will update the tests in web-platform-tests as part of fixing this in Gecko (bug 1426050).

@bzbarsky
Copy link

I find that calling new Animation(new KeyframeEffect(…)) and new Animation(new KeyframeEffect(…), undefined) and having the first one set timeline to document.timeline and the other to undefined is not intuitive to a JS developer

Those two constructor calls have identical behavior, per WebIDL. In both cases, the optional argument is missing. The old spec text used to say "not provided", which doesn't have a defined meaning, but had it said "missing" the behavior would be clearly specified to use the document timeline in both cases.

Specifically, see https://heycam.github.io/webidl/#es-overloads step 14 substep 4 subsubstep 2 (what happens if you pass undefined explicitly) and compare to step 15 substep 2 (what happens if you don't pas anything).

@bzbarsky
Copy link

ccing @graouts because apparently that got lost in the issue move. Please see #2078 (comment)

@birtles
Copy link
Contributor Author

birtles commented Jan 10, 2018

Discussed this yesterday with @graouts and agreed to revert the change and clarify the wording from "not provided" to "missing".

birtles added a commit that referenced this issue Jan 10, 2018
@birtles
Copy link
Contributor Author

birtles commented Jan 10, 2018

Updated the language in 4bf5657.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants