Support logical styles for programmatic animations

This patch adds support for logical styles in animations created via
element.animate(). Previously, CSSPropertyValueSets were kept for
shorthand properties for the sole purpose of serialization of the
shorthand property values in getKeyframes. These sets now hold
additional usage for reevaluation as needed to resolve naming conflicts
or applying updates after a change to the text direction or writing
mode. An optimistic strategy is used during the initial parse to
minimize computational overhead. A second pass of the property name
expansion is only applied when needed.

During style update, a change in text direction or writing mode will
trigger an update to all running animations. CSS animations and
transitions are affected only if modified by a setKeyframes call.
Keyframes without logical properties are also uneffected. Any keyframe
model with one or more affected keyframes is automatically invalidated
and resampled.

Bug: 865579
Change-Id: Ia2a34b465fadbb8c8f81aeefa72ea38d52fd97d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425167
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811291}

third_party/blink/renderer/core/animation/css/css_animations.cc

@@ -593,7 +593,7 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update,
                                              const ComputedStyle& style,
                                              const ComputedStyle* parent_style,
                                              StyleResolver* resolver) {
-  const ElementAnimations* element_animations =
+  ElementAnimations* element_animations =
       animating_element ? animating_element->GetElementAnimations() : nullptr;
 
   bool is_animation_style_change =
@@ -614,8 +614,22 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update,
   const ComputedStyle* old_style =
       animating_element ? animating_element->GetComputedStyle() : nullptr;
   bool logical_property_mapping_change =
-      old_style && (old_style->Direction() != style.Direction() ||
-                    old_style->GetWritingMode() != style.GetWritingMode());
+      !old_style || old_style->Direction() != style.Direction() ||
+      old_style->GetWritingMode() != style.GetWritingMode();
+
+  if (logical_property_mapping_change && element_animations) {
+    // Update computed keyframes for any running animations that depend on
+    // logical properties.
+    for (auto& entry : element_animations->Animations()) {
+      Animation* animation = entry.key;
+      if (auto* keyframe_effect =
+              DynamicTo<KeyframeEffect>(animation->effect())) {
+        keyframe_effect->SetLogicalPropertyResolutionContext(
+            style.Direction(), style.GetWritingMode());
+        animation->UpdateIfNecessary();
+      }
+    }
+  }
 
   const CSSAnimationData* animation_data = style.Animations();
   const CSSAnimations* css_animations =

third_party/blink/renderer/core/animation/effect_input.cc

@@ -46,6 +46,7 @@
 #include "third_party/blink/renderer/core/css/css_style_sheet.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/element.h"
+#include "third_party/blink/renderer/core/dom/node_computed_style.h"
 #include "third_party/blink/renderer/core/frame/frame_console.h"
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
 #include "third_party/blink/renderer/core/frame/local_frame.h"
@@ -707,6 +708,13 @@ StringKeyframeVector EffectInput::ParseKeyframesArgument(
                            ? element->GetDocument()
                            : *LocalDOMWindow::From(script_state)->document();
 
+  // Map logical to physical properties.
+  const ComputedStyle* style = element ? element->GetComputedStyle() : nullptr;
+  TextDirection text_direction =
+      style ? style->Direction() : TextDirection::kLtr;
+  WritingMode writing_mode =
+      style ? style->GetWritingMode() : WritingMode::kHorizontalTb;
+
   StringKeyframeVector parsed_keyframes;
   if (script_iterator.IsNull()) {
     parsed_keyframes = ConvertObjectForm(element, document, keyframes_obj,
@@ -717,6 +725,11 @@ StringKeyframeVector EffectInput::ParseKeyframesArgument(
                          script_state, exception_state);
   }
 
+  for (wtf_size_t i = 0; i < parsed_keyframes.size(); i++) {
+    StringKeyframe* keyframe = parsed_keyframes[i];
+    keyframe->SetLogicalPropertyResolutionContext(text_direction, writing_mode);
+  }
+
   if (!ValidatePartialKeyframes(parsed_keyframes)) {
     exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
                                       "Partial keyframes are not supported.");

third_party/blink/renderer/core/animation/keyframe_effect.cc

@@ -689,6 +689,18 @@ ActiveInterpolationsMap KeyframeEffect::InterpolationsForCommitStyles() {
   return results;
 }
 
+void KeyframeEffect::SetLogicalPropertyResolutionContext(
+    TextDirection text_direction,
+    WritingMode writing_mode) {
+  if (auto* model = DynamicTo<StringKeyframeEffectModel>(Model())) {
+    if (model->SetLogicalPropertyResolutionContext(text_direction,
+                                                   writing_mode)) {
+      ClearEffects();
+      InvalidateAndNotifyOwner();
+    }
+  }
+}
+
 void KeyframeEffect::CountAnimatedProperties() const {
   if (target_element_) {
     Document& document = target_element_->GetDocument();

third_party/blink/renderer/core/animation/keyframe_effect.h

@@ -142,6 +142,9 @@ class CORE_EXPORT KeyframeEffect final : public AnimationEffect {
   bool GetIgnoreCSSKeyframes() { return ignore_css_keyframes_; }
   void SetIgnoreCSSKeyframes() { ignore_css_keyframes_ = true; }
 
+  void SetLogicalPropertyResolutionContext(TextDirection text_direction,
+                                           WritingMode writing_mode);
+
  private:
   EffectModel::CompositeOperation CompositeInternal() const;
 

third_party/blink/renderer/core/animation/keyframe_effect_model.cc

@@ -287,6 +287,24 @@ bool KeyframeEffectModelBase::IsTransformRelatedEffect() const {
          Affects(PropertyHandle(GetCSSPropertyTranslate()));
 }
 
+bool KeyframeEffectModelBase::SetLogicalPropertyResolutionContext(
+    TextDirection text_direction,
+    WritingMode writing_mode) {
+  bool changed = false;
+  for (wtf_size_t i = 0; i < keyframes_.size(); i++) {
+    if (auto* string_keyframe = DynamicTo<StringKeyframe>(*keyframes_[i])) {
+      if (string_keyframe->HasLogicalProperty()) {
+        string_keyframe->SetLogicalPropertyResolutionContext(text_direction,
+                                                             writing_mode);
+        changed = true;
+      }
+    }
+  }
+  if (changed)
+    ClearCachedData();
+  return changed;
+}
+
 void KeyframeEffectModelBase::Trace(Visitor* visitor) const {
   visitor->Trace(keyframes_);
   visitor->Trace(keyframe_groups_);

third_party/blink/renderer/core/animation/keyframe_effect_model.h

@@ -159,6 +159,11 @@ class CORE_EXPORT KeyframeEffectModelBase : public EffectModel {
 
   bool IsTransformRelatedEffect() const override;
 
+  // Update properties used in resolving logical properties. Returns true if
+  // one or more keyframes changed as a result of the update.
+  bool SetLogicalPropertyResolutionContext(TextDirection text_direction,
+                                           WritingMode writing_mode);
+
   virtual KeyframeEffectModelBase* Clone() = 0;
 
   void Trace(Visitor*) const override;

third_party/blink/renderer/core/animation/string_keyframe.h

@@ -23,20 +23,16 @@ class StyleSheetContents;
 // StringKeyframe are expanded to shorthand and de-duplicated, with newer
 // properties replacing older ones. SVG attributes are similarly de-duplicated.
 //
-// TODO(smcgruer): By the spec, a StringKeyframe should not de-duplicate or
-// expand shorthand properties; that is done for computed keyframes.
 class CORE_EXPORT StringKeyframe : public Keyframe {
  public:
   StringKeyframe()
-      : css_property_map_(MakeGarbageCollected<MutableCSSPropertyValueSet>(
-            kHTMLStandardMode)),
-        presentation_attribute_map_(
+      : presentation_attribute_map_(
             MakeGarbageCollected<MutableCSSPropertyValueSet>(
                 kHTMLStandardMode)) {}
   StringKeyframe(const StringKeyframe& copy_from);
 
   MutableCSSPropertyValueSet::SetResult SetCSSPropertyValue(
-      const AtomicString& property_name,
+      const AtomicString& custom_property_name,
       const String& value,
       SecureContextMode,
       StyleSheetContents*);
@@ -46,6 +42,7 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
       SecureContextMode,
       StyleSheetContents*);
   void SetCSSPropertyValue(const CSSProperty&, const CSSValue&);
+
   void SetPresentationAttributeValue(const CSSProperty&,
                                      const String& value,
                                      SecureContextMode,
@@ -53,13 +50,16 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
   void SetSVGAttributeValue(const QualifiedName&, const String& value);
 
   const CSSValue& CssPropertyValue(const PropertyHandle& property) const {
+    EnsureCssPropertyMap();
     int index = -1;
-    if (property.IsCSSCustomProperty())
+    if (property.IsCSSCustomProperty()) {
       index =
           css_property_map_->FindPropertyIndex(property.CustomPropertyName());
-    else
+    } else {
+      DCHECK(!property.GetCSSProperty().IsShorthand());
       index = css_property_map_->FindPropertyIndex(
           property.GetCSSProperty().PropertyID());
+    }
     CHECK_GE(index, 0);
     return css_property_map_->PropertyAt(static_cast<unsigned>(index)).Value();
   }
@@ -86,6 +86,11 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
 
   Keyframe* Clone() const override;
 
+  bool HasLogicalProperty() { return has_logical_property_; }
+
+  bool SetLogicalPropertyResolutionContext(TextDirection text_direction,
+                                           WritingMode writing_mode);
+
   void Trace(Visitor*) const override;
 
   class CSSPropertySpecificKeyframe
@@ -160,21 +165,75 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
     String value_;
   };
 
+  class PropertyResolver : public GarbageCollected<PropertyResolver> {
+   public:
+    // Custom properties must use this version of the constructor.
+    PropertyResolver(CSSPropertyID property_id, const CSSValue& css_value);
+
+    // Shorthand and logical properties must use this version of the
+    // constructor.
+    PropertyResolver(const CSSProperty& property,
+                     const MutableCSSPropertyValueSet* property_value_set,
+                     bool is_logical);
+
+    static PropertyResolver* CreateCustomVariableResolver(
+        const CSSValue& css_value);
+
+    const CSSValue* CssValue();
+
+    void AppendTo(MutableCSSPropertyValueSet* property_value_set,
+                  TextDirection text_direction,
+                  WritingMode writing_mode);
+
+    void SetProperty(MutableCSSPropertyValueSet* property_value_set,
+                     CSSPropertyID property_id,
+                     const CSSValue& value,
+                     TextDirection text_direction,
+                     WritingMode writing_mode);
+
+    static bool HasLowerPriority(PropertyResolver* first,
+                                 PropertyResolver* second);
+
+    // Helper methods for resolving longhand name collisions.
+    // Longhands take priority over shorthands.
+    // Physical properties take priority over logical.
+    // Two shorthands with overlapping longhand properties are sorted based
+    // on the number of longhand properties in their expansions.
+    bool IsLogical() { return is_logical_; }
+    bool IsShorthand() { return css_property_value_set_; }
+    unsigned ExpansionCount() {
+      return css_property_value_set_ ? css_property_value_set_->PropertyCount()
+                                     : 1;
+    }
+
+    void Trace(Visitor* visitor) const;
+
+   private:
+    CSSPropertyID property_id_ = CSSPropertyID::kInvalid;
+    Member<const CSSValue> css_value_ = nullptr;
+    Member<ImmutableCSSPropertyValueSet> css_property_value_set_ = nullptr;
+    bool is_logical_ = false;
+  };
+
  private:
   Keyframe::PropertySpecificKeyframe* CreatePropertySpecificKeyframe(
       const PropertyHandle&,
       EffectModel::CompositeOperation effect_composite,
       double offset) const override;
 
+  void InvalidateCssPropertyMap() { css_property_map_ = nullptr; }
+  void EnsureCssPropertyMap() const;
+
   bool IsStringKeyframe() const override { return true; }
 
-  // The unresolved property and their values. This is needed for correct
-  // implementation of KeyframeEffect.getKeyframes(). We use a single list for
-  // CSS, SVG properties. The only requirement for a property value to be
-  // in this list is that it parses correctly.
-  //
+  // Mapping of unresolved properties to a their resolvers. A resolver knows
+  // how to expand shorthands to their corresponding longhand property names,
+  // convert logical to physical property names and compare precedence for
+  // resolving longhand name collisions.  The resolver also knows how to
+  // create serialized text for a shorthand, which is required for getKeyframes
+  // calls.
   // See: https://drafts.csswg.org/web-animations/#keyframes-section
-  HeapHashMap<PropertyHandle, Member<const CSSValue>> input_properties_;
+  HeapHashMap<PropertyHandle, Member<PropertyResolver>> input_properties_;
 
   // The resolved properties are computed from unresolved ones applying these
   // steps:
@@ -182,17 +241,21 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
   //      one (e.g., margin, margin-top)
   //  2. Expand shorthands to longhands
   //  3. Expand logical properties to physical ones
-  //
-  // See:
-  // https://drafts.csswg.org/web-animations/#calculating-computed-keyframes
-  //
-  // TODO(816956): AFAICT we don't do (1) at the moment rather we parse and feed
-  // values into the MutableCSSPropertyValueSet which keeps replacing values as
-  // they come in. I am not sure if it leads to the same conflict resolution
-  // that web-animation expects. This needs more investigation.
-  Member<MutableCSSPropertyValueSet> css_property_map_;
+  mutable Member<MutableCSSPropertyValueSet> css_property_map_;
   Member<MutableCSSPropertyValueSet> presentation_attribute_map_;
   HashMap<const QualifiedName*, String> svg_attribute_map_;
+
+  // If the keyframes contain one or more logical properties, these need to be
+  // remapped to physical properties when the writing mode or text direction
+  // changes.
+  bool has_logical_property_ = false;
+
+  // The following properties are required for mapping logical to physical
+  // property names. Though the same for all keyframes within the same model,
+  // we store the value here to facilitate lazy evaluation of the CSS
+  // properties.
+  TextDirection text_direction_ = TextDirection::kLtr;
+  WritingMode writing_mode_ = WritingMode::kHorizontalTb;
 };
 
 using CSSPropertySpecificKeyframe = StringKeyframe::CSSPropertySpecificKeyframe;

third_party/blink/web_tests/TestExpectations

@@ -4448,9 +4448,6 @@ crbug.com/1045599 virtual/layout-ng-grid/fast/css-grid-layout/should-not-collaps
 crbug.com/1045599 virtual/layout-ng-grid/fast/css-grid-layout/swap-lines-if-start-is-further-endward-than-end-line.html [ Failure Crash ]
 crbug.com/1045599 virtual/layout-ng-grid/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html [ Failure Crash ]
 
-# [css-logical]
-crbug.com/865579 external/wpt/css/css-logical/animation-001.html [ Failure Pass ]
-
 # [css-animations]
 crbug.com/993365 external/wpt/css/css-transitions/Element-getAnimations.tentative.html [ Failure Pass ]
 

third_party/blink/web_tests/external/wpt/css/css-logical/animation-001-expected.txt

+This is a testharness.js-based test.
+PASS Logical properties can be animated using object notation
+PASS Logical properties can be animated using array notation
+PASS Logical properties are NOT stored as physical properties
+PASS Logical properties in animations respect the writing-mode
+PASS Logical properties in animations respect the direction
+PASS Physical properties win over logical properties in object notation
+PASS Physical properties win over logical properties in array notation
+PASS Physical properties with variables win over logical properties
+FAIL Logical shorthands follow the usual prioritization based on number of component longhands assert_equals: expected "300px" but got "0px"
+PASS Physical longhands win over logical shorthands
+PASS Logical longhands win over physical shorthands
+PASS Physical shorthands win over logical shorthands
+PASS Physical shorthands using variables win over logical shorthands
+PASS Physical properties and logical properties can be mixed
+PASS Physical shorthands and logical shorthands can be mixed
+PASS Physical properties win over logical properties even when some keyframes only have logical properties
+PASS Animations update when the writing-mode is changed
+PASS Filling animations update when the writing-mode is changed
+PASS Animations with implicit from values update when the writing-mode is changed
+PASS Animations with overlapping physical and logical properties update when the writing-mode is changed
+PASS Animations update when the writing-mode is changed through a CSS variable
+PASS Animations update when the direction is changed
+PASS Logical shorthand with variable references animates correctly
+PASS writing-mode is not animatable
+PASS direction is not animatable
+Harness: the test ran to completion.
+

third_party/blink/web_tests/external/wpt/web-animations/interfaces/Animation/commitStyles-expected.txt

-This is a testharness.js-based test.
-PASS Commits styles
-PASS Commits styles for an animation that has been removed
-PASS Commits shorthand styles
-PASS Commits logical properties
-FAIL Commits logical properties as physical properties assert_equals: expected "20px" but got "10px"
-PASS Commits values calculated mid-interval
-PASS Commits variable references as their computed values
-PASS Commits custom variables
-PASS Commits em units as pixel values
-PASS Commits relative line-height
-PASS Commits transforms
-PASS Commits transforms as a transform list
-PASS Commits matrix-interpolated relative transforms
-PASS Commits "none" transform
-PASS Commits the intermediate value of an animation in the middle of stack
-PASS Commit composites on top of the underlying value
-PASS Triggers mutation observers when updating style
-PASS Does NOT trigger mutation observers when the change to style is redundant
-PASS Throws if the target element is a pseudo element
-PASS Throws if the target element is not something with a style attribute
-PASS Throws if the target effect is display:none
-PASS Throws if the target effect's ancestor is display:none
-PASS Treats display:contents as rendered
-PASS Treats display:contents in a display:none subtree as not rendered
-PASS Throws if the target effect is disconnected
-PASS Checks the pseudo element condition before the not rendered condition
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/web-animations/timing-model/timelines/update-and-send-events-replacement-expected.txt

@@ -17,9 +17,9 @@ PASS Removes an animation after updating another animation's effect to one with
 PASS Removes an animation after updating its effect to one with different properties
 PASS Removes an animation when another animation uses a shorthand
 PASS Removes an animation that uses a shorthand
-FAIL Removes an animation by another animation using logical properties assert_equals: expected "removed" but got "active"
-FAIL Removes an animation using logical properties assert_equals: expected "removed" but got "active"
-FAIL Removes an animation by another animation using logical properties after updating the context assert_equals: expected "removed" but got "active"
+PASS Removes an animation by another animation using logical properties
+PASS Removes an animation using logical properties
+PASS Removes an animation by another animation using logical properties after updating the context
 PASS Removes an animation after updating another animation's effect's target
 PASS Removes an animation after updating its effect's target
 PASS Removes an animation after updating another animation's effect to one with a different target