Note: There are a few cases of duplicates in user autocompletion which are being worked on.

implement transitionend event for end of CSS transitions

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla1.9.3a1
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(URL)

Attachments

(8 attachments, 7 obsolete attachments)

4.09 KB, patch
Details | Diff | Splinter Review
9.58 KB, patch
Details | Diff | Splinter Review
6.90 KB, patch
Details | Diff | Splinter Review
4.06 KB, patch
Details | Diff | Splinter Review
24.94 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.41 KB, patch
Details | Diff | Splinter Review
6.73 KB, patch
Details | Diff | Splinter Review
1.72 KB, text/html
Details
(Assignee)

Description

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

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

8 years ago
Created attachment 417233 [details] [diff] [review]
patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code
Attachment #417233 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 6

8 years ago
Created attachment 417234 [details] [diff] [review]
patch 2: make refresh driver reference counted
Attachment #417234 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

8 years ago
Created attachment 417235 [details] [diff] [review]
patch 3: make transition manager reference counted
Attachment #417235 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

8 years ago
Created attachment 417236 [details] [diff] [review]
patch 4: make refresh driver hold reference to observers while notifying
Attachment #417236 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

8 years ago
Created attachment 417237 [details] [diff] [review]
patch 5: add function for which CSS property computed values should use different name for the property
Attachment #417237 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

8 years ago
Created attachment 417238 [details] [diff] [review]
patch 6: dispatch transitionend events
Attachment #417238 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

8 years ago
Created attachment 417239 [details] [diff] [review]
patch 7: tests
(Assignee)

Updated

8 years ago
Attachment #417233 - Attachment description: part 1: implement event in the event code → patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code
(Assignee)

Comment 12

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

Updated

8 years ago
Attachment #417234 - Flags: review?(bzbarsky) → review+

Comment 13

8 years ago
Comment on attachment 417234 [details] [diff] [review]
patch 2: make refresh driver reference counted

r=bzbarsky

Comment 14

8 years ago
Comment on attachment 417235 [details] [diff] [review]
patch 3: make transition manager reference counted

r=bzbarsky
Attachment #417235 - Flags: review?(bzbarsky) → review+

Comment 15

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

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

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

Comment 18

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

Comment 19

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

Comment 20

8 years ago
Created attachment 417241 [details] [diff] [review]
patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code

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

Comment 21

8 years ago
Created attachment 417242 [details] [diff] [review]
patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code

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)

Updated

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

Comment 23

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

Comment 24

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

Comment 25

8 years ago
Created attachment 418839 [details] [diff] [review]
patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code

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

Updated

8 years ago
Attachment #417242 - Attachment is obsolete: true

Updated

8 years ago
Attachment #418839 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

8 years ago
Depends on: 536382
(Assignee)

Comment 26

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

Comment 27

8 years ago
Created attachment 418900 [details] [diff] [review]
patch 6: dispatch transitionend events

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

Comment 28

8 years ago
Created attachment 418901 [details] [diff] [review]
patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code

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

Updated

8 years ago
Attachment #418901 - Attachment description: implement transitionend event / nsIDOMTransitionEvent in the event code → patch 1: implement transitionend event / nsIDOMTransitionEvent in the event code
(Assignee)

Updated

8 years ago
Attachment #418839 - Attachment is obsolete: true
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.

Updated

8 years ago
Attachment #418901 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 30

8 years ago
Created attachment 418919 [details] [diff] [review]
patch 2: make refresh driver reference counted

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

Comment 31

8 years ago
Created attachment 418920 [details] [diff] [review]
patch 3: make transition manager reference counted

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)

Updated

8 years ago
Attachment #418900 - Flags: review?(bzbarsky) → review+

Comment 32

8 years ago
Comment on attachment 418919 [details] [diff] [review]
patch 2: make refresh driver reference counted

r=bzbarsky
Attachment #418919 - Flags: review?(bzbarsky) → review+

Comment 33

8 years ago
Comment on attachment 418920 [details] [diff] [review]
patch 3: make transition manager reference counted

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

Comment 34

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

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Backed out due to pretty consistent timeouts in the new test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 36

8 years ago
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
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

8 years ago
(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.
Keywords: dev-doc-needed
(Assignee)

Updated

8 years ago
Blocks: 537142
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
Keywords: dev-doc-needed → dev-doc-complete
Whiteboard: [doc-waiting-1.9.3]
(Assignee)

Updated

7 years ago
Depends on: 537573

Comment 39

5 years ago
Created attachment 621544 [details]
Tests support for transitionend across browsers

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+

Comment 40

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