Support zoom on pinch, using both pointer and touch events #42

samelhusseini · 2017-08-04T22:54:11Z

First of all, browser support for touch gestures is crazy and all over the place.

IOS support gesture apis, which returns the scale of the pinch through the event, and the browser handles all the complex maths.
Android and most browsers on Surface / Surface books (except Edge) support touch events.
Edge support pointer events. (It seems most browsers are leaning towards adopting this standard but this hasn't quite made it)

All have their own glory and variations in the amount of flexibility they give the developer in tweaking the gesture.

I've implemented all three. Favouring the IOS gestures when on an IOS device. Using pointer events when that that is supported, and touch events in other cases.
Note: there are still some fine tunings that can be done, but this is a pretty good first attempt.

@rachel-fenichel this might be something to consider bringing to mainline blockly.

rachel-fenichel · 2017-08-04T23:10:46Z

@MicrosoftSam I'm definitely interested, and will take a look.

In the past we've strongly avoided platform-specific code because increasing our number of code paths made testing difficult. The move to Gestures may help because it's now a mapping onto the same underlying code.

Note that Apple has stated that Safari will not support pointer events. The pointer events polyfill exists to fill that gap but bloats the library.

rachel-fenichel · 2017-08-04T23:35:07Z

From a quick glance at your code, I think it's in the wrong place for longer-term merge stability. It should be in gesture.js, or (better) you should create a class that extends from gesture.js and has all of this code.

How does the new code interact with drags or pinches that start on blocks and fields?

I expect you've also got two listeners bound for touch start, move, and end, if you left the normal blockly listeners in. In a touch environment Blockly binds events for both mouse and touch when you ask it to bind events for things like mouseDown.

samelhusseini · 2017-08-05T00:20:26Z

Thanks for the quick feedback @rachel-fenichel, I'll look into splitting this out a little and moving it to a new file.
As for your question about interaction with the current events, I ran into this issue with pointerevents as well. Both events are handled (hence why it's important to make sure I didn't call preventDefault), but the difference is the pinch gesture event is only relevant if there are two touches. So in some cases you could still zoom and drag at the same time, but I thought that was a fair side effect. I generally called cancel current gesture when I detected more than two touches.

samelhusseini · 2017-08-05T00:21:50Z

The IOS gesturechange events don't interfere with any of the Blockly code which is nice.

pelikhan · 2017-09-12T17:31:59Z

@MicrosoftSam status on this?

samelhusseini · 2017-09-12T17:58:18Z

Need to do some refactoring, haven't had a chance to look into this, will look into it in the next couple of weeks.

samelhusseini · 2017-12-29T21:49:49Z

@rachel-fenichel this is ready.

A summary of the changes made:
Since Blockly already splits all touch events into multiple events, this worked out well for converging the touchevent and pointer events path (as pointer events are handled one by one).
I dropped support for the IOS specific gesturechange and gestureend events, touch events were enough for pinch to zoom support.

As advised, I've created a new touch_gesture file that extends gesture.js and added the support for pinch to zoom there.

Here's the basic idea of how touch_gesture works: when a gesture is created, it adds a handler for another touchstart / pointerdown. This handler passes /opt_noCaptureIdentifier/ to ensure all events are handled (including those that don't meet the checkTouchIdentifier check. This handler is watching in order to convert a gesture into a "multi-touch" event.
As with gesture.js it also adds handlers for move and end, but same as above, it passes noCaptureIdentifier to receive all events (needed for multi-touch).

I've made sure this doesn't conflict with dragging. If at any point a drag is going on, pinch to zoom is ignored. That is for both a workspace drag and a block drag.
Since the start, move and up events are handling all events (opt_noCaptureIdentifier), I've had to include Blockly.Touch.shouldHandleEvent in these event handlers to ignore multi-touch events in cases where we don't want to handle it (ie: when dragging a block or the workspace).

This PR also includes changes to support pointer events (I hadn't yet integrated the change from master to develop), the change solves it correctly. Handling checkTouchIdentifier to support pointer id.

I've also handled long hold properly here, with pointer event support. Fixes https://github.com/Microsoft/pxt-blockly/issues/90

When you get a chance, it'd be great if you could review this, and please ask me all the questions for anything that doesn't make sense.

This change passes eslint.

This change has been tested on:

Surface Book (IE, Edge, Chrome, Firefox)
IOS 10 (iPad, iPhone)
Android (Chrome, Firefox for Android)
Mac (Chrome, Safari)

…into zoomonpinch

rachel-fenichel

I think this will work!

Once it's in we'll probably want to do some tweaking on how we handle different sequences of touch starts and stops (e.g. one touch starts, second touch starts, do a pinch, second touch ends, second touch starts again). But I think that can easily be done within the framework you currently have.

I have a bunch of questions to make sure I understand everything, a few style nits, and some places where I think there's dead code.

I also want to fully understand what coordinate system we're working in for the touch points, and to make sure that anything expensive to compute (e.g. metrics) gets cached after the first call to it. Expect more comments on that.

rachel-fenichel · 2018-01-05T23:09:35Z

core/touch_gesture.js

+ // store the pointerId in the current list of pointers
+ this.cachedPoints[pointerId] = this.getTouchPoint(e);
+ var pointers = Object.keys(this.cachedPoints);
+ // If two pointers are down, check for pinch gestures


Please confirm: the check is actually just that there are exactly two pointers down. There are no other checks (e.g. not needing to have a series of movements that looks like a pinch in order for the pinch to officially start).

Yes, if two pointers are down, we assume you are trying to zoom. In the future, if there are more multi-touch scenarios supporter this can be altered.

rachel-fenichel · 2018-01-05T23:11:52Z

core/touch_gesture.js

+ var pointers = Object.keys(this.cachedPoints);
+ // If two pointers are down, check for pinch gestures
+ if (pointers.length == 2) {
+ var points = {


If the points in the cachedPoints array are already goog.math.coordinates, I would write this as

var point0 = this.cachedPoints[pointers[0]]; var point1 = this.cachedPoints[pointers[1]]; this.touchDistance_ = goog.math.Coordinate.distance(point0, point1);

rachel-fenichel · 2018-01-09T01:22:19Z

core/touch_gesture.js

+
+ /**
+ * A map of cached points used for tracking multi-touch gestures.
+ * @type {boolean}


Not boolean. Maps from string (or number?) to point in ??? coordinate system.

Yep, not the right type.

rachel-fenichel · 2018-01-09T01:22:32Z

core/touch_gesture.js

+ this.cachedPoints = {};
+
+ /**
+ * A scale value tracking the previous gesture scale.


What's the range of this number?

I've seen it between 0 and 8.0

rachel-fenichel · 2018-01-09T01:27:37Z

core/touch_gesture.js

+ * @type {boolean}
+ * @private
+ */
+ this.cachedPoints = {};


Style: name ends in an underscore if it's private.

rachel-fenichel · 2018-01-09T01:33:19Z

core/touch_gesture.js

+};
+
+/**
+ * Bind gesture events


Add a note in the comment that you're overriding the Gesture definition of this function.

In that note, say that you're binding the same functions for onMoveWrapper_ and onUpWrapper_, but that the opt_noCaptureIdentifier part is different.

rachel-fenichel · 2018-01-09T01:39:13Z

core/touch_gesture.js

+ * @package
+ */
+Blockly.TouchGesture.prototype.bindMouseEvents = function() {
+ this.onStartWrapper_ = Blockly.bindEventWithChecks_(


Add an onStartWrapper_ property in the TouchGesture constructor (and document it).

rachel-fenichel · 2018-01-09T01:43:57Z

core/touch_gesture.js

+ * @package
+ */
+Blockly.TouchGesture.prototype.handleStart = function(e) {
+ if (Blockly.Touch.isTouchEvent(e) && !this.isDragging()) {


Slight improvement in clarity:

if (this.isDragging()) { // A drag has already started, so this can no longer be a pinch-zoom. return; } if (Blockly.Touch.isTouchEvent(e)) { ... }

Alternatively, you could uninstall this handler when a drag starts. We can check how involved that would be.

rachel-fenichel · 2018-01-09T02:05:42Z

core/touch_gesture.js

+ if (this.onStartWrapper_) {
+ Blockly.unbindEvent_(this.onStartWrapper_);
+ }
+ if (this.onGestureChangeWrapper_) {


What are onGestureChangeWrapper and onGestureEndWrapper? I don't see them being set anywhere.

Yep, dead code (was there when I had was handling gestureChange and gestureEnd for IOS)

samelhusseini · 2018-01-09T05:53:15Z

Updated with PR feedback, found a few more style nits and tried to simplify the logic in handleMove.

rachel-fenichel · 2018-01-09T22:46:35Z

core/touch.js

- // e was a touch event. It needs to pretend to be a mouse event.
- e.clientX = e.changedTouches[0].clientX;
- e.clientY = e.changedTouches[0].clientY;
+ if (e.changedTouches) {


Why the additional checks? Is it to handle a difference between touch events and pointer events? If so, add a comment.

Yes and will do.

rachel-fenichel · 2018-01-10T01:05:23Z

core/touch_gesture.js

+ * @param {!Event} e The event that kicked off this gesture.
+ * @param {!Blockly.WorkspaceSvg} creatorWorkspace The workspace that created
+ * this gesture and has a reference to it.
+ * @constructor


Missing annotation: @extends {Blockly.Gesture}

Within the annotation the whole class can disappear during compression.

Good to know.

rachel-fenichel · 2018-01-10T01:11:03Z

core/touch_gesture.js

+};
+
+/**
+ * Handle a mouse down, touch move, or pointer move event.


Update comment and param: handle a mouse down, touch start or pointer down event.

Ah nice catch.

rachel-fenichel · 2018-01-10T01:29:50Z

core/touch.js

+};
+
+/**
+ * Check whether a given event is a touch event.


is a touch event or pointer event.

rachel-fenichel · 2018-01-10T01:32:27Z

core/touch_gesture.js

+
+/**
+ * Whether this gesture is part of a mulit-touch gesture.
+ * @return {boolean} whether this gesture was a click on a workspace.


Fix return description. Nit on line above: "multi-", not "mulit-".

rachel-fenichel · 2018-01-10T01:37:28Z

core/touch_gesture.js

+ if (!this.startWorkspace_) {
+ return null;
+ }
+ var metrics = this.metrics_ || (this.metrics_ = this.startWorkspace_.getMetrics());


Why do you need the metrics? All of the work that you're doing with these pointer locations involves deltas, so you shouldn't need the workspace info. There is on call to Blockly.utils.mouseToSvg (line 248), but that uses the event directly and not the stored coordinate.

I think you might be right, since the touch point is all relative, the distance can be calculated irrespective of the workspace absolute position. I think I needed the metrics at some point for the zoom, but as you mentioned I'm using mouseToSvg above instead.

rachel-fenichel · 2018-01-10T01:40:04Z

core/touch_gesture.js

+
+ if (this.previousScale_ > 0 && this.previousScale_ < Infinity) {
+ var gestureScale = scale - this.previousScale_;
+ var delta = gestureScale > 0 ? gestureScale * 5 : gestureScale * 6;


Can you explain this line?

Yeah this line is interesting, should probably be changed to use constants instead of inline values, but this is where the calibration happens.

gestureScale gives us a scale but it's too small for a delta, so I use a multiplier to scale it so something that worked well (after doing some calibration on a bunch of touch devices). I'm scaling it differently whether it's a zoom in or a zoom out.

I think for starters, the multipliers should be moved to constants, and we can continue to calibrate these values to something that works well. These haven't been perfected yet.

rachel-fenichel · 2018-01-10T01:46:12Z

core/touch_gesture.js

+ var startDistance = this.startDistance_;
+ var scale = this.touchScale_ = moveDistance / startDistance;
+
+ if (this.previousScale_ > 0 && this.previousScale_ < Infinity) {


This will always fail on the first call because previousScale was 0. Intentional?

Yes. We need the previous scale to figure out whether the intention is a zoom in or out.

rachel-fenichel · 2018-01-10T01:46:48Z

core/touch_gesture.js

+ * @type {number}
+ * @private
+ */
+ this.previousScale_ = 0;


This is the ratio between the starting distance between the touch points and the most recent distance between the touch points.
Scales between 0 and 1 mean the most recent zoom was a zoom out.
Scales above 1.0 mean the most recent zoom was a zoom in.

rachel-fenichel

Looks good! One remaining nit (periods at the ends of comments) and it's done!

I'm stoked to get this into Blockly as well so we can start tweaking it and tuning the behaviour.

rachel-fenichel · 2018-01-10T21:48:34Z

core/touch_gesture.js

+goog.inherits(Blockly.TouchGesture, Blockly.Gesture);
+
+/**
+ * A multiplier used to convert the gesture scale to a zoom in delta


Nit: end comment with period, here and for the ZOOM_OUT_MULTIPLIER.

samelhusseini · 2018-01-10T22:02:45Z

Yay! Thanks for reviewing!

Sam El-Husseini added 2 commits August 4, 2017 15:37

Support zooming on pinch using gesture, pointer and touch events

eeb5ec1

Remove IE10

139fc7b

samelhusseini requested a review from guillaumejenkins August 4, 2017 22:54

msftclas added the cla-already-signed label Aug 4, 2017

rachel-fenichel mentioned this pull request Aug 25, 2017

Support pinch-to-zoom google/blockly#1295

Closed

msftgits removed the cla-already-signed label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 27, 2017

Sam El-Husseini added 10 commits November 14, 2017 10:41

Merge branch 'develop' into zoomonpinch

b1676d6

Create a touch gesture file, in hope to move code into it.

b17a9a7

Merge master

d1c7d61

Rebuild

925da14

Move all multi-touch gestures to TouchGesture.js

db84b88

Fix long hold for pointer events

32adfee

Fix workspace drag with multi-touch

0bb1815

Add /*opt_noCaptureIdentifier*/ comment

8544209

minor comment fix

7e524fb

Better comments

5708096

Re-build

f74ea6e

samelhusseini requested review from pelikhan and removed request for guillaumejenkins December 29, 2017 21:52

Calibrate a litte.

d71fb6e

samelhusseini mentioned this pull request Dec 29, 2017

Support Blockly pinch to zoom gestures microsoft/pxt#3682

Merged

Better naming.

fc1b932

Sam El-Husseini added 2 commits January 1, 2018 22:54

Merge branch 'zoomonpinch' of https://github.com/Microsoft/pxt-blockly …

27d3ac1

…into zoomonpinch

Update build

4cf5369

rachel-fenichel reviewed Jan 9, 2018

View reviewed changes

Updated with PR comments.

0425a59

Sam El-Husseini added 2 commits January 8, 2018 22:26

Merge develop

ed8d1bb

Rebuild

6bdae25

rachel-fenichel reviewed Jan 10, 2018

View reviewed changes

Sam El-Husseini added 3 commits January 9, 2018 23:45

More PR feedback

8e5d6a5

Merge branch 'develop' into zoomonpinch

40c1902

Merge branch 'develop' into zoomonpinch

5f77105

samelhusseini changed the title ~~Support zooming on pinch using gesture, pointer and touch events~~ Support zoom on pinch gesture using both pointer and touch events Jan 10, 2018

samelhusseini changed the title ~~Support zoom on pinch gesture using both pointer and touch events~~ Support zoom on pinch, using both pointer and touch events Jan 10, 2018

rachel-fenichel approved these changes Jan 10, 2018

View reviewed changes

Fix last remaining nit and re-build.

5615999

samelhusseini merged commit d3a0d90 into develop Jan 10, 2018

samelhusseini deleted the zoomonpinch branch January 10, 2018 22:05

samelhusseini mentioned this pull request Jan 10, 2018

Pinch to Zoom support in master #94

Merged

Support zoom on pinch, using both pointer and touch events #42

Support zoom on pinch, using both pointer and touch events #42

Conversation

samelhusseini commented Aug 4, 2017

rachel-fenichel commented Aug 4, 2017

rachel-fenichel commented Aug 4, 2017

samelhusseini commented Aug 5, 2017

samelhusseini commented Aug 5, 2017

pelikhan commented Sep 12, 2017

samelhusseini commented Sep 12, 2017 • edited Loading

samelhusseini commented Dec 29, 2017

rachel-fenichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samelhusseini Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samelhusseini commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samelhusseini Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachel-fenichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samelhusseini commented Jan 10, 2018

samelhusseini commented Sep 12, 2017 •

edited

Loading

samelhusseini Jan 9, 2018 •

edited

Loading

samelhusseini Jan 10, 2018 •

edited

Loading