1916897 - An APZHandledResult is returned for dynamic toolbar movement is improper for pull-to-refresh

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: t.html

I think this is a remaining case that we do trigger pull-to-refresh inappropriately on Uber.com (bug 1903682).

Attaching file is a test case to see this improper pull-to-refresh. STR is;

1. Open the attachment with enabling both of dynamic toolbar and pull-to-refresh
2. Scroll down the blue area until the dynamic toolbar hidden, the blue area is a sub scroll container
3. Try to move up on the white area to make the dynamic toolbar visible
4. Try to scroll up the blue-ish area

You can see pull-to-refresh is triggered even though the blue-ish area should scroll up.

(In reply to Markus Stange [:mstange] from bug 1903682 comment 0)

Steps to reproduce:

1. Go to https://m.uber.com/
2. Log in with your Uber account.
3. Open the sidebar with the hamburger button in the top right.
4. Tap on "My Trips".
5. Scroll down to the "Past" section and to the bottom of the list of your past trips (the list has four entries for me).
6. Try to scroll back up.

I guess the sidebar (or the "My Trips") is a sub scroll container.

Anyways, the code in question is here in APZHandledResult::UpdateForTouchEvent. Though we do use the root content APZC to set APZHandledResult::mOverScrollDirections, at that moment when we try to scroll up the sub scroll container, the root content APZC's scroll position is 0, thus it triggers pull-to-refresh.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1916897 - Get rid of ScrollDirection::eVertical from APZHandledResult::mOverscrollDirections if mayTriggerPullToRefresh is false. r?botond

Though I personally think this fix is subtle, I have no other good idea to
solve the issue. Once after we have a separate API for dynamic toolbar, we
could handle this bug case in a clearer way.

This change doesn't have a dedicated test since there have been two JUnit tests
hitting this bug conditions.

(If we had been running tests on browsers which support both dynamic toolbar
and pull-to-refresh, this bug might have been noticed earlier?)

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43fa5456c465
Get rid of ScrollDirection::eVertical from APZHandledResult::mOverscrollDirections if mayTriggerPullToRefresh is false. r=botond,geckoview-reviewers,owlish

Comment 3

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/43fa5456c465