915328 - Defect - Start screen should not scroll horizontally during cross-slide to select tiles

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect, P2)

Description

[Stephen Horlander [:shorlander]]

On the Firefox Start screen when you pull a tile down to select it the horizontal scrolling should be locked. 

Currently while performing the selection gesture horizontally scrolling still happens causing things to slide around.

I put this under Start, but really this applies to anywhere we have tiles.

Comment 1

[Jim Mathies [:jimm]]

To prevent this with apz, we need to call preventDefault on the first touchstart or touchmove.

Comment 2

[Sam Foster [:sfoster] (he/him)]

We don't know if the user intends to select a tile (cross-slide gesture) until the drag passes a (currently) 20px threshold on the cross-axis, see bug 886584 and in particular https://bugzilla.mozilla.org/show_bug.cgi?id=886584#c4
So we can give this a go and my concerns might be unfounded, but I'm worried that we may not know not to scroll until its too late to prevent it.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Sam Foster [:sfoster] from comment #2)
> We don't know if the user intends to select a tile (cross-slide gesture)
> until the drag passes a (currently) 20px threshold on the cross-axis, see
> bug 886584 and in particular
> https://bugzilla.mozilla.org/show_bug.cgi?id=886584#c4
> So we can give this a go and my concerns might be unfounded, but I'm worried
> that we may not know not to scroll until its too late to prevent it.

Once you call preventDefault, that'll be it for apz, it won't scroll the page until a new touch sequence starts. If you don't call it and wait, the page will start scrolling on the second touchmove, which content won't get until scrolling starts. Seems like we need to make a trade off.

Comment 4

[Sam Foster [:sfoster] (he/him)]

CrossSlide checks a drag (touchstart + n touchmoves) is within a 90° cone off the cross-axis to determine if the gesture is within bounds and valid. We don't have tight integration of this gesture with the rest of our touch input code so we just have to juggle these thresholds. 45° either side of the cross-axis might be too broad. If I'm on a horizontally scrolling page and I drag something up or down within 10-20° of the vertical axis, currently the page pans by that amount. That seems odd, all gestures aside. As I get closer to 30 or 45° (the edge between the cross axis and the scroll axis) I might expect some horizontal panning. Is there any way in apz or our integration of it to snap or bias the panning a bit so we get more expected behavior at the edges?

Comment 5

[Sam Foster [:sfoster] (he/him)]

Also, :shorlander are we barking up the right tree here? What I'm seeing is that on the start page, especially if I've panned over to, say, the Recent History grid, if I swipe down there the page drifts left or right a bit. The drift seems not at all in proportion to the amount of degrees off vertical my swipe might have been. I'm understanding this bug to be about fixing that. 

And the more I test, I'm more inclined to think this is a panning/scrolling bug, which can be mitigated /further/ by a tighter "cone" in the CrossSlide implementation.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Short video which shows me selecting 3 tiles with a vertical cross-slide. Notice how especially the 2nd and 3rd cause the page/scroll to jiggle and drift around: https://dl.dropboxusercontent.com/u/1719101/share/fx-start-swipe-jiggle.swf

Comment 7

[Sam Foster [:sfoster] (he/him)]

Also (so sorry for the bugspam, you are getting stream of consciousness) this isn't particular to the start page. Do the same gesture (short horizontal swipe on most vertically-scrolling web pages) and you'll see the same effect. The inertia doesn't match the gesture.

Comment 8

[Jim Mathies [:jimm]]

 (In reply to Sam Foster [:sfoster] from comment #4)
> CrossSlide checks a drag (touchstart + n touchmoves) is within a 90° cone
> off the cross-axis to determine if the gesture is within bounds and valid.
> We don't have tight integration of this gesture with the rest of our touch
> input code so we just have to juggle these thresholds. 45° either side of
> the cross-axis might be too broad. If I'm on a horizontally scrolling page
> and I drag something up or down within 10-20° of the vertical axis,
> currently the page pans by that amount. That seems odd, all gestures aside.
> As I get closer to 30 or 45° (the edge between the cross axis and the scroll
> axis) I might expect some horizontal panning. Is there any way in apz or our
> integration of it to snap or bias the panning a bit so we get more expected
> behavior at the edges?

I can file a bug about biasing against movement based on perpendicular movement. I can't promise it'll get fixed in time for whenever we release. We should try to address this in the front end for now.

Is this whole issue muted by the drop of support for tile selection in 8.1? Do we plan to do the same?

Comment 9

[Asa Dotzler [:asa]]

If the cross-slide gesture  is causing us problems that are difficult to fix, and if dropping the gesture in favor of the long press selection in 8.1 is a time saver, I'd be OK with that.

Comment 10

[Matt Brubeck (:mbrubeck)]

Axis locking (bug 892684) might make it easier to fix this, if APZC were smart enough not to "grab" the input when the gesture is along a non-scrollable axis.

Comment 11

[Matt Brubeck (:mbrubeck)]

I'm planning to work on this by making axis locking play nicely with cross-swipe.  Estimated effort p=8.

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This mostly-working patch demonstrates my basic approach.  If axis locking is enabled, and a gesture is locked to a non-scrollable axis, then AsyncPanZoomController enters the "CROSS_SWIPING" state and will ignore that gesture.

We may want to add a pref for this behavior or some other way for the Metro start page to opt into it without affecting other users (like B2G or Android).

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: Add cross-slide support to APZC

Depends on the patches from bug 892684.

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Enable cross-slide for Metro front-end

This turns on cross-slide support in Metro.  It also switches us to "sticky" axis locking so that you can "break out" of the lock by changing directions; this helps if you start in the cross direction accidentally.  Lastly, it cancels tile selection if you switch directions in mid-gesture.

Depends on the patches in bug 892684 and on "part 1" from this bug.

Comment 15

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

Comment on attachment 812379 [details] [diff] [review]
part 1: Add cross-slide support to APZC

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

Minusing for now since there's some things that need fixing and some that need discussion.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +471,2 @@
>        // May happen if the user double-taps and drags without lifting after the
>        // second tap. Ignore the move if this happens.

I don't think this comment is applicable to the CROSS_SLIDING states. I would split these case statements to a separate section with a comment like "in the CROSS_SLIDING states we don't want to consume any touchmove events for panning/zooming, so just ignore them and let the caller handle them"

@@ +762,5 @@
>  
>    double angle = atan2(dy, dx); // range [-pi, pi]
>    angle = fabs(angle); // range [0, pi]
>  
> +  if (angle < AXIS_LOCK_ANGLE || angle > (M_PI - AXIS_LOCK_ANGLE)) {

Above this I think you still need the old code, slightly modified:

if (!gCrossSlideEnabled && (!mX.Scrollable() || !mY.Scrollable())) {
  SetState(PANNING);
  return;
}

The reason for this code is that if a page is only scrollable vertically, and you do a horizontal drag on the page (i.e. the cross-slide action, but with cross-sliding disabled) you don't want to get into a horizontal axis lock, because that will prevent you from panning the page at all. Your new code will do that, I think.

Please verify what I just wrote above and if applicable add a comment to the code block that states what it's there for.

@@ +767,3 @@
>      mY.SetScrollingDisabled(true);
> +    if (gCrossSlideEnabled && !mX.Scrollable()) {
> +      SetState(CROSS_SLIDING_X);

So based on this code I guess you're still relying on Windows to actually do the cross-slide gesture detection and give you appropriate events; the code changes here are only so that APZC doesn't consume the events if they "look like" they might be a cross-slide.

This is fine for now, but it means we have two different pieces of code both implementing the cross-slide gesture detection (i.e. the code here in APZC and the code in Windows) that might not be identical. This means if they get out of sync it's possible for touches to trigger neither panning nor cross-sliding, which might be unexpected. As a concrete example, consider the case where a page is scrollable only vertically, but the user moves their finger horizontally by 20 pixels and down by 10 pixels. On B2G (where cross-slide is disabled) this will have the effect of panning the page vertically by 10 pixels. On Windows this *should* (pan the page vertically by 10 pixels) XOR (trigger a cross-slide). Which option gets chosen depends on the specific "cone" angle used to trigger the cross-slide, but exactly one of the two options should get chosen. If the axis lock angle in APZC is different from the Windows cross-slide cone action it's possible for APZC to think it's a cross-slide, and go into CROSS_SLIDING_X but for Windows to think it's *not* a cross-slide. So the user gets neither.

I think the way to fix the above scenario is to fully implement cross-slide in APZC, and use that instead of the Windows cross-slide code, if that's possible. We can add methods to GeckoContentController for HandleCrossSlideBegin(const CSSPoint& startpoint) and HandleCrossSlideEnd(const CSSPoint& endpoint) and anything else needed to properly define the interface.

@@ +858,5 @@
>  
>      float breakThreshold = AXIS_BREAKOUT_THRESHOLD * APZCTreeManager::GetDPI();
>  
>      if (fabs(dx) > breakThreshold || fabs(dy) > breakThreshold) {
> +      if (mState == PANNING_LOCKED_X || mState == CROSS_SLIDING_X) {

Is it possible to break out of a cross-slide once you're in it? Based on the docs I was reading at http://msdn.microsoft.com/en-us/library/windows/apps/hh465299.aspx I thought that once you activate a cross-slide you're in that mode until you release your finger. If so I wouldn't expect this change to be necessary here.

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> >        // May happen if the user double-taps and drags without lifting after the
> >        // second tap. Ignore the move if this happens.
> 
> I don't think this comment is applicable to the CROSS_SLIDING states.

You're right; I'll fix that.

> @@ +762,5 @@
> >  
> >    double angle = atan2(dy, dx); // range [-pi, pi]
> >    angle = fabs(angle); // range [0, pi]
> >  
> > +  if (angle < AXIS_LOCK_ANGLE || angle > (M_PI - AXIS_LOCK_ANGLE)) {
> 
> Above this I think you still need the old code, slightly modified:
> 
> if (!gCrossSlideEnabled && (!mX.Scrollable() || !mY.Scrollable())) {
>   SetState(PANNING);
>   return;
> }

Good catch.  I had meant to restore that but forgot.

> So based on this code I guess you're still relying on Windows to actually do
> the cross-slide gesture detection and give you appropriate events; the code
> changes here are only so that APZC doesn't consume the events if they "look
> like" they might be a cross-slide.
> 
> This is fine for now, but it means we have two different pieces of code both
> implementing the cross-slide gesture detection (i.e. the code here in APZC
> and the code in Windows) that might not be identical.

Nope, we are not using any Windows cross-slide gesture events.  Our JavaScript code in the start page just consumes touch events.  All it needs is to receieve touch events for swipes in one direction without disabling async panning in the other direction.  Pointer Events has this built in, but to make it work for Touch Events we need to make sure that "axis locking" kicks in even for non-scrollable axes.

We *could* introduce cross-slide gesture events at the platform level like Windows does, but I'd rather not.  I think touch events (and later pointer events) suffice.

> Is it possible to break out of a cross-slide once you're in it?

Not always, though in some cases (like on the Windows Start screen) pulling away from a cross-slide breaks out into a rearranging gesture.  But in our case I wanted to make sure you can break this mode in cases other than the about:start page, like on a normal web page.  Another solution would be to find a way to enable this mode only on about:start.

Comment 17

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

Comment on attachment 812379 [details] [diff] [review]
part 1: Add cross-slide support to APZC

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

Ok, in that case r+ with the previous comments addressed.

Comment 18

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 812380 [details] [diff] [review]
part 2: Enable cross-slide for Metro front-end

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

This patch looks fine, but with the patches applied from this bug and bug 892684 I'm finding the cross-slide gesture somtimes very difficult to achieve. Sometimes I'll get a run where I can select a load of tiles in sequence, other times a tile seems impossible to select. I'm trying more or less vertical swipes, lingering or not on the tile, starting at the top/bottom/middle of the tile, swiping fast or slow, a little or a lot. It seems the a very fast flick has most chance of succeeding and a deliberate touch->slide down never works. But there's in-betweens that sometimes do, sometimes don't

I'll r+ this bit but we may want to tweak elsewhere.

Comment 19

[Matt Brubeck (:mbrubeck)]

Attachment: part 3: don't consume event when starting a cross-slide

The problem that Sam noticed in comment 18 is a bug in the "part 1" patch above.  After calling StartPanning, it still returned eConsumeNoDefault even when beginning a cross slide.  This caused our widget code to stop dispatching events to content.  With this fix, we consume events only when actually panning.

Attaching the fix as a separate patch for review, but I'll fold it into part 1 for landing.

Comment 20

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

Comment on attachment 812897 [details] [diff] [review]
part 3: don't consume event when starting a cross-slide

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +791,5 @@
>      SetState(PANNING);
>    }
> +
> +  return IsPanningState(mState) ? nsEventStatus_eConsumeNoDefault
> +                                : nsEventStatus_eIgnore;

Add a comment here saying you don't want to consume the event that starts off a cross-slide.

Comment 21

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: Add cross-slide support to APZC (v2)

Parts 1 and 3 folded together, with review comments adddressed, and rebased to apply on top of the latest patches in bug 892684.  Carrying r=kats.

Comment 22

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/e1b2032de128
https://hg.mozilla.org/integration/fx-team/rev/e46cfd222fbd

Comment 23

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e1b2032de128
https://hg.mozilla.org/mozilla-central/rev/e46cfd222fbd