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

VERIFIED FIXED in mozilla2.0b10

Status

()

Core
CSS Parsing and Computation
P2
normal
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: cpearce, Assigned: dbaron)

Tracking

Trunk
mozilla2.0b10
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
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.
(Reporter)

Updated

7 years ago
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
(Reporter)

Comment 3

7 years ago
(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?
blocking2.0: ? → final+
(Reporter)

Comment 4

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

Comment 5

7 years ago
Created attachment 502021 [details]
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....

Updated

7 years ago
Whiteboard: [hardblocker]
(Assignee)

Comment 7

7 years ago
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?
(Assignee)

Comment 8

7 years ago
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.
(Assignee)

Comment 9

7 years ago
Er, sorry, it should check that the endpoint of the transition is other than the current value.
(Assignee)

Comment 10

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

7 years ago
(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

7 years ago
Created attachment 502192 [details]
Mochitest version of attachment 502021 [details]

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

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

Comment 15

7 years ago
Created attachment 502213 [details]
Mochitest version of attachment 502021 [details] (v2)

 * 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
(Assignee)

Comment 16

7 years ago
Created attachment 502246 [details] [diff] [review]
patch
Attachment #502246 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

7 years ago
Created attachment 502248 [details] [diff] [review]
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.
Attachment #502248 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Whiteboard: [hardblocker] → [hardblocker][needs review]
(Assignee)

Updated

7 years ago
OS: Windows 7 → All
Priority: -- → P2
Hardware: x86 → All
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.
Attachment #502246 - Flags: review?(bzbarsky) → review+
Comment on attachment 502248 [details] [diff] [review]
test

r=me
Attachment #502248 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

7 years ago
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.
(Assignee)

Comment 21

7 years ago
http://hg.mozilla.org/mozilla-central/rev/ea7bedcd069c
http://hg.mozilla.org/mozilla-central/rev/fe3f812af314
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][needs review] → [hardblocker]
Target Milestone: --- → mozilla2.0b10
(Assignee)

Comment 22

7 years ago
Marking fixed on the assumption that the reduced testcase represents the same thing as the original problem, which seems likely, although not certain.
(Assignee)

Updated

7 years ago
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
(Assignee)

Comment 24

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

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

Thanks a lot!
Status: RESOLVED → VERIFIED

Comment 26

7 years ago
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.
Hongli, could you please file a new bug?
You need to log in before you can comment on or make changes to this bug.