Bug 1265611 - Don't trigger transitions for properties that are disabled draft
authorBrian Birtles <birtles@gmail.com>
Thu, 21 Apr 2016 08:46:34 +0900
changeset 354493 87932afdb6b34ed8a262d463e884dc810bc35296
parent 354492 efe6b7005d4f055ee909a804b5c6b2cb10074926
child 354494 036d56f4da74b77a1af1c65f2a1688f631939c12
push id16093
push userbbirtles@mozilla.com
push dateThu, 21 Apr 2016 00:32:40 +0000
bugs1265611
milestone48.0a1
Bug 1265611 - Don't trigger transitions for properties that are disabled Currently if we have transition-property: 'all' and trigger a transition on the 'color' property we will end up generating a transition on -webkit-text-fill-color even if that property is disabled. However, when we later call StyleAnimationValue::ToValue() in nsTransitionManager::UpdateTransitions() to see if there are any transitions we need to cancel, the comparison for currentValue != anim->ToValue() will pass (since, as of the first patch in this patch series, ToValue() returns a null value) so we end up cancelling the transition as soon as we create it). Nevertheless, we will still trigger the warning introduced in the first patch in this series when we call ToValue(). This patch stops us from creating transitions in the first place (and hence triggering the warning). It also removes the code that suppresses transition events for transitions on disabled properties since we should no longer be generating such transitions in the first place (unless the pref is switched while the transition is in motion which is probably not worth worrying about). Note that we only test if the property is enabled for all content. This is consistent with what we do throughout animation code including the existing code in nsTransitionManager which iterates through shorthand sub-properties using CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES with the flag nsCSSProps::eEnabledForAllContent. The test case in this patch doesn't actually fail without this change, all it does it trigger the warning in StyleAnimationValue::ToValue() introduced in the first patch in this series. It's still a useful regression test however, particularly if we later upgrade the warning in StyleAnimationValue::ToValue() to a fatal assertion. MozReview-Commit-ID: H9swDKLyiOf
layout/style/nsCSSProps.h
layout/style/nsTransitionManager.cpp
layout/style/test/mochitest.ini
layout/style/test/test_transitions_with_disabled_properties.html
--- a/layout/style/nsCSSProps.h
+++ b/layout/style/nsCSSProps.h
@@ -221,16 +221,18 @@ static_assert((CSS_PROPERTY_PARSE_PROPER
 //   pref, this property is usable everywhere.
 // * If any of the flags is set, this property is always enabled in the
 //   specific contexts regardless of the value of the pref. If there is
 //   no pref for this property at all in this case, it is an internal-
 //   only property, which cannot be used anywhere else, and should be
 //   wrapped in "#ifndef CSS_PROP_LIST_EXCLUDE_INTERNAL".
 // Note that, these flags have no effect on the use of aliases of this
 // property.
+// Furthermore, for the purposes of animation (including triggering
+// transitions) these flags are ignored.
 #define CSS_PROPERTY_ENABLED_MASK                 (3<<22)
 #define CSS_PROPERTY_ENABLED_IN_UA_SHEETS         (1<<22)
 #define CSS_PROPERTY_ENABLED_IN_CHROME            (1<<23)
 #define CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME \
   (CSS_PROPERTY_ENABLED_IN_UA_SHEETS | CSS_PROPERTY_ENABLED_IN_CHROME)
 
 // This property's unitless values are pixels.
 #define CSS_PROPERTY_NUMBERS_ARE_PIXELS           (1<<24)
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -128,34 +128,24 @@ CSSTransition::QueueEvents()
     return;
   }
 
   dom::Element* owningElement;
   CSSPseudoElementType owningPseudoType;
   mOwningElement.GetElement(owningElement, owningPseudoType);
   MOZ_ASSERT(owningElement, "Owning element should be set");
 
-  // Do not queue any event for disabled properties. This could happen
-  // if the property has a default value which derives value from other
-  // property, e.g. color.
-  nsCSSProperty property = TransitionProperty();
-  if (!nsCSSProps::IsEnabled(property, nsCSSProps::eEnabledForAllContent) &&
-      (!nsContentUtils::IsSystemPrincipal(owningElement->NodePrincipal()) ||
-       !nsCSSProps::IsEnabled(property, nsCSSProps::eEnabledInChrome))) {
-    return;
-  }
-
   nsPresContext* presContext = mOwningElement.GetRenderedPresContext();
   if (!presContext) {
     return;
   }
 
   nsTransitionManager* manager = presContext->TransitionManager();
   manager->QueueEvent(TransitionEventInfo(owningElement, owningPseudoType,
-                                          property,
+                                          TransitionProperty(),
                                           mEffect->GetComputedTiming()
                                             .mDuration,
                                           AnimationTimeToTimeStamp(EffectEnd()),
                                           this));
 }
 
 void
 CSSTransition::Tick()
@@ -532,16 +522,23 @@ nsTransitionManager::ConsiderStartingTra
   nsCSSPropertySet* aWhichStarted)
 {
   // IsShorthand itself will assert if aProperty is not a property.
   MOZ_ASSERT(!nsCSSProps::IsShorthand(aProperty),
              "property out of range");
   NS_ASSERTION(!aElementTransitions ||
                aElementTransitions->mElement == aElement, "Element mismatch");
 
+  // Ignore disabled properties. We can arrive here if the transition-property
+  // is 'all' and the disabled property has a default value which derives value
+  // from another property, e.g. color.
+  if (!nsCSSProps::IsEnabled(aProperty, nsCSSProps::eEnabledForAllContent)) {
+    return;
+  }
+
   if (aWhichStarted->HasProperty(aProperty)) {
     // A later item in transition-property already started a
     // transition for this property, so we ignore this one.
     // See comment above and
     // http://lists.w3.org/Archives/Public/www-style/2009Aug/0109.html .
     return;
   }
 
--- a/layout/style/test/mochitest.ini
+++ b/layout/style/test/mochitest.ini
@@ -266,16 +266,17 @@ skip-if = (toolkit == 'gonk' && debug) |
 [test_transitions.html]
 skip-if = (android_version == '18' && debug) # bug 1159532
 [test_transitions_bug537151.html]
 [test_transitions_dynamic_changes.html]
 [test_transitions_per_property.html]
 skip-if = buildapp == 'b2g' || toolkit == 'android' #bug 775227 # b2g(times out, needs more time + various failures) b2g-debug(times out, needs more time + various failures) b2g-desktop(times out, needs more time + various failures)
 [test_transitions_step_functions.html]
 [test_transitions_with_displaynone.html]
+[test_transitions_with_disabled_properties.html]
 [test_unclosed_parentheses.html]
 [test_unicode_range_loading.html]
 support-files = ../../reftests/fonts/markA.woff ../../reftests/fonts/markB.woff ../../reftests/fonts/markC.woff ../../reftests/fonts/markD.woff
 [test_units_angle.html]
 [test_units_frequency.html]
 [test_units_length.html]
 [test_units_time.html]
 [test_unprefixing_service.html]
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_transitions_with_disabled_properties.html
@@ -0,0 +1,45 @@
+<!doctype html>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1265611
+-->
+<head>
+  <meta charset=utf-8>
+  <title>Test for bug 1265611</title>
+  <script type="application/javascript"
+    src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <style>
+  #display {
+    transition: all 0.01s;
+  }
+  </style>
+</head>
+<body>
+<a target="_blank"
+  href="https://bugzilla.mozilla.org/show_bug.cgi?id=1265611">Mozilla Bug
+  1265611</a>
+
+<div id="display"></div>
+<pre id="test">
+<script>
+SimpleTest.waitForExplicitFinish();
+
+SpecialPowers.pushPrefEnv({'set': [['layout.css.prefixes.webkit', false],
+                                   ['dom.animations-api.core.enabled', true]] },
+  () => {
+    var display = document.getElementById('display');
+    display.style.color = 'green';
+
+    var transitionedProperties =
+      display.getAnimations().map(transition => transition.transitionProperty);
+
+    ok(!transitionedProperties.includes('-webkit-text-fill-color'),
+       'We should not fire transitions for properties disabled by prefs');
+    SimpleTest.finish();
+  }
+);
+</script>
+</pre>
+</body>
+</html>