1796942 - Overscroll transform on YouTube.com Notifications dropdown gets stuck + broken hit testing

Status: VERIFIED FIXED

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

Description

[Gregory Pappas [:gregp]]

Attachment: youtube_overscroll.webm

Steps to reproduce:

1. Navigate to about:config
2. Set apz.overscroll.enabled to true
3. Navigate to https://www.youtube.com/
4. Login
5. Click the bell icon in the top right on the page
6. Scroll down then scroll back up
7. Before the overscroll transform disappears, scroll down again

Actual results:
Overscroll transform gets stuck and hit testing becomes weird

Expected results:
Overscroll transform does not get stuck

Comment 1

[Dan Robertson (:dlrobertson)]

Hiro, does this also occur on windows?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Hey Greg, thanks for reporting. Is this issue a recent regression? Or it's been a while? If it's regression, then finding the culprit would be very helpful. Thanks!

Comment 3

[Gregory Pappas [:gregp]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

Hey Greg, thanks for reporting. Is this issue a recent regression? Or it's been a while? If it's regression, then finding the culprit would be very helpful. Thanks!

I've noticed this since I manually enabled overscroll a few months ago. I will see if it's a regression!

Edit: It's not a regression, I can reproduce as far back as 90.

Comment 4

[Gregory Pappas [:gregp]]

Attachment: reduced.html

Comment 5

[Gregory Pappas [:gregp]]

Attachment: reduced.html

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I can reproduce the issue with the reduced test case in comment 5 on Windows. This would be a candidate of blockers of bug 1393102.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case to recognize the overscroll gutter easier

I do see the stuck gutter on Mac as well with using this test case. A STR provided by Greg;

1. Scroll down a fair bit on the nested scroll container
2. Scroll back up
3. Before the overscroll gutter disappears, start a new gesture by scrolling down again

You will see the yellow gutter.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Forgot adding bug 1393102 into blocker list. I will investigate this once after I finished bug 1794070.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

A relevant part is here in PanGestureBlockState::SetConfirmedTargetApzc;

     RefPtr<AsyncPanZoomController> scrollableApzc =                             
          apzc->BuildOverscrollHandoffChain()->FindFirstScrollable(               
              *aFirstInput, &mAllowedScrollDirections);

The aFirstInput is the pan-start event in the second pan gesture, which means it's upward direction at the 2) step in comment 7. So the FindFirstScrollable returns the root content APZC. Normally if the scroll position of the child scroll container is at the top edge, returning the parent (the root) APZC is correct, but in this specific case, the child scroll container is overscrolled, thus FindFirstScrollable needs to return the child one rather than the parent.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I somehow missed the test cases have a wheel event listener with preventDefault() on the root scroller. So I was about downgrading this priority since it's rarely used, but realized this issue happens on YouTube, a popular site. Anyway, comment 9 is not the right place to fix this issue I think.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

The comment 10 was wrong. I am sorry for confusion. I did diagnosed the problem correctly in comment 9. I need to understand why we need to call FindFirstScrollable again when PanGestureBlockState::SetConfirmedTargetApzc gets called.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I checked Safari hands off the new gesture at 3 to the root scroll container as well as Firefox does currently but Safari restores the sub scroll container's overscroll state gracefully with a smooth animation while the root scroll container is being overscrolled. I will try to do the same thing on Firefox.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1796942 - Restore overscrolling gutters on the previous target APZC when we change it to the new target. r?botond

Comment 14

[Gregory Pappas [:gregp]]

I downloaded this try build and it seems the test case from comment 7 is fixed. Sadly the bug is still present on YouTube :/

Right now I am trying to assemble a new testcase that better matches what YouTube does.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e6f9965b3f1
Restore overscrolling gutters on the previous target APZC when we change it to the new target. r=botond

Comment 16

[Marian-Vasile Laza]

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

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Gregory, are you still able to see the original Youtube issue? The change merged into mozilla-central is much safer than the original change you tested in comment 14. I'd hope it fixes the original issue. Thanks!

Comment 18

[Gregory Pappas [:gregp]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)

Gregory, are you still able to see the original Youtube issue?

Yes, I can still reproduce the bug using the steps in comment 0 on Firefox Nightly 111.0a1(20230117161302).

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Gregory, can you please file a new bug for the remaining case? I found a new edge case (bug 1810935) but it's Linux specific.

Comment 20

[Gregory Pappas [:gregp]]

Yes, filed bug 1810943.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Thanks!

Comment 22

[Dana Lucaci, Desktop QA]

Reproduced the issue with Firefox 108.0a1 (2022-10-22) using the steps from the description on Windows 10.
The issue is verified fixed with Firefox 112.0a1 (20230226214053) and Firefox 111.0b6 (20230226190100) on Windows 10 and macOS 12.