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
Product: WebKit
Classification: Unclassified
Component: HTML Events
: 528+ (Nightly build)
: Macintosh Mac OS X 10.8
: P2 Major
Assigned To: Alexis Menard (darktears)
: WebExposed
Depends on: 105771
Blocks: 93136
  Show dependency treegraph
 
Reported: 2012-12-21 11:38 PST by Philip Walton
Modified: 2013-01-15 11:19 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Walton 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 Paul Irish 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 Alexis Menard (darktears) 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 Paul Irish 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 Alexis Menard (darktears) 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 Nico Weber 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 Alexis Menard (darktears) 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 Paul Irish 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 Paul Irish 2012-12-29 08:14:01 PST
> As Bug 104647 was fixed...

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

Alternative approach, julien what do you think?
Comment 15 Alexis Menard (darktears) 2013-01-15 04:11:49 PST
Created attachment 182735 [details]
Patch
Comment 16 Alexis Menard (darktears) 2013-01-15 08:55:29 PST
Created attachment 182783 [details]
Patch
Comment 17 Julien Chaffraix 2013-01-15 10:26:59 PST
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.
Comment 18 Alexis Menard (darktears) 2013-01-15 10:50:14 PST
Created attachment 182806 [details]
Patch for landing
Comment 19 WebKit Review Bot 2013-01-15 11:19:09 PST
Comment on attachment 182806 [details]
Patch for landing

Clearing flags on attachment: 182806

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