Closed Bug 613888 Opened 14 years ago Closed 14 years ago

transitionend doesn't fire when an unrelated style change happens while the transition is close enough to being done that it rounds to its final value


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




Tracking Status
blocking2.0 --- final+


(Reporter: cpearce, Assigned: dbaron)



(Whiteboard: [hardblocker])


(4 files, 1 obsolete file)

Sometimes we don't send a transitionend event when a CSS transition ends. This is particularly the case in XUL, where we have the following bugs:

Bug 608589 - Tab fails to close, waiting for the closing animation's transitionend event
Bug 613312 - Throbber shows until user starts playback on preload=metadata videos

In both these bugs, UI actions are only happen when we receive a transitionend event, and we never receive that event.

I've been trying to debug bug 613312, and it looks like the transition isn't even starting.
Probably worth noting: I couldn't reproduce this in an HTML only test case, perhaps it only happend in xul/xbl?

The video controls throbber gets stuck on in the URL listed in bug 613312 in about 1 in every 5 loads I do, so that's the most reliable way I know to reproduce this bug so far.
blocking2.0: --- → ?
Note that whether a transition _starts_ depends on the exact order of various restyles, what scripts run when, etc.

What do the style changes look like that are expected to trigger a transition here?  Is there anything like async XBL loads involved?
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
(In reply to comment #2)
> What do the style changes look like that are expected to trigger a transition
> here?

We set an attribute "fadeout" on a xul element with class name statusOverlay in the video controls here:

Which means it matches the styles:

.statusOverlay:not([immediate]) {
  -moz-transition-property: opacity;
  -moz-transition-duration: 300ms;
  -moz-transition-delay: 750ms;
.statusOverlay[fadeout] {
  opacity: 0;

From here:

Basically this should cause the grey buffering/throbber box which appears over video elements while they're loading to fade out. Sometimes the fade out doesn't seem to start.

> Is there anything like async XBL loads involved?

I'm not sure! How do we tell?
I'm no longer convinced bug 613312 was caused by a transitionend erroneously not firing. It looks like the transitionend wasn't being fired because the transition was being reversed before it's transition-delay had expired, so no transitions were starting at all, and hence no transitionend event was dispatched. As far as I can tell, this is the intended behaviour. 

It would be handy if there was a transitionaborted event!

At first blush it doesn't appear that bug 608589 is caused by the same incorrect usage of transitions, so I'll leave this bug open.
No longer blocks: 613312
Attached file HTML testcase
This is a testcase for HTML. There are three tests in the document, and the second always fails on my system.
Mozilla/5.0 (Windows NT 6.0; rv:2.0b9pre) Gecko/20110107 Firefox/4.0b9pre
Thanks for the testcase!  So the issue there is that the background-color event does fire for that middle <pre>, but the color one does not....
Whiteboard: [hardblocker]
Are these lines in the test:

	tooFast2.textContent += '\n  color: '+getColor(tooFast);

		tooFast2.textContent += '\n  color: '+getColor(tooFast);

intentionally crossing data between tooFast and tooFast2, or was that a typo?
What happens in attachment 502021 [details] is the following:

The transitions are reversed, and because they're using different curves, the duration of the reverse transitions for the two properties is very slightly different.

At the time the first of these (background-color) ends, the color transition is almost done:  close enough that interpolation of its current position is the same as the final value.  We then do a style change during the transitionend handling on background-color, and hit the bit in ConsiderStartingTransition with the comment:
      // We're in the middle of a transition, but just got a
      // non-transition style change changing to exactly the
      // current in-progress value.   (This is quite easy to cause
      // using 'transition-delay'.)

I think the right fix here is probably to make the conditions around that code also check that interpolation of the transition we're about to remove yields a value other than the current value.
Er, sorry, it should check that the endpoint of the transition is other than the current value. fixes attachment 502021 [details], and also passes existing transitions mochitests (and all the others in layout/style.

I still need to convert the attachment to an automated test.
(In reply to comment #7)
> Are these lines in the test:
>     tooFast2.textContent += '\n  color: '+getColor(tooFast);
>         tooFast2.textContent += '\n  color: '+getColor(tooFast);
> intentionally crossing data between tooFast and tooFast2, or was that a typo? is a typo. I wanted to check "is the animation surely processed?"
I guessed that this failure was caused because the animation was canceled when the color of tooFast/tooFast2 was just same to the initial color rgb(0,0,0).
Attached file Mochitest version of attachment 502021 (obsolete) —
Mochitest-based simplified version.

Ny the way, this test must use the legacy parser, because this test always successes anyway when the HTML5 parser is available. So I commented out the "<!DOCTYPE HTML>" line.
-Ny the way
+By the way
The HTML5 parser is used no matter what the doctype.  So all you're really doing is putting the page in quirks mode.  And the reason that matters is that quirk.css has this rule in it:

  body > pre:-moz-only-whitespace:-moz-first-node { margin-bottom: 0; }

which will cause a <pre> whose child list changes (e.g. because you added some text to it) to be restyled.  That's the "then do a style change during the transitionend handling on background-color" bit from comment 8.

At least that's my hypothesis; easy enough to test by putting the <pre> inside a <div> in the testcase and seeing whether the bug goes away as a result.  If so, then you can probably leave the testcase in standards mode, and just change that transitionend handler to do an explicit restyle (change the border style say, or the opacity, or something) instead of implicitly doing one by changing the contents of the <pre>.
* All elements for the test are moved under pre#test.
 * Simply changes their "outline", instead of adding text contents.
Attachment #502192 - Attachment is obsolete: true
Attached patch testSplinter Review
Thanks for reducing this testcase and for turning it into a mochitest.  It made it possible to find the bug.

I made a few tweaks to the mochitest file you attached; mainly reordering the parts of the test so they're in the usual order (style in the head, then content, then script), and changing the test so that it completes when all the transitionend events fire (so it finishes faster).

That means that without this patch, it fails by timing out, but that's fine; it's a perfectly good failure mode.  With the patch, it passes.
Attachment #502248 - Flags: review?(bzbarsky)
Whiteboard: [hardblocker] → [hardblocker][needs review]
OS: Windows 7 → All
Priority: -- → P2
Hardware: x86 → All
Comment on attachment 502246 [details] [diff] [review]

It seems like that whole comment at the end there should be before the if condition.  r=me either way, though.
Attachment #502246 - Flags: review?(bzbarsky) → review+
Comment on attachment 502248 [details] [diff] [review]

Attachment #502248 - Flags: review?(bzbarsky) → review+
Well, it's just inside the if, since the first paragraph makes more sense that way.  It reads better in non-diff format, I think.
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][needs review] → [hardblocker]
Target Milestone: --- → mozilla2.0b10
Marking fixed on the assumption that the reduced testcase represents the same thing as the original problem, which seems likely, although not certain.
Summary: Sometimes transitionend doesn't fire → transitionend doesn't fire when an unrelated style change happens while the transition is close enough to being done that it rounds to its final value
philor pointed me to the failure in:
as well.

I landed which I think is likely (though not certain) to fix these failures.
Verified on Mozilla/5.0 (Windows NT 6.0; rv:2.0b13pre) Gecko/20110225

Thanks a lot!
It appears that this bug is not (fully?) fixed in Firefox 4.0. I have a test case here:
This test case uses AJAX so it should be put on a web server.

This test case was an exercise for me in Spine.js and CSS transformations. The idea is that if you click on any of the navigation links on top, it'll load the target using Ajax and show a smooth transition animation ala iPhone before displaying the new content.

If you open index.html and click on "book.txt", MainController.showNewContent() will be called which executes two animations using $.fn.transition(). One moves the old content pane to the left until it's no longer visible, the other one moves the new content pane to the center, to the place where the old content pane was. This test case works perfectly on Chrome, Safari and Opera, but on Firefox 4.0 the first click doesn't work properly because the transitionend event isn't fired. The second click works, but the third one doesn't, etc.
Hongli, could you please file a new bug?
You need to log in before you can comment on or make changes to this bug.