Add CSS Transition support to registered custom properties

third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp

Index: third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
diff --git a/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp b/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
index e2e9d25d658720769c4dae7281cd1a32930e06c5..81af391596abb2b08e3f7f46a18d0e41814ed5ec 100644
--- a/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
+++ b/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
@@ -50,6 +50,8 @@
 #include "core/css/CSSPropertyEquality.h"
 #include "core/css/CSSPropertyMetadata.h"
 #include "core/css/CSSValueList.h"
+#include "core/css/PropertyRegistry.h"
+#include "core/css/parser/CSSVariableParser.h"
 #include "core/css/resolver/CSSToStyleMap.h"
 #include "core/css/resolver/StyleResolver.h"
 #include "core/dom/Element.h"
@@ -616,7 +618,7 @@ void CSSAnimations::calculateTransitionUpdateForProperty(
     TransitionUpdateState& state,
     const PropertyHandle& property,
     size_t transitionIndex) {
-  state.listedProperties.set(property.cssProperty() - firstCSSProperty);
+  state.listedProperties.insert(property);
 
   // FIXME: We should transition if an !important property changes even when an
   // animation is running, but this is a bit hard to do with the current
@@ -651,8 +653,17 @@ void CSSAnimations::calculateTransitionUpdateForProperty(
     }
   }
 
