Closed Bug 1199468 Opened 8 years ago Closed 5 years ago

Firefox mouse-wheel scrolling on Windows looks worse than IE/Edge

Categories

(Core :: Graphics, defect, P3)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox43 --- affected

People

(Reporter: vladan, Assigned: mstange)

References

Details

(Whiteboard: gfx-noted)

Attachments

(4 files)

Fiddle with Async scrolling parameters
2.00 KB, patch
mstange
: feedback+
phlsa
: feedback+
Details | Diff | Splinter Review
Visual Studio 2013 Concurrency Analyzer Trace profiling IE
77 bytes, text/plain
Details
MozReview Request: Bug 1199468 - Create prefs for the smooth scroll timing function shape. r?kats
58 bytes, text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1199468 - Tighten the smooth scroll animation prefs. r?avih
58 bytes, text/x-review-board-request
Details 
So I did some investigation in this, there's basically two distinct things going on that I feel the people here that looked at it generally think makes edge look better:

1. Edge doesn't snap its transforms during async scrolling, they just let things be blurry during the scroll animation. This means that particularly for slow moving animation there's less 'sampling artifacts' because of discretization.
2. Their animation happens fast and in a slightly different manner, I've fiddled with our parameters a little and people seem to generally agree this looks better, patch attached.
Attachment #8653794 - Flags: feedback?(botond)
Whiteboard: gfx-noted
Comment on attachment 8653794 [details] [diff] [review]
Fiddle with Async scrolling parameters

Review of attachment 8653794 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to defer to Markus on this, as he has tweaked this animation code before.
Attachment #8653794 - Flags: feedback?(botond) → feedback?(mstange)
(In reply to Bas Schouten (:bas.schouten) from comment #1)
> 1. Edge doesn't snap its transforms during async scrolling, they just let
> things be blurry during the scroll animation. This means that particularly
> for slow moving animation there's less 'sampling artifacts' because of
> discretization.

What do we snap our transforms to?
Layer pixels. Or in other words, when a layer's transform is just a translation, we make sure to draw the layer device-pixel aligned so that no resampling occurs. Edge apparently doesn't do that.
Comment on attachment 8653794 [details] [diff] [review]
Fiddle with Async scrolling parameters

Philipp, can you comment on whether you agree that the new animation is better? You can test with http://www.cbc.ca/news/business/uber-airbnb-here-to-stay-so-get-rules-in-place-ontario-chamber-of-commerce-says-1.3201984 for example 

Bas, can you link to a build?
Attachment #8653794 - Flags: feedback?(philipp)
I still can't get a build environment running on my Windows machine, so a build would be very helpful :)
ni on behalf of phlsa :)
Flags: needinfo?(bas)
On a side note, while looking into Intel flush() synchronization issues, we found out that IE does WaitForVBlank() during some scrolls. See the long sync attached in this profile at timestamp 17262. The call stack looks like this:

ntdll.dll!LdrInitializeThunk+0xe
gdi32.dll!_NtGdiDdDDIWaitForVerticalBlankEvent@4+0x15
dxgi.dll!CDXGIOutput::WaitForVBlank+0x57
mshtml.dll!CDXResourceDomain::WaitForVBlank+0x57
mshtml.dll!CDXSystem::WaitForVBlank+0x1a
mshtml.dll!CSmoothScroller::WaitForNextFrame+0x52
mshtml.dll!Layout::ContainerBox::SmoothScroll+0x61
mshtml.dll!Layout::ContainerBox::ScrollTo+0xa7
mshtml.dll!Layout::ContainerBox::ScrollByPercent+0xfaa9
mshtml.dll!Layout::SContainerBoxMouseWheelClient::ScrollByPercent+0x5b
mshtml.dll!HandleMouseWheel+0x22e
mshtml.dll!`CBackgroundInfo::Property<CBackgroundImage>'::`7'::`dynamic atexit destructor for 'fieldDefaultValue''+0xd2ae
mshtml.dll!Layout::ContainerBox::HandleMessage+0xd7ca
mshtml.dll!Tree::CIE9DocumentLayout::HandleMessage+0xc6
mshtml.dll!CElement::HandleMessage+0xa9c3
mshtml.dll!CElement::HandleWindowMessage+0x5bf3
mshtml.dll!CDoc::PumpMessage+0x510
mshtml.dll!CDoc::OnMouseMessage+0x2e1
mshtml.dll!`CBackgroundInfo::Property<CBackgroundImage>'::`7'::`dynamic atexit destructor for 'fieldDefaultValue''+0xa423
mshtml.dll!CServer::WndProc+0x60
user32.dll!_InternalCallWinProc@20+0x23
user32.dll!_UserCallWinProcCheckWow@32+0xb7
user32.dll!_DispatchMessageWorker@8+0xed
user32.dll!_DispatchMessageW@4+0xf
ieframe.dll!CTabWindow::_TabWindowThreadProc+0x59e
ieframe.dll!LCIETab_ThreadProc+0x388
iertutil.dll!_IsoThreadProc_WrapperToReleaseScope+0x1c
ieshims.dll!NS_CreateThread::DesktopIE_ThreadProc+0x91

