Apparently some sites (in particular the MapBox toolkit) depend on webkitTransitionStart / webkitTransitionEnd events, at least in browsers that support "-webkit" prefixed CSS. See bug 1236930 as an example. Depending on how prevalent this is, we should consider adding these events, probably just as aliases for the standard versions. I believe Edge added support for them, according to bug 1236930. (I don't think this is covered by an existing bug, but let me know if I'm wrong.)

This quick and dirty test case currently works in Edge and Safari: http://jsbin.com/vivuvaquci/1/edit?html,css,js,console,output -- they're both sending a webkitTransitionEnd event after an unprefixed transition.

From my local testing with Chrome, it looks like they *only* send a webkitTransitionEnd event if there are no standard-'transitionend' listeners.

Edge, Safari, and Chrome all agree on testcases 1 and 2 -- they only send one event, and it'll be the unprefixed event if any listeners have been registered for that. (This happens on a per-element basis. For example: if I modify the testcase to have two divs, div1 which has a listener for 'transitionend' & 'webkitTransitionEnd', and div2 which *only* has a listener for 'webkitTransitionEnd', then Chrome sends *only* 'transitionend' for div1, while still sending 'webkitTransitionEnd' for div2.) I'll bet that some sites register listeners for both the prefixed & unprefixed events, and if any browser sent both events, they'd end up getting the listener behavior triggered twice, which would be bad.

