Add CSS Transition support to registered custom properties

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

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 32 matching lines @@
 #include "core/animation/InterpolationEnvironment.h"
 #include "core/animation/InterpolationType.h"
 #include "core/animation/KeyframeEffectModel.h"
 #include "core/animation/KeyframeEffectReadOnly.h"
 #include "core/animation/TransitionInterpolation.h"
 #include "core/animation/css/CSSAnimatableValueFactory.h"
 #include "core/css/CSSKeyframeRule.h"
 #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"
 #include "core/dom/PseudoElement.h"
 #include "core/dom/StyleEngine.h"
 #include "core/events/AnimationEvent.h"
 #include "core/events/TransitionEvent.h"
 #include "core/frame/UseCounter.h"
 #include "core/layout/LayoutObject.h"
 #include "core/paint/PaintLayer.h"
@@ skipping 546 matching lines @@
         UseCounter::mapCSSPropertyIdToCSSSampleIdForHistogram(
             property.cssProperty()));
   }
   clearPendingUpdate();
 }
 
 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
   // applyMatchedProperties system.
   if (state.update.activeInterpolationsForAnimations().contains(property) ||
       (state.animatingElement->elementAnimations() &&
        state.animatingElement->elementAnimations()
            ->cssAnimations()
            .m_previousActiveInterpolationsForAnimations.contains(property))) {
     return;
@@ skipping 14 matching lines @@
       DCHECK(!state.animatingElement->elementAnimations() ||
              !state.animatingElement->elementAnimations()
                   ->isAnimationStyleChange());
 
       if (to->equals(runningTransition->reversingAdjustedStartValue.get())) {
         interruptedTransition = runningTransition;
       }
     }
   }
 
-  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;
   }
 
   if (!to)
     to = CSSAnimatableValueFactory::create(property, state.style);
   RefPtr<AnimatableValue> from =
       CSSAnimatableValueFactory::create(property, state.oldStyle);
 
   // 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;
   InterpolationValue end = nullptr;
   const InterpolationType* transitionType = nullptr;
   for (const auto& interpolationType : map.get(property)) {
     start = interpolationType->maybeConvertUnderlyingValue(oldEnvironment);
     if (!start) {
       continue;
     }
@@ skipping 90 matching lines @@
   TransitionKeyframeEffectModel* model =
       TransitionKeyframeEffectModel::create(keyframes);
   state.update.startTransition(
       property, from.get(), to.get(), reversingAdjustedStartValue,
       reversingShorteningFactor, *InertEffect::create(model, timing, false, 0));
   DCHECK(
       !state.animatingElement->elementAnimations() ||
       !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,
     size_t transitionIndex) {
   if (transitionProperty.propertyType !=
       CSSTransitionData::TransitionKnownProperty) {
     return;
   }
 
   CSSPropertyID resolvedID =
@@ skipping 13 matching lines @@
     if (!animateAll &&
         !CSSPropertyMetadata::isInterpolableProperty(longhandID)) {
       continue;
     }
 
     calculateTransitionUpdateForProperty(state, property, transitionIndex);
   }
 }
 
 void CSSAnimations::calculateTransitionUpdate(CSSAnimationUpdate& update,
+                                              PropertyPass propertyPass,
                                               const Element* animatingElement,
                                               const ComputedStyle& style) {
   if (!animatingElement)
     return;
 
   if (animatingElement->document().finishingOrIsPrinting())
     return;
 
   ElementAnimations* elementAnimations = animatingElement->elementAnimations();
   const TransitionMap* activeTransitions =
       elementAnimations ? &elementAnimations->cssAnimations().m_transitions
                         : nullptr;
   const CSSTransitionData* transitionData = style.transitions();
 
   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 &&
       layoutObject && layoutObject->style() && transitionData) {
     TransitionUpdateState state = {
         update,         animatingElement,  *layoutObject->style(),
         style,          activeTransitions, listedProperties,
         *transitionData};
 
     for (size_t transitionIndex = 0;
          transitionIndex < transitionData->propertyList().size();
          ++transitionIndex) {
       const CSSTransitionData::TransitionProperty& transitionProperty =
           transitionData->propertyList()[transitionIndex];
       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() {
   for (const auto& runningAnimation : m_runningAnimations) {
     runningAnimation->animation->cancel();
     runningAnimation->animation->update(TimingUpdateOnDemand);
   }
 
   for (const auto& entry : m_transitions) {
     entry.value.animation->cancel();
@@ skipping 45 matching lines @@
     newEffects.push_back(updatedAnimation.effect);
 
   ActiveInterpolationsMap activeInterpolationsForAnimations(
       EffectStack::activeInterpolations(
           effectStack, &newEffects, &update.suppressedAnimations(),
           KeyframeEffectReadOnly::DefaultPriority, isStylePropertyHandle));
   update.adoptActiveInterpolationsForAnimations(
       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;
   EffectStack* effectStack =
       elementAnimations ? &elementAnimations->effectStack() : nullptr;
 
   ActiveInterpolationsMap activeInterpolationsForTransitions;
   if (update.newTransitions().isEmpty() &&
       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())
       newTransitions.push_back(entry.value.effect.get());
 
     HeapHashSet<Member<const Animation>> cancelledAnimations;
     if (!update.cancelledTransitions().isEmpty()) {
       DCHECK(elementAnimations);
       const TransitionMap& transitionMap =
           elementAnimations->cssAnimations().m_transitions;
       for (const PropertyHandle& property : update.cancelledTransitions()) {
         DCHECK(transitionMap.contains(property));
         cancelledAnimations.insert(transitionMap.at(property).animation.get());
       }
     }
 
     activeInterpolationsForTransitions = EffectStack::activeInterpolations(
         effectStack, &newTransitions, &cancelledAnimations,
-        KeyframeEffectReadOnly::TransitionPriority, isStylePropertyHandle);
+        KeyframeEffectReadOnly::TransitionPriority,
+        stylePropertyFilter(propertyPass));
   }
 
   // Properties being animated by animations don't get values from transitions
   // applied.
   if (!update.activeInterpolationsForAnimations().isEmpty() &&
       !activeInterpolationsForTransitions.isEmpty()) {
     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 {
   return EventPath::eventTargetRespectingTargetRules(*m_animationTarget);
 }
 
 void CSSAnimations::AnimationEventDelegate::maybeDispatch(
     Document::ListenerType listenerType,
     const AtomicString& eventName,
     double elapsedTime) {
@@ skipping 160 matching lines @@
              isCustomPropertyHandle);
 }
 
 DEFINE_TRACE(CSSAnimations) {
   visitor->trace(m_transitions);
   visitor->trace(m_pendingUpdate);
   visitor->trace(m_runningAnimations);
 }
 
 }  // namespace blink