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] Drop the *ReadOnly interfaces #2068

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

[web-animations-1] Drop the *ReadOnly interfaces #2068

birtles opened this issue Dec 5, 2017 · 10 comments

Comments

@birtles
Copy link
Contributor

birtles commented Dec 5, 2017

From @birtles on April 7, 2017 6:27

Originally we introduced the *ReadOnly interfaces for CSS. The logic went something like this:

  • It's pretty much impossible to map changes made via API back to style (we tried, and there were lots of cases where it's just not clear what the expected behavior should be, particularly when the list length changes.)
  • So we decided to make it a one-way mapping, i.e. style updates the API objects but changing the objects doesn't update style.
  • As a result you need to know when an object gets a particular timing property from style and when it gets it from the API.
  • We tried doing this on a property-by-property basis but it leads to a very confusing API. How do you tell which properties you've overridden? How do you reset them so that they start reflecting style again? Do we add three methods per property?
  • Instead we opted for an API where you very deliberately have to break from style as follows:
const animation = div.getAnimations()[0];
animation.effect = new KeyframeEffect(animation.effect); // Make a mutable copy
// Now you can tweak it and its obvious that changes to style do not apply
effect.timing.duration = 3000; // This works
div.style.animationDuration = '4s'; // This doesn't

However, in #168 I'm proposing we simplify the timing interfaces to have only:

animation.effect.getTiming();
animation.effect.setTiming(); // Resets all properties to their defaults
animation.effect.setTimingProperty({ duration: 3000 }; // Overrides a single property

The last one is basically equivalent to:

effect.setTiming(Object.assign(effect.getTiming(), { duration: 3000 }));

I'm now wondering if there's some way we can exploit this setup (and similarly setKeyframes()) to signal which properties you want to override. Perhaps we could change the timing dictionary to have default values which we interpret as the initial value for script-generated animations and the style-specified value for CSS animations/transitions such that setTiming() resets everything?

That would certainly be nicer for authors: being able to override the delay of a single instance of a CSS animation programmatically is certainly something I've wanted to do, and it would be nice not to have to completely divorce it from style changes in order to do so. It would also avoid some of the confusion and complexity around the *ReadOnly interfaces so it's probably worth considering at least.

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

@birtles
Copy link
Contributor Author

birtles commented Dec 5, 2017

From @alancutter on April 7, 2017 6:58

What happens when you do

const animation = div.getAnimations()[0];
animation.effect.setTimingProperty({ duration: 3000 });
div.style.animationDuration = '4s';

under this new scheme?

@birtles
Copy link
Contributor Author

birtles commented Dec 5, 2017

I think you're saying you want to override the duration so we should ignore the value coming from the cascade for that property. That is, the duration is 3s.

(But this isn't so much a concrete proposal as a memo that we should double-check the *ReadOnly approach is the one we want before we ship.)

@birtles
Copy link
Contributor Author

birtles commented Dec 5, 2017

From @alancutter on April 10, 2017 4:15

Looking at your example of the current interfaces:

const animation = div.getAnimations()[0];
animation.effect = new KeyframeEffect(animation.effect); // Make a mutable copy
// Now you can tweak it and its obvious that changes to style do not apply
effect.timing.duration = 3000; // This works
div.style.animationDuration = '4s'; // This doesn't

The current *ReadOnly design seems to only cover most but not all aspects of overriding the cascade. A completely read only design wouldn't allow you to modify animation.effect IMO. I think you'd need to implement the same notion of script overrides for changing animation.effect as you would for animation.effect.setTimingProperty/setKeyframes. Please let me know if my understanding is incorrect.

I would be in favour of all or nothing rather than the current semi-read only design.

@birtles
Copy link
Contributor Author

birtles commented Dec 5, 2017

I think what you're saying is that the current setup lets you override the effect but doesn't let you restore it unless you hold onto the old effect?

I think we definitely want to allow modifying generated CSS animations/transitions. That's important for extending those functions (e.g. by making a CSS transition that goes via some intermediate state, or making a transition on one property update another property too--all the while still allowing those transitions to be cancelled in the normal way). The question is just about the mechanism we use to allow overriding. Is it a "break the connection once" interaction, or a property-by-property override.

@birtles
Copy link
Contributor Author

birtles commented Jan 10, 2018

What is the status of this, do you really plan to remove the interfaces?

Yes. This is currently blocked on #2065 which I hope to get to in the next couple of weeks.

@birtles
Copy link
Contributor Author

birtles commented Jan 12, 2018

Thinking aloud, what if:

  • We track which properties have been overridden on a property-by-property basis . e.g. cssAnim.effect.updateTiming({ duration: 3000 }) means we'll ignore subsequent changes to the computed value of animation-duration.

    • cssAnim.effect.setTiming(...) would mean we'll ignore changes to all timing-related properties since it sets them all.
    • cssAnim.effect.setKeyframes(...) would likewise mean we ignore changes to @keyframes / animation-name.
    • We might want to revisit the quite complex mapping for animation-play-state to make it also follow this once-overridden-always-overridden behavior.
  • We don't provide a way to tell which properties have been overridden, or at least not initially.

  • We don't provide a way to reset the overridden properties so that they resume tracking changes to markup.

    • In future we could add cssAnim.effect.resetTiming() for this and something similar for keyframes, but only if there are use cases/demand for it. That would imply introducing a subclass of KeyframeEffect to be used by CSS animations/transitions that includes resetTiming() and resetKeyframes() since they wouldn't make sense on script-generated animations but that should be fine and possible to introduce later without breaking content--I hope.
  • cssAnim.effect = new KeyframeEffect(...) continues to break all association with markup except animation-play-state.

  • We possibly would no longer need the KeyframeEffect copy constructor now that it's less common to need to duplicate a KeyframeEffectReadOnly in order to get a mutable version. Would clone() be a more idiomatic way to create a copy?

    If we don't add this to the API the equivalent JS would be something like:

const clone = new KeyframeEffect(
  src.target,
  Object.assign(src.getTiming(), {
    composite: src.composite,
    iterationComposite: src.iterationComposite,
  }),
  src.getKeyframes()
);

Which is complex enough that it's probably worth adding this.

CC @graouts, @flackr, @stephenmcgruer

@graouts
Copy link
Contributor

graouts commented Jan 16, 2018

I really like your latest proposal @birtles. It feels natural to me as a (mostly) JS developer, once you've written one property, the link is gone and it's all JS from now on. This embraces one of the core benefits of the Web Animations API, which is that there is now an explicit, typed and easy-to-use API to program animations, where the CSSOM was just not a convenient way to do this before.

Curious to see what Google folks think, but this feels right.

@flackr
Copy link
Contributor

flackr commented Feb 2, 2018

This makes sense to me as well, though as I mentioned on #2065 I feel it is slightly more idiomatic to have a modifiable object of override properties - as it's roughly equivalent to what developers have with CSS style, inline style and computed style. That being said, it's probably not worth the complexity it would add.

birtles added a commit to birtles/csswg-drafts that referenced this issue Mar 12, 2018
birtles added a commit to birtles/csswg-drafts that referenced this issue Mar 16, 2018
birtles added a commit that referenced this issue Mar 16, 2018
@birtles
Copy link
Contributor Author

birtles commented Mar 16, 2018

Closed by 953041f

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

4 participants
@graouts @flackr @birtles and others