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
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla2.0b10
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: cpearce, Assigned: dbaron)
References
Details
(Whiteboard: [hardblocker])
Attachments
(4 files, 1 obsolete file)
2.55 KB,
text/html
|
Details | |
2.21 KB,
text/html
|
Details | |
2.75 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.76 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•14 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•14 years ago
|
blocking2.0: --- → ?
Comment 2•14 years ago
|
||
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•14 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+
Assignee: nobody → dbaron
Reporter | ||
Comment 4•14 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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
Whiteboard: [hardblocker]
Assignee | ||
Comment 7•14 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•14 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•14 years ago
|
||
Er, sorry, it should check that the endpoint of the transition is other than the current value.
Assignee | ||
Comment 10•14 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•14 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•14 years ago
|
||
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•14 years ago
|
||
-Ny the way
+By the way
Comment 14•14 years ago
|
||
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•14 years ago
|
||
* 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•14 years ago
|
||
Attachment #502246 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 17•14 years ago
|
||
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•14 years ago
|
Whiteboard: [hardblocker] → [hardblocker][needs review]
Assignee | ||
Updated•14 years ago
|
OS: Windows 7 → All
Priority: -- → P2
Hardware: x86 → All
Comment 18•14 years ago
|
||
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 19•14 years ago
|
||
Comment on attachment 502248 [details] [diff] [review]
test
r=me
Attachment #502248 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 20•14 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•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ea7bedcd069c
http://hg.mozilla.org/mozilla-central/rev/fe3f812af314
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][needs review] → [hardblocker]
Target Milestone: --- → mozilla2.0b10
Assignee | ||
Comment 22•14 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•14 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
This test may be failing intermittently, see
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295089458.1295093090.5028.gz
Assignee | ||
Comment 24•14 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•14 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•14 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.
Comment 27•14 years ago
|
||
Hongli, could you please file a new bug?
Comment 28•14 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•