smart-facility / petajakarta-web

Web files for PetaJakarta.org
http://petajakarta.org
Other
8 stars 6 forks source link

touch detection broken in IE #114

Open matthewberryman opened 8 years ago

matthewberryman commented 8 years ago

For IE desktop the zoom buttons are not shown: screenshot 2016-03-05 12 40 18 but for IE mobile (set via browser profile not emulation) the zoom buttons are shown: screenshot 2016-03-05 12 58 07 This is the opposite of the desired behaviour.

matthewberryman commented 8 years ago

Seems related to https://github.com/Leaflet/Leaflet/issues/3944

matthewberryman commented 8 years ago

More info at: https://msdn.microsoft.com/library/hh673557(v=vs.85).aspx https://msdn.microsoft.com/en-au/library/hh772144(v=vs.85).aspx

matthewberryman commented 8 years ago

Note: I have MS Edge turned on in my IE11 VM. In IE11 if I am reading the docs correctly the ms prefix version is deprecated, and is therefore removed in Edge.

I have got what I think should be a fix: https://github.com/smart-facility/petajakarta-web/commit/094ee9a52c5e73328db96e99e00597a8fde4d3c0 and this now works correctly when I pretend to have a mobile device, but not desktop (and also when I emulate 9 or lower in desktop mode).

The only thing I can think of is that IE is detecting that I have a trackpad device connected not a mouse and is correctly reporting that as a touch device. I'm not at work and won't be in until Tuesday so I can't confirm by using a Windows PC with a mouse, although I strongly doubt that IE is detecting my trackpad as as pinch-to-zoom doesn't work, although this may be an artefact of the fact I am connecting via VNC to a Mac running IE11 in VMWare. I can still zoom using mouse wheel and by using two finger scroll on the trackpad (at home).

matthewberryman commented 8 years ago

Just realised there's a Windows PC at work I can RDP into. Testing there reveals yes, we still have the bug. I guess we could force it on using browser string? @benatwork99

benatwork99 commented 8 years ago

I think your fix is working correctly, but Leaflet is doing its own touch detection - https://github.com/Leaflet/Leaflet/blob/v0.7.7/src/core/Browser.js#L34 - and is detecting IE as touch-enabled.

I think Leaflet could update their detection here (checking master on Leaflet it has changed but not looking at maxTouchPoints).

In the meantime though, maybe we can use our touch detection to override Leaflet's. I'll see if we can poke into the Browser object successfully (and if so, should we use the other parts of their touch detection too?). Otherwise maybe we can set the L.NO_TOUCH flag if we're using IE and we don't think we support touch.

matthewberryman commented 8 years ago

Since we're using our self-hosted copy of leaflet we could just edit that line in leaflet to also include maxTouchPoints and msMaxTouchPoints (and then minify). If so I'm happy to work on this tomorrow and fix myself so you can focus on more pressing things.

benatwork99 commented 8 years ago

We could - although then we need to remember to port that change to Leaflet if we update it in the future. In these cases I normally prefer to keep a vendor branch (i.e. have a separately versioned copy of the third party code where we can merge in their updates with our patches - although this is a lot of overhead but useful for extensive changes) or create a README file explaining the patches made to the third-party source.

We should probably raise an issue with Leaflet and see if they'll introduce maxTouchPoints into their touch detection.

I'd think that using the configuration option for Leaflet might be a nicer way to do this as we'd be using the public configuration options, but I noticed there were a couple of other tests Leaflet did for touch that we don't and I'd want to check if they were useful for us.

FYI our current Leaflet is minified so if you're going to do this you'll want to replace it with the un-minified source from the same version.

matthewberryman commented 8 years ago

@benatwork99 I forked and then branched (from v0.7.7) of leaflet to add in the extra checks https://github.com/smart-facility/Leaflet/commit/275c41da42b6d9db5e4bd0f85242279b4c86a0d0 but it made no difference (and hence I haven't made or pushed any commits to petajakarta-web). I also tried v1.0.0-beta2 of Leaflet. @talltom and I discussed and the best way is probably to use the config option for now as you suggest. If that doesn't resolve it then it's somewhere in our code where we use our touch detection. I will look in more depth at issues & dev work ongoing on Leaflet and if none look like addressing our issue I can update / create an issue there.

benatwork99 commented 8 years ago

I think with that commit we're still getting detected as a touch device in leaflet because of the first test for 'pointer' (because IE supports the PointerEvents tests).

A handy way I found for testing this is entering L.Browser.touch in the console (and you can just inspect L.Browser too which is informative).

From the sounds of 'maxTouchPoints' we could probably use it as a definitive test, if it exists, otherwise fall back to the myriad of touch detection ways that Leaflet (and everyone else) seems to use.

I did take a skim through the Leaflet issues yesterday and saw a few shorter ones which were closed and linked to the one you found earlier. I was thinking we could make a case that looking at the value of maxTouchPoints can improve touch detection reliability if it's supported by the browser. But I'm not yet certain how common it is or it it can be relied upon in the future - see https://developer.mozilla.org/en-US/docs/Web/API/Navigator/maxTouchPoints but I haven't yet delved into those specs.

But it does seem like a good approach right now for the state of current browsers.

matthewberryman commented 8 years ago

My understanding is that the other code should be pretty portable, see https://hacks.mozilla.org/2013/04/detecting-touch-its-the-why-not-the-how/ for the ontouchstart code. I will have a play with that line in our fork of Leaflet and see how I go.

FYI IE is only 4% of public web hits to the map over the last month as per mapbox stats—this number is more reliable than Google Analytics as it's specific to the map and also includes users who use Adblock Plus (which can block Google Analytics).

matthewberryman commented 8 years ago

I've got a branch of petajakarta-web which uses the latest commit of the same-named branch of the fork of leaflet: https://github.com/smart-facility/Leaflet/commit/6878389b4e19d6bc5b19c86095340345732478a8

This now gives zoom buttons for IE 11 desktop but also adds them in on IE 11 on touch devices (unless the emulation is broken in some way, I don't have a Windows mobile device but will try and track one down tomorrow).

I have tested on Firefox, Chrome, Safari on desktop and behaviour is as expected. I have also tested on Chrome on an Android mobile and on Safari on iOS (all iOS browsers use the same rendering engine) and also saw expected behaviour.

benatwork99 commented 8 years ago

That's very mysterious about IE11 touch devices!

One of the guys in the office has a windows mobile phone, I can take a look on his device tomorrow.

matthewberryman commented 8 years ago

That'd be good, thanks.

benatwork99 commented 8 years ago

The Windows Mobile device I had access to didn't have IE11, it had edge. It was apparently a pre-release version of the OS, versions were:

Windows 10 Mobile build 10.0 Edge 25

It behaved correctly - i.e. no touch buttons on mobile. Unfortunately no IE11 on a real Windows Mobile to test here though.

matthewberryman commented 8 years ago

Since https://msdn.microsoft.com/en-au/library/hh772144(v=vs.85).aspx applies to 11 onwards (including Edge) I'm satisfied we have solved this. Would be fixed by inclusion of the modified version of leaflet. I will submit pull requests now for you and @talltom to review.