Leaflet: Adding 'leaflet-dragging' class causes drag start slow down

[danzel]

https://github.com/Leaflet/Leaflet/blob/master/src/dom/Draggable.js#L95

When you start a map drag we add this class so that you get the grabbing hand.
In larger apps, this causes a bit of a slow down as the browser recalculates styles

(We have 1000 markers with leaflet.labels attached)

If you comment out this line the drag start is super smooth and there are no big lumps in the timeline during a drag.
If anyone has any ideas that still give us the dragging hand without the performance hit that'd be awesome :)
I'll fiddle around a bit later today.

[jfirebaugh]

I've seen something very similar in iD and have a hunch that there was a recalculate style-related performance regression somewhere in the last several Chrome versions.

Worth checking Chrome Canary.

[danzel]

Just tested in Canary 33, issue still exists there.
Any idea how far back I should go in Chrome to confirm the hunch?

[jfirebaugh]

No, couldn't really say.

Do you know a good way of testing old Chrome versions? I wanted to do that but they seemed very difficult to obtain.

[danzel]

Not an exact version, but they do keep continuous chromium builds on here:
http://commondatastorage.googleapis.com/chromium-browser-continuous/index.html?path=Win/
Page takes a bit to load, if you click through you can see the date it was built to help track it down.
edit: this took me ages of searching when I originally found it :)

[mourner]

@danzel is it only reproducible on Chrome, and not on other browsers like Firefox, Safari, IE?

[danzel]

Good question!
I'm pretty certain there is a small lag at the start of a drag in IE10, its hard to confirm with no timeline view.
I don't have FF in front of me, I'll do some further cross browser testing tomorrow.

[mourner]

OK, if we don't figure this out, lets just sacrifice having the dragging hand cursor outside of the map view, and set the dragging class on map container. I think it's a good compromise if it means better performance for an essential operation like dragging. GMaps does this apparently btw. Won't help with the 1000 markers problem though I suppose...

[mourner]

Another option to try is having a transparent empty div with the cursor set over everything while you drag. In theory it shouldn't cause relayout of all other elements, but I'm not sure.

[jfirebaugh]

.leaflet-dragging is also sometimes used in app CSS to implement effects like "hovering" a marker during drags.

[danzel]

IIRC leaflet.draw uses an empty transparent div when drawing to prevent clicking on existing vector layers etc

[danzel]

Setting style on document.body:

Setting the style on this._element:

Mouse pointer still changes, so this is a win.

Was thinking about setting it on e.target, it works on map-mobile.html, but on a map with a vector overlay it doesn't, we'd need to set the style on that too. Complexity is probably not worth the win in this case.

[mourner]

@danzel I remember using this initially, but eventually switched to document as setting cursor on the element introduced some undesirable cursor effects, such as cursor jumping back and forth when dragging near a marker, or dragging a marker. Is it not like that in modern browser versions?

Also, what's the reason that 2468 elements are affected if you change cursor on just one element?

[danzel]

Will do some further testing on this on monday.
I believe it means it had to check for style changes on that many elements (as they are now inheriting the dragging class). It's much less as it isn't hitting the elements that aren't in the map.

[danzel]

Mouse pointer doesn't bug in chrome or firefox when dragging near a marker, or dragging a marker quickly.
IE doesn't support the grabby hand pointers at all by the looks, so you just get the arrow on them.

[mourner]

Hmm, perhaps it was like that with draggable markers...

[mourner]

OK, fiddled with this a bit today and the pull is generally an improvement indeed, although there's a small glitch with draggable markers — if you drag a marker close to its edge, you can see the cursor flickering back and forth because marker position isn't completely in sync with cursor position, so it sometimes gets over the map which isn't getting the cursor treatment. Also, the change isn't solving the case where there are lots of elements on a map (like in your case).

I tried putting a transparent overlay, and it works when starting to drag the map, but there's a glitch — for some reason cursor isn't updated when the overlay is removed — it's only updated after you drag the mouse a little.

While I'll investigate some more, I think that maybe it would be best to postpone solving the issue to post-0.7, and meanwhile suggest just removing the .leaflet-dragging part in Leaflet CSS in case it is a performance problem.

[mourner]

@danzel so the e.target thing doesn't work on maps with vector layers because L.DomUtil.add/removeClass doesn't work on SVG elements. I'll look into patching it...

