1707217 - [Proton] Infinite overscroll effect is triggered when scrolling over the category bar on youtube.com

Status: VERIFIED FIXED

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

Description

[Andrei Purice]

Attachment: inf scroll.mov

Affected Versions:
Nightly 90.0a1
Beta 89.0b3

Tested On:
MacOS 11.2.1 and 11.0.1

Steps to Reproduce:

1. Reach https://youtube.com .
2. Point the cursor under the site's categories recommendations.
3. Start scrolling in order to achieve the overscroll effect.

Expected Results:
The overscroll effect is done without issues and the page bounces to normal state when stopped/interrupted.

Actual Result:
The overscroll effect can achieve an infinite status if the cursor is overlapping the categories bar after each scroll.

Not reproducible on Safari nor Chrome.

Severity Suggestion:
Due to the fact that this issue can only reproduced only on Youtube.com so far QA considers a Severity of S3.

Notes:
We will monitor other sites as well and complete the list here or file a new bug if more issues surface.
Not a regression since the feature was introduced in Firefox 89.

Comment 1

[Timothy Nikkel (:tnikkel)]

To reproduce you have to have the mouse cursor just below the fixed position category list, then scroll down, until your cursor is over the fixed position category list, and then remove your fingers from the touchpad.

Putting a horizontally scrollable element inside fixed content doesn't seem to reproduce the bug reliably, but it reveals some inconsistencies: if we fling while over the horizontally scrollable element inside the fixed content we go into overscroll at the start/end of the root scroll frame, but if we then try to overscroll while already at the start or end of the main scroll frame we can't get overscroll.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

A key factor is code;

a.boundWheelScroll=function(b){
  b.stopPropagation();
  b.preventDefault();
  a.offset+=Math.abs(b.deltaX)>Math.abs(b.deltaY)?b.deltaX:b.deltaY
}

This is an event listener on an element id#scroll-container inside the fixed content. If I dropped the three line in the listener, the problem goes away.

Comment 3

[Timothy Nikkel (:tnikkel)]

Interesting. I tried adding a prevent default and stopPropagation to a listener in my simple testcase but it wasn't enough to reproduce.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: A minimized case

I could manage to create a case. (TBH, I did fail to create it initially too.)

Comment 5

[Timothy Nikkel (:tnikkel)]

Ha, I guess I was making it too complicated with a scrollable element inside the fixed pos.

Comment 6

[Botond Ballo [:botond]]

I can reproduce this even if the element in question isn't position:fixed.

Comment 7

[Timothy Nikkel (:tnikkel)]

Ha! Same here. We really over thought this one!

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

Putting a horizontally scrollable element inside fixed content doesn't seem to reproduce the bug reliably, but it reveals some inconsistencies: if we fling while over the horizontally scrollable element inside the fixed content we go into overscroll at the start/end of the root scroll frame, but if we then try to overscroll while already at the start or end of the main scroll frame we can't get overscroll.

Filed bug 1707347 for the issue described here. (Also found bug 1707348 with the same testcase.)

Comment 9

[Hiroyuki Ikezoe (:hiro)]

When we receive a PANGESTURE_END event on the preventDefault-ed element, we convert the PANGESTURE_END event to a PANGESTURE_START event, then after that we will never receive any pan gesture events to start an overscroll animation unfortunately. The conversion was introduced in bug 1229039, I haven't looked the bug in detail yet.

Comment 10

[Botond Ballo [:botond]]

To summarize, this bug occurs when:

• There is an element on the page with a wheel listener that calls preventDefault().
• The cursor starts away from the element.
• A touchpad scroll is performed that causes overscrolling which brings the element under the cursor.
• As soon as the element is under the cursor, the overscroll gets stuck with no snap-back to relieve it.

I have a candidate fix for this which clears the overscroll as soon as the element reaches the cursor. This prevents the "stuck in overscroll" issue, but it's not clear if this is the desired behaviour.

Chrome's behaviour here is different: it ignores the preventDefault() (in fact, it doesn't even call the listener!) and continues overscrolling. (Moreover, in a surprise twist, this appears to be Chrome's behaviour even during regular scrolling: wheel event listeners on elements which were not under the cursor at the beginning of the scroll gesture, do not seem to be invoked.)

We can't easily adopt Chrome's behaviour here. In addition to implementation complexity, it may have webcompat implications for situations unrelated to overscroll.

In the absence of a better idea, I'm going to pursue the candidate fix I described above (but I'm open to other suggestions).

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1707217 - Clear pan gesture state when a pan gesture is interrupted by a preventDefault(). r=hiro

This matches our existing behaviour for touch gestures.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

With D113748 it will introduce another undesired behavior, here is a recording on Youtube.com with D113748.
https://drive.google.com/file/d/1oPGqIRMEjSq6uJHLwWGAjLc8QyHsqAG2/view

