Bug 105647 – Unprefixed transitionend event doesn't seem to be implemented, which breaks many sites

• Component: DOM
• Status: RESOLVED FIXED

[Philip Walton]

Many libraries (including bootstrap and modernizr) recommend detecting the transitionend event with code like the following:

var transitionEnd = (function() {
    
  var el = document.createElement('div');
  var eventMap = {
    // list the w3 event first so it's used if found
    'transition'       : 'transitionend',
    'WebkitTransition' : 'webkitTransitionEnd',
    'MozTransition'    : 'transitionend',
    'OTransition'      : 'oTransitionEnd otransitionend'
  };

  for (var key in eventMap) {
    if (eventMap.hasOwnProperty(key) && el.style[key] != null) {

      // return the transition end event in the map
      return eventMap[key];
    }
  };
})();

Recently the the plain old "transitionend" event started to appear (unprefixed) as a key in the DOM elements style object, but the unprefixed transitionend event doesn't seem to be working via JavaScript.

I've confirmed that this breaks applications in Chrome Canary and Webkit Nightly, but not in Chrome stable.

Here is a test case showing what happens:
http://jsfiddle.net/Pab7e/

[Paul Irish]

AFAICT, this is due to http://trac.webkit.org/changeset/138184 and enabling by default.

Verified on Chrome Canary that while unprefixed transitions do now work, the unprefixed transitionend event does not. 

Metabug for unprefixing TTA: https://bugs.webkit.org/show_bug.cgi?id=93136

[Alexis Menard (darktears)]

