Bug 1402219 - Compute css variables with custom properties in keframes for getKeyframes(). r?birtles draft
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Tue, 26 Sep 2017 11:34:53 +0900
changeset 670340 f468126882a201813d1de75a4e48c06bdaac7bb1
parent 670339 792bd5d465c58ef0bd4a70961de338ee5ba1e9df
child 733210 3e0063833618eb477889c7a9ad5abc8641db42f9
push id81603
push userhikezoe@mozilla.com
push dateTue, 26 Sep 2017 09:33:47 +0000
reviewersbirtles
bugs1402219
milestone58.0a1
Bug 1402219 - Compute css variables with custom properties in keframes for getKeyframes(). r?birtles MozReview-Commit-ID: 7CMnWbzzemY
dom/animation/KeyframeEffectReadOnly.cpp
dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html
layout/style/ServoBindingList.h
servo/components/style/properties/declaration_block.rs
servo/ports/geckolib/glue.rs
--- a/dom/animation/KeyframeEffectReadOnly.cpp
+++ b/dom/animation/KeyframeEffectReadOnly.cpp
@@ -1250,28 +1250,48 @@ KeyframeEffectReadOnly::GetKeyframes(JSC
     }
 
     JS::Rooted<JS::Value> keyframeJSValue(aCx);
     if (!ToJSValue(aCx, keyframeDict, &keyframeJSValue)) {
       aRv.Throw(NS_ERROR_FAILURE);
       return;
     }
 
+    RefPtr<RawServoDeclarationBlock> customProperties;
+    // For CSS Animations in servo backend, custom properties in keyframe are
+    // stored in a servo's declaration block. Find the declaration block
+    // to resolve CSS variables in the keyframe.
+    if (isServo && isCSSAnimation) {
+      for (const PropertyValuePair& propertyValue : keyframe.mPropertyValues) {
+        if (propertyValue.mProperty ==
+              nsCSSPropertyID::eCSSPropertyExtra_variable) {
+          customProperties = propertyValue.mServoDeclarationBlock;
+          break;
+        }
+      }
+    }
+
     JS::Rooted<JSObject*> keyframeObject(aCx, &keyframeJSValue.toObject());
     for (const PropertyValuePair& propertyValue : keyframe.mPropertyValues) {
       nsAutoString stringValue;
       if (isServo) {
+        // Don't serialize the custom properties for this keyframe.
+        if (propertyValue.mProperty ==
+              nsCSSPropertyID::eCSSPropertyExtra_variable) {
+          continue;
+        }
         if (propertyValue.mServoDeclarationBlock) {
           const ServoStyleContext* servoStyleContext =
             styleContext ? styleContext->AsServo() : nullptr;
           Servo_DeclarationBlock_SerializeOneValue(
             propertyValue.mServoDeclarationBlock,
             propertyValue.mProperty,
             &stringValue,
-            servoStyleContext);
+            servoStyleContext,
+            customProperties);
         } else {
           RawServoAnimationValue* value =
             mBaseStyleValuesForServo.GetWeak(propertyValue.mProperty);
 
           if (value) {
             Servo_AnimationValue_Serialize(value,
                                            propertyValue.mProperty,
                                            &stringValue);
--- a/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html
+++ b/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html
@@ -131,23 +131,31 @@
 }
 
 @keyframes anim-background-size {
   to { background-size: 50%, 6px, contain }
 }
 
 :root {
   --var-100px: 100px;
+  --end-color: rgb(255, 0, 0);
 }
 @keyframes anim-variables {
   to { transform: translate(var(--var-100px), 0) }
 }
 @keyframes anim-variables-shorthand {
   to { margin: var(--var-100px) }
 }
+@keyframes anim-variables-in-keyframe {
+  to { --end-color: rgb(0, 255, 0); color: var(--end-color) }
+}
+@keyframes anim-only-custom-property-in-keyframe {
+  from { transform: translate(100px, 0) }
+  to { --not-used: 200px }
+}
 </style>
 <body>
 <script>
 "use strict";
 
 function getKeyframes(e) {
   return e.getAnimations()[0].effect.getKeyframes();
 }
@@ -694,11 +702,52 @@ test(function(t) {
       marginRight: "100px",
       marginTop: "100px" },
   ];
   for (var i = 0; i < frames.length; i++) {
     assert_frames_equal(frames[i], expected[i], "ComputedKeyframe #" + i);
   }
 }, 'KeyframeEffectReadOnly.getKeyframes() returns expected values for ' +
    'animations with CSS variables as keyframe values in a shorthand property');
