1217399 - APZ on Facebook: Needs to handle elements scrolled via javascript; currently scrolling visibly jumps around

Status: RESOLVED WORKSFORME

(Core :: Panning and Zooming, defect)

Description

[gordon]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.71 Safari/537.36

Steps to reproduce:

Login to www.facebook.com and scroll the friends list on the right hand side with the mousewheel.


Actual results:

Scrolling visibly jumps around.


Expected results:

Scrolling with the mousewheel should be smooth

This appears to occur wherever an application has overridden scrolling behavior via javascript using "e.preventDefault();" in the mousewheel handler and then controlling it themselves. Or where they are setting the elements "scrollTop" position.

APZ needs to correctly work with the various javascript scroll API's/commands.

Comment 1

[ovidiu boca[:Ovidiu]]

I could reproduce the bug in Windows 10 x64 and Windows 7 x64 Firefox version 41.0.2

Steps to reproduce:

Login to www.facebook.com and scroll the friends list on the right hand side with the mousewheel.


Actual results:

Scrolling visibly jumps around.


Expected results:

Scrolling with the mousewheel should be smooth

Comment 2

[ovidiu boca[:Ovidiu]]

Hi, 

I Reproduced with the latest nightly 44.0a1 (2015-10-25) on Win 7 and 10 64 bit.

Comment 3

[gordon]

I have very little in the way of technical understanding around how APZ has been implemented in Firefox.  But I assume what is required here is for calls made via javascript to "window.scroll()" should go directly into the compositor thread and into the APZ path, when enabled.

I'm sure one of the Mozilla guys will correct me if I'm wrong.

Comment 4

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

Ovidiu, why did you move this to the XUL component? APZ-related bugs should be in the Panning and Zooming component. That being said, this is probably an issue on their end. If the page is intercepting wheel events and driving scrolling via window.scrollTo or other similar APIs they can already send it as a compositor scroll by using the smooth scroll APIs. So they need to do that, and nothing needs to be done on our end.

I'll leave this open until I have a chance to verify this.

Comment 5

[gordon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> If the page is intercepting wheel events and driving
> scrolling via window.scrollTo or other similar APIs they can already send it
> as a compositor scroll by using the smooth scroll APIs. So they need to do
> that, and nothing needs to be done on our end.
> 
> I'll leave this open until I have a chance to verify this.

This does not work.  It results in even more jumpy/janky behaviour than before.  If you intercept the mousewheel with "e.preventefault();", normalize the delta and finally call window.scrollTo({left:0, top:100, behavior:smooth}) for each "pulse" of the mousewheel, then the proceeding, running animation is not taken into account and everything jumps every time you move the wheel.  The animation basically starts accelerating from it's stopping position again, rather than continue from it's with its current acceleration/velocity.

Calling window.scrollTo without behavior:smooth does not currently go directly into the compositor.  Therefore whenever a site uses javascript to scroll and has APZ enabled the result is jumpy and janky  because the min thread and the compositor thread are not in sync in terms of scroll position.

The only two solutions I can think of with my limited understanding of APZ are:

1: Ensure that all window.scroll/window.scrollTo calls go straight into the compositor.
2: Ensure that when using behavior:smooth, any subsequent calls that are made while an animation is already running, continue from the current point of acceleration/velocity rather than start from the animations resting point.

I would suggest that both should be implemented.  But again, I have to admit my limited understanding of APZ.  Is it stands, any sites that have to intercept and scroll via javascript are essentially broken with APZ enabled.

You could argue that they shouldn't be doing that in the first place, but in a lot of cases (particularly Facebook) this is unfeasible as the friends bar would then have nested scrollbars since Firefox has no way to customize their look.

Comment 6

[gordon]

Attachment: scrolling_example.html

Here's a testcase showing what javascript scrolling using the current "smooth scroll" API is like.

While it is perfect for single scrolls, made with a single call to scroll/scrollTo/scrollBy, as soon as you send more "pulses" from the mousewheel while the previous animation is running, everything screws up.

This example uses "window.scrollBy" as opposed to "window.scroll/window.scrollTo" because that way there is no need to track the scroll position in javascript in the main thread, which of course will be out of sync with the compositor.

addWheelListener(document.body, function(e){
	e.preventDefault();
        var delta = (e.deltaY > 0) ? 1 : -1;
        window.scrollBy({
		left : 0,
		top : (delta * 100),
		behavior : 'smooth'
	});
});

Comment 7

[gordon]

The result is even worse if you decouple the call from the mousewheel handler using "requestAnimationFrame".

Comment 8

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

Not sure I fully understand the issue yet, but some comments:

(In reply to gordon from comment #5)
> If you intercept the mousewheel with "e.preventefault();",
> normalize the delta and finally call window.scrollTo({left:0, top:100,
> behavior:smooth}) for each "pulse" of the mousewheel, then the proceeding,
> running animation is not taken into account and everything jumps every time
> you move the wheel.

I'm assuming you meant scrollBy rather than scrollTo here, based on your test case.

>  The animation basically starts accelerating from it's
> stopping position again, rather than continue from it's with its current
> acceleration/velocity.
> 
> Calling window.scrollTo without behavior:smooth does not currently go
> directly into the compositor.  Therefore whenever a site uses javascript to
> scroll and has APZ enabled the result is jumpy and janky  because the min
> thread and the compositor thread are not in sync in terms of scroll position.

I'm not sure why this would be any jumpier or jankier than calling window.scrollTo without APZ. In other words, doing this:

for (i = 0; i < 100; i++) { window.scrollTo(0, i); }

should be pretty much the same with and without APZ. Without APZ it is handled entirely on the main thread, and paints are sent over to the compositor for each scroll. With APZ the scrollTo calls also trigger paints on the main thread and the APZ is updated with the new scroll offsets as they arrive in the compositor. I don't see why this would be any jumpier or jankier with APZ than without.

(In reply to gordon from comment #6)
> Created attachment 8679593 [details]
> scrolling_example.html
> 
> Here's a testcase showing what javascript scrolling using the current
> "smooth scroll" API is like.

I tried this on OS X with and without APZ and didn't really notice much jank in any scenario. However, it scrolls a lot slower with APZ. It looks like if I scroll a bit, and say the code does four calls to scrollBy({... top:100 ...}), it doesn't actually scroll by 400 pixels. Instead the four animations kind of overlap/clobber each other and so you end up scrolling somewhere between 100 and 400 pixels, and it's not a smooth animation curve because each animation is interrupting the previous ones. I'm assuming this is the problem you're referring to.

(In reply to gordon from comment #5)
> The only two solutions I can think of with my limited understanding of APZ
> are:
> 
> 1: Ensure that all window.scroll/window.scrollTo calls go straight into the
> compositor.

I would say this instead (not sure if it's the same thing you're saying, but it fits better with my mental model of how everything works): what's happening here is that if a behavior:smooth scrollBy call interrupts an already-running behavior:smooth animation, the new scrollBy is taking the transient scroll offset from the first animation rather than the destination scroll offset. It should use the destination scroll offset so as to not clobber part of the scroll from the previous animation. This applies for scrollBy interrupting both scrollBy and scrollTo, but doesn't matter to scrollTo interrupting a previous scrollBy or scrollTo, because scrollTo takes absolute coordinates anyway.

This sounds reasonable enough, but I'm not sure if the current behaviour is intentional or not. I vaguely recall some tests actually testing this sort of thing so I'll have to dig a bit to see if this is specced or done on purpose.

> 2: Ensure that when using behavior:smooth, any subsequent calls that are
> made while an animation is already running, continue from the current point
> of acceleration/velocity rather than start from the animations resting point.

So the way things are implemented now with APZ, a second behavior:smooth animation (either scrollTo or scrollBy) interrupting a previous one will cancel the first one and then start a new animation from scratch. What I think you're saying is that the animation should continue but to the new destination target. I believe the main-thread implementation of this behaves that way, and we should make APZ do the same. I agree with that. (For my own future reference, this would be done at [1] by taking the existing smooth-scroll animation if there is one and updating the destination rather than canceling and starting a new one).

Am I interpreting what you said correctly? Or am I way off the mark?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=ed2fdc36728d&mark=3170-3172#3170

Comment 9

[gordon]

> I'm assuming you meant scrollBy rather than scrollTo here, based on your
> test case.

On Windows, at least on my particular configuration, the issue persists regardless of whether I use scrollto or scrollby. Scrollto is smoother however, I assume because the compositor ends up syncing to the main thread more regularly than when using scrollby.


> > Calling window.scrollTo without behavior:smooth does not currently go
> > directly into the compositor.  Therefore whenever a site uses javascript to
> > scroll and has APZ enabled the result is jumpy and janky  because the min
> > thread and the compositor thread are not in sync in terms of scroll position.
> 
> I'm not sure why this would be any jumpier or jankier than calling
> window.scrollTo without APZ. In other words, doing this:
> 
> for (i = 0; i < 100; i++) { window.scrollTo(0, i); }
> 
> should be pretty much the same with and without APZ. Without APZ it is
> handled entirely on the main thread, and paints are sent over to the
> compositor for each scroll. With APZ the scrollTo calls also trigger paints
> on the main thread and the APZ is updated with the new scroll offsets as
> they arrive in the compositor. I don't see why this would be any jumpier or
> jankier with APZ than without.

I don't understand it either but I lose between 10-20 FPS on average, according to the Dec tools profiler and the experience is noticeable different. In layman a terms it feels as though things are jumping every now and then in order to sync between the main/composited thread. 


> I tried this on OS X with and without APZ and didn't really notice much jank
> in any scenario.

On Windows 10 the experience is horrid. If I scroll the mousewheel reasonable quickly the scroll position sticks, stays still, then after a second or so, when the mouse wheel pulse rate has slowed enough to allow a full animation to be completed between pulses, the scroll position finally updates, jumping down or up the page.


> it's not a smooth animation curve because
> each animation is interrupting the previous ones. I'm assuming this is the
> problem you're referring to.

Exactly. It appears not to be as pronounced on a Mac... But yes this is the problem I am referring to.  I do not have an apple device to hand with which to test I'm afraid.


> scrollTo interrupting a previous scrollBy or scrollTo, because scrollTo
> takes absolute coordinates

I'm afraid I don't understand why this should be either, but the experience and the resulting FPS are noticeable worse with APZ enabled. in layman a terms I would assume it is down to javing to calculate a new scroll position in the main thread, then sending it to APZ which potentially has a different scroll positions altogether. While this would be fine for instant scroll position changes. If you are trying to do anything more interesting such as using JavaScript to implement your own smooth scrolling or rendering your own DOM based scroll bars. Then I'm afraid the result is poor at best. And this is exactly what Facebook does.

> Am I interpreting what you said correctly? Or am I way off the mark?
 
Yes. You appear to have the gist of the issue I am encountering. Animation clobbering on a currently animating scroll position. My apologies for not being able to articulate this adequately.

Comment 10

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

Gordon, bug 1228407 should have improved this a lot - please try this again on tomorrow's nightly and let me know if there are still things that need fixing. Thanks!

Comment 11

[gordon]

Kartikaya, the experience on Facebook does indeed appear to be improved.  It still 'sticks' at either when you are scrolled to either the top or the very bottom.  But that maybe actually be down to a facebook implementation issue rather than a Firefox/APZ one.

However, attempting to scroll with the mousewheel using scroll/scrollBy({top : top, scroll-behavior:smooth}); still does not work as expected.  Successive calls to start the animation, still interrupt the previously running one.  Strangely, it is better than before.  Now, as long as a scroll reasonably slowly the experience is okay.  But if I scroll the mousewheel very quickly, as in 'flicking' the mousewheel, the scrolling actually slows down.

It's good to see improvements are being made however.

Comment 12

[Michelle Funches - QA]

Name 	Firefox
Version 	47.0a1
Build ID 	20160201030241
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0

Comment 13

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

Gordon, feel free to reopen this bug for issues you feel are outstanding, and that you can create test cases for. Thanks! (and sorry for the long turnaround on some of these bugs)