1180865 - Introduce a mechanism similar to axis locking for pinch gestures

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond]]

APZ has a mechanism related to panning called "axis locking" where if we detect that you're primarily panning horizontally or vertically, we "lock" the pan to that direction and do not allow small movements of the finger in the other direction to move the page.

Milan suggested that, as a follow-up to bug 1031443, where we allow two-fingered panning even on pages that can't be zoomed, that we introduce a similar mechanism where, on a page that *can* be zoomed, if we detect that a two-fingered gesture is primarily a pan, we "lock" it into being a pan so that the fingers moving slightly closer together or further apart doesn't zoom the page.

Comment 1

[Botond Ballo [:botond]]

We'd like a UX opinion before implementing this. Gordon, are you the right person to ask for this sort of thing? (I know you mentioned in Whistler that you mostly work on browser.html these days.) If not, I'd appreciate it if you could direct me to someone who is. Thanks!

Comment 2

[Gordon Brander :gordonb]

- Yes, I believe this is something we want. Should reduce false positives when panning sloppily.
- It will probably require tuning

I'm no longer the point person for Firefox OS work, since I've moved over to Research. I'm happy to help where needed, but probably won't be as responsive. I've NI Firefox OS UX Team so they can assign a new point person for this work.

Comment 3

[Gordon Brander :gordonb]

Francis, would you be the right point person for APZC work?

Comment 4

[Tiffanie Shakespeare [:tif]]

Passing NI to Rob.

Thanks for pinging the UX team!

Comment 5

[Botond Ballo [:botond]]

As we have general guidance to go ahead with the idea, with subsequent tuning with the help of UX (as per comment 2), I think we can go ahead and implement this.

It's not high priority, but it would make a good mentored bug.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Dropping stale needinfos. We might still want this for Fennec and (eventually) desktop. I think that once we get around to implementing it we can ping the platform UI team for some feedback on tuning.

Comment 7

[jlogandavison]

Hi. I've been having a look at this issue. Thinking I might be able to tackle it.
Can I get assigned?

I'm building now, but I think this is possible to do by modifying "AsyncPanZoomController::OnScale" (similar to bug 1031443).

Comment 8

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #7)
> Hi. I've been having a look at this issue. Thinking I might be able to
> tackle it.
> Can I get assigned?

Sure! I assigned the bug to you.

> I'm building now, but I think this is possible to do by modifying
> "AsyncPanZoomController::OnScale" (similar to bug 1031443).

Yep, that's definitely a good place to start looking.

Let me know if you have any questions or run into any issues!

Comment 9

[jlogandavison]

I've had a closer look at the code. Specifically how axis-locking is handled.

One solution I think might work involves dropping from a PINCHING state to a PANNING state somewhere in the OnScaleBegin/OnScale methods. We use some heuristic (eg: finger span vs panning distance) to detect a two finger pan
and just switch the states.

Of course this leaves us with no option to break the pinch locking later in the gesture (similar to sticky axis locking) and switching states so unexpectedly might cause unforeseen issues.

