1385139 - stylo: CSS variables not resolved in values returned by getKeyframes()

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

Running dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html on a Stylo build from yesterday's m-c, I get the following failures:

  #1: KeyframeEffectReadOnly.getKeyframes() returns expected values for animations with CSS variables as keyframe values
     assert_equals: value for 'transform' on ComputedKeyframe #1 expected "translate(100px, 0px)" but got " translate(var(--var-100px), 0) "

  #2: KeyframeEffectReadOnly.getKeyframes() returns expected values for animations with CSS variables as keyframe values in a shorthand property
     assert_equals: value for 'marginTop' on ComputedKeyframe #1 expected "100px" but got ""

Comment 1

[Brian Birtles (:birtles)]

I'm not sure if this just effects getKeyframes() or if it actually affects the visual result. Setting P2 for now but we might downgrade it later if it only affects getKeyframes().

Comment 2

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #1)
> I'm not sure if this just effects getKeyframes() or if it actually affects
> the visual result. Setting P2 for now but we might downgrade it later if it
> only affects getKeyframes().

I just checked this. Looks like the visual result is correct: we create a css animation with the correct property value, so I think this only affects getKeyframes().

Comment 3

[Brian Birtles (:birtles)]

Marking this as P3 since the visual result is correct. It would be really good to fix this since although we don't ship getKeyframes() it affects the tooltips reported by DevTools. It would be odd for those to report `translate(var(--var-100px), 0)` but that shouldn't block shipping Stylo.

Comment 4

[Brian Birtles (:birtles)]

I spent a while trying to work out where we compute values before making up CSS keyframes but then I realized we don't. Apparently in bug 1358353 we decided just to keep specified values.

However, I'm concerned that without doing this, there may be bugs when variables are used in shorthands and we fail to overlap them correctly.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

So what we should do for CSS animations is that when we get a DeclarationBlock in Servo_StyleSet_GetKeyframesForName (which is used only for CSS animations), if the DeclarationBlock is WithVariables we need to resolve the variables with substitute_variables (if I recall its purpose correctly).

Comment 6

[Brian Birtles (:birtles)]

Yes, that sounds right. Thanks!

Comment 7

[Brian Birtles (:birtles)]

Attachment: How not to fix this bug

Comment 8

[Brian Birtles (:birtles)]

Attachment: Test case - changing custom properties

Comment 9

[Brian Birtles (:birtles)]

I went ahead and expanded the custom properties in Servo_StyleSet_GetKeyframesForName (attachment 8898604 [details] [diff] [review]) and the tests here pass.

However, it introduces another bug. In attachment 8898605 [details] we change custom properties mid-way through an animation. With this patch applied nothing causes us to call Servo_StyleSet_GetKeyframesForName when the variable changes and hence we keep animating with the old values. Without this patch, however, it works.

Some background to this bug:

* Originally we decided to expand shorthands to longhands because of the complications involved in mapping
  a declaration block to Web Animations' format seemed too complicated.[1]

* Subsequently when we encountered variables we decided in order to map them to longhands we should just
  convert them to computed values (and for consistency we do the same for property values that don't use
  variables). (See bug 1268858 comment 19.)

* In Servo we map to longhands but for custom properties we store their values as unparsed values. So, if you
  have `margin: var(--yer)` then internally we store:

     margin-left: (unparsed value)
     margin-top: (unparsed value)
     margin-right: (unparsed value)
     margin-bottom: (unparsed value)

  which we'll serialize as:

     marginLeft: '',
     marginTop: '',
     marginRight: '',
     marginBottom: '',

  So, the visual result is correct, and since we internally store the variable reference, when variables
  change the animation updates (presumably we *do* regenerate the computed property values even though we
  don't regenerate the specified keyframes).

Now a few observations:

* Expanding CSS declaration blocks to longhands with computed values seems sub-optimal from an authoring point
  of view. In future I'd like to reconsider if we can keep shorthands in @keyframes when we set up Keyframe
  objects.

* (Related, but somewhat separate to the above point, I wonder if, once we've dropped the Gecko style system
  if we could just store all the property values for a Keyframe in a single declaration block and then just
  convert to-and-from the declaration block at the point where we get/set keyframes from the API.)

* Similarly, it would be preferable if we don't need to convert specified values to computed values.

* The values returned by getKeyframes() for CSS animations are not really exposed anywhere.
  - We don't ship `getKeyframes()` to release channel
  - When we do ship `getKeyframes()` we still won't ship CSSAnimation objects for quite some time
  - DevTools uses `getProperties()` when available NOT `getKeyframes()`[2]

Some options for fixing this:

a. Make us regenerate keyframes for CSS when variables change (presumably Gecko does this)
b. Skip those tests for Stylo
c. Make the tests check for the Stylo values
d. Resolve the variables in GetKeyframes() only

I don't want to do (a). It sounds very wasteful.

(b) and (c) are based on the assumption that we will eventually change this behavior anyway so we shouldn't waste time on it. I'm not totally sure about them, however, since this seems oddly inconsistent.

I'll try (d) and if it proves difficult I might just give up and do (c).

[1] http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/layout/style/nsAnimationManager.cpp#635-677
[2] http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/devtools/client/animationinspector/components/animation-timeline.js#683

Comment 10

[Boris Chiou [:boris]]

Thanks for taking this, Brian. :)

Comment 11

[Brian Birtles (:birtles)]

WIP patch for serializing on output:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8707eb75fb9c8230b4164f3c5daba8669a4a60cb

