Bug 105647 - Unprefixed transitionend event doesn't seem to be implemented, which breaks many sites
: Unprefixed transitionend event doesn't seem to be implemented, which breaks m...
Status: RESOLVED FIXED
: WebKit
HTML Events
: 528+ (Nightly build)
: Macintosh Mac OS X 10.8
: P2 Major
Assigned To:
:
: WebExposed
: 105771
: 93136
  Show dependency treegraph
 
Reported: 2012-12-21 11:38 PST by
Modified: 2013-01-15 11:19 PST (History)


Attachments
Patch (22.92 KB, patch)
2013-01-11 11:30 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.91 KB, patch)
2013-01-14 03:28 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.76 KB, patch)
2013-01-15 03:23 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.56 KB, patch)
2013-01-15 04:11 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.51 KB, patch)
2013-01-15 08:55 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (14.84 KB, patch)
2013-01-15 10:50 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-12-21 11:38:32 PST
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/
------- Comment #1 From 2012-12-21 14:54:25 PST -------
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
------- Comment #2 From 2012-12-22 03:02:55 PST -------
(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.
------- Comment #3 From 2012-12-22 19:36:53 PST -------
Thanks Alexis.. makes sense.
I"m currently looking for someone to flip the feature flag for Chromium.
------- Comment #4 From 2012-12-23 04:56:35 PST -------
(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.
------- Comment #5 From 2012-12-26 10:00:06 PST -------
> 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.
------- Comment #6 From 2012-12-27 06:07:23 PST -------
(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.
------- Comment #7 From 2012-12-29 08:12:57 PST -------
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.
------- Comment #8 From 2012-12-29 08:14:01 PST -------
> As Bug 104647 was fixed...

Apologies, make that Bug 105771
------- Comment #9 From 2013-01-11 11:30:54 PST -------
Created an attachment (id=182382) [details]
Patch
------- Comment #10 From 2013-01-11 11:32:11 PST -------
*** Bug 106529 has been marked as a duplicate of this bug. ***
------- Comment #11 From 2013-01-14 03:28:40 PST -------
Created an attachment (id=182540) [details]
Patch
------- Comment #12 From 2013-01-14 10:06:01 PST -------
(From update of attachment 182540 [details])
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.
------- Comment #13 From 2013-01-15 03:23:05 PST -------
Created an attachment (id=182730) [details]
Patch
------- Comment #14 From 2013-01-15 03:23:34 PST -------
(In reply to comment #13)
> Created an attachment (id=182730) [details] [details]
> Patch

Alternative approach, julien what do you think?
------- Comment #15 From 2013-01-15 04:11:49 PST -------
Created an attachment (id=182735) [details]
Patch
------- Comment #16 From 2013-01-15 08:55:29 PST -------
Created an attachment (id=182783) [details]
Patch
------- Comment #17 From 2013-01-15 10:26:59 PST -------
(From update of attachment 182783 [details])
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.
------- Comment #18 From 2013-01-15 10:50:14 PST -------
Created an attachment (id=182806) [details]
Patch for landing
------- Comment #19 From 2013-01-15 11:19:09 PST -------
(From update of attachment 182806 [details])
Clearing flags on attachment: 182806

Committed r139762: <http://trac.webkit.org/changeset/139762>
------- Comment #20 From 2013-01-15 11:19:15 PST -------
All reviewed patches have been landed.  Closing bug.