1705280 - Overscroll handoff doesn't work on where pan gestures happen on a child scroll container and the scroll position of the parent scroll container is at the edge

Status: VERIFIED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: an example

This is the issue I've been seeing on 9to5mac.com (bug 1702988), and I believe overflow:hidden document in iframe is commonly used (for ads mainly?), so this should be fixed until we release Fission.

Anyway, the issue was a bit complicated than I thought initially. Attaching case is an example to see the issue with Fission. A STR is just

1. Overscroll upwards by panning on the iframe when the root scroll position is (0, 0)

If the root scroll position is not at (0, 0), the issue doesn't happen, i.e.

1. Scroll down a bit
2. Overscroll upwards by panning on the iframe

Then, the issue doesn't happen.

So a problem is here in OverscrollHandoffChain::FindFirstScrollable

  for (size_t i = 0; i < Length(); i++) {                                       
    if (mChain[i]->CanScroll(aInput)) {                                         
      return mChain[i];                                                         
    }                                                                           
                                                                                
    *aOutAllowedScrollDirections &= mChain[i]->GetAllowedHandoffDirections();   
    if (aOutAllowedScrollDirections->isEmpty()) {                               
      return nullptr;                                                           
    }                                                                           
  }

With the upwards pan gestures at (0, 0) position in the root scroller, the mChain[i]->CanScroll(aInput) will never return true, so that we will never handoff to the root one.

I wonder whether the root content APZC could be the last resort for overscroll handoff.

Comment 1

[Botond Ballo [:botond]]

How does it work with non-Fission?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

With non-Fission, the iframe's overflow:hidden document doesn't have any APZC (If I read properly a log output by MOZ_LOG=apz:manager).

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Now I am convinced the root content APZC should be the last one for pan gestures with checking overscroll-behavior is either "auto" or "contains".

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Another example which is not related to Fission

Looks like I did unintentionally fix non fission cases, I just realized it when I wrote a gtest.

In this case, there is a child scroll container which is vertically scrollable, then if the root scroll position is at top, and if the child scroll container's position is also at top, then scroll handoff doesn't work, I mean panning on the child scroll container doesn't cause overscrolling on the root scroll container.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1705280 - Allow overscroll handoff to the root content APZC on pan gestures even if the root APZC is not scrollable to the given input directions. r?botond

To be precise the root APZC should actually be scrollable in the opposed
directions of the given input, it's checked in
AsyncPanZoomController::GetOverscrollableDirections.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Note that I gave up writing a gtest which will be working with WebRender hit test code since it requires more work than I thought.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86a1362f15e6
Allow overscroll handoff to the root content APZC on pan gestures even if the root APZC is not scrollable to the given input directions. r=botond

Comment 9

[Iulian Moraru]

Backed out changeset 86a1362f15e6 (Bug 1705280) for causing Android Gtest failures in APZCOverscrollTesterForLayersOnly.

Push with failures

Failure log

Backout link

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Forgot to enclose the gtest with "#ifndef MOZ_WIDGET_ANDROID".

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b9fbea8a60e
Allow overscroll handoff to the root content APZC on pan gestures even if the root APZC is not scrollable to the given input directions. r=botond

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/3b9fbea8a60e

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9217602 [details]
Bug 1705280 - Allow overscroll handoff to the root content APZC on pan gestures even if the root APZC is not scrollable to the given input directions. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: [Required for MR1 / Proton] overscroll effect doesn't happen at all
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is effective only for overscroll state with pan gesture.
• String changes made/needed: none

Comment 15

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9217602 [details]
Bug 1705280 - Allow overscroll handoff to the root content APZC on pan gestures even if the root APZC is not scrollable to the given input directions. r?botond

Approved for 89 beta 6, thanks.

Comment 16

[Pascal Chevrel (relman team) -> :pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/e7736cb7c4fa

Comment 17

[Andrei Purice]

Verified fixed on the latest Firefox Beta 89.0b8 (20210504185920) and Nightly 90.0a1 (20210505215208).