search

Leaflet / Leaflet.draw

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

Polygon issues on IE11 #618

Open Greigrm opened 8 years ago

Greigrm commented 8 years ago

Seeing a couple of issues with IE11 and the polygon feature. Release 0.4.1 / Leaflet 1.0

Firstly - intersections are constantly warned about (incorrectly) - can be seen in github example. Secondly - you can never click the first point to complete the shape.

ddproxy commented 8 years ago

Notes:

  1. Something is not binding?
  2. Related to a touch handler

I'll look into this, thanks

WangJi commented 8 years ago

Draw Polygon fails on Edge too. It's hard to click on the first point to close the polygon, The node-icon displays in Edge is in 20px 20px size, while by default it should be 8px 8px. image

WangJi commented 8 years ago

L.Browser.touch = false; is a ugly fix

ddproxy commented 8 years ago

I agree this is an issue with touch. Ugly fix too, but this is severely browser dependent.

Greigrm commented 8 years ago

Yes, setting L.Browser.touch to false only under IE works for me - thanks. Happy for you to close this.

richardhinkamp commented 7 years ago

Setting L.Browser.touch = false; gives normal sized node icon but closing the polygon is still nearly impossible. Same for ending a polyline by clicking on the last node.

I'm using leaflet 1.0.2 and draw 0.4.7.

Having this issue in IE11 and Edge, also with Safari on iPad.

With Chrome on Android it closes the polygon, but also adds an extra node close the starting node, so that seems to do both adding a node and closing the polygon.

ddproxy commented 7 years ago

Are you targeting touch devices or both?

richardhinkamp commented 7 years ago

We have users with ipads, android tablets, all desktop browsers and all desktop browsers with touchscreens, so pretty much everything. We might get away with disabling draw capabilities for touchscreens on desktop browsers, forcing them to use the mouse.

B3nCr commented 7 years ago

Forcing them to use the mouse might not fix the issue.

Back in 4.3 we had users with touch screen laptops that only used mouse and they still had the issue and also users that didn't have touch screens and still had the issue in IE.

On Thu, Dec 8, 2016 at 7:56 AM, Richard Hinkamp notifications@github.com wrote:

We have users with ipads, android tablets, all desktop browsers and all desktop browsers with touchscreens, so pretty much everything. We might get away with disabling draw capabilities for touchscreens on desktop browsers, forcing them to use the mouse.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/618#issuecomment-265676346, or mute the thread https://github.com/notifications/unsubscribe-auth/ABN9VCZ1YtxXTybWY5qx3xu1ElniNFGUks5rF7g3gaJpZM4Kd-TW .

richardhinkamp commented 7 years ago

Well since L.Browser.touch is always true in IE, even with no touchscreen, that makes sense.

richardhinkamp commented 7 years ago

I've been doing a little digging. When enabling a L.Draw.Polygon on the first click 2 touch events and 1 mouse up event occur. So it adds 3 markers to the map at the same location. Clicking on that marker will click the one at the top, which is the 3rd and so you can never click the first marker. This is in IE11 which behaves as pointer AND touch. Will need to check what happens on ipad, maybe just 2 touch events which results in the same problem.

So there is a bug in the event handling, but maybe draw can get smarter too. When drawing a polyline or polygon, multiple consecutive markers at the same location make no sense and could be filtered?

B3nCr commented 7 years ago

That's basically what my PR addressed. I checked to see if the event was handled and then didn't handle it more than once.

On Thu, Dec 8, 2016 at 10:56 AM, Richard Hinkamp notifications@github.com wrote:

I've been doing a little digging. When enabling a L.Draw.Polygon on the first click 2 touch events and 1 mouse up event occur. So it adds 3 markers to the map at the same location. Clicking on that marker will click the one at the top, which is the 3rd and so you can never click the first marker. This is in IE11 which behaves as pointer AND touch. Will need to check what happens on ipad, maybe just 2 touch events which results in the same problem.

So there is a bug in the event handling, but maybe draw can get smarter too. When drawing a polyline or polygon, multiple consecutive markers at the same location make no sense and could be filtered?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/618#issuecomment-265712660, or mute the thread https://github.com/notifications/unsubscribe-auth/ABN9VPRDMl1M5-5RL5ikGU29r_qQzEcmks5rF-JXgaJpZM4Kd-TW .

germanjoey commented 7 years ago

@Richard Hinklamp - I'm honestly surprised that the code that checks for poly self-intersections doesn't prevent multiple consecutive markers already.

On Thu, Dec 8, 2016 at 2:56 AM, Richard Hinkamp notifications@github.com wrote:

I've been doing a little digging. When enabling a L.Draw.Polygon on the first click 2 touch events and 1 mouse up event occur. So it adds 3 markers to the map at the same location. Clicking on that marker will click the one at the top, which is the 3rd and so you can never click the first marker. This is in IE11 which behaves as pointer AND touch. Will need to check what happens on ipad, maybe just 2 touch events which results in the same problem.

