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
--- 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>