Another option is to add another state (something like PANNING_LOCKED) that we switch to and we have HandlePinching and HandlePinchingUpdate methods to detect -> set -> break the pinch locking on each call to OnScaleBegin/OnScale (mirroring how it's done in the panning code).

Any thoughts?

Comment 10

[Botond Ballo [:botond]]

Note that we already have support for two-finger panning, which is currently only activated on pages that cannot be zoomed.

It's done by remaining in the PINCHING state, but skipping the branch in OnScale() that does the zooming [1].

We'll probably want to re-use this codepath for "locked" pinching. That is, we'll want to stay in the PINCHING state, and just change the condition for skipping the 'doScale' branch in OnScale() to include the pinch being "locked".

For keeping track of whether the pinch is locked, we can add a variable similar to Axis::mAxisLocked (except this one can be in AsyncPanZoomController directly, since we don't need a separate one for each axis).

Other than that, the general idea you describe is right: run some heuristics on each pinch gesture event and decide whether to enter and exit the locked state.

[1] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/AsyncPanZoomController.cpp#1445
[2] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/Axis.h#273

Comment 11

[jlogandavison]

Attachment: patch.diff

Alright, I've come up with a working solution.

Everything is handled within a new method "APZC::HandlePinchLocking" which is called on every "APZC::onScale".

Axis locking is handled with two methods "APZC::HandlePanning" and "APZC::HandlePanningUpdate". The former being called in "APZC::OnPanBegin" and the latter being called in the "APZC::OnPan". Not much point in doing the same in this case because there seems to be no gesture information passed to "APZC::OnScaleBegin". There's not enough information to decide whether to pinch-lock  until we reach "APZC::OnScale"

----

The heuristic for deciding to pinch-lock uses two variables: the change in span between the touch points (ie: how much have we pinched); and the change in location of the focus point (ie: how far have we scrolled).

If the change in span is BELOW a given threshold and the change in focus point is ABOVE a given threshold then we treat this as a 2-finger pan.

Then (if the locking mode is STICKY) we unlock if the change in span rises ABOVE a given threshold.

---

4 new gfxPrefs are declared:

 * apz.pinch_lock.mode

   FREE | STANDARD | STICKY

 * apz.pinch_lock.scroll_lock_threshold
 * apz.pinch_lock.span_breakout_threshold
 * apz.pinch_lock.span_lock_threshold

   Our lock/breakout thresholds for pinching/panning

---
Notes for reviewer:

 * New gfxPrefs aren't showing in about:config. Should I have registered them somewhere?

 * There's currently no axis locking while doing a 2-finger pan. Should we consider implementing that too?

[APZC::OnPanBegin]: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/AsyncPanZoomController.cpp#2157
[APZC::OnPan]: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/AsyncPanZoomController.cpp#2189
[APZC::OnScaleBegin]: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/AsyncPanZoomController.cpp#1327
[APZC::OnScale]: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/AsyncPanZoomController.cpp#1360
[APZC::HandlePanning]: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/AsyncPanZoomController.cpp#2570
[APZC::HandlePanningUpdate]: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/gfx/layers/apz/src/AsyncPanZoomController.cpp#2597

Comment 12

[jlogandavison]

Attachment: pinch_lock_fsm.png

I drew up this FSM while trying to figure out the locking logic for each mode. Posting it here for the sake of review/documentation.

Comment 13

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #11)
> Created attachment 8918657 [details] [diff] [review]
> patch.diff

It looks like what you uploaded is several patch files concatenated into a single file?

Our Splinter review tool doesn't seem to handle that very well. Could you please either post each patch file as a separate attachment, or combine the patches into one (using e.g. "hg histedit") and post the combined patch file? Thanks!

>  * New gfxPrefs aren't showing in about:config. Should I have registered
> them somewhere?

Yes, for a pref to show up in about:config it needs to be listed in modules/libpref/init/all.js. There is a section in that file for APZ prefs already [1]. In addition, APZ-related prefs are listed and documented in this comment [2], which goes into generated Doxygen documentation (or at least did at some point; I don't know whether / where the genrated documentation is hosted at the moment).

[1] http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/modules/libpref/init/all.js#664
[2] http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/gfx/layers/apz/src/AsyncPanZoomController.cpp#126

>  * There's currently no axis locking while doing a 2-finger pan. Should we
> consider implementing that too?

I would consider that a low-priority feature, but would accept a patch that adds it. Please feel free to file a new issue for it (and to work on it if you're interested).

Comment 14

[jlogandavison]

Attachment: patch.diff

Sorry for the delayed response.

I've added the prefs to "all.js" and they're showing in "about:config" now.
I've also included comments for the prefs in "AsyncPanZoomController.cpp"

I've folded the commits into one patch this time. :)

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8920903 [details] [diff] [review]
patch.diff

Review of attachment 8920903 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this generally looks great! I have a few comments:

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1392,3 @@
>    // If zooming is not allowed, this is a two-finger pan.
>    // Tracking panning distance and velocity.
> +  if (!mZoomConstraints.mAllowZoom || mPinchLocked) {

Let's factor out a variable "allowZoom" here, initialized to "mZoomConstraints.mAllowZoom && !mPinchLocked". Then this condition and the one below can check "!allowZoom".

@@ +2642,5 @@
>  
> +void AsyncPanZoomController::HandlePinchLocking(const PinchGestureInput& aEvent) {
> +  float spanDistance = fabsf(aEvent.mPreviousSpan - aEvent.mCurrentSpan);
> +
> +  ParentLayerPoint focusPoint = aEvent.mLocalFocusPoint - mFrameMetrics.GetCompositionBounds().TopLeft();

There is a small problem here: to safely access mFrameMetrics, mRecursiveMutex needs to be held (see this comment [1]).

One solution would be to acquire it for the duration of this function, by doing "RecursiveMutexAutoLock lock(mRecursiveMutex);" at the beginning of the function.

However, if you look at OnScale(), you can see we already lock the mutex a few lines after the call to HandlePinchLocking(). Looking at the preceding code, I don't really see any reason why the mutex couldn't be held while running that code. So, instead of locking the mutex two times over the course of OnScale(), I would suggest doing it just once, before calling HandlePinchLocking(). (The extra pair of braces and level of indentation that starts where the mutex is currently locked can also be removed, they don't seem to serve any purpose.)

Also, notice how we already compute |focusPoint| in OnScale(). Instead of computing it again here, we can just compute it before the call to HandlePinchLocking(), and pass it in as an argument.

[1] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/gfx/layers/apz/src/AsyncPanZoomController.h#733

@@ +2645,5 @@
> +
> +  ParentLayerPoint focusPoint = aEvent.mLocalFocusPoint - mFrameMetrics.GetCompositionBounds().TopLeft();
> +  ParentLayerPoint focusChange = mLastZoomFocus - focusPoint;
> +
> +  float scrollDistanceSquared = pow(focusChange.x, 2) + pow(focusChange.y, 2);

You can use focusChange.Length() here (and then don't square scrollLockThreshold below).

@@ +2647,5 @@
> +  ParentLayerPoint focusChange = mLastZoomFocus - focusPoint;
> +
> +  float scrollDistanceSquared = pow(focusChange.x, 2) + pow(focusChange.y, 2);
> +
> +  if(mPinchLocked) {

nit: as per coding style, please put a space between 'if' and '(' throughout this function

Comment 16

[jlogandavison]

Attachment: patch.diff

New patch!

  * So I've factored out the boolean into an "allowZoom" variable.

    At one point in the "OnScale()" method I've eliminated an if statement because it simply set "doScale" to false based on "allowZoom". Seemed cleaner just to && the two.


  * Switched to "focusChange.Length()" in "HandlePinchLocking()"


  * Fixed my if statement style.


------

I've had some trouble refactoring the position of that mutex. If I move the mutex constructor call to the top of "OnScale" I'm met with deadlock warnings (and some observable crashes) when "OnScale" is called where "allowZoom == false". I've narrowed it down to the calls to "Axis<X/Y>::UpdateWithTouchAtDevicePoint(...)", but I'm not sure what to do about it.

For now I've kept things as-is and just placed a second mutex acquisition in "HandlePinchLocking()"

The suggestion of calculating "focusChange" once and passing as an argument to HandlePinchLocking() is also a little tricky for the same reason. If we want to pass in focusChange we need to calculate it before the call to HandlePinchLocking(), which means moving the "mFrameMetrics" access (and the mutex) up above the calls to "UpdateWithTouchAtDevicePoint(...)"


This all being said, I'm fairly happy with current mutex-ing arrangement.

Comment 17

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #16)
> I've had some trouble refactoring the position of that mutex. If I move the
> mutex constructor call to the top of "OnScale" I'm met with deadlock
> warnings (and some observable crashes) when "OnScale" is called where
> "allowZoom == false". I've narrowed it down to the calls to
> "Axis<X/Y>::UpdateWithTouchAtDevicePoint(...)", but I'm not sure what to do
> about it.

Hmm, good point. I didn't realize that Axis::UpdateWithTouchAtDevicePoint() cannot be called while AsyncPanZoomController::mRecursiveMutex is held.

(If you're wondering why, it's because of the following call chain:

  Axis::UpdateWithTouchAtDevicePoint()
  Axis::ApplyFlingCurveToVelocity()
  Axis::ToLocalVelocity()
  AsyncPanZoomController::ToScreenCoordinates()
  AsyncPanZoomController::GetTransformToThis()
  APZCTreeManager::GetScreenToApzcTransform()

APZCTreeManager::GetScreenToApzcTransform() acquires APZCTreeManager::mTreeLock, and doing that while AsyncPanZoomController::mRecursiveMutex is held violates the lock ordering described here [1]

It's funny how little details like this (in this case, the fact that notifying Axis of a new touch point causes it to recalculate its velocity, but its velocity is clamped to a maximum value expressed in screen coordinates, and calculating the conversion from screen coordinates to the APZC's local coordinates requires locking and traversing the tree of APZCs) can leak through and cause surprises like this :)).

> For now I've kept things as-is and just placed a second mutex acquisition in
> "HandlePinchLocking()"
> 
> The suggestion of calculating "focusChange" once and passing as an argument
> to HandlePinchLocking() is also a little tricky for the same reason. If we
> want to pass in focusChange we need to calculate it before the call to
> HandlePinchLocking(), which means moving the "mFrameMetrics" access (and the
> mutex) up above the calls to "UpdateWithTouchAtDevicePoint(...)"

I agree that we need to acquire the mutex twice, but we can still avoid recalculating "focusChange", like this:

AsyncPanZoomController::OnScale(...) {
  // ...

  ParentLayerPoint focusChange;

  {   // start scope for mutex

    // acquire the mutex the first time

    // calculate and set |focusChange|

  }  // end of scope, mutex is released

  // call HandlePinchLocking, passing in |focusChange|
  // (HandlePinchLocking no longer needs to acquire the mutex)
  
  // call UpdateWithTouchAtDevicePoint

  // for the rest of the function, acquire the mutex a second time

  // ...
}

Please also add a comment along the following lines before the calls UpdateWithTouchAtDevicePoint():

  // UpdateWithTouchAtDevicePoint() acquires the tree lock, so
  // it cannot be called while the mRecursiveMutex lock is held.

[1] https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/gfx/layers/apz/src/APZCTreeManager.h#55

Comment 18

[Botond Ballo [:botond]]

Otherwise the patch looks good, thanks! I would just like to see the change described in the previous comment.

(By the way, when uploading a patch, there is a "review" flag you can set on it. We generally use that instead of the more generic "needinfo" for reviewing patches.)

Comment 19

[jlogandavison]

Attachment: patch.diff

new patch! Calculating focus change only once and no deadlock warnings in sight. :)

Comment 20

[jlogandavison]

Attachment: patch.diff

Oops. I forgot to include the comment about UpdateWithTouchAtDevicePoint(). Fixed

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8929868 [details] [diff] [review]
patch.diff

Review of attachment 8929868 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2657,5 @@
> +    if (GetPinchLockMode() != PINCH_FREE) {
> +      float spanLockThreshold = gfxPrefs::APZPinchLockSpanLockThreshold() * APZCTreeManager::GetDPI();
> +      float scrollLockThreshold = gfxPrefs::APZPinchLockScrollLockThreshold() * APZCTreeManager::GetDPI();
> +
> +      if (spanDistance < spanLockThreshold && focusChange.Length() > scrollLockThreshold) {

Sorry, there is an additional issue here that I didn't notice before.

The preference |apz.pinch_lock.scroll_lock_threshold| is in screen inches, which means |scrollLockThreshold| is in screen pixels, but |focusChange| is in ParentLayer coordinates - there is a unit mismatch.

|focusChange| needs to be converted to Screen coordinates before it can be compared to |scrollLockThreshold|.

The conversion can be done using the function AsyncPanZoomController::ToScreenCoordinates(). That function takes two arguments, a "vector" and an "anchor". The "vector" would be |focusChange|. For the "anchor", we can use |focusPoint|. 

It's probably easiest to do the conversion in OnScale(), and have HandlePinchLocking() take the resulting ScreenPoint as an argument. Note that ToScreenCoordinates() needs to be called after mRecursiveMutex is released.

The comparison |spanDistance < spanLockThreshold| actually suffers from the same coordinate mismatch. While |spanDistance| is not annotated as being in ParentLayer units (it really should have been, its type should have been |ParentLayerCoord| rather than |float|), it is in fact in ParentLayer units, because it's calculated as subtracting |aEvent.mPreviousSpan - aEvent.mCurrentSpan|, and those are in ParentLayer units.

So |spanDistance| needs to be converted to Screen units as well. Once again this can be done using ToScreenCoordinates(). Since this is not a Point but a Coord, a couple of additional steps are necessary: to obtain a Point to pass as the "vector" argument, we can do |ParentLayerPoint(0, focusChange)|. To convert the Point returned by ToScreenCoordinates() back to a Coord, we can use Length(). For the "anchor" argument, we can once again use |focusPoint|.

Comment 22

[Botond Ballo [:botond]]

Hi jlogandavidson, I just wanted to check in here and see how things are going. Are you having any issues addressing the latest comment?

The patch is very close to being ready to commit, it just needs the (fairly minor) issue I described above fixed.

Comment 23

[jlogandavison]

(In reply to Botond Ballo [:botond] from comment #22)

Hi Botond, I'm definitely still around. Things got a bit held up when switching to a new PC.

I'll make sure to commit some time in the next week to updating the patch. Beyond the unit-mismatch issue are there other things I should be thinking about, specifically should i be writing tests for this?

Comment 24

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #23)
> Hi Botond, I'm definitely still around. Things got a bit held up when
> switching to a new PC.
> 
> I'll make sure to commit some time in the next week to updating the patch.

Sounds good, thanks!

> Beyond the unit-mismatch issue are there other things I should be thinking
> about, specifically should i be writing tests for this?

Writing some tests is definitely a good idea. However, I suggest that we do it in a separate bug, as a follow-up to this one.

Comment 25

[jlogandavison]

Attachment: patch.diff

New patch. Arguments passed into HandlePinchLocking() are now in Points/Coords in screen space.

When converting |spanDistance| to a ScreenCoord your suggestion was to use |ParentLayerPoint(0, focusChange)| as the vector. I guessed you meant |ParentLayerPoint(0, spanDistance)|, so that's what I did. Let me know if that's incorrect.

I was unsure as to whether I should change the "float" type variables (the ones local to |HandlePinchLocking()|) to make it clear which coordinate system them belong in. I changed them to |ScreenCoord|, but I wasn't totally sure that that was right type to use.

----
Happy New Year!

Comment 26

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #25)
> When converting |spanDistance| to a ScreenCoord your suggestion was to use
> |ParentLayerPoint(0, focusChange)| as the vector. I guessed you meant
> |ParentLayerPoint(0, spanDistance)|, so that's what I did. Let me know if
> that's incorrect.

Yes, sorry, that's what I meant :)

> I was unsure as to whether I should change the "float" type variables (the
> ones local to |HandlePinchLocking()|) to make it clear which coordinate
> system them belong in. I changed them to |ScreenCoord|, but I wasn't totally
> sure that that was right type to use.

Yep, that's great!

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8939338 [details] [diff] [review]
patch.diff

Review of attachment 8939338 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 28

[Botond Ballo [:botond]]

Pushed the patch to the Try server to make sure it's passing tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da5db37474be340b06053de5b503bd6affc9a236

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd8ce4f59dd
Implement pinch locking in APZC. r=botond

Comment 30

[Botond Ballo [:botond]]

The test run looked good so I pushed the patch to mozilla-inbound. It should get merged to mozilla-central automatically within the next day or so.

Comment 31

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
> Writing some tests is definitely a good idea. However, I suggest that we do
> it in a separate bug, as a follow-up to this one.

I filed bug 1428387 for writing tests for this. Feel free to work on it if you're interested!

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9cd8ce4f59dd

Comment 33

[jlogandavison]

This is awesome. Thank you so much for helping me through the process. It's really been good fun!

Yes, I'll definitely have a look at bug 1428387. GTest is a little alien to me, but I'm sure that won't be a problem.

If you have any other suggestions as to bugs that you think might be good to work on, then I'm all ears!

Again, Many Thanks!

Comment 34

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #33)
> This is awesome. Thank you so much for helping me through the process. It's
> really been good fun!

Great, thanks for your work here!

> If you have any other suggestions as to bugs that you think might be good to
> work on, then I'm all ears!

Another bug that I'm mentoring that you could give a try, if you don't mind doing something that's more refactoring than a new feature, is bug 1420512.

There's also this site where you can search all mentored bugs based on the component of Firefox and the language:

https://www.joshmatthews.net/bugsahoy/