Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make drag handler insensitive to order of handlers events #4387

Merged
merged 4 commits into from
Apr 15, 2016

Conversation

perliedman
Copy link
Member

Draggable handles touch events, and does not rely on
simulated mouse events; under some circumstances, it
even breaks on simulated events (see #4315).

This PR ignores any simulated events in the event handlers,
to just deal with the real events. This makes it insensitive
to if the tap handler has run before, and fired simulated
events.

Close #4315.

@yohanboniface
Copy link
Member

Related: #3872 (comment)

@yohanboniface
Copy link
Member

The more we go, the more I feel uncomfortable with having both a native event and a simulated one sent in the same time. I'm wondering then if the "proper" fix is not a bit before in the chain, trying to send either the native event or the simulated one.
Cf also #3872 (comment) that I just pasted above.

Also, what about making a prostetic unit test for this, do you feel it's doable and can be isolated enough without being to much coupled (given some events are simulated, if we simulate the simulation, what are we testing? :p) ?

@perliedman
Copy link
Member Author

Wow, yeah your comment was indeed very spot on, it more or less exactly describes this issue:

when disabling/reenabling dragging, mousedown comes first, because this time L.Map.Tap has precedence on event callback order

As it is now, only the first touch from then touch event is passed on to the mouse event (https://github.com/Leaflet/Leaflet/blob/master/src/map/handler/Map.Tap.js#L101-103). As far as I could understand, that might be the core of at least this particular issue. How about passing along all touches in the simulated event? I think (but have not tested) this would fix this issue. What other consequences would it have, is there good reasoning behind just passing one touch?

@yohanboniface yohanboniface added this to the 1.0-rc1 milestone Apr 5, 2016
@yohanboniface
Copy link
Member

@perliedman any chance we can remote pair on this during the week-end so we can proceed on rc1? :)

@perliedman
Copy link
Member Author

@yohanboniface sounds like a good idea! I think I will be able to set aside time for this on friday (15th of april).

@perliedman
Copy link
Member Author

@yohanboniface sorry, I obviously didn't read that sentence fully :) I think I'll have a hard time scheduling time tomorrow, unfortunately :(

@yohanboniface
Copy link
Member

@perliedman that's ok, let's plan a session on Friday then :)
@IvanSanchez @hyperknot @danzel @mourner Leaflet day on Friday April 15th, book your agenda! :)

Per Liedman and others added 4 commits April 15, 2016 15:29
Draggable handles touch events, and does not rely on
simualted mouse events; under some circumstances, it
even breaks on simualted events (see #4315).

This ignores any simulated events in the event handlers,
to just deal with the real events.

Close #4315.
@IvanSanchez IvanSanchez merged commit fcffa30 into master Apr 15, 2016
@IvanSanchez IvanSanchez deleted the fix-disable-enable-drag branch April 15, 2016 13:33
perliedman added a commit that referenced this pull request Apr 18, 2016
perliedman pushed a commit that referenced this pull request Apr 18, 2016
* First version of rc1 changelog (cf #4379)

* fixed changelog

typos, closed PR removed, contributor added

* Prettify and fix minor typos in rc1 changelog

* Mention #4396 in the CHANGELOG

* Add mention of #4371

* Add docs updates by nathancahill

* Add info on #4387

* Update date for 1.0-rc1

* Remove double PR reference for #4418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants