Scrolling in vertical tabs is super slow and janky
Categories
(Firefox :: Tabbed Browser, defect, P3)
Tracking
()
People
(Reporter: gerard-majax, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: perf, reproducible, Whiteboard: [fidefe-sidebar])
Attachments
(2 files)
I have been hitting this since day one of enabling (in dublin), but always assumed it would get fixed soon. Here we are it is not, so I believe it is either not reported or I'm one of the few hitting this.
STR:
- have (not that much) tabs, maybe one hundreds?
- use mouse wheel to scroll up/down
Expected:
Nice gentle scrolling like everywhere
Actual:
Super slow / unreliable / unsuable scrolling: not scrolling, or just of one or two tabs when I let the wheel run a lot.
After capturing a profile https://share.firefox.dev/4dOlUPv florian suspects the cause is from https://searchfox.org/mozilla-central/rev/0cd8e314881fc6c216586d1adc13eb8a03f5d040/browser/components/tabbrowser/content/tabs.js#55-57
|
||
Comment 1•1 year ago
|
||
Blocking Vertical Tabs for now, though it may also affect horizontal tabs (TBV).
|
Reporter | |
Comment 2•1 year ago
|
||
(In reply to Marco Bonardo [:mak] from comment #1)
Blocking Vertical Tabs for now, though it may also affect horizontal tabs (TBV).
maybe should be duped to 1908905 ?
But my experience is much much much worse than what we see on those videos (I'm not sure how to capture the video and showing the mouse wheel at the same time, because in my case it's really not moving that much)
|
|
||
Comment 3•1 year ago
|
||
(In reply to :gerard-majax from comment #2)
But my experience is much much much worse than what we see on those videos
That's why I only added it as a See Also, there may be more.
|
||
Comment 4•1 year ago
•
|
||
(In reply to :gerard-majax from comment #0)
After capturing a profile https://share.firefox.dev/4dOlUPv florian suspects the cause is from https://searchfox.org/mozilla-central/rev/0cd8e314881fc6c216586d1adc13eb8a03f5d040/browser/components/tabbrowser/content/tabs.js#55-57
Why would this be so much worse for vertical tabs than for horizontal tabs? That code exists in both cases...
Also, is this still happening? I'm wondering if the change from bug 1918208 has perhaps improved something here.
Do we know if this is Linux-specific? Is smoothscroll turned on/off? Anything interesting about how the wheel is set up (line vs. pixel scrolling)? Looks like the way that the wheel event is processed differs a lot based on the deltaMode of the wheel events. At least on Windows, I can't reproduce but deltaMode is the LINE version in both horizontal and vertical tabs mode and would therefore always invoke the getter referenced here. I tested with 100 tabs. Would be useful to understand this a bit better (and how widespread it is).
EDit: context of the deltaMode reference: https://searchfox.org/mozilla-central/rev/ec82d34b786f309f9901efe989d59cb55bd61cf0/toolkit/content/widgets/arrowscrollbox.js#650,683,688,691 - the lineScrollAmount getter is what's invoking the offending getter.
|
||
Updated•1 year ago
|
|
|
||
Comment 5•1 year ago
|
||
Actually, looking at your profile again, I think your mouse / Linux events are weird.
I would expect to see 1 wheel event for every "tick" your mouse wheel scrolls. On my machine on Windows, over the space of 500ms, I see 5 wheel events while I'm scrolling (which seems reasonable).
In the profile from comment 0 there are 46 wheel events in fewer than 200ms. Sending 20 times as many events kind of makes sense of this causing jank.
We could try to fix the getter to do some caching (though the obvious question is when we'd invalidate) but also... why are we getting that many events? That seems wrong... I don't know who would know about wheel event dispatch and why it is doing that on your machine.
|
|
||
Comment 6•1 year ago
|
||
And, sorry - I cleared severity because if we've got a serious performance cliff on vertical tabs, given the target audience of vertical tabs being people who use tabs heavily, I think this probably warrants at least a little bit more digging to avoid people being burned immediately as soon as they try the feature (ie I think it might be higher severity than S4, assuming this happens on more machines than just :gerard-majax's :-) ).
|
|
Reporter | |
Comment 7•1 year ago
|
||
I'm not sure what you need from me, as I stated, scrolling content is super smooth, the same scrolling is super janky only on vertical tabs. Before vertical tabs landed i was using Tab Center Reborn and scrolling was super smooth as well. I never used scrolling on horizontal tabs, so maybe it was janky there as well :).
I have no idea why there would be so many events, I'm not doing anything special.
Prefs shows:
general.smoothScroll=falsegeneral.smoothScroll.mouseWheel.migrationPercent=0
But to be honest, i dont even remember when i would have changed those values. Likely a decade (litterally) ago?
|
||
Comment 8•1 year ago
|
||
I never used scrolling on horizontal tabs, so maybe it was janky there as well :)
Worth testing on horizontal tabs? With animations turned off? With a touchpad vs discrete scroll wheel vs continuous scroll wheel?
Because on Linux (with Wayland & libinput), it kind of reminds me of https://bugzilla.mozilla.org/show_bug.cgi?id=1683221#c6 and bug #1826839 in a way.
|
|
Reporter | |
Comment 9•1 year ago
|
||
(In reply to Jeff Fortin from comment #8)
I never used scrolling on horizontal tabs, so maybe it was janky there as well :)
Worth testing on horizontal tabs? With animations turned off? With a touchpad vs discrete scroll wheel vs continuous scroll wheel?
Because on Linux (with Wayland & libinput), it kind of reminds me of https://bugzilla.mozilla.org/show_bug.cgi?id=1683221#c6 and bug #1826839 in a way.
Thanks, only repros on my mouse, not with the touchpad of my laptop. Still, the very same mouse scrolls very well on content and everywhere else ...
|
|
Reporter | |
Comment 10•1 year ago
|
||
When you mention animations is it via pref sidebar.animation.enabled ?
|
||
Comment 11•1 year ago
|
||
(In reply to :Gijs (he/him) from comment #4)
(In reply to :gerard-majax from comment #0)
After capturing a profile https://share.firefox.dev/4dOlUPv florian suspects the cause is from https://searchfox.org/mozilla-central/rev/0cd8e314881fc6c216586d1adc13eb8a03f5d040/browser/components/tabbrowser/content/tabs.js#55-57
I'm not sure what information you need from me so I'll just assume you wanted more context on this sentence where I was mentioned. Here's the relevant part of the discussion of the profile in #profiler on Matrix:
gerard-majax: @florian: https://share.firefox.dev/3XUvomo profile of scrolling ; a first part of scrolling the list of channels in elements (very fast) and a second part of scrolling vertical tab (super bad)
julienw: I'd say the paint takes too much time, maybe because this is repainting too many things?
[…]
florian: The JS code is slow too: https://share.firefox.dev/4dOlUPv For every wheel event, it's iterating through all tabs: https://searchfox.org/mozilla-central/source/browser/components/tabbrowser/content/tabs.js#56
florian: for the Painting, I'm not sure. The profiler makes it artificially slow withmozilla::image::ImageResource::AutoProfilerImagePaintMarker::AutoProfilerImagePaintMarkerthat takes a lot of time, probably because there are some super long data: urls involved
15871 image paint markers with data urls: https://share.firefox.dev/3BDVaUG
that probably takes significant space in the JSON file too
might be a good idea to make the marker code detect data URL, and just store "data: url" instead of the actual url in that case
So, I said this JS code was slower than it should (I think it should use some caching instead of iterating through all tabs for every wheel event), but I didn't say this was the cause of the bug.
We filed bug 1924060 for the profiler overhead.
|
|
Reporter | |
Comment 12•1 year ago
|
||
i just verified, switching back to horizontal tabs scrolls very fast
|
|
Reporter | |
Comment 13•1 year ago
•
|
||
New profile: https://share.firefox.dev/3UoRPPI
- "Nightly" setup
- switched back to horizontal tabs
- started the profiler
- scrolled on horizontal tabs
- toggled the pref to switch to vertical tabs
- scrolled on vertical tabs
|
|
||
Comment 14•1 year ago
|
||
(In reply to :gerard-majax from comment #13)
New profile: https://share.firefox.dev/3UoRPPI
The second half of the profile is visibly jankier, but I can't really say what's taking longer. Markus, do you see anything interesting in this profile?
|
|
||
Comment 15•1 year ago
|
||
(In reply to :gerard-majax from comment #13)
New profile:
- "Nightly" setup
- switched back to horizontal tabs
- started the profiler
- scrolled on horizontal tabs
- toggled the pref to switch to vertical tabs
- scrolled on vertical tabs
Thanks. This definitely makes it clear that vertical tabs is worse. What is less clear to me is why.
I gathered some stats on the markers and time spent in on_wheel (the JS code). In both cases I started a selection at the start of a series of wheel markers, and ended after the last one. I tried to get roughly comparable time periods:
3.5s sample period
413 filtered wheel event markers
avg duration of marker: 0.5116 ms
call tree: 5% of running samples (176), with 186ms of traced running time
vertical
3.7s sample period
627 filtered wheel event markers
avg duration of marker: 0.4933
call tree: 7% of running samples (239) with 270ms of traced running time
The vertical tabs case happens to have more wheel events over a similar time period (50% more) and has 50% more running time in the JS... but if I'm looking at the jank markers, it seems like on_wheel "only" consumes e.g. here, 28 samples of the 161. Off-hand it looks like in layout land, the getFrameForPoint calls are more expensive (but perhaps I'm misreading that? I'm not that familiar with layout).
I will file a bug to make on_wheel cheaper because as Florian says, that would be a sensible thing to do. However, I suspect that won't fix this bug. Florian, could you take a look at this newer profile and offer some opinions on what would improve the performance situation overall?
Edit: mid-aired with Florian. Will not set needinfo and hopefully Markus can offer some advice.
(my gut sense is that this many wheel events is too many (cf comment 5) but I don't know if that is something we can adjust or what controls that... or even who to ask. Maybe Masayuki? And anyway maybe I'm just wrong and we should cater for that kind of situation either way...)
|
|
||
Comment 16•1 year ago
|
||
(In reply to :gerard-majax from comment #10)
When you mention animations is it via pref
sidebar.animation.enabled?
I meant with GNOME's global "Disable Animations" (a.k.a. "Reduce Animations") accessibility setting, it makes a night-and-day difference if global animations are turned off here.
(But then you wouldn't have animated scrolling at all in Firefox's tabs, which is very disorientating as I was pointing out in bug #1826839, because it jumps not by "one tab per scroll unit", but rather "a bunch of tabs per scroll unit" and your eyes cannot follow what goes where without a visual animation.)
|
||
Comment 17•1 year ago
|
||
(In reply to :Gijs (he/him) from comment #15)
Off-hand it looks like in layout land, the
getFrameForPointcalls are more expensive
I agree, for some reason the display list we build during hit testing seems to be a lot bigger with vertical tabs. I think it's caused by an ancestor transform item. When I remove this will-change: translate, hit testing becomes cheaper.
Timothy, can you check why the will-change: translate makes hit testing more expensive?
Another thing I noticed in the display list building part was that we spend some time in MarkAbsoluteFramesForDisplayList. I found two position:absolute elements per tab, .tab-loading-burst and its ::before pseudo-element. It looks like those elements are always visible even when they are not needed. Hiding them would reduce the display list times by 10% or so (in my profile I have 60 samples in MarkAbsoluteFramesForDisplayList compared to 550 in DisplayList building).
|
||
Comment 18•1 year ago
•
|
||
(In reply to Markus Stange [:mstange] from comment #17)
(In reply to :Gijs (he/him) from comment #15)
Off-hand it looks like in layout land, the
getFrameForPointcalls are more expensiveI agree, for some reason the display list we build during hit testing seems to be a lot bigger with vertical tabs. I think it's caused by an ancestor transform item. When I remove this
will-change: translate, hit testing becomes cheaper.Timothy, can you check why the
will-change: translatemakes hit testing more expensive?
I haven't reproduced, but based on the info here, I think it is because ShouldPrerenderTransformedContent expands the rect greatly. It should be entirely disabled for hittesting, I put up a patch in bug 1927315 for that.
My reasoning: will-change:transform will trigger ActiveLayerTracker::IsTransformMaybeAnimated to be true. In the profile I see non-zero time spent all the way to the end of ShouldPrerenderTransformedContent during hittesting, which is not common even during painting. The time spent in the function is relatively small, but it greatly expands the rect, which would greatly expand the time taken to build the display list.
|
|
||
Comment 19•1 year ago
|
||
(In reply to Markus Stange [:mstange] from comment #17)
Another thing I noticed in the display list building part was that we spend some time in
MarkAbsoluteFramesForDisplayList. I found twoposition:absoluteelements per tab,.tab-loading-burstand its::beforepseudo-element. It looks like those elements are always visible even when they are not needed. Hiding them would reduce the display list times by 10% or so (in my profile I have 60 samples inMarkAbsoluteFramesForDisplayListcompared to 550 inDisplayList building).
Most of the time under MarkAbsoluteFramesForDisplayList is spent in IsFixedPosFrameInDisplayPort. We call IsFixedPosFrameInDisplayPort(parent) once for every child of parent. This is a waste if there is more than one child. This problem has shown up in other profiles I've looked at. It's a little awkward, but I have some patches to fix this so we only call it once per parent that I need to clean up and submit that should hopefully help with this too.
|
|
||
Comment 20•1 year ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #19)
Most of the time under MarkAbsoluteFramesForDisplayList is spent in IsFixedPosFrameInDisplayPort. We call IsFixedPosFrameInDisplayPort(parent) once for every child of parent. This is a waste if there is more than one child. This problem has shown up in other profiles I've looked at. It's a little awkward, but I have some patches to fix this so we only call it once per parent that I need to clean up and submit that should hopefully help with this too.
Ah! It's actually much better than that, we never need to call IsFixedPosFrameInDisplayPort during hittesting. Filed bug 1927375 with a simple patch. (The other patch mentioned is still valid, it'll just apply to painting situations only since we completely removed the slowness here from hit testing.)
|
|
Reporter | |
Comment 21•1 year ago
|
||
Just updated to 20241029155057 where bug 1927375 is fixed and there's no visible improvement so far.
|
|
Reporter | |
Comment 22•1 year ago
|
||
(In reply to :gerard-majax from comment #21)
Just updated to 20241029155057 where bug 1927375 is fixed and there's no visible improvement so far.
New profile on https://share.firefox.dev/3A6Db9a, same steps as before
|
|
||
Comment 23•1 year ago
|
||
Thanks for re-profiling. That build doesn't have bug 1927315. A profile after that bug would be good.
But also, I'm not sure that making display list hit testing faster is going to be enough here. It might be that we want to do something to coalesce wheel events that happen in the same location in a short interval (we can't respond to input any faster then we tick the refresh driver, so that shouldn't affect responsiveness), I think we do something like that with mouse moves in the content process.
|
|
Reporter | |
Comment 24•1 year ago
|
||
On 20241030214633 so bug 1927375 should be there, and mostly no improvement visible.
|
|
Reporter | |
Comment 25•1 year ago
|
||
(In reply to :gerard-majax from comment #24)
On 20241030214633 so bug 1927375 should be there, and mostly no improvement visible.
|
|
||
Comment 26•1 year ago
|
||
(In reply to :gerard-majax from comment #25)
(In reply to :gerard-majax from comment #24)
On 20241030214633 so bug 1927375 should be there, and mostly no improvement visible.
Thanks. The profile shows that bug 1927315 achieved what it set out to achieve. The remaining problem it shows is the amount of time in _getScrollableElements, which bug 1927094 aims to improve. So once you have a build that includes bug 1927094, please get another profile.
|
||
Comment 27•1 year ago
|
||
The severity field is not set for this bug.
:dao, could you have a look please?
For more information, please visit BugBot documentation.
|
||
Updated•1 year ago
|
|
||
Comment 29•1 year ago
|
||
A peculiar thing I just noticed is that it doesn't happen on the pinned tabs, in the collapsed view, despite containing a similar amount of tabs.
|
|
||
Comment 30•1 year ago
|
||
Screencast of scrolling, in sidebar in collapsed view, on pinned and not pin tabs.
|
|
||
Comment 31•1 year ago
|
||
I just tried the latest nightly build which I believe to contain to fix for 1927094.
However this does not seem to fix the issue for me, It's still the same.
firefox-nightly-bin-134.0a1.20241114.211619-1-x86_64.pkg.tar.zst
|
|
||
Comment 32•1 year ago
|
||
:gerard-majax, can you get check if bug 1927094 improved the situation on your machine? And please share a new horizontal vs vertical profile.
|
|
Reporter | |
Comment 33•1 year ago
|
||
(In reply to Markus Stange [:mstange] from comment #32)
:gerard-majax, can you get check if bug 1927094 improved the situation on your machine? And please share a new horizontal vs vertical profile.
i was waiting for that to stick on central since the last time i got tricked and it was backed out :(
|
|
Reporter | |
Comment 34•1 year ago
|
||
ok so I'm on 20241114211619, which has the changes https://hg.mozilla.org/mozilla-central/rev/74b8af028eb7 and I still experience it. Profile will come ...
|
|
Reporter | |
Comment 35•1 year ago
|
||
|
|
||
Comment 36•1 year ago
|
||
(In reply to :gerard-majax from comment #35)
Hmm, there's one period of jank around 8s that corresponds to "theme changed", odd. Other than that it looks like painting is keeping up. Is the jank in the middle the problem you are seeing or its stuttering the whole time?
|
|
Reporter | |
Comment 37•1 year ago
|
||
it's the whole time
|
|
||
Comment 38•1 year ago
•
|
||
(copying in some folks from the see also bug for visibility as that bug hasn't moved much and has no performance profiles - would be interesting to see those from other folks if they are also experiencing this to see if the root cause is the same)
Some quick notes while I continue to be on sick leave (gotta amuse myself somehow):
- I think the only thing the scrollbox is trying to do is make sure that at the end of a wheel scroll the scrolled area aligns with the start/ends of tabs. ISTM that in 2024, that may be better accomplished by CSS scroll snapping? It's unclear to me off-hand how easy it is to replace all the logic in the scrollbox with that, but it seems worth investigating. I also don't know if it'd fix this issue entirely
- like comment 23 I wonder if we need to coalesce wheel events - ISTM there are, like, an absolute tonne coming from the scrollwheel in the profile (multiple events per few ms) and making that fast and smooth is always gonna be tricky. The budget for processing these events doesn't appear to be 16ms as you'd expect at 60fps but more like... less than 1ms.
- it's not clear to me from the profile off-hand if the scrolling happens in an expanded or collapsed vertical tabstrip. There's a separate bug on file (that I can't find real quick off-hand) that discusses that we
display: none/non-none the close button on hover for the vertical tabstrip as well, and that means more complex re-layout stuff happens. It may be that addressing that would further help with the style/layout/paint cycles here, though I still think the previous point may be more impactful.
|
|
Reporter | |
Comment 39•1 year ago
|
||
i experience the same whether sidebar is collapsed or expanded, the profiles i shared are all expanded
|
|
||
Comment 40•1 year ago
|
||
(In reply to :gerard-majax from comment #35)
I can't see any obvious signs of jank in that profile. Yes, hit testing and paints still take a bit longer, but from what I can tell, the time between the wheel event and the resulting composite seems to be reasonable.
(In reply to Timothy Nikkel (:tnikkel) from comment #36)
Hmm, there's one period of jank around 8s that corresponds to "theme changed", odd.
That's the part where :gerard-majax changed from horizontal tabs to vertical tabs. The profile contains both configurations.
|
|
Reporter | |
Comment 41•1 year ago
|
||
|
|
||
Comment 42•1 year ago
|
||
Okay so maybe this isn't about performance at all! Maybe the tab scrollbox just doesn't interpret the scroll delta the way you'd expect.
Actually, if your mouse is sending fractional "line" scroll deltas, then the scrollbox probably scrolls in fractional "tab size" increments. Tabs are wider than they are high. So maybe the vertical speed is just lower than the horizontal speed.
|
||
Comment 43•1 year ago
|
||
I don't think it's the speed itself being low, what I'm seeing is:
- With my mouse I scroll "one tick", the amount of scroll is reasonable and smooth (assuming your mouse wheel has ticks)
- But if I scroll multiple ticks at once, somehow the amount of scroll is just equal to the one tick or just slightly more than that.
|
|
||
Comment 44•1 year ago
|
||
I see. Then I think you need to add logging to arrowscrollbox.js and see what happens to this._destination. The code there tries to accumulate multiple ticks correctly.
|
|
||
Comment 45•1 year ago
|
||
I added a logging breakpoint here: https://searchfox.org/mozilla-central/rev/2d6125b505457248120cecdd0bcc8ecec4d05d13/toolkit/content/widgets/arrowscrollbox.js#716
In a single "multi-tick" wheel scroll, I get this:
onwheel: this._destination: 622
onwheel: this._destination: 406
onwheel: this._destination: 190
onwheel: this._destination: 608
onwheel: this._destination: 608
onwheel: this._destination: 608
onwheel: this._destination: 608
onwheel: this._destination: 608
onwheel: this._destination: 392
onwheel: this._destination: 392
onwheel: this._destination: 608
onwheel: this._destination: 608
onwheel: this._destination: 608
It's weird that it kinda goes back and forth?
|
|
||
Comment 46•1 year ago
|
||
It hits line 706 mulitple time: https://searchfox.org/mozilla-central/rev/2d6125b505457248120cecdd0bcc8ecec4d05d13/toolkit/content/widgets/arrowscrollbox.js#706
Per my reading that should only happen when I start scrolling in the reverse direction, but it happens multiple times during the single scroll.
|
|
||
Comment 47•1 year ago
|
||
Seems that's because _isScrolling becomes false during the scroll. And it gets multiple scrollend event during a single scroll.
|
|
||
Comment 48•1 year ago
|
||
Via customElement.js's handleEvent I get:
on_wheel
on_wheel
on_wheel
on_wheel
on_scrollend
on_scroll
on_wheel
on_wheel
on_wheel
on_scrollend
on_scroll
on_wheel
on_scrollend
on_wheel
on_scrollend
on_scroll
on_scroll
on_scroll
on_scroll
on_scroll
on_scroll
on_scroll
on_scroll
...
on_scroll
on_scrollend
There seems to be some pattern, perhaps scrollend implementation is faulty here?
|
|
||
Comment 49•1 year ago
|
||
Hello Dan, I see you have worked on scroll events in bug 1785102, do you have some idea about what could be going wrong here?
|
||
Comment 50•1 year ago
|
||
Ah, this is interesting and could be related to several issues (e.g. bug 1919566 and bug 1875211))
|
|
Reporter | |
Comment 51•1 year ago
|
||
I'd like to emphasize that now that the sidebar can be resized (this is welcome), this issue is becoming increasingly painful: grabbing the scrollbar is very difficult because of the sidebar resizeability. We're reaching the point where I can state that the feature is becoming unusable just because of this. Basically, the reasonning justifiying the backout in bug 1927094 is already applicable to my case, but it's even worse now.
|
||
Comment 52•1 year ago
|
||
(In reply to Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11) from comment #43)
I don't think it's the speed itself being low, what I'm seeing is:
- With my mouse I scroll "one tick", the amount of scroll is reasonable and smooth (assuming your mouse wheel has ticks)
- But if I scroll multiple ticks at once, somehow the amount of scroll is just equal to the one tick or just slightly more than that.
I would like to second this. I am testing on wayland with stable firefox, however I will try to give some details to all my observation so that hopefully someone can piece together why this is happening:
- One scroll wheel click seems to work fine, however if I give a second scroll while the animation of the previous scroll event is still happening it seems to slow down scrolling instead of speeding up. More scroll wheel clicks (events) the slower the scrolling gets. It doesn't seem to completely cancel the previous animation though, so I suspect it just does some wrong math somewhere.
- Following the above, in my case touchpad scrolling is completely broken. It will do a small janky jump at first and then no scrolling.
libinput debug-eventsconfirms that the touchpad is indeed giving a continuous scroll events. - The amount of a single scroll of the vertical tabs seems to be multiple times longer than on a webpage.
- Horizontal tabs scrolling and content scrolling has always been fine for me.
- Smooth scrolling setting doesn't have any effect for me.
(In reply to Jeff Fortin from comment #8)
I never used scrolling on horizontal tabs, so maybe it was janky there as well :)
Worth testing on horizontal tabs? With animations turned off? With a touchpad vs discrete scroll wheel vs continuous scroll wheel?
Because on Linux (with Wayland & libinput), it kind of reminds me of https://bugzilla.mozilla.org/show_bug.cgi?id=1683221#c6 and bug #1826839 in a way.
Turning off animations in my system settings (which firefox follows) fixes vertical tab scrolling for me.
Thank you for working on this feature. Hopefully someone can piece together what kind of code is at fault from the above.
|
|
Reporter | |
Comment 53•1 year ago
|
||
(In reply to Jeff Fortin from comment #8)
I never used scrolling on horizontal tabs, so maybe it was janky there as well :)
Worth testing on horizontal tabs? With animations turned off? With a touchpad vs discrete scroll wheel vs continuous scroll wheel?
Because on Linux (with Wayland & libinput), it kind of reminds me of https://bugzilla.mozilla.org/show_bug.cgi?id=1683221#c6 and bug #1826839 in a way.
Somehow, I gave a new try to turning off animations system-wide, and now it helps a lot on that specific matter, but the user experience of the desktop is severely degraded IMHO. Can we try to move forward on that issue ?
|
||
Comment 54•1 year ago
|
||
I can reproduce this. Quite counterintuitively, the faster I move the mouse wheel (time delta between ticks), the slower the sidebar contents scroll.
I think a good next step here is to debug the arrowscrollbox.js code in more detail to understand how this artifact arises. If there is something about the event deltas or scrollend events coming from the platform that the arrowscrollbox.js code is not expecting, we should clarify what the code's expectations are, so we can make sure the platform is conforming to it (or adjust the code's expectations, as appropriate).
|
||
Comment 55•1 year ago
|
||
The Performance Impact Calculator has determined this bug's performance impact to be low. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.
Impact on browser: Causes noticeable jank
Configuration: Specific but common
[x] Able to reproduce locally
|
|
||
Comment 56•1 year ago
•
|
||
(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #47)
Seems that's because
_isScrollingbecomes false during the scroll. And it gets multiple scrollend event during a single scroll.
I think this answers:
(In reply to Botond Ballo [:botond] [away until Jan 6] from comment #54)
I think a good next step here is to debug the arrowscrollbox.js code in more detail to understand how this artifact arises. If there is something about the event deltas or scrollend events coming from the platform that the arrowscrollbox.js code is not expecting, we should clarify what the code's expectations are, so we can make sure the platform is conforming to it (or adjust the code's expectations, as appropriate).
The arrowscrollbox code tracks when scrolling starts and ends using scroll and scrollend events, respectively. This causes it to update its internal _isScrolling bool: https://searchfox.org/mozilla-central/rev/b6718bf263ae780289da471f0ea35797a09e2f05/toolkit/content/widgets/arrowscrollbox.js#751-752,757-758 .
When processing wheel events, this bool is consulted and determines whether we add the new scroll amount (determined by the information in the wheel event + the size of the items in the scroll box) to an existing tracker of how much we want to scroll, or whether we reset to the current position + whatever the scroll amount is. https://searchfox.org/mozilla-central/rev/b6718bf263ae780289da471f0ea35797a09e2f05/toolkit/content/widgets/arrowscrollbox.js#705-711 . Then we call scrollBy with that amount. I think in the cases reported with the performance profiles here, instant is never set (so we use auto as the behaviour value).
From what I can tell, if the user were to make one continuous motion with a scroll wheel, this code would expect exactly 1 scrollend event, when the scrolling is done. Instead, we seem to be getting several. I'm not an APZ/scrolling expert so I would not want to comment on whether the expectations or the behaviour is at fault here! :-)
Botond, does that help?
Edit: and it would seem that Hiro filed bug 1932985 which more or less covers a potential explanation of why there are so many scrollend events and how they're bogus...
|
|
||
Comment 57•1 year ago
|
||
Thanks, Gijs, this is helpful.
(In reply to :Gijs (he/him) from comment #56)
From what I can tell, if the user were to make one continuous motion with a scroll wheel, this code would expect exactly 1
scrollendevent, when the scrolling is done. Instead, we seem to be getting several.
I agree, this expectation is reasonable. To make it a bit more precise: if wheel events are triggering smooth scrolling, and a subsequent wheel tick happens while the scrolling from previous ticks is not yet done, the new wheel tick should extend the existing scroll animation, and we shouldn't get a scrollend until the entire animation is done.
I was initially confused about bug 1932985 because the test case there was exhibiting a different behaviour (instant scrolling), but I posted a revised test case there which exercises the scenario described above, and confirmed that there's a platform bug affecting this scenario where we incorrectly get a scrollend event for each tick. I'll mark this as depending on bug 1932985 and we'll pursue a platform fix there.
|
||
Comment 58•1 year ago
|
||
I can confirm this with a mouse wheel, and with a touchpad and a touch screen. Scrolling vertical tabs is slow, scrolling the pinned tabs is normal.
With a touchpad and a touch screen on the vertical tabs the scrolling stops as soon as the fingers are lifted, making it harder to scroll to the top or bottom.
With a touchpad and a touch screen on the pinned tabs and the main window, scrolling continues for a bit after the fingers are lifted.
Linux, Wayland.
|
|
||
Comment 59•1 year ago
|
||
With a touchpad and a touch screen on the vertical tabs the scrolling stops as soon as the fingers are lifted, making it harder to scroll to the top or bottom.
With a touchpad and a touch screen on the pinned tabs and the main window, scrolling continues for a bit after the fingers are lifted.
To elaborate, the vertical tabs behave exactly as if a window scroll is started with the gesture, and then the screen is immediately tapped, stopping the scroll.
In the main window, the scroll gesture triggers a fast scroll, and the tap stops the scrolling immediately.
|
|
Reporter | |
Comment 60•1 year ago
|
||
With Bug 1932985 should we expecting improvements ?
|
|
||
Comment 61•1 year ago
|
||
(In reply to :gerard-majax from comment #60)
With Bug 1932985 should we expecting improvements ?
Yes -- let me know please if the situation is improved in the latest nightly.
|
|
Reporter | |
Comment 62•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #61)
(In reply to :gerard-majax from comment #60)
With Bug 1932985 should we expecting improvements ?
Yes -- let me know please if the situation is improved in the latest nightly.
Youpi banane! Switching back animations enabled in accessibility settings of GNOME I now I have a properly decent and fast scrolling on the vertical tab.
|
|
Reporter | |
Comment 63•1 year ago
|
||
Profile where things are fast, just in case https://share.firefox.dev/4jCglI2
|
|
Reporter | |
Updated•1 year ago
|
|
|
Reporter | |
Comment 64•1 year ago
|
||
Assuming it's fixed now.
|
||
Updated•1 year ago
|
|
||
Updated•1 year ago
|
|
||
Comment 65•1 year ago
|
||
I can't seem to be able to reproduce on my Ubuntu 22 system (having AMD® Fx(tm)-8320 eight-core processor × 8, NVIDIA NV106) with extensive testing. This being considered we won't be able to verify this fix.
|
|
||
Updated•1 year ago
|
|
||
Comment 66•1 year ago
|
||
Despite this issue's metadata saying it was fixed in 136, it's still an issue in the 136 release.
I'm marking this as unfixed in 136, and fixed in 137, based on https://bugzilla.mozilla.org/show_bug.cgi?id=1947114 .
|
|
||
Comment 67•1 year ago
•
|
||
(In reply to Josh Triplett from comment #66)
Despite this issue's metadata saying it was fixed in 136, it's still an issue in the 136 release.
I'm marking this as unfixed in 136, and fixed in 137, based on https://bugzilla.mozilla.org/show_bug.cgi?id=1947114 .
Note that there were two issues here:
- an issue affecting mousewheel input, which was fixed in Firefox 136 (bug 1932985)
- an issue affecting touchpad input, which was fixed in Firefox 137 (bug 1947114)
I suspect you were likely using touchpad input when you tested Firefox 136.
|
|
||
Comment 68•1 year ago
•
|
||
(In reply to Botond Ballo [:botond] from comment #67)
(In reply to Josh Triplett from comment #66)
Despite this issue's metadata saying it was fixed in 136, it's still an issue in the 136 release.
I'm marking this as unfixed in 136, and fixed in 137, based on https://bugzilla.mozilla.org/show_bug.cgi?id=1947114 .
Note that there were two issues here:
- an issue affecting mousewheel input, which was fixed in Firefox 136 (bug 1932985)
- an issue affecting touchpad input, which was fixed in Firefox 137 (bug 1947114)
I suspect you were likely using touchpad input when you tested Firefox 136.
Ah, I didn't realize the distinction. Yes, I was using a touchpad to scroll.
I think since this issue appears to be a parent issue to both of those, it makes sense for it to be marked as fixed for the release that has both of those issues fixed, so I think it still makes sense for this to be marked as unfixed in 136 and fixed in 137.
|
||
Updated•1 year ago
|
Description
•