1201101 - Nesting a vertical scrollable element in a horizontally scrollable element with no vertical overflow results in no axis-lock

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

As the summary says, the configuration of

<frame 1>
  <frame 2 />
</frame 1>

Where frame 1 is horizontally scrollable only and frame 2 is vertically scrollable only results in a situation where axis-lock cannot be activated, despite there being both horizontal and vertical scrolling available simultaneously.

See the given URL on a FirefoxOS device for a demonstration.

Comment 1

[Botond Ballo [:botond]]

We have logic that says "if we can't scroll in both directions, don't even check the angles to see if axis lock should be activated" [1].

To evaluate whether we can scroll in both directions, we currently only check the event-target APZC.

This logic has been in place ever since axis locking was first implemented by :mbrubeck in bug 892684, but as that was just a couple of weeks after I implemented scroll handoff in bug 898478, the interaction between the two probably wasn't considered.

We should probably just update the check to also consider APZCs further down the handoff chain when evaluating whether we can scroll in both directions.

As with bug 1201098, I'm going to make this a mentored bug, but Chris you should feel free to take it (or ask me to) if you need it fixed more urgently.

[1] https://dxr.mozilla.org/mozilla-central/rev/fb720c90eb49590ba55bf52a8a4826ffff9f528b/gfx/layers/apz/src/AsyncPanZoomController.cpp#1983

Comment 2

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1201101-Enable-axis-locking-over-multiple-APZCs..patch

This replaces the checks of just the current APZC with checking the overscroll handoff chain of the current touch pan block. Seems to work fine.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8659858 [details] [diff] [review]
0001-Bug-1201101-Enable-axis-locking-over-multiple-APZCs..patch

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1980,5 @@
>  
>  void AsyncPanZoomController::HandlePanning(double aAngle) {
>    ReentrantMonitorAutoEnter lock(mMonitor);
> +  nsRefPtr<const OverscrollHandoffChain> overscrollHandoffChain =
> +    CurrentTouchBlock()->GetOverscrollHandoffChain();

HandlePanning() can be called during the processing of pan gesture events as well as touch events. If it's called during the processing of a pan gesture event, CurrentTouchBlock() will return null.

We can instead use GetInputQueue()->CurrentBlock()->GetOverscrollHandoffChain() which will work in both cases.

@@ +1983,5 @@
> +  nsRefPtr<const OverscrollHandoffChain> overscrollHandoffChain =
> +    CurrentTouchBlock()->GetOverscrollHandoffChain();
> +
> +  if (!gfxPrefs::APZCrossSlideEnabled() &&
> +      !overscrollHandoffChain->CanScrollInBothDirections(this)) {

If I'm reading CanScrollInBothDirections(this) correctly, it's equivalent to

  CanScrollInDirection(this, Layer::HORIZONTAL) &&
  CanScrollInDirection(this, Layer::VERTICAL)

I'd rather we just do the latter and avoid adding a new function to OverscrollHandoffChain, even if it means traversing the handoff chain twice (usually they're pretty short).

@@ +1988,4 @@
>      SetState(PANNING);
>    } else if (IsCloseToHorizontal(aAngle, gfxPrefs::APZAxisLockAngle())) {
>      mY.SetAxisLocked(true);
> +    if (overscrollHandoffChain->CanScrollInDirection(this, Layer::HORIZONTAL)) {

CanScrollInDirection() returns true if any APZC's relevant axis CanScroll().

CanScroll() is slightly different from CanScrollNow() in that CanScrollNow() also checks that we're not axis-locked.

As only the event-target APZC will be axis-locked, it's sufficient to check that here rather than throughout the chain:

  if (!mX.IsAxisLocked() && 
      overscrollHandoffChain->CanScrollInDirection(this, Layer::HORIZONTAL))

(IsAxisLocked() needs to be added to Axis to expose mAxisLocked.)

This applies to all calls to CanScrollInDirection() in this function, including the ones I'm proposing we replace the call to CanScrollInBothDirections() with.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1201101-Enable-axis-locking-over-multiple-APZCs..patch

Made the suggested changes.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8660745 [details] [diff] [review]
0001-Bug-1201101-Enable-axis-locking-over-multiple-APZCs..patch

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

LGTM, thanks!

Comment 6

[Chris Lord [:cwiiis]]

Adding checkin-needed, now that bug 1201098 is also ready.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/246008ed43bf

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/246008ed43bf