1105109 - (apz-autoscrolling) APZ: Use APZ for autoscrolling

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[mitchell]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141125030206

Steps to reproduce:

1. Enable autoscrolling in Options > Advanced.
2. Open a page, any page.
3. Middle mouse click to activate autoscrolling.

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141125030206 CSet: 74edfbf0e6a3


Actual results:

Juddery, old style scrolling.



Expected results:

Buttery smooth, apz style scrolling

Comment 1

[avada]

What about keyboard scrolling? Up/down and Page up/down?
How can you tell? To me scrolling is usually smooth even without APZ.

Comment 2

[Botond Ballo [:botond]]

(In reply to avada from comment #1)
> What about keyboard scrolling? Up/down and Page up/down?

Those don't currently use APZ, either.

> How can you tell? To me scrolling is usually smooth even without APZ.

For pages that keep Gecko's main thread too busy to keep up with input events that trigger scrolling - for example because they are complex to render, or because they run a lot of script - scrolling will jank (i.e. pause until the main thread catches up) without APZ, while it should not jank with APZ (it may checkerboard instead, which means a portion of the page content brought into view is blank for a brief period before being rendered).

For normal pages on fast hardware you may not notice any difference.

Comment 3

[avada]

(In reply to Botond Ballo [:botond] from comment #2)
> (In reply to avada from comment #1)
> > What about keyboard scrolling? Up/down and Page up/down?
> 
> Those don't currently use APZ, either.

Surprising I thought APZ was intended to work with all kinds of scrolling in general. Didn't think that the different forms of scrolling was handled differently at all.

Is APZ usage expected to be extended to the other ways of scrolling?

Comment 4

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

Eventually, yes. With some forms of input (e.g. touch) the slow painting is more noticeable than with other forms of input (keyboard) so we are trying to get the more important cases first.

Comment 5

[Botond Ballo [:botond]]

(In reply to avada from comment #3)
> (In reply to Botond Ballo [:botond] from comment #2)
> > (In reply to avada from comment #1)
> > > What about keyboard scrolling? Up/down and Page up/down?
> > 
> > Those don't currently use APZ, either.
> 
> Surprising I thought APZ was intended to work with all kinds of scrolling in
> general. 

It is. It's just a matter of implementing it for various forms of input.

APZ started as a feature for mobile platforms (Fennec, B2G), where keyboard-driven scrolling was virtually nonexistent, so it didn't make sense to support it at the time.

Comment 6

[avada]

Since I'm using autoscroll 90% of the time. (It's by fat the most convenient) I'm wondering as APZ bugs go what's the priority on this bug?

Comment 7

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

I would say that this is a "feature" rather than a "bug", similar to how we don't yet support touch input for APZ on desktop. So it would relatively low priority, and I would be ok with shipping APZ to users in the release channel without this. However it is definitely something we would like to have.

Comment 8

[avada]

Is this still the case? Hotkey scrolling seems to be smoother to me also.

Comment 9

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

(In reply to avada from comment #8)
> Is this still the case?

Not sure what you're referring to. Supporting autoscrolling with APZ is still something we want to implement but it's not super high-priority. We will probably get telemetry first (bug 1238137) to see which input methods are the most used.

> Hotkey scrolling seems to be smoother to me also.

What do you mean by hotkey scrolling?

Comment 10

[avada]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> (In reply to avada from comment #8)
> > Is this still the case?
> 
> Not sure what you're referring to. Supporting autoscrolling with APZ is
> still something we want to implement but it's not super high-priority. We
> will probably get telemetry first (bug 1238137) to see which input methods
> are the most used.
> 
> > Hotkey scrolling seems to be smoother to me also.
> 
> What do you mean by hotkey scrolling?

By hotkey scrolling I mean up/down, home/end, page up/down scrolling. (I actually meant to write autoscrolling too, but forgot)
What should I call it?

Comment 11

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

I would just call it keyboard scrolling.

Comment 12

[sarthakm27]

Middle mouse scrolling is janky and doesn't work with apz also.
This seems like a good demo, has a 500ms  delay in javascript
https://staktrace.com/resources/extras/spout/scroll-behavior.html

Comment 13

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

Yes, APZ is not yet supported for autoscrolling.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Will look at keyboard input first (bug 1351783.)  Parked until that one is done.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #15)
> Will look at keyboard input first (bug 1351783.)  Parked until that one is
> done.

Scratch that; I misunderstood the dependency on the keyboard input APZ.  There isn't any.  Comment 10 and comment 11 led me astray.

Comment 17

[Botond Ballo [:botond]]

I looked a bit into how autoscrolling currently works.

To start autoscrolling:

  - Chrome JS in the content process receives a mousedown for
    the middle mouse button.

  - It records the position of the mouse, chooses a scroll frame
    based on the mouse position, and registers a mousemove 
    listener and an rAF callback.

      - The mousemove listener keeps track of the current 
        position of mouse.

      - The rAF callback scrolls the chosen scroll frame based
        on the distance between the initial mouse position and
        the current mouse position.

  - It sends an Autoscroll:Start message to the parent process.

      - The parent process, upon receiving that message, shows
        the autoscroll popup (the little circle with the arrows).

To stop autoscrolling:

  - Listeners in the parent process detect the relevant events
    (like a left or right mouse button click), hide the popup,
    and send an Autoscroll:Stop message to the child process.

  - The child process stops registering its rAF callback.


So, the general idea of getting this work with APZ would be:

  - Send additional info that APZ needs to perform the
    autoscrolling along with the Autoscroll:Start message.
    At minimum, this will include info that identifies the
    scroll frame to be scrolled.

  - Have the parent process notify APZ about the start of
    autoscrolling.

  - Have APZ kick off an APZ animation (something that runs 
    on each composite, the equivalent of the rAF callback)
    to perform the scrolling.

      - APZ already receives all mouse events, so it can
        update the relevant state when it sees a mousemove
        event.

  - Let the content process know somehow that APZ is 
    handling the autoscroll so it shouldn't also try to
    scroll.

  - When the parent process detects autoscrolling should
    stop, notify APZ.

Note that the fact that the autoscroll popup lives in the parent
process means it's already going into a distinct layer from the
scrolled content, so that part's good.

Comment 18

[Botond Ballo [:botond]]

Note that, in the approach described above, kicking off autoscrolling still requires a round-trip to the content process main thread. That's possible to avoid (in principle), but it would be a lot of additional work, so I'm going to start with the above, which still gives us a lot of benefit.

Comment 19

[Botond Ballo [:botond]]

WIP patch can be found here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=526dafbf7cd63e6654056a153dacf07b2a4e2c51

Not in a working state yet.

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter.

Review commit: https://reviewboard.mozilla.org/r/150618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150618/

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

Review commit: https://reviewboard.mozilla.org/r/150620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150620/

Comment 22

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Have APZCTreeManager keep track of the current mouse position.

Review commit: https://reviewboard.mozilla.org/r/150622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150622/

Comment 23

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Add a ScrollByAndClamp() utility function to AsyncPanZoomController.

Review commit: https://reviewboard.mozilla.org/r/150624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150624/

Comment 24

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Add a BeforeSample() method to AsyncPanZoomAnimation.

This gives the animation a chance to perform work that requires the APZC
lock not to be held.

Review commit: https://reviewboard.mozilla.org/r/150626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150626/

Comment 25

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Add autoscrolling support to AsyncPanZoomController.

This involves adding a new type of AsyncPanZoomAnimation, a new APZC state,
and methods to start and stop autoscrolling.

Review commit: https://reviewboard.mozilla.org/r/150628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150628/

Comment 26

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

The messages are routed through nsITabParent, nsIWidget, and IAPZCTreeManager
(the latter possibly remoted via PAPZCTreeManager if out-of-process compositing
it used).

Review commit: https://reviewboard.mozilla.org/r/150630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150630/

Comment 27

[Botond Ballo [:botond]]

Now split into parts and beginning to resemble what will be the final patch series. Still very much a WIP.

Comment 28

[avada]

(In reply to Botond Ballo [:botond] from comment #27)
> Now split into parts and beginning to resemble what will be the final patch
> series. Still very much a WIP.

Do you think it'll make it before v56 beta is split off?

Comment 29

[Botond Ballo [:botond]]

(In reply to avada from comment #28)
> (In reply to Botond Ballo [:botond] from comment #27)
> > Now split into parts and beginning to resemble what will be the final patch
> > series. Still very much a WIP.
> 
> Do you think it'll make it before v56 beta is split off?

That is my hope. I'm at a conference this week, but I plan to pick this up next week. However, no promises :)

Comment 30

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Fix unified build errors.

Review commit: https://reviewboard.mozilla.org/r/159006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159006/

Comment 31

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Notify content when APZ is handling an autoscroll.

Review commit: https://reviewboard.mozilla.org/r/159008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159008/

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8879330 [details]
Bug 1105109 - Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150618/diff/1-2/

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8879331 [details]
Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150620/diff/1-2/

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8879332 [details]
Bug 1105109 - Have APZCTreeManager keep track of the current mouse position.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150622/diff/1-2/

Comment 35

[Botond Ballo [:botond]]

Comment on attachment 8879333 [details]
Bug 1105109 - Add a ScrollByAndClamp() utility function to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150624/diff/1-2/

Comment 36

[Botond Ballo [:botond]]

Comment on attachment 8879334 [details]
Bug 1105109 - Add a BeforeSample() method to AsyncPanZoomAnimation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150626/diff/1-2/

Comment 37

[Botond Ballo [:botond]]

Comment on attachment 8879335 [details]
Bug 1105109 - Add autoscrolling support to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150628/diff/1-2/

Comment 38

[Botond Ballo [:botond]]

Comment on attachment 8879336 [details]
Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150630/diff/1-2/

Comment 39

[Botond Ballo [:botond]]

Attachment: [WIP] Get the coordinate systems right

Review commit: https://reviewboard.mozilla.org/r/160760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160760/

Comment 40

[Botond Ballo [:botond]]

Comment on attachment 8879330 [details]
Bug 1105109 - Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150618/diff/2-3/

Comment 41

[Botond Ballo [:botond]]

Comment on attachment 8879331 [details]
Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150620/diff/2-3/

Comment 42

[Botond Ballo [:botond]]

Comment on attachment 8879332 [details]
Bug 1105109 - Have APZCTreeManager keep track of the current mouse position.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150622/diff/2-3/

Comment 43

[Botond Ballo [:botond]]

Comment on attachment 8879333 [details]
Bug 1105109 - Add a ScrollByAndClamp() utility function to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150624/diff/2-3/

Comment 44

[Botond Ballo [:botond]]

Comment on attachment 8879334 [details]
Bug 1105109 - Add a BeforeSample() method to AsyncPanZoomAnimation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150626/diff/2-3/

Comment 45

[Botond Ballo [:botond]]

Comment on attachment 8879335 [details]
Bug 1105109 - Add autoscrolling support to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150628/diff/2-3/

Comment 46

[Botond Ballo [:botond]]

Comment on attachment 8879336 [details]
Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150630/diff/2-3/

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8888116 [details]
Bug 1105109 - Fix unified build errors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159006/diff/1-2/

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159008/diff/1-2/

Comment 49

[Botond Ballo [:botond]]

The patch series now passes a basic smoke test, and I can see checkerboarding on long pages, indicating that APZ is driving the autoscrolling.

Next steps:
  - The last patch needs significant cleanup
  - More testing
  - Put behind a pref and land

Comment 50

[Botond Ballo [:botond]]

I think I have this working well enough to get reviewed and landed behind a pref that's off by default.

We can then turn on the pref on the Nightly channel after more testing.

Comment 51

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Put APZ autoscrolling behind a pref.

For now, the pref is off by default.

Review commit: https://reviewboard.mozilla.org/r/161754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161754/

Comment 52

[Botond Ballo [:botond]]

Comment on attachment 8888116 [details]
Bug 1105109 - Fix unified build errors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159006/diff/2-3/

Comment 53

[Botond Ballo [:botond]]

Comment on attachment 8879330 [details]
Bug 1105109 - Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150618/diff/3-4/

Comment 54

[Botond Ballo [:botond]]

Comment on attachment 8879331 [details]
Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150620/diff/3-4/

Comment 55

[Botond Ballo [:botond]]

Comment on attachment 8879332 [details]
Bug 1105109 - Have APZCTreeManager keep track of the current mouse position.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150622/diff/3-4/

Comment 56

[Botond Ballo [:botond]]

Comment on attachment 8879333 [details]
Bug 1105109 - Add a ScrollByAndClamp() utility function to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150624/diff/3-4/

Comment 57

[Botond Ballo [:botond]]

Comment on attachment 8879335 [details]
Bug 1105109 - Add autoscrolling support to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150628/diff/3-4/

Comment 58

[Botond Ballo [:botond]]

Comment on attachment 8879336 [details]
Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150630/diff/3-4/

Comment 59

[Botond Ballo [:botond]]

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159008/diff/2-3/

Comment 60

[Botond Ballo [:botond]]

Note: the "Have the parent process notify APZ of the start and stop of autoscrolling" patch is *mostly* just plumbing, but the coordinate translation that happens in TabParent::StartApzAutoscroll() is important for correctness; I just wanted to point that out, so it doesn't get lost in the sea of plumbing that the rest of that patch is.

Comment 61

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

Comment on attachment 8888116 [details]
Bug 1105109 - Fix unified build errors.

https://reviewboard.mozilla.org/r/159006/#review167314

I think this is obsolete now, it was fixed in http://searchfox.org/mozilla-central/diff/a48f3ac6fe5aaa94f0ef52fd0ed3d9b537198dbd/gfx/layers/composite/TextureHost.cpp#101

Comment 62

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

Comment on attachment 8879330 [details]
Bug 1105109 - Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter.

https://reviewboard.mozilla.org/r/150618/#review167316

Comment 63

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

Comment on attachment 8879331 [details]
Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

https://reviewboard.mozilla.org/r/150620/#review167320

::: toolkit/content/browser-content.js:144
(Diff revision 4)
> +    let scrollable = this._scrollable;
> +    if (scrollable instanceof Ci.nsIDOMWindow) {
> +      // getViewId() needs an element to operate on.
> +      scrollable = scrollable.document.documentElement;
> +    }
> +    let scrollId = domUtils.getViewId(scrollable);

the implementation of getViewId returns NS_ERROR_NOT_AVAILABLE if the element doesn't have a view id. I think there might be scenarios where that legitimately happens (maybe even if e10s/APZ is off). In that case I think the JS code will throw an exception and autoscroll will effectively break. Worth checking and guarding against.

::: toolkit/content/browser-content.js:150
(Diff revision 4)
> +                                     scrollId,
> +                                     presShellId});

Should these parameters be labeled also? It looks like they're being packaged up into a JS object. I'm surprised this doesn't result in a JS error, it doesn't look like valid syntax.

Comment 64

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

Comment on attachment 8879332 [details]
Bug 1105109 - Have APZCTreeManager keep track of the current mouse position.

https://reviewboard.mozilla.org/r/150622/#review167322

::: gfx/layers/apz/src/APZCTreeManager.h:601
(Diff revision 4)
>     */
>    int32_t mRetainedTouchIdentifier;
>    /* Tracks the number of touch points we are tracking that are currently on
>     * the screen. */
>    TouchCounter mTouchCounter;
> +  /* Stores the current mouse position in screen coordination.

s/coordination/coordinates/

::: gfx/layers/apz/src/APZCTreeManager.cpp:987
(Diff revision 4)
>        break;
>      } case MOUSE_INPUT: {
>        MouseInput& mouseInput = aEvent.AsMouseInput();
>        mouseInput.mHandledByAPZ = true;
>  
> +      if (mouseInput.mType == MouseInput::MOUSE_MOVE) {

We could probably omit the MOUSE_MOVE guard here and just update the position on any mouse event. I suspect that on some platforms the OS might drop events. e.g. if the user rapidly moves the mouse and clicks it might just send the down/up events at the new coordinates without sending a move beforehand.

Comment 65

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

Comment on attachment 8879333 [details]
Bug 1105109 - Add a ScrollByAndClamp() utility function to AsyncPanZoomController.

https://reviewboard.mozilla.org/r/150624/#review167326

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2916
(Diff revision 4)
> +void AsyncPanZoomController::ScrollByAndClamp(const CSSPoint& aOffset) {
> +  CSSPoint newScrollPos = mFrameMetrics.GetScrollOffset() + aOffset;
> +  newScrollPos = mFrameMetrics.CalculateScrollRange().ClampPoint(newScrollPos);
> +  mFrameMetrics.SetScrollOffset(newScrollPos);
> +}

Looks like we have this pattern of

metrics.SetScrollOffset(metrics.CalculateScrollRange().ClampPoint(something))

in a couple of other places too. Might be worth putting that as a ClampAndSetScrollOffset function on FrameMetrics?

Comment 66

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

Comment on attachment 8879335 [details]
Bug 1105109 - Add autoscrolling support to AsyncPanZoomController.

https://reviewboard.mozilla.org/r/150628/#review167330

This patch needs a moz.build update too

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1103
(Diff revision 4)
>      case SMOOTH_SCROLL:
>      case OVERSCROLL_ANIMATION:
>      case WHEEL_SCROLL:
>      case KEYBOARD_SCROLL:
>      case PAN_MOMENTUM:
> +    case AUTOSCROLL:

I'm a little concerned about the interaction between autoscroll and other input mechanisms. From this patch it looks like if you do a touch-down during an autoscroll animation it will abort the APZ-side autoscroll animation, but I think the browser-content.js code will probably still think it's in an autoscroll. This might result in the autoscroll visual indicator remaining up, but i'm not sure. I'll have to play with the different interactions and see what happens. We can deal with those in follow-up bugs but I just wanted to write it down so we don't forget.

::: gfx/layers/apz/src/AutoscrollAnimation.cpp:12
(Diff revision 4)
> +#include "AutoscrollAnimation.h"
> +
> +#include <cmath>  // for sqrtf()
> +
> +#include "AsyncPanZoomController.h"
> +#include "mozilla/ToString.h"

ToString.h doesn't appear to be used

Comment 67

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

Comment on attachment 8879336 [details]
Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

https://reviewboard.mozilla.org/r/150630/#review167346

::: commit-message-0c726:5
(Diff revision 4)
> +Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling. r=kats
> +
> +The messages are routed through nsITabParent, nsIWidget, and IAPZCTreeManager
> +(the latter possibly remoted via PAPZCTreeManager if out-of-process compositing
> +it used).

s/it used/is used/

::: dom/interfaces/base/nsITabParent.idl:84
(Diff revision 4)
> +   * in global screen coordinates. aScrollId and aPresShellId identify the 
> +   * scroll frame content chose to scroll.
> +   */
> +  void startApzAutoscroll(in float aAnchorX, in float aAnchorY, 

some trailing whitespace here

::: dom/interfaces/base/nsITabParent.idl:84
(Diff revision 4)
>    readonly attribute boolean hasBeforeUnload;
> +
> +  /**
> +   * Notify APZ to start autoscrolling.
> +   * (aAnchorX, aAnchorY) are the coordinates of the autoscroll anchor,
> +   * in global screen coordinates. aScrollId and aPresShellId identify the 

"in global screen coordinates" here implies they are in ScreenPixels but actually they are LayoutDevicePixels relative to the screen.

::: dom/interfaces/base/nsITabParent.idl:85
(Diff revision 4)
> +
> +  /**
> +   * Notify APZ to start autoscrolling.
> +   * (aAnchorX, aAnchorY) are the coordinates of the autoscroll anchor,
> +   * in global screen coordinates. aScrollId and aPresShellId identify the 
> +   * scroll frame content chose to scroll.

s/frame content/frame that content/ for easier parsing. (when i first read it i interpreted "scroll frame content" as "content of the scroll frame")

::: dom/interfaces/base/nsITabParent.idl:92
(Diff revision 4)
> +  void startApzAutoscroll(in float aAnchorX, in float aAnchorY, 
> +                          in nsViewID aScrollId, in uint32_t aPresShellId);
> +
> +  /**
> +   * Notify APZ to stop autoscrolling.
> +   * aScrollId and apresShellId identify the scroll frame that is being

s/apresShellId/aPresShellId/

::: dom/ipc/TabParent.cpp:3290
(Diff revision 4)
> +  }
> +
> +  if (RenderFrameParent* renderFrame = GetRenderFrame()) {
> +    uint64_t layersId = renderFrame->GetLayersId();
> +    if (nsCOMPtr<nsIWidget> widget = GetWidget()) {
> +      ScrollableLayerGuid guid{layersId, aPresShellId, aScrollId};

I've probably asked you this before but forgotten the answer: any reason you prefer the {} syntax to () ?

::: gfx/layers/ipc/APZCTreeManagerParent.cpp:308
(Diff revision 4)
> +  // Unlike RecvStartScrollbarDrag(), there is no need to check the layers id
> +  // here, because it comes from the parent process (TabParent).

And actually the layers id will not match mLayersId, because the layers id being passed in is for a content layer tree whereas this APZCTreeManagerParent will be for the root. It might be worth mentioning that in the comment.

::: gfx/layers/ipc/PAPZCTreeManager.ipdl:82
(Diff revision 4)
> +  
> +  async StartAutoscroll(ScrollableLayerGuid aGuid, ScreenPoint aAnchorLocation);
> +  

trailing ws

::: gfx/layers/moz.build:294
(Diff revision 4)
>      'AnimationHelper.cpp',
>      'AnimationInfo.cpp',
>      'apz/public/IAPZCTreeManager.cpp',
>      'apz/src/APZCTreeManager.cpp',
>      'apz/src/AsyncPanZoomController.cpp',
> +    'apz/src/AutoscrollAnimation.cpp',

This belongs in the previous patch

Comment 68

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

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

https://reviewboard.mozilla.org/r/159008/#review167368

Dropping flag on this one for now, if you want to do that scroll id change as a followup i'd be ok with that.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:978
(Diff revision 3)
> +  }
> +
> +  MOZ_ASSERT(NS_IsMainThread());
> +  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +  MOZ_ASSERT(observerService);
> +  // TODO: Send the |aScrollId| as an argument.

Yeah I would prefer using the scroll id rather than the docshell. the scroll id is unique per process so it should be good for this purpose. You should be able to serialize it into a UTF16 string and pass it in the "data" parameter of NotifyObservers

::: gfx/layers/ipc/PAPZ.ipdl:67
(Diff revision 3)
>    async NotifyAPZStateChange(ScrollableLayerGuid aGuid, APZStateChange aChange, int aArg);
>  
>    async NotifyFlushComplete();
>  
>    async NotifyAsyncScrollbarDragRejected(ViewID aScrollId);
> +  

ws

::: toolkit/content/browser-content.js:47
(Diff revision 3)
>      this.autoscrollLoop = this.autoscrollLoop.bind(this);
>  
>      Services.els.addSystemEventListener(global, "mousedown", this, true);
>  
>      addMessageListener("Autoscroll:Stop", this);
> +    

ws

Comment 69

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

Comment on attachment 8890591 [details]
Bug 1105109 - Put APZ autoscrolling behind a pref.

https://reviewboard.mozilla.org/r/161754/#review167370

I'd also like to see (either in this bug or a follow-up) an update to the ScrollInputMethods telemetry that captures APZ autoscrolling.

Comment 70

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #63)
> ::: toolkit/content/browser-content.js:150
> (Diff revision 4)
> > +                                     scrollId,
> > +                                     presShellId});
> 
> Should these parameters be labeled also? It looks like they're being
> packaged up into a JS object. I'm surprised this doesn't result in a JS
> error, it doesn't look like valid syntax.

It's a shorthand for:

   { 
     ...,
     scrollId: scrollId,
     presShellId: presShellId
   }

(See [1].)

I initially wrote the long form, but in a Try push the ESLint task told me "Expected property shorthand".

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer

Comment 71

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #67)
> ::: dom/ipc/TabParent.cpp:3290
> (Diff revision 4)
> > +  }
> > +
> > +  if (RenderFrameParent* renderFrame = GetRenderFrame()) {
> > +    uint64_t layersId = renderFrame->GetLayersId();
> > +    if (nsCOMPtr<nsIWidget> widget = GetWidget()) {
> > +      ScrollableLayerGuid guid{layersId, aPresShellId, aScrollId};
> 
> I've probably asked you this before but forgotten the answer: any reason you
> prefer the {} syntax to () ?

  - The {} syntax is a uniform syntax that can be used for both aggregate 
    initialization and calling a constructor. Using it means that you don't
    have to think, at the initialization site, about which one a class/struct 
    uses (it could even change from one to the other as an implementation
    detail).

  - The {} syntax is not prone to the "vexing parse":

      struct A {
        A(int);
      };
      struct S {
        S(A, A);
      };
      void foo(int x, int y) {
        S foo(A(x), A(y));  // #1
      }

    Here, thanks to an oddity of the language where a parameter name is 
    allowed to be enclosed in parentheses, line #1 actually declares a 
    function named "foo", with parameters of type "A" named "x" and "y"
    and return type "S". That's probably not what the author intended.
    Consistently using {} for initialization avoids problems like this.

Comment 72

[Milan Sreckovic [:milan] (needinfo for best results)]

I always read {} as aggregate initilization, didn't even know you could do a regular constructor that way :)

Comment 73

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #71)
>       struct A {
>         A(int);
>       };
>       struct S {
>         S(A, A);
>       };
>       void foo(int x, int y) {
>         S foo(A(x), A(y));  // #1
>       }

(Oops, I meant to use a different name than "foo" on line #1.)

Comment 74

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Introduce a FrameMetrics::ClampAndSetScrollOffset() helper.

Review commit: https://reviewboard.mozilla.org/r/162628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162628/

Comment 75

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - ScrollInputMethod telemetry for APZ autoscrolling.

Review commit: https://reviewboard.mozilla.org/r/162630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162630/

Comment 76

[Botond Ballo [:botond]]

Comment on attachment 8879330 [details]
Bug 1105109 - Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150618/diff/4-5/

Comment 77

[Botond Ballo [:botond]]

Comment on attachment 8879331 [details]
Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150620/diff/4-5/

Comment 78

[Botond Ballo [:botond]]

Comment on attachment 8879332 [details]
Bug 1105109 - Have APZCTreeManager keep track of the current mouse position.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150622/diff/4-5/

Comment 79

[Botond Ballo [:botond]]

Comment on attachment 8879333 [details]
Bug 1105109 - Add a ScrollByAndClamp() utility function to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150624/diff/4-5/

Comment 80

[Botond Ballo [:botond]]

Comment on attachment 8879335 [details]
Bug 1105109 - Add autoscrolling support to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150628/diff/4-5/

Comment 81

[Botond Ballo [:botond]]

Comment on attachment 8879336 [details]
Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150630/diff/4-5/

Comment 82

[Botond Ballo [:botond]]

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159008/diff/3-4/

Comment 83

[Botond Ballo [:botond]]

Comment on attachment 8890591 [details]
Bug 1105109 - Put APZ autoscrolling behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161754/diff/1-2/

Comment 84

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

Comment on attachment 8879331 [details]
Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

https://reviewboard.mozilla.org/r/150620/#review167976

Comment 85

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

Comment on attachment 8891506 [details]
Bug 1105109 - Introduce a FrameMetrics::ClampAndSetScrollOffset() helper.

https://reviewboard.mozilla.org/r/162628/#review167978

Comment 86

[Botond Ballo [:botond]]

The updated patch series addresses all review comments except this one:

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #66)
> I'm a little concerned about the interaction between autoscroll and other
> input mechanisms. From this patch it looks like if you do a touch-down
> during an autoscroll animation it will abort the APZ-side autoscroll
> animation, but I think the browser-content.js code will probably still think
> it's in an autoscroll. This might result in the autoscroll visual indicator
> remaining up, but i'm not sure. I'll have to play with the different
> interactions and see what happens. We can deal with those in follow-up bugs
> but I just wanted to write it down so we don't forget.

The idea is that the kinds of input events that trigger a CancelAnimation should also, independently, trigger the end of autoscrolling in browser.xml.

However, you're right that there are probably some input events for which that's not true, and we should probably also have a message from APZ to browser.xml to stop autoscrolling to be safe. I'll leave that for a follow-up.

Comment 87

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

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

https://reviewboard.mozilla.org/r/159008/#review167980

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:27
(Diff revision 4)
>  #include "nsIScrollableFrame.h"
>  #include "nsLayoutUtils.h"
>  #include "nsIInterfaceRequestorUtils.h"
>  #include "nsIContent.h"
>  #include "nsIDocument.h"
> +#include "nsIDocShell.h"

Include isn't needed any more

::: toolkit/content/browser-content.js:48
(Diff revision 4)
>      this.autoscrollLoop = this.autoscrollLoop.bind(this);
>  
>      Services.els.addSystemEventListener(global, "mousedown", this, true);
>  
>      addMessageListener("Autoscroll:Stop", this);
> +    

trailing ws

::: toolkit/content/browser-content.js:250
(Diff revision 4)
> -      left: actualScrollX,
> +        left: actualScrollX,
> -      top: actualScrollY,
> +        top: actualScrollY,
> -      behavior: "instant"
> +        behavior: "instant"
> -    });
> +      });
> +    }
>      content.requestAnimationFrame(this.autoscrollLoop);

I think it might be better to move this requestAnimationFrame inside the if condition as well. I don't think there's anything in autoscrollLoop that needs to execute once APZ takes over, right?

::: toolkit/content/browser-content.js:284
(Diff revision 4)
>          this.stopScroll();
>          break;
>        }
>      }
>    },
> +  

trailing ws

Comment 88

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

Comment on attachment 8891507 [details]
Bug 1105109 - ScrollInputMethod telemetry for APZ autoscrolling.

https://reviewboard.mozilla.org/r/162630/#review167984

Comment 89

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

(In reply to Botond Ballo [:botond] from comment #86)
> The idea is that the kinds of input events that trigger a CancelAnimation
> should also, independently, trigger the end of autoscrolling in browser.xml.
> 
> However, you're right that there are probably some input events for which
> that's not true, and we should probably also have a message from APZ to
> browser.xml to stop autoscrolling to be safe. I'll leave that for a
> follow-up.

Sounds good, thanks! (And thanks also for the easy-to-review patch series! :))

Comment 90

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #87)
> I think it might be better to move this requestAnimationFrame inside the if
> condition as well. I don't think there's anything in autoscrollLoop that
> needs to execute once APZ takes over, right?

It was useful for me to compare the values computed by content and the values computed by APZ when debugging coordinate issues :) But now there's no longer any reason. I changed it to just early-exit near the top of autoscrollLoop().

Comment 91

[Botond Ballo [:botond]]

Comment on attachment 8879336 [details]
Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150630/diff/5-6/

Comment 92

[Botond Ballo [:botond]]

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159008/diff/4-5/

Comment 93

[Botond Ballo [:botond]]

Comment on attachment 8890591 [details]
Bug 1105109 - Put APZ autoscrolling behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161754/diff/2-3/

Comment 94

[Botond Ballo [:botond]]

Comment on attachment 8891507 [details]
Bug 1105109 - ScrollInputMethod telemetry for APZ autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162630/diff/1-2/

Comment 95

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44d50b3f0a57f20fdc42452d115ce3ab696cadcc

Comment 96

[Botond Ballo [:botond]]

Moved Quantum tracking flags over to bug 1385463.

Comment 97

[Botond Ballo [:botond]]

There were a bunch of unified compilation errors on Windows and Android which, after like 10 rounds of Try pushes, are hopefully fixed now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b0860772c6b8b76b99b8d30933f1c3a8809f60

Comment 98

[Botond Ballo [:botond]]

Attachment: Bug 1105109 - Fix unified compilation errors.

Review commit: https://reviewboard.mozilla.org/r/162690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162690/

Comment 99

[Botond Ballo [:botond]]

Comment on attachment 8879330 [details]
Bug 1105109 - Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150618/diff/5-6/

Comment 100

[Botond Ballo [:botond]]

Comment on attachment 8879331 [details]
Bug 1105109 - Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150620/diff/5-6/

Comment 101

[Botond Ballo [:botond]]

Comment on attachment 8879332 [details]
Bug 1105109 - Have APZCTreeManager keep track of the current mouse position.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150622/diff/5-6/

Comment 102

[Botond Ballo [:botond]]

Comment on attachment 8891506 [details]
Bug 1105109 - Introduce a FrameMetrics::ClampAndSetScrollOffset() helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162628/diff/1-2/

Comment 103

[Botond Ballo [:botond]]

Comment on attachment 8879333 [details]
Bug 1105109 - Add a ScrollByAndClamp() utility function to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150624/diff/5-6/

Comment 104

[Botond Ballo [:botond]]

Comment on attachment 8879335 [details]
Bug 1105109 - Add autoscrolling support to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150628/diff/5-6/

Comment 105

[Botond Ballo [:botond]]

Comment on attachment 8879336 [details]
Bug 1105109 - Have the parent process notify APZ of the start and stop of autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150630/diff/6-7/

Comment 106

[Botond Ballo [:botond]]

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159008/diff/5-6/

Comment 107

[Botond Ballo [:botond]]

Comment on attachment 8890591 [details]
Bug 1105109 - Put APZ autoscrolling behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161754/diff/3-4/

Comment 108

[Botond Ballo [:botond]]

Comment on attachment 8891507 [details]
Bug 1105109 - ScrollInputMethod telemetry for APZ autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162630/diff/2-3/

Comment 109

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

Comment on attachment 8891577 [details]
Bug 1105109 - Fix unified compilation errors.

https://reviewboard.mozilla.org/r/162690/#review168056

Comment 110

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #97)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=42b0860772c6b8b76b99b8d30933f1c3a8809f60

This is building now on all platforms (the Windows XP and Windows 8 build failures seem infrastructure-related), but the "Linux x64 debug" browser chrome mochitests are failing due to leaks.

Comment 111

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #110)
> the "Linux x64 debug" browser
> chrome mochitests are failing due to leaks.

Bisection pointed to the "Notify content when APZ is handling an autoscroll" patch.

I noticed that in that patch, we register an observer in browser-content.js but never remove it. I modified the code to register the observer in startScroll() rather than init(), and remove it in stopScroll(). Let's see if that fixes the leak:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e11e83a1159001da062895ab128fc02837b7689

Comment 112

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #111)
> Let's see if that fixes the leak:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0e11e83a1159001da062895ab128fc02837b7689

Yup, that looks much better!

Comment 113

[Botond Ballo [:botond]]

Comment on attachment 8888117 [details]
Bug 1105109 - Notify content when APZ is handling an autoscroll.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159008/diff/6-7/

Comment 114

[Botond Ballo [:botond]]

Comment on attachment 8890591 [details]
Bug 1105109 - Put APZ autoscrolling behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161754/diff/4-5/

Comment 115

[Botond Ballo [:botond]]

Comment on attachment 8891507 [details]
Bug 1105109 - ScrollInputMethod telemetry for APZ autoscrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162630/diff/3-4/

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3b7b55bfb28
Fix unified compilation errors. r=kats
https://hg.mozilla.org/integration/autoland/rev/54025a37b21b
Have nsIDOMWindowUtils.getPresShellId() return the pres shell id via the return value rather than an out-parameter. r=kats
https://hg.mozilla.org/integration/autoland/rev/05ed5feb9c46
Have content send the parent process a scroll id and pres shell id as part of the Autoscroll:Start message. r=kats
https://hg.mozilla.org/integration/autoland/rev/9d646df30d90
Have APZCTreeManager keep track of the current mouse position. r=kats
https://hg.mozilla.org/integration/autoland/rev/c52debb710fa
Introduce a FrameMetrics::ClampAndSetScrollOffset() helper. r=kats
https://hg.mozilla.org/integration/autoland/rev/bb484002766e
Add a ScrollByAndClamp() utility function to AsyncPanZoomController. r=kats
https://hg.mozilla.org/integration/autoland/rev/6b28c83f4ef4
Add autoscrolling support to AsyncPanZoomController. r=kats
https://hg.mozilla.org/integration/autoland/rev/ab07a3654f59
Have the parent process notify APZ of the start and stop of autoscrolling. r=kats
https://hg.mozilla.org/integration/autoland/rev/3ec1e0cf48cb
Notify content when APZ is handling an autoscroll. r=kats
https://hg.mozilla.org/integration/autoland/rev/598ea131759f
Put APZ autoscrolling behind a pref. r=kats
https://hg.mozilla.org/integration/autoland/rev/9a5b55eec89f
ScrollInputMethod telemetry for APZ autoscrolling. r=kats

Comment 117

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f3b7b55bfb28
https://hg.mozilla.org/mozilla-central/rev/54025a37b21b
https://hg.mozilla.org/mozilla-central/rev/05ed5feb9c46
https://hg.mozilla.org/mozilla-central/rev/9d646df30d90
https://hg.mozilla.org/mozilla-central/rev/c52debb710fa
https://hg.mozilla.org/mozilla-central/rev/bb484002766e
https://hg.mozilla.org/mozilla-central/rev/6b28c83f4ef4
https://hg.mozilla.org/mozilla-central/rev/ab07a3654f59
https://hg.mozilla.org/mozilla-central/rev/3ec1e0cf48cb
https://hg.mozilla.org/mozilla-central/rev/598ea131759f
https://hg.mozilla.org/mozilla-central/rev/9a5b55eec89f

Comment 118

[avada]

Hi!

So I tested apz autoscoll a bit. And it was immediately obvious that it's much more sensitive than normal scolling. For the same amount of movement it accelerates a lot more. It makes it a lot more difficult to use effectively.


(OS: Windows 8.1)

Comment 119

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

I just tried it on the latest Nightly on a Win 10 machine and I agree, it feels like we get a larger scroll amount for the same mousemove delta.

Comment 120

[Botond Ballo [:botond]]

(In reply to avada from comment #118)
> So I tested apz autoscoll a bit. 

Thanks for testing it!

> And it was immediately obvious that it's
> much more sensitive than normal scolling. For the same amount of movement it
> accelerates a lot more. It makes it a lot more difficult to use effectively.

I filed bug 1386742 for this.

I had noticed the speed difference during testing, but as I was testing with a debug build, I assumed the reason was that the main thread can't paint at anywhere near 60 fps in a debug build. But now I've tested with an opt build and I still see it, so there's more to it.

Comment 121

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #66)
> I'm a little concerned about the interaction between autoscroll and other
> input mechanisms. From this patch it looks like if you do a touch-down
> during an autoscroll animation it will abort the APZ-side autoscroll
> animation, but I think the browser-content.js code will probably still think
> it's in an autoscroll. This might result in the autoscroll visual indicator
> remaining up, but i'm not sure. I'll have to play with the different
> interactions and see what happens. We can deal with those in follow-up bugs
> but I just wanted to write it down so we don't forget.

FWIW I spent a bit of time today playing with different interactions between autoscroll and other input methods and in all cases that I could think of the APZ autoscroll behaved the same as the non-APZ autoscroll. So that's good :)