Closed Bug 1064937 Opened 10 years ago Closed 8 years ago

CSS Animations should support @keyframes animations of non-interpolable properties

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: dbaron, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Bug 1064937 - Part 1: CSS Animations and Transitions should support transitions/animations of non-interpolable properties.
58 bytes, text/x-review-board-request
birtles
: review+
pbro
: review+
Details
Bug 1064937 - Part 2: Add tests.
58 bytes, text/x-review-board-request
birtles
: review+
Details 
The CSS WG previously resolved for CSS animations, and a few minutes ago resolved for CSS transitions, that non-interpolable properties should be supported.

They should flip when the *timing function* crosses 50%.  This produces particularly good results for step-start and step-end.

There's some web-compat risk here; we might back off by either:
 * only doing this for step-start, step-end, step-mid timing functions (or equivalent steps(1), or
 * only doing it for CSS Animations and not CSS Transitions
Just some notes in case Ryo decides to look into this. At first glance, some of the things we will probably need to do are:

* Add a new animation type to the nsStyleAnimType in layout/style/nsCSSProps.h
  This is because we need to distinguish between properties where we do the 50% switch, and properties which we should not try to animate at all (at least animation-*, transition-* properties, but there may be others; 'display' is probably one)
* Update layout/style/nsCSSPropList.h to replace most of uses of eStyleAnimType_None with eStyleAnimType_Discrete
* Implement the 50% switching, probably in layout/style/StyleAnimationValue.cpp I think
* Write tests. We will need to update layout/style/test/property_database.js I think.
* We'll probably also need a pref to turn this off in case we hit compatibility issues.

There are probably a lot of other things we will need to adjust, but that should give you enough to get started.
Assignee: nobody → daisuke
I ended up having to do a fair bit of this in bug 1245748.

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad005f05833e

There's still likely some work here to make properties that aren't marked as having enum animation type be able to be animated, however.
See bug 1260655 comment 42 for details of what still needs to be done here.
Status: NEW → ASSIGNED
Depends on: 1260655
(In reply to Brian Birtles (:birtles, away 13 April) from comment #3)
> See bug 1260655 comment 42 for details of what still needs to be done here.

For example, we currently have 'flex-direction' marked as eStyleAnimType_EnumU8 but 'align-items' marked as eStyleAnimType_None. Both are marked in the spec as "Animatable: no" so I'm not sure why we have chosen different animation types for them. We probably need to go through all those marked as eStyleAnimType_None and decide if they should be eStyleAnimType_EnumU8. There may be other types two that take some sort of string that should be animatable and we'll need to find a suitable animation type to represent them.

We should probably look at what Chrome and other browsers do.

On the specification side, I have an action[1] to update all the CSS specs from something like:

  Animatable: no

to

  Animation type: discrete

That will let us distinguish between properties that really shouldn't be animatable in any sense (e.g. 'animation-duration').

At the same time I need to fix Web Animations to describe "animation types" instead of "animation behaviors" (and rename the existing "animation type" concept).

[1] https://github.com/w3c/csswg-drafts/issues/72
Daisuke has put together a tool to test the animation behavior of different browsers for each property:

  http://dadaa.github.io/IsAnimatableCSS/

There might be a few missing properties however.

I am currently summarizing the results here:

  https://docs.google.com/spreadsheets/d/1apb5mEiom_yFieSmm19TTcMd3X_S5VcLyGHJcB6i0i8/edit#gid=0

Based on that I plan to present a recommendation to the CSSWG for the edits I intend to make (i.e. which properties to mark as "discrete" etc.)
(In reply to Brian Birtles (:birtles. away 29 Apr-1 May) from comment #5)
> I am currently summarizing the results here:
>  
> https://docs.google.com/spreadsheets/d/
> 1apb5mEiom_yFieSmm19TTcMd3X_S5VcLyGHJcB6i0i8/edit#gid=0

Oops, that should be:

  https://docs.google.com/spreadsheets/d/1apb5mEiom_yFieSmm19TTcMd3X_S5VcLyGHJcB6i0i8/edit?usp=sharing
Depends on: 1277433
Keywords: meta
(In reply to Brian Birtles (:birtles) from comment #4)
> (In reply to Brian Birtles (:birtles, away 13 April) from comment #3)
> > See bug 1260655 comment 42 for details of what still needs to be done here.
> 
> For example, we currently have 'flex-direction' marked as
> eStyleAnimType_EnumU8 but 'align-items' marked as eStyleAnimType_None. Both
> are marked in the spec as "Animatable: no" so I'm not sure why we have
> chosen different animation types for them. We probably need to go through
> all those marked as eStyleAnimType_None and decide if they should be
> eStyleAnimType_EnumU8. There may be other types two that take some sort of
> string that should be animatable and we'll need to find a suitable animation
> type to represent them.

The eStyleAnimType_EnumU8 stuff was, I believe, done for properties that were supposed to be animatable with SMIL but not CSS.

In light of the decision to allow all non-interpolable properties to be animated, it's not clear to me that we want to use the current EnumU8 approach to make properties that aren't interpolable animatable in CSS.  We might be able to do something more general with less code.

(It looks like bug 1277433 is supposed to allow discrete animations of flexbox properties.  Did I miss a change that causes the current set of EnumU8 properties to be animatable in CSS?  I'd rather not expose that somewhat-random set of properties; I'd prefer to make this change all at once.)
(In reply to David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) (vacation June 4-12) from comment #7)
> The eStyleAnimType_EnumU8 stuff was, I believe, done for properties that
> were supposed to be animatable with SMIL but not CSS.
> 
> In light of the decision to allow all non-interpolable properties to be
> animated, it's not clear to me that we want to use the current EnumU8
> approach to make properties that aren't interpolable animatable in CSS.  We
> might be able to do something more general with less code.

Ok, I admit I didn't know the the history of that type. The only code specific to that type I can see is in StyleAnimationValue::ExtractComputedValue. Was there some particular no-longer-necessary code you were concerned about?

> (It looks like bug 1277433 is supposed to allow discrete animations of
> flexbox properties.  Did I miss a change that causes the current set of
> EnumU8 properties to be animatable in CSS?  I'd rather not expose that
> somewhat-random set of properties; I'd prefer to make this change all at
> once.)

Yes, bug 1260572. Basically when StyleAnimationValue::Interpolate fails we apply the 50% switch behavior in KeyframeEffectReadOnly::ComposeStyle.[1]

[1] https://hg.mozilla.org/mozilla-central/rev/3baed5c339e3#l1.12
Now that bug 1277433 has landed, is there anything blocking someone just converting all the remaining eStyleAnimType_None properties to eStyleAnimType_Discrete in one go?  Where is the list of properties that are defined to never animate?
(In reply to Cameron McCormack (:heycam) (away 15–18 September) from comment #9)
> Now that bug 1277433 has landed, is there anything blocking someone just
> converting all the remaining eStyleAnimType_None properties to
> eStyleAnimType_Discrete in one go?  Where is the list of properties that are
> defined to never animate?

As far as I know, the only thing blocking it is doing the spec work (which is on my list of things to do later this week although I probably won't finish it this week)--and even that doesn't really need to block it since Chrome is already shipping this.

To the best of my knowledge, the list of properties which is not animatable is:

* animation-*
* transition-*
* display (although we might revisit this in future, it's fine to make it non-animatable for now)
Also, I guess we don't want to make any of the properties defined in CSS_PROP_LIST_EXCLUDE_INTERNAL blocks animatable, except probably for -moz-system-font so that animations like:

  @keyframes {
    from { font: message-box; }
    to   { font: caption; }
  }

switch the system font used at the right time.
Keywords: meta
Also, we will support logical properties in another bug 1309752 because animation of the properties are failed an assertion with current implementation.
# failed at here.
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleData.h#81
Comment on attachment 8801006 [details]
Bug 1064937 - Part 1: CSS Animations and Transitions should support transitions/animations of non-interpolable properties.

https://reviewboard.mozilla.org/r/85810/#review86430

r=me assuming we can determine whether the following three properties should be animatable or not:
* contain
* will-change
* -moz-binding

::: layout/style/nsCSSPropList.h:633
(Diff revision 3)
>  CSS_PROP_DISPLAY(
>      -moz-binding,
>      binding,
>      CSS_PROP_DOMPROP_PREFIXED(Binding),
>      CSS_PROPERTY_PARSE_VALUE,
>      "",
>      VARIANT_HUO,
>      nullptr,
>      CSS_PROP_NO_OFFSET,
> -    eStyleAnimType_None) // XXX bug 3935
> +    eStyleAnimType_Discrete) // XXX bug 3935

I wonder if we should just leave this. Does it make sense to animation -moz-binding?

::: layout/style/nsCSSPropList.h:1561
(Diff revision 3)
>  CSS_PROP_DISPLAY(
>      contain,
>      contain,
>      Contain,
>      CSS_PROPERTY_PARSE_VALUE |
>          CSS_PROPERTY_VALUE_PARSER_FUNCTION |
>          CSS_PROPERTY_FIXPOS_CB,
>      "layout.css.contain.enabled",
>      // Does not affect parsing, but is needed for tab completion in devtools:
>      VARIANT_HK | VARIANT_NONE,
>      kContainKTable,
>      CSS_PROP_NO_OFFSET,
> -    eStyleAnimType_None)
> +    eStyleAnimType_Discrete)

I wonder if 'contain' should be animatable. I wonder what impact that would have on the optimizations mentioned in bug 1150081. It doesn't look like any of the people involved in that bug are still available. Perhaps we can ask dholbert when he gets back from PTO.

::: layout/style/nsCSSPropList.h:4398
(Diff revision 3)
>  CSS_PROP_DISPLAY(
>      will-change,
>      will_change,
>      WillChange,
>      CSS_PROPERTY_PARSE_FUNCTION |
>          CSS_PROPERTY_VALUE_LIST_USES_COMMAS,
>      "",
>      0,
>      nullptr,
>      CSS_PROP_NO_OFFSET,
> -    eStyleAnimType_None)
> +    eStyleAnimType_Discrete)

I suspect we don't want to make will-change animatable. Does Chrome make it animatable?

::: layout/style/test/test_animations_omta.html:185
(Diff revision 3)
>  <pre id="test">
>  <script type="application/javascript">
>  "use strict";
>  
>  /** Test for css3-animations running on the compositor thread (Bug 964646) **/
> - 
> +

Nit: It might be nice to make these whitespace changes in another patch but since there are only 3~4 of them it's not a big deal.
Attachment #8801006 - Flags: review?(bbirtles) → review+
Comment on attachment 8801007 [details]
Bug 1064937 - Part 2: Add tests.

https://reviewboard.mozilla.org/r/85812/#review86438

Thanks for doing this!

::: dom/animation/test/mozilla/file_discrete-animations.html:4
(Diff revision 3)
> +<!doctype html>
> +<head>
> +<meta charset=utf-8>
> +<title>Test setting easing in sandbox</title>

This is not right.

::: dom/animation/test/mozilla/file_discrete-animations.html:11
(Diff revision 3)
> +</head>
> +<body>
> +<script>
> +"use strict";
> +
> +const gCSSProperties = {

Call this gMozillaSpecificProperties to make it clear we are only testing Mozilla-specific properties?

Then add a comment describing where the tests for standardized properties live?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:209
(Diff revision 3)
> +    ]
> +  },
> +  "content": {
> +    // https://drafts.csswg.org/css-content-3/#propdef-content
> +    tests: [
> +      discrete("url(\"http://localhost/test-1\")", "url(\"http://localhost/test-2\")")

I don't think this needs to be a URL. It could just be a string like "a" and "b".

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:635
(Diff revision 3)
>      tests: [
> -      discrete("fill", "none")
> +      discrete("visibleFill", "visiblePainted")
> +    ]

What's wrong with 'fill' and 'none' ?
Attachment #8801007 - Flags: review?(bbirtles) → review+
Thank you so much for doing this. Ideally it would be good to fix bug 1309752 in the same version but maybe that's less important since I think we're the only browser that supports those properties?
(In reply to Brian Birtles (:birtles, high review load) from comment #23)
> Comment on attachment 8801006 [details]
> Bug 1064937 - Part 1: CSS Animations and Transitions should support
> transitions/animations of non-interpolable properties.
> 
> https://reviewboard.mozilla.org/r/85810/#review86430
> 
> r=me assuming we can determine whether the following three properties should
> be animatable or not:
> * contain
> * will-change
> * -moz-binding

I discussed with Brian about the properties.
As the result, we do same as the specs.
Support 'contain' and 'will-change'
* https://drafts.csswg.org/css-containment/#propdef-contain
* https://drafts.csswg.org/css-will-change/#propdef-will-change
Don't support '-moz-binding'
* https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-binding
Any chance of landing this soon?  This will conflict bug 1305325 and bug 1312301.

While I was working on bug 1312301, I noticed that 'word-wrap' is missed in test cases in part 2 patch. Also 
'scroll-snap-coordinate', 'scroll-snap-destination', 'scroll-snap-points-x/y' were removed from the latest spec, we should drop them from the test case.
Flags: needinfo?(daisuke)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> Any chance of landing this soon?  This will conflict bug 1305325 and bug
> 1312301.

I'm sorry for my late.
Maybe yes. I'll modify today the points which Brian reviewed.

> While I was working on bug 1312301, I noticed that 'word-wrap' is missed in
> test cases in part 2 patch. 

I re-confirm this.

> Also 'scroll-snap-coordinate', 'scroll-snap-destination',
> 'scroll-snap-points-x/y' were removed from the latest spec, we should drop
> them from the test case.

Okay, I'll move these tests into mozilla/file_discrete-animations.html since we have to test anyway.
Flags: needinfo?(daisuke)
(In reply to Daisuke Akatsuka (:daisuke) from comment #28)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> > Any chance of landing this soon?  This will conflict bug 1305325 and bug
> > 1312301.

> > While I was working on bug 1312301, I noticed that 'word-wrap' is missed in
> > test cases in part 2 patch. 

I discussed with Brian. 
As the result, we drop this test.
The spec is,
https://drafts.csswg.org/css-text-3/#propdef-word-wrap
"For legacy reasons, UAs must treat word-wrap as an alternate name for the overflow-wrap property, as if it were a shorthand of overflow-wrap. "

> > Also 'scroll-snap-coordinate', 'scroll-snap-destination',
> > 'scroll-snap-points-x/y' were removed from the latest spec, we should drop
> > them from the test case.

Also, we drop these tests since they are deprecated.
Hi Patrick,
Could you review following devtools test code in part 1 patch?
https://reviewboard.mozilla.org/r/85810/diff/4#0

We converted many CSS properties to animatable in this bug.
This test code that I want your review refers devtools/client/animationinspector/test/doc_keyframes.html
The document is using 'background' shorthand property to make animation.
The following extracted longhands became to animatable. 
  "background-attachment",
  "background-clip",
  "background-image",
  "background-origin",
  "background-repeat",
Therefore, I modified the test.

Thanks!
Flags: needinfo?(pbrosset)
Comment on attachment 8801006 [details]
Bug 1064937 - Part 1: CSS Animations and Transitions should support transitions/animations of non-interpolable properties.

https://reviewboard.mozilla.org/r/85810/#review91514

The devtools test changes look good. Thank you Daisuke.
Attachment #8801006 - Flags: review?(pbrosset) → review+
Flags: needinfo?(pbrosset)
Comment on attachment 8801007 [details]
Bug 1064937 - Part 2: Add tests.

https://reviewboard.mozilla.org/r/85812/#review92236

::: dom/animation/test/mozilla/file_discrete-animations.html:4
(Diff revisions 3 - 4)
>  <!doctype html>
>  <head>
>  <meta charset=utf-8>
> -<title>Test setting easing in sandbox</title>
> +<title>Test Mozilla specific discrete animatable properties</title>

Nit: Mozilla-specific

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:462
(Diff revisions 3 - 4)
>      ]
>    },
>    "justify-content": {
>      // https://drafts.csswg.org/css-align/#propdef-justify-content
>      tests: [
> -      discrete("baseline", "last-baseline")
> +      discrete("baseline", "last baseline")

last-baseline is correct, right?

https://drafts.csswg.org/css-align/#typedef-baseline-position

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:636
(Diff revisions 3 - 4)
>      ]
>    },
>    "pointer-events": {
>      // https://svgwg.org/svg2-draft/interact.html#PointerEventsProperty
>      tests: [
> -      discrete("visibleFill", "visiblePainted")
> +      discrete("fill", "none")

(What was wrong with the previous values?)
(In reply to Brian Birtles (:birtles) from comment #36)
> > -      discrete("baseline", "last-baseline")
> > +      discrete("baseline", "last baseline")
> 
> last-baseline is correct, right?
> 
> https://drafts.csswg.org/css-align/#typedef-baseline-position

Nope, "last baseline" (no hyphen) is correct. The spec is just lagging on an update there.  References: https://github.com/w3c/csswg-drafts/issues/210#issuecomment-256411592 and bug 1313254 (where we dropped the hyphen).
(In reply to Daniel Holbert [:dholbert] from comment #37)
> (In reply to Brian Birtles (:birtles) from comment #36)
> > > -      discrete("baseline", "last-baseline")
> > > +      discrete("baseline", "last baseline")
> > 
> > last-baseline is correct, right?
> > 
> > https://drafts.csswg.org/css-align/#typedef-baseline-position
> 
> Nope, "last baseline" (no hyphen) is correct. The spec is just lagging on an
> update there.  References:
> https://github.com/w3c/csswg-drafts/issues/210#issuecomment-256411592 and
> bug 1313254 (where we dropped the hyphen).

Thanks Brian, Daniel!
I leave it as it is (no hyphen).
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/073a6c76e49a
Part 1: CSS Animations and Transitions should support transitions/animations of non-interpolable properties. r=birtles,pbro
https://hg.mozilla.org/integration/autoland/rev/1b48d4300536
Part 2: Add tests. r=birtles
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/073a6c76e49a
https://hg.mozilla.org/mozilla-central/rev/1b48d4300536
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
It looks like (from a testcase I wrote, and birtles seems to agree) that this implemented animation of non-interpolable properties only for CSS Animations and not for CSS Transitions.  Updating the summary to reflect that.  (It's also not great that the commit message says that it did CSS Transitions, since that makes the commit history confusing.  Also, in general, commit messages should generally describe the changes being made and not be copied from the bug summary.)

Could you file a followup bug on making the same change for transitions, since that wasn't done in this bug?  The current spec says that we should change for transitions, although it seems likely not to be web-compatible.  I think either we or Chrome will need to try doing it to demonstrate that it's not.
Flags: needinfo?(daisuke)
Summary: CSS Animations and Transitions should support transitions/animations of non-interpolable properties → CSS Animations should support @keyframes animations of non-interpolable properties
Thank you very much, David.
I filed a bug for CSS Transition( bug 1320854 ).

Also, I'm so sorry, I'll be more careful about commit message.

Thanks.
Flags: needinfo?(daisuke)
I have a few quick question with regards to the MDN docs here: 
- Does that mean that *all* properties that were not animatable before are now animatable (in a discrete way)? [I'm mostly asking for layout properties like display or the box alignment ones]
- Will *future* property be at least animatable, by discrete jumps, by default? We sometimes implement properties that are marked as not animatable, but in fact it now means that they will be animated by discrete jumps. Correct?

Thanks in advance.
Flags: needinfo?(dakatsuka)
The only non-animatable properties are animation-* properties, transition-* properties, and the display property. All other properties are animatable. That said, we don't currently support animation of logical properties, I believe. Also, we don't support animation of custom properties yet.
... which I suppose are covered by bug 991950 and bug 1309752?
Yes, although I believe the yet-to-land patches in bug 1273706 cover animation for custom properties.

Also, it's worth mentioning that for transitions, according to the spec, these properties that animate by discrete jumps should animate in the same way for transitions. We don't implement that yet but are there is work in progress on that in bug 1320854. That said, both David and I suspect this will not be web-compatible and will ultimately be backed out and the spec updated (once we can prove to the CSSWG it's not web-compatible). If that happens, there will be a class of properties that are discretely animatable by CSS animations / Web animations but not by transitions.
Thanks, we updated correctly our database then :-) 
There also is a mention there: https://developer.mozilla.org/en-US/Firefox/Releases/52#Changes_and_removals

Marking this as complete!
Flags: needinfo?(dakatsuka)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: