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

Don't register mouse events if pointer events are supported. #96

Merged
merged 5 commits into from
Mar 20, 2018

Conversation

samelhusseini
Copy link
Contributor

@samelhusseini samelhusseini commented Feb 9, 2018

Chrome on touch devices fires both a pointer and a mouse down (click) event when a user taps (ie: no drag).

This is problematic as it triggers event handlers that register on 'mousedown' twice. eg: in the case of field_number's numPad hitting a key triggers the handler twice (once for PointerEvent and another for MouseEvent).

I don't see any reason to register both pointer and mouse events if the former is supported.
@rachel-fenichel any thoughts? do you know of any reason to keep both events if pointer events are supported? or do we need the two event handlers. If so, we'll have to figure out some way to detect the 'tap' and only call the event handler once.

@samelhusseini
Copy link
Contributor Author

I'm afraid I don't think this is the right fix, even if we clear out pointer events, touch events are still going to be fired.
Ideally e.preventDefault and stopPropagation should be taking care of stopping the event for trickling down, so I'll keep digging..

@samelhusseini
Copy link
Contributor Author

Okay finally figured out what the issue is, don't have a solution yet, but the issue also exists in Blockly as well.

The issue only repro's on Chrome, when in an iframe, and with touch (either from the dev tools on on an Android).
(Quite the repro..)

but basically when in an iframe and on a touch device, I'm getting both Pointer and Mouse events for clicks on various elements. A good test in pxt-blockly / scratch-blocks is the numpad picker, and for Blockly just the standard number field (it pops up the alert dialog twice after one click).

@rachel-fenichel any ideas?

@rachel-fenichel
Copy link
Contributor

Oh gosh, iframes again.

For the sake of testing, this instance of Blockly is in an iframe: https://developers.google.com/blockly/

No immediate solutions. I'll have to think.

@samelhusseini
Copy link
Contributor Author

Just dropping this here as it might help, I found someone else (a different project) that ran into this issues months ago, and it looks like they haven't figured out a fix but here it is for reference:
phaserjs/phaser-ce#32

@samelhusseini
Copy link
Contributor Author

Although the Blockly hosted at https://developers.google.com/blockly/ doesn't have the changes in develop that enabled pointer events. The issue is the same with touch events where you get a touchstart -> mousedown event and e.preventDefault doesn't cancel it.
eg:
sameissue
(As you can see above, It was only one click that closed the initial dropdown and then selected the one after it).
Doing the same thing outside either not in an iframe or not using touch yields the right interaction:
notiniframe

@samelhusseini
Copy link
Contributor Author

Another repro: same setup (iframe + touch), create a variable button triggers the callback twice. repros in blockly develop.

@samelhusseini
Copy link
Contributor Author

samelhusseini commented Feb 9, 2018

I'm leaning towards the initial solution which is to handle either mouse or pointer events but not both.

@pelikhan
Copy link
Member

pelikhan commented Feb 9, 2018 via email

@samelhusseini
Copy link
Contributor Author

Tried out Firefox + touch, don't seem to be running into much trouble after dropping mouse events (when pointer events are supported)

@rachel-fenichel
Copy link
Contributor

This looks right to me. Obviously I'm still confused about iframes and how they cause these problems, but this looks like a reasonable solution since pointer events are supposed to replace mouse events, not just supplement them.

Register event if name not in touchmap. (for other events)
@rachel-fenichel
Copy link
Contributor

I just tested https://github.com/google/blockly/pull/1614/files on my github.io site and I'm having problems.

Testing here: https://jsfiddle.net/jjpxc9f3/
Test steps:

  • open the page
  • click on the variables category
  • click "create variable" button

Expected:
Create variable modal appears once. I enter a variable name and the variable is created.

Actual:
Chrome on android touchscreen or with touch simulation: Same as expected.
Firefox in normal mode: Same as expected.
Firefox in responsive mode with touch simulation: I can open the category, but clicking the button does nothing.

Firefox version: Firefox ESR 52.6.0 (64-bit)

I may disable pointer events through touch.js for this release--this is the only blocking bug right now, and I don't think we fully understand it yet.

@samelhusseini
Copy link
Contributor Author

so Firefox doesn't support PointerEvents until version 59 on desktop (which is currently in Beta).
and doesn't support pointer events at all on Android.

The responsive design mode with touch simulation isn't an accurate representation of an Android device (and I think is more trying to simulate a touch device, like say Surface Book), I tested this with Firefox 59 (beta) and it returned an object for window.PointerEvent even though it's not supported on Android.

The touch simulation issue with the Create variable button has nothing to do with the pointer events change, you can try this with develop blockly and the same issue exists. (Not saying we shouldn't investigate this)

@samelhusseini
Copy link
Contributor Author

Tested this change on Chrome Android and Firefox Android and all is well :)

@samelhusseini samelhusseini merged commit af31454 into develop Mar 20, 2018
@samelhusseini samelhusseini deleted the develop_mousepointer branch March 20, 2018 22:18
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.

3 participants