1159985 - Flinging doesn't always work on desktop

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Get touch events to drive APZ on Linux. To do this:
        - Apply the patch in bug 978679.
        - Modify the call to DispatchEvent in nsWindow::OnTouchEvent
          to be DispatchAPZAwareEvent.
        - Build with '--enable-default-toolkit=cairo-gtk3'
        - Turn on the 'layers.async-pan-zoom.enabled' and
          'dom.w3c_touch_events.enabled' prefs.
  2. Load a scrollable page, such as http://people.mozilla.org/~bballo/grid.html
  3. Fling the page.

Expected results:
  Flinging always works.

Actual results:
  Sometimes the page comes to an abrupt halt after lifting the finger.

I will investigate.

Comment 1

[Botond Ballo [:botond]]

Investigation revealed the following:

  - The problem occurs when the fling velocity has a component in a direction
    in which we have no room to scroll. For example, if you fling up and to
    the right, and you're already at the left edge of the page.

  - The problem is not specific to desktop, but on B2G it's largely covered 
    up by axis locking being enabled, which makes it such that you rarely
    end up in the situation described above.

The problem arises because in this scenario, the fling goes into overscroll and is handed off right away. If there is no one to hand off to, or the thing handed off can't scroll in the intended direction (e.g. vertically), then no kinetic motion occurs.

We could paper over this by tweaking the velocity-dropping code here [1] yet again, but it wouldn't handle all scenarios (for example, it wouldn't handle the scenario where you're *near* the left edge of the page when you start the fling; in that case, the fling would start but then abruptly stop when you reach the left edge).

I would like to propose finally implementing what I believe is the proper solution here: partial handoff, where we hand off just the component of the fling that's in overscroll, and keep the other component in the original APZC (if the other component isn't also in overscroll). Kats, any objections to this, or other thoughts on this topic?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=262243fb509a#439

Comment 2

[Kartikaya Gupta (email:[e-mail])]

That sounds fine to me.

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: bz://1159985/botond

/r/8621 - Bug 1159985 - If only one component of a fling is in overscroll, continue the fling in the other component. r=kats
/r/8623 - Bug 1159985 - In APZ gtests, allow panning in both directions. r=kats
/r/8625 - Bug 1159985 - Gtest. r=kats

Pull down these commits:

hg pull -r 99987406ab48814fc9c3f04bb9ffa163b62c0464 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[Botond Ballo [:botond]]

These are WIP patches for implementing partial fling handoff. They need more testing.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8604433 [details]
MozReview Request: bz://1159985/botond

/r/8621 - Bug 1159985 - If only one component of a fling is in overscroll, continue the fling in the other component. r=kats
/r/8623 - Bug 1159985 - In APZ gtests, allow panning in both directions. r=kats
/r/8625 - Bug 1159985 - Gtest. r=kats

Pull down these commits:

hg pull -r 99987406ab48814fc9c3f04bb9ffa163b62c0464 https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Botond Ballo [:botond]]

Tested the patches on desktop and B2G. They seem to fix the issue on desktop, while not breaking anything obviously on b2g.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8604433 [details]
MozReview Request: bz://1159985/botond

https://reviewboard.mozilla.org/r/8619/#review7513

::: gfx/layers/apz/src/AsyncPanZoomController.h:530
(Diff revision 1)
> +  void SetVelocityVector(const ParentLayerPoint& aVelocityVector);

Doesn't look like this function is used anywhere. Remove?

Comment 8

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> ::: gfx/layers/apz/src/AsyncPanZoomController.h:530
> (Diff revision 1)
> > +  void SetVelocityVector(const ParentLayerPoint& aVelocityVector);
> 
> Doesn't look like this function is used anywhere. Remove?

Ah, yes - this was used in an older version of the patch, and then the use was removed. Thanks for catching it.

(I also just realized that with this removed, this basically ended up being a one-line fix :)).

Comment 9

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cdfbf778323

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/890152532a77
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd543fa64fe8
https://hg.mozilla.org/integration/mozilla-inbound/rev/961ab2f19835

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/890152532a77
https://hg.mozilla.org/mozilla-central/rev/dd543fa64fe8
https://hg.mozilla.org/mozilla-central/rev/961ab2f19835

Comment 13

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1159985 - Gtest. r=kats

Comment 14

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1159985 - If only one component of a fling is in overscroll, continue the fling in the other component. r=kats

Comment 15

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1159985 - In APZ gtests, allow panning in both directions. r=kats