Closed Bug 1339332 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(4 files)

Bug 1339332 - Part 1: Introduce neutral value concept for missing keyframe in CSS Animation.
59 bytes, text/x-review-board-request
birtles
: review+
hiro
: review+
Details
Bug 1339332 - Part 2: Remove no longer used method.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1339332 - Part 3: Test for missing keyframes in CSS Animation.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1339332 - Part 4: Drop setting 'add' composite operation for missing keyframes in script animation.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
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.
Assignee: nobody → dakatsuka
FYI, in stylo we are already filling missing keyframes, we should also fix it altogether.
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
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
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.
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?
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.
Ah, no I meant entire story of (2).  Sorry for the confusion.
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.
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).
(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.
Summary: Should not generate keyframes for missing 0%/100% values in CSS Animation → Do not fill computed values in missing 0%/100% keyframes in CSS Animation when generating keyframes
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 on attachment 8839279 [details]
Bug 1339332 - Part 2: Remove no longer used method.

https://reviewboard.mozilla.org/r/113960/#review115752
Attachment #8839279 - Flags: review?(bbirtles) → review+
(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?
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?
(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?
(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 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.
Attachment #8839278 - Flags: review?(bbirtles)
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').
Attachment #8839278 - Flags: review?(bbirtles) → review+
(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.
(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 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.
Attachment #8839278 - Flags: review?(hikezoe)
(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'.
The relevant part of Web Animation API spec has been changed?
The test is for getProperties() which is a chrome-only API, not part of Web Animations.
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'..
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?
Thanks. It gets much clearer to me.  Will we drop setting 'add' value for script animations in future?
This patch already does that in the change to AppendInitialSegment/AppendFinalSegment.
Yeah, I just realized it. I always read the test cases first.
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 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
Attachment #8839278 - Flags: review+
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.
Attachment #8843170 - Flags: review?(hikezoe) → review+
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.
Attachment #8843171 - Flags: review?(hikezoe) → review+
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.
(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.
I just landed patches for bug 1340322.  It might cause conflict patches for this bug. Sorry for the inconvenience.
(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!
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by bbirtles@mozilla.com:
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
Keywords: checkin-needed
Depends on: 1430884
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: