1222524 - Consider making axis-lock only breakable by the apzc that initiated it and only on a scrollable axis

Status: RESOLVED INCOMPLETE

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

A problem we have in FirefoxOS is that it's too easy to break axis-lock on the home screen and end up with a very visually confusing/unpleasant interaction.

I've been thinking about this for a while, and short of adding some kind of CSS to express to the browser our intention (which would likely take a very long time to get agreed and done), I think we could implement a more sensible default behaviour;

I suggest that axis-lock can only be broken by the apzc that initiated it in the first place, and only if it's scrollable on both axes. This would mean in the case of the home-screen that as soon as vertical axis lock was initiated by the apps or pinned pages panels, you would not be able to scroll horizontally to switch panels.

Similarly, as soon as horizontal axis-lock is initiated when switching panels, you would not be able to scroll vertically to scroll a panel.

Things that might be tricky:
- The FxOS browser uses a scrollgrab container for the auto-hiding/showing toolbar. We may need to disable axis-lock on scrollgrab apzcs, or at least disable this new behaviour.
- We likely want to css scroll-snap any containers in the overscroll hand-off chain that can't be scrolled on the locked axis - otherwise, you could initiate vertical axis-lock on a panel while the outer, horizontally scrolling panel has been partially scrolled.

Botond points out on IRC that the axis-lock break happens here: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp#2154

I'm taking this, with the intention of starting on it next week.

Comment 1

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #0)
> Things that might be tricky:
> - The FxOS browser uses a scrollgrab container for the auto-hiding/showing
> toolbar. We may need to disable axis-lock on scrollgrab apzcs, or at least
> disable this new behaviour.

Discussed this over IRC. I think that as long as we're doing the check ("are you scrollable on both axes?") on the event-target APZC (that is, the APZC whose HandlePanningUpdate() method is being called) rather than the first APZC in the handoff chain, we should be fine. (In the browser, the event-target APZC will be the APZC for the page or one of its subframes, while the first APZC in the handoff chain will be the scrollgrab container.)

Comment 2

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

That was both much easier and much more effective than I thought it'd be... Huzzah!

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8685023 [details] [diff] [review]
0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

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

Cool, glad to hear it's working well!

::: gfx/layers/apz/src/OverscrollHandoffState.cpp
@@ +129,5 @@
> +          apzc->RequestSnap();
> +        }
> +        break;
> +      default:
> +        break;

MOZ_ASSERT(false); break;

Comment 4

[Chris Lord [:cwiiis]]

There's some oddness going on I think caused by the snapping back apzcs that are locked on both axes, will try to resolve that today.

Comment 5

[Chris Lord [:cwiiis]]

Current issue I'm having is that the requested snap-back can kill panning altogether - but if you don't do it, you can end up with apzs that should snap that haven't, and also it doesn't feel as good.

This seems to happen when the apz that initiates the lock is not at the end of the chain, I think... I'm guessing getting the scrollTo from content is just clobbering state.

Still messing with this...

Comment 6

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

Significant change, requires re-review.

To correctly snap an apz that has been locked out from scrolling, I had to introduce another state (well, two, one for each axis) - When panning is locked and an apz can no longer scroll, we request a snap. If the snap is received while scrolling with axis-lock, we enter this new state, run the animation and if the animation completes, we restore the previous snapped state. If touch ends before the animation is completed, we cancel the animation and it is otherwise treated the same as a touch-end for the pan-locked state.

Behaviourally, this means if you start scrolling a container that is only scrollable on one axis, but we end up locking to the other axis, that container will snap on the opposing axis while the user is still able to scroll on the locked axis.

In real terms, this means if you enable home-screen paging, scroll diagonally from the apps screen so that both containers scroll, but then horizontally so it ends up locking horizontally, the apps container will correctly snap to a boundary.

I believe this now achieves the behaviour UX desired without sacrificing behaviour elsewhere.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8685965 [details] [diff] [review]
0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

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

I'm going to have to play around with this on a phone to try and wrap my head around it.

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> I'm going to have to play around with this on a phone to try and wrap my
> head around it.

