961280 - Scrolling in Metro Firefox with mouse wheel, trackpad, or keyboard abruptly becomes very slow

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Rodrigo Silveira [:rsilveira] [:rodms]]

Started noticing this as of this week. Mouse wheel scroll gets really slow. If I right click to show tab bar and scroll again it's fast for the first scroll, then gets back to being slow for next scrolls.

Comment 1

[Matt Brubeck (:mbrubeck)]

This regressed on Aurora between the 01-16 nightly and the 01-17 nightly:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=1dd80f8faa2f&tochange=2c033140eff4

I'm working on bisecting further.

Comment 2

[Matt Brubeck (:mbrubeck)]

The first bad revision is:

changeset:   178259:b3b07adaf7cd
user:        Kartikaya Gupta <[e-mail]>
date:        Tue Jan 14 16:43:43 2014 -0500
summary:     Bug 951113 - Trigger a repaint request when getting a scroll offset update

Testing notes:  The bug can't be reproduced on the Firefox Start page.  Sometimes I have to use the mouse wheel several times to trigger the bug, but I can always trigger it by scrolling back and forth for 10 seconds or so.

Comment 3

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

Hm. The second patch on bug 951113 is likely the culprit. It assumes that scrolling is mostly being "driven" via touch. When you're scrolling with the mouse wheel this assumption is broken and we'll end up painting twice as much. :(

Comment 4

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

Attachment: Patch

Turns out it's not extra painting that's the problem, it's that when we request the paint, we interrupt the "smooth scroll" that the mouse wheel triggers. The attached patch helps significantly on my Surface but doesn't fix the problem entirely because there's a race with the APZ's repaint and the content's incremental update of the scroll offset as it does the smooth scrolling. A better long-term (but much more involved) is to add a smooth scroll API to APZ and use that. I recall discussion about this on some bug somewhere but can't find it now.

Rodrigo (and Matt) - could you test with this patch to see if the result is acceptable?

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8362084 [details] [diff] [review]
Patch

Scrolling is improved with this patch but still very visibly regressed.  Instead of moving smoothly, it goes in fits and starts, repeatedly speeding up and then slowing to a crawl.

Comment 6

[Rodrigo Silveira [:rsilveira] [:rodms]]

I'm getting the same behavior Matt described, plus it sometime jumps as a page up/down.

Comment 7

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

After giving this some thought I think the only reasonable solution here is to put in the right fix for the long term. This involves routing the WidgetWheelEvent to the APZ rather than gecko, and having the APZ code implement smooth scrolling in the compositor.

Comment 8

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

Incidentally this would also fix the problem where if you scroll with the wheel while zoomed in, you scroll farther than if you are zoomed out. (Same amount of content, but it encompasses more pixels on the screen).

Comment 9

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

Attachment: Part 1 - Add APZ code to support smooth scrolling

Comment 10

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

Attachment: Part 2 - Hook up metro widget code to send wheel inputs to APZ

Comment 11

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

The above two patches route mouse wheel events through the APZ. I haven't tested this extensively but the basic feature works. The actual scroll amounts need to be tuned some more, and we might need to still dispatch the wheel events to content - I don't know how hard it will be to do that without also triggering scrolling code in nsEventStateManager. But presumably it can be done.

If anybody else wants to take these patches and run with them feel free to do so.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Created attachment 8362084 [details] [diff] [review]
> Patch
> 
> Turns out it's not extra painting that's the problem, it's that when we
> request the paint, we interrupt the "smooth scroll" that the mouse wheel
> triggers. The attached patch helps significantly on my Surface but doesn't
> fix the problem entirely because there's a race with the APZ's repaint and
> the content's incremental update of the scroll offset as it does the smooth
> scrolling. A better long-term (but much more involved) is to add a smooth
> scroll API to APZ and use that. I recall discussion about this on some bug
> somewhere but can't find it now.

That was in bug 907100.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> The above two patches route mouse wheel events through the APZ. I haven't
> tested this extensively but the basic feature works. The actual scroll
> amounts need to be tuned some more, and we might need to still dispatch the
> wheel events to content - I don't know how hard it will be to do that
> without also triggering scrolling code in nsEventStateManager. But
> presumably it can be done.
> 
> If anybody else wants to take these patches and run with them feel free to
> do so.

Note bug 23456, comment 16 and bug bug 23456, comment 17. There's a lot to support here including event stream cancellation and legacy desktop wheel prefs. Any chance we could punt on this and instead just fix the dom smooth scroll issue?

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> > The above two patches route mouse wheel events through the APZ. I haven't
> > tested this extensively but the basic feature works. The actual scroll
> > amounts need to be tuned some more, and we might need to still dispatch the
> > wheel events to content - I don't know how hard it will be to do that
> > without also triggering scrolling code in nsEventStateManager. But
> > presumably it can be done.
> > 
> > If anybody else wants to take these patches and run with them feel free to
> > do so.
> 
> Note bug 23456, comment 16 and bug bug 23456, comment 17. There's a lot to
> support here including event stream cancellation and legacy desktop wheel
> prefs. Any chance we could punt on this and instead just fix the dom smooth
> scroll issue?

Sorry, I meant bug 907100, comment 16 and bug 907100, comment 17.

Comment 15

[Jim Mathies [:jimm]]

Also, we need a wheel scroll metro mochitest test to catch further regressions here. Someone on the metro team can help put that together if you like.

Comment 16

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

So then I can think of a couple of options. One is to create a new scroll type (APZC) that just returns early somewhere around [1] after processing all the prefs and computing the scroll amount, but before actually doing the scroll. Then the widget code can take that and trigger an APZ scroll.

The other option is to expose via nsIScrollableFrame whether or not a smooth scroll is happening, and then check that at [2] and not do the scroll if it is. This would be the easier approach but is also more brittle and probably not the best solution for the long-term. It also means that legitimate interruptions of smooth scrolling via touch input won't happen (i.e. you do a fling with the wheel and then attempt to stop it with your finger). But for a short term solution it's low effort to implement and debug.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/events/nsEventStateManager.cpp?rev=9961f372f396#2969
[2] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp?rev=3f13d3dd021d#104

Comment 17

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

I'm testing out that second option now.

Comment 18

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

Attachment: Alternate patch (short-term workaround)

Seems to work ok for me. What do you guys think?

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 8363045 [details] [diff] [review]
Alternate patch (short-term workaround)

This reverts back to the original behavior afaict.

Comment 20

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

Btw I filed bug 962218 for the long-term plan of smooth scrolling in APZ code.

Comment 22

[Matt Brubeck (:mbrubeck)]

Restoring the beta28 flag.  This breaks core functionality and has become our top feedback issue, so we need to track this for Metro Firefox 28.

Comment 23

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

Trees are closed :(

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbc3cd1bf54

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0fbc3cd1bf54

Comment 26

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

Comment on attachment 8363045 [details] [diff] [review]
Alternate patch (short-term workaround)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 951113
User impact if declined: scrolling with a mouse wheel is really slow
Testing completed (on m-c, etc.): locally by a few people, on m-c now
Risk to taking this patch (and alternatives if risky): low risk; should only affect metrofox
String or IDL/UUID changes made by this patch: none

Note: this needs to be uplifted to 28. Since we are close to the next merge this might need beta approval instead of aurora approval if we go past the merge.

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 8363045 [details] [diff] [review]
Alternate patch (short-term workaround)

Given low risk and impacts only approving for uplift. Adding verify for QA verification

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7fb6d40e554f

Comment 29

[Manuela Muntean [Away]]

Verified as fixed with Firefox 28 beta 7 on a Dell XPS 12 ultrabook with Windows 8 64-bit.

Comment 30

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Aurora (build ID: 20140304004003) using a Microsoft Surface Pro 2 device running Windows 8.1 64bit.