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

VERIFIED FIXED in Firefox 28

Status

()

Core
Panning and Zooming
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: rsilveira, Assigned: kats)

Tracking

(Blocks: 1 bug, {regression})

29 Branch
mozilla29
x86_64
Windows 8.1
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 unaffected, firefox28 verified, firefox29 verified)

Details

(Whiteboard: [beta28] [metro] [gfx] r=ff28)

Attachments

(1 attachment, 3 obsolete attachments)

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.
status-firefox28: --- → affected
status-firefox29: --- → affected
Keywords: regressionwindow-wanted
Version: unspecified → 28 Branch
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.
The first bad revision is:

changeset:   178259:b3b07adaf7cd
user:        Kartikaya Gupta <kgupta@mozilla.com>
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.
Keywords: regressionwindow-wanted → regression
Summary: Scrolling with mouse wheel is really slow → Scrolling in Metro Firefox with mouse wheel abruptly becomes very slow
Whiteboard: [beta28]
Version: 28 Branch → 29 Branch
Blocks: 951113, 838081
Component: General → Panning and Zooming
Product: Firefox for Metro → Core
Whiteboard: [beta28] → [metro] [beta28] [defect] p=0
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. :(
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.

Rodrigo (and Matt) - could you test with this patch to see if the result is acceptable?
Attachment #8362084 - Flags: feedback?(rsilveira)
Attachment #8362084 - Attachment is patch: true
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.
Attachment #8362084 - Flags: feedback?(rsilveira) → feedback-
I'm getting the same behavior Matt described, plus it sometime jumps as a page up/down.
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.
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).
Created attachment 8362635 [details] [diff] [review]
Part 1 - Add APZ code to support smooth scrolling
Attachment #8362084 - Attachment is obsolete: true
Created attachment 8362637 [details] [diff] [review]
Part 2 - Hook up metro widget code to send wheel inputs to APZ
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

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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

4 years ago
(In reply to Jim Mathies [:jimm] from comment #13)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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

4 years ago
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.
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
I'm testing out that second option now.
Created attachment 8363045 [details] [diff] [review]
Alternate patch (short-term workaround)

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

Comment 19

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

This reverts back to the original behavior afaict.
Attachment #8363045 - Flags: feedback+
Attachment #8363045 - Flags: review?(tnikkel)
Btw I filed bug 962218 for the long-term plan of smooth scrolling in APZ code.

Updated

4 years ago
Assignee: nobody → bugmail.mozilla
Whiteboard: [metro] [beta28] [defect] p=0 → [metro] [beta28]

Updated

4 years ago
Whiteboard: [metro] [beta28] → [metro] [beta28] [gfx]

Updated

4 years ago
Blocks: 844132
Duplicate of this bug: 960992

Updated

4 years ago
Blocks: 861680, 963116
No longer blocks: 838081, 844132
Whiteboard: [metro] [beta28] [gfx] → [metro] [gfx]
status-firefox27: --- → unaffected
tracking-firefox28: --- → ?
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.
tracking-firefox28: ? → ---
Summary: Scrolling in Metro Firefox with mouse wheel abruptly becomes very slow → Scrolling in Metro Firefox with mouse wheel, trackpad, or keyboard abruptly becomes very slow
Whiteboard: [metro] [gfx] → [beta28] [metro] [gfx]

Updated

4 years ago
Blocks: 838081
No longer blocks: 861680
Attachment #8363045 - Flags: review?(tnikkel) → review+
Trees are closed :(
Keywords: checkin-needed
Attachment #8362635 - Attachment is obsolete: true
Attachment #8362637 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0fbc3cd1bf54
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
status-firefox29: affected → fixed
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.
Attachment #8363045 - Flags: approval-mozilla-aurora?

Updated

4 years ago
No longer blocks: 838081
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
Attachment #8363045 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Keywords: verifyme

Updated

4 years ago
Depends on: 966282
No longer depends on: 966282
Verified as fixed with Firefox 28 beta 7 on a Dell XPS 12 ultrabook with Windows 8 64-bit.
status-firefox28: fixed → verified

Updated

4 years ago
Blocks: 861680
Status: RESOLVED → VERIFIED

Updated

4 years ago
Whiteboard: [beta28] [metro] [gfx] → [beta28] [metro] [gfx] r=ff28
Verified as fixed on latest Aurora (build ID: 20140304004003) using a Microsoft Surface Pro 2 device running Windows 8.1 64bit.
status-firefox29: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.