1339332 - Do not fill computed values in missing 0%/100% keyframes in CSS Animation when generating keyframes

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Daisuke Akatsuka (:daisuke)]

As Brian suggests in bug 1295401 comment 42,
we do not generate the missing keyframes for CSS Animation.
Also, KeyframeEffectReadOnly::GetKeyframes should not return the missing keyframes.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

FYI, in stylo we are already filling missing keyframes, we should also fix it altogether.

Comment 2

[Daisuke Akatsuka (:daisuke)]

I have tried this.
However, the following sample, it can't work along the spec[1] if we don't generate the keyframe of 0% which has the specified timing function( in sample case steps(2) ).

@keyframes anim {
  50% {
    margin-left: 100px;
  }
  to {
    margin-left: 200px;
  }
}

div {
  animation: anim 1s steps(2);
}

I talked with Brian, 
we will stop to fix this bug since we can't keep a consistency.

[1] https://drafts.csswg.org/css-animations/#animation-timing-function

Comment 3

[Brian Birtles (:birtles)]

One idea we talked about was re-using the neutral value concept from the Web Animations spec.[1] We could make this a special internal use value and then when someone calls getKeyframes() from JS, resolve it to a computed value (underlying value) at that time.

Later, we can consider if this should be exposed through the Web-facing API.

The only tricky bit is for discrete properties that are not normally able to be added, we need to make sure we can add <neutral> such that it gives back the original value.

[1] https://w3c.github.io/web-animations/#neutral-value-for-composition

Comment 4

[Brian Birtles (:birtles)]

Daisuke and I discussed where neutral values should be converted into computed values. There are two obvious places:

1) In UpdateProperties
2) In ComposeStyle

The advantage of (1) is it is very simple. The disadvantage, however, is that we need to call UpdateProperties any time the base style changes. (However, having said that, will we end up calling UpdateProperties every time base style changes anyway?)

Thinking about (2), it seems we can re-use additive animation quite a lot. Basically we need to:

* Allow nsCSSValue to represent a neutral value (can we just use a null value of some sort here?)
* Make GetKeyframes() turn neutral nsCSSValue objects into the equivalent computed style
* In KeyframeUtils::GetComputedKeyframeValues turn neutral nsCSSValue objects into null StyleAnimationValue objects with 'composite: add' AND the corresponding timing function from the keyframe (this is the difference between simply just omitting the keyframe).

I think that be re-using additive animation like this it should be fairly easy to maintain.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Agree. (2) is a preferable approach in the end.  To achieve (2) we need bug 1338944 first. Or do we have a way to get the computed values without nsStyleContext?

Comment 6

[Brian Birtles (:birtles)]

Do you mean we need bug 1338944 for GetKeyframes()? The proposal is that we only turn neutral nsCSSValue objects into the equivalent computed style for the JS version of GetKeyframes() where we should be ok to resolve style.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Ah, no I meant entire story of (2).  Sorry for the confusion.

Comment 8

[Brian Birtles (:birtles)]

Oh, you mean for it to be optimized? I don't really understand what bug 1338944 is about so I'm not sure what you're saying won't work.

Comment 9

[Brian Birtles (:birtles)]

I'm guessing bug 1338944 is intended to be part of bug 1254424. So perhaps this is just about correctness in some cases (where we are already incorrect for CSS animations etc. anyway).

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Ah, no I meant entire story of (2).  Sorry for the confusion.

I wanted to write 'Ah, no. I meant..'. As for GetKeyframes() bug 1338944 is not related at all.

 (In reply to Brian Birtles (:birtles) from comment #9)
> I'm guessing bug 1338944 is intended to be part of bug 1254424. So perhaps
> this is just about correctness in some cases (where we are already incorrect
> for CSS animations etc. anyway).

Yes, that's right.

Comment 11

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

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

Comment 12

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1339332 - Part 2: Remove no longer used method.

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

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

https://reviewboard.mozilla.org/r/113958/#review115528

Some initial feedback. Later I'll want Hiro to have a look too.

I'm currently wondering if we need to set the composite mode on these keyframes to additive or whether null is sufficient. I suspect the way you've done it is simpler and probably better but I need to think about it more. Maybe Hiro has some suggestions.

::: dom/animation/KeyframeEffectReadOnly.cpp:1183
(Diff revision 1)
> -          : propertyValue.mProperty;
>          propertyValue.mValue.AppendToString(
> -          propertyForSerializing, stringValue, nsCSSValue::eNormalized);
> +          eCSSProperty_UNKNOWN, stringValue, nsCSSValue::eNormalized);
> +      } else {
> +        nsCSSValue cssValue = propertyValue.mValue;
> +        if (propertyValue.mValue.GetUnit() == eCSSUnit_Null) {

I think we should add a comment here saying:

  // We use an uninitialized nsCSSValue to represent the "neutral value". We
  // currently only do this for keyframes generated from CSS animations with
  // missing 0%/100% keyframes. Furthermore, currently (at least until bug
  // 1339334) keyframes generated from CSS animations only contain longhand
  // properties so we only need to handle null nsCSSValues for longhand
  // properties.

::: dom/animation/KeyframeEffectReadOnly.cpp:1183
(Diff revision 1)
> -          : propertyValue.mProperty;
>          propertyValue.mValue.AppendToString(
> -          propertyForSerializing, stringValue, nsCSSValue::eNormalized);
> +          eCSSProperty_UNKNOWN, stringValue, nsCSSValue::eNormalized);
> +      } else {
> +        nsCSSValue cssValue = propertyValue.mValue;
> +        if (propertyValue.mValue.GetUnit() == eCSSUnit_Null) {

s/propertyValue.mValue/cssValue/ here

(Normally I would say we should try to avoid copying an nsCSSValue in the common case, since they can be expensive. However, I don't think this JS-only GetKeyframes() needs to be heavily optimized.)

::: dom/animation/KeyframeEffectReadOnly.cpp:1184
(Diff revision 1)
>          propertyValue.mValue.AppendToString(
> -          propertyForSerializing, stringValue, nsCSSValue::eNormalized);
> +          eCSSProperty_UNKNOWN, stringValue, nsCSSValue::eNormalized);
> +      } else {
> +        nsCSSValue cssValue = propertyValue.mValue;
> +        if (propertyValue.mValue.GetUnit() == eCSSUnit_Null) {
> +          // Use base style since the value is neutral.

If we have the comment I mentioned above, I don't think we need this comment.

::: dom/animation/KeyframeEffectReadOnly.cpp:1580
(Diff revision 1)
>        if (segment.mFromComposite != CompositeOperation::Replace ||
>            segment.mToComposite != CompositeOperation::Replace) {
>          mCumulativeChangeHint = ~nsChangeHint_Hints_CanIgnoreIfNotVisible;
>          return;
>        }
> -      RefPtr<nsStyleContext> fromContext =
> +      RefPtr<nsStyleContext> fromContext;
> +      if (segment.mFromValue.IsNull()) {
> +        fromContext = aStyleContext;
> +      } else {
> +        fromContext =
> -        CreateStyleContextForAnimationValue(property.mProperty,
> +          CreateStyleContextForAnimationValue(property.mProperty,
> -                                            segment.mFromValue.mGecko,
> +                                              segment.mFromValue.mGecko,
> -                                            aStyleContext);
> +                                              aStyleContext);
> +      }
>  
> -      RefPtr<nsStyleContext> toContext =
> +      RefPtr<nsStyleContext> toContext;
> +      if (segment.mToValue.IsNull()) {
> +        toContext = aStyleContext;
> +      } else {
> +        toContext =
> -        CreateStyleContextForAnimationValue(property.mProperty,
> +          CreateStyleContextForAnimationValue(property.mProperty,
> -                                            segment.mToValue.mGecko,
> +                                              segment.mToValue.mGecko,
> -                                            aStyleContext);
> +                                              aStyleContext);
> +      }

I thought we were going to make nsAnimationManager.cpp set the composite mode to additive for these keyframes so we wouldn't need this change?

Shouldn't we be hitting the early return here?

::: dom/animation/KeyframeEffectReadOnly.cpp:1758
(Diff revision 1)
> +void
> +KeyframeEffectReadOnly::GetBaseStyleCSSValue(const nsCSSPropertyID aProperty,
> +                                             nsCSSValue& aResult) const
> +{
> +  DebugOnly<bool> uncomputeResult =
> +    StyleAnimationValue::UncomputeValue(aProperty,
> +                                        BaseStyle(aProperty), aResult);
> +
> +  MOZ_ASSERT(uncomputeResult,
> +             "Unable to get specified value from computed value");
> +
> +  MOZ_ASSERT(aResult.GetUnit() != eCSSUnit_Null, "Got null computed value");
> +}

I'm not sure if we need this as a separate method? It's only called once and basically just wraps UncomputeValue. If we do keep it I think we should probably make the signature:

  nsCSSValue
  KeyframeEffectReadOnly::BaseStyleAsCSSValue(const nsCSSPropertyID aProperty)

That is:

* Return the nsCSSValue by value (we have move-constructors defined for nsCSSValue so this should be fairly cheap)
* Drop the 'Get' from the name since we never expect the result to be null
* Call it BaseStyleAsCSSValue?

::: dom/animation/KeyframeEffectReadOnly.cpp:1768
(Diff revision 1)
> +
> +  MOZ_ASSERT(aResult.GetUnit() != eCSSUnit_Null, "Got null computed value");

Drop the blank line between these two assertions.

::: dom/animation/KeyframeUtils.cpp:657
(Diff revision 1)
> +          // As neutral value.
> +          PropertyStyleAnimationValuePair* neutralPair = values.AppendElement();
> +          neutralPair->mProperty = pair.mProperty;

I would drop the initial comment and add a comment below, as:

  PropertyStyleAnimationValuePair* neutralPair = values.AppendElement();
  neutralPair->mProperty = pair.mProperty;
  // An uninitialized nsCSSValue represents the underlying value which we
  // represent as an uninitialized AnimationValue so we just leave
  // neutralPair->mValue as-is.

::: layout/style/nsAnimationManager.cpp:1042
(Diff revision 1)
>      nsCSSPropertyID aProperty,
>      nsTArray<PropertyValuePair>& aPropertyValues)
>  {
>    PropertyValuePair propertyValue;
>    propertyValue.mProperty = aProperty;
> -  propertyValue.mValue = GetComputedValue(aPresContext, aProperty);
> +  propertyValue.mValue = nsCSSValue(); // As neutral value.

Weren't we going to make these keyframes use a composite mode of 'add'? Maybe that's not necessary?

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8839279 [details]
Bug 1339332 - Part 2: Remove no longer used method.

https://reviewboard.mozilla.org/r/113960/#review115752

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #13)
> Comment on attachment 8839278 [details]
> Bug 1339332 - Part 1: Introduce natural value concept for missing keyframe
> in CSS Animation.
> 
> https://reviewboard.mozilla.org/r/113958/#review115528
> 
> Some initial feedback. Later I'll want Hiro to have a look too.
> 
> I'm currently wondering if we need to set the composite mode on these
> keyframes to additive or whether null is sufficient. I suspect the way
> you've done it is simpler and probably better but I need to think about it
> more. Maybe Hiro has some suggestions.

Yes, we should set 'add'.  Without this I think KeyframeEffectReadOnly::BaseStyle() does not work at all.  Worked?

Comment 16

[Brian Birtles (:birtles)]

Daisuke and I are wondering if we need to use composite: add for missing keyframes. It seems that if you have a special missing/neutral/underlying value, then it behaves the same regardless of the 'composite' mode, i.e. it just equals the underlying value.

So, we are wondering if it would be simpler to just use 'replace' in that case. I think that would at least involve

* Using replace in AppendInitialSegment/AppendFinalSegment
* Making EnsureBaseStyle check for *either* keyframes without replace, *or* keyframes with null values
* Making CalculateCumulativeChangeHint support null values

Can you see any problems with this approach?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #16)
> Daisuke and I are wondering if we need to use composite: add for missing
> keyframes. It seems that if you have a special missing/neutral/underlying
> value, then it behaves the same regardless of the 'composite' mode, i.e. it
> just equals the underlying value.
> 
> So, we are wondering if it would be simpler to just use 'replace' in that
> case. I think that would at least involve
> 
> * Using replace in AppendInitialSegment/AppendFinalSegment
> * Making EnsureBaseStyle check for *either* keyframes without replace, *or*
> keyframes with null values
> * Making CalculateCumulativeChangeHint support null values
> 
> Can you see any problems with this approach?

Nothing. That should work.  Does that imply neutral value will be defined against replace as well?

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #16)

> * Making CalculateCumulativeChangeHint support null values

Oh, I forget to mention about this.  We should skip CalculateCumulativeChangeHint in case of null value.  We can not determine correct change hints.

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

https://reviewboard.mozilla.org/r/113958/#review115806

Clearing review request for now since I'd like to check this again with the various issues addressed (although, as we discovered, it's not as important to fix this as we previously thought).

::: dom/animation/KeyframeUtils.cpp:711
(Diff revision 1)
> -      entry->mComposite =
> -        frame.mComposite ? frame.mComposite.value() : aEffectComposite;
> +      entry->mComposite = entry->mValue.IsNull() // Neutral value
> +                          ? dom::CompositeOperation::Add
> +                          : frame.mComposite
> +                          ? frame.mComposite.value() : aEffectComposite;

Apart from this being quite hard to read, as discussed, I think we should just leave this as-is and change EnsureBaseStyles to produce base styles when we have null values too.

Comment 20

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

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

Comment 21

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839279 [details]
Bug 1339332 - Part 2: Remove no longer used method.

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

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

https://reviewboard.mozilla.org/r/113958/#review117024

Commit message: s/natural/neutral/

Other than that, looks good to me with the following changed, but I'd like to get Hiro to have a look to, to make sure he agrees.

::: dom/animation/KeyframeEffectReadOnly.cpp:333
(Diff revision 2)
>    CompositeOperation aCompositeOperation)
>  {
>    switch (aCompositeOperation) {
>      case dom::CompositeOperation::Replace:
> -      return aValueToComposite;
> +      // Return the underlying value if |aValueToComposite| is null (i.e.
> +      // missing keyframe). Otherwise, just return the |aValueToComposite|.

Nit: Drop the 'the' in this sentence.

i.e. "Otherwise, just return |aValueToComposite|."

::: dom/animation/KeyframeEffectReadOnly.cpp:334
(Diff revision 2)
>  {
>    switch (aCompositeOperation) {
>      case dom::CompositeOperation::Replace:
> -      return aValueToComposite;
> +      // Return the underlying value if |aValueToComposite| is null (i.e.
> +      // missing keyframe). Otherwise, just return the |aValueToComposite|.
> +      return aValueToComposite.IsNull() ? aUnderlyingValue : aValueToComposite;

As discussed, I think it would be slightly simpler if we make null values work the same regardless of composite mode. i.e. they should always just mean "use the underlying value".

Regarding wanting to check that we don't accidentally set null when we don't mean it, we can add an assertion somewhere else for that---although I'm not really sure where. I thought we could add it to GetComputedKeyframeValues but it doesn't quite seem right. (Do we want to check for keyframes themselves have an explicit replace mode? Or also for a missing composite mode but where the effect has a replace composite mode?) Maybe we don't need this check, or maybe CompositeValue is the right place to do this.

In any case, for this function, I would suggest just adding an early return at the beginning of the function:

  if (aValueToComposite.IsNull()) {
    return aUnderlyingValue;
  }

(Note to self: Some day we should probably rework this function to benefit from RVO but returning the same object every time.)

::: dom/animation/KeyframeEffectReadOnly.cpp:426
(Diff revision 2)
>    MOZ_ASSERT(!aValueToComposite.IsNull() ||
> -             aCompositeOperation == CompositeOperation::Add,
> -             "InputValue should be null only if additive composite");
> +             aCompositeOperation == CompositeOperation::Replace,
> +             "InputValue should be null only if replace composite");
>  

Based on the above, I think we can just drop this assertion.

::: dom/animation/KeyframeEffectReadOnly.cpp:451
(Diff revision 2)
> -      if (segment.mFromComposite == dom::CompositeOperation::Replace &&
> +      if (!segment.mFromValue.IsNull() && !segment.mToValue.IsNull() &&
> +          segment.mFromComposite == dom::CompositeOperation::Replace &&
>            segment.mToComposite == dom::CompositeOperation::Replace) {
>          continue;

Nit: I think this would be easier to read with each condition on a separate line:

      if (!segment.mFromValue.IsNull() &&
          !segment.mToValue.IsNull() &&
          segment.mFromComposite == dom::CompositeOperation::Replace &&
          segment.mToComposite == dom::CompositeOperation::Replace) {

::: dom/animation/KeyframeUtils.cpp:1173
(Diff revision 2)
>                       const KeyframeValueEntry& aFirstEntry)
>  {
>    AnimationPropertySegment* segment =
>      aAnimationProperty->mSegments.AppendElement();
>    segment->mFromKey        = 0.0f;
> -  segment->mFromComposite  = dom::CompositeOperation::Add;
> +  segment->mFromComposite  = dom::CompositeOperation::Replace;

I think we don't need to set this since AnimationPropertySegment initializes it to 'replace'.

::: dom/animation/KeyframeUtils.cpp:1189
(Diff revision 2)
>      aAnimationProperty->mSegments.AppendElement();
>    segment->mFromKey        = aLastEntry.mOffset;
>    segment->mFromValue      = aLastEntry.mValue;
>    segment->mFromComposite  = aLastEntry.mComposite;
>    segment->mToKey          = 1.0f;
> -  segment->mToComposite    = dom::CompositeOperation::Add;
> +  segment->mToComposite    = dom::CompositeOperation::Replace;

Likewise here.

::: layout/base/nsLayoutUtils.cpp:599
(Diff revision 2)
>          UpdateMinMaxScale(aFrame, { baseStyle, nullptr }, aMinScale, aMaxScale);
>        }
>  
>        for (const AnimationPropertySegment& segment : prop.mSegments) {
>          // In case of add or accumulate composite, StyleAnimationValue does
> -        // not have a valid value.
> +        // not have a valid valuea.

I think this change is unintended (i.e. should be just 'value').

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #22)

> ::: dom/animation/KeyframeEffectReadOnly.cpp:334
> (Diff revision 2)
> >  {
> >    switch (aCompositeOperation) {
> >      case dom::CompositeOperation::Replace:
> > -      return aValueToComposite;
> > +      // Return the underlying value if |aValueToComposite| is null (i.e.
> > +      // missing keyframe). Otherwise, just return the |aValueToComposite|.
> > +      return aValueToComposite.IsNull() ? aUnderlyingValue : aValueToComposite;
> 
> As discussed, I think it would be slightly simpler if we make null values
> work the same regardless of composite mode. i.e. they should always just
> mean "use the underlying value".
> 
> Regarding wanting to check that we don't accidentally set null when we don't
> mean it, we can add an assertion somewhere else for that---although I'm not
> really sure where. I thought we could add it to GetComputedKeyframeValues
> but it doesn't quite seem right. (Do we want to check for keyframes
> themselves have an explicit replace mode? Or also for a missing composite
> mode but where the effect has a replace composite mode?) Maybe we don't need
> this check, or maybe CompositeValue is the right place to do this.

Yeah, totally agree.  I thought we should add an assertion that the null value && Replace happens only for CSS Animations somewhere.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #22)
> ::: dom/animation/KeyframeUtils.cpp:1173
> (Diff revision 2)
> >                       const KeyframeValueEntry& aFirstEntry)
> >  {
> >    AnimationPropertySegment* segment =
> >      aAnimationProperty->mSegments.AppendElement();
> >    segment->mFromKey        = 0.0f;
> > -  segment->mFromComposite  = dom::CompositeOperation::Add;
> > +  segment->mFromComposite  = dom::CompositeOperation::Replace;
> 
> I think we don't need to set this since AnimationPropertySegment initializes
> it to 'replace'.

Yes, we shouldn't. This change breaks script animations.
I will look this patch once a revised patch is posted.
Thanks!

Comment 25

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

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

Comment 26

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839279 [details]
Bug 1339332 - Part 2: Remove no longer used method.

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

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

https://reviewboard.mozilla.org/r/113958/#review117026

I don't still understand why we need to modify the expected results in test_animation_properties.html.
I can't review the whole of this patch until I surely understand this change.

::: layout/style/nsAnimationManager.cpp:1027
(Diff revision 3)
>      if (!aAnimatedProperties.HasProperty(prop)) {
>        continue;
>      }
>  
>      if (startKeyframe && !aPropertiesSetAtStart.HasProperty(prop)) {
> -      AppendProperty(aPresContext, prop, startKeyframe->mPropertyValues);
> +      AppendProperty(prop, startKeyframe->mPropertyValues);

I think we can now append a PropertyValuePair to startKeyframe->mPropertyValues directly here instead of calling AppendProperty.

Comment 28

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> Comment on attachment 8839278 [details]
> Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe
> in CSS Animation.
> 
> https://reviewboard.mozilla.org/r/113958/#review117026
> 
> I don't still understand why we need to modify the expected results in
> test_animation_properties.html.
> I can't review the whole of this patch until I surely understand this change.

With these patches when we have missing keyframes we now synthesize a keyframe with a null value and a composite mode of 'replace' instead of 'add'.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

The relevant part of Web Animation API spec has been changed?

Comment 30

[Brian Birtles (:birtles)]

The test is for getProperties() which is a chrome-only API, not part of Web Animations.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Oh, OK. Hmm it's really weird to me. The spec says 'add' is used for missing offset 0 or 1, but getProperties() returns 'replace'..

Comment 32

[Brian Birtles (:birtles)]

Perhaps we should change the sentence:

  "Define the neutral value for composition as a value which, when combined with an underlying value using the add composite operation, produces the underlying value."

to:

  "Define the neutral value for composition as a value which, when combined with an underlying value using any composite operation, produces the underlying value."

And then say that the synthesized keyframes just use a composite mode of replace?

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Thanks. It gets much clearer to me.  Will we drop setting 'add' value for script animations in future?

Comment 34

[Brian Birtles (:birtles)]

This patch already does that in the change to AppendInitialSegment/AppendFinalSegment.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Yeah, I just realized it. I always read the test cases first.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

https://reviewboard.mozilla.org/r/113958/#review118114

OK. Now I think I understand this correctly.  We should have split this patch into more smaller ones. e.g. drop setting 'add' for missing keyframes for script animations, changes for css animations and getProperties() part. I am not sure getProperties() part can be split.

That being said, at least the commit message should be changed and should leave more comments about the impact against script animations.

::: dom/animation/KeyframeEffectReadOnly.cpp:384
(Diff revision 3)
>    CompositeOperation aCompositeOperation)
>  {
> +  if (aValueToComposite.IsNull()) {
> +    // Return the underlying value if |aValueToComposite| is null (i.e.
> +    // missing keyframe).
> +    MOZ_ASSERT(aCompositeOperation == dom::CompositeOperation::Replace,

After the discussion with Brian, now I am not sure this assertion is valuable.

::: dom/animation/KeyframeEffectReadOnly.cpp:470
(Diff revision 3)
> -  if (aCompositeOperation == CompositeOperation::Replace) {
> -    MOZ_ASSERT(!aValueToComposite.IsNull(),
> -      "Input value should be valid in case of replace composite");
> +  if (aCompositeOperation == CompositeOperation::Replace &&
> +      !aValueToComposite.IsNull()) {
> +    // Just copy the input value in case of 'Replace' composite

Unlike the above change in /* static */ CompositeChange(), why we check 'replace' composite instead of MOZ_ASSERT?  Is there any strong reason?  I think now we can drop the check.

::: dom/animation/KeyframeEffectReadOnly.cpp:503
(Diff revision 3)
> -      if (segment.mFromComposite == dom::CompositeOperation::Replace &&
> +      if (!segment.mFromValue.IsNull() &&
> +          !segment.mToValue.IsNull() &&
> +          segment.mFromComposite == dom::CompositeOperation::Replace &&
>            segment.mToComposite == dom::CompositeOperation::Replace) {

I think now we should add a method to check this  AnimationPropertySegment. Then we can use it in CalculateCumulativeChangeHint() as well.

::: dom/animation/KeyframeEffectReadOnly.cpp
(Diff revision 3)
> -      if (fromValue.IsNull() || toValue.IsNull()) {
> -        continue;
> -      }
>  
> +      MOZ_ASSERT(!fromValue.IsNull(), "from value should not be null");
> +      MOZ_ASSERT(!toValue.IsNull(), "to value should not be null");

Please leave as it is.  IIRC, we don't use MOZ_ASSERT intentionally here.

::: dom/animation/KeyframeEffectReadOnly.cpp:1253
(Diff revision 3)
> +          // CSS animations only contain longhand properties so we only need to
> +          // handle null nsCSSValues for longhand properties.
> +          DebugOnly<bool> uncomputeResult =
> +            StyleAnimationValue::UncomputeValue(
> +              propertyValue.mProperty,
> +              BaseStyle(propertyValue.mProperty), cssValue);

Can't we move this base style?

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

https://reviewboard.mozilla.org/r/113958/#review118120

Comment 38

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.

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

Comment 39

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.

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

Comment 40

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

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

Comment 41

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839279 [details]
Bug 1339332 - Part 2: Remove no longer used method.

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

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8843170 [details]
Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.

https://reviewboard.mozilla.org/r/116930/#review118570

We should unify assert_properties_equal() and value() somehow.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8843171 [details]
Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.

https://reviewboard.mozilla.org/r/116932/#review118572

I think this should be part 1, it does not really matter though.

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8843171 [details]
Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.

https://reviewboard.mozilla.org/r/116932/#review118572

Ah, we can't do it.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #42)
> Comment on attachment 8843170 [details]
> Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.
> 
> https://reviewboard.mozilla.org/r/116930/#review118570
> 
> We should unify assert_properties_equal() and value() somehow.

To be clear, I mean assert_properties_equal and value in other test files.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

I just landed patches for bug 1340322.  It might cause conflict patches for this bug. Sorry for the inconvenience.

Comment 47

[Daisuke Akatsuka (:daisuke)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> I just landed patches for bug 1340322.  It might cause conflict patches for
> this bug. Sorry for the inconvenience.

Okay, Thank you for the information!

Comment 48

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

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

Comment 49

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839279 [details]
Bug 1339332 - Part 2: Remove no longer used method.

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

Comment 50

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843170 [details]
Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.

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

Comment 51

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843171 [details]
Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.

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

Comment 52

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843170 [details]
Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.

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

Comment 53

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843171 [details]
Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.

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

Comment 54

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843170 [details]
Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.

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

Comment 55

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843171 [details]
Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.

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

Comment 56

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839278 [details]
Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.

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

Comment 57

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8839279 [details]
Bug 1339332 - Part 2: Remove no longer used method.

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

Comment 58

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843170 [details]
Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.

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

Comment 59

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8843171 [details]
Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.

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

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c88d48b94ec
Part 1: Introduce neutral value concept for missing keyframe in CSS Animation. r=birtles,hiro
https://hg.mozilla.org/integration/autoland/rev/bdec9101fe1b
Part 2: Remove no longer used method. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d15db3c13987
Part 3: Test for missing keyframes in CSS Animation. r=hiro
https://hg.mozilla.org/integration/autoland/rev/9b60539590ed
Part 4: Drop setting 'add' composite operation for missing keyframes in script animation. r=hiro

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c88d48b94ec
https://hg.mozilla.org/mozilla-central/rev/bdec9101fe1b
https://hg.mozilla.org/mozilla-central/rev/d15db3c13987
https://hg.mozilla.org/mozilla-central/rev/9b60539590ed