Closed Bug 1235686 Opened 8 years ago Closed 8 years ago

Mouse scroll speed may not be correct if Synatpcis's or Alps's touchpad and another pointing device are used on an environment

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Version:
42 Branch
Platform:
x86_64
Windows 10
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- fixed
firefox46 --- verified

People

(Reporter: nikpmr, Assigned: masayuki)

References

Details

Attachments

(5 files, 8 obsolete files)

screenshot of the tray in the taskbar
52.40 KB, image/png
Details
part.1 MouseScrollHandler should refresh the cache of system settings at handling wheel messages if the pref doesn't allow to cache system settings (r=jimm)
13.38 KB, patch
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta-
Details | Diff | Splinter Review
part.2 Don't refresh the cache of system settings at handling wheel messages if the scroll amout values are initialized with prefs (r=jimm)
4.05 KB, patch
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta-
Details | Diff | Splinter Review
part.3 Don't trust system settings cache if SynTP of Synaptics is installed (r=jimm)
6.95 KB, patch
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta-
Details | Diff | Splinter Review
part.4 Don't trust system settings cache if Apoint of Alps is installed (r=jimm)
6.33 KB, patch
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta-
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

Start Firefox, first use the touchpad to scroll down a page, then switch back to the mouse wheel. 


Actual results:

The page scrolls one line at a time. 


Expected results:

The page should scroll multiple lines at a time.

Starting Firefox and then first using the mouse wheel to scroll produces the desired results. It is as if Firefox sets the scroll speed based on which input device it detects first.
Product: Firefox → Core
Hi reporter,

I have managed to reproduce this issue on the latest Firefox release (43.0.3) and on the latest Nightly build (46.0a1-20160104030217) on Windows 10 x64. I have tested this using a Dell XPS 12 2 in 1 laptop device. 
I marking this issue as NEW and I'm setting a component for it. If the component is not the right one, please feel free to update it with a more appropriate one. 

Thanks,
Paul
Status: UNCONFIRMED → NEW
status-firefox43: --- → affected
status-firefox46: --- → affected
Component: Untriaged → Event Handling
Ever confirmed: true
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Thank you for your attention to this Paul. 
Hopefully a resolution can be found soon.
masayuki, have you ever seen this kind of behavior?

I need to find a mouse to test this.
(In reply to Olli Pettay [:smaug] from comment #4)
> masayuki, have you ever seen this kind of behavior?

Yeah, probably, I can reproduce this. Indeed, I *feel* the scroll speed slower if I scroll a page by touchpad.

My touchpad is Synaptics's touchpad and my mouse is Logitech's MX Master.
I can reproduce this bug without restarting Firefox. After you reproduce this bug,

1. Open Settings.
2. Move to Devices -> Mouse and Touchpad
3. Change scroll lines to different value.
4. Restore scroll lines to original value.
5. You can test it again.

According to this STR, we failed to refresh system settings in MouseScrollHandler?
When I use touchpad first, SystemParametersInfo(SPI_GETWHEELSCROLLLINES) returns 1. However, when I use mouse first, SystemParametersInfo(SPI_GETWHEELSCROLLLINES) returns 3. However, there is no WM_SETTINGCHANGE message with SPI_SETWHEELSCROLLLINES at changing the device.
Component: Event Handling → Widget: Win32
I confirmed that any message isn't sent to us when I change mouse device. We should stop caching mouse scroll speed at least on Win 10.

Does somebody know this is reproducible on Win 8.1 or earlier?
Hi, could you tell me which vendor's touchpad are you using?

I guess that in the tray in the taskbar, there is a touchpad's settings like this screenshot. From this, you can check your touchpad's vendor. I think that the name may be one of "Synaptics", "Elantech" or "Alps".
Flags: needinfo?(paul.oiegas)
Flags: needinfo?(nikpmr)
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
I reproduced this bug on x86_32 Windows 7 and the latest Nightly.

I'm using Synaptics TouchPad V7.5 and Toshiba dynabook's mouse (USB wireless type).
I also reproduced this bug on the following Firefox

38.5.2 (esr)
43.0.4 (release)
44.0b6 (beta)
45.0a2 (2016-01-07)(aurora)
46.0a1 (2016-01-07)(nightly)
I am using a Synaptics touch pad and Logitech mouse as well on Windows 10.
Flags: needinfo?(nikpmr)
Ryo Kato-san and nikpmr, thank you very much for reporting your environment. That's very helpful information.

I'm thinking that we should forget the cache if Synaptics's driver is installed into the environment.
I confirmed that I can reproduce same bug with Alps Apoint 10.100.404.105.

Unfortunately, I don't have a PC whose touchpad is Elantech.
Hi Masayuki,

Sorry for the delay, other timezone. :)
Device Dell XPS 12 2 in 1 laptop. And I'm using Dell Touchpad that is manufactured by Synaptics. Driver version is 19.0.15.2.
Flags: needinfo?(paul.oiegas)
Thank you, Paul.

