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

Use Leaflet.Draw fork that supports touch #502

Merged
merged 1 commit into from
Jul 1, 2015
Merged

Use Leaflet.Draw fork that supports touch #502

merged 1 commit into from
Jul 1, 2015

Conversation

rajadain
Copy link
Member

Introduction

We are switching to Michael Guild's fork of Leaflet.Draw to get support for touch screens. There are a number of considerations that must be taken into account.

Original Issue

The original issue on Leaflet.Draw is here: Leaflet/Leaflet.draw#2. The conversation on that thread started with a few contributors who came up with various solutions which worked in some cases and didn't in others, until Guild came along and essentially took ownership of the effort, and has been working on it on-and-off for more than a year now. As the conversation moves along, the comments get more and more supportive and appreciative, and the bugs become rarer and more edge-case-y. There is also some discussion about Tom Blackmore's fork, and Jacob Toye (who seems to be a core developer of Leaflet.Draw) initially seemed to favor his approach. However, Guild's fork has been more active, has been used and tested by more people, and is up to date with the current Leaflet.Draw codebase.

Pull Request

Guild has an open pull request to the main repo here: Leaflet/Leaflet.draw#285. Initial conversation is about syntax, but eventually people start testing it and liking it, and Toye chimes in with his approval and a promise to carve out time for Leaflet.Draw in the future. The main issues people have with the PR are:

  • It has too many commits, should be squashed to make more readable
  • It has other experiments, particularly related to styling, that are unrelated to the core touch functionality, that people want to be separated out
  • There is some discussion of errors in Internet Explorer, not sure if they relate to click or touch functionality
  • He seems to have done more work in private somewhere, which hasn't been added to his GitHub yet

But overall consensus is positive, with the hopes that his work will be merged into the main Leaflet.Draw repo at some point in the near future. The main blocker for that effort is simply time on the part of both Guild and Toye, who had not been able to pay much attention to this, until recently when activity has started again.

Concerns

  1. So far we have been using Leaflet.Draw 0.2.3, but all of Guild's work is up-to-date with master, thus we are not just getting the touch support, but all sorts of changes to Leaflet.Draw.
  2. Guild's repo is very much a development repo, not a release one. There aren't any tests, or tags, or releases. The current head seems to work well enough, and that's what I've pinned in package.json. All our existing tests are passing, and prelimnary testing in Chrome and Mobile Safari on iOS has been flawless. But we need to test more.

Testing

To test this, check out the PR branch and run

$ ./scripts/npm.sh install
$ ./scripts/bundle.sh --vendor

Then check with various browsers and devices that the map is usable:

  • Test drawing a polygon Area of Interest
  • Test delineating a watershed
  • Test zooming and panning with the polygon drawn
  • Test drawing modifications

Tested on:

  • Chrome 43.0.2357.130 (64-bit) on Mac OS X 10.10
  • Mobile Safari on iPad Mini 2 running iOS 8.3 — Modifications needed to be tapped twice, the first time simulated a hover and the second a click.
  • ⚠️ Mobile Safari on iPad 2 running iOS 8.1 — Had some odd behavior when entering /analyze view. Modifications needed to be tapped twice, the first time simulated a hover and the second a click.
  • Mobile Safari on iPhone 6 running iOS 8.3
  • Chrome 43.0.2357.61 on iPhone 6 running iOS 8.3
  • Firefox 38.0.5 on Mac OS X 10.10
  • ⚠️ Safari 8.0.6 on Mac OS X 10.10 — Had some odd behavior when entering /analyze view.
  • Chrome 43.0.2357.93 on Moto E running Android 5.1 Lollipop
  • Firefox on Nexus 5 on Android 4.4 KitKat
  • Chrome on Nexus 7 running Android 5.0 Lollipop
  • Chrome on Linux
  • Firefox on Linux
  • Chrome 43.0.2357.130 m on Windows 7 Enterprise SP1
  • Firefox 38.0.5 on Windows 7 Enterprise SP1
  • ⚠️ Internet Explorer 11.0.9600.17843 on Windows 7 Enterprise SP1 — Precipitation Slider does not show up. SVG patterns do not show up. See Internet Explorer 11 issues #504
  • ❌ Internet Explorer 11.0.9600 on Surface Pro 3 running Windows 8.1 Pro — The site does not render at all because IE on Surface does not support the touch events in Guild's repo and throws an error.
  • ❌ Chrome 43 on Surface Pro 3 running Windows 8.1 Pro — Touch interaction works (when you touch the screen with a finger), but click interaction (with trackpad and stylus) fails to close the polygon, instead keeps adding new overlapping points.
  • ❌ Firefox 38 on Surface Pro 3 running Windows 8.1 Pro — Click interaction works (when you use trackpad or stylus), but touch interaction fails to register a mouseclick event. Each touch acts as a hover / mousemove event instead of a click.
  • IE Mobile on Windows Phone

