Bug 1461070: Skip starting other transitions based on specified, not started transitions. r?birtles
Needs a test and all that.
Also I wonder if it'd be worth to rewrite that transition-start / stop loop in
Rust code, and put it in the style crate.
It'd help Servo to fix its completely broken transitions code, WDYT?
MozReview-Commit-ID: 3D5elrj2Ypi
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -457,57 +457,68 @@ nsTransitionManager::DoUpdateTransitions
const ComputedStyle& aNewStyle)
{
MOZ_ASSERT(!aElementTransitions ||
aElementTransitions->mElement == aElement, "Element mismatch");
// Per http://lists.w3.org/Archives/Public/www-style/2009Aug/0109.html
// I'll consider only the transitions from the number of items in
// 'transition-property' on down, and later ones will override earlier
- // ones (tracked using |whichStarted|).
+ // ones (tracked using |propertiesChecked|).
bool startedAny = false;
- nsCSSPropertyIDSet whichStarted;
- for (uint32_t i = aDisp.mTransitionPropertyCount; i-- != 0; ) {
+ nsCSSPropertyIDSet propertiesChecked;
+ for (uint32_t i = aDisp.mTransitionPropertyCount; i--; ) {
+ // We're not going to look at any further transitions, so we can just avoid
+ // looking at this if we know it will not start any transition.
+ if (i == 0 && aDisp.GetTransitionCombinedDuration(i) <= 0.0f) {
+ continue;
+ }
+
// Check the combined duration (combination of delay and duration)
// first, since it defaults to zero, which means we can ignore the
// transition.
- if (aDisp.GetTransitionCombinedDuration(i) > 0.0f) {
- // We might have something to transition. See if any of the
- // properties in question changed and are animatable.
- // FIXME: Would be good to find a way to share code between this
- // interpretation of transition-property and the one below.
- nsCSSPropertyID property = aDisp.GetTransitionProperty(i);
- if (property == eCSSPropertyExtra_no_properties ||
- property == eCSSPropertyExtra_variable ||
- property == eCSSProperty_UNKNOWN) {
- // Nothing to do, but need to exclude this from cases below.
- } else if (property == eCSSPropertyExtra_all_properties) {
- for (nsCSSPropertyID p = nsCSSPropertyID(0);
- p < eCSSProperty_COUNT_no_shorthands;
- p = nsCSSPropertyID(p + 1)) {
+ nsCSSPropertyID property = aDisp.GetTransitionProperty(i);
+ if (property == eCSSPropertyExtra_no_properties ||
+ property == eCSSPropertyExtra_variable ||
+ property == eCSSProperty_UNKNOWN) {
+ // Nothing to do.
+ continue;
+ }
+ // We might have something to transition. See if any of the
+ // properties in question changed and are animatable.
+ // FIXME: Would be good to find a way to share code between this
+ // interpretation of transition-property and the one below.
+ // FIXME(emilio): This should probably just use the "all" shorthand id, and
+ // we should probably remove eCSSPropertyExtra_all_properties.
+ if (property == eCSSPropertyExtra_all_properties) {
+ for (nsCSSPropertyID p = nsCSSPropertyID(0);
+ p < eCSSProperty_COUNT_no_shorthands;
+ p = nsCSSPropertyID(p + 1)) {
+ startedAny |=
ConsiderInitiatingTransition(p, aDisp, i, aElement, aPseudoType,
aElementTransitions,
aOldStyle, aNewStyle,
- &startedAny, &whichStarted);
- }
- } else if (nsCSSProps::IsShorthand(property)) {
- CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(subprop, property,
- CSSEnabledState::eForAllContent)
- {
+ propertiesChecked);
+ }
+ } else if (nsCSSProps::IsShorthand(property)) {
+ CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(subprop, property,
+ CSSEnabledState::eForAllContent)
+ {
+ startedAny |=
ConsiderInitiatingTransition(*subprop, aDisp, i, aElement, aPseudoType,
aElementTransitions,
aOldStyle, aNewStyle,
- &startedAny, &whichStarted);
- }
- } else {
+ propertiesChecked);
+ }
+ } else {
+ startedAny |=
ConsiderInitiatingTransition(property, aDisp, i, aElement, aPseudoType,
aElementTransitions,
aOldStyle, aNewStyle,
- &startedAny, &whichStarted);
- }
+ propertiesChecked);
}
}
// Stop any transitions for properties that are no longer in
// 'transition-property', including finished transitions.
// Also stop any transitions (and remove any finished transitions)
// for properties that just changed (and are still in the set of
// properties to transition), but for which we didn't just start the
@@ -626,52 +637,65 @@ GetTransitionKeyframes(nsCSSPropertyID a
}
static bool
IsTransitionable(nsCSSPropertyID aProperty)
{
return Servo_Property_IsTransitionable(aProperty);
}
-void
+bool
nsTransitionManager::ConsiderInitiatingTransition(
nsCSSPropertyID aProperty,
const nsStyleDisplay& aStyleDisplay,
uint32_t transitionIdx,
dom::Element* aElement,
CSSPseudoElementType aPseudoType,
CSSTransitionCollection*& aElementTransitions,
const ComputedStyle& aOldStyle,
const ComputedStyle& aNewStyle,
- bool* aStartedAny,
- nsCSSPropertyIDSet* aWhichStarted)
+ nsCSSPropertyIDSet& propertiesChecked)
{
// 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");
+ if (propertiesChecked.HasProperty(aProperty)) {
+ return false;
+ }
+
+ // A later item in transition-property already specified 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 .
+ propertiesChecked.AddProperty(aProperty);
+
// 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, CSSEnabledState::eForAllContent)) {
- 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;
+ return false;
}
if (!IsTransitionable(aProperty)) {
- return;
+ return false;
+ }
+
+ float delay = aStyleDisplay.GetTransitionDelay(transitionIdx);
+
+ // The spec says a negative duration is treated as zero.
+ float duration =
+ std::max(aStyleDisplay.GetTransitionDuration(transitionIdx), 0.0f);
+
+ // The combined duration of this transition is 0 or less, so don't start a
+ // transition.
+ if (delay + duration <= 0.0f) {
+ return false;
}
dom::DocumentTimeline* timeline = aElement->OwnerDoc()->Timeline();
AnimationValue startValue, endValue;
bool haveValues =
ExtractNonDiscreteComputedValue(aProperty, aOldStyle, startValue) &&
ExtractNonDiscreteComputedValue(aProperty, aNewStyle, endValue);
@@ -712,17 +736,17 @@ nsTransitionManager::ConsiderInitiatingT
//
// Likewise, if we got a style change that changed the value to the
// endpoint of our finished transition, we also don't want to start
// a new transition for the reasons described in
// https://lists.w3.org/Archives/Public/www-style/2015Jan/0444.html .
if (haveCurrentTransition && haveValues &&
aElementTransitions->mAnimations[currentIndex]->ToValue() == endValue) {
// GetAnimationRule already called RestyleForAnimation.
- return;
+ return false;
}
if (!shouldAnimate) {
if (haveCurrentTransition) {
// We're in the middle of a transition, and just got a non-transition
// style change to something that we can't animate. This might happen
// because we got a non-transition style change changing to the current
// in-progress value (which is particularly easy to cause when we're
@@ -740,27 +764,21 @@ nsTransitionManager::ConsiderInitiatingT
if (animations.IsEmpty()) {
aElementTransitions->Destroy();
// |aElementTransitions| is now a dangling pointer!
aElementTransitions = nullptr;
}
// GetAnimationRule already called RestyleForAnimation.
}
- return;
+ return false;
}
const nsTimingFunction &tf =
aStyleDisplay.GetTransitionTimingFunction(transitionIdx);
- float delay = aStyleDisplay.GetTransitionDelay(transitionIdx);
- float duration = aStyleDisplay.GetTransitionDuration(transitionIdx);
- if (duration < 0.0) {
- // The spec says a negative duration is treated as zero.
- duration = 0.0;
- }
AnimationValue startForReversingTest = startValue;
double reversePortion = 1.0;
// If the new transition reverses an existing one, we'll need to
// handle the timing differently.
// FIXME: Move mStartForReversingTest, mReversePortion to CSSTransition,
// and set the timing function on transitions as an effect-level
@@ -834,17 +852,17 @@ nsTransitionManager::ConsiderInitiatingT
if (!aElementTransitions) {
bool createdCollection = false;
aElementTransitions =
CSSTransitionCollection::GetOrCreateAnimationCollection(
aElement, aPseudoType, &createdCollection);
if (!aElementTransitions) {
MOZ_ASSERT(!createdCollection, "outparam should agree with return value");
NS_WARNING("allocating collection failed");
- return;
+ return false;
}
if (createdCollection) {
AddElementCollection(aElementTransitions);
}
}
OwningCSSTransitionPtrArray& animations = aElementTransitions->mAnimations;
@@ -881,21 +899,20 @@ nsTransitionManager::ConsiderInitiatingT
);
}
animations[currentIndex]->CancelFromStyle();
oldPT = nullptr; // Clear pointer so it doesn't dangle
animations[currentIndex] = animation;
} else {
if (!animations.AppendElement(animation)) {
NS_WARNING("out of memory");
- return;
+ return false;
}
}
EffectSet* effectSet = EffectSet::GetEffectSet(aElement, aPseudoType);
if (effectSet) {
effectSet->UpdateAnimationGeneration(mPresContext);
}
- *aStartedAny = true;
- aWhichStarted->AddProperty(aProperty);
+ return true;
}
--- a/layout/style/nsTransitionManager.h
+++ b/layout/style/nsTransitionManager.h
@@ -334,22 +334,21 @@ protected:
// could be a nullptr if we don't have any transitions.
bool DoUpdateTransitions(const nsStyleDisplay& aDisp,
mozilla::dom::Element* aElement,
mozilla::CSSPseudoElementType aPseudoType,
CSSTransitionCollection*& aElementTransitions,
const mozilla::ComputedStyle& aOldStyle,
const mozilla::ComputedStyle& aNewStyle);
- void ConsiderInitiatingTransition(nsCSSPropertyID aProperty,
+ // Returns whether the transition actually started.
+ bool ConsiderInitiatingTransition(nsCSSPropertyID aProperty,
const nsStyleDisplay& aStyleDisplay,
uint32_t transitionIdx,
mozilla::dom::Element* aElement,
mozilla::CSSPseudoElementType aPseudoType,
CSSTransitionCollection*& aElementTransitions,
const mozilla::ComputedStyle& aOldStyle,
const mozilla::ComputedStyle& aNewStyle,
- bool* aStartedAny,
- nsCSSPropertyIDSet* aWhichStarted);
-
+ nsCSSPropertyIDSet& propertiesChecked);
};
#endif /* !defined(nsTransitionManager_h_) */
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -320573,16 +320573,22 @@
]
],
"css/css-transitions/transitions-animatable-properties-01.html": [
[
"/css/css-transitions/transitions-animatable-properties-01.html",
{}
]
],
+ "css/css-transitions/zero-duration-multiple-transition.html": [
+ [
+ "/css/css-transitions/zero-duration-multiple-transition.html",
+ {}
+ ]
+ ],
"css/css-typed-om/CSSMatrixComponent-DOMMatrix-mutable.html": [
[
"/css/css-typed-om/CSSMatrixComponent-DOMMatrix-mutable.html",
{}
]
],
"css/css-typed-om/declared-styleMap-accepts-inherit.html": [
[
@@ -516325,17 +516331,17 @@
"04a6a67837127ba47e757e5cc3a9ff91c1e01d9a",
"reftest"
],
"css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-028.html": [
"cafd680e210a6d677bde00ae4c5f9263c1e7b48e",
"reftest"
],
"css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-029.html": [
- "67161f08b8aec3fe9bd94e14cbe865dd4c9d3664",
+ "763518bc498fa2560e91eb6dfd13803a5dab2e17",
"reftest"
],
"css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-030.html": [
"e5b82f3b41a856a34152af46c9cc659202c61843",
"reftest"
],
"css/css-shapes/shape-outside/supported-shapes/polygon/reference/shape-outside-polygon-007-ref.html": [
"3ee945e85e3c2a9b502e93c04ed53966551e7df1",
@@ -529972,16 +529978,20 @@
"css/css-transitions/transitioncancel-001.html": [
"4db1d84665ccf75d993d877eb08574b1fa7d0203",
"testharness"
],
"css/css-transitions/transitions-animatable-properties-01.html": [
"538b95863c061da60e95c1a61ef9dc93da007aa4",
"testharness"
],
+ "css/css-transitions/zero-duration-multiple-transition.html": [
+ "7db6bc8641a6a2eb5f1ee1fdbc9b6215a0462bbc",
+ "testharness"
+ ],
"css/css-typed-om/CSSMatrixComponent-DOMMatrix-mutable.html": [
"f6056e2480829c7aa9885673d332496faf7777b5",
"testharness"
],
"css/css-typed-om/OWNERS": [
"f5f0861ac3382b3b12008133c1334f812a5a2caa",
"support"
],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-transitions/zero-duration-multiple-transition.html
@@ -0,0 +1,29 @@
+<!doctype html>
+<meta charset="utf-8">
+<link rel="help" href="https://drafts.csswg.org/css-transitions/#starting">
+<link rel="author" title="Emilio Cobos Álvarez" href="emilio@crisal.io">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+ #t {
+ color: red;
+ transition-property: color, color;
+ transition-duration: 1s, 0s;
+ }
+</style>
+<div id="t"></div>
+<script>
+test(function() {
+ let div = document.getElementById("t");
+ assert_equals(getComputedStyle(div).color, "rgb(255, 0, 0)");
+
+ div.style.color = "green";
+ assert_equals(
+ div.getAnimations().length,
+ 0,
+ "No transition should've started"
+ );
+
+ assert_equals(getComputedStyle(div).color, "rgb(0, 128, 0)");
+}, "transition-duration of 0 prevents earlier transitions with the same property from starting.");
+</script>