1295401 - Use computed value for discretely animatable property value of KeyframeEffect::getKeyframes() if the value is inherit, initial and unset

Status: RESOLVED WONTFIX

(Core :: DOM: Animation, defect, P3)

Description

[Daisuke Akatsuka (:daisuke)]

Attachment: should-be-underlying-value.html

I attached the test case.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

This will be fixed once we use underlying values for additive/accumulate composition.  Or is this really specific for discrete animations?

Comment 2

[Brian Birtles (:birtles)]

Yes, it's due to the way StyleAnimationValue::ExtractComputedValue is implemented for the discrete animation type. For CSS Animations hopefully we'll be able to just drop this underlying value from the keyframes and rely on the additive animation behavior. However, for transitions I suspect we might need to fix this.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #2)
> But, the GetPropertyValue updates the animation[1].
> So added a method GetPropertyValueWithoutAnimation to avoid the update.
> # Also Element::GetAnimations updates too. (in case of if flush of style is
> needed?)
> 
> I'll check GetComputedStyleMap and SetResolvedStyleContext.

After I commented, I found SetFrameStyleContext, you can re-use it for the purpose.

> But, the GetPropertyValue updates the animation[1].
> So added a method GetPropertyValueWithoutAnimation to avoid the update.

Also you don't need to *WithoutAnimation* name there because you should pass an nsStyleContext (which does not included animation rules at all) to the SetFrameStyleContext and just create another variant of nsComputedDOMStyle::GetPropertyCSSValue() that just does get a function pointer by GetComputedStyleMap and call the function pointer.

Comment 4

[Daisuke Akatsuka (:daisuke)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #2)
> > But, the GetPropertyValue updates the animation[1].
> > So added a method GetPropertyValueWithoutAnimation to avoid the update.
> > # Also Element::GetAnimations updates too. (in case of if flush of style is
> > needed?)
> > 
> > I'll check GetComputedStyleMap and SetResolvedStyleContext.
> 
> After I commented, I found SetFrameStyleContext, you can re-use it for the
> purpose.
> 
> > But, the GetPropertyValue updates the animation[1].
> > So added a method GetPropertyValueWithoutAnimation to avoid the update.
> 
> Also you don't need to *WithoutAnimation* name there because you should pass
> an nsStyleContext (which does not included animation rules at all) to the
> SetFrameStyleContext and just create another variant of
> nsComputedDOMStyle::GetPropertyCSSValue() that just does get a function
> pointer by GetComputedStyleMap and call the function pointer.

Oh, I see.
Will try that!

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review107386

I think this patch should be also reviewed by dholbert or other person who are familiar with CSS stuff to clarigy this approach is a good way or not.

Couple of comments:

1. As per spec[1] the initial value of align-content is 'stretch', but our implementation seems to be wrong. Let's use other property whose initial value is correct in test cases.  I'd prefer to do in other patch though.
2. Transition part should be done in another bug.
3. I don't think we need to call ResolveStyleByRemovingAnimation() in either case of CSS animation or transition. (the |aStyleContext| we are passing to ExtractComputedValue() is the style got by ResolveStyleByRemovingAnimation)
4. I don't think the nsComputedDOMStyle needs Element* too, is it really necessary? If not we can drop Element* argument

[1] https://drafts.csswg.org/css-flexbox-1/#propdef-align-content

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review107386

Thank you Hiro!

Okay, I'll request the review to Daniel as well.

1. Ok, will change the test property in as patch part 2.
2. Will fix in bug 1320854
3. Ok. ( Should we add MOZ_ASSERAT here to check the StyleContext has any animation rules? )
4. Ok, please let me check this.

Comment 8

[Daisuke Akatsuka (:daisuke)]

As discussed with Hiro,
we'll modify the following points.

1. Changes name to GetDiscretelyAnimatablePropertyValue etc.
2. Change the method to static function.
3. The method set the computed value to given nsCSSValue.
4. Do not use NS_IMETHODIMP
5. Avoid to create new instance of nsComputedDOMStyle.
6. Leave the test code as is since the draft of css spec indicates 'normal' as initial value.[1]

[1] https://drafts.csswg.org/css-align/#propdef-align-content

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

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

Comment 10

[Daisuke Akatsuka (:daisuke)]

Hi Daniel,
Could you review this bug, especially the following files?

* layout/style/StyleAnimationValue.cpp
  https://reviewboard.mozilla.org/r/106428/diff/2/#1
* layout/style/nsComputedDOMStyle.h
  https://reviewboard.mozilla.org/r/106428/diff/2/#2
* layout/style/nsComputedDOMStyle.cpp
  https://reviewboard.mozilla.org/r/106428/diff/2/#3

Thanks.

Comment 11

[Daisuke Akatsuka (:daisuke)]

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106426/#review107756

I think this is basically fine, but as I mentioned in comment 6, I want to hear opinion from Daniel.

::: layout/style/nsComputedDOMStyle.h:135
(Diff revision 2)
>  
>    // nsIMutationObserver
>    NS_DECL_NSIMUTATIONOBSERVER_PARENTCHAINCHANGED
>  
> +  static void
> +  GetDiscretelyAnimatedCSSValue(nsStyleContext* aContext,

I don't think we want an animated value here, I think we just want *not animating* value. Right?

Also, we should add description about this static function, for what purpose and restrictions (i.e. |aContext| must not have any animation rules).

::: layout/style/nsComputedDOMStyle.h:137
(Diff revision 2)
>    NS_DECL_NSIMUTATIONOBSERVER_PARENTCHAINCHANGED
>  
> +  static void
> +  GetDiscretelyAnimatedCSSValue(nsStyleContext* aContext,
> +                                const nsCSSPropertyID aPropID,
> +                                nsCSSValue* aValue);

This should be nsCSSValue& aValue.

::: layout/style/nsComputedDOMStyle.h:173
(Diff revision 2)
>                                       StyleType aStyleType,
>                                       AnimationFlag aAnimationFlag);
>  
> +
> +  void GetDiscretelyAnimatedCSSValue(const nsCSSPropertyID aPropID,
> +                                     nsCSSValue* aValue);

Likewise.

