User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170824053622 Steps to reproduce: nightly 58.0a1 I've used mouse wheel to scroll through tabs (mouse over the tabs and scroll mouse wheel) In tabs scrolling with mouse wheel is about 5-7 times slower then in 55.0.3. Actual results: Scrolling is very, very slow. Expected results: Tabs scrolling should be as fast as in 55.0.3

Regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=db88d543de14b78333b095485de56cb8e9832af9&tochange=e1c23455946bd42445a0a12d223c7548c39c9785 bug 1356705.

Given that this was triaged as a P3, I doubt this will make it into 57. Untracked.

[Tracking Requested - why for this release]: Renominating, since I think this is more important than a P3 and I would prefer to see it triaged into a P1 if it's a widespread issue. At least one other bug was duped to this one, so it's not uncommon at least. And given that it's a major UX interaction it seems kinda bad to ship this regression in Firefox Quantum.

Marco, can you also clarify the rationale for marking this as a P3?

We triaged this this morning, and the decision was to put it into the Photon Performance backlog until I could investigate it further, which I'm in the process of doing. Putting it into the backlog made it a P3 under mmucci's system.

I've attached a screen recording of me scrolling tabs with my mouse. I'm doing 4 single clicks with my scroll wheel, and then a long spin. The top browser is Firefox Nightly (58.0a1) and the bottom browser is 56. The 58 scrolling actually seems like an improvement to me over the 56 scrolling. Am I not reproducing the bug using the right steps?

Ah, okay, I think I see what's going on. We're not taking high-velocity scrolls into account. Yeah, we should fix this before 57 goes out.

Tracked for 57. Hi Marco, should this still be a P3?

Hey Mike, do you have the cycles for this for 57?

Yes, I'm actively working on this.

Comment on attachment 8917100 [details] Bug 1403563 - Add a chrome-only scrollend event. I fiddled with this a bit, and this feels roughly the same as with the old rAF driven scrolling - though I had to (by feel) generate a few factors, which is a bit annoying. I had to generate them because (unless I missed something) the expected duration of the smooth scroll isn't something that's exposed to us. If this doesn't look like the right approach, we might also want to consider reintroducing the scrollAnim sampler thing, but have it use scrollTo in instant mode. What do you think, dao?

We're past the point where I think we're likely to take a patch for this on 57, so I'm going to wontfix it for there. canuckistani, just double-checking that you don't think this is a release blocker? I think it's pretty annoying for users that have hundreds of tabs, but I suspect we can get a fix for them in 58.

Comment on attachment 8917100 [details] Bug 1403563 - Add a chrome-only scrollend event. https://reviewboard.mozilla.org/r/188106/#review195484 ::: toolkit/content/widgets/scrollbox.xml:523 (Diff revision 1) > + const repeatFactor = 4; > + > + let sameDirection = (oldDestination > 0) == (scrollAmount > 0); > + let throttled = Math.abs(oldDestination) > Math.abs(throttleFactor * scrollAmount); > + if (sameDirection && !throttled) { > + scrollAmount = oldDestination + (scrollAmount / repeatFactor); Not really sure what throttleFactor, repeatFactor and throttled mean. This could use some documentation. This appears to be the heart of the patch, so I'm afraid I don't have much useful feedback at the moment. ::: toolkit/content/widgets/scrollbox.xml:616 (Diff revision 1) > <handler event="scroll"><![CDATA[ > + if (this.smoothScroll) { > + // Smooth scrolling is driven by the refresh driver, which means that > + // we should get scroll events for each tick. If we don't get a > + // scroll event on the tick after this one, then scrolling must have > + // finished. If I remember correctly, this is false. When scrolling slows down at the end of a scroll animation, I think you can get frames that don't move the needle before another frame scrolls by another pixel.

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #14) > We're past the point where I think we're likely to take a patch for this on > 57, so I'm going to wontfix it for there. > > canuckistani, just double-checking that you don't think this is a release > blocker? I think it's pretty annoying for users that have hundreds of tabs, > but I suspect we can get a fix for them in 58. Is the crossover hundreds of tabs, dozens of tabs? Most users do not have hundreds of tabs, so that makes this patch a lot less important. If users were affected with a dozen tabs, I would on the other hand suggest this is a blocker.

it's iritating even if you have like 70 tabs which is not something unusual.

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #16) > Is the crossover hundreds of tabs, dozens of tabs? Most users do not have > hundreds of tabs, so that makes this patch a lot less important. If users > were affected with a dozen tabs, I would on the other hand suggest this is a > blocker. Yeah, I think we're talking on the order of dozens (plural), and not a dozen tabs.

