1846516 - [css-properties-values-api] Support using custom properties in discrete animation.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task, P3)

Comment 1

[Frédéric Wang (:fredw)]

Attachment: var-in-keyframes.html

Here is a basic testcase.

Chrome and WebKit don't seem to interpolate perfectly (for optimization reason?).

Comment 2

[Frédéric Wang (:fredw)]

Attachment: Bug 1846516 - [css-properties-values-api] Animate custom properties in a discrete way. r=fredw,zsun,#animation!,#style

Comment 3

[Frédéric Wang (:fredw)]

Attachment: Bug 1846516 - [css-properties-values-api] Use PropertyDeclarationId/PropertyDeclarationIdSet for animated properties. r=fredw,zsun,#animation!,#style

Comment 4

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1846516 - [css-properties-values-api] Make PropertyDeclarationId::Custom hold an Atom name rather than a reference. r=fredw,zsun,#style

This makes possible to use it as hashmap keys in the animation code
without lifetime management issues.

Comment 5

[Frédéric Wang (:fredw)]

Attachment: Bug 1846516 - [css-properties-values-api] Introduce AnimatedPropertyID/AnimatedPropertIDSet. r=fredw,zsun,#animation!,#style

This will make possible to animate custom properties on Gecko side. For now, the
animation code keeps only dealing with nsCSSPropertyID, so behavior is unchanged.

Comment 6

[Frédéric Wang (:fredw)]

Attachment: Bug 1846516 - [css-properties-values-api] Use AnimatedPropertyID to communicate between Gecko and Servo. r=fredw,zsun,#animation!,#style

Comment 7

[Frédéric Wang (:fredw)]

@Emilio: Hi, this is not finished and I won't be able to make more progress on this for some time (at least until next Monday), but just curious if you think this is the right approach in term of data structure etc. In particular, I'm also not sure how we are supposed to deal with the workaround introduced in bug 1402219 for custom properties (and the related bug 1391537), so I would appreciate any guidance here.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Yeah at a glance using PropertyDeclarationIdSet is the right thing to do to be able to animate custom props.

I don't have all the context in bug 1391537, but I'm not sure how expanding shorthands into longhands is related to custom properties here (in the sense that custom properties are always longhands). Can you clarify?

Comment 9

[Frédéric Wang (:fredw)]

So my basic idea is to use represent custom properties by ID eCSSPropertyExtra_variable + an extra Atom for their custom names. In my set of patches, there are two remaining functions to update, namely Servo_GetComputedKeyframeValues and Servo_StyleSet_GetKeyframesForName. The former was tweaked in bug 1402219 to make custom properties usable in keyframes, but it seems to me that conflicts with the goal of making them animatable, since I would just treat them as other properties. The corresponding change is https://hg.mozilla.org/mozreview/gecko/rev/c4b5b40ce56147427ddd29e9718f5d791ffe7422 which mentions this workaround would go away when bug 1391537 is fixed. So I'm not sure I understand the connection either, but thought you may have a better idea than me. Anyway, I can dig more when I have more time to go back to this.

Comment 10

[Frédéric Wang (:fredw)]

Attachment: animated-length.html

Comment 11

[Frédéric Wang (:fredw)]

Attachment: animated-length-2.html

Comment 12

[Frédéric Wang (:fredw)]

Today's set of patches exhibits the expected animation for the basic testcases I attached to this bug. Unfortunately I'll have to focus on other tasks for a while, so a quick summary of the status:

1. This is actually not implementing interpolation of custom properties, my plan was to do that in a separate 6th patch on top of the others. Need to be implemented (but can probably be postpone until discrete animations are working).

2. For now I'm removing the workaround mentioned in comment 7, so that we can effectively animate custom properties. However, I assume that means bug 1402219 would be failing again. Need to investigate.

3. Some crashes are happening. For example attachment 9361181 [details] (double release of ref-counted Atoms for custom prop names), attachment 8911056 [details] (in mozilla::AnimatedPropertyID::Hash for custom prop) and css/css-properties-values-api/at-property-animation.html (hitting the assert in nsCSSProps::IsShorthand). Need to debug and fix.

4. Some changes made in the 5th patch D190758 should probably be merged in previous patches, but for the latest versions I haven't got a chance to do that and check each part properly builds and cause no CI errors. Need to that before review.

Comment 13

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #12)