So, now, all victims are using Synaptics. And I confirmed the Alps's touchpad too. We should not use system settings cache if one of them is installed.
We need a new path not to use system settings cache for computing scroll amount.

This patch creates the path and user can disable or forcibly enable it intentionally if our check logic (implemented by following patches) doesn't work well.
Attachment #8705529 - Flags: review?(jmathies)
If user or auto mated test overrides the scroll amount with prefs, we can trust the cached scroll amount. So, in such case, we should not use the new path.
Attachment #8705530 - Flags: review?(jmathies)
If we can retrieve driver version of SynTP (Synaptics's touchpad) from registry, we should mark the system settings cache dirty at handling every WM_MOUSE(H)WHEEL.
Attachment #8705544 - Flags: review?(jmathies)
Apoint which is Alps's touchpad also returns different value for calls of SystemParametersInfo(SPI_GETWHEELSCROLLLINES). So, we should do same thing as SynTP for Apoint too.
Attachment #8705572 - Flags: review?(jmathies)
Hi, for checking we have same path in the registry, I'd be happy if you'd test the patched build:
http://archive.mozilla.org/pub/firefox/try-builds/masayuki@d-toybox.com-515ec3691fa987ff6c2c160be72d42f2dc271333/try-win32/firefox-46.0a1.en-US.win32.zip

If this build doesn't fix this bug on your environment, let me know the registry path to the version of SynTP driver. The path must be under HKEY_LOCAL_MACHINE\SOFTWARE\Synaptics(\SynTP).
Flags: needinfo?(paul.oiegas)
Flags: needinfo?(nikpmr)
Flags: needinfo?(foobar094)
Hi,

Just tested your build on the Dell XPS 12 and I cannot reproduce the issue anymore. Both touchapd and mouse scroll functionalities are working fast and smooth now.

Good job :)
Flags: needinfo?(paul.oiegas)
I've just tested the build and confirmed that this issue cannot be reproduce on x86_32 Windows 7. Both ways of scrolling are working smoothly ;)
Flags: needinfo?(foobar094)
Comment on attachment 8705529 [details] [diff] [review]
part.1 MouseScrollHandler should clear system settings cache at handling wheel messages if the pref doesn't allow to cache system settings