[mourner]

OK patched DomUtil.add/removeClass to work with SVG elements (and improve their perf significantly with classList)! @danzel now we can patch Draggable to only set the cursor on e.target, but lets make sure we don't break the use case @jfirebaugh mentioned. Perhaps that means moving the cursor styles to another class, and still adding leaflet-dragging to the body, while the target gets something like leaflet-dragging-hand.

[mourner]

Oops, except maybe IE9, which doesn't support classList, damn! http://caniuse.com/#search=classlist
But maybe IE9 will be fine setting el.className, I'm not sure — it would be nice to check just in case.

[klimashkin]

Please, do not use 'classList' in el, replace it with el.classList !== undefined
It's 5-10 times faster: http://jsperf.com/hasownproperty-vs-in-vs-undefined/12

[klimashkin]

Even better determine it ones in some var:

var cl = document.documentElement.classList !== undefined;
if (cl) {...} else {}

To avoid every time walk on all prototypes of html element

[mourner]

@klimashkin nice tips, but it's not used that often to have any difference.

[mourner]

@danzel OK fixed the IE9 issue (in theory).

[klimashkin]

@mourner Grain by grain, and the hen fills her belly ;)

[danzel]

Testing this now.
Deleting the styles related to dragging but still adding the class to body doesn't improve performance, we get the same style recalculation.

[mourner]

That's weird, it wasn't affecting performance in my tests... Will check again.

[mourner]

Yeah, you're right, it does recalculate styles even if the class is empty, which is dumb.

[danzel]

I deleted this:
https://github.com/Leaflet/Leaflet/blob/master/dist/leaflet.css#L173-179

Will see if I can make a reproducable (bad performing) case outside of our app.

Setting the class on e.target removes the performance issue, but the pointer can flicker on and off.

[mourner]

BTW, added Path className option finally as a bonus. :)

[mourner]

Weird, just checked it again and removing the lines above fixes the performance for me (there's no recalculate styles with thousands of elements).

[danzel]

Heres a test page, put it in debug/map/slow.html:
https://gist.github.com/danzel/01ee1a6de9b996117e60

[mourner]

@danzel yep, just tried it and no style recalculation on class add if I remove the grab cursor styles.

[danzel]

Damn you are right!
Now to figure out what is wrong in our app :)

[mourner]

Added leaflet-drag-target class to e.target (in addition to leaflet-dragging on the body) for more flexible styling. Now the behavior can be changed purely in CSS — remove cursor, add them only on on target, or only inside the container, etc. I think this is good enough to close now.

[fab1an]

I'm having this issue as well. The layout() calculation depends on the size of the layout-hierarchy which is invalidated. If you have a complex page, setting the class on document.body triggers the layout of all elements.

In my case those are ~3000 elements.
Chrome needs 15ms for that, Safari on IOS 7.1 needs 60ms.

In my opinion the class should be set on the map-pane or leaflet-container, so only leaflet's parts are invalidated though I don't know how to obtain a reference to getPane() in Draggable.js.

[monkora]

We're still having this issue, where on click-to-drag the addition of 'leaflet-dragging' to the body class list is causing the browser Recalculate Styles for our entire application. Removing line 5862 in leaflet-src addClass(document.body, 'leaflet-dragging'); fixes the issue but we're not sure what the side effects of this patch would be.

[Malvoz]

In https://github.com/Leaflet/Leaflet/issues/2164#issuecomment-28334951, @mourner wrote:

no style recalculation on class add if I remove the grab cursor styles.

However this article suggests that _any_ DOM changes will cause browsers to re-calculate styles:

Changing the DOM, through adding and removing elements, changing attributes, classes, or through animation, will all cause the browser to recalculate element styles, in many cases, layout (or reflow) the page, or parts of it.

@mourner It seems from comments above that the performance issue wasn't resolved. Would you consider re-opening this?

Edit: Because there seem to be use cases for leaflet-dragging (as opposed to only having leaflet-drag-target), would it make sense to perhaps make leaflet-dragging opt-in (or opt-out for backwards compatibility)? This would allow developers to effectively isolate the leaflet container in terms of rendering/recalculations, using CSS Containment, without having some of the performance benefits being negated by DOM changes on <body>.