Not able to get b2g running at the moment :( Filed bug 1223934.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
> Not able to get b2g running at the moment :( Filed bug 1223934.

I was able to get b2g running, and after fixing a locking issue (HandlePanningUpdate() needs to acquire mMonitor before calling CanScroll()), play around with the patch a bit.

If I understand correctly what it does, it allows an APZC to be animated _while_ it's being panned (more specifically, to be snapped back along one axis while it's being panned, with axis lock enabled, along the other). That's pretty cool; it's also a new concept in APZC, as prior to this, animating and panning were mutually exclusive. I'll have to think some more about whether this breaks any assumptions made by APZC code.

Comment 11

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Botond Ballo [:botond] from comment #9)
> > Not able to get b2g running at the moment :( Filed bug 1223934.
> 
> I was able to get b2g running, and after fixing a locking issue
> (HandlePanningUpdate() needs to acquire mMonitor before calling
> CanScroll()), play around with the patch a bit.
> 
> If I understand correctly what it does, it allows an APZC to be animated
> _while_ it's being panned (more specifically, to be snapped back along one
> axis while it's being panned, with axis lock enabled, along the other).
> That's pretty cool; it's also a new concept in APZC, as prior to this,
> animating and panning were mutually exclusive. I'll have to think some more
> about whether this breaks any assumptions made by APZC code.

You understand correctly, and thanks - fingers crossed on that!

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8685965 [details] [diff] [review]
0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

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

r+ with a few comments/requests:

  - HandlePanning and HandlePanningUpdate can be called while processing
    both touch events, and pan-gesture events. The latter are used on Mac
    when scrolling with the trackpad.

    We don't currently having axis-locking enabled on desktop, but we could
    in the future, and so we should test the interaction between this
    feature and pan-gesture events. Could you test this please, and make
    any necessary adjustments to the APZC functions that handle pan-gesture
    events (e.g. OnPan())? To do this, set the pref "apz.axis_lock.mode"
    to 1 (standard mode) or 2 (sticky mode).

    If you don't have access to a Mac machine, Kats has offered to test it
    if you prepare a Try build with the patch and the pref set.

  - Kats and I agree that due to the fairly risky nature of this patch,
    it should land on trunk only (not the 2.5 branch), so we have an
    opportunity to discover and iron out any side effects it may cause.

  - Please file a follow-up about the issue I brought up on IRC about it being 
    easy to unintentionally get into a horizontal axis lock due to the axis 
    lock determination being made based on the overall delta from the pan start 
    position to the current position (e.g. if you scroll diagonally up, and then 
    down, resulting in a horizontal delta), and let's be on the lookout for
    user complaints about this.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2442,5 @@
>  }
>  
>  void AsyncPanZoomController::StartSmoothScroll(ScrollSource aSource) {
> +  // Only cancel animation and change state if the user isn't pan-locked.
> +  // Snapping is request during panning if the opposing axis of an unscrollable

s/request/requested

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +778,4 @@
>                                   the finger is lifted. */
>      SMOOTH_SCROLL,            /* Smooth scrolling to destination. Used by
>                                   CSSOM-View smooth scroll-behavior */
> +    SMOOTH_SCROLL_LOCKED_X,   /* Smooth scrolling initiated during an axis-locked

Please call these states something like PANNING_LOCKED_X_SMOOTH_SCROLL_Y, and adjust the comments accordingly, to make it clear that an animation and panning are happening at the same time.

Comment 13

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

Suggested changes made, carrying r+

Kats, could you test this on Mac for me please? (I'm working from home for the next couple of days and don't have a Mac to test on. If Windows 10 will do, let me know and I can test it myself, though you might have a better idea of what to look for)

try build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a4c803343ba

Comment 14

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

As far as I can tell the patch works ok on Mac.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8adfc6882298

Comment 16

[Chris Lord [:cwiiis]]

A shocking amount of orange on treeherder, but pushing an unpatched build gets me the same oranges and all of them have open bugs associated with them I think, so going to risk pushing this to inbound. Apologies to sheriffs in advance if this doesn't work out...

https://hg.mozilla.org/integration/mozilla-inbound/rev/8adfc6882298
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8adfc6882298

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8adfc6882298

Comment 18

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

I backed this out for now because nobody has time to deal with the regressions that resulted (bug 1227770, bug 1227789). We should investigate those before re-landing this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f61e7d37cd1b

Comment 19

[Chris Lord [:cwiiis]]

Botond, do you think you could help debug this when you have time? I'm on PTO for the next month, so I won't really be able to get to it before next year.

Comment 20

[Botond Ballo [:botond]]

As per conversations with Kats this isn't a priority, so I can't make any promises, but I will try to find time for it. Leaving needinfo for now.

Comment 21

[Chris Lord [:cwiiis]]

Unassigning as I'm not going to get to this soon. Botond said he may get to it in the near future.

Comment 22

[Chris Lord [:cwiiis]]

Reassigning as this will otherwise not get fixed for a while I don't think. Will try to get some time to look at it soon-ish.

Comment 23

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

So after rebasing, I can't reproduce either of the bugs that this patch previously caused. The code underneath has changed in ways that may affect this (the removal of CROSS_SLIDING states, removal of paint throttling).

I'm going to try running with this patch for the next few days, and if I can't reproduce any issues still, I'd like to try re-landing it as-is. What do you think?

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8715283 [details] [diff] [review]
0001-Bug-1222524-Change-behaviour-of-axis-lock-with-neste.patch

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

Sounds reasonable to me.