::: layout/style/nsComputedDOMStyle.cpp:246
(Diff revision 2)
>    mExposedPropertyCount = index;
>  }
>  
> +nsComputedDOMStyle::nsComputedDOMStyle(nsStyleContext* aContext)
> +{
> +  SetFrameStyleContext(aContext);

I think it's better to set mStyleContext directly here.

::: layout/style/nsComputedDOMStyle.cpp:355
(Diff revision 2)
> +/* static */ void
> +nsComputedDOMStyle::GetDiscretelyAnimatedCSSValue(nsStyleContext* aContext,
> +                                                  const nsCSSPropertyID aPropID,
> +                                                  nsCSSValue* aReturn)
> +{
> +  nsComputedDOMStyle computedStyle(aContext);

Is there no way to check the nsStyleContext has no animation rules at all?  We should do check in MOZ_ASSERT() or ifdef DEBUG block.

::: layout/style/nsComputedDOMStyle.cpp:375
(Diff revision 2)
> +  nsString cssText;
> +  ErrorResult error;
> +  cssValue->GetCssText(cssText, error);
> +  MOZ_ASSERT(error.StealNSResult() == NS_OK, "Error in CSSValue::GetCssText");
> +
> +  nsIDocument* doc = mStyleContext->Arena()->GetDocument();
> +  nsCSSParser parser;
> +  parser.ParseLonghandProperty(aPropID,
> +                               cssText,
> +                               doc->GetDocumentURI(),
> +                               doc->GetDocumentURI(),
> +                               doc->NodePrincipal(),
> +                               *aReturn);

Can we somehow add a helper function which converts CSSValue to nsCSSValue directly?

Comment 13

[Daisuke Akatsuka (:daisuke)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Comment on attachment 8829329 [details]
> Bug 1295401: Support underlying value for discretely animation type.
> 
> https://reviewboard.mozilla.org/r/106426/#review107756
> 

( I replied from the reviewboard, but it did not reflect to bugzilla. I re-write to bugzilla directory.  )

> ::: layout/style/nsComputedDOMStyle.cpp:355
> (Diff revision 2)
> > +/* static */ void
> > +nsComputedDOMStyle::GetDiscretelyAnimatedCSSValue(nsStyleContext* aContext,
> > +                                                  const nsCSSPropertyID aPropID,
> > +                                                  nsCSSValue* aReturn)
> > +{
> > +  nsComputedDOMStyle computedStyle(aContext);
> 
> Is there no way to check the nsStyleContext has no animation rules at all? 
> We should do check in MOZ_ASSERT() or ifdef DEBUG block.

I added MOZ_ASSERT here, however got assertion failure.
In that case, ExtractComputedStyle is called from CSSAnimationBuilder::GetKeyframePropertyValues.
Will investigate the reason.

> ::: layout/style/nsComputedDOMStyle.cpp:375
> (Diff revision 2)
> > +  nsString cssText;
> > +  ErrorResult error;
> > +  cssValue->GetCssText(cssText, error);
> > +  MOZ_ASSERT(error.StealNSResult() == NS_OK, "Error in CSSValue::GetCssText");
> > +
> > +  nsIDocument* doc = mStyleContext->Arena()->GetDocument();
> > +  nsCSSParser parser;
> > +  parser.ParseLonghandProperty(aPropID,
> > +                               cssText,
> > +                               doc->GetDocumentURI(),
> > +                               doc->GetDocumentURI(),
> > +                               doc->NodePrincipal(),
> > +                               *aReturn);
> 
> Can we somehow add a helper function which converts CSSValue to nsCSSValue
> directly?

As discussed with hiro on irc, we wait for Daniel's opinion.

Comment 14

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

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

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review109674

Sorry for the delay here. Some preliminary review thoughts:

This seems a bit convoluted, though maybe it's necessary.  (I'm wishing/hoping we could do something more direct, though I don't know if we can.)

If I understand correctly: the idea here is to fill in the cases that Bug 1277433 didn't cover for its GetDiscretelyAnimatedCSSValue() impl -- particularly, cases where the property doesn't explicitly have a value set on a rule for this element (or when the proximal rule sets a value like "unset"/"initial"/"inherit").

It looks like this patch handles these cases by calling GetComputedStyle on the element's style context, to generate a string representation of the computed-style value of the property, so that it can feed that result back into a CSS parser.  This produces a nsCSSValue that we can then use in a StyleAnimationValue instance (via SetAndAdoptCSSValueValue).  And we need that nsCSSValue so that we can eventually satisfy calls to StyleAnimationValue::UncomputeValue().

Is this roughly correct?

::: layout/style/StyleAnimationValue.cpp:4802
(Diff revision 3)
> -      auto cssValue = MakeUnique<nsCSSValue>(eCSSUnit_Unset);
> +      auto cssValue = MakeUnique<nsCSSValue>(eCSSUnit_Dummy);
>        aStyleContext->RuleNode()->GetDiscretelyAnimatedCSSValue(aProperty,
>                                                                 cssValue.get());
> +      if (cssValue->GetUnit() == eCSSUnit_Dummy) {

Two questions:

    (1) It looks like you're testing for eCSSUnit_Dummy to see whether GetDiscretelyAnimatedCSSValue succeeded or failed.  But that function explicitly returns a bool to indicate whether it succeeded. Shouldn't we just test its boolean return value to check for failure, instead of inspecting the outparam's unit?

    (2) Could we initialize |cssValue| with its default constructor (no explicit Dummy unit, and just have it take the default default "eCSSUnit_Null" instead)? e.g.:
      auto cssValue = MakeUnique<nsCSSValue>();
This would make its initialization a bit shorter, and it has the benefit of making sure we actually do initialize its unit to something in the code below this. (because we have assertions about eCSSUnit_Null-valued nsCSSValues)

::: layout/style/nsComputedDOMStyle.cpp:244
(Diff revision 3)
> +nsComputedDOMStyle::nsComputedDOMStyle(nsStyleContext* aContext)
> +{
> +  mStyleContext = aContext;

I'm not yet sure if/why we need this new constructor -- in particular, comment 8 said "Avoid to create new instance of nsComputedDOMStyle" which sounds like maybe this is going away..?

But if we do need this, then:
 (1) it should do this assignment in a constructor initialization list.

 (2) It should initialize all the other member variables that we initialize in the other constructor, for consistency & safety. (Particularly the members that don't have their own constructor, like integer/pointer-valued variables)  We don't want to have uninitialized raw-pointers floating around on this object.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Daisuke Akatsuka (:daisuke) from comment #13)
> > Can we somehow add a helper function which converts CSSValue to nsCSSValue
> > directly?
> 
> As discussed with hiro on irc, we wait for Daniel's opinion.

I agree that this seems useful to split out, from a separation-of-concerns perspective. But I think it should just be a static helper function, defined just above the one caller that needs it -- it shouldn't be (for example) a public method on CSSValue or anything like that.

(The CSSValue->nsCSSValue conversion process is a bit clumsy and expensive, since it involves feeding text into a CSS parser -- so unless we already forsee other needs for this functionality, it's probably better that other hypothetical-future-callers look into other alternatives before sharing this codepath).

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review109700

::: layout/style/nsComputedDOMStyle.cpp:354
(Diff revision 3)
>  
> +/* static */ void
> +nsComputedDOMStyle::GetDiscretelyAnimatablePropertyCSSValue(
> +                      nsStyleContext* aContext,
> +                      const nsCSSPropertyID aPropID,
> +                      nsCSSValue& aReturn)

Let's call this "aResult" instead of "aReturn", for consistency with ParseLonghandProperty (which is what we're ultimately using to fill in this value).

(It's also a slightly-more-direct name for this thing -- it *is* the result that we produce.)

::: layout/style/nsComputedDOMStyle.cpp:371
(Diff revision 3)
> +  const nsComputedStyleMap::Entry* propEntry =
> +    GetComputedStyleMap()->FindEntryForProperty(aPropID);
> +  RefPtr<CSSValue> cssValue = (this->*propEntry->mGetter)();
> +  MOZ_ASSERT(cssValue, "CSSValue is not found");

The FindEntryForProperty documentation says it can return null (for properties that aren't exposed / are disabled).

So, you should null-check |propEntry| before using it, either in an assertion (if you're really sure it can never fail here -- which I'm not sure about), OR in an early-return.

::: layout/style/nsComputedDOMStyle.cpp:376
(Diff revision 3)
> +  const nsComputedStyleMap::Entry* propEntry =
> +    GetComputedStyleMap()->FindEntryForProperty(aPropID);
> +  RefPtr<CSSValue> cssValue = (this->*propEntry->mGetter)();
> +  MOZ_ASSERT(cssValue, "CSSValue is not found");
> +
> +  nsString cssText;

Use nsAutoString instead of nsString, for local variables. (nsAutoString brings along a stack-allocated buffer and lets us avoid heap allocation.)

::: layout/style/nsComputedDOMStyle.cpp:379
(Diff revision 3)
> +  MOZ_ASSERT(cssValue, "CSSValue is not found");
> +
> +  nsString cssText;
> +  ErrorResult error;
> +  cssValue->GetCssText(cssText, error);
> +  MOZ_ASSERT(error.StealNSResult() == NS_OK, "Error in CSSValue::GetCssText");

Without more explanation here, I don't have a high level of confidence that this (fatal!) assertion is warranted.  GetCssText is allowed to fail. If it fails, we should handle that gracefully. (again, probably with an early return)

::: layout/style/nsComputedDOMStyle.cpp:381
(Diff revision 3)
> +  nsIDocument* doc = mStyleContext->Arena()->GetDocument();
> +  nsCSSParser parser;
> +  parser.ParseLonghandProperty(aPropID,
> +                               cssText,
> +                               doc->GetDocumentURI(),

GetDocument() is allowed to return null (by convention, pointer-valued getters with "Get" in the name are allowed to return null).  So you need to null-check |doc| before dereferencing it. Again, probably with an early-return.

::: layout/style/nsComputedDOMStyle.cpp:821
(Diff revision 3)
> +    if (mContent) {
> -    mContent->RemoveMutationObserver(this);
> +      mContent->RemoveMutationObserver(this);
> -  }
> +    }
> +  }

It looks like you're making this change in order to allow for |mContent| to be null (because this code assumes mContent is non-null).

But, it looks like there are a number of other functions in this file that simply assume that mContent is non-null.   You need to audit those usages and add some reasonable fallback behavior for the ones that could be triggered as part of this new codepath-with-null-mContent.  In particular, I think there are 2 DoGet*** methods that dereference mContent and that could be invoked via your new code here: DoGetGridTemplateColumns() and DoGetGridTemplateRows() [1].

Those properties (grid-template-columns/rows) use eStyleAnimType_Discrete[2], so it seems that they can trigger this animation codepath. So I expect they could be made to trigger a crash (from null mContent deref in their DoGet methods) with the current patch-stack.

So, we need to either figure out how to get the right content node for your dummy nsComputedDOMStyle node here [not sure that's possible], OR just make sure we can have reasonable fallback behavior when mContent is null.  And we probably need to add some documentation alongside the mContent decl in nsComputedDOMStyle.h, to explain that it might be null.

This also goes for any other pointers which are normally guaranteed non-null (and which DoGet*** methods assume are non-null). For example, mPresShell -- it looks like you're not initializing that, but the DoGetOsxFontSmoothing() function dereferences it. [3] We need to be sure that won't crash. (Perhaps just by grabbing the presshell off of the style context when we set up our nsComputedDOMStyle instance?)

[1] 
https://dxr.mozilla.org/mozilla-central/rev/9c06e744b1befb3a2e2fdac7414ce18220774a1d/layout/style/nsComputedDOMStyle.cpp#3031-3037,3047-3053
[2] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h#2248-2260
[3] https://dxr.mozilla.org/mozilla-central/rev/9c06e744b1befb3a2e2fdac7414ce18220774a1d/layout/style/nsComputedDOMStyle.cpp#1703-1706

Comment 18

[Daniel Holbert [:dholbert]]

One other thought here:

From a "does stuff animate" perspective, we don't technically need to do any of this. In StyleAnimationValue, if our aStyleContext->RuleNode()->GetDiscretelyAnimatedCSSValue() query produces "unset" or "inherit", then we *can* just use that value directly as our uncomputed value, and everything will just work.

We *do* need this change in order to satisfy the JS API (as shown in the attached testcase) -- but I think that's it, right?

So, would it be possible/sane to scope this new code such that it's only invoked when we're answering queries from the JS api?  (so we don't have to bother with it during actual animation when we're not being inspected via JS)

(I might be showing my ignorance of how this all fits together here -- maybe the JS api plugs in too late in the pipeline, for what I'm suggesting to be possible.)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review110844

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Thank you Daniel.

(In reply to Daniel Holbert [:dholbert] from comment #18)
> One other thought here:
> 
> From a "does stuff animate" perspective, we don't technically need to do any
> of this. In StyleAnimationValue, if our
> aStyleContext->RuleNode()->GetDiscretelyAnimatedCSSValue() query produces
> "unset" or "inherit", then we *can* just use that value directly as our
> uncomputed value, and everything will just work.
> 
> We *do* need this change in order to satisfy the JS API (as shown in the
> attached testcase) -- but I think that's it, right?

Right, at this moment. But this mechanism will be used for CSS transition (bug 1320854) and script aniamtions (bug 1323119).

That being said, honestly, I'd like to fix these issues on stylo since IIUC this will be fixed more easier (On stylo we don't basically use StyleAnimationValue at all).

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review110996

Clearning review flag for now.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Just a quick note: 

I did confirm that on style this problem will not happen.  (I made align-items animatable and implemented interpolate() for align-items there.) Actually not checked inherit type properties though.

Comment 23

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review109674

Thank you very much for kindly review, Daniel!
And I'm sorry for my delay.

As hiro adviced, I wanted to convert CSSValue to nsCSSValue directory, but CSSValue has no information to determine nsCSSUnit in detail. So I wrote by such the procedure.

> Two questions:
> 
>     (1) It looks like you're testing for eCSSUnit_Dummy to see whether GetDiscretelyAnimatedCSSValue succeeded or failed.  But that function explicitly returns a bool to indicate whether it succeeded. Shouldn't we just test its boolean return value to check for failure, instead of inspecting the outparam's unit?
> 
>     (2) Could we initialize |cssValue| with its default constructor (no explicit Dummy unit, and just have it take the default default "eCSSUnit_Null" instead)? e.g.:
>       auto cssValue = MakeUnique<nsCSSValue>();
> This would make its initialization a bit shorter, and it has the benefit of making sure we actually do initialize its unit to something in the code below this. (because we have assertions about eCSSUnit_Null-valued nsCSSValues)

1) nsRuleNode.GetDiscretelyAnimatedCSSValue is "void" now. But I will change to bool to determine the suceeded/failed.
2) Okay!

> I'm not yet sure if/why we need this new constructor -- in particular, comment 8 said "Avoid to create new instance of nsComputedDOMStyle" which sounds like maybe this is going away..?
> 
> But if we do need this, then:
>  (1) it should do this assignment in a constructor initialization list.
> 
>  (2) It should initialize all the other member variables that we initialize in the other constructor, for consistency & safety. (Particularly the members that don't have their own constructor, like integer/pointer-valued variables)  We don't want to have uninitialized raw-pointers floating around on this object.

Oh, I'm sorry.
I meant avoid "new" operator to make this instance.
But will check above points.

Also, will fix that you pointed.

Comment 24

[Daisuke Akatsuka (:daisuke)]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> One other thought here:
> 
> From a "does stuff animate" perspective, we don't technically need to do any
> of this. In StyleAnimationValue, if our
> aStyleContext->RuleNode()->GetDiscretelyAnimatedCSSValue() query produces
> "unset" or "inherit", then we *can* just use that value directly as our
> uncomputed value, and everything will just work.
> 
> We *do* need this change in order to satisfy the JS API (as shown in the
> attached testcase) -- but I think that's it, right?
> 
> So, would it be possible/sane to scope this new code such that it's only
> invoked when we're answering queries from the JS api?  (so we don't have to
> bother with it during actual animation when we're not being inspected via JS)
> 
> (I might be showing my ignorance of how this all fits together here -- maybe
> the JS api plugs in too late in the pipeline, for what I'm suggesting to be
> possible.)

Yes, the animation was working well.
It looks just JS api problem of KeyframeEffect.getKeyframes().
So I'll move the logic into KeyframeEffect.

Also, may I change the title of this bug to something like 'Use computed value for the property value of KeyframeEffect.getKeyframes() if the value is inherit, initial and unset'?

For CSS transition, will fix in bug 1320854.

Thanks.

Comment 25

[Daniel Holbert [:dholbert]]

Sounds good to me. Thanks!

Comment 26

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106428/diff/3-4/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review112242

I will look into this after the big 'if' block becomes into smaller chuncks.

Thanks!

::: dom/animation/KeyframeEffectReadOnly.cpp:1224
(Diff revision 4)
> +        nsCSSValue value = propertyValue.mValue;
> +        if (nsCSSProps::kAnimTypeTable[propertyValue.mProperty] ==
> +            eStyleAnimType_Discrete && GetPresShell() &&
> +            (value.GetUnit() == eCSSUnit_Inherit ||
> +             value.GetUnit() == eCSSUnit_Initial ||
> +             value.GetUnit() == eCSSUnit_Unset)) {

Could you please split this block into (a) smaller function(s)?

::: dom/animation/KeyframeEffectReadOnly.cpp:1235
(Diff revision 4)
> +          // GetPresShell() is NULL. In this case, does not come in this block
> +          // to use uncomputed value as is.
> +          // const effect = new KeyframeEffect(null,
> +          //                                   [{ alignContent: "inherit" }],
> +          //                                   1000);
> +          // effect.getKeyframes()[0].alignContent; // this shoud be "inherit".

I think this is not !GetPresShell() case, it's no target element case. No?

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> Comment on attachment 8829329 [details]
> Bug 1295401: Use computed value for discetely animatable value in
> GetKeyframes().
> 
> https://reviewboard.mozilla.org/r/106428/#review112242
> 
> I will look into this after the big 'if' block becomes into smaller chuncks.
> 
> Thanks!
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp:1224
> (Diff revision 4)
> > +        nsCSSValue value = propertyValue.mValue;
> > +        if (nsCSSProps::kAnimTypeTable[propertyValue.mProperty] ==
> > +            eStyleAnimType_Discrete && GetPresShell() &&
> > +            (value.GetUnit() == eCSSUnit_Inherit ||
> > +             value.GetUnit() == eCSSUnit_Initial ||
> > +             value.GetUnit() == eCSSUnit_Unset)) {
> 
> Could you please split this block into (a) smaller function(s)?
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp:1235
> (Diff revision 4)
> > +          // GetPresShell() is NULL. In this case, does not come in this block
> > +          // to use uncomputed value as is.
> > +          // const effect = new KeyframeEffect(null,
> > +          //                                   [{ alignContent: "inherit" }],
> > +          //                                   1000);
> > +          // effect.getKeyframes()[0].alignContent; // this shoud be "inherit".
> 
> I think this is not !GetPresShell() case, it's no target element case. No?

Ah, and the target element is not associated with any document case.

Comment 29

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review112242

> Could you please split this block into (a) smaller function(s)?

Thank you very much for your quick review, Hiro!

Okay, will add a function something like IsDiscreteAnimationComputedValueNeeded. (maybe inline?)

> I think this is not !GetPresShell() case, it's no target element case. No?

Yap, thank you!
Will check mTarget instead.

Also, will add a test for that element is not associated with any document.

Comment 30

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106428/diff/4-5/

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Oops, sorry. I meant that I would like you to split 'processes' inside the if block.

Comment 32

[Daisuke Akatsuka (:daisuke)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> Oops, sorry. I meant that I would like you to split 'processes' inside the
> if block.

Oh, I'm sorry. will do that!

Comment 33

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106428/diff/5-6/

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review112682

::: dom/animation/KeyframeEffectReadOnly.cpp:1182
(Diff revision 6)
> +  return
> +    aValue.GetUnit() <= eCSSUnit_Unset && eCSSUnit_Inherit <= aValue.GetUnit();
> +}

Instead we should check all unset, initial and inherit here, since its order is hidden here.

I think we don't need to split these conditions in the inline functions.

::: dom/animation/KeyframeEffectReadOnly.cpp:1807
(Diff revision 6)
> +  }
> +
> +  nsIAtom* pseudo = mTarget->mPseudoType < CSSPseudoElementType::Count
> +                    ? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType)
> +                    : nullptr;
> +  return nsComputedDOMStyle::GetStyleContextForElement(mTarget->mElement,

In GetStyleContextForElement() we are gettig a pres shell from the given element and if we have the pres shell, we are using the pres shell threre.  I think we should just pass another pres shell for owner document in the case where we can't get the pres shell from the given element.

Also, I don't know much about the difference between composed document and owner document.  I think Brian knows of this difference.

::: dom/animation/KeyframeEffectReadOnly.cpp:1852
(Diff revision 6)
> +  // Finally, compute using the context.
> +  nsComputedDOMStyle::GetDiscretelyAnimatableCSSValue(
> +                        mTarget->mElement, styleContextWithDeclaration,
> +                        aPropertyValue.mProperty, aResult);

I am not sure this is a right way here. But if the style context has correct information, why can't we use GetDiscretelyAnimatedCSSValue() here?

Comment 35

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review112682

> I am not sure this is a right way here. But if the style context has correct information, why can't we use GetDiscretelyAnimatedCSSValue() here?

GetDiscretelyAnimatedCSSValue() returns uncomputed values as is.
So, we need to compute.

Comment 36

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106428/diff/6-7/

Comment 37

[Daniel Holbert [:dholbert]]

Can you clarify *where* the spec actually requires this behavior (using the computed value rather than just "initial" in getKeyframes)?  I don't see anything at https://w3c.github.io/web-animations/ about any expectations about the values in the property-value pairs.

If it doesn't say anything about that, perhaps it should (and perhaps it doesn't even need to require this complexity & could just allow for these keywords to be OK?)  This still feels like a lot of complexity, and I'm not clear on why it's needed.  If an author specifies "inherit" in their keyframes, it doesn't seem so bad to me that we might return the keyword "inherit" in getKeyframes...

Particularly if "inherit" happens to correspond to something that's animating separately (& faster) on the parent element, it seems like "inherit" would actually the most-correct thing to return in the child's getKeyframes output...

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review112954

Partial review:

::: dom/animation/AnimValuesStyleRule.cpp:64
(Diff revision 7)
>  {
> -  MOZ_ASSERT(false, "GetDiscretelyAnimatedCSSValue is not implemented yet");
> +  StyleAnimationValue styleValue;
> +  if (!this->GetValue(aProperty, styleValue)) {
> -  return false;
> +    return false;
> -}
> +  }
> +  *aValue = *styleValue.GetCSSValueValue();

In general, it's not safe to call GetCSSValueValue on an arbitrary StyleAnimationValue object, without having checked what sort of value it's storing.

In this case, I think the call is safe as long as the callers can be trusted.  So I think you're really relying on two (related) assumptions here (which IIUC the callers must enforce) -- you're assuming:
(1) aProperty is eAnimType_Discrete
(2) styleValue->GetUnit() is eUnit_DiscreteCSSValue

Please add MOZ_ASSERTs to verify both of those things -- verifying (1) at the beginning of the function (as a sanity-check on the args), and verifying (2) before the GetCSSValueValue call. (With assertion messages along the lines of "This API should only be called for discretely-animated properties")

::: dom/animation/KeyframeEffectReadOnly.h:449
(Diff revision 7)
> +  // Append given PropertyValue style to given style context.
> +  already_AddRefed<nsStyleContext> AppendStyle(
> +    const PropertyValuePair aPropertyValue, nsStyleContext* aContext);

The name & documentation for this method make it sound like it *modifies* |aContext| (by appending to it), but I don't think it actually does, right?

I think this really does something closer to:
  // Create & return a new nsStyleContext, which is the same as |aContext|,
  // but with one additional CSS rule to set the given PropertyValuePair.

(I also find the name confusing, but I can't come up with a reasonably-short clearer name at the moment, so maybe just fixing the documentation is sufficient.)

::: dom/animation/KeyframeEffectReadOnly.cpp:1238
(Diff revision 7)
> +          // If the unit of propertyValue.mValue is initial, inherit or unset,
> +          // we use computed value, but compute only the discretely animation
> +          // in here since other type is computed by nsCSSValue::AppendToString.
> +          // Also in case of such a following JavaScript code,
> +          // mTarget is NULL. In this case, do not handle in here
> +          // to use uncomputed value as is.

I didn't really understand what either of these sentences was saying at first. I think it'd be clearer if you explained this at a higher-level, perhaps. Something like the following:

          // For discretely-animatable properties, we preserve the author's
          // specified values in our animation code, which means that we may
          // have the special CSS keywords "unset", "inherit", or "initial" in
          // propertyValue.mValue at this point (if the author specified those
          // keywords).  Those are not valid as computed-style values, though,
          // so we cannot return them via this getKeyframes API!  So, we query
          // the style context to figure out what computed-style they actually
          // map to for our target element (if we have a target element).

(CAVEAT: I'm not actually sure about the "we cannot return them via this getKeyframes API" part of this comment. I'm assuming that's the case based on this bug, but per comment 37, I'm not clear on *why* that's the case, nor am I clear that it *should* be the case.)

::: dom/animation/KeyframeEffectReadOnly.cpp:1247
(Diff revision 7)
> +          // mTarget is NULL. In this case, do not handle in here
> +          // to use uncomputed value as is.
> +          // const effect = new KeyframeEffect(null,
> +          //                                   [{ alignContent: "inherit" }],
> +          //                                   1000);
> +          // effect.getKeyframes()[0].alignContent; // this shoud be "inherit".

typo: s/shoud/should/

Also: Are you actually sure that "inherit" is the right thing to return here? Does the spec actually cover this case, with a null target?  I can imagine "inherit" being the correct thing to return here, but I could also imagine that the spec might require the property's initial value here.  (i.e. maybe this should match the getComputedStyle behavior for root elements -- there, 'inherit' just produces the initial value.)

::: dom/animation/KeyframeEffectReadOnly.cpp:1251
(Diff revision 7)
> +            MOZ_ASSERT(baseStyleContext, "NULL style context");
> +            MOZ_ASSERT(baseStyleContext->PresContext()->StyleSet()->IsGecko(),
> +                       "Should be Gecko");

In general, it'd be helpful if these assertion messages (or code-comments alongside them) more clearly explained *why* the assertion is justified, rather than just repeating the asserted condition.

Assertions like...
  MOZ_ASSERT(foo, "foo shouldn't be null")
  MOZ_ASSERT(condition, "condition should hold")
...are a little too mysterious/hand-wavy (except for trivial cases like function-parameter null-checks when the function is documented as requiring a non-null parameter).

In the former case ("NULL style context"), I don't know enough from looking at this code to understand whether this assertion is justified.  And actually, after I dig a bit, it looks like it's *not* justified -- the function you're calling (GetTargetOwnerDocumentStyleContext) has a "return nullptr" case as one of the very first lines in the function! So I think this fatal null-check needs to be relaxed and that error-condition needs to actually be handled, unless you're really sure we can't hit that condition (in which case please make sure that's clear in the code).

In the latter case ("Should be Gecko"), please use something like this clearer assertion-message, which I copypasted from an assertion elsewhere in this file for a similar IsGecko() assertion:
 "ServoStyleSet should not use StyleAnimationValue for animations".

::: dom/animation/KeyframeEffectReadOnly.cpp:1808
(Diff revision 7)
> +already_AddRefed<nsStyleContext>
> +KeyframeEffectReadOnly::GetTargetOwnerDocumentStyleContext() {

Opening curly-brace should be on newline.

Also: since this only has one caller and only uses one piece of private data (mTarget):  you might want to just make this a static function in this file (rather than a KeyframeEffectReadOnly method) and have it take a "const OwningAnimationTarget&" as a parameter.

Also, the name is confusing -- it sounds like it gets the target's owner document's style context (and I don't know what that means).  I think maybe GetTargetStyleContextViaOwnerDoc() might be clearer?

::: dom/animation/KeyframeEffectReadOnly.cpp:1832
(Diff revision 7)
> +  RefPtr<css::Declaration> declaration = new css::Declaration();
> +  declaration->InitializeEmpty();
> +  nsCSSExpandedDataBlock block;

(I'm not familiar enough with our Declaration/nsCSSExpandedDataBlock/nsStyleSet/etc APIs & structures to feel comfortable reviewing these changes -- I'd prefer for a style-system peer to sign off on this, if possible.)

::: layout/style/nsComputedDOMStyle.h:134
(Diff revision 7)
> +  // Get computed style of discretely animatable property with given
> +  // nsStyleContext.
> +  // Animation type of nsCSSPropertyID must be eStyleAnimType_Discrete.
> +  static void GetDiscretelyAnimatableCSSValue(mozilla::dom::Element* aElement,

This needs to be documented a bit more thoroughly (to explain what it's producing and why), in part because the outparam (the returned "computed style") is a nsCSSValue, which isn't a type that we normally associate with computed style.  And also because aContext & aPropID would generally be the only things you'd need to know, to look up the computed style.

::: layout/style/nsComputedDOMStyle.cpp:722
(Diff revision 7)
> +{
> +  MOZ_ASSERT(!nsCSSProps::IsShorthand(aPropID),
> +             "nsCSSPropertyID should be longhand property");
> +  MOZ_ASSERT(nsCSSProps::kAnimTypeTable[aPropID] == eStyleAnimType_Discrete,
> +             "Animation type should be eStyleAnimType_Discrete");
> +  MOZ_ASSERT(aContext->Arena()->GetDocument(), "NULL nsIDocument");

Are we justified in asserting this?  (I'm not sure, but the "Get" in GetDocument suggests that we're not.)

I'm willing to believe there are (or could be in the future) edge cases around GetDocument()'s return value here, if e.g. an outer page destroys an iframe and then queries some state about that iframe.

This goes for all of the null-check fatal assertions below this -- if it's possible for them to return null, you really need to handle that case instead of just asserting about it. Or if it's not possible, you need to explain a bit more clearly (in the assertion message or in a comment alongside it) *why* you expect it to not be possible.

So: unless you're absolutely sure that GetDocument() must always return non-null here (and can explain the reason why in the assertion), we should make this an early-return (perhaps with nonfatal NS_ERROR) instead of an fatal assertion.

::: layout/style/nsComputedDOMStyle.cpp:724
(Diff revision 7)
> +  nsComputedDOMStyle computedStyle(aElement, EmptyString(), aContext->Arena(),
> +                                   nsComputedDOMStyle::eDefaultOnly);
> +  computedStyle.mStyleContext = aContext;
> +  const nsComputedStyleMap::Entry* propEntry =
> +    computedStyle.GetComputedStyleMap()->FindEntryForProperty(aPropID);

This style-churning is hard to follow. Please include some explanatory comments here.

e.g.:
  // To produce a nsCSSValue for the object, we:
  // * Create a nsComputedDOMStyle object, and invoke its getter for the
  // property that we're interested in, to produce a CSSValue that we can
  // then stringify.

...and then further down:
  // * Parse the stringified value, to produce a nsCSSValue.

::: layout/style/nsComputedDOMStyle.cpp:742
(Diff revision 7)
> +  if (error.StealNSResult() != NS_OK) {
> +    NS_WARNING("CSSValue::GetCssText failed");
> +    return;
> +  }
> +
> +  nsIDocument* doc = computedStyle.mStyleContext->Arena()->GetDocument();

(I think |computedStyle.mStyleContext| could be shortened to just |aContext| here, right?)

Comment 39

[Hiroyuki Ikezoe (:hiro)]

(In reply to Daniel Holbert [:dholbert] from comment #37)
> Can you clarify *where* the spec actually requires this behavior (using the
> computed value rather than just "initial" in getKeyframes)?  I don't see
> anything at https://w3c.github.io/web-animations/ about any expectations
> about the values in the property-value pairs.

Yesterday, I asked Brian the same thing, whether getKefyrames() returns computed value or specified. :-) 
Currently getKeyframes() returns specified values that actually were specified keyframes for script animations. *But* for CSS animations, it returns computed value due to css variables and, well, I forgot another one..

Comment 40

[Daniel Holbert [:dholbert]]

OK. If it's reasonably-sensible for the spec to allow inherit/unset/initial here, it seems worthwhile to fix this in the spec and leave our behavior as-is, IMO.

Let me elaborate on this scenario a bit, too, since I was a little hand-wavy:
(In reply to Daniel Holbert [:dholbert] from comment #37)
> Particularly if "inherit" happens to correspond to something that's
> animating separately (& faster) on the parent element, it seems like
> "inherit" would actually the most-correct thing to return in the child's
> getKeyframes output...

More details about the setup here:
 - Imagine we have two nested divs.
 - The parent div has an animation that snaps "align-content" (or any discretely-animated CSS property) between two values (say, "center" and "end") very rapidly (say, 5 times per second).
 - The child div has an animation that statically sets the property to inherit, e.g.:
     "0% { align-content: inherit } 100% { align-content: inherit }

In this setup, I would expect the child's actual computed value to rapidly snap between values just like the parent does.

So, what happens if we call getKeyframes() on the child here? What should it report as the value in its keyframes?  I think the patch right now would just use the current computed style at the time getKeyframes() is called. But that's not really an accurate reflection of what the animation keyframes reflect.  Really, "inherit" itself is the most accurate keyword that we can report for that scenario in getKeyframes, I think.

(This isn't specific to "inerhit", either -- a similar thing would happen for "unset" if the property inherits by default, since unset==inherit in that circumstance.  "initial" is simpler & could conceivably be translated away to a single defined value in all cases -- but I don't think it's worth giving that any special treatment here. If we allow inherit/unset, we might as well allow initial, since it's in the same "CSS-Wide" category of special keywords.)

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

https://reviewboard.mozilla.org/r/106428/#review112954

> This style-churning is hard to follow. Please include some explanatory comments here.
> 
> e.g.:
>   // To produce a nsCSSValue for the object, we:
>   // * Create a nsComputedDOMStyle object, and invoke its getter for the
>   // property that we're interested in, to produce a CSSValue that we can
>   // then stringify.
> 
> ...and then further down:
>   // * Parse the stringified value, to produce a nsCSSValue.

(Sorry - when I said "nsCSSValue for the object" in the first line of my suggested-sample-comment here, I think I really meant "...for the property" or something like that.)

Comment 42

[Brian Birtles (:birtles)]

Filling in some background here.

We can probably break the problem cases into two types:

a) When we are missing a keyframe and need to use the underlying value:

  @keyframes abc {
    to { margin-left: 100px; }
  }

b) When we have a keyframe that depends on something context-sensitive:

  e.g.

  @keyframes abc {
    from { margin-left: inherit; }
    to   { margin-left: 100px; }
  }

For (a), the spec says:

  "If a 0% or from keyframe is not specified, then the user agent constructs
  a 0% keyframe using the computed values of the properties being animated. If
  a 100% or to keyframe is not specified, then the user agent constructs a 100%
  keyframe using the computed values of the properties being animated."
  [ https://drafts.csswg.org/css-animations/#keyframes ]

There used to be language in the spec that said that these values were
snapshotted. I don't know when the disappeared but I can't find it now.

Test case for whether changes to the underlying value cause the animation to
update: https://jsfiddle.net/52yt7dp4/1/

Firefox and Chrome: The animation updates
Edge and Safari: The animation does not update (i.e. the keyframe values are
snapshotted).

What should getKeyframes() return here. Previously we did not have way to
represent underlying values. Also, we thought the spec required the snapshotting
behavior so we filled in a 0% keyframe with the computed values at the time when
the animation was generated (and, it seems we regenerate that keyframe in this
particular test case).

However, now that we can both represent underlying values by simply omitting
0%/100% keyframes (bug 1305325 -- and it seems the regressions following on from
that bug have been subsequently addressed) I suggest getKeyframes() should not
return any keyframe when none is specified. (We will probably also want to
update CSS animations to say "...the user agent *effectively* constructs...").
That is, when we generate the keyframes for CSS animations we should *not* fill
in 0% / 100% values. This matches our current behavior (and Chrome's) and is
generally simpler.

For (b) we have three problems:

i.   The spec needs to be updated to say that values used in keyframes are fully
     live.[1] This is dbaron's G-β proposal. I wanted to implement this before
     speccing it (to make sure I understood it properly) but I should probably
     just go ahead and spec it. According to Google folk, this gets *very*
     tricky when custom properties are involved, however.

ii.  We don't currently implement this. See these test cases:

     https://jsfiddle.net/4czqy600/3/
     https://jsfiddle.net/ykbppsL6/
     https://jsfiddle.net/frp0m8dh/1/

     Neither do Edge or Safari. Chrome does, however.
     Bug 1254424 covers part of this.

iii. Even if we implement this, it's not entirely clear what getKeyframes()
     should return.

For (iii) there is a description of some of the issues here:

  http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/layout/style/nsAnimationManager.cpp#688

Basically, if we want to preserve the input as closely as possible we need to do
mapping like the following:

  { margin: 10px; margin-left: 20px }
  => { margin: '10px', marginLeft: '20px' }

  { margin-left: 20px; margin: 10px }
  => { margin: '10px' }

  { margin-left: 20px; margin-left: 30px }
  => { marginLeft: '30px' }

I believe there were similar issues involving variables (see
bug 1268858 comment 19) but I don't remember exactly what they were. (Perhaps
the issue of how to handle variables that resolve to invalid values? Or are
there cases where a variable that expands to "20px 30px" might affect which
properties we return differently that if it just expanded to "20px").

In effect, we'd need to apply all the rules of the CSS cascade manually. So
instead, we just decided that until we have a spec saying otherwise, we should
just use the regular CSS cascade and pick up the computed values.

That seems less than ideal both for authors and because it means we really need
to regenerate those keyframes on every frame in order to produce the live
behavior of the test cases above. So I think we should probably try again to
work out a mapping for this. We could try something like servo where we expand
everything out and then group into shorthands but I think that would be hard to
spec and probably still means we end up converting 'inherit' and '20em' to their
computed values.


So, in summary, I think we need to:

* Not generate keyframes for missing 0%/100% values and relying on our support
  for animating from underlying values. (We'll still need the functions to get
  animation values from underlying style for transitions -- but in that case
  I think snapshotting is ok?)

* Try to work out a mapping from @keyframes to Keyframe objects that preserves
  the specified values.

[1] https://github.com/w3c/csswg-drafts/issues/66

Comment 43

[Brian Birtles (:birtles)]

Just a few notes about the mapping part since it's quite complicated.

For of all, just explaining how CSSAnimationBuilder::BuildAnimationFrames
*currently* works.

We get input that can look like the following:

  @keyframes abc {
    10%, 90% {
      margin-left: 10px;
      opacity: 0;
    }
    10% {
      opacity: 1;
    }
  }

So we iterate over each of the rules:

  10%, 90% {
    margin-left: 10px;
    opacity: 0;
  }

  and

  10% {
    opacity: 1;
  }

Then we make up key frames for each of the keys:

  10% {
    margin-left: 10px;
    opacity: 0;
  }

  90% {
    margin-left: 10px;
    opacity: 0;
  }

  10% {
    opacity: 1;
  }

Then do a *stable* sort:

  10% {
    margin-left: 10px;
    opacity: 0;
  }

  10% {
    opacity: 1;
  }

  90% {
    margin-left: 10px;
    opacity: 0;
  }

Then we walk backwards through the keyframes and pick the last value for each
property at each offset. We do something a little bit clever where we look for
an existing keyframe for a given offset, and if we don't have any we swap the
unique property values we've collected from the current keyframe with its
existing values. Anyway, we end up with:

  10% {
    margin-left: 10px;
    opacity: 1;
  }

  90% {
    margin-left: 10px;
    opacity: 0;
  }

One extra complication is that we only re-use an existing keyframe if it has
the same timing function as our current keyframe.

That means that, for example, in the following case we can merge the 'margin:
30px' with the 'left: 20px' because they have the same timing function:

  @keyframes abc {
    10% { margin: 10px,
          left: 20px,
          animation-timing-function: linear }
    20% { margin: 20px }
    30%, 10% { margin: 30px, animation-timing-function: linear }
  }
  =>
  [
    { margin: '30px', left: '10px', offset: 0.1, easing: 'linear' },
    { margin: '20px', offset: 0.2, easing: 'ease' },
    { margin: '30px', offset: 0.3, easing: 'linear' },
  ]

(Recall that the default value of animation-timing-function is 'ease'.)

But if they didn't, we'd end up with two keyframes with the same offset and
different timing functions.

  @keyframes abc {
    10% { margin: 10px,
          left: 20px,
          animation-timing-function: linear }
    20% { margin: 20px }
    30%, 10% { margin: 30px }
  }
  =>
  [
    { margin: '30px', offset: 0.1, easing: 'ease' },
    { left: '10px',   offset: 0.1, easing: 'linear' },
    { margin: '20px', offset: 0.2, easing: 'ease' },
    { margin: '30px', offset: 0.3, easing: 'ease' },
  ]

In future we will likely need to merge across independent keyframes but this
is not specced yet and I don't think anyone implements it. e.g.

  @keyframes abc {
    10% { margin: 10px }
    20% { margin: 30px }
  }
  @keyframes abc {
    10% { margin: 20px }
  }
  => [ { margin: '20px', offset: 0.1 }, { margin: '30px', offset: 0.2 } ]

As for the mapping, the challenge is we need to take some input that relies on
the CSS cascade and turn it into something that we can represent in
Javascript and these have different processing rules.

For CSS we have at least the following rules:

  1. Later declarations win over earlier ones.
     "The last declaration in document order wins."
     (https://drafts.csswg.org/css-cascade-3/#cascade-order)
  2. Shorthands reset all their shorthand properties.
     "A shorthand property sets all of its longhand sub-properties, exactly as
     if expanded in place."
     (https://drafts.csswg.org/css-cascade-3/#shorthand)

  I'm not sure if there are further requirements on invalid values and
  I wonder if we can actually handle them properly.

For JS we should assume there is ordering between object properties (there is
a roughly interoperable enumeration order, but it's not really defined and
differs in some cases, so we should assume there is none). Web Animations
defines the order in which these properties apply as follows:

  "If conflicts arise when expanding shorthand properties due to shorthand
  properties overlapping with existing longhand properties or other with other
  shorthand properties, apply the following rules in order until the conflict
  is resolved:

    1. Longhand properties override shorthand properties (e.g. border-top-color
       overrides border-top).

    2. Shorthand properties with fewer longhand components override those with
       more longhand components (e.g. border-top overrides border-color)

    3. For shorthand properties with an equal number of longhand components,
       properties whose IDL name (see the CSS property to IDL attribute
       algorithm [CSSOM]) appears earlier when sorted in ascending order by
       the Unicode codepoints that make up each IDL name, override those who
       appear later."

(From: https://w3c.github.io/web-animations/#calculating-computed-keyframes)

Combining those two sets of rules means we need a mapping like the following:

  {
    margin-left: 10px;
    margin: 20px;
  }
  => { margin: '20px' }

  (Since the margin shorthand clobbers margin-left.)

  {
    margin: 20px;
    margin-left: 10px;
  }
  => { margin: '20px', marginLeft: '10px' }
  (OR { marginLeft: '10px', margin: '20px' }, they are identical)

  (Since later declarations win, and Web Animations applies longhands on top
  of shorthands.)

  {
    margin: 20px;
    margin-left: 10px;
    margin-left: 30px;
  }
  => { margin: '20px', marginLeft: '30px' }
     { marginLeft: '30px', margin: '20px' }

But what about the following?

  {
    border-top: yellow thin;
    border-color: blue;
  }

Normally that would give:

  border-top: yellow thin;
    -> border-top-width: thin;
       border-top-style: none;
       border-top-color: currentColor;

  border-color: blue:
    -> border-top-color: blue;
       border-right-color: blue;
       border-bottom-color: blue;
       border-left-color: blue;

  i.e.
    -> border-top-width: thin;
       border-top-style: none;
       border-top-color: blue;
       border-right-color: blue;
       border-bottom-color: blue;
       border-left-color: blue;

However, if we produce the following keyframe,

  { borderTop: 'yellow thin', borderColor: 'blue' }

Web Animations will make border-top win so border-top-color will be yellow.

So we have a fundamental problem where because JS doesn't allow you to specify
an ordering between properties, we have to define a fixed ordering, but no
matter how we define that, for some cases it won't be the order we want.

We could expand the shorthands like so:

  { borderTopWidth: 'thin',
    borderTopStyle: 'width',
    borderColor: 'blue' }

However, what if we have:

  {
    border-top: var(--my-border-top);
    border-color: blue;
  }

We want to keep the variable reference as-is, but we also need to expand it so
that the border-top-color doesn't clobber border-color. But we don't know if
--my-border-top expands to 'yellow' or 'thin' or 'yellow thin' so we can't
just assign the variable to the long hand components.

Likewise, if we just try to repeat the overlapping value;

  { borderTop: 'yellow thin',
    borderColor: 'blue',
    borderTopColor: 'blue' }

it doesn't work with variable references:

  {
    border-top: yellow thin;
    border-color: var(--my-border-colors);
  }

Since we don't know if --my-border-colors is 'blue' or 'orange green red' so
we can't just assign |borderTopColor| to 'var(--my-border-colors)'.

Incidentally this is the same problem we face if we say we want to expand
all shorthands to longhands but keep the specified values.

It's easy enough until you get to CSS variables:

  { margin: 10px }
  => { marginLeft: 10px, ... }

  { margin: inherit }
  => { marginLeft: 'inherit', ... }

  { margin: var(--my-margin) }
  => { marginLeft: 'var(--my-margin)', ... }

  But what if --my-margin is '10px 30px'? Some longhands need a different
  part.

It's tempting to say, "Well, variables are complicated, expand them, but keep
the rest as specified values". The trouble is, we really *do* want to be able
to preserve variables as-is since in future we want to be able to animate
them, and it would be nice if we *don't* have to reconstruct keyframes on
every tick just to updated variable references. But maybe that's unavoidable?
We'd probably have to do that if we have 'animation-timing-function:
var(--my-timing-function)' so maybe that's just what we do?

Comment 44

[Daisuke Akatsuka (:daisuke)]

Thank you very much, Daniel, Hiro, Brian!

I filed new two bugs.
* bug 1339332 - Should not generate keyframes for missing 0%/100% values in CSS Animation
* bug 1339334 - Set specified value into keyframes

I'll fix above bugs, then return to this. (might no need?)

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8829329 [details]
Bug 1295401: Use computed value for discetely animatable value in GetKeyframes().

Clearing review request since, IIUC, we don't need to do this.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Closing since this bug is specific for the old style system.