Oh, I found a regression. The patched build cannot scroll tabbar and tree view smoothly.
Attachment #8705529 - Flags: review?(jmathies) → review-
Attachment #8705530 - Flags: review?(jmathies) → review-
Attachment #8705544 - Flags: review?(jmathies) → review-
Attachment #8705572 - Flags: review?(jmathies) → review-
Paul, Ryo Kato-san, thank you very much for your testing!
I can also confirm that your build corrects the issue.
Flags: needinfo?(nikpmr)
Okay, at handling mouse wheel messages, we should update the cache if the cache is disabled by pref and only when the new value is different from old value (i.e., user changed the input device), we should reset the transaction.
Attachment #8705529 - Attachment is obsolete: true
Attachment #8705530 - Attachment is obsolete: true
Attachment #8705544 - Attachment is obsolete: true
Attachment #8705572 - Attachment is obsolete: true
Attachment #8706069 - Flags: review?(jmathies)
If user or automated test overrides the scroll amount with prefs, we can trust the cached scroll amount. So, in such case, we should not use the new path.
Attachment #8706070 - Flags: review?(jmathies)
If we can retrieve driver version of SynTP (Synaptics's touchpad) from registry, we should update the system settings cache at handling every WM_MOUSE(H)WHEEL.
Attachment #8706071 - Flags: review?(jmathies)
Apoint which is Alps's touchpad also returns different value for calls of SystemParametersInfo(SPI_GETWHEELSCROLLLINES). So, we should do same thing as SynTP for Apoint too.
Attachment #8706072 - Flags: review?(jmathies)
(In reply to nikpmr from comment #28)
> I can also confirm that your build corrects the issue.

Thank you for your test. I guess all synatipcs users must be helped with the patches.

And I found a test who has Elantech's touchpad. However, he said that Elantech's touchpad doesn't cause this bug. I confirmed it with log files which are logged by him.
Summary: Scroll speed varies based on what input devices is used first → Mouse scroll speed may not be correct if Synatpcis's or Alps's touchpad and another pointing device are used on an environment
Curious, could we not have driver specific checks, and just call RefreshCache always (or in other words, not cache)? If not, why not?
Yeah, we can stop caching system settings in any environments. However, I'm afraid to waste CPU power at handling every wheel message since e.g., Microsoft and Logitech's expensive mouse send much a lot of wheel messages for smoother scrolling.

Anyway, this is hack for illegal touchpad drivers. So, I don't like that normal environment's running cost will increase for supporting illegal devices...
Well, aren't synaptics and alps the most common touchpads? So we'd end up bypassing the cache
anyway rather often.
Could we detect the case when OS has more than one pointing device?
(In reply to Olli Pettay [:smaug] from comment #36)
> Well, aren't synaptics and alps the most common touchpads?

Although, I cannot find the market share. I feel that most notebook users use Synaptics's touchpad. Maybe, next player is Elantech because I saw them on cheaper notebooks/netbooks around 2009, and Alps is now rare (I was surprised my wife's PC has it!) because I don't see it after touchpad supports multi-touch.

> So we'd end up bypassing the cache anyway rather often.

Absolutely, except desktop PC users.

> Could we detect the case when OS has more than one pointing device?

I guess that we can do that with using GetRawInputDeviceList.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms645598%28v=vs.85%29.aspx

And we can detect the change of number of input devices with WM_INPUT_DEVICE_CHANGE.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms645591%28v=vs.85%29.aspx

But I'm not familiar with raw input handling, so, it's much risky to uplift.
(I think that this bug is making a lot of our users feel Firefox slow, so, we should uplift these patches as far as possible.)
Attachment #8706070 - Flags: review?(jmathies) → review+
Attachment #8706071 - Flags: review?(jmathies) → review+
Attachment #8706072 - Flags: review?(jmathies) → review+
Comment on attachment 8706069 [details] [diff] [review]
part.1 MouseScrollHandler should refresh the cache of system settings at handling wheel messages if the pref doesn't allow to cache system settings

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

::: widget/windows/WinMouseScrollHandler.h
@@ +278,5 @@
> +    // device to another one, the result of SystemParametersInfo() may be
> +    // changed without WM_SETTINGCHANGE message.  For avoiding this trouble,
> +    // we need to modify cache of system settings at every wheel message
> +    // handling if we meet known device whose utility may hook the API.
> +    void WillHandleWheelMessage(bool aIsVertical);

Maybe use something a bit more accurate like "TrustedScrollSettingsDriver()". WillHandleWheelMessage doesn't really seem to match what this method does.
Attachment #8706069 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e701ede86cd8111898ae2435b2dd95ab500284a7
Bug 1235686 part.1 MouseScrollHandler should refresh the cache of system settings at handling wheel messages if the pref doesn't allow to cache system settings r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/47df2f4f6e9321f52e4c797f70d722eb53b8118d
Bug 1235686 part.2 Don't refresh the cache of system settings at handling wheel messages if the scroll amout values are initialized with prefs r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c543377304031c4d7c9a948d4c5bf310d7bdc96
Bug 1235686 part.3 Don't trust system settings cache if SynTP of Synaptics is installed r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/d262f278909c57dfeae0f51067d0be734c8d4f7e
Bug 1235686 part.4 Don't trust system settings cache if Apoint of Alps is installed r=jimm
Approval Request Comment
[Feature/regressing bug #]: Not a regression, but this fix may make users feel Firefox faster
[User impact if declined]: Users who use Synatpcis's touchpad (according to the Synaptics's data, the market share is 70%) and external mouse, the external mouse's wheel scrolls content very slow (typically, 1/3 of system default settings) if user tries to scroll the touchpad first.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low because these patches just disable system settings of scroll speed cache is disabled if Synaptics's touchpad driver or Alps's touchpad driver is install into the system.
[String/UUID change made/needed]: Nothing.
Attachment #8706069 - Attachment is obsolete: true
Attachment #8708268 - Flags: approval-mozilla-beta?
Attachment #8708268 - Flags: approval-mozilla-aurora?
Approval Request Comment
See comment 41.
Attachment #8706071 - Attachment is obsolete: true
Attachment #8708271 - Flags: approval-mozilla-beta?
Attachment #8708271 - Flags: approval-mozilla-aurora?
Attachment #8708271 - Attachment description: bug1235686-3.diff → part.3 Don't trust system settings cache if SynTP of Synaptics is installed (r=jimm)
Approval Request Comment
See comment 41.
Attachment #8706072 - Attachment is obsolete: true
Attachment #8708272 - Flags: approval-mozilla-beta?
Attachment #8708272 - Flags: approval-mozilla-aurora?
Comment on attachment 8708268 [details] [diff] [review]
part.1 MouseScrollHandler should refresh the cache of system settings at handling wheel messages if the pref doesn't allow to cache system settings (r=jimm)

Firefox 44 release enters RC mode this week and the update bar continues to be high. We are only taking fixes for critical (recent) regressions, top crashes and sec-crit fixes. Given that criteria, this issue does not meet the bar for Beta44.
Attachment #8708268 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8708270 [details] [diff] [review]
part.2 Don't refresh the cache of system settings at handling wheel messages if the scroll amout values are initialized with prefs (r=jimm)

Does meet the Beta44 RC uplift criteria, please my last comment for more details.
Attachment #8708270 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8708271 [details] [diff] [review]
part.3 Don't trust system settings cache if SynTP of Synaptics is installed (r=jimm)

Does not meet the Beta44 RC uplift criteria, please my last comment for more details.
Attachment #8708271 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8708272 [details] [diff] [review]
part.4 Don't trust system settings cache if Apoint of Alps is installed (r=jimm)

Does not meet the Beta44 RC uplift criteria, please my last comment for more details.
Attachment #8708272 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Hey Ritu, what about Firefox 45? Is it going to be skipped?
Guys, could you verify that it is fixed on 46? This is a pretty big patch and I would like to be sure that we are correctly fixing the issue before making a call. Thanks
Flags: needinfo?(paul.oiegas)
Flags: needinfo?(nikpmr)
Using 46 now. Problem has been resolved. Thanks a bunch.
Flags: needinfo?(nikpmr)
Hi Sylvestre,

I have retested this on latest Nightly build (46.0a1-20160118030338) and the original issue is no longer reproducible. I have also tested with slower and faster scrolling speeds settings for both touchpad and mouse and both the devices are working independently of each other now.

Do you guys think of any more thorough test scenarios on this ? If yes, I would be glad to help to execute them.

Thanks,
Paul
status-firefox46: fixed → verified
Flags: needinfo?(paul.oiegas)
No, I trust you judgment, if you consider the issue as fixed, I am happy!
Comment on attachment 8708268 [details] [diff] [review]
part.1 MouseScrollHandler should refresh the cache of system settings at handling wheel messages if the pref doesn't allow to cache system settings (r=jimm)

So, let's take it!
Attachment #8708268 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8708270 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8708271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8708272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.