Closed Bug 531585 Opened 15 years ago Closed 14 years ago

implement transitionend event for end of CSS transitions

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 7 obsolete files)

4.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.90 KB, patch
Details | Diff | Splinter Review
4.06 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
24.94 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.73 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.72 KB, text/html
Details
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
Blocks: 521890
(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.
Attachment #417233 - Attachment description: part 1: implement event in the event code → patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code
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?
Attachment #417234 - Flags: review?(bzbarsky) → review+
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
Attachment #417235 - Flags: review?(bzbarsky) → review+
Comment on attachment 417236 [details] [diff] [review]
patch 4: make refresh driver hold reference to observers while notifying

r=bzbarsky
Attachment #417236 - Flags: review?(bzbarsky) → review+
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?
Attachment #417237 - Flags: review?(bzbarsky) → review+
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.
Attachment #417238 - Flags: review?(bzbarsky) → review+
(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).
Attachment #417233 - Attachment is obsolete: true
Attachment #417241 - Flags: review?(Olli.Pettay)
Attachment #417233 - Flags: review?(Olli.Pettay)
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
Attachment #417241 - Attachment is obsolete: true
Attachment #417242 - Flags: review?(Olli.Pettay)
Attachment #417241 - Flags: review?(Olli.Pettay)
Blocks: 528306
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.
Attachment #417242 - Flags: review?(Olli.Pettay) → review-
(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?
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
Attachment #418839 - Flags: review?(Olli.Pettay)
Attachment #418839 - Flags: review?(Olli.Pettay) → review+
(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.
Attachment #417238 - Attachment is obsolete: true
Attachment #418900 - Flags: review?(bzbarsky)
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.
Attachment #418901 - Flags: review?(Olli.Pettay)
Attachment #418901 - Attachment description: implement transitionend event / nsIDOMTransitionEvent in the event code → patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code
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.
Attachment #418901 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #417234 - Attachment is obsolete: true
Attachment #418919 - Flags: review?(bzbarsky)
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.
Attachment #417235 - Attachment is obsolete: true
Attachment #418920 - Flags: review?(bzbarsky)
Attachment #418900 - Flags: review?(bzbarsky) → review+
Comment on attachment 418919 [details] [diff] [review]
patch 2: make refresh driver reference counted

r=bzbarsky
Attachment #418919 - Flags: review?(bzbarsky) → review+
Comment on attachment 418920 [details] [diff] [review]
patch 3: make transition manager reference counted

r=bzbarsky
Attachment #418920 - Flags: review?(bzbarsky) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Backed out due to pretty consistent timeouts in the new test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
(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.
Whiteboard: [doc-waiting-1.9.3]
This is already documented along with the rest of CSS transitions:

https://developer.mozilla.org/en/CSS/CSS_transitions
Whiteboard: [doc-waiting-1.9.3]
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.
Attachment #621544 - Flags: review+
Attachment #621544 - Flags: feedback+
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)
Attachment #621544 - Flags: review+
Attachment #621544 - Flags: feedback+
You need to log in before you can comment on or make changes to this bug.