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

Status: VERIFIED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#728

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:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/media/videocontrols.css#200

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?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 5

[YUKI "Piro" Hiroshi]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

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

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?

Comment 8

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

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.

Comment 9

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

Er, sorry, it should check that the endpoint of the transition is other than the current value.

Comment 10

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

http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/18822b8ab514/dont-cancel-transitions 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.

Comment 11

[YUKI "Piro" Hiroshi]

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

Oops...it 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).

Comment 12

[YUKI "Piro" Hiroshi]

Attachment: Mochitest version of attachment 502021

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.

Comment 13

[YUKI "Piro" Hiroshi]

-Ny the way
+By the way

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[YUKI "Piro" Hiroshi]

Attachment: Mochitest version of attachment 502021 (v2)

* All elements for the test are moved under pre#test.
 * Simply changes their "outline", instead of adding text contents.

Comment 16

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

Attachment: patch

Comment 17

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

Attachment: test

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 502246 [details] [diff] [review]
patch

It seems like that whole comment at the end there should be before the if condition.  r=me either way, though.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 502248 [details] [diff] [review]
test

r=me

Comment 20

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

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.

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/ea7bedcd069c
http://hg.mozilla.org/mozilla-central/rev/fe3f812af314

Comment 22

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

Marking fixed on the assumption that the reduced testcase represents the same thing as the original problem, which seems likely, although not certain.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This test may be failing intermittently, see
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295089458.1295093090.5028.gz

Comment 24

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

philor pointed me to the failure in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295104315.1295107956.17841.gz
as well.

I landed http://hg.mozilla.org/mozilla-central/rev/0d8c14c175f1 which I think is likely (though not certain) to fix these failures.

Comment 25

[YUKI "Piro" Hiroshi]

Verified on Mozilla/5.0 (Windows NT 6.0; rv:2.0b13pre) Gecko/20110225
Firefox/4.0b13pre

Thanks a lot!

Comment 26

[Hongli Lai]

It appears that this bug is not (fully?) fixed in Firefox 4.0. I have a test case here: http://izumi.plan99.net/firefox-transitionend-bug.tar.gz
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.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Hongli, could you please file a new bug?

Comment 28

[Hongli Lai]

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=652543