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

VERIFIED FIXED in Firefox 28

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: rsilveira, Assigned: kats)

Tracking

({regression})

29 Branch
mozilla29
x86_64
Windows 8.1
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.
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.
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
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. :(
Posted patch Patch (obsolete) — Splinter Review
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).
Attachment #8362084 - Attachment is obsolete: true
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.
(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.
(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?
(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.
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.
Seems to work ok for me. What do you guys think?
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.
Assignee: nobody → bugmail.mozilla
Whiteboard: [metro] [beta28] [defect] p=0 → [metro] [beta28]
Whiteboard: [metro] [beta28] → [metro] [beta28] [gfx]
Duplicate of this bug: 960992
Whiteboard: [metro] [beta28] [gfx] → [metro] [gfx]
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.
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]
Blocks: metrov1backlog
No longer blocks: metrobacklog
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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?
No longer blocks: metrov1backlog
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+
Keywords: verifyme
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.
Blocks: metrobacklog
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.