462935 - Magnify (pinch) gesture should match Safari's behavior on MacOS

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tom Dyas]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
Build Identifier: Mozilla checkout as of 11/3/08

Safari only allows the zoom gesture to zoom the text in or out one level per gesture.  However, the current Firefox gestures support allows a single gesture to zoom the text in or out multiple levels.  Firefox uses a threshold value of 100 units, but it is currently unknown how those units map to the actual size of the user's finger movements.  We should match Safari's behavior and only rely upon the sign of the motion and not its magnitude for magnify (pinch) gestures.  See the documentation in dom/public/idl/events/nsIDOMSimpleGestureEvent.idl for more details.

Reproducible: Always

Comment 1

[Tom Dyas]

Attachment: Allow magify (pinch) gestures to be "latched"

The attached patch makes Firefox handle pinch gestures in a similar manner to Safari.  That is, once the user makes the first move in or out, the associated action triggers.  If the user continues making a motion in the same direction, the associated action will not trigger because it has "latched."

Comment 2

[Tom Dyas]

Attachment: Support latched gestures and make gesture thresholds and latching configurable.

I improved the patch so that latching can be turned on or off via preferences for both pinch and twist gestures.  The default is for pinch gestures to be operate in "latched" mode to match Safari's behavior.  The thresholds are also now configurable via preferences.

Comment 3

[Ed Lee :Mardak]

Comment on attachment 347071 [details] [diff] [review]
Support latched gestures and make gesture thresholds and latching configurable.

>+++ b/browser/base/content/browser.js
>@@ -693,9 +693,52 @@
>+  // Closure called for each "update" event.
>+  _doUpdate: null,
Probably don't need to explicitly declare this.. see below.

>+
>+  /**
>+   * Called at the start of "pinch" and "twist" gestures to setup all
>+   * of the information needed to process the gesture.
>+   *
>+   * @param aEvent
>+   *        The gesture event to handle
>+   * @param aGesture
>+   *        Name of the gesture to handle
>+   * @param aDefaultIsLatched
>+   *        True if the gesture should be "latched" by default
>+   * @param aDefaultThreshold
>+   *        Default threshold value if non-latched mode is in effect
>+   * @param aInc
>+   *        Command to trigger for increasing motion
>+   * @param aDec
>+   *        Command to trigger for decreasing motion
For both aInc and aDec, we can share the aGesture name to avoid discrepancy.
>+   */
>+  _setupGesture: function GS__setupGesture(aEvent, aGesture, aDefaultIsLatched,
>+                                           aDefaultThreshold, aInc, aDec) {
let inc = aGesture + "." + aInc;
let dec = aGesture + "." + aDec;

>+    let isLatched = aDefaultIsLatched;
>+    try {
>+      let prefName = "browser.gesture." + aGesture + ".latched";
Let's move the browser.gesture part into the object above and use it in all places. _prefBranch: "browser.gesture."

>+      isLatched = gPrefService.getBoolPref(prefName);
>+    } catch (ex) {
>+    }
>+
>+    if (isLatched)
>+      return this.onStartLatched(aEvent, aInc, aDec);
Create doUpdate here:
this._doUpdate = function(aEvent) this._handleLatchedUpdate(aEvent, inc, dec);
return this.onStartLatched(aEvent);

>+
>+    let threshold = aDefaultThreshold;
>+    try {
>+      let prefName = "browser.gesture." + aGesture + ".threshold";
>+      threshold = gPrefService.getIntPref(prefName);
>+    } catch (ex) {
>+    }
>+
>+    return this.onStart(aEvent, threshold, aInc, aDec);
Also create doUpdate here:
this._doUpdate = function(aEvent) this._handleUpdate(aEvent, threshold, inc, dec);
return this.onStart(aEvent);
>+  },
>+
>+  /**
>+   * Dispatch events based on the type of mouse gesture event.  For
>+   * now, make sure to stop propagation of every gesture event so that
>+   * web content cannot receive gesture events.
>    *
>    * @param aEvent
>    *        The gesture event to handle
>@@ -706,13 +749,24 @@
>     switch (aEvent.type) {
>       case "MozSwipeGesture":
>         return this.onSwipe(aEvent);
>+
>       case "MozMagnifyGestureStart":
>+        return this._setupGesture(aEvent, "pinch", true,
>+                                  100, "pinch.out", "pinch.in");
Just 100, "out", "in");
>+      
>       case "MozRotateGestureStart":
>-        return this.onStart(aEvent);
>+        return this._setupGesture(aEvent, "twist", false,
>+                                  22.5, "twist.right", "twist.left");
>+
>       case "MozMagnifyGestureUpdate":
>-        return this._handleUpdate(aEvent, 100, "pinch.out", "pinch.in");
>       case "MozRotateGestureUpdate":
>-        return this._handleUpdate(aEvent, 22.5, "twist.right", "twist.left");
>+        if (this._doUpdate != null)
>+          return this._doUpdate(aEvent);
Shouldn't need the null check here. Just unconditionally call doUpdate and don't need to worry about the else fallthrough
>+      case "MozMagnifyGesture":
>+      case "MozRotateGesture":
>+        this._doUpdate = null;
>+        return;
Shouldn't need to do this either. setupGesture call in Start should always happen, yes? Shouldn't need to clear out here.

>@@ -829,8 +883,11 @@
>    * @param aEvent
>    *        The continual motion event
>    */
>-  onStart: function GS_onStart(aEvent) {
>+  onStart: function GS_onStart(aEvent, aThreshold, aInc, aDec) {
No need to update the args here (and the comments) after the above changes

>@@ -854,8 +911,77 @@
>     // Do the gesture action when we pass the threshold and then reset motion
>     if (Math.abs(this._lastOffset) > aThreshold) {
>       this._doAction(aEvent, this._lastOffset > 0 ? aInc : aDec);
>-      this.onStart(aEvent);
>-    }
>+      this.onStart(aEvent, aThreshold, aInc, aDec);
No change here either.

>+  _sameSign: function GS__sameSign(v1, v2) {
>+    let v1_pos = (v1 > 0) ? 1 : 0;
>+    let v2_pos = (v2 > 0) ? 1 : 0;
>+    return (v1_pos ^ v2_pos) == 0; // the xor is 0 if same sign
return v1 > 0 ? v2 > 0 : v2 <= 0;
But some reason I think it can be even simpler..... maybe :p

>+  onStartLatched: function GS_onStartLatched(aEvent, aInc, aDec) {
Don't need the aInc, aDec (see below)

>+    // Setup the handler for the update events.
>+    this._doUpdate = function(evt) {
>+      return this._handleLatchedUpdate(evt, aInc, aDec);
>+    };
Not needed here anymore.

>+
>+    // Negate the sign of the delta to force the action when we call
>+    // into _handleLatchedUpdate().
>+    this._lastOffset = - aEvent.delta;
>+    return this._handleLatchedUpdate(aEvent, aInc, aDec);
We can reuse _doUpdate(aEvent) :)

Comment 4

[Tom Dyas]

Attachment: Latch gestures - v3

This patch incorporates Ed's comments.  I left in the explicit definition of "_doUpdate" since I prefer that it not be implicitly defined by usage in the code itself (even though I realize it doesn't matter in Javascript).

Comment 5

[Tom Dyas]

(In reply to comment #3)
> (From update of attachment 347071 [details] [diff] [review])
> >+++ b/browser/base/content/browser.js
> >@@ -693,9 +693,52 @@
> >+  // Closure called for each "update" event.
> >+  _doUpdate: null,
> Probably don't need to explicitly declare this.. see below.

Maybe not, but I'd rather have it "out in the open" as opposed to buried in the code.

> Shouldn't need to do this either. setupGesture call in Start should always
> happen, yes? Shouldn't need to clear out here.

Yes, the widget level code will always send the start event first.

Comment 6

[Ed Lee :Mardak]

(In reply to comment #5)
> > >+  // Closure called for each "update" event.
> > >+  _doUpdate: null,
> > Probably don't need to explicitly declare this.. see below.
> Maybe not, but I'd rather have it "out in the open" as opposed to buried in the
> code.
I was talking more about explicitly setting it to null because of the null check later on. But if we are explicitly declaring it, we'll need to set something to it. ;)

If we're targeting 3.1, probably will need tests from now on. So for testing idea.. we can set the gesture pref to an id of a command that we create just for the test.

Something like..
let cmd = commandset.appendChild(document.createElement("command"));
cmd.setAttribute("id", "testPinchIn");
cmd.setAttribute("oncommand", function() gTriggered++);

Make sure it triggers just once when dispatching multiple pinch in events.

Comment 7

[Tom Dyas]

(In reply to comment #6)
> If we're targeting 3.1, probably will need tests from now on. So for testing
> idea.. we can set the gesture pref to an id of a command that we create just
> for the test.
> 
> Something like..
> let cmd = commandset.appendChild(document.createElement("command"));
> cmd.setAttribute("id", "testPinchIn");
> cmd.setAttribute("oncommand", function() gTriggered++);
> 
> Make sure it triggers just once when dispatching multiple pinch in events.

I am assuming that this would have to be in a browser-chrome test (probably the same file where the other gesture event tests are)?

Comment 8

[Ed Lee :Mardak]

(In reply to comment #7)
> I am assuming that this would have to be in a browser-chrome test (probably the
> same file where the other gesture event tests are)?
sdwilsh: Should be right, yeah? I don't think there are any other tests that have the whole browser open for us to dynamically add commands to the main command set of the browser.

One potential problem is scope because setting an attribute on a node probably doesn't keep the same closure as the defining scope. (setTimeout("function() {}"..) vs setTimeout(function() {}..)

But the command is called with "this" as the command node, so perhaps it could just this.triggered++ and later we just check theCommand.triggered = ??

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 347099 [details] [diff] [review]
Latch gestures - v3

I'm not a browser peer, so I'm going to punt to gavin.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm fine with the patch, especially because of the prefs. I actually think that it might be useful for a pinch to zoom multiple levels, the key is ensuring that the sensitivity is such that the user has good control over the number of zoom levels being applied.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347099 [details] [diff] [review]
Latch gestures - v3

uir+ for the default of 1-zoom-per pinch to match Safari, though we might want to tweak that later based on feedback

Comment 12

[Tom Dyas]

(In reply to comment #8)
> 
> One potential problem is scope because setting an attribute on a node probably
> doesn't keep the same closure as the defining scope. (setTimeout("function()
> {}"..) vs setTimeout(function() {}..)
> 
> But the command is called with "this" as the command node, so perhaps it could
> just this.triggered++ and later we just check theCommand.triggered = ??

Scope was an issue.  I tried to have the command reference a variable in the test file but the reference failed.  Doing "this.callCount++;" for the command and checking the command element worked just as you suggested.

I'll have a version of the patch with tests posted in the next day or so.  (Your prior patch that allows changing the commands triggered by the gestures actually makes the higher-level testing easier!)  I can add tests for the swipes and non-latched gestures as well.

Comment 13

[Tom Dyas]

Attachment: Latched gestures - v4

I added tests for all three types of gestures. Fixed a bug in the handling of non-latched gestures where the motion included in the "start" event was ignored. Looking through the code, the current threshold tracking code also ignores any motion in excess of that required to trigger each action (because it resets the tracking counter to zero).  Thoughts on fixing this last bug?

Comment 14

[Tom Dyas]

Attachment: Latch gestures - v5

Fixed the bug I uncovered where motion in excess of that needed to trigger a threshold was dropped.  See the test cases I added.

This patch moves the few statements in onStart and onStartLatched into _setupGesture directly.  _handleUpdate has been changed to handle excess motion. The crossing zero trigger is necessary if the user moves a little past a threshold and then moves back the other way.

Comment 15

[Ed Lee :Mardak]

Actually, the trigger-only-once on huge motion was somewhat on purpose. Switching tabs one at a time is more predictable than sometimes jumping by one or two or three depending on how much you turn in one quick motion. You can still switch multiple tabs in a motion, but you would need to keep turning in slower gestures.

Comment 16

[Tom Dyas]

Hmm ... I guess it depends on how responsive we want the gestures to be.  I'm passing a build on to a friend.  I'll have him give me feedback on whether the twist gesture is too sensitive.

I can put back the trigger-only-once-on-huge-motion behavior for now.  I'd like to get the latching behavior in before beta2 comes out.  Your thoughts?

Comment 17

[Tom Dyas]

Attachment: Latch gestures - v6

I put the threshold handling to the way it was before.  No sense in changing this part of gGestureSupport in this patch.  Better to obtain user feedback and see if any tweaking is needed in the future.  The patch should be in its final form now.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Perhaps sdwilsh should review this, given that he reviewed the initial patch? 

It probably would have been a good idea to get review from a browser peer in bug 456520, even if only to have them be familiar with the code... having to familiarize myself with the existing gesture code means this is a harder review to complete than it normally would be.

Comment 19

[Tom Dyas]

I did originally list sdwilsh, but he punted to you.  See comment #9 above.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Ah, I see. Perhaps I should have read the bug before commenting! Mardak, looks live you've already reviewed - can you mark r+? I can do a quick look-over once you've signed off.

Comment 21

[Ed Lee :Mardak]

Comment on attachment 348498 [details] [diff] [review]
Latch gestures - v6

>+  // Closure called for each "update" event.
>+  _doUpdate: null,
Could you do a full /** * @param aEvent */ comment here with a dummy function plus describe what we want to capture in the closure: inc/dec actions and threshold.
_doUpdate: function(aEvent) {},

>+  _setupGesture: function GS__setupGesture(aEvent, aGesture, aDefaultIsLatched,
>+    try {
>+      let prefName = this._prefBranch + aGesture + ".latched";
>+      isLatched = gPrefService.getBoolPref(prefName);
>+    } catch (ex) {
>+    }
nit: "catch (e) {}" on a new line separate from the try's }

>+      this._doUpdate = function(aEvent) {
nit: don't need the {
>+        this._handleLatchedUpdate(aEvent, inc, dec);
>+      };
nit: and this };

>+      this._lastOffset = - aEvent.delta;
nit: no space between negate and var
>+      this._doUpdate(aEvent);
>+
>+      return;
nit: might as well get rid of that space and combine these two lines

>+    try {
>+      let prefName = this._prefBranch + aGesture + ".threshold";
>+      threshold = gPrefService.getIntPref(prefName);
>+    } catch (ex) {
>+    }
>+
>+    this._doUpdate = function(aEvent) {
>+      this._handleUpdate(aEvent, threshold, inc, dec);
>+    };
nit: as above
>+
>+    this._lastOffset = 0;
>+    this._doUpdate(aEvent); // start events include an initial delta
nit: comment above these two lines. maybe change the comment to "handle deltas included in start events"

>     switch (aEvent.type) {
>       case "MozSwipeGesture":
>         return this.onSwipe(aEvent);
>+
>       case "MozMagnifyGestureStart":
>+        return this._setupGesture(aEvent, "pinch", true, 100, "out", "in");
>+      
>       case "MozRotateGestureStart":
>+        return this._setupGesture(aEvent, "twist", false, 22.5, "right", "left");
>+
nit: no need for extra newlines

>-          if (node && node.hasAttribute("oncommand"))
>+          if (node && node.hasAttribute("oncommand")) {
>             // XXX: Use node.oncommand(event) once bug 246720 is fixed
>             return node.getAttribute("disabled") == "true" ? true :
>               new Function("event", node.getAttribute("oncommand")).
>               call(node, fakeEvent);
>+          }
nit: no need for this change

>-  /**
>-   * Helper function to determine if a continual motion event has passed some
>-   * threshold and should trigger some action. If the action is triggered, the
>-   * tracking of the motion is reset as if a new motion has started.
>+   * Helper function to determine if a continual motion event has
>+   * passed some threshold and should trigger some action. If the
>+   * action is triggered, the tracking of the motion is reset to
>+   * remove the threshold amount from consideration.
Is this comment change still needed/accurate? Also, are you wrapping to something other than 80 characters?

>+  /**
>+   * Determine if two integers have the same sign (positive or
>+   * negative). Assumes that neither value is zero.
nit: check line wrapping

>+  /**
>+   * Helper function to handle gestures in a "latched"
>+   * manner. Latching ensures that we only rely upon the sign
>+   * (positive or negative) of the motion and never its
>+   * magnitude. Actions are only triggered when the sign of the motion
>+   * changes for the first time and not when the motion is continued
>+   * in the same direction over multiple events.
nit: line wrapping

>+  _handleLatchedUpdate: function GS__handleLatchedUpdate(aEvent, aInc, aDec) {
>+    if (! this._sameSign(this._lastOffset, aEvent.delta)) {
nit: no space after negate, no need for {
>+      this._doAction(aEvent, aEvent.delta > 0 ? aInc : aDec);

>+    // Store the last delta value. (We do not keep track of a
>+    // cumulative value since we only care about whether the sign
>+    // changes from one event to the next in latched mode.)
nit: wrap

Attach an updated patch and gavin should be able to handle the r? But FYI, it would be unlikely for this to get into beta 2 at this point. 3.1 b3/rc1/whatever it may be, should be fine.

Comment 22

[Ed Lee :Mardak]

Comment on attachment 348498 [details] [diff] [review]
Latch gestures - v6

>+function test_removeCommand(cmd)
>+{
>+  gPrefService.setCharPref(cmd.origPrefName, cmd.origPrefValue);
>+  test_commandset.removeChild(cmd);
FYI, there's a clearUserPref(prefName) method of nsIPrefBranch.

Thanks again for the tests. :)

Comment 23

[Ed Lee :Mardak]

Actually Tom, if you haven't started updating things yet, I'll make an updated patch r?gavin and a1.9.1b2? per beltzner.

Comment 24

[Ed Lee :Mardak]

Attachment: v6.1

Updated with my nit comments

Comment 25

[Ed Lee :Mardak]

Tom, I'm assuming the back-end doesn't do any kind of filtering of events. It just passes them directly to listeners?

I'm guessing Safari does some sort of filtering because with the latching code, zooming can be kinda finicky with tiny delta values -- accidentally unzooming when you're just taking your fingers off the trackpad.

Comment 26

[Justin Dolske [:Dolske]]

(In reply to comment #25)

> I'm guessing Safari does some sort of filtering because with the latching code,
> zooming can be kinda finicky with tiny delta values

I've noticed this on my new MacBook -- I do 2-finger scrolling all the time, and often accidentally trigger a page zoom because I accidentally spread my fingers a bit.

Comment 27

[Tom Dyas]

(In reply to comment #25)
> Tom, I'm assuming the back-end doesn't do any kind of filtering of events. It
> just passes them directly to listeners?

Correct.  The backend does not filter the events.  (This is not entirely true.  The backend will filter out spurious magnify events that get sent in the middle of a rotate gesture and vice versa so that event listeners do not become confused. Apparently, this does happen on occasion as I discovered in my initial testing.  I was doing a rotate gesture and moved my finger a little bit like a zoom and one magnify event ended up in the middle of a 100+ stream of rotate events (including the begin/end events).)

> I'm guessing Safari does some sort of filtering because with the latching code,
> zooming can be kinda finicky with tiny delta values -- accidentally unzooming
> when you're just taking your fingers off the trackpad.

We might need some sort of minimum threshold.  I haven't encountered the problem in my testing, but I have big fingers and any movement of my fingers would overcome any small threshold.  Maybe 10-15 units (but adjustable via a preference)?

Comment 28

[Tom Dyas]

(In reply to comment #26)
> (In reply to comment #25)
> 
> > I'm guessing Safari does some sort of filtering because with the latching code,
> > zooming can be kinda finicky with tiny delta values
> 
> I've noticed this on my new MacBook -- I do 2-finger scrolling all the time,
> and often accidentally trigger a page zoom because I accidentally spread my
> fingers a bit.

Playing around with Safari just now.  It seems like it does have some sort of minimum threshold.

Comment 29

[Tom Dyas]

Attachment: Latch minimum threshold

This patch contains support for not triggering a latched gesture until a certain minimum threshold has been reached in order to avoid a user accidentally triggering the pinch gesture.  The minimum threshold can be changed via the "minthreshold" preference, i.e. "browser.gesture.pinch.minthreshold"  I set the default to 50, which seems to work okay on my Macbook Pro.  The threshold may need adjustment based on user testing.

Comment 30

[Tom Dyas]

I'll need to update the test cases for the minthreshold patch.  Just wanted to get the basic patch out to everyone first.

Comment 31

[Ed Lee :Mardak]

Comment on attachment 348679 [details] [diff] [review]
v6.1

Tom, shouldn't we be checking a minimum for both "normal" and "latched" behavior -- not just latched?

And the filtering out of "switching to another motion" that you mentioned.. A rotate turning into a pinch. What if the user wants to switch motions without lifting up his/her fingers? Couldn't we send a new start motion for the new gesture?

Comment 32

[Ed Lee :Mardak]

Er. nevermind me :p I was thinking about something else about what dolske said. No need for min threshold for things already looking for a threshold. ;)

Comment 33

[Tom Dyas]

(In reply to comment #31)
> 
> And the filtering out of "switching to another motion" that you mentioned.. A
> rotate turning into a pinch. What if the user wants to switch motions without
> lifting up his/her fingers? Couldn't we send a new start motion for the new
> gesture?

Removing the filtering would significantly complicate both the widget-level and DOM-level code with little added benefit.  Right now, the invariant is that DOM-level code only sees one active gesture at a time consisting of a start event, multiple update events, and then a final wrap-up event.  Breaking this invariant requires information that the undocumented API currently does not have (at least to my knowledge).

Every gesture is marked by calls to the "beginGestureWithEvent" and "endGestureWithEvent" method calls.  The widget-level code keeps track of these begin/end method calls and associates them with a single active gesture.  Cocoa (the windowing library) does not interleave the begin/end calls. The only way I have found to determine the active gesture is to wait for the first "magnifyWithEvent" or "rotateWithEvent" method call that occurs after "beginGestureWithEvent."  (This is when the start event is sent, not upon beginGestureWithEvent.)  It will make for *very* complicated code if I can't rely on "endGestureWithEvent" as the end marker for the active gesture. I would have to detect switches between gestures and then send the roll-up event for the previous gesture and send a start event for the new one.

Moreover, the situation didn't seem to be "switch to another motion" but rather a "spurious method call." In a stream of 100+ "rotateWithEvent" method calls, I saw 1 or 2 "magnifyWithEvent" method calls triggered by me moving my fingers slightly toward each other even while continuing the twist motion.  Throwing out the current state of the twist because it "ended" will confuse users I believe.  And supporting multiple gestures at a time is currently not possible as it is not possible to associate endGestureWithEvent with more than one gesture at a time as Cocoa will only ever have one begin/end sequence active at a time.

I tried to avoid the filtering but I added it in the interest of making the API easy enough to use.  Hopefully Apple will document the API and/or improve it.  This is the reason why web content is not allowed to see the gesture events.  We can improve the API to our heart's content in the future once we have more information on the system's API.

Comment 34

[Ed Lee :Mardak]

Attachment: v7

Incorporates the "minimum" threshold by just using the threshold value instead of having a separate minimum one for latched gestures. I also made the gesture behavior even-more-Safari-like by allowing gestures to be canceled out and go in the other direction like Tom's original latching code.

E.g., you can zoom in then zoom out back to the starting zoom then zoom out one more time.

But instead of hard coding it to 1 level for "latched", you can customize it to be any number. I've updated the testcase to test for this behavior.

Comment 35

[Tom Dyas]

(In reply to comment #34)
> Created an attachment (id=348944) [details]
> v7
> 
> Incorporates the "minimum" threshold by just using the threshold value instead
> of having a separate minimum one for latched gestures. I also made the gesture
> behavior even-more-Safari-like by allowing gestures to be canceled out and go
> in the other direction like Tom's original latching code.

With the new default threshold of 250, I was unable to trigger a zoom.  I had to lower the threshold in order to be able to make use of the pinch gesture.  (I lowered it to 50 in my case but I didn't do any testing of what would be the best value.)  Typo?

Comment 36

[Tom Dyas]

Also, even with "actions" set to 1, a single pinch gesture can now sometimes trigger multiple zoom ins or outs, which means the "latched" behavior doesn't exist anymore with this patch (at least on my system).

Comment 37

[Ed Lee :Mardak]

Are you talking about multiple (2) zoom outs after you do 1 zoom in? It'll let you revert the zoom back to the original then do another in the same direction. Or are you talking about 2 zoom in's from the starting position.

Comment 38

[Tom Dyas]

It will let me zoom out and then go back the other way and then do two zoom ins.  (From comment #34, this seems to be intended?)  I made the same gesture in Safari and it zoomed out and let me cancel the zoom out when I went back the other way but didn't then do another zoom in.  (I used the full vertical distance of my trackpad for the pinch.)  Thus, at least on my system (MBP early 2008), the v7 patch does not match Safari's behavior.  I have Safari 3.2 installed.

Another issue is that it is hard to know when to "stop" with this patch so that you only cancel the first zoom in or zoom out and not go into the opposite type of zoom.  Anyone with big fingers (like myself) is going to have a problem with the sensitivity of this patch as subtle finger motion is not exactly their (my) forte.

Also, by relying on specific thresholds, the v7 patch defeats the purpose of the latching behavior, which was to avoid depending the magnitude of the pinch motion.  (While the minthreshold patch does use a threshold value, the value is small and only intended to prevent errant finger movements from triggering a gesture.)

I'd prefer the minthreshold version of the patch make it into the tree.  We should revisit the issue in the future once users have a chance to try out the gesture support.

Comment 39

[Ed Lee :Mardak]

(In reply to comment #26)
> I've noticed this on my new MacBook -- I do 2-finger scrolling all the time,
> and often accidentally trigger a page zoom because I accidentally spread my
> fingers a bit.
Dolske, I'm assuming you're talking about trunk and not the patches in this bug? If so, the threshold on trunk is 100.

Tom, so according to that, even 100 isn't enough. Doing a latched behavior with say 5 threshold would probably just cause things to keep zooming in and out frantically.

(In reply to comment #38)
> It will let me zoom out and then go back the other way and then do two zoom
> ins.  (From comment #34, this seems to be intended?)
Yeah, I made the patch more like your initial firefox gesture patch.

> the v7 patch does not match Safari's behavior
Yeah, I see that too, so perhaps we'll go with the "do this at most X times in the initial direction"

> Another issue is that it is hard to know when to "stop" with this patch so that
> you only cancel the first zoom in or zoom out and not go into the opposite type
> of zoom.
Curious, what threshold did you set after you changed it from 250?

Comment 40

[Tom Dyas]

(In reply to comment #39)
> > Another issue is that it is hard to know when to "stop" with this patch so that
> > you only cancel the first zoom in or zoom out and not go into the opposite type
> > of zoom.
> Curious, what threshold did you set after you changed it from 250?

50

Comment 41

[Tom Dyas]

(In reply to comment #39)
> (In reply to comment #38)
> > It will let me zoom out and then go back the other way and then do two zoom
> > ins.  (From comment #34, this seems to be intended?)
> Yeah, I made the patch more like your initial firefox gesture patch.

Which was before I opened this bug.  :)

> > the v7 patch does not match Safari's behavior
> Yeah, I see that too, so perhaps we'll go with the "do this at most X times in
> the initial direction"

That should cover the pinch case.  However, for the twist case, you wouldn't want to do that since the user should be able to rotate freely in both directions.

Regardless, we shouldn't overgeneralize the gesture support.  I see no problem in having separate code paths (like in the previous patches) for latched and non-latched behavior.  v6.1 + minthreshold works fine.  Not as exciting as multiple zooms from one pinch, but I think simpler is better in this case!

Comment 42

[Ed Lee :Mardak]

Attachment: v8

Restores latching behavior to only allow motion in one direction and back. Reverted testcase behavior and refactored it so it doesn't require changing each ok() check if you insert/remove something in between.

Comment 43

[Tom Dyas]

(In reply to comment #42)
> Created an attachment (id=349588) [details]
> v8
> 
> Restores latching behavior to only allow motion in one direction and back.
> Reverted testcase behavior and refactored it so it doesn't require changing
> each ok() check if you insert/remove something in between.

Pinches now operate as they do under Safari.

One nit:  I believe the 150 threshold needs to be lower so the pinch is more sensitive. (I have to pinch over most of the vertical distance of my trackpad.)  50 seemed like a decent number in the ad hoc testing I just did.

Comment 44

[Tom Dyas]

Just wanted to add:  v8 patch looks good otherwise so let's get into the tree if we can.  (From looking at trunk, we may have missed 3.1b2 it seems.)

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 349588 [details] [diff] [review]
v8

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+          return command;

Why are you returning command? None of the callers seem to care.

>+    ["UP", "RIGHT", "DOWN", "LEFT"].forEach(function(aDir) {
>+      if (aEvent.direction == aEvent["DIRECTION_" + aDir])
>+        this._doAction(aEvent, ["swipe", aDir.toLowerCase()]);
>+    }, this);

This is silly - the switch might not be as concise or easy to add values to, but it's more efficient to run, and you're not going to be adding any more values here any time soon :)

I didn't look at the tests.

Comment 46

[Ed Lee :Mardak]

Attachment: v8.1

Adds customizability for pinch/rotate gestures and a bunch of tests.

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 350300 [details] [diff] [review]
v8.1

a191=beltzner

Comment 48

[Ed Lee :Mardak]

Attachment: v8.1.1

Just updating the commit message

Comment 49

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/f77eb57bf63d

Comment 50

[Aakash Desai [:aakashd]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f77eb57bf63d


tests were pushed to testsuite as well...

...can we get a verification that the testcases added to testsuite are green?

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There was no backout or any comment which mentioned that we fail in those tests. So we assume everything is fine. Marking as verified for trunk and 1.9.1.