Leaflet / Leaflet.draw

Vector drawing and editing plugin for Leaflet
https://leaflet.github.io/Leaflet.draw/docs/leaflet-draw-latest.html
MIT License
1.97k stars 992 forks source link

Clockwise drawing of polygon fails + cannot close polygon on Windows + Chrome #637

Open z0d14c opened 7 years ago

z0d14c commented 7 years ago

May be related to https://github.com/Leaflet/Leaflet.draw/issues/563 although that appears to be occurring on IE11.

Basically the issue we're having is that on someone's Windows 8.x + Chrome (latest version) box, the closing of polygons is very difficult (it appears that you have to click on the center pixel instead of any pixel in the draw handle) and drawing them clockwise is impossible (leaflet.draw says there is an intersection when there isn't).

I will be testing this further and trying to come up with a fix, but if someone more intimately involved with Leaflet.draw thinks they can do it faster don't let me stop you.

update: see post-merge comments on #640

z0d14c commented 7 years ago

Reproduced on windows + edge as well.

z0d14c commented 7 years ago

It appears that #563 does indeed fix the issue for us. Can @Smeats solution not be merged into this project?

B3nCr commented 7 years ago

It's also related to this https://github.com/Leaflet/Leaflet.draw/issues/631 and this https://github.com/Leaflet/Leaflet.draw/issues/624.

It seems to me as if this is becoming a major issue.

I've fixed this another way in another PR. I haven't tested @Smeats fix so I don;t know what affect that has on the touch events.

Please can you test this and see if it does what you need https://b3ncr.github.io/docs/examples/full.html

My PR is here, I may re-create it because it has an unnecessary change in there but if I take it out the github.io page will break. https://github.com/Leaflet/Leaflet.draw/pull/636

z0d14c commented 7 years ago

@B3nCr It does appear to fix the issues, in maybe a cleaner way than @Smeats'. We may fork your branch until this is cleared up (i.e. merged into Leaflet.draw master). Thanks for looking into this, I'll try to remain actively pushing to get attention for this bug as it is pretty bad for many users.

z0d14c commented 7 years ago

strangely, Leaflet.draw 0.4.7 does not seem to fix the issues for me, although my branch of @B3nCR's branch does. im not sure why this is the case.

z0d14c commented 7 years ago

forgot to leave a comment after I discovered new information -- here's my comment from B3nCr's PR:

Yeah, I'm pretty sure that regresses your fix, workaround or no, which basically renders IE/Android/iOS unusable for the respectively bugged draw shapes. I'll see if I can take a look at working on a PR this week as I'd like to continue using the main repo, but we'll see. @B3nCr

Basically it looks like Leaflet.draw 0.4.7 promptly regressed B3nCr's fix, and we likely need more tests to address this situation.

z0d14c commented 7 years ago

i have a PR up to address some of these issues, further testing needed on IE because i think touchstart events may need some special handling in that context (and my IE VM was running extremely slow)

ddproxy commented 7 years ago

Thanks for the PR, I'll review after I land.

This seems to have occurred on Chrome 55.x on Mac as well.

z0d14c commented 7 years ago

I think theres one small addition thats needed, which is binding only the touch or the mouseclick event binding in addHooks, depending on L.Browser.touch, in order for IE to work correctly (duplicate events are messing things up)

On Dec 13, 2016 6:44 PM, "Jon West" notifications@github.com wrote:

Thanks for the PR, I'll review after I land.

This seems to have occurred on Chrome 55.x on Mac as well.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/637#issuecomment-266927175, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBpMWzB36LFO9Eb0lPpPbLx7TW9vepTks5rH1f4gaJpZM4KzF3h .

z0d14c commented 7 years ago

I believe my PR fixes all of these issues https://github.com/Leaflet/Leaflet.draw/pull/657

jedfong commented 7 years ago

I've seen a similar issue when L.Browser.touch is disabled (leaflet v1.0.2 / leaflet-draw v0.4.9). Polylines/Polygons will not close. The problem goes away if I comment out the following line in addHooks (L.Draw.Polyline):

.on('touchstart', this._onTouch, this)

Maybe this should be conditional based on if touch is enabled/disabled?