Closed Bug 1236930 Opened 8 years ago Closed 8 years ago

Zoom on maps using Leaflet.js broken due to lack of webkitTransitionEnd event support (strava, weather.com, etc.)

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1236979
Tracking Flags:
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed

People

(Reporter: flod, Unassigned)

References

Details

I think we broke something but I'm not sure what (hence starting from General)

Example: open this page and try to zoom twice on the map
https://www.strava.com/activities/462895119

Firefox gets stuck after the first zoom in-out, no related error reported in the console.

Dev Edition still works fine 45.0a2 (2016-01-05), Nightly used to work. So far I'm only sure that 46.0a1 (2016-01-04) is already broken.
Works in 46.0a1 (2016-01-01) from
http://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-01-03-03-30-mozilla-central-l10n/

Already broken on 46.0a1 (2016-01-03) Linux (previous tests were from OS X).
Narrowed it down even further. It's broken in both Linux and OS X, e10s and non-e10s windows

Works in 46.0a1 (2016-01-01) from
http://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-01-03-03-30-mozilla-central-l10n/
 
Broken in 46.0a1 (2016-01-02)
http://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-02-03-02-17-mozilla-central-l10n/
Noticed bug 1213126 in the range. If I set layout.css.prefixes.webkit to false, reload the page, zoom starts working again.
Blocks: 1213126
I see they're using a MapBox open street map embed -- the following two demos continue to work, so it's possible (hopefully) this is strava specific:

https://www.openstreetmap.org/#map=2/49.3/-17.6
https://www.mapbox.com/mapbox.js/example/v1.0.0/
Hm, no. This open street map/mapbox example is also broken with layout.css.prefixes to true: <http://codepen.io/stevepepple/pen/aOMmpe>

For Strava, there seems to be something broken with the webkitTransitionEnd listener on <div style="transform: translate(0px, 0px);" class="leaflet-map-pane leaflet-zoom-anim"></div>. If I remove that listener from Chrome DevTools, the map is broken in the same way.
Doing some exploring...

There's a o.DomUtil.testProp method to determine what prefix (if any) the app should be listening for:

testProp: function(t) {
  for (var i = document.documentElement.style, n = 0; n < t.length; n++)
    if (t[n] in i)
        return t[n];
  return !1
}

later on, they use that testProp to pick the right transform* event to listen to:

o.DomUtil.TRANSFORM = o.DomUtil.testProp(["transform", "WebkitTransform", "OTransform", "MozTransform", "msTransform"])

Right now in Nightly (or w/ layout.css.prefixes.webkit to true elsewhere) that returns "transform". And indeed, we're firing "transitionend" events when you do a zoom. But the script is apparently listening for webkitTransitionEnd so there's a mismatch there. 

I see the same thing in Edge (testProp returns "transform" and there's a webkitTransitionEnd listener on div.leaflet-map-pane).
Sounds like there may be a bug in the library which we should see about getting fixed. If both a prefixed & unprefixed form of an API are available (and the site is making an effort to detect them), they should default to the unprefixed one.

Based on comment 7, it sounds like they're already trying to do this, but something's going wrong in their logic and they're using the prefixed API anyway.

I filed bug 1236979 on adding support for the webkit-prefixed versions of the transition events, which would make this work -- but it may be worth holding off and seeing if we can get this library fixed first (or also).
Agreed we should try to get this fixed upstream (probably w/ openstreetmap?) -- but I think we might have to hack around it given how widespread it's used. :/

Hallvord, want to dig in and find the bug here? this._mapPane is the DOM element in question, I don't know why o.DomUtil.TRANSITION_END would be the webkit prefixed variant, rather than unprefixed in https://d3nn82uaxijpm6.cloudfront.net/assets/mapbox-0fd71b6e080ef35cc068dfb9ab80c9b2.js:

o.DomUtil.TRANSITION && o.Map.addInitHook(function() {
        this._zoomAnimated = this.options.zoomAnimation && o.DomUtil.TRANSITION && 
                             o.Browser.any3d && !o.Browser.android23 && !o.Browser.mobileOpera,
        this._zoomAnimated && o.DomEvent.on(this._mapPane, o.DomUtil.TRANSITION_END, this._catchTransitionEnd, this)
    }),
Flags: needinfo?(hsteen)
Strava is using Mapbox: https://github.com/mapbox/mapbox.js/, which is built on top of Leaflet.js: http://leafletjs.com/ - demos from those work, so that's at least reassuring.
(In reply to Mike Taylor [:miketaylr] from comment #9)
> I don't know why o.DomUtil.TRANSITION_END would be the
> webkit prefixed variant, rather than unprefixed in
> https://d3nn82uaxijpm6.cloudfront.net/assets/mapbox-0fd71b6e080ef35cc068dfb9ab80c9b2.js:

Because they test "webkitTransition" before the unprefixed "transition". If the order is intentional, we will have to fix bug 1236979 :(

o.DomUtil.TRANSITION=o.DomUtil.testProp(["webkitTransition","transition","OTransition","MozTransition","msTransition"]),
o.DomUtil.TRANSITION_END="webkitTransition"===o.DomUtil.TRANSITION||"OTransition"===o.DomUtil.TRANSITION?o.DomUtil.TRANSITION+"End":"transitionend",
Masatoshi-san nailed it (thanks!) :)
Flags: needinfo?(hsteen)
Oh, silly -- I copied it down incorrectly when testing things locally. Good catch Masatoshi. Note how they don't use the same order for TRANSFORM:

o.DomUtil.TRANSFORM=o.DomUtil.testProp(["transform","WebkitTransform","OTransform","MozTransform","msTransform"]),
o.DomUtil.TRANSITION=o.DomUtil.testProp(["webkitTransition","transition","OTransition","MozTransition","msTransition"])

I'll try to find the right place to upstream a fix, but I still suspect
(oops)

...but I still suspect we need to fix 1236979. See https://github.com/search?p=2&q=DomUtil.TRANSITION+testProp+webkitTransition&ref=searchresults&type=Code&utf8=%E2%9C%93. This leaflet library is potentially widely used and apparently has *two* different orderings if you search through the results.

o.DomUtil.TRANSITION=o.DomUtil.testProp(["webkitTransition","transition"...
o.DomUtil.TRANSITION=o.DomUtil.testProp(["transition", "webkitTransition"...

I'm not sure if the change was intentional or not, but I can try to dig through the history of the repo.
And there we have it:

https://github.com/Leaflet/Leaflet/commit/64ca0af124b7145e8c2a746e2fb087b2298a4b72

The order was switched because older Android browsers had prefixless transition but not prefixless transitionEnd event. :(
Summary: Zoom on maps broken on strava.com → Zoom on maps broken on strava.com (openstreetmap)
Leaflet dev here. 

We could work around the problem ( see https://github.com/Leaflet/Leaflet/commit/ecc68197c3a9d1634fef60ae34968fdc55b4afdc ), but this would apply only to webpages with an updated version of the library (leaving a lot of webpages unusable).

Unfortunately the problem is not limited to the Leaflet library. Users of Bootstrap are probably also affected, see https://github.com/twbs/bootstrap/blob/master/js/transition.js#L19 :-(
Hey ivan,

(In reply to ivan from comment #17)
> Leaflet dev here. 
> 
> We could work around the problem ( see
> https://github.com/Leaflet/Leaflet/commit/
> ecc68197c3a9d1634fef60ae34968fdc55b4afdc ), but this would apply only to
> webpages with an updated version of the library (leaving a lot of webpages
> unusable).

Yeah, getting old web sites to update is really, really hard. 

> Unfortunately the problem is not limited to the Leaflet library. Users of
> Bootstrap are probably also affected, see
> https://github.com/twbs/bootstrap/blob/master/js/transition.js#L19 :-(

Thanks for pointing that out, it makes me wonder if an older version of Modernizr did the same thing at some point too.
Depends on: 1236979
the weather map at weather.com also stops zooming after zooming once.
(In reply to Gary [:streetwolf] from comment #19)
> the weather map at weather.com also stops zooming after zooming once.

It's worth noting that weather.com uses the Bing Maps JS library, and not Leaflet.
Thanks Gary and ivan, I spun out Bug 1239342 for the weather.com issue. It could be the same problem, or something slightly different -- let's take diagnosis over there.
Summary: Zoom on maps broken on strava.com (openstreetmap) → Zoom on maps using Leaflet.js broken due to lack of webkitTransitionEnd event support (strava, weather.com, etc.)
Tracking as it impacts a bunch of websites. We may need to delay the webkit css prefixes.
tracking-firefox46: --- → +
(FWIW, the pref is behind a guard as of bug 1238827, so it won't ride the trains (enabled) past Aurora for now.)
This is working for me in today's Nightly. Can you verify flod?
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(francesco.lodolo)
Resolution: --- → FIXED
Yes, strava.com definitely works with 47.0a1 (2016-02-03)
Status: RESOLVED → VERIFIED
Flags: needinfo?(francesco.lodolo)
This still seems to be broken on 46 aurora. Mike can you double check?
Flags: needinfo?(miket)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #29)
> This still seems to be broken on 46 aurora. Mike can you double check?

That's correct -- the fix (Bug 1236979) hasn't been backported to Aurora yet. Requested approval 2 days ago, no response yet (not too surprising, sometimes it takes a couple days). Hopefully granted soon.
Flags: needinfo?(miket)
I've just realized that this bug has been solved but there's no patch. Does it make sense to dupe it to bug 1236979 instead?
Probably, yeah.
Status: VERIFIED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: FIXED → DUPLICATE
I'll update this bug's dupes to be direct dupes of bug 1236979, too.
No longer depends on: 1236979
Tracking in the duplicated bug.
tracking-firefox46: + → ---
You need to log in before you can comment on or make changes to this bug.