Closed Bug 1105109 (apz-autoscrolling) Opened 5 years ago Closed 3 years ago

APZ: Use APZ for autoscrolling

Categories

(Core :: Panning and Zooming, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mitchell, Assigned: botond)

References

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(11 files, 3 obsolete files)

59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
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
What about keyboard scrolling? Up/down and Page up/down?
How can you tell? To me scrolling is usually smooth even without APZ.
(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.
(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?
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.
(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.
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?
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.
Is this still the case? Hotkey scrolling seems to be smoother to me also.
(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?
Status: UNCONFIRMED → NEW
Depends on: 1238137
Ever confirmed: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
I would just call it keyboard scrolling.
Keywords: feature
Whiteboard: [gfx-noted]
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
Yes, APZ is not yet supported for autoscrolling.
Summary: Autoscrolling events don't cause apz scrolling when apz is enabled → Use APZ for autoscrolling
Blocks: QuantumFlow
Whiteboard: [gfx-noted] → [gfx-noted][qf]
Whiteboard: [gfx-noted][qf] → [gfx-noted][qf:p1]
Duplicate of this bug: 1327543
Will look at keyboard input first (bug 1351783.)  Parked until that one is done.
See Also: → apz-keyboard
(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.
Assignee: nobody → 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.
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.
Alias: apz-autoscrolling
Summary: Use APZ for autoscrolling → APZ: Use APZ for autoscrolling
Now split into parts and beginning to resemble what will be the final patch series. Still very much a WIP.
(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?
(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 :)
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
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.
Attachment #8879334 - Attachment is obsolete: true
Attachment #8889700 - Attachment is obsolete: true
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 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
Attachment #8879330 - Flags: review?(bugmail) → review+
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.
Attachment #8879331 - Flags: review?(bugmail)
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.
Attachment #8879332 - Flags: review?(bugmail) → review+
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?
Attachment #8879333 - Flags: review?(bugmail) → review+
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
Attachment #8879335 - Flags: review?(bugmail) → review+
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
Attachment #8879336 - Flags: review?(bugmail) → review+
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
Attachment #8888117 - Flags: review?(bugmail)
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.
Attachment #8890591 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
I always read {} as aggregate initilization, didn't even know you could do a regular constructor that way :)
(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.)
Attachment #8888116 - Attachment is obsolete: true
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
Attachment #8879331 - Flags: review?(bugmail) → review+
Comment on attachment 8891506 [details]
Bug 1105109 - Introduce a FrameMetrics::ClampAndSetScrollOffset() helper.

https://reviewboard.mozilla.org/r/162628/#review167978
Attachment #8891506 - Flags: review?(bugmail) → review+
The updated patch series addresses all review comments except this one:

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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
Attachment #8888117 - Flags: review?(bugmail) → review+
Comment on attachment 8891507 [details]
Bug 1105109 - ScrollInputMethod telemetry for APZ autoscrolling.

https://reviewboard.mozilla.org/r/162630/#review167984
Attachment #8891507 - Flags: review?(bugmail) → review+
(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! :))
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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().
Blocks: 1385463
Moved Quantum tracking flags over to bug 1385463.
No longer blocks: QuantumFlow, QRC_FX57
Whiteboard: [gfx-noted][qf:p1] → [gfx-noted]
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 on attachment 8891577 [details]
Bug 1105109 - Fix unified compilation errors.

https://reviewboard.mozilla.org/r/162690/#review168056
Attachment #8891577 - Flags: review?(bugmail) → review+
(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.
(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
(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!
Pushed by bballo@mozilla.com:
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
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)
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.
Blocks: 1386742
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :)
Blocks: 1390247
You need to log in before you can comment on or make changes to this bug.