-  if (CSSPropertyEquality::propertiesEqual(property.cssProperty(),
-                                           state.oldStyle, state.style)) {
+  const PropertyRegistry* registry =
+      state.animatingElement->document().propertyRegistry();
+
+  if (property.isCSSCustomProperty()) {
+    if (!registry || !registry->registration(property.customPropertyName()) ||
+        CSSPropertyEquality::registeredCustomPropertiesEqual(
+            property.customPropertyName(), state.oldStyle, state.style)) {
+      return;
+    }
+  } else if (CSSPropertyEquality::propertiesEqual(
+                 property.cssProperty(), state.oldStyle, state.style)) {
     return;
   }
 
@@ -663,7 +674,7 @@ void CSSAnimations::calculateTransitionUpdateForProperty(
 
   // TODO(alancutter): Support transitions on registered custom properties and

[suzyh_UTC10 (ex-contributor), 2017/03/10 04:08:28]

Obsolete TODO?

[alancutter (OOO until 2018), 2017/03/12 23:52:07]

Thanks, removed.

   // give the map a PropertyRegistry.
-  CSSInterpolationTypesMap map(nullptr);
+  CSSInterpolationTypesMap map(registry);
   InterpolationEnvironment oldEnvironment(map, state.oldStyle);
   InterpolationEnvironment newEnvironment(map, state.style);
   InterpolationValue start = nullptr;
@@ -774,6 +785,23 @@ void CSSAnimations::calculateTransitionUpdateForProperty(
       !state.animatingElement->elementAnimations()->isAnimationStyleChange());
 }
 
+void CSSAnimations::calculateTransitionUpdateForCustomProperty(
+    TransitionUpdateState& state,
+    const CSSTransitionData::TransitionProperty& transitionProperty,
+    size_t transitionIndex) {
+  if (transitionProperty.propertyType !=

[suzyh_UTC10 (ex-contributor), 2017/03/10 04:08:28]

This is a little confusing. Comparing it to the propertyType !=
TransitionKnownProperty below, I wonder if it would make sense to move this
check to a separate function, and have something like

calculateTransitionUpdateForProperty(..) {
  if (propertyType == TransitionKnownProperty) {
    calculateTransitionUpdateForStandardProperty();
  } else if (propertyType == TransitionUnknownProperty) {
    calculateTransitionUpdateForCustomProperty();
  }
}

If you do this, then there might be other stuff that makes sense to move into
this function, but I haven't thought about it.

Having written this comment, I then realised that
calculateTransitionUpdateForProperty already exists and is called by this. Maybe
these functions are the wrong way around, i.e. calling code should call into
calculateTransitionUpdateForProperty rather than
calculateTransitionUpdateFor*Property. I haven't looked at the calling code to
see what's easiest to read at that point.

[alancutter (OOO until 2018), 2017/03/12 23:52:07]

I put this logic here because it's specific to custom properties and to avoid
adding too much to the call site calculateTransitionUpdate().
I'm open to naming suggestions, I agree the current names are a bit backwards
when viewed together but it's hard to come up with alternatives, in isolation
they are accurate names.

+      CSSTransitionData::TransitionUnknownProperty) {
+    return;
+  }
+  if (!CSSVariableParser::isValidVariableName(
+          transitionProperty.propertyString)) {
+    return;
+  }
+  calculateTransitionUpdateForProperty(
+      state, PropertyHandle(transitionProperty.propertyString),
+      transitionIndex);
+}
+
 void CSSAnimations::calculateTransitionUpdateForStandardProperty(
     TransitionUpdateState& state,
     const CSSTransitionData::TransitionProperty& transitionProperty,
@@ -807,6 +835,7 @@ void CSSAnimations::calculateTransitionUpdateForStandardProperty(
 }
 
 void CSSAnimations::calculateTransitionUpdate(CSSAnimationUpdate& update,
+                                              PropertyPass propertyPass,
                                               const Element* animatingElement,
                                               const ComputedStyle& style) {
   if (!animatingElement)
@@ -824,7 +853,7 @@ void CSSAnimations::calculateTransitionUpdate(CSSAnimationUpdate& update,
   const bool animationStyleRecalc =
       elementAnimations && elementAnimations->isAnimationStyleChange();
 
-  std::bitset<numCSSProperties> listedProperties;
+  HashSet<PropertyHandle> listedProperties;
   bool anyTransitionHadTransitionAll = false;
   const LayoutObject* layoutObject = animatingElement->layoutObject();
   if (!animationStyleRecalc && style.display() != EDisplay::None &&
@@ -842,26 +871,31 @@ void CSSAnimations::calculateTransitionUpdate(CSSAnimationUpdate& update,
       if (transitionProperty.unresolvedProperty == CSSPropertyAll) {
         anyTransitionHadTransitionAll = true;
       }
-      calculateTransitionUpdateForStandardProperty(state, transitionProperty,
+      if (propertyPass == PropertyPass::Custom) {
+        calculateTransitionUpdateForCustomProperty(state, transitionProperty,
                                                    transitionIndex);
+      } else {
+        DCHECK_EQ(propertyPass, PropertyPass::Standard);
+        calculateTransitionUpdateForStandardProperty(state, transitionProperty,
+                                                     transitionIndex);
+      }
     }
   }
 
   if (activeTransitions) {
     for (const auto& entry : *activeTransitions) {
       const PropertyHandle& property = entry.key;
-      // TODO(alancutter): Handle transitions on custom properties.
-      DCHECK(!property.isCSSCustomProperty());
-      CSSPropertyID id = property.cssProperty();
       if (!anyTransitionHadTransitionAll && !animationStyleRecalc &&
-          !listedProperties.test(id - firstCSSProperty)) {
+          !listedProperties.contains(property)) {
         update.cancelTransition(property);
       } else if (entry.value.animation->finishedInternal()) {
         update.finishTransition(property);
       }
     }
   }
-  calculateTransitionActiveInterpolations(update, animatingElement);
+
+  calculateTransitionActiveInterpolations(update, propertyPass,
+                                          animatingElement);
 }
 
 void CSSAnimations::cancel() {
@@ -927,8 +961,26 @@ void CSSAnimations::calculateAnimationActiveInterpolations(
       activeInterpolationsForAnimations);
 }
 
+static bool isCustomStylePropertyHandle(const PropertyHandle& property) {

[suzyh_UTC10 (ex-contributor), 2017/03/10 04:08:28]

I think these functions and isStylePropertyHandle belong in a util library.

[alancutter (OOO until 2018), 2017/03/12 23:52:07]

I disagree. They're only used in one file and they don't have any utility
elsewhere. Other places just call property.isCSSCustomProperty().

+  return property.isCSSCustomProperty();
+}
+
+static bool isStandardStylePropertyHandle(const PropertyHandle& property) {
+  return isStylePropertyHandle(property) && !property.isCSSCustomProperty();
+}
+
+static EffectStack::PropertyHandleFilter stylePropertyFilter(
+    CSSAnimations::PropertyPass propertyPass) {
+  if (propertyPass == CSSAnimations::PropertyPass::Custom) {
+    return isCustomStylePropertyHandle;
+  }
+  DCHECK_EQ(propertyPass, CSSAnimations::PropertyPass::Standard);
+  return isStandardStylePropertyHandle;
+}
+
 void CSSAnimations::calculateTransitionActiveInterpolations(
     CSSAnimationUpdate& update,
+    PropertyPass propertyPass,
     const Element* animatingElement) {
   ElementAnimations* elementAnimations =
       animatingElement ? animatingElement->elementAnimations() : nullptr;
@@ -940,7 +992,8 @@ void CSSAnimations::calculateTransitionActiveInterpolations(
       update.cancelledTransitions().isEmpty()) {
     activeInterpolationsForTransitions = EffectStack::activeInterpolations(
         effectStack, nullptr, nullptr,
-        KeyframeEffectReadOnly::TransitionPriority, isStylePropertyHandle);
+        KeyframeEffectReadOnly::TransitionPriority,
+        stylePropertyFilter(propertyPass));
   } else {
     HeapVector<Member<const InertEffect>> newTransitions;
     for (const auto& entry : update.newTransitions())
@@ -959,7 +1012,8 @@ void CSSAnimations::calculateTransitionActiveInterpolations(
 
     activeInterpolationsForTransitions = EffectStack::activeInterpolations(
         effectStack, &newTransitions, &cancelledAnimations,
-        KeyframeEffectReadOnly::TransitionPriority, isStylePropertyHandle);
+        KeyframeEffectReadOnly::TransitionPriority,
+        stylePropertyFilter(propertyPass));
   }
 
   // Properties being animated by animations don't get values from transitions
@@ -969,8 +1023,15 @@ void CSSAnimations::calculateTransitionActiveInterpolations(
     for (const auto& entry : update.activeInterpolationsForAnimations())
       activeInterpolationsForTransitions.erase(entry.key);
   }
-  update.adoptActiveInterpolationsForStandardTransitions(
-      activeInterpolationsForTransitions);
+
+  if (propertyPass == PropertyPass::Custom) {
+    update.adoptActiveInterpolationsForCustomTransitions(
+        activeInterpolationsForTransitions);
+  } else {
+    DCHECK_EQ(propertyPass, PropertyPass::Standard);

[suzyh_UTC10 (ex-contributor), 2017/03/10 04:08:28]

This if-else structure is a repeated pattern. Instead of an enum value guarding
two functions with very similar names, is there an alternative design using
inheritance/polymorphism?

[alancutter (OOO until 2018), 2017/03/12 23:52:07]

I could create a strategy pattern here though it would require defining three
new classes. I don't think the boilerplate and code indirection for the reader
is worth it. Perhaps if this were intended to grow more branches in future but
that's not the case here.

+    update.adoptActiveInterpolationsForStandardTransitions(
+        activeInterpolationsForTransitions);
+  }
 }
 
 EventTarget* CSSAnimations::AnimationEventDelegate::eventTarget() const {