(In reply to comment #1)
> AFAICT, this is due to http://trac.webkit.org/changeset/138184 and enabling by default.
> 
> Verified on Chrome Canary that while unprefixed transitions do now work, the unprefixed transitionend event does not. 
> 
> Metabug for unprefixing TTA: https://bugs.webkit.org/show_bug.cgi?id=93136

It's still implementation in progress so yes stuff like that is missing.I put it blocker of the meta bug so we don't forget about it. In the other hand chromium should deactivate the feature on their shipping release until it's ready.

[Paul Irish]

Thanks Alexis.. makes sense.
I"m currently looking for someone to flip the feature flag for Chromium.

[Alexis Menard (darktears)]

(In reply to comment #3)
> Thanks Alexis.. makes sense.
> I"m currently looking for someone to flip the feature flag for Chromium.

Thanks. Worst case I'll send an email to webkit-dev next week. That's why I put the feature flag in the first place, to avoid shipping an half implemented feature while we are working on it and make sure we're aligned with the spec.

[Nico Weber]

> It's still implementation in progress so yes stuff like that is missing.I put it blocker of the meta bug so we don't forget about it. In the other hand chromium should deactivate the feature on their shipping release until it's ready.

I'll turn this off for chromium for now. I see that your patch set the flag to 'on' for all ports. Would't it make more sense to default the flag to 0 everywhere and only flip it once it's actually working? Tests could set a pref in the mean time if you did this for testing.

[Alexis Menard (darktears)]

(In reply to comment #5)
> > It's still implementation in progress so yes stuff like that is missing.I put it blocker of the meta bug so we don't forget about it. In the other hand chromium should deactivate the feature on their shipping release until it's ready.
> 
> I'll turn this off for chromium for now. I see that your patch set the flag to 'on' for all ports. Would't it make more sense to default the flag to 0 everywhere and only flip it once it's actually working? Tests could set a pref in the mean time if you did this for testing.

Yes it's on purpose, we have couple of feature activated on trunk but deactivated on shipping products. I need the bots to run the code, I need some people to be able to test so we decided to keep it on by default on trunk only (it was on the email sent on webkit-dev and nobody objected at that time). Also it's not a feature per say as the transitions are already supported via the prefix -webkit. But don't worry I'll fix the events as soon as possible.

[Paul Irish]

As Bug 104647 was fixed, this issue is now resolved at least for Chrome Canary. I imagine it is still broken for WebKit nightly , but I see no immediate reason to resolve that.

[Paul Irish]

> As Bug 104647 was fixed...

Apologies, make that Bug 105771

[Alexis Menard (darktears)]

Created attachment 182382 [details]
Patch

[Alexis Menard (darktears)]

*** Bug 106529 has been marked as a duplicate of this bug. ***

[Alexis Menard (darktears)]

Created attachment 182540 [details]
Patch

[Julien Chaffraix]

Comment on attachment 182540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182540&action=review

> Source/WebCore/ChangeLog:31
> +        (Event):

Let's remove those meaningless lines.

> Source/WebCore/ChangeLog:33
> +        (WebCore::Event::mutateToPrefixed): this will swap the event name from

Sentences start with a capital letter.

> Source/WebCore/dom/Event.cpp:84
> +: m_type(eventType)
> +, m_prefixedType(prefixedEventType)
> +, m_canBubble(canBubbleArg)
> +, m_cancelable(cancelableArg)
> +, m_propagationStopped(false)
> +, m_immediatePropagationStopped(false)
> +, m_defaultPrevented(false)
> +, m_defaultHandled(false)
> +, m_cancelBubble(false)
> +, m_eventPhase(0)
> +, m_currentTarget(0)
> +, m_createTime(convertSecondsToDOMTimeStamp(currentTime()))

Doh'! Style violation: it should be indented.

> Source/WebCore/dom/Event.h:187
> +    AtomicString m_prefixedType;

It's bad that now every Event is bigger just to handle a corner case. Also because of this design, you *have* to pass both unprefixed and prefixed version around which adds a lot of boilerplate code.

I would prefer an out-of-band mechanism to handling sending the unprefixed version. For example, in EventTarget, you could keep track of prefixed events matching the corresponding unprefixed so that you properly apply your fallback logic.

[Alexis Menard (darktears)]

Created attachment 182730 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #13)
> Created an attachment (id=182730) [details]
> Patch

Alternative approach, julien what do you think?

[Alexis Menard (darktears)]

Created attachment 182735 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 182783 [details]
Patch

[Julien Chaffraix]

Comment on attachment 182783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182783&action=review

> Source/WebCore/ChangeLog:12
> +        work on CSS Transitions. This patch adds support to save a prefixed and
> +        an unprefixed name in the Event class which is used later in the event
> +        dispatching to figure out whether we want to send the prefixed or the
> +        unprefixed event. In the case of the CSS Transitions, WebKit will now

The sentence is stale: you don't store any information in the event anymore.

> Source/WebCore/ChangeLog:19
> +

Nit: Adding the thread to webkit-dev would be nice as this was discussed broadly and no one objected or found a better alternative.

> Source/WebCore/ChangeLog:26
> +        (WebCore::Document::addListenerTypeIfNeeded): Add support for
> +        transitionend in case there is only an event listener on the unprefixed
> +        event.

I see what you meant but the wording feels weird. How about something like (feel free to edit to your needs):

Register the prefixed listener too as transitionend listeners so that we properly dispatch events for them.

> Source/WebCore/dom/Document.cpp:3733
> +    else if (eventType == eventNames().transitionendEvent)
> +        addListenerType(TRANSITIONEND_LISTENER);

Nit: You could put the 2 ifs on the same line as they are logically equivalent.

if (eventType == eventNames().webkitTransitionEndEvent || eventType == eventNames().transitionendEvent)

> Source/WebCore/dom/EventTarget.cpp:164
> +static PassRefPtr<Event> createPrefixedEvent(const Event* event)

Nit: I would rename this to createMatchingPrefixedEvent to emphasize that it's strictly the same event but prefixed.

> Source/WebCore/dom/EventTarget.cpp:204
> +        prefixedEvent->setTarget(event->target());
> +        prefixedEvent->setCurrentTarget(event->currentTarget());
> +        prefixedEvent->setEventPhase(event->eventPhase());

This should probably go into the createPrefixedEvent as you really want to create a cloned / match event.

[Alexis Menard (darktears)]

Created attachment 182806 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 182806 [details]
Patch for landing

Clearing flags on attachment: 182806

Committed r139762: <http://trac.webkit.org/changeset/139762>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass move bugs into the DOM component.