We should implement the transitionend event as described in http://dev.w3.org/csswg/css3-transitions/#transition-events- I have a patch for the DOM side of this; it's relatively straightforward. The style system side is a little more interesting. Callers clearly want these events so that they can make changes to styles in response to the end of the transition; they may want to do this prior to the next update in order to avoid flashing. Since the transition ticks are off the refresh driver, which is itself off a timer, this should be safe. (And it's safe to run script off a flush, isn't it? I'm thinking it better be ok to run transitionend events delayed if we're suppressing the processing of transitions, too...) My thoughts on how to do this are: * make nsARefreshObserver reference counted (with AddRef and Release signatures matching nsISupports) * make nsRefreshDriver no longer a subobject of the pres context (and give it a back pointer that's cleared from the pres context's destructor) * make nsRefreshDriver and nsTransitionManager reference counted * make the refresh driver hold on to itself during its own timer callback, but check that its back-pointer to its pres context hasn't been cleared after each WillRefresh call it makes to an observer * make the refresh driver hold on to observers as it gives them a WillRefresh call * make the TransitionManager fire the events at the end of its own WillRefresh Does this sound reasonable? scary? Am I missing something? (I think we'll want to do similar things for other types of events that we'll also eventually want to be fired from the refresh driver, such as onresize events (see bug 457862 and bug 473805).)

