522643 - Setting transition duration during a transition results in strange behaviour

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Markus Stange [:mstange]]

STR:
 1. Load the testcase.
 2. Click the buttons in this order:
     duration 5s, width 400px, duration 0, width 0

Actual results:
The box grows all the way to 400px, and only when it's finished its width is set to zero.

Expected results:
Changing the width to 0 should be instantaneous.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[d]

This looks fixed to me, using the latest trunk build.

Comment 3

[Markus Stange [:mstange]]

It's fixed insofar as clicking the "width 0" button immediately causes a change of direction and doesn't complete the running transition first. But the transition back to zero width still takes time, even though transition-duration has been set to zero.

Comment 4

[d]

Oh, I see. That is indeed not correct behavior.

Comment 5

[Markus Stange [:mstange]]

Attachment: fixed testcase

I take that back. The only thing that has changed is that transition-duration no longer accepts the value '0' without a unit, which broke the testcase. With the fixed testcase nothing has changed.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

Oops, I managed to miss the bugmail for this bug when you filed it (but, luckily, I saw it today).

Here's a patch that fixes it.  I'm not entirely happy with it, though; I'd like to consolidate the new code block in StyleContextChanged a bit better with the one right above it, at least.

(And maybe I should also move the move-the-static-function-up and further-optimize-GetElementTransitions parts into separate patches, although I don't think it's that hard to follow...)

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch 1: move static function higher

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch 2: optimize GetElementTransitions

The next patch will make us call GetElementTransitions much more, so we should make it faster than a hashtable lookup in most cases.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch 3: the fix

The diff -w for the chunk that involves reindentation is:

@@ -487,5 +484,10 @@
   // 'transition-property'.
-  if (et && disp->mTransitions[0].GetProperty() !=
-            eCSSPropertyExtra_all_properties) {
+  // Also stop any transitions for properties that just changed (and are
+  // still in the set of properties to transition), but we didn't just
+  // start the transition because delay and duration are both zero.
+  if (et) {
+    PRBool checkProperties =
+      disp->mTransitions[0].GetProperty() != eCSSPropertyExtra_all_properties;
     nsCSSPropertySet allTransitionProperties;
+    if (checkProperties) {
     for (PRUint32 i = disp->mTransitionPropertyCount; i-- != 0; ) {
@@ -512,2 +514,3 @@
     }
+    }
 
@@ -516,2 +519,3 @@
     NS_ABORT_IF_FALSE(i != 0, "empty transitions list?");
+    nsStyleAnimation::Value currentValue;
     do {
@@ -519,3 +523,7 @@
       ElementPropertyTransition &pt = pts[i];
-      if (!allTransitionProperties.HasProperty(pt.mProperty)) {
+      if ((checkProperties &&
+           !allTransitionProperties.HasProperty(pt.mProperty)) ||
+          !TransExtractComputedValue(pt.mProperty, aNewStyleContext,
+                                     currentValue) ||
+          currentValue != pt.mEndValue) {
         pts.RemoveElementAt(i);

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 416851 [details] [diff] [review]
patch 2: optimize GetElementTransitions

r=bzbarsky

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 416853 [details] [diff] [review]
patch 3: the fix

>+++ b/layout/style/nsTransitionManager.cpp
>+      if ((checkProperties &&
>+           !allTransitionProperties.HasProperty(pt.mProperty)) ||
>+          !TransExtractComputedValue(pt.mProperty, aNewStyleContext,
>+                                     currentValue) ||
>+          currentValue != pt.mEndValue) {

This condition could use a comment explaining what it's really testing.

r=me with that.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/5bd926e01c24
http://hg.mozilla.org/mozilla-central/rev/2ecf535b1505
http://hg.mozilla.org/mozilla-central/rev/a189e7282d6a