Bug 1301305 - Expand the set of geometric properties to include margin and padding properties; r=hiro
authorBrian Birtles <birtles@gmail.com>
Fri, 02 Dec 2016 09:00:05 +0900
changeset 325328 9a3bdee53cec09705fdf604c0c0c93364eb16c2a
parent 325327 aba14a8202e05d4c62b478ff9c61f0ac02dc8438
child 325329 ece7cb85c38122ec03a8870e2f0054ece2024fa0
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewershiro
bugs1301305
milestone53.0a1
Bug 1301305 - Expand the set of geometric properties to include margin and padding properties; r=hiro If margin or padding is being animated then we should synchronize with transform animations. Originally I included the border-*-width properties in this set. However I removed them because: 1. Generally animations of border-width are more subtle and it won't be noticeable if they are not synchronized with transform animations. 2. If authors animate the border shorthand (e.g. border: 1px blue -> 1px black) we will end up interpolating each of the longhands (including the widths despite there being no change) and yet such an animation does not really need to be synchronized with transform animations. Until we add code to workaround that it seems best to ignore border properties. I have verified that the tests added in this patch fail without the code changes in this patch. MozReview-Commit-ID: AJiDAvTpFuN
dom/animation/KeyframeEffectReadOnly.cpp
dom/animation/test/chrome/test_animation_performance_warning.html
--- a/dom/animation/KeyframeEffectReadOnly.cpp
+++ b/dom/animation/KeyframeEffectReadOnly.cpp
@@ -1250,20 +1250,31 @@ KeyframeEffectReadOnly::GetPresContext()
   }
   return shell->GetPresContext();
 }
 
 /* static */ bool
 KeyframeEffectReadOnly::IsGeometricProperty(
   const nsCSSPropertyID aProperty)
 {
+  MOZ_ASSERT(!nsCSSProps::IsShorthand(aProperty),
+             "Property should be a longhand property");
+
   switch (aProperty) {
     case eCSSProperty_bottom:
     case eCSSProperty_height:
     case eCSSProperty_left:
+    case eCSSProperty_margin_bottom:
+    case eCSSProperty_margin_left:
+    case eCSSProperty_margin_right:
+    case eCSSProperty_margin_top:
+    case eCSSProperty_padding_bottom:
+    case eCSSProperty_padding_left:
+    case eCSSProperty_padding_right:
+    case eCSSProperty_padding_top:
     case eCSSProperty_right:
     case eCSSProperty_top:
     case eCSSProperty_width:
       return true;
     default:
       return false;
   }
 }
--- a/dom/animation/test/chrome/test_animation_performance_warning.html
+++ b/dom/animation/test/chrome/test_animation_performance_warning.html
@@ -286,16 +286,54 @@ function testKeyframesWithGeometricPrope
         assert_animation_property_state_equals(
           animation.effect.getProperties(),
           subtest.expected.withoutGeometric);
       });
     }, 'An animation has: ' + subtest.desc);
   });
 }
 
+// Test that the expected set of geometric properties all block transform
+// animations.
+function testSetOfGeometricProperties() {
+  const geometricProperties = [
+    'width', 'height',
+    'top', 'right', 'bottom', 'left',
+    'margin-top', 'margin-right', 'margin-bottom', 'margin-left',
+    'padding-top', 'padding-right', 'padding-bottom', 'padding-left'
+  ];
+
+  geometricProperties.forEach(property => {
+    promise_test(function(t) {
+      const keyframes = {
+        [propertyToIDL(property)]: [ '100px', '200px' ],
+        transform: [ 'translate(0px)', 'translate(100px)' ]
+      };
+      var animation = addDivAndAnimate(t, { class: 'compositable' },
+                                       keyframes, 100 * MS_PER_SEC);
+
+      return animation.ready.then(function() {
+        assert_animation_property_state_equals(
+          animation.effect.getProperties(),
+          [
+            {
+              property,
+              runningOnCompositor: false
+            },
+            {
+              property: 'transform',
+              runningOnCompositor: false,
+              warning: 'CompositorAnimationWarningTransformWithGeometricProperties'
+            }
+          ]);
+      }, 'Transform animation should be run on the main thread');
+    }, `${property} is treated as a geometric property`);
+  });
+}
+
 // Performance warning tests that set and clear a style property.
 function testStyleChanges() {
   [
     {
       desc: 'preserve-3d transform',
       frames: {
         transform: ['translate(0px)', 'translate(100px)']
       },
@@ -850,16 +888,17 @@ function testSmallElements() {
 function start() {
   var bundleService = SpecialPowers.Cc['@mozilla.org/intl/stringbundle;1']
     .getService(SpecialPowers.Ci.nsIStringBundleService);
   gStringBundle = bundleService
     .createBundle("chrome://global/locale/layout_errors.properties");
 
   testBasicOperation();
   testKeyframesWithGeometricProperties();
+  testSetOfGeometricProperties();
   testStyleChanges();
   testIdChanges();
   testMultipleAnimations();
   testMultipleAnimationsWithGeometricKeyframes();
   testMultipleAnimationsWithGeometricAnimations();
   testSmallElements();
 
   promise_test(function(t) {