Replace AnimationEffectTiming(ReadOnly) with setTiming() ? #168

Open
birtles opened this Issue Sep 14, 2016 · 10 comments

Projects

None yet

6 participants

@birtles
Contributor
birtles commented Sep 14, 2016

Just a thought about how we could simplify the API and make it more consistent.

Currently, we manage timing properties using the following interfaces:

  • AnimationEffectTimingProperties — property bag for passing to constructors etc.
  • ComputedTimingProperties — extends AnimationEffectTimingProperties with some extra properties we compute
  • AnimationEffectTimingReadOnly — Live object with getters for reading the specified timing properties. Fortunately, this happens to have the same members as AnimationEffectTimingProperties so you can pass it to constructors/methods that expect a AnimationEffectTimingProperties and it should work.
  • AnimationEffectTiming — Subclass of AnimationEffectTimingReadOnly that adds getters for the different parameters (used by mutable effects, i.e. those not tied to CSS)

In practice it looks like this:

// Set up a new animation
const anim = elem.animate({...}, { duration: 2000, fill: 'auto' });

// Read back the specified values
anim.effect.timing.duration; // 2000
anim.effect.timing.fill; // 'auto'

// Read back computed values
anim.effect.getComputedTiming().duration; // 2000
anim.effect.getComputedTiming().fill; // 'none'
// NOTE: |fill| and |duration| are a little bit special in that they allow 'auto'
// values that get computed into something else. It's not particularly useful, though,
// until we have groups (where, e.g., a group's auto duration computes to the length
// of its children).

// Read other computed values
anim.effect.getComputedTiming().activeDuration // 2000
anim.effect.getComputedTiming().progress // 0.0

// Update specified values
anim.effect.timing.duration *= 2;
anim.effect.timing.iterations++;

The parallel here is with style:

  • anim.effect.timing =~ elem.style (sort of, kind of)
  • anim.effect.getComputedTiming =~ window.getComputedStyle(elem)

However, for keyframes we do something completely different. We don't have live keyframe objects since that would be hard and instead we just use getKeyframes() and setKeyframes().

In addition, keyframe offsets can be computed. The author can omit them and we'll fill them in. We don't just fill in the offset member with the computed value when we return keyframes using getKeyframes(), however, since if we did that you couldn't take the result of getKeyframes() and pass it to setKeyframes() on another effect without freezing the offsets at that point (and losing the ability to auto-distribute keyframes from then on). Instead, we return the computed offset as a separate member: computedOffset.

So my thought here was:

  • Drop AnimationEffectTimingReadOnly.
  • Drop AnimationEffectTiming.
  • Drop effect.timing.
  • Rename effect.getComputedTiming() to effect.getTiming().
  • Extend ComputedTimingProperties to include computedDuration and computedFill.
  • Add effect.setTiming() that takes an AnimationEffectTimingProperties (which ComputedTimingProperties extends so you could pass the result of getTiming() to setTiming()).

Advantages:

  • 2 fewer interfaces
  • Fewer live objects
  • Consistency within the API — keyframes and timing properties follow the same idiom
  • Bulk setting of timing properties is easier

Disadvantages:

  • More awkward to update an individual timing property, e.g.
// Current API
anim.effect.timing.duration *= 2;
// Proposed API
const updatedTiming = anim.effect.getTiming();
updatedTiming.duration *= 2;
anim.effect.setTiming(updatedTiming);
  • More inefficient to update individual timing properties since you need to read and write these property bags. From what I understand, at least for Gecko, that can be quite slow.