Try run with unsuccessful approach to see if we have any tests covering this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa4a442538e096ef91ba9af0fb12fa19256643f4

Comment 12

[Brian Birtles (:birtles)]

Attachment: Bug 1385139 - Add test for changes to CSS variables;

This test passes both before and after the code changes in the following patch
in this series. However, both tests fail if we try to expand variables in
Servo_StyleSet_GetKeyframesForName.

Review commit: https://reviewboard.mozilla.org/r/170524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170524/

Comment 13

[Brian Birtles (:birtles)]

Attachment: Bug 1385139 - Expand var() references in keyframes from CSS animations when serializing;

This is a temporary step until we implement bug 1391537. It is needed to
maintain consistent behavior with Gecko and provide sensible output,
particularly in the case where we have longhands whose values are drawn from
unparsed shorthands.

Review commit: https://reviewboard.mozilla.org/r/170526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170526/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8899303 [details]
Bug 1385139 - Add test for changes to CSS variables;

https://reviewboard.mozilla.org/r/170524/#review175682

::: layout/style/test/test_animations_variable_changes.html:23
(Diff revision 1)
> +  const div = document.createElement('div');
> +  document.body.append(div);

Nit: We can use addDiv() here and drop div.remove() at the end of this test.

::: layout/style/test/test_animations_variable_changes.html:39
(Diff revision 1)
> +
> +  div.remove();
> +}, 'Animation reflects changes to custom properties');
> +
> +test(() => {
> +  const parent = document.createElement('div');

Nit: We can use addDivi() for 'parent' too.

Comment 15

[Brian Birtles (:birtles)]

We don't have addDiv in layout/style/test

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Oh nvm, I thought the test in dom/animation/test.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8899304 [details]
Bug 1385139 - Expand var() references in keyframes from CSS animations when serializing;

https://reviewboard.mozilla.org/r/170526/#review175690

::: dom/animation/KeyframeEffectReadOnly.cpp:1225
(Diff revision 1)
> +    // The following will flush style but that's ok since if you update
> +    // a variable's computed value, you expect to see that updated value in the
> +    // result of getKeyframes().
> +    //

Just as a side note that we should keep in mind about flushing style.  Flushing style might destroy pres shell, so if it happens, rest of codes in this function will be run without pres shell.  nsComputedDOMStyle::GetStyleContext() (which is called from GetTargetStyleContext) holds a reference of nsIPresShell so it's safe in the function but after GetTargetStyleContext in this function, there is no guarantee that we have valid pres shell.  For now there is no such risk in this function, just a thing I am concerned.

::: servo/components/style/properties/declaration_block.rs:521
(Diff revision 1)
>          }
>          removed_at_least_one
>      }
>  
>      /// Take a declaration block known to contain a single property and serialize it.
> -    pub fn single_value_to_css<W>(&self, property: &PropertyId, dest: &mut W) -> fmt::Result
> +    pub fn single_value_to_css<W>(

Can we mark this function as gecko only?

::: servo/components/style/properties/declaration_block.rs:545
(Diff revision 1)
> +                        (&PropertyDeclaration::WithVariables(id, ref unparsed),
> +                         Some(ref computed_values)) => unparsed
> +                            .substitute_variables(
> +                                id,
> +                                &computed_values.custom_properties(),
> +                                QuirksMode::NoQuirks,

Actually it may not be a problem but I guess we should pass mElement->OwnerDoc()->GetCompatibilityMode() to this quirk mode?

Comment 18

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> ::: servo/components/style/properties/declaration_block.rs:521
> (Diff revision 1)
> >          }
> >          removed_at_least_one
> >      }
> >  
> >      /// Take a declaration block known to contain a single property and serialize it.
> > -    pub fn single_value_to_css<W>(&self, property: &PropertyId, dest: &mut W) -> fmt::Result
> > +    pub fn single_value_to_css<W>(
> 
> Can we mark this function as gecko only?

I think it's ok as-is? The added code only takes effect if a ComputedValues is passed-in and when it's not passed-in, I think the behavior is sensible for the Servo case too.

> ::: servo/components/style/properties/declaration_block.rs:545
> (Diff revision 1)
> > +                        (&PropertyDeclaration::WithVariables(id, ref unparsed),
> > +                         Some(ref computed_values)) => unparsed
> > +                            .substitute_variables(
> > +                                id,
> > +                                &computed_values.custom_properties(),
> > +                                QuirksMode::NoQuirks,
> 
> Actually it may not be a problem but I guess we should pass
> mElement->OwnerDoc()->GetCompatibilityMode() to this quirk mode?

Yeah, originally I did that but then I found that in a lot of similar call sites we were hard-coding eCompatibility_FullStandards further up the stack anyway. In the end I decided it wasn't worth the complexity to pass it down, especially since this is only used in code that is disabled on release builds.

Comment 19

[Brian Birtles (:birtles)]

Attachment: Servo PR #18165

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8899303 [details]
Bug 1385139 - Add test for changes to CSS variables;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170524/diff/1-2/

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8899304 [details]
Bug 1385139 - Expand var() references in keyframes from CSS animations when serializing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170526/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36e73b132139
Add test for changes to CSS variables; r=hiro
https://hg.mozilla.org/integration/autoland/rev/575d130ab5f6
Expand var() references in keyframes from CSS animations when serializing; r=hiro

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/36e73b132139
https://hg.mozilla.org/mozilla-central/rev/575d130ab5f6