Closed Bug 1201101 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: cwiiis, Assigned: cwiiis, Mentored)

References

()

Details

(Keywords: polish, Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

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.
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
Mentor: botond
Whiteboard: [lang=c++]
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.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Attachment #8659858 - Flags: review?(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.
Attachment #8659858 - Flags: review?(botond)
Made the suggested changes.
Attachment #8659858 - Attachment is obsolete: true
Attachment #8660745 - Flags: review?(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!
Attachment #8660745 - Flags: review?(botond) → review+
Adding checkin-needed, now that bug 1201098 is also ready.
Keywords: checkin-needed
Keywords: polish
https://hg.mozilla.org/mozilla-central/rev/246008ed43bf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.