⚠️ = Minor problems, possibly unrelated to touch functionality
❌ = Major problems related to touch functionality

I will leave this PR open until I've tested at least on the above platforms.

Connects #476

@hectcastro
Copy link
Contributor

@azavea-bot rebuild

@mmcfarland
Copy link
Contributor

Thanks for listing out the relevant considerations to evaluate. As we discussed offline, though this should make us a little nervous, the touch support is something of must-have for the client, so this is a necessary risk. I think we just need to remain active on the issue/PR, encourage its getting merged and leave an outstanding issue in our repo to follow up when it is. If instability or other problems occur, we'll have to re-evaluate then, but I'm at least buoyed by the community response to this fork.

@kdeloach
Copy link
Contributor

Nice write up.

@rajadain
Copy link
Member Author

I've just updated the main comment with more testing information. The general feel is:

  • New Leaflet.Draw code seems to work in all existing environments, so it seems like we are unscathed by those updates
  • The touch functionality works great in both laptops and phones / tablets, but fails completely on dual-mode devices like the Surface Pro 3. With the old plugin (Leaflet.Draw 0.2.3 on develop and master), click functionality (but not touch) works in Firefox and Chrome. On Internet Explorer, even click doesn't work, but the page renders.
  • There are other issues, unrelated to this change, present in Safari and Internet Explorer

I will test further today.

@rajadain
Copy link
Member Author

Created #504 for Internet Explorer issues.

@rajadain rajadain self-assigned this Jun 25, 2015
@rajadain
Copy link
Member Author

A potential large deal breaker is that click functionality is impeded in Chrome on Windows when running on a touchscreen: The polygon refuses to close, and clicking on the starting point simply adds more overlapping points instead of closing the polygon.

Internet Explorer won't even render the page.

This can be alleviated by adding a button, only visible during drawing, that allows one to close the polygon.

@mmcfarland
Copy link
Contributor

I got confirmation from Stroud today that they are comfortable supporting iPad and desktop browsers specifically, even knowing the current limitations with the dual-input devices like the Surface. We should track the life of this fork so we can revert to the origin, but let's deal with "minor" devices and their problems as their reported.

@rajadain
Copy link
Member Author

rajadain commented Jul 1, 2015

I'm subscribed to the original issue and the pull request, as well as the fork, so should be notified of any activity there. I'm going to rebase this on top of develop and run a final test to ensure it still works, and will then merge.

@rajadain
Copy link
Member Author

rajadain commented Jul 1, 2015

Created #526 and #527 for Android and iPad issues respectively. While this PR adds touch support, we need some more work to polish the UX in touch devices. Those issues are there to track whatever issues we discover as we test.

Rebasing on develop was straightforward. Tested on iPad and Android, as well as traditional non-touch "desktop" browsers to ensure that things still work.

rajadain added a commit that referenced this pull request Jul 1, 2015
Use Leaflet.Draw fork that supports touch

Connects #476
@rajadain rajadain merged commit 81d95f9 into WikiWatershed:develop Jul 1, 2015
@rajadain rajadain removed the in review label Jul 1, 2015
@rajadain rajadain deleted the feature/touch-support branch July 1, 2015 17:53
@ddproxy
Copy link

ddproxy commented Jul 2, 2015

Hey guys, I'm working on a leaflet.draw fork that will be needing IE support for drawing polygons etc.
https://github.com/ddproxy/Leaflet.draw

Just wanted to inject myself into this conversation going forward. Cool work with watershed by the way!

@SRGDamia1
Copy link
Member

I just got a report that this problem is also coming up in touchscreen Chromebooks.

This was referenced Aug 15, 2017
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.

6 participants