1. This is actually not implementing interpolation of custom properties, my plan was to do that in a separate 6th patch on top of the others. Need to be implemented (but can probably be postpone until discrete animations are working).

Also, I forgot to say that the patches do something for CSS transitions but that's untested and I understand it would only make sense for custom properties that can be animated in a non-discrete way.

Comment 14

[Phabricator Automation]

Comment on attachment 9359130 [details]
Bug 1846516 - [css-properties-values-api] Use AnimatedPropertyID to communicate between Gecko and Servo. r=fredw,zsun,#animation!,#style

Revision D191322 was moved to bug 1807253. Setting attachment 9359130 [details] to obsolete.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Zach, reminder about ni?ing me about what you got stuck on from our previous conversation? I'll try to dig into the crashes and such tomorrow otherwise. Thanks!

Comment 17

[Zach Hoffman [:zrhoffman]]

Sorry for not responding sooner. It was crashing in at-property-animation.html when trying to check if a custom property is a shorthand https://phabricator.services.mozilla.com/D191059?vs=on&id=793059#C7129453NL498 by passing the property's mID to nsCSSProps::IsShorthand (which fails on an assert in nsCSSProps::IsShorthand https://searchfox.org/mozilla-central/rev/78a2c17cc80680a5a82446e4ce7c45a73b935383/layout/style/nsCSSProps.h#70-71 , because that assert did not consider eCSSPropertyExtra_variable a valid variant.

With that MOZ_ASSERT fixed (change is pushed to D191059), at-property-animation.html (and, after a delay, attachment animated-length-2.html) is crashing from a double free of AnimatedPropertyID.mCustomName https://phabricator.services.mozilla.com/D191059?vs=on&id=793060#C7129458NL22, which I have not tried to dig into yet. If you find the cause of that, it would certainly speed things up.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Some uses of Atom::from_addrefed in the patches are wrong, they don't steal the reference, so they need to addref:

diff --git a/servo/components/style/properties/properties.mako.rs b/servo/components/style/properties/properties.mako.rs
index e02bd31327436..3ead09d323868 100644
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -2057,7 +2057,7 @@ impl PropertyDeclarationId {
                 return Err(());
             }
             PropertyDeclarationId::Custom(unsafe {
-                Atom::from_addrefed(property.mCustomName.mRawPtr)
+                Atom::from_raw(property.mCustomName.mRawPtr)
             })
         } else {
             PropertyDeclarationId::Longhand(match LonghandId::from_nscsspropertyid(property.mID) {
@@ -2386,7 +2386,7 @@ impl PropertyId {
                 return Err(());
             }
             PropertyId::Custom(unsafe {
-                Atom::from_addrefed(property.mCustomName.mRawPtr)
+                Atom::from_raw(property.mCustomName.mRawPtr)
             })
         } else {
             NonCustomPropertyId::from_nscsspropertyid(property.mID)?.to_property_id()

Hope it helps. Thanks!

Comment 19

[Zach Hoffman [:zrhoffman]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

Some uses of Atom::from_addrefed in the patches are wrong, they don't steal the reference, so they need to addref:

That fixes the crash. Thanks Emilio!

The patchset no longer crashes on properties and values WPTs, let's mark them ready for review.

Comment 20

[Frédéric Wang]

(In reply to Zach Hoffman [:zrhoffman] from comment #17)

Sorry for not responding sooner. It was crashing in at-property-animation.html when trying to check if a custom property is a shorthand https://phabricator.services.mozilla.com/D191059?vs=on&id=793059#C7129453NL498 by passing the property's mID to nsCSSProps::IsShorthand (which fails on an assert in nsCSSProps::IsShorthand https://searchfox.org/mozilla-central/rev/78a2c17cc80680a5a82446e4ce7c45a73b935383/layout/style/nsCSSProps.h#70-71 , because that assert did not consider eCSSPropertyExtra_variable a valid variant.

Just to comment on this, instead of tweaking the assert to accept eCSSPropertyExtra_variable, my approach was to modify the callers of nsCSSProps::IsShorthand to handle eCSSPropertyExtra_variable separately (e.g. my change to the MOZ_ASSERT in nsTransitionManager::ConsiderInitiatingTransition). If you instead choose to just fix the assert so it can handle eCSSPropertyExtra_variable, some of the changes I made to avoid calling nsCSSProps::IsShorthand with eCSSPropertyExtra_variable can probably be removed.

Also, technically CSS variables are all longhand properties, so the method should probably return false. But eCSSPropertyExtra_variable is the last value at https://searchfox.org/mozilla-central/source/layout/style/nsCSSPropertyID.h.in#35 so I believe your current version returns true. So calling nsCSSProps::IsShorthand with eCSSPropertyExtra_variable could make things wrong (for example the assert in ConsiderInitiatingTransition I just mentioned would cause a crash when we make custom properties transitionable).

(In reply to Zach Hoffman [:zrhoffman] from comment #19)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

Some uses of Atom::from_addrefed in the patches are wrong, they don't steal the reference, so they need to addref:

That fixes the crash. Thanks Emilio!

The patchset no longer crashes on properties and values WPTs, let's mark them ready for review.

OK, that bad usage of Atom:: on Servo was what I suspected. Thanks Emilio and Zach for taking care of that and glad there are no more crashes!

Comment 21

[Zach Hoffman [:zrhoffman]]

Attachment: Bug 1846516 - Move PropertyDeclarationId to its own module r=fredw,zsun,#style

Because PropertyDeclarationId and its implementation do not make use of
templating, we might as well move it out of mako.

This will be useful later when creating OwnedPropertyDeclarationId,
which can be added to the same module.

Comment 22

[Zach Hoffman [:zrhoffman]]

File bug 1869185 for interpolation so bug 1846516 can focus on discrete animation.

Comment 23

[Frédéric Wang]

Hi Zach. Thanks again for taking over the work on this. Now that the crash is fixed and the patches essentially accepted, I'm curious how that improves the WPT tests and what's happening with the testcase of 1402219?

Comment 24

[Zach Hoffman [:zrhoffman]]

(In reply to Frédéric Wang from comment #20)

(In reply to Zach Hoffman [:zrhoffman] from comment #17)

Sorry for not responding sooner. It was crashing in at-property-animation.html when trying to check if a custom property is a shorthand https://phabricator.services.mozilla.com/D191059?vs=on&id=793059#C7129453NL498 by passing the property's mID to nsCSSProps::IsShorthand (which fails on an assert in nsCSSProps::IsShorthand https://searchfox.org/mozilla-central/rev/78a2c17cc80680a5a82446e4ce7c45a73b935383/layout/style/nsCSSProps.h#70-71 , because that assert did not consider eCSSPropertyExtra_variable a valid variant.

Just to comment on this, instead of tweaking the assert to accept eCSSPropertyExtra_variable, my approach was to modify the callers of nsCSSProps::IsShorthand to handle eCSSPropertyExtra_variable separately (e.g. my change to the MOZ_ASSERT in nsTransitionManager::ConsiderInitiatingTransition). If you instead choose to just fix the assert so it can handle eCSSPropertyExtra_variable, some of the changes I made to avoid calling nsCSSProps::IsShorthand with eCSSPropertyExtra_variable can probably be removed.

Also, technically CSS variables are all longhand properties, so the method should probably return false. But eCSSPropertyExtra_variable is the last value at https://searchfox.org/mozilla-central/source/layout/style/nsCSSPropertyID.h.in#35 so I believe your current version returns true. So calling nsCSSProps::IsShorthand with eCSSPropertyExtra_variable could make things wrong (for example the assert in ConsiderInitiatingTransition I just mentioned would cause a crash when we make custom properties transitionable).

Thanks, nsCSSProps::IsShorthand is fixed now WRT behavior for eCSSPropertyExtra_variable in D191059, though we have yet to remove unnecessary nsCSSProps::IsShorthand() calls and avoid eCSSPropertyExtra_variable being passed to it.

(In reply to Frédéric Wang from comment #23)

Hi Zach. Thanks again for taking over the work on this. Now that the crash is fixed and the patches essentially accepted, I'm curious how that improves the WPT tests and what's happening with the testcase of 1402219?

As seen in the WPT metadata changes in D190758, we get 4 tests passing with the changeset as-is (with a couple more passes not yet marked in css/css-animations/animation-base-response-001.html and css/css-animations/keyframes-unrelated-custom-property.html), though none in Interop 2023 yet. I think the main reason for not getting more passes so far is that most of the Properties and Values animation WPTs check assert that custom properties have a particular value, rather than using reftests, so probably bug 1811897 will need to be fixes in order to get most of them passing.

Unfortunately, the changeset also introduces a large number of regressions, as well as several crashes (see try run <https://treeherder.mozilla.org/push-health/push?repo=try&revision=3433c4cc3ba33031d0f32551185f67f1f64ca6bc&tab=tests&testGroup=pr&selectedTest=csscss-animationsanimation-base-response-001html&selectedTaskId=&selectedJobName=>), which will probably need to be resolved before the changeset can land.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Please ni? me if you need me to look at some of the crashes / regressions.

Comment 26

[Zach Hoffman [:zrhoffman]]

Some tests were crashing from a failed in AnimatedPropertiIDSet:

css/css-transforms/animation/transform-interpolation-inline-value.html
web-animations/interfaces/Animation/commitStyles-crash.html
web-animations/interfaces/Animation/commitStyles.html
web-animations/interfaces/Animation/style-change-events.html

which was fixed by changing an && to an || in <https://phabricator.services.mozilla.com/D191059?id=797970\>.

Comment 27

[Zach Hoffman [:zrhoffman]]

Hi Emilio,

• Non-custom-property interpolation seems to crash now at <https://searchfox.org/mozilla-central/source/servo/components/style/properties/helpers/animated_properties.mako.rs#461> (see css/css-logical/animation-004.html). If you are able to figure out why, that would help a lot.

• I cannot reproduce the crashes in

  • fenced-frame/embedder-require-corp.https.html
  • webvr/webvr-enabled-by-feature-policy-attribute-redirect-on-load.https.sub.html
    on a linux debug build. Maybe they are unrelated?

I think those are the only crashes, I will reach out about the failures once I have looked at them if they look tricky. Thanks!

Comment 28

[Zach Hoffman [:zrhoffman]]

(In reply to Frédéric Wang from comment #23)

Hi Zach. Thanks again for taking over the work on this. Now that the crash is fixed and the patches essentially accepted, I'm curious how that improves the WPT tests and what's happening with the testcase of 1402219?

The testcases for bug 1402219 (<https://bug1402219.bmoattachments.org/attachment.cgi?id=8911056> and <https://bug1402219.bmoattachments.org/attachment.cgi?id=8911616) fail with bug 1846516's changeset but seem mostly fine in Nightly). Any suggestions about how to get those testcases to work again?

Comment 29

[Frédéric Wang]

(In reply to Zach Hoffman [:zrhoffman] from comment #28)

The testcases for bug 1402219 (<https://bug1402219.bmoattachments.org/attachment.cgi?id=8911056> and <https://bug1402219.bmoattachments.org/attachment.cgi?id=8911616) fail with bug 1846516's changeset but seem mostly fine in Nightly). Any suggestions about how to get those testcases to work again?

I'm not quite sure to be honest. As I said in comment 12 (and before), I needed to remove the workaround of bug 1402219 (which IIRC is essentially having a servo declaration block to store some values of custom properties) in order to make custom properties animatable but I suspected it would break the use of a non-animated custom properties inside @keyframes (which you confirmed now). Maybe there is a way to distinguish between these two cases (animated custom properties Vs non-animated custom properties inside a @keyframe) and decide whether or not it's needed to put the custom property in the shared servo declaration block.

BTW in our previous meeting, Emilio was mentioning the "combined" case where we have an animated custom property which is itself used in the keyframe of another animation i.e. something like

  @property --endColor {
    syntax: "<color>";
    inherits: false;
    initialValue: "yellow";
  }
  @keyframes foo {
    from {
      color: blue;
       --endColor: cyan;
    }
    to {
       color: var(--endColor);
       --endColor: green;
    }
  }
  #target {
    --endColor: red;
    animation: foo 1s both;
  }

We needed to check what the spec says for this case and similar ones. But perhaps these are really edge cases we don't need to worry about, so only dealing with the two separate cases I mentioned above could be fine for a first implementation...

Emilio: do you have more thoughts on that?

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1846516 - Minor fix ups and logical animation clean-up. r=zrhoffman

Comment 31

[Emilio Cobos Álvarez (:emilio)]

(In reply to Zach Hoffman [:zrhoffman] from comment #27)

• Non-custom-property interpolation seems to crash now at <https://searchfox.org/mozilla-central/source/servo/components/style/properties/helpers/animated_properties.mako.rs#461> (see css/css-logical/animation-004.html). If you are able to figure out why, that would help a lot.

That didn't crash for me, maybe because you updated the patch stack. But logical transitions still didn't work, and comment 30 fixes it (and cleans up a little bit too).

I'll look tomorrow at bug 1402219 and co.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

The remaining regressions are due to IsAnimatable and co not returning true for shorthands anymore, fwiw.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29cbdd821419
Move PropertyDeclarationId to its own module r=firefox-style-system-reviewers,emilio

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1846516 - More fixes on top of bug 1870009. r=zrhoffman

Fixes some leaks, and fixes IsAnimatable/IsTransitionable to look at
PropertyId.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1846516 - More fixes on top of bug 1870009. r=zrhoffman

Well, this includes a preliminary version of that bug, need to rebase on
top properly, but the rest of the changes still stand.

This fixes some leaks, and fixes IsAnimatable/IsTransitionable to look
at PropertyId.

Comment 36

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/29cbdd821419

Comment 37

[Donal Meehan [:dmeehan]]

:zrhoffman anything you want to nominate here for a release note? (Process info)

Comment 38

[Daniel Holbert [:dholbert]]

We've also got this bug linkified from https://wpt.fyi/results/css/css-properties-values-api?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2023-property%20at-property-animation (via the bug icon shown next to our 1 of 18 subtests score); but it's not clear to me if that linkage is correct, since it doesn't look like we're removing test-failure annotations in the patch stack here. Do we need another bug for those test failures?

Comment 39

[Zach Hoffman [:zrhoffman]]

(In reply to Donal Meehan [:dmeehan] from comment #37)

:zrhoffman anything you want to nominate here for a release note? (Process info)

Probably not for 122, unless this patch stack lands before the freeze is over. As-is, none of the patches that have landed so far change behavior.

Comment 40

[Zach Hoffman [:zrhoffman]]

(In reply to Daniel Holbert [:dholbert] from comment #38)

We've also got this bug linkified from https://wpt.fyi/results/css/css-properties-values-api?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2023-property%20at-property-animation (via the bug icon shown next to our 1 of 18 subtests score); but it's not clear to me if that linkage is correct, since it doesn't look like we're removing test-failure annotations in the patch stack here. Do we need another bug for those test failures?

That linkage was made when bug 1846516 was still named [css-properties-values-api] Support interpolating registered custom properties.. But because this patch stack covers only discrete animation, I renamed bug 1846516 accordingly in comment 22. So the linkage should be updated to bug 1869185, which should cover interpolation (sorry for the confusion).

That said even once interpolation is updated, the assertions in at-property-animation.html will not pass until bug 1811897 is resolved, since it depends on checking the interpolated custom property values, rather than anything visual.

Comment 41

[Daniel Holbert [:dholbert]]

Thanks. Given the multiple dependencies there, I just filed a dedicated bug (bug 1870370) to track that test failure, with its various dependencies linked up.

Comment 42

[Zach Hoffman [:zrhoffman]]

Thanks Daniel, and thanks for updating the css-properties-values-api WPT linkage!

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/753bc39f59ff
[css-properties-values-api] Use PropertyDeclarationId/PropertyDeclarationIdSet for animated properties. r=firefox-style-system-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/63603267402e
[css-properties-values-api] Introduce AnimatedPropertyID/AnimatedPropertIDSet. r=firefox-animation-reviewers,firefox-style-system-reviewers,layout-reviewers,emilio,hiro
https://hg.mozilla.org/integration/autoland/rev/729abedf9bda
[css-properties-values-api] Use AnimatedPropertyID to communicate between Gecko and Servo. r=emilio,layout-reviewers,firefox-style-system-reviewers
https://hg.mozilla.org/integration/autoland/rev/6f62586a139b
[css-properties-values-api] Animate custom properties in a discrete way. r=firefox-style-system-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/e26c8d5274eb
Minor fix ups and logical animation clean-up. r=zrhoffman
https://hg.mozilla.org/integration/autoland/rev/1c44d9efbe3a
More fixes on top of bug 1870009. r=zrhoffman

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfe8430bb23c
Fix some atom leaks caught by ASAN.

Comment 45

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/753bc39f59ff
https://hg.mozilla.org/mozilla-central/rev/63603267402e
https://hg.mozilla.org/mozilla-central/rev/729abedf9bda
https://hg.mozilla.org/mozilla-central/rev/6f62586a139b
https://hg.mozilla.org/mozilla-central/rev/e26c8d5274eb
https://hg.mozilla.org/mozilla-central/rev/1c44d9efbe3a
https://hg.mozilla.org/mozilla-central/rev/cfe8430bb23c