joseph / Monocle

A silky, tactile browser-based ebook JavaScript library.
http://monocle.inventivelabs.com.au
MIT License
743 stars 200 forks source link

Contact event rework #216

Closed danshultz closed 10 years ago

danshultz commented 10 years ago

This commit addresses issues with Microsoft products (IE8, Surface, etc) not handling events correctly due to incorrect event detection. It also address the github issue #204 where on some android devices, swiping fails, etc.

Swiping would fail due to a mouse event firing first and the listener would subscribe to mouse events instead of touch events.

@joseph - I know this is a bit of a revamp of the onContact method so let me know if you would like to discuss further. I've tested this on a handful of devices including IE 10, Surface, Samsung Nexus, Galaxy S, Chrome, Chrome simulating touch events, etc and I think this is a solid solution to properly supporting touch events.

I went with an approach to only listen to touch events on mobile user agents to help clean up event handling so we are not using unnecessary resources on mobile devices. We could bind to mouse events on these devices also but I feel it's unnecessary.

joseph commented 10 years ago

Dan, at first glance I like this a lot. The 'register listeners on first contact' approach was never ideal. I'm a bit concerned about the UAS regex to detect a mouse — does this cause problems on devices that have both a mouse and a touch screen? I know that Firefox on a touchscreen in particular is affected by this. I'd like to have some more testing in such an environment, if possible. I don't have a touchscreen monitor myself. If you can, please test in general, but specifically consider the case that someone with a touchscreen might want to use the touchscreen.

On the forEach polyfill, I'm going to be a bit of a stickler. Monocle proudly has no dependencies on a framework like jQuery or etc, but I don't therefore want to build a de facto framework of polyfills and stubs and utility functions. Let's write old-school, super-compatible JavaScript. So, ugly as it may be, please favour an ordinary for loop over an ES5 polyfill. I'm going to push up a branch with my edits, which is rebased off the active dev branch, position-consistency. Rebase this off that and change this PR to that base. Hopefully you'll be able to do a GH compare on my branch against yours for the next round of edits.

(The other stickler-y change is to go with separate var statements for multi-line variable declarations. The syntax is arguably more verbose, but clearer and easier to change and yes I know I'm at odds with Noders on this.)

joseph commented 10 years ago

My edits to your branch are here: https://github.com/joseph/Monocle/tree/danshultz-contact-event-rework

I've sent a PR which may or may not work for you.

danshultz commented 10 years ago

Thanks Joseph - I'll take a look over the PR.

Regarding the UAS regex to detect a mouse - it's not so much detecting a mouse, it's more identifying devices that cannot have a mouse attached.

This helps insulate things a bit against the multiple event firing (mouse events and touch events both firing, eg - touchstart -> mousemove -> touchend -> click or touchstart -> mousemove -> touchmove -> touchend -> mouseup

In the event the regex does not match, we bind to both mouse and touch handlers. This provides systems that fire both touch and mouse events to work with either set of events.

I understand the thought against the forEach polyfill although I find benefit of including an "internal" framework rolled up inside the app. It provides better readability and with most polyfills like this, the overall payload of the "app" goes down due to less lines of duplicate code, etc.

Regarding the single vs multiple var - I don't have a strong preference and frequently go back and forth. I am however against declaring all var at the top of a method. I think you gain communication when a var is declared as close to where it's actually used as possible.

I'll let you know how the merge, etc goes and I'll remove the polyfill for forEach.

--Dan

danshultz commented 10 years ago

On Windows 8, the following event firing is present for Chrome and Firefox (IE uses MSPointerEvents)

I'm going to have to try and normalize a bit more for Firefox/Chrome that are running on devices with multiple inputs. This mostly affects the "tap" action and in a bit of testing, the onConctact works as expected but does fire quite a few events. This may require a bit of throttling or debouncing to normalize things.

I'm going to push another commit with this work shortly

Taping the touch screen

Firefox Chrome Internet Explorer >= 10 ***
touchstart touchstart MSPointerDown
touchmove touchmove MSPointerMove
touchmove touchmove MSPointerUp
touchend touchend
mousemove mousemove
mousedown mousedown
mouseup mouseup
click click

Swiping or Dragging your finger across the screen

Firefox Chrome Internet Explorer >= 10 ***
touchstart touchstart MSPointerDown
touchmove touchmove MSPointerMove
touchmove touchmove MSPointerMove
touchmove touchmove MSPointerMove
touchmove touchmove MSPointerMove
touchmove touchmove MSPointerMove
touchmove touchmove MSPointerMove
mousemove touchend MSPointerUp
mousedown
mousemove
mousemove
touchmove
touchmove
mousemove
mousemove
touchmove
touchmove
touchend
mouseup
click

*\ Only listening to MSPointerEvents