So there is a bug in the event handling, but maybe draw can get smarter too. When drawing a polyline or polygon, multiple consecutive markers at the same location make no sense and could be filtered?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/618#issuecomment-265712660, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzNEv02GOKsZeI3i4CY3zsg3gHS65Jtks5rF-JXgaJpZM4Kd-TW .

germanjoey commented 7 years ago

Oh, interesting, it seems that the poly intersection check only happens when editing a polygon/polyline, not when drawing a new one. That shouldn't be too hard to fix, lemme see if I can do that right now.

On Thu, Dec 8, 2016 at 1:43 PM, Joseph Ryan jfr7p@virginia.edu wrote:

@Richard Hinklamp - I'm honestly surprised that the code that checks for poly self-intersections doesn't prevent multiple consecutive markers already.

On Thu, Dec 8, 2016 at 2:56 AM, Richard Hinkamp notifications@github.com wrote:

I've been doing a little digging. When enabling a L.Draw.Polygon on the first click 2 touch events and 1 mouse up event occur. So it adds 3 markers to the map at the same location. Clicking on that marker will click the one at the top, which is the 3rd and so you can never click the first marker. This is in IE11 which behaves as pointer AND touch. Will need to check what happens on ipad, maybe just 2 touch events which results in the same problem.

So there is a bug in the event handling, but maybe draw can get smarter too. When drawing a polyline or polygon, multiple consecutive markers at the same location make no sense and could be filtered?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/618#issuecomment-265712660, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzNEv02GOKsZeI3i4CY3zsg3gHS65Jtks5rF-JXgaJpZM4Kd-TW .

germanjoey commented 7 years ago

What does this comment mean? I could perhaps fix this but I need to know what "degenerate cases" means.

// @method newLatLngIntersects(): boolean // Check for intersection if new latlng was added to this polyline. // NOTE: does not support detecting intersection for degenerate cases.

// @method newPointIntersects(): boolean

// Check for intersection if new point was added to this polyline. // newPoint must be a layer point. // NOTE: does not support detecting intersection for degenerate cases.

On Thu, Dec 8, 2016 at 4:19 PM, Joseph Ryan jfr7p@virginia.edu wrote:

Oh, interesting, it seems that the poly intersection check only happens when editing a polygon/polyline, not when drawing a new one. That shouldn't be too hard to fix, lemme see if I can do that right now.

On Thu, Dec 8, 2016 at 1:43 PM, Joseph Ryan jfr7p@virginia.edu wrote:

@Richard Hinklamp - I'm honestly surprised that the code that checks for poly self-intersections doesn't prevent multiple consecutive markers already.

On Thu, Dec 8, 2016 at 2:56 AM, Richard Hinkamp <notifications@github.com

wrote:

I've been doing a little digging. When enabling a L.Draw.Polygon on the first click 2 touch events and 1 mouse up event occur. So it adds 3 markers to the map at the same location. Clicking on that marker will click the one at the top, which is the 3rd and so you can never click the first marker. This is in IE11 which behaves as pointer AND touch. Will need to check what happens on ipad, maybe just 2 touch events which results in the same problem.

So there is a bug in the event handling, but maybe draw can get smarter too. When drawing a polyline or polygon, multiple consecutive markers at the same location make no sense and could be filtered?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/618#issuecomment-265712660, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzNEv02GOKsZeI3i4CY3zsg3gHS65Jtks5rF-JXgaJpZM4Kd-TW .

ddproxy commented 7 years ago

So, this is legacy... old... detection code. Apparently the algorithm used doesn't detect some complex cases - which I can only imagine as spirals?

richardhinkamp commented 7 years ago

I got a simple fix for not adding double markers, which fixes some issues. iPad works, IE has still some issues because mouse+touch events. I noticed some more weird event stuff, 1 mousedown followed by a double mouseup, double touch events. I guess broken events should be fixed first. I'll see if I can find anything.

richardhinkamp commented 7 years ago

Double events seem to be a leaflet problem with PointerEvent, being worked on: https://github.com/Leaflet/Leaflet/issues/5180 This occurs in IE11 and Chrome 55+. When that's fixed, this issue can (hopefully) be properly fixed.

z0d14c commented 7 years ago

I have a fix for the "difficulty closing the polyline/polygon" issue in my PR: https://github.com/Leaflet/Leaflet.draw/pull/654

Unfortunately, the duplicated pointerevent/clicks/touches issue persists. I have a fork of the 0.4.3 version of Leaflet-draw where this does not occur that we've been using in production, so it must be some changed in Leaflet/Leaflet-draw that has occurred since then (possibly in conjunction with chrome updates) which causes these issues.

cosme-benito commented 7 years ago

Any updates regarding the oversized square node icons?

I am aware that setting L.Browser.touch = false; might fix it but we have users with touch devices so this is not really desired... In the meanwhile I made a stupid workaround, which is far from perfect but at least mitigates the issue: L.Browser.touch = (('ontouchstart' in window) || (navigator.msMaxTouchPoints > 0));

mlazowik commented 7 years ago

Some of these got resolved in #661, right?