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] Allow setting timeline in Element.animate() #5013

Closed
majido opened this issue Apr 27, 2020 · 6 comments · Fixed by #5033
Closed

[web-animations-1] Allow setting timeline in Element.animate() #5013

majido opened this issue Apr 27, 2020 · 6 comments · Fixed by #5033

Comments

@majido
Copy link
Contributor

majido commented Apr 27, 2020

Proposal

Web Animations allows creation of AnimationTimeline but it does not support using a custom timeline in element.animate() method. The only option is to use Animation constructor directly.

I believe as we get closer to having ScrollTimeline the ability to continue to use the familiar and simple to use Element.animate() for scroll linked animations is important. This issue is here to consider introducing a way to to pass in a timeline as part of Element.animate(keyframes, options).

Syntax Options

If it is decided that we want this, here are some ideas on syntax:

Extend options to have a timeline member

This is an example of how it may work:

const timeline = new ScrollTimeline({source: $some_scroller, timeRange: 1000});
$div.animate({opacity: [0, 1]}, {delay: 100, duration: 500, timeline: timeline});

Pros:

  • Simple to undestand
  • keeps timing related options in a single dictionary (I consider timeline a timing related option)
    Cons:
  • At the moment spec passes options verbatim to KeyframeEffect but timeline is an animation concept so we have to separate these in spec before passing the options to keyframes. This is not a big deal IMHO.

FWIW, this is the method that @flackr has chosen in their scroll-timeline polyfill.

Add a third parameter just for timeline

This is an example of how it may work:

const timeline = new ScrollTimeline({source: $some_scroller, timeRange: 1000});
$div.animate({opacity: [0, 1]}, {delay: 100, duration: 500}, {timeline: timeline} /* or just timeline */);

I find this a bit less ergonomic that the previous one. It may be more appealing if there were other animation specific options.

@birtles
Copy link
Contributor

birtles commented Apr 27, 2020

* At the moment spec passes options verbatim to KeyframeEffect but timeline is an animation concept so we have to separate these in spec before passing the options to keyframes. This is not a big deal IMHO.

I don't think there's any need to separate these out. We already have KeyframeAnimationOptions which augments KeyframeEffectOptions with the id member that only applies to animations (as opposed to keyframe effects). And we just pass the options parameter to animate() straight along to the KeyframeEffect constructor. I think it would be fine to do the same here.

Add a third parameter just for timeline

Since we're already putting animation options in the options parameter, I think we should continue to do so.

You'll probably need to be careful, however, to match the semantics of the timeline parameter to the Animation() constructor, however where there is a distinction between passing null (explicitly setting the timeline to null) and not passing anything (uses the default document timeline). I can't remember what WebIDL does if you explicitly pass undefined, however, or whether you can produce that same behavior with a dictionary member.

@majido
Copy link
Contributor Author

majido commented Apr 30, 2020

Thanks for the feedback. Let's add timeline to KeyframeAnimationOptions which seems to be fairly straightforward.

As for matching the semantic of timeline param in Animation ctor, I believe if we use an optional nullable type then we should be able to differentiate between "explicit null", "not passing anything" cases.

For example I think the following IDL and text should give the desired behavior with the exact text as Animation constructor:

dictionary KeyframeAnimationOptions : KeyframeEffectOptions {
    DOMString id = "";
    AnimationTimeline? timeline;
};

Animation constructor text:

Run the procedure to set the timeline of an animation on animation passing
timeline as the new timeline or, if a timeline argument is missing,
passing the default document timeline of the Document associated with the
Window that is the current global object.

Unspecified value case:

Per Webidl an unspecified optional dictionary member is considered missing (Note that dictionary members are optional unless they are explicitly marked as required). So for:

 div.animate({/* keyframes*/ }, { duration: 1000});

We should receive a missing timeline value which we trun into
the default document timeline.

Explicit null case:

Since type is nullable passing null is valid and we will get null value which
we can pass into set the timeline of an animation procedure.

 div.animate({/* keyframes*/ }, { duration: 1000, timeline: null});

@birtles
Copy link
Contributor

birtles commented May 1, 2020

Right, I was mostly just curious what happens when you explicitly specify { timeline: undefined }.

I believe WebIDL makes that the same as { timeline: null } (spec ref) which is not very idiomatic JS (since JSON.stringify({ a: null }) !== JSON.stringify({ a: undefined }) but JSON.stringify({ a: undefined }) === JSON.stringify({})) but oh well.

@majido
Copy link
Contributor Author

majido commented May 7, 2020

I got confused by the spec section that you linked because it contradicts with this note in WebIDL spec regarding the dictionary.

In the ECMAScript binding, a value of undefined for the property corresponding
to a dictionary member is treated the same as omitting that property. Thus, it 
will cause an error if the member is required, or will trigger the default value if
one is present, or will result in no entry existing in the dictionary value otherwise.

But I think I now have figured out the source of the confusion.

The conversion process for turning an ECMAScript Object values into an IDL dictionary
(spec ref) handles undefined members differently than the conversion process for a nullable type in general (which is the one you referenced).

Here is the key part of the algorithm that converts a member of the object into a member of the dictionary:

esMemberValue <-  ?Get(esDict, key)
if (esMemberValue is defined)
  convert it to IDL type
else if esMemberValue is undefined and member is optional and has a default value
  use default value 
otherwise if member is required
  throw!
// note that if member is optional with no default we do nothing which leaves
// the IDL dict for that key empty!

If I am reading this correctly an explicitly (or implicitly) undefined
objected member in JS does not get mapped to null (even when using a nullable type)
but it either gets a the default value if one is specified in the IDL dictionary or nothing at all.

This is consistent with the note in the spec too.

So I think with our current definition, we get no data members which we handles
correctly. wdyt?

majido added a commit to majido/csswg-drafts that referenced this issue May 7, 2020
@birtles
Copy link
Contributor

birtles commented May 7, 2020

So { timeline: undefined } here behaves differently to passing undefined to the Animation() constructor? That sounds unfortunate.

@majido
Copy link
Contributor Author

majido commented May 7, 2020

No I think they behave the same.

Correct me if I am wrong but here is the current Animation() behavior w.r.t timelines:

  • explicit null: new Animation(null, null) => animation.timeline is null
  • explicit undefined: new Animation(null, undefined) => animation.timeline is document timeline
  • implicit undefined: new Animation(null) => animation.timeline is document timeline

My quick test shows the same in both Firefox and Chrome.

Here is the proposed IDL and spec text which I also have #5033:

dictionary KeyframeAnimationOptions : KeyframeEffectOptions {
    DOMString id = "";
    AnimationTimeline? timeline;
};

and the proposed spec text:

if <code>timeline</code> member of
<var>options</var> is missing, be the <a>default document timeline</a>
of the <a>node document</a> of the element on which this method was
called.

In this IDL the timeline is an optional value with no default.

So per earlier discussion of the conversion algorithm, passing { timeline: undefined } and {} both leads to no having a missing member in the converted IDL dictionary which leads to us picking document timeline.

Passing {timeline: null} on the other hand would lead to having an IDL with its timeline member being null which
we set.

So I believe we have the same behavior as Animation().

majido added a commit that referenced this issue May 8, 2020
The options dictionary in `Element.animate` now has an optional nullable timeline
member. This allows timeline to be set via that method in addition to Animation
constructor. The semantic of handling explicit null and implicit/explicit undefined
timelines are the same between the two methods.

Here are some examples of how this may be used:
```
// Create animation with a null timeline.
div.animate({color: 'red' }, { duration: 1000, timeline: null});
```

```
// Create a scroll-linked animation.
div.animate({color: 'red' }, { duration: 1000, timeline: new ScrollTimeline()});
```
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 28, 2020
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 28, 2020
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 1, 2020
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 1, 2020
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 1, 2020
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773733}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 1, 2020
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773733}
pull bot pushed a commit to Yannic/chromium that referenced this issue Jun 1, 2020
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773733}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 3, 2020
….animate(), a=testonly

Automatic update from web-platform-tests
[ScrollTimeline] Add timeline to Element.animate()

This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773733}

--

wpt-commits: a0740be4f6a5f18e016abd3dcadef53177dde234
wpt-pr: 23847
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jun 3, 2020
….animate(), a=testonly

Automatic update from web-platform-tests
[ScrollTimeline] Add timeline to Element.animate()

This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773733}

--

wpt-commits: a0740be4f6a5f18e016abd3dcadef53177dde234
wpt-pr: 23847
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 4, 2020
….animate(), a=testonly

Automatic update from web-platform-tests
[ScrollTimeline] Add timeline to Element.animate()

This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773733}

--

wpt-commits: a0740be4f6a5f18e016abd3dcadef53177dde234
wpt-pr: 23847

Differential Revision: https://phabricator.services.mozilla.com/D78334
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jun 5, 2020
….animate(), a=testonly

Automatic update from web-platform-tests
[ScrollTimeline] Add timeline to Element.animate()

This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773733}

--

wpt-commits: a0740be4f6a5f18e016abd3dcadef53177dde234
wpt-pr: 23847

Differential Revision: https://phabricator.services.mozilla.com/D78334
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This patch adds a timeline option to Element.animate() function based on
the recent spec change: w3c/csswg-drafts#5013.

Change-Id: Ibf7e6f824f9e013f62da015cebdbc893255142dd
Bug: 1080720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220352
Commit-Queue: Yi Gu <[email protected]>
Reviewed-by: Majid Valipour <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#773733}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: d3931637492464d7781d2699c3c549c8f845aec6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants