Bug 1404803 - Convert empty values to suitable zero values even when using discrete interpolation; r=hiro
authorBrian Birtles <birtles@gmail.com>
Wed, 04 Oct 2017 15:04:23 +0900
changeset 384600 d8974272e3e372f4a445fad9b3276a09b0bced96
parent 384599 118c5ac3af292e0e31517111160d508f8347b367
child 384601 7a0d51bfe126bc59d03835371f885639e9093369
push id32631
push userarchaeopteryx@coole-files.de
push dateThu, 05 Oct 2017 08:51:33 +0000
treeherdermozilla-central@66042a706980 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1404803
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1404803 - Convert empty values to suitable zero values even when using discrete interpolation; r=hiro Normally when we interpolate values for CSS properties we convert empty values (such as the empty "from" value created for a by-animation) to a suitable zero value. However, when we use discrete interpolation that step never occurs and in some rare cases that means we end up setting the empty value on the target attribute directly which will have an incorrect result (e.g. setting "" for the height property will have no effect, whereas setting "0px" will). This patch makes us perform the empty value to zero value conversion when performing discrete animation. Unfortunately it is not possible to test this behavior with shorthands since there are currently no shorthand properties that are additive. MozReview-Commit-ID: JFT1tis1RAR
dom/smil/nsSMILAnimationFunction.cpp
dom/smil/nsSMILCSSValueType.cpp
dom/smil/nsSMILCSSValueType.h
dom/smil/test/mochitest.ini
dom/smil/test/test_smilAdditionFallback.html
--- a/dom/smil/nsSMILAnimationFunction.cpp
+++ b/dom/smil/nsSMILAnimationFunction.cpp
@@ -465,16 +465,40 @@ nsSMILAnimationFunction::InterpolateResu
       // are the same as <set> animations.  Instead, we treat it as a
       // discrete animation with two values (the underlying value and
       // the to="" value), and honor keyTimes="" as well.
       uint32_t index = (uint32_t)floor(scaledSimpleProgress * 2);
       aResult = index == 0 ? aBaseValue : aValues[0];
     } else {
       uint32_t index = (uint32_t)floor(scaledSimpleProgress * aValues.Length());
       aResult = aValues[index];
+
+      // For animation of CSS properties, normally when interpolating we perform
+      // a zero-value fixup which means that empty values (values with type
+      // nsSMILCSSValueType but a null pointer value) are converted into
+      // a suitable zero value based on whatever they're being interpolated
+      // with. For discrete animation, however, since we don't interpolate,
+      // that never happens. In some rare cases, such as discrete non-additive
+      // by-animation, we can arrive here with |aResult| being such an empty
+      // value so we need to manually perform the fixup.
+      //
+      // We could define a generic method for this on nsSMILValue but its faster
+      // and simpler to just special case nsSMILCSSValueType.
+      if (aResult.mType == &nsSMILCSSValueType::sSingleton) {
+        // We have currently only ever encountered this case for the first
+        // value of a by-animation (which has two values) and since we have no
+        // way of testing other cases we just skip them (but assert if we
+        // ever do encounter them so that we can add code to handle them).
+        if (index + 1 >= aValues.Length()) {
+          MOZ_ASSERT(aResult.mU.mPtr, "The last value should not be empty");
+        } else {
+          // Base the type of the zero value on the next element in the series.
+          nsSMILCSSValueType::FinalizeValue(aResult, aValues[index + 1]);
+        }
+      }
     }
     rv = NS_OK;
   }
   return rv;
 }
 
 nsresult
 nsSMILAnimationFunction::AccumulateResult(const nsSMILValueArray& aValues,
--- a/dom/smil/nsSMILCSSValueType.cpp
+++ b/dom/smil/nsSMILCSSValueType.cpp
@@ -867,8 +867,56 @@ nsSMILCSSValueType::PropertyFromValue(co
 
   const ValueWrapper* wrapper = ExtractValueWrapper(aValue);
   if (!wrapper) {
     return eCSSProperty_UNKNOWN;
   }
 
   return wrapper->mPropID;
 }
+
+// static
+void
+nsSMILCSSValueType::FinalizeValue(nsSMILValue& aValue,
+                                  const nsSMILValue& aValueToMatch)
+{
+  MOZ_ASSERT(aValue.mType == aValueToMatch.mType, "Incompatible SMIL types");
+  MOZ_ASSERT(aValue.mType == &nsSMILCSSValueType::sSingleton,
+             "Unexpected SMIL value type");
+
+  ValueWrapper* valueWrapper = ExtractValueWrapper(aValue);
+  // If |aValue| already has a value, there's nothing to do here.
+  if (valueWrapper) {
+    return;
+  }
+
+  const ValueWrapper* valueToMatchWrapper = ExtractValueWrapper(aValueToMatch);
+  if (!valueToMatchWrapper) {
+    MOZ_ASSERT_UNREACHABLE("Value to match is empty");
+    return;
+  }
+
+  bool isServo = !valueToMatchWrapper->mServoValues.IsEmpty();
+
+  if (isServo) {
+    ServoAnimationValues zeroValues;
+    zeroValues.SetCapacity(valueToMatchWrapper->mServoValues.Length());
+
+    for (auto& valueToMatch : valueToMatchWrapper->mServoValues) {
+      RefPtr<RawServoAnimationValue> zeroValue =
+        Servo_AnimationValues_GetZeroValue(valueToMatch).Consume();
+      if (!zeroValue) {
+        return;
+      }
+      zeroValues.AppendElement(Move(zeroValue));
+    }
+    aValue.mU.mPtr = new ValueWrapper(valueToMatchWrapper->mPropID,
+                                      Move(zeroValues));
+  } else {
+    const StyleAnimationValue* zeroValue =
+      GetZeroValueForUnit(valueToMatchWrapper->mGeckoValue.GetUnit());
+    if (!zeroValue) {
+      return;
+    }
+    aValue.mU.mPtr = new ValueWrapper(valueToMatchWrapper->mPropID,
+                                      *zeroValue);
+  }
+}
--- a/dom/smil/nsSMILCSSValueType.h
+++ b/dom/smil/nsSMILCSSValueType.h
@@ -119,14 +119,30 @@ public:
    *
    * @param   aValue   The nsSMILValue to examine.
    * @return           The nsCSSPropertyID enum value of the property animated
    *                   by |aValue|, or eCSSProperty_UNKNOWN if the type of
    *                   |aValue| is not nsSMILCSSValueType.
    */
   static nsCSSPropertyID PropertyFromValue(const nsSMILValue& aValue);
 
+  /**
+   * If |aValue| is an empty value, converts it to a suitable zero value by
+   * matching the type of value stored in |aValueToMatch|.
+   *
+   * There is no indication if this method fails. If a suitable zero value could
+   * not be created, |aValue| is simply unmodified.
+   *
+   * @param aValue        The nsSMILValue (of type nsSMILCSSValueType) to
+   *                      possibly update.
+   * @param aValueToMatch A nsSMILValue (of type nsSMILCSSValueType) for which
+   *                      a corresponding zero value will be created if |aValue|
+   *                      is empty.
+   */
+  static void FinalizeValue(nsSMILValue& aValue,
+                            const nsSMILValue& aValueToMatch);
+
 private:
   // Private constructor: prevent instances beyond my singleton.
   constexpr nsSMILCSSValueType() {}
 };
 
 #endif // NS_SMILCSSVALUETYPE_H_
--- a/dom/smil/test/mochitest.ini
+++ b/dom/smil/test/mochitest.ini
@@ -8,16 +8,17 @@ support-files =
   db_smilMappedAttrList.js
   file_smilWithTransition.html
   smilAnimateMotionValueLists.js
   smilExtDoc_helper.svg
   smilTestUtils.js
   smilXHR_helper.svg
 
 [test_smilAccessKey.xhtml]
+[test_smilAdditionFallback.html]
 [test_smilAnimateMotion.xhtml]
 [test_smilAnimateMotionInvalidValues.xhtml]
 [test_smilAnimateMotionOverrideRules.xhtml]
 [test_smilBackwardsSeeking.xhtml]
 [test_smilCSSFontStretchRelative.xhtml]
 [test_smilCSSFromBy.xhtml]
 [test_smilCSSFromTo.xhtml]
 [test_smilCSSInherit.xhtml]
new file mode 100644
--- /dev/null
+++ b/dom/smil/test/test_smilAdditionFallback.html
@@ -0,0 +1,30 @@
+<!doctype html>
+<meta charset=utf-8>
+<head>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<p id=display></p>
+<div id=content>
+<svg id=svg>
+<!-- These two animations will have a default duration of indefinite which means
+     they will keep producing their first value forever -->
+<animate calcMode="discrete" attributeName="height" by="10" dur="1s"
+  fill="freeze"/>
+<animate calcMode="discrete" attributeName="height" by="10" dur="1s"
+  fill="freeze"/>
+</svg>
+</div>
+<pre id="test">
+<script>
+'use strict';
+
+SimpleTest.waitForExplicitFinish();
+
+window.addEventListener('load', () => {
+  const svg = document.getElementById('svg');
+  is(getComputedStyle(svg).height, '0px', 'Computed height should be zero');
+  SimpleTest.finish();
+});
+</script>
+</pre>