(Dropping "webkitTransitionStart" from summary, since we don't support transitionstart at all yet -- just transitionend. transitionstart is in level 2 of the transitions spec.)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1236930#c15, Leaflet.js prioritizes webkitTransitionEnd (for the past 3 years) before transitionend to work around some Android browsers not having unprefixed transitionEnd events. Given this libs popularity (like 12k stars and 2k forks on GitHub) and some of the names on its "Trusted by the best" -- https://cloudup.com/cJrk9P8SSE8, we probably need to do this for compat. :(

Probably need to spec this too...

I filed https://github.com/whatwg/compat/issues/24 as a spec bug (at least as a place to start).

Here's a testcase with listeners for both types of events, and with the "transitionend" listeners removed one at a time. It demonstrates that, after all transitionend listeners have been removed, then webkitTransitionEnd listeners will fire for subsequent transitions.

(that's the behavior I see in Chrome, at least)

Comment 5 wasn't entirely correct. This attached testcase demonstrates (in Chrome) that it's possible for a single transition to trigger both types of event-listeners (prefixed & unprefixed), if the listeners are on different elements. So it looks like basically, as this event fires and the bubbles, we do the following for each node that it visits: - If any 'transitionend' listeners exist on this node, invoke them. - Otherwise, if any 'webkitTransitionEnd' listeners exist on this node, invoke them.

I don't know our event-handling code very well, but from poking around in GDB, I *think* this bug's changes might want to go in EventListenerManager::HandleEventInternal() (or its helpers). That seems to be the place where, for an event that's being fired on a given node, we actually iterate over the listeners on that node to see which ones match the event's type. (I imagine we could e.g. see which flavor(s) of transitionend event listeners are registered have, and decide based on that whether to fire the transitionend vs. webkittransitionend listeners.)

Alternately, we could handle this up a few levels, in EventDispatcher::HandleEvent (where we look up the EventListenerManager). Here's a demo patch which detects this scenario and logs with NS_WARNING, in HandleEvent. (Not sure yet exactly what action we can/should take, at the C++ level, when this case is detected.)

smaug points out that the same thing likely happens for prefixed animation events, and the code for doing the conversion (from modern type to prefixed/legacy type) is here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/events/EventTarget.cpp&sq=package:chromium&l=290&dr=C&rcl=1452688339 Summing up the upshot of my IRC discussion with smaug just now: So after the current loop over our listeners in EventListenerManager::HandleEventInternal, *if* no listener was invoked, then we should call some function to see if there's a webkit-prefixed version of our event type. And if so, repeat the loop with the event type temporarily adjusted. (via Event::SetEventType)

(In reply to Daniel Holbert [:dholbert] from comment #15) > smaug points out that the same thing likely happens for prefixed animation > events The comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1239342#c2 suggests the same thing, I think.

FWIW, I guess the strategy here would be valuable for me to follow for unprefixing fullscreen events. My current local pending patch for bug 743198 adds a flag on document for whether the prefixed event should be triggered and the flag is set in EventListenerManager::AddEventListenerInternal. This method seems to work with Youtube and Facebook, so probably I can just go that way. But if the solution here makes more sense, I'm happy to follow as well.

(we probably need a better title for this bug, Bug 1239136 depends on webkitAnimationEnd events -- just like smaug predicted in Comment #15).

Tracking this out of a vague uneasy feeling. I ended up here from poking through regressions from bug 1213126 (ie this looks like some more work to support enabling layout.css.prefixes.webkit by default)

I've got a local patch which makes us behave like Chrome on my attached testcases - hooray!

Here's my current WIP "part 2" patch (the main patch here). It's not ready for review yet; just posting for reference. I believe this patch makes us fire the correct listeners, but it doesn't adjust event.type yet.

Comment on attachment 8709660 [details] [diff] [review] part 2 wip Review of attachment 8709660 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/events/EventListenerManager.cpp @@ +1147,5 @@ > popupStatePusher.emplace(Event::GetEventPopupControlState(aEvent, *aDOMEvent)); > } > > bool hasListener = false; > + Maybe<EventMessage> legacyEventMessage; I'd suggest making this just > EventMessage eventMessage = aEvent->mMessage; so that we can eliminate the additional check inside ListenerCanHandle.

Comment on attachment 8709658 [details] [diff] [review] part 1: add names & enums for webkit-prefixed transition/animation events >@@ -915,16 +915,17 @@ NON_IDL_EVENT(MozEdgeUICanceled, > eEdgeUICanceled, > EventNameType_None, > eSimpleGestureEventClass) > NON_IDL_EVENT(MozEdgeUICompleted, > eEdgeUICompleted, > EventNameType_None, > eSimpleGestureEventClass) > >+// CSS Transition & Animation events: > NON_IDL_EVENT(transitionend, > eTransitionEnd, > EventNameType_None, > eTransitionEventClass) > NON_IDL_EVENT(animationstart, > eAnimationStart, > EventNameType_None, > eAnimationEventClass) >@@ -932,16 +933,34 @@ NON_IDL_EVENT(animationend, > eAnimationEnd, > EventNameType_None, > eAnimationEventClass) > NON_IDL_EVENT(animationiteration, > eAnimationIteration, > EventNameType_None, > eAnimationEventClass) > >+// Webkit-prefixed versions of Transition & Animation events, for web compat: >+NON_IDL_EVENT(webkitTransitionEnd, >+ eWebkitTransitionEnd, >+ EventNameType_None, >+ eTransitionEventClass) >+NON_IDL_EVENT(webkitAnimationEnd, >+ eWebkitAnimationEnd, >+ EventNameType_None, >+ eAnimationEventClass) >+NON_IDL_EVENT(webkitAnimationIteration, >+ eWebkitAnimationIteration, >+ EventNameType_None, >+ eAnimationEventClass) >+NON_IDL_EVENT(webkitAnimationStart, >+ eWebkitAnimationStart, >+ EventNameType_None, >+ eAnimationEventClass) >+ blink at least exposes onfoo properties on window object for these event types. So you want to use WINDOW_EVENT (and then some patch should add the same EventHandlers to webidl) We don't seem to expose ontransitionend etc at properties. Should we add those too? https://bugzilla.mozilla.org/show_bug.cgi?id=911987 is related.

(In reply to Olli Pettay [:smaug] (expect slower than usual review time for couple of days) from comment #24) > We don't seem to expose ontransitionend etc at properties. > Should we add those too? > https://bugzilla.mozilla.org/show_bug.cgi?id=911987 is related. I think so. window.onfoo properties are frequently used for feature detection, for better or worse.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #23) > > + Maybe<EventMessage> legacyEventMessage; > > I'd suggest making this just > > EventMessage eventMessage = aEvent->mMessage; > so that we can eliminate the additional check inside ListenerCanHandle. Yeah, I thought about that; I kind of like the explicitness of using Maybe<>, because it makes it extra-clear why we're passing in this extra value (alongside the one that aEvent contains) and whether it's relevant. (And that in the normal case, we defer to aEvent's own bundled value.) But maybe I should just add extra documentation and get rid of the Maybe<> wrapper to save us the extra comparison.

(In reply to Olli Pettay [:smaug] (expect slower than usual review time for couple of days) from comment #24) > blink at least exposes onfoo properties on window object for these event > types. A few thoughts: - I feel a little odd supporting "onwebkittransitionend" without supporting "ontransitionend". - So far we haven't run into content that depends on "onwebkittransitionend". - Maybe we'll add support for all of these "onfoo" attributes (prefixed & unprefixed), but I'd like to decouple that step from this bug here. > We don't seem to expose ontransitionend etc at properties. > Should we add those too? > https://bugzilla.mozilla.org/show_bug.cgi?id=911987 is related. Maybe? I'd rather not gate this bug here on the discussion in that bug, though. (Also related: bug 531585 comment 23, where we elected not to implement ontransitionend in the first place.) For now, bug 911987 comment 9 suggests the following: - other browsers may be moving towards *removing* these "onfoo" attributes - usage is low ...so it would be a shame if we added support for them as a mandatory part of implementing support for the events, & implicitly encouraged usage, & baked these attributes into the web just when they were about to be removed.

(smaug, what are your thoughts on comment 27? Are you OK with us supporting listeners for these events, without their "onwebkitXYZ" attributes, for the time being -- with the intent to maybe add them after bug 911987 sorts out a bit more?)

It seems like Edge doesn't have any global `onwebkitfoo` event handlers, at least exposed to the console.

(In reply to Daniel Holbert [:dholbert] from comment #27) > A few thoughts: > - I feel a little odd supporting "onwebkittransitionend" without supporting > "ontransitionend". I agree. I think we should support them both. There are no real signs that other browsers would be dropping them. > - So far we haven't run into content that depends on > "onwebkittransitionend". > - Maybe we'll add support for all of these "onfoo" attributes (prefixed & > unprefixed), but I'd like to decouple that step from this bug here. Don't know why. For consistency wouldn't it make sense to support them all. But ok, I could live also with the setup Edge has - so make sure we support at least those webkit* prefixed names it supports. > For now, bug 911987 comment 9 suggests the following: > - other browsers may be moving towards *removing* these "onfoo" attributes In practice it looks like that isn't happening.

Using the following test case, which sets up window.onwebkittransitionend, inline elem onwebkittransitionend attribute and document.addEventListener('webkitTransitionEnd') listeners: https://miketaylr.com/bzla/onwebkittransitionend.html Safari and Chrome log: elem.onwebkittransitionend listener was triggered. document webkitTransitionEnd listener was triggered. window.onwebkittransitionend listener was triggered. Edge only logs: document webkitTransitionEnd listener was triggered.

This patch creates a helper-class that lets us temporarily adjust the event's type, by overriding its "mMessage" member-variable. I believe smaug originally suggested using Event::SetEventType() to make this tweak, but after trying to use that function to override & then later restore the event's type, I think it's not really what we want, because it requires a nsAString& arg. This means it incurs extra overhead (converting from enum --> string --> enum), both when setting the overriding event-type and when restoring the original event-type. It's simpler if we just directly swap out mMessage, as this helper-class does. As long as the Event's "typeString" is empty (as it always seems to be for recognized event types, based on my testing), then mMessage is really the only thing we need to set -- and we end up producing the correct stringified value for JS queries to event.type, because Event::GetType() just converts the mMessage enum to a string. The next patch will add a usage of this class, if you want to see it in-context.

Most of this patch is reindenting, so I'll post a "diff -w" version for review.

(Reposting part 3, with a nit fixed -- reverted an unnecessary blank-line insertion.)

I just noticed my new #include for Maybe.h at the bottom of part 3 (inside EventListenerManager.h) can move to the .cpp file, too. I've made that change locally -- not bothering re-re-posting for that.

Here's my mochitest patch. It verifies the following things, for each type of "legacy" event that we support: - If there's only a handler registered for the legacy event (the exact scenario that we're hitting with site breakage), we fire that handler, with the correct legacy name in event.type. - If handlers are registered for both the "modern" & "legacy" event, then we only fire the modern one. - As an event bubbles up through ancestors, it can fire different types of handlers (legacy vs. modern) at each level, depending on what handlers are registered at that level. This seemed like an appropriate scenario to use JS Promises. I'm somewhat new to promises, but I think I'm using them correctly here; please feel free to correct me if I'm doing anything clearly-wrong. I did skim http://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html and the MDN promises page, for reference.

(I improved the mochitest a bit -- now it triggers animationiteration in a more targeted, robust way; see comment before triggerAnimationIteration in this patch for more details. Tweaked some other comments/naming while I was at it, too.)

Note that Apple have now said they will *not* remove the onanimation* event handlers after all (see bug 911987 comment 13)

Comment on attachment 8713290 [details] [diff] [review] part 2: Create an RAII helper-class to temporarily override an Event's mMessage (i.e. its DOM-exposed 'type') >+ explicit EventMessageAutoOverride(nsIDOMEvent* aEvent, >+ EventMessage aOverridingMessage) >+ : mEvent(aEvent->InternalDOMEvent()), >+ mOrigMessage(mEvent->mEvent->mMessage) >+ { >+ NS_ASSERTION(aOverridingMessage != mOrigMessage, >+ "Don't use this class if you're not actually overriding"); >+ NS_ASSERTION(aOverridingMessage != eUnidentifiedEvent, >+ "Only use this class with a valid overriding EventMessage"); >+ NS_ASSERTION(mOrigMessage != eUnidentifiedEvent && >+ mEvent->mEvent->typeString.IsEmpty(), >+ "Only use this class on events whose overridden type is " >+ "known (so we can restore it properly)"); Please use MOZ_ASSERT for all these.

Comment on attachment 8714112 [details] [diff] [review] part 4 v2: mochitest (Hopefully I didn't miss anything crucial in this Promise-y test)

Thanks for the review! I'll swap out those asserts before landing.

Here's a successful Try run from yesterday, BTW: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfcc3aebc9e8

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f4de652525e https://hg.mozilla.org/integration/mozilla-inbound/rev/d7136545a16e https://hg.mozilla.org/integration/mozilla-inbound/rev/f62e6638add1 https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b1e085b161

Backed out all patches for xpcshell failures (didn't think to test that suite on Try): https://hg.mozilla.org/integration/mozilla-inbound/rev/39f872a217ff Failure is something about checking prefs off the main thread. So apparently this event-dispatching code runs off the main thread sometimes. Depending on whether the off-main-thread event handling is web-exposed at all [I suspect/hope not?], maybe we can just behave as if the pref is off (and webkit prefixes are unsupported) for off-main-thread scenarios.

The offending off-main-thread usage was in WorkerRunnable::Run, which I think means it's a worker-thread (which makes sense that it's off-main-thread). Log showing a failure: https://treeherder.mozilla.org/logviewer.html#?job_id=20880257&repo=mozilla-inbound (In this case we're dispatching an OfflineStatusChangeEvent, it seems.) I assert that our behavior w.r.t. animation/transition events in this context (on a worker thread) doesn't really matter, because real animation/transition events should only be dispatched on the main thread anyway. (I think the only way we could get animation/transition-flavored events in a worker-thread would be if an author synthesized & dispatched their own events with these types, manually, in JS. And even then, this bug's code would only matter if the author synthesized an *unprefixed* event, and then registered a handler for the prefixed event name. On a worker thread.) SO: for as long as we have the webkit about:config pref-check here, I think we're OK to simply err on the conservative side for off-main-thread runs through this code, & just behave as if the pref is ( / might be) false -- i.e. don't support webkit events as special legacy versions of modern events in that scenario.

Here's the diff for my update to part 3, to address the xpcshell failure. smaug, does this look OK to you? (see my previous comment)

Comment on attachment 8714683 [details] [diff] [review] interdiff for updated part 3 (skipping webkit emulation, if we're main-thread) We don't want even more thread checks for this hot code path. And we already know in EventListenerManager in which thread we are. Use mIsMainThreadELM for the thread check.

Thanks -- here's a new interdiff using that member-var instead. (To see that member-var, we have to be a method, not a static function. So I promoted GetLegacyEventMessage to be a protected method, moved it down to be alongside other protected methods, and added this bool check.) I verified locally that this passes this bug's test & one of the formerly-asserting xpcshell tests.

https://hg.mozilla.org/integration/mozilla-inbound/rev/725cda04b0fb https://hg.mozilla.org/integration/mozilla-inbound/rev/45b314cc2495 https://hg.mozilla.org/integration/mozilla-inbound/rev/23c492f3f6db https://hg.mozilla.org/integration/mozilla-inbound/rev/0475bea08514

Comment on attachment 8709658 [details] [diff] [review] part 1: add names & enums for webkit-prefixed transition/animation events Requesting approval to backport to Aurora. Approval Request Comment [Feature/regressing bug #]: Webkit-prefixed CSS support (enabled in bug 1213126. This feature will be automatically disabled in beta & release builds for now, as of bug 1238827, so neither this regression nor this fix should end up affecting beta or release). [User impact if declined]: Site breakage, at Yahoo's mobile site (bug 1239136), and in maps at Strava & Weather.com (bug 1236930), due to these sites strangely favoring webkit-prefixed transition/animation event names over standard event names if they detect browser support for webkit-prefixed CSS properties. [Describe test coverage new/current, TreeHerder]: I believe our event-dispatching code has a pretty robust mochitest testsuite. This bug includes tests for the new behavior, too. (part 4) [Risks and why]: Small risk of new site breakage as a result of this change, either from bugs in the patches or from sites making more weird assumptions that we haven't forseen. This feels pretty safe for Aurora, though (after it's had a few days of Nightly testing at least). [String/UUID change made/needed]: None.

(My aurora approval request is for all of the patches here (parts 1 through 4) -- everything that landed on inbound in comment 51.)

I believe the patches here fix part of the problem described in Bug 1244464 (but not everything).

https://hg.mozilla.org/mozilla-central/rev/725cda04b0fb https://hg.mozilla.org/mozilla-central/rev/45b314cc2495 https://hg.mozilla.org/mozilla-central/rev/23c492f3f6db https://hg.mozilla.org/mozilla-central/rev/0475bea08514

Sounds like this potentially could fix many other bugs and it will be disabled past aurora. While we might uncover new work to be done by waiting, I don't think we have to wait to uplift this.

Comment on attachment 8709658 [details] [diff] [review] part 1: add names & enums for webkit-prefixed transition/animation events Approved for uplift to aurora, all 4 patches that landed on m-c. Includes new webkit tests. This will be automatically disabled for beta.

https://hg.mozilla.org/releases/mozilla-aurora/rev/080f6d59dbd7 https://hg.mozilla.org/releases/mozilla-aurora/rev/bdeb576d0948 https://hg.mozilla.org/releases/mozilla-aurora/rev/ddf2c54bd8b9 https://hg.mozilla.org/releases/mozilla-aurora/rev/bd428ed7ad2e

This behavior was specced in DOM: https://dom.spec.whatwg.org/#concept-event-listener-invoke

(forgot to add dev-doc-needed, would fit well here: https://developer.mozilla.org/en-US/Firefox/Releases/49#Compatibility)