We should probably investigate touch resampling aligned with vsync on Windows like we do with b2g. It provided a very impressive smoothness improvement on b2g.
Nevermind, I pushed to try myself:

Build for the before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14a89cd75cc3
Build with Bas's patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e3967013e09

Builds should be ready in a few hours
Flags: needinfo?(bas)
Comment on attachment 8653794 [details] [diff] [review]
Fiddle with Async scrolling parameters

OMG, YES!
So much better!
Flags: needinfo?(philipp)
Attachment #8653794 - Flags: feedback?(philipp) → feedback+
Scroll behavior is a highly subjective thing.

The patch does two things:

1. remove pixel snapping:

This change could have both performance related effects and subjective quality difference effect. (subjective, because rendering at non-integer pixel offset, especially for text, is not necessarily objectively better).

I didn't experiment with it enough, but my initial observation is that I don't notice the disabled pixel snapping, possibly because this patch also makes it scroll much faster. I probably would if I look longer, and I didn't try to evaluate the performance impact of it.


2. Change the scroll "profile" to be more similar to IE (mainly shorter duration, but also more abrupt deceleration).

I did notice this change clearly, and I agree that the patch achieves its goal of making Firefox scroll more similar to IE, however, subjectively I never liked the fast and abrupt "smooth" scroll in IE, and therefore subjectively I don't think it's an improvement.


In general, the shorter the scroll duration is, the less meaningful IMO whether it pixel-snaps or not.

I suggest to test these two aspects completely independently because.. well.. they're completely independent, and with different effects categories.