+
+test(function(t) {
+  var div = addDiv(t);
+  div.style.animation = 'anim-variables-in-keyframe 100s';
+
+  var frames = getKeyframes(div);
+
+  assert_equals(frames.length, 2, "number of frames");
+
+  var expected = [
+    { offset: 0, computedOffset: 0, easing: "ease",
+      color: "rgb(0, 0, 0)" },
+    { offset: 1, computedOffset: 1, easing: "ease",
+      color: "rgb(0, 255, 0)" },
+  ];
+  for (var i = 0; i < frames.length; i++) {
+    assert_frames_equal(frames[i], expected[i], "ComputedKeyframe #" + i);
+  }
+}, 'KeyframeEffectReadOnly.getKeyframes() returns expected values for ' +
+   'animations with a CSS variable which is overriden by the value in keyframe');
+
+test(function(t) {
+  var div = addDiv(t);
+  div.style.animation = 'anim-only-custom-property-in-keyframe 100s';
+
+  var frames = getKeyframes(div);
+
+  assert_equals(frames.length, 2, "number of frames");
+
+  var expected = [
+    { offset: 0, computedOffset: 0, easing: "ease",
+      transform: "translate(100px, 0px)" },
+    { offset: 1, computedOffset: 1, easing: "ease",
+      transform: "none" },
+  ];
+  for (var i = 0; i < frames.length; i++) {
+    assert_frames_equal(frames[i], expected[i], "ComputedKeyframe #" + i);
+  }
+}, 'KeyframeEffectReadOnly.getKeyframes() returns expected values for ' +
+   'animations with only custom property in a keyframe');
+
 done();
 </script>
 </body>
--- a/layout/style/ServoBindingList.h
+++ b/layout/style/ServoBindingList.h
@@ -386,17 +386,18 @@ SERVO_BINDING_FUNC(Servo_DeclarationBloc
                    RawServoDeclarationBlockBorrowed a,
                    RawServoDeclarationBlockBorrowed b)
 SERVO_BINDING_FUNC(Servo_DeclarationBlock_GetCssText, void,
                    RawServoDeclarationBlockBorrowed declarations,
                    nsAString* result)
 SERVO_BINDING_FUNC(Servo_DeclarationBlock_SerializeOneValue, void,
                    RawServoDeclarationBlockBorrowed declarations,
                    nsCSSPropertyID property, nsAString* buffer,
-                   ServoStyleContextBorrowedOrNull computed_values)
+                   ServoStyleContextBorrowedOrNull computed_values,
+                   RawServoDeclarationBlockBorrowedOrNull custom_properties)
 SERVO_BINDING_FUNC(Servo_DeclarationBlock_Count, uint32_t,
                    RawServoDeclarationBlockBorrowed declarations)
 SERVO_BINDING_FUNC(Servo_DeclarationBlock_GetNthProperty, bool,
                    RawServoDeclarationBlockBorrowed declarations,
                    uint32_t index, nsAString* result)
 SERVO_BINDING_FUNC(Servo_DeclarationBlock_GetPropertyValue, void,
                    RawServoDeclarationBlockBorrowed declarations,
                    const nsACString* property, nsAString* value)
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -577,35 +577,48 @@ impl PropertyDeclarationBlock {
     }
 
     /// Take a declaration block known to contain a single property and serialize it.
     pub fn single_value_to_css<W>(
         &self,
         property: &PropertyId,
         dest: &mut W,
         computed_values: Option<&ComputedValues>,
+        custom_properties_block: Option<&PropertyDeclarationBlock>,
     ) -> fmt::Result
     where
         W: fmt::Write,
     {
         match property.as_shorthand() {
             Err(_longhand_or_custom) => {
                 if self.declarations.len() == 1 {
                     let declaration = &self.declarations[0];
-                    // If we have a longhand declaration with variables, those variables will be
-                    // stored as unparsed values. As a temporary measure to produce sensible results
-                    // in Gecko's getKeyframes() implementation for CSS animations, if
-                    // |computed_values| is supplied, we use it to expand such variable
-                    // declarations. This will be fixed properly in Gecko bug 1391537.
+                    let custom_properties = if let Some(cv) = computed_values {
+                        // If there are extra custom properties for this declaration block,
+                        // factor them into.
+                        if let Some(block) = custom_properties_block {
+                            block.custom_properties_with_inherited(&cv.custom_properties())
+                        } else {
+                            cv.custom_properties()
+                        }
+                    } else {
+                        None
+                    };
+
                     match (declaration, computed_values) {
+                        // If we have a longhand declaration with variables, those variables will be
+                        // stored as unparsed values. As a temporary measure to produce sensible results
+                        // in Gecko's getKeyframes() implementation for CSS animations, if
+                        // |computed_values| is supplied, we use it to expand such variable
+                        // declarations. This will be fixed properly in Gecko bug 1391537.
                         (&PropertyDeclaration::WithVariables(id, ref unparsed),
-                         Some(ref computed_values)) => unparsed
+                         Some(ref _computed_values)) => unparsed
                             .substitute_variables(
                                 id,
-                                &computed_values.custom_properties(),
+                                &custom_properties,
                                 QuirksMode::NoQuirks,
                             )
                             .to_css(dest),
                         (ref d, _) => d.to_css(dest),
                     }
                 } else {
                     Err(fmt::Error)
                 }
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -64,16 +64,17 @@ use style::gecko_bindings::bindings::Raw
 use style::gecko_bindings::bindings::RawGeckoFontFaceRuleListBorrowedMut;
 use style::gecko_bindings::bindings::RawGeckoServoAnimationValueListBorrowed;
 use style::gecko_bindings::bindings::RawGeckoServoAnimationValueListBorrowedMut;
 use style::gecko_bindings::bindings::RawGeckoServoStyleRuleListBorrowedMut;
 use style::gecko_bindings::bindings::RawServoAnimationValueBorrowed;
 use style::gecko_bindings::bindings::RawServoAnimationValueMapBorrowedMut;
 use style::gecko_bindings::bindings::RawServoAnimationValueStrong;
 use style::gecko_bindings::bindings::RawServoAnimationValueTableBorrowed;
+use style::gecko_bindings::bindings::RawServoDeclarationBlockBorrowedOrNull;
 use style::gecko_bindings::bindings::RawServoStyleRuleBorrowed;
 use style::gecko_bindings::bindings::RawServoStyleSet;
 use style::gecko_bindings::bindings::ServoStyleContextBorrowedOrNull;
 use style::gecko_bindings::bindings::nsTArrayBorrowed_uintptr_t;
 use style::gecko_bindings::bindings::nsTimingFunctionBorrowed;
 use style::gecko_bindings::bindings::nsTimingFunctionBorrowedMut;
 use style::gecko_bindings::structs;
 use style::gecko_bindings::structs::{CSSPseudoElementType, CompositeOperation};
@@ -583,17 +584,17 @@ macro_rules! get_property_id_from_nscssp
 pub extern "C" fn Servo_AnimationValue_Serialize(value: RawServoAnimationValueBorrowed,
                                                  property: nsCSSPropertyID,
                                                  buffer: *mut nsAString)
 {
     let uncomputed_value = AnimationValue::as_arc(&value).uncompute();
     let mut string = String::new();
     let rv = PropertyDeclarationBlock::with_one(uncomputed_value, Importance::Normal)
         .single_value_to_css(&get_property_id_from_nscsspropertyid!(property, ()), &mut string,
-                             None);
+                             None, None /* No extra custom properties */);
     debug_assert!(rv.is_ok());
 
     let buffer = unsafe { buffer.as_mut().unwrap() };
     buffer.assign_utf8(&string);
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_Shorthand_AnimationValues_Serialize(shorthand_property: nsCSSPropertyID,
@@ -2276,27 +2277,36 @@ pub extern "C" fn Servo_DeclarationBlock
         decls.to_css(unsafe { result.as_mut().unwrap() }).unwrap();
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SerializeOneValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property_id: nsCSSPropertyID, buffer: *mut nsAString,
-    computed_values: ServoStyleContextBorrowedOrNull)
+    computed_values: ServoStyleContextBorrowedOrNull,
+    custom_properties: RawServoDeclarationBlockBorrowedOrNull)
 {
+
     let property_id = get_property_id_from_nscsspropertyid!(property_id, ());
-    read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| {
-        let mut string = String::new();
-        let rv = decls.single_value_to_css(&property_id, &mut string, computed_values);
-        debug_assert!(rv.is_ok());
-
-        let buffer = unsafe { buffer.as_mut().unwrap() };
-        buffer.assign_utf8(&string);
-    })
+
+    let global_style_data = &*GLOBAL_STYLE_DATA;
+    let guard = global_style_data.shared_lock.read();
+    let decls = Locked::<PropertyDeclarationBlock>::as_arc(&declarations).read_with(&guard);
+
+    let mut string = String::new();
+
+    let custom_properties = Locked::<PropertyDeclarationBlock>::arc_from_borrowed(&custom_properties);
+    let custom_properties = custom_properties.map(|block| block.read_with(&guard));
+    let rv = decls.single_value_to_css(
+        &property_id, &mut string, computed_values, custom_properties);
+    debug_assert!(rv.is_ok());
+
+    let buffer = unsafe { buffer.as_mut().unwrap() };
+    buffer.assign_utf8(&string);
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_SerializeFontValueForCanvas(
     declarations: RawServoDeclarationBlockBorrowed,
     buffer: *mut nsAString) {
     use style::properties::shorthands::font;