I do wonder how common those last two items are, though. (And for the first one, if you're just blindingly overwriting the value, you can use Object.assign() or, one day, the object spread operator to write it more compactly.)

The other consideration is this proposal may be less forwards compatible. For example, suppose we extend iterations to take a keyword value. Presumably it will still compute to a float. Code that currently calls anim.effect.getComputedTiming().iterations would continue to work when this new property is used. However, under this proposal, code that calls anim.effect.getTiming().iterations would need to be updated to call anim.effect.getTiming.computedIterations instead.

That said, I think we already have this problem in that anyone currently calling anim.effect.timing.iterations and assuming it will be a float (which I expect is not uncommon since it's less to type than anim.effect.getComputedTiming().iterations) will have the same problem.

@shans, @dstockwell Any thoughts about this?

@heycam, @bzbarsky Is returning snapshot objects generally the preferred approach over maintaining live objects?

@birtles birtles changed the title from Replace `AnimationEffectTiming(ReadOnly)` with `setTiming()` ? to Replace AnimationEffectTiming(ReadOnly) with setTiming() ? Sep 14, 2016
@alancutter
Contributor

One slight downside to dropping effect.timing is its no longer possible to see the specified value of fill or duration as they get "resolved" in getTiming().

Dropping the live effect.timing object SGTM though, I don't think there are many use cases for it not being a snapshot.

@birtles
Contributor
birtles commented Sep 14, 2016

One slight downside to dropping effect.timing is its no longer possible to see the specified value of fill or duration as they get "resolved" in getTiming().

The proposal here is that getTiming().fill returns the specified value, e.g. "auto", and we add a computedFill member for the computed value, e.g. "none". Likewise for duration.

@bzbarsky

Is returning snapshot objects generally the preferred approach over maintaining live objects?

It really depends on the situation and the design considerations (performance tradeoffs, how the objects will typically be used, etc).

@birtles
Contributor
birtles commented Sep 20, 2016

@BorisChiou, @hiikezoe let me know if you have any thoughts. Just to clarify one point of confusion, the idea is that you'd have:

anim.effect.getTiming().duration // "auto"
anim.effect.getTiming().computedDuration // 0
@hiikezoe

I like this idea.
We do update individual property in many places in test code but I don't think it's common to change individual property dynamically in the real world.

@shans
Contributor
shans commented Sep 29, 2016

The More awkward to update an individual timing property disadvantage is a major disadvantage, I think.

Modification of animations in-flight is a major capability that the unshipped parts of the API will give us, and this proposal makes that significantly harder for timing properties.

Anecdata-ly, I found myself doing this a reasonable amount when playing with the polyfill.

In contrast, looking at the advantages:

  • 2 fewer interfaces - this means nothing to web developers.
  • Fewer live objects - sort of, but it's not likely that people will actually 'tear off' and store the timing.
  • Consistency within the API — keyframes and timing properties follow the same idiom - obviously we're trading off consistency with the style approach here. Not sure we buy anything.
  • Bulk setting of timing properties is easier - not really, unless you're talking about the very specific use case of bulk updating a large set of animations to all have exactly the same timing. Even here, wrapping the update in a loop will end up looking almost exactly the same.
@dstockwell
Contributor

The More awkward to update an individual timing property disadvantage is a major disadvantage, I think.

I thought that's how we ended up with the current design. Getting/setting timing being a more frequent use case than getting/setting keyframes, so we thought it OK to be different/harder.

@birtles
Contributor
birtles commented Sep 29, 2016

I don't think setting individual timing properties after setting up the animation is common since in most cases doing that that will cause the animation to jump which is generally undesirable. (Test code etc. being obviously quite different since you're not concerned with smooth motion.)

Regarding bulk updating, it was partly an observation from seeing people like Paul & Surma use the API and store a separate timing object for passing to animate(). It seems a bit odd that you can pass that to animate() but beyond that point you can't use it to update an animation.

I think consistency within the API is more important than consistency with the style API since internal consistency trumps external consistency and it's not actually a good parallel with the style API anyway (since the style API doesn't actually represent specified style, but only style specified on the style attribute).

@birtles
Contributor
birtles commented Nov 4, 2016
@birtles birtles added the affects API label Nov 9, 2016
@birtles
Contributor
birtles commented Jan 10, 2017

Just to close the loop on this, I discussed this with Shane and Ian in November 2016 and I think we were thinking that with the addition of setTimingProperty for updating an individual property, this might be ok.
See meeting minutes.

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