- Current nightly vs:
1. Same scroll profile as now but with disabled pixel snapping.
2. the faster profile with same pixel snapping as now.
(In reply to Avi Halachmi (:avih) from comment #12)
> The patch does two things:
> 
> 1. remove pixel snapping:

The patch doesn't do this. We'll deal with that in a separate bug.

Philipp's UI review was purely about the more aggressive timing function.
Comment on attachment 8653794 [details] [diff] [review]
Fiddle with Async scrolling parameters

f+ but r-. The " / 3" part needs to be replaced by a change to the duration prefs.
Attachment #8653794 - Flags: feedback?(mstange) → feedback+
(In reply to Avi Halachmi (:avih) from comment #12)
> Scroll behavior is a highly subjective thing.
> 
> The patch does two things:
> 
> 1. remove pixel snapping:

No it doesn't.
So, this has been around for a while. Can we actually land it? And ideally uplift it to 43 and/or 42?
Flags: needinfo?(bas)
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #16)
> So, this has been around for a while. Can we actually land it? And ideally
> uplift it to 43 and/or 42?

The main issue description at the original post is IMO:

> The single-click and three-click mouse-wheel scrolls consistently looked
> smoother in IE and Edge.

"Looks smoother" is a bit vague, but at the lack of more explicit description, I'd interpret it as "has less missed frames" - which I agree with. IE just never misses a frame during scroll, while Firefox may miss frames sometimes.

However, the patch addresses a completely different issue, the issue of "IE scroll duration is shorter than in Firefox and I like it better", which is both highly subjective and unrelated to the main issue of this bug as far as I can tell. The mere fact that Firefox scroll behavior is different than IE's (and it is different, and IMO better) should not be considered a bug.

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #11)
> OMG, YES!
> So much better!

Could you please describe in what way it's better? Do you just like the shorter duration? or do feel that it actually misses less frames?
I found and filed two real issues when scrolling with the mouse wheel, which could be related to this bug:

1. Bug 1207412 - the scroll "jumps forward" 4 times/sec when scrolling with the mouse wheel since 2015-08-24.

2. Bug 1207656 - the animation duration is fixed to 400ms instead of varying between 200-400ms when scrolling with the mouse wheel. This makes the scroll feel "slow and laggy" when trying to scroll quickly (at which case the animation should have been 200ms, but ends up as 400). Probably since mouse wheel scroll moved to APZ by default (bug 1139220 ?).
Depends on: 1207412, 1207656
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #16)
> So, this has been around for a while. Can we actually land it? And ideally
> uplift it to 43 and/or 42?

This isn't really a decision I should probably make, if we're certain about this I can prepare the patch that Markus suggested although someone should probably look into Avi's comments as well.
Flags: needinfo?(philipp)
Flags: needinfo?(bas)
Flags: needinfo?(avihpit)
(In reply to Bas Schouten (:bas.schouten) from comment #19)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #16)
> > So, this has been around for a while. Can we actually land it? And ideally
> > uplift it to 43 and/or 42?
> 
> This isn't really a decision I should probably make, if we're certain about
> this I can prepare the patch that Markus suggested although someone should
> probably look into Avi's comments as well.

There's certainly some risk associated with changing basic behavior like scrolling, but it did look really good in my tests. We probably can't say much more for sure until we get it in front of some people which is why I'd propose to land it in Nightly and watch the feedback channels (perhaps even run a heartbeat-study about scroll performance).

I can't, of course, comment on the technical risks associated with this change.
Flags: needinfo?(philipp)
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #20)
> There's certainly some risk associated with changing basic behavior like
> scrolling, but it did look really good in my tests.

There is bug 1207656 (landed today) which fixes an actual bug of "duration is too long when scrolling quickly", which was introduced when mouse scrolling started using APZ.

I'd appreciate if you could test with the fix (tomorrow's nightly or already today after setting  layers.async-pan-zoom.enabled = false since the issue is only with APZ).

This will make it scroll intentionally "leisurely" when scrolling slowly, but more snappy when scrolling quickly.

You can control the snappy/leisure extremes using the prefs:
- "leisure": general.smoothScroll.mouseWheel.durationMaxMS (400 ms)
- "snappy" : general.smoothScroll.mouseWheel.durationMinMS (200 ms)

And this pref controls how we move between these extremes - the higher this value, the quicker the user needs to scroll in order to make the response more snappy (so lower value = more snappy even for slower scroll):
- general.smoothScroll.durationToIntervalRatio (200).

> We probably can't say
> much more for sure until we get it in front of some people which is why I'd
> propose to land it in Nightly and watch the feedback channels (perhaps even
> run a heartbeat-study about scroll performance).

A study seems fine to me. But landing it and expecting people to shout wouldn't be representative IMO.

But we should only conduct such study IMO after bug 1207656 and bug 1207412 arrive to nightly, since they're meaningfully affecting the results.
Flags: needinfo?(avihpit)
Philipp, I'd appreciate a respond on comment 21. Thanks.
Flags: needinfo?(philipp)
Blocks: 1213469
An important consideration here is that Microsoft Edge/IE uses Microsoft Direct Manipulation library for it's scrolling behaviour.  This library is also used for all windows 8.x Metro/Modern apps as well as for all Windows 10 Universal/XAML apps.

In Windows 10 MS is replacing all win32 apps and UI components with XAML and as such this scrolling behaviour is becoming standard and is now present and consistent across the whole of Windows 10's UI. ie. The photos app, calendar app, mail, cortana, settings etc.

Regardless of the subjectivity of preference between the two profiles I would argue that Firefox should match the scrolling behaviour of Windows as much as possible.  While this never used to be a problem, now that Windows 10 has this scrolling behaviour system wide, the switch to the behaviour found in Firefox is actually quite jarring.
Attachment #8703142 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8703142 [details]
MozReview Request: Bug 1199468 - Create prefs for the smooth scroll timing function shape. r?kats

https://reviewboard.mozilla.org/r/29285/#review26063

::: gfx/thebes/gfxPrefs.h:214
(Diff revision 1)
> +  DECL_GFX_PREF(Live, "general.smoothScroll.currentVelocityWeighting",

Move this one up to just after general.smoothScroll, so that it's in alphabetical order by pref name.
>  DECL_GFX_PREF(Live, "general.smoothScroll.mouseWheel.durationMaxMS",
>                WheelSmoothScrollMaxDurationMs, int32_t, 400);

I'd imagine you also wanted to change this ^ to 250?

What did you base your changes on? Was it solely on the premise that it should be as similar to native as possible? or as similar to IE as possible? Or does it also imply an opinion that IE is better? Whose opinion is that?

Also, as far as I can tell, there's no such attempt to make the Linux/OS X scroll similar to native (I believe both could be considered to have smooth scroll disabled by default to begin with, yet it's enabled in Firefox - and rightfully so IMO).

So it would seem to me that the goal here has been to make it as similar to IE as possible, implying that it's the best smooth scroll we know of thus far. Would that be correct?

While in general I do agree that Firefox should behave as similar to native as possible, we do have quite a few cases where it doesn't, and IMO this is one case where it shouldn't. Mostly because IMO Firefox just does it better than IE, though again, it's clear to me that this is a subjective opinion.

I'll experiment with the new patch/prefs before I express my opinion on it, but meanwhile you could answer the questions above.

Specifically for the 400->250 change, this is the max duration which is only used when scrolling very slowly, or one click at a time. However, I _think_ that users usually complain about slow/laggy scrolling only when scrolling quickly - which is where the minimum value is used (200ms was and still is with your patch). Does it imply a preference that a single scroll "tick" should be quicker than it was, but longer/quicker consecutive ticks should behave the same? Any interesting reason that the patch sets it as 250 rather than 200 like the minimum value?

I still think we should do some controlled experiment (even with only the people who expressed interest in this subject at this bug, though preferably with others too), where we come up with few (2-4) different configurations, a single test page which is light enough to not miss frames on most systems (though with APZ it's mostly moot these days), and let people grade them and provide minimal textual feedback too.

If a clear consensus emerges, other than potential improvements, I'd imagine everyone would agree that's the way to go forward, myself included.
Comment on attachment 8703143 [details]
MozReview Request: Bug 1199468 - Tighten the smooth scroll animation prefs. r?avih

https://reviewboard.mozilla.org/r/29287/#review26325

I still don't think a single person should approve the behavior changes.

::: modules/libpref/init/all.js:2106
(Diff revision 1)
> -pref("general.smoothScroll.mouseWheel.durationMaxMS", 400);
> +pref("general.smoothScroll.mouseWheel.durationMaxMS", 250);

I prefer the way it was, but I can live with that.

::: modules/libpref/init/all.js:2132
(Diff revision 1)
> -pref("general.smoothScroll.stopDecelerationWeighting", "0.4");
> +pref("general.smoothScroll.stopDecelerationWeighting", "0.82");

These two changes completely break smoothness IMO. Yes, it's somewhat more similar to IE, but IMO that's a bad thing.
Attachment #8703143 - Flags: review?(avihpit)
After discussing this with mstange on IRC, we both understand the spectrum and dimensions this can end up as, and also agree it's largely subjective.

I don't dispute that the patch at comment 25 makes it more similar to IE, but I do argue that our goal here should be to make it as best as we can rather than as similar to IE as we can. Such goal would also suggest we should end up with similar behavior on all relevant platforms rather than as similar to each platform's native behavior (which is a hard thing to define even on its own).

Whether or not the "best for most people" is similar to IE is a question which remains open.

Both mstange and myself also agree that the most reasonable approach in such case would be to ask several people to choose between few configs, and choose the one which seems to have the highest consensus.

Also, note that this is completely orthogonal to issues of missed frames or pixel aligning. I.e the patch (and IMO the discussion too) is focused on the behavior/curves/profiles of the scroll progression and "connection" of consecutive scroll events. I think the issue of missed frames is largely solved with APZ, and if we still want, we can assess pixel snapping in a different discussion.

So mstange and myself will probably come up with 2 more prefs combinations which people could try out and vote on.

We'll probably want to land the first patch (comment 24) which already has r+ and will allow more dimensions of the configurations to try out.

Markus, sounds about right?
Flags: needinfo?(mstange)
Yes, that sounds right. I've landed the prefs patch.
Flags: needinfo?(mstange)
Keywords: leave-open
Sorry, this completely fell off my radar.
There seem to be huge differences between different input methods (touch, touchpad, precision touchpad, scroll wheel) in how well various configurations work.
I'm very sympathetic to the suggestion in comment 23, which is to use the same pan/zoom behavior that Edge uses (since they seem to have figured out how to deal with those differences pretty well). I don't know how big of an undertaking that would be.
Flags: needinfo?(philipp)
Here's a try build with tweaked smooth scroll physics: https://queue.taskcluster.net/v1/task/Uj55p0V0Q3i-fj7FDziM8w/runs/0/artifacts/public/build/target.zip

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24727cb0ac670c6753fb02803f44cc0405a8aa49

I've tried to make scrolling with regular scroll wheels feel a bit more like in Edge. I haven't tested this change with other input methods. The goal here was to maybe find a quick way of tweaking things to get a cheap improvement in 57.
I don't have the domain knowledge or the time to look into using DirectManipulation for scrolling.

I'd like to encourage interested parties to test the build.
Might want to move any follow-up discussion to bug 1388848
See Also: → 1388848

The leave-open keyword is there and there is no activity for 6 months.
:jbonisteel, maybe it's time to close this bug?

Flags: needinfo?(jbonisteel)

Patch landed in Firefox 46, closing this bug. Any follow-up can happen in bug 1388848.

Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jbonisteel)
Keywords: leave-open
Resolution: --- → FIXED
Assignee: nobody → mstange
You need to log in before you can comment on or make changes to this bug.