When a fling happens over preventDefaut-led element, bouncing back effects happen repeatedly. I suppose this is because if there are subsequent pan momentum events once after overscroll state was cleared, those subsequent pan momentum causes overscrolling again then it's cleared again and so on. Presumably we should discard subsequent momentum?

Also note about Safari and Chrome behaviors, with two pan genstures they don't prevent overscrolling even if the second pan gesture started on preventDefault-ed elements, it looks like it's based on the original element position.

Comment 13

[Botond Ballo [:botond]]

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

With D113748 it will introduce another undesired behavior, here is a recording on Youtube.com with D113748.
https://drive.google.com/file/d/1oPGqIRMEjSq6uJHLwWGAjLc8QyHsqAG2/view

When a fling happens over preventDefaut-led element, bouncing back effects happen repeatedly. I suppose this is because if there are subsequent pan momentum events once after overscroll state was cleared, those subsequent pan momentum causes overscrolling again then it's cleared again and so on.

I couldn't verify this (the behaviour is different on Linux with FlingAnimation rather than pan-momentum events), but that sounds plausible, yeah.

Presumably we should discard subsequent momentum?

That could work, though we'd want to be careful to avoid regressing bug 1229039.

Is the momentum behaviour better with this build?

Also note about Safari and Chrome behaviors, with two pan genstures they don't prevent overscrolling even if the second pan gesture started on preventDefault-ed elements, it looks like it's based on the original element position.

I wonder what model leads to this behaviour:

1. Do not dispatch subsequent events in a pan gesture block (after the pan-start) to Gecko at all?
2. Dispatch them to Gecko, but do not call event handlers if the event target is different from the target of the pan-start?
3. Call event handlers, but do not respect prevent-default for subsequent events?

To me, all of these seem somewhat risky with potential impacts outside of overscroll.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #13)

Is the momentum behaviour better with this build?

Yeah, now it's much better.
https://drive.google.com/file/d/1Q9eQXUUtTe_9lQoV8B0C2-0j30DdyjG7/view

There seems sometimes a small bounce back effect I can see and it's not clear where it comes from though, it's acceptable for now and could polish it later I think.

Also note about Safari and Chrome behaviors, with two pan genstures they don't prevent overscrolling even if the second pan gesture started on preventDefault-ed elements, it looks like it's based on the original element position.

I wonder what model leads to this behaviour:

1. Do not dispatch subsequent events in a pan gesture block (after the pan-start) to Gecko at all?
2. Dispatch them to Gecko, but do not call event handlers if the event target is different from the target of the pan-start?
3. Call event handlers, but do not respect prevent-default for subsequent events?

To me, all of these seem somewhat risky with potential impacts outside of overscroll.

I'd bet 2 is the Chrome behavior. anyway I agree it's quite risky since it's a fundamental change which has been exposed in the wild.

Comment 15

[Botond Ballo [:botond]]

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

Yeah, now it's much better.
https://drive.google.com/file/d/1Q9eQXUUtTe_9lQoV8B0C2-0j30DdyjG7/view

There seems sometimes a small bounce back effect I can see and it's not clear where it comes from though, it's acceptable for now and could polish it later I think.

I think this happens when the cursor starts close to the element, and you do a scroll with the fingers on the touchpad that's long enough that the element reaches the cursor, jumps back, and reaches the cursor a second time (and possibly additional times) in the same motion.

I'm not sure what would be the best way to address this part.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fec1174ad8b
Clear pan gesture state when a pan gesture is interrupted by a preventDefault(). r=hiro

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7fec1174ad8b

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 9219123 [details]
Bug 1707217 - Clear pan gesture state when a pan gesture is interrupted by a preventDefault(). r=hiro

Beta/Release Uplift Approval Request

• User impact if declined: [Needed for MR1 / Proton] The page can sometimes get stuck in an overscrolled state on pages that contain elements with "wheel" event listeners which call preventDefault()
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): While the motivation for the change is to fix a bug involving overscroll, the change affects the behaviour of touchpad scrolling on pages containing elements with "wheel" event listeners that call preventDefault() in general.
• String changes made/needed:

Comment 19

[Pascal Chevrel:pascalc]

Comment on attachment 9219123 [details]
Bug 1707217 - Clear pan gesture state when a pan gesture is interrupted by a preventDefault(). r=hiro

This is medium risk but having this visible bug on a major site like Youtube seems to be something that may be noticed by end users, so let's take this one in 89 beta 8 and be on the lookout for any regression it may cause, thanks.

Comment 20

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/3c232ce53c83

Comment 21

[Andrei Purice]

We checked the fix and it no longer produces the infinite effect but has introduced a new bug : 1709582 which seems to be a regression of this fix.
Can you guys look into it?

Comment 22

[Botond Ballo [:botond]]

Responded in bug 1709582 comment 4.

Comment 23

[Andrei Purice]

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