Comment on attachment 8917100 [details] Bug 1403563 - Add a chrome-only scrollend event. https://reviewboard.mozilla.org/r/188106/#review203816 I'm ok with this, although I'd prefer you moved this patch to bug 1172171 and made it a dependency of this bug. You might run into the same problem that Etienne reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1172171#c5 so watch out for that. ::: layout/generic/nsGfxScrollFrame.cpp:4420 (Diff revision 2) > +{ > + MOZ_ASSERT(mOuter->GetContent()); > + RefPtr<Event> event = NS_NewDOMEvent(mOuter->GetContent(), nullptr, nullptr); > + event->InitEvent(NS_LITERAL_STRING("scrollend"), true, false); > + event->SetTrusted(true); > + event->WidgetEventPtr()->mFlags.mOnlyChromeDispatch = true; Markus' patch in bug 1172171 does this using nsLayoutUtils::DispatchTrustedEvent which seems more convenient that writing out the code in full here. I'd suggest using that as well :)

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review204486 ::: toolkit/content/widgets/scrollbox.xml:138 (Diff revision 1) > > <property name="scrollSize" readonly="true"> > <getter><![CDATA[ > return this.orient == "vertical" ? > - this._scrollbox.scrollHeight : > - this._scrollbox.scrollWidth; > + this.scrollBoxObject.scrolledHeight : > + this.scrollBoxObject.scrolledWidth; Why this change? I'd rather move away from nsIScrollBoxObject to standard DOM APIs as much as possible.

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review204486 > Why this change? I'd rather move away from nsIScrollBoxObject to standard DOM APIs as much as possible. Accessing scrolledWith using nsIScrollBoxObject allows us to avoid layout flushes. Accessing via nsIScrollBoxObject gives me the data I need while only flushing frames. https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/layout/xul/ScrollBoxObject.cpp#82 ^-- the `false` we're passing to `GetFrame` causes us to enter this branch: https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/layout/xul/BoxObject.cpp#114-120

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #24) > Comment on attachment 8927407 [details] > Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. > > https://reviewboard.mozilla.org/r/198710/#review204486 > > > Why this change? I'd rather move away from nsIScrollBoxObject to standard DOM APIs as much as possible. > > Accessing scrolledWith using nsIScrollBoxObject allows us to avoid layout > flushes. Accessing via nsIScrollBoxObject gives me the data I need while > only flushing frames. Maybe so but nsIScrollBoxObject is legacy tech that we need to move away from. I'm also skeptical that we don't need to flush layout here to get accurate numbers.

(In reply to Dão Gottwald [::dao] from comment #25) > Maybe so but nsIScrollBoxObject is legacy tech that we need to move away > from. I'm also skeptical that we don't need to flush layout here to get > accurate numbers. How did you want to proceed then? A solution that causes us to flush layout synchronously is kind of a non-starter, so did you have something else in mind?

You don't seem to be using scrollSize, so I'm not sure why you're touching this in the first place. As for scrollBoxObject in the wheel event handler, doesn't scrollByPixels need to flush layout anyway? So what's the win from not flushing layout already before that call?

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review204992 ::: toolkit/content/widgets/scrollbox.xml:134 (Diff revision 2) > <property name="scrollSize" readonly="true"> > <getter><![CDATA[ > return this.orient == "vertical" ? > - this._scrollbox.scrollHeight : > - this._scrollbox.scrollWidth; > + this.scrollBoxObject.scrolledHeight : > + this.scrollBoxObject.scrolledWidth; > ]]></getter> > </property> Dao's right - I need to revert this change, since it's leftover code from development that I stopped using.

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review205368 ::: toolkit/content/widgets/scrollbox.xml:157 (Diff revision 3) > > + <property name="scrollPosition" readonly="true"> > + <getter><![CDATA[ > + return this.orient == "vertical" ? > + this._scrollbox.scrollTop : > + this._scrollbox.scrollLeft; Is this correct for RTL?

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review205368 > Is this correct for RTL? Should be - this is pretty much a verbatim resurrection of the `scrollPosition` that was used by the old requestAnimationFrame scroll code: https://hg.mozilla.org/mozilla-central/file/3845af01ea9f/toolkit/content/widgets/scrollbox.xml#l180 I'll test RTL to be sure and report back.

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review205368 > Should be - this is pretty much a verbatim resurrection of the `scrollPosition` that was used by the old requestAnimationFrame scroll code: https://hg.mozilla.org/mozilla-central/file/3845af01ea9f/toolkit/content/widgets/scrollbox.xml#l180 > > I'll test RTL to be sure and report back. Seems okay for RTL.

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review205460 Thanks!

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. https://reviewboard.mozilla.org/r/198710/#review205460 Thank _you_ for the review!

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d040a96e2213 Allow multiple wheel events to accelerate scrollbox scrolling. r=dao

Hi Adrian, Can you help check if this issue was fixed in the latest nightly?

(In reply to Gerry Chang [:gchang] from comment #37) > Hi Adrian, > Can you help check if this issue was fixed in the latest nightly? I can confirm that scrolling is back to normal in the latest nightly. But so you guys really mean scrolling won't properly work in the release version until March 2018? :O I guess I'll have to use the nightly until then.. (or roll back to 56 once again.)

(In reply to kdano from comment #38) > But so you guys really mean scrolling won't properly work in the release > version until March 2018? :O > I guess I'll have to use the nightly until then.. (or roll back to 56 once > again.) Presumably this fix will get uplifted to Beta for Fx58 once the fix is verified on Nightly.

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. Approval Request Comment [Feature/Bug causing the regression]: bug 1356705 [User impact if declined]: see comment 0 [Is this code covered by automated tests?]: XUL scrollbox is covered to some extent [Has the fix been verified in Nightly?]: not set [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: / [Is the change risky?]: no [Why is the change risky/not risky?]: pretty straightforward fix, baked in Nightly for more than a week [String changes made/needed]: /

Comment on attachment 8927407 [details] Bug 1403563 - Allow multiple wheel events to accelerate scrollbox scrolling. Fix a tabs scrolling issue with mouse wheel. Beta58+.

I have repro this bug using the STR from comment 0, on an old Nightly build 58.0a1 (2017-09-27). This is verified fixed on latest Nightly 59.0a1 (2017-12-06) and Beta 58.0b9 (20171204150510) on the following OSes: Win 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.