Would be great to see the patch (even if it isn't fully ready yet). Seems like http://dev.w3.org/csswg/css3-transitions/#transition-events- doesn't say where the event is dispatched. I'd guess to the element which has the relevant CSS transition.

The (completely untested, since I haven't yet written any code to fire the events) patch for the event code side is: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/04e5bc1260fd/css-transitions-events

(In reply to comment #0) > * make the refresh driver hold on to itself during its own timer callback, but > check that its back-pointer to its pres context hasn't been cleared after each > WillRefresh call it makes to an observer bz says this is unneeded since the timer holds a reference on the stack (in addition to the one in its member variable) However, I realize an additional step is needed: we need to cancel the refresh driver's timer from the pres context's destructor so its timer doesn't keep it alive in a cycle.

And, actually, this sentence in the spec makes it reasonably clear when the event fires: In the case where a transition is removed before completion, such as if the transition-property is removed, then the event will not fire.

One other thing I wonder: maybe I should have made the nsTransitionEvent contain a PRUint32 for the property name (as nsCSSProperty enum, but without the type to avoid dragging nsCSSProperty into nsGUIEvent.h), and only convert to a string when making an nsDOMTransitionEvent. Do we only create the nsDOMTransitionEvent rarely, such that that would be worthwhile?

Comment on attachment 417234 [details] [diff] [review] patch 2: make refresh driver reference counted r=bzbarsky

Comment on attachment 417235 [details] [diff] [review] patch 3: make transition manager reference counted r=bzbarsky

Comment on attachment 417236 [details] [diff] [review] patch 4: make refresh driver hold reference to observers while notifying r=bzbarsky

Comment on attachment 417237 [details] [diff] [review] patch 5: add function for which CSS property computed values should use different name for the property r=bzbarsky, but what about the *-start-value and *-end-value properties?

Comment on attachment 417238 [details] [diff] [review] patch 6: dispatch transitionend events r=bzbarsky. You probably want to check with smaug on the event frequency thing.

(In reply to comment #16) > r=bzbarsky, but what about the *-start-value and *-end-value properties? They don't have computed values; the computed values always end up in *-left-value and *-right-value.

I just stuck in some printfs, and I think it's silly not to move the string conversion later; there doesn't seem to be any need to store DOM-initialized events in nsTransitionEvent (and thus no need for arbitrary strings), and nsDOMTransitionEvents are created only when there's someone actually listening to them. So I'll revise patches 1 and (slightly) 6 accordingly.

New versions of patches 6 and 7 are: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/9300d74724b8/transitionend-dispatch http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/9300d74724b8/transition-events-tests but the change to patch 6 trivial (just dropping NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(...)) around aProperty).

OK, one more tweak: TransitionEvent is probably a more likely name for the parameter to CreateEvent than TransitionEndEvent, given the interface name. Matching tests at http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/55200a75237b/transition-events-tests

Comment on attachment 417242 [details] [diff] [review] patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code >+ // FIXME: REVIEW: Is this the right EventNameType? >+ { &nsGkAtoms::ontransitionend, { NS_TRANSITION_END, EventNameType_HTMLXUL }}, So do we need to support ontransitionend event handler attributes? Where is that specified? Or is it just something which is implemented in other engines? If <element ontransitionend="..."> is supported, should also element.ontransitionend be supported? >+nsDOMTransitionEvent::nsDOMTransitionEvent(nsPresContext *aPresContext, >+ nsTransitionEvent *aEvent) >+ : nsDOMEvent(aPresContext, aEvent) >+{ >+ if (aEvent) { >+ NS_ABORT_IF_FALSE(aEvent->propertyName < PRUint32(eCSSProperty_COUNT), >+ "invalid aEvent->propertyName"); >+ CopyUTF8toUTF16(nsCSSProps::GetStringValue( >+ nsCSSProperty(aEvent->propertyName)), >+ mPropertyName); >+ mElapsedTime = aEvent->elapsedTime; >+ } else { >+ mElapsedTime = 0; >+ } >+} To keep this closer to what other events do, could you create nsTransitionEvent struct if one isn't passed as parameter.

(In reply to comment #22) > So do we need to support ontransitionend event handler attributes? > Where is that specified? Or is it just something which is implemented in other > engines? > If <element ontransitionend="..."> is supported, should also > element.ontransitionend > be supported? I think we do NOT need to support either of these. But I could be wrong. The spec doesn't say. > To keep this closer to what other events do, could you create > nsTransitionEvent struct if one isn't passed as parameter. That's actually rather hard given the approach I took; the DOM API allows arbitrary strings to be passed as property names, but I really don't want to do the conversion of property IDs to strings unless there's actually a listener that requires us to construct an nsDOMTransitionEvent. (I'd also note that nsDOMPageTransitionEvent doesn't do this either.) Why isn't it ok to just let nsDOMEvent::nsDOMEvent construct an nsEvent?

I landed patches 2 through 4 because bzbarsky wants to work on top of them, and they're independent of the rest: http://hg.mozilla.org/mozilla-central/rev/fa5326c011b8 http://hg.mozilla.org/mozilla-central/rev/8b22441911b0 http://hg.mozilla.org/mozilla-central/rev/cfa10b01b1f6

This patch: * reverts the performance-improving changes in comment 12 / comment 19 / comment 20 * creates the correct inner event per comment 22 * uses the fields of that inner event object instead of member variables * sets the correct EventNameType (I should have figured out what that was for earlier) per comment 22

(In reply to comment #24) > I landed patches 2 through 4 because bzbarsky wants to work on top of them, and > they're independent of the rest: > http://hg.mozilla.org/mozilla-central/rev/fa5326c011b8 > http://hg.mozilla.org/mozilla-central/rev/8b22441911b0 > http://hg.mozilla.org/mozilla-central/rev/cfa10b01b1f6 I backed these out because they are the most likely candidate for causing new rando-orange bug 536382: http://hg.mozilla.org/mozilla-central/rev/f32e7f33b015 http://hg.mozilla.org/mozilla-central/rev/313bf48d30ac

This differs from the previous patch only by adding a copy-constructor to TransitionEventInfo, since nsEvent and derived classes don't actually support copy-construction, so adding a hand-written copy-constructor to TransitionEventInfo is the easiest way to fix the resulting leak-stats botch.

This differs from the previously-reviewed patch only by: 1. adding ~nsDOMTransitionEvent (which is needed to run the right nsTransitionEvent destructor and thus not leak string buffers), and 2. initializing mEvent->time in nsDOMTransitionEvent::nsDOMTransitionEvent.

Comment on attachment 418901 [details] [diff] [review] patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code Ah, I should have captured the leak. I didn't care about the mEvent->time handling, since I'll change that anyway soon, I hope.

This is revised to attempt to fix bug 536382: the change in this patch is to swap the order in ~nsPresContext so the refresh driver is disconnected after the transition manager.

This is revised to attempt to fix bug 536382; the change in this patch is to move code from ~nsTransitionManager to nsTransitionManager::Disconnect so that we don't keep ElementTransitions objects alive past when the transition manager is disconnected from the pres context.

Comment on attachment 418919 [details] [diff] [review] patch 2: make refresh driver reference counted r=bzbarsky

Comment on attachment 418920 [details] [diff] [review] patch 3: make transition manager reference counted r=bzbarsky

Landed all 7 patches (relanding of 2-3-4), though in the order 2-3-4-1-5-6-7: http://hg.mozilla.org/mozilla-central/rev/188c2ab04c3d http://hg.mozilla.org/mozilla-central/rev/b9a8430469bc http://hg.mozilla.org/mozilla-central/rev/9f938021c83f http://hg.mozilla.org/mozilla-central/rev/3647e6e07a59 http://hg.mozilla.org/mozilla-central/rev/079fd624199b http://hg.mozilla.org/mozilla-central/rev/70a9757074b9 http://hg.mozilla.org/mozilla-central/rev/108a32520dab

Backed out due to pretty consistent timeouts in the new test.

Hooray for running on overloaded tinderbox machines. Relanded with the test modified (see addition of "poll_start_reversal"): http://hg.mozilla.org/mozilla-central/rev/3a81a1e824bb http://hg.mozilla.org/mozilla-central/rev/85b7535011ea http://hg.mozilla.org/mozilla-central/rev/29e07c3f08f1 http://hg.mozilla.org/mozilla-central/rev/78cf2e18de1c http://hg.mozilla.org/mozilla-central/rev/b6e75a58ab3f http://hg.mozilla.org/mozilla-central/rev/09a185326560 http://hg.mozilla.org/mozilla-central/rev/ff977518808e Note to anyone else: if the test fails again such that a backout is required: back out only the test and not the rest.

(In reply to comment #35) > Backed out due to pretty consistent timeouts in the new test. For the record, the original test timed out 3 of 16 times that it ran: in all cases because it never got the transitionend event for the reversing transition (id="four"). I presume that's because the 100ms timeout ended up firing before the forward transition even got a chance to start, so I introduced a polling function that checks that the style has started to change before reversing it.

This is already documented along with the rest of CSS transitions: https://developer.mozilla.org/en/CSS/CSS_transitions

As Firefox supports transitionend but doesn't feature ontransitionend either on the element or the window it is impossible for people to test browser support for this feature. This is a test taken from a Yahoo product that fails in Firefox but works fine in other browsers.

Christian, if that was a request to add ontransitionend, please file a separate bug for that? For what it's worth, feature-detection of this event should just be a matter of feature-detecting transition support, since the spec requires the event...

Comment on attachment 621544 [details] Tests support for transitionend across browsers (unclear why r+/f+ was set, clearing)