657634 - Don't use high resolution scrolling when scrolling speed is customized by prefs

Status: VERIFIED FIXED

Description

[Alex]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110517 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110517 Firefox/6.0a1

Speed ​​page scrolling does not change when changing these values​​.
Remains equal to 3 lines.

Reproducible: Always

Steps to Reproduce:
1. Open about:config
2. find mousewheel.withnokey.numlines
3. change to anything number
4. restart
OR
1. Open about:config
2. find mousewheel.withnokey.numlines
3. change to anything number
4. find mousewheel.withnokey.sysnumlines
5. change to false
6. restart

Actual Results:  
The scrolling speed is still equal to 3 lines.

Expected Results:  
Scroll speed becomes equal to the value mousewheel.withnokey.numlines.

Comment 1

[Alice0775 White]

Confirmed on
http://hg.mozilla.org/mozilla-central/rev/f717485edc51
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110517 Firefox/6.0a1 ID:20110517030625

Regression window:
Works;
http://hg.mozilla.org/mozilla-central/rev/3e7a21049b6c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110516 Firefox/6.0a1 ID:20110516160657
Fails:
http://hg.mozilla.org/mozilla-central/rev/0a1e7ec7e268
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110516 Firefox/6.0a1 ID:20110516172821
Regressed by:
0a1e7ec7e268	Masayuki Nakano — Bug 605648 Support high resolution scrolling on Windows r=jimm+smaug

Comment 2

[Masayuki Nakano [:masayuki] (JST, +0900)]

Created attachment 533201 [details] [diff] [review]
Patch v1.0

If the wheel action doesn't scroll contents or the scroll amount is customized, we shouldn't use high resolution scrolling for now. I'll file a follow up bug of this.

Comment 3

[Olli Pettay [:smaug]]

Comment on attachment 533201 [details] [diff] [review]
Patch v1.0

This is getting complicated.
Does the patch which disables pixel scrolling on Windows fix this problem
temporarily?

I'll try to review this tomorrow.

Comment 4

[Masayuki Nakano [:masayuki] (JST, +0900)]

Created attachment 533549 [details] [diff] [review]
Patch v1.0.1

fix a nit, sorry for the spam.

Comment 5

[Masayuki Nakano [:masayuki] (JST, +0900)]

Comment on attachment 533549 [details] [diff] [review]
Patch v1.0.1

I found a mistake, I'll post new patch after I check the fix.

Comment 6

[Masayuki Nakano [:masayuki] (JST, +0900)]

Created attachment 533608 [details] [diff] [review]
Patch v1.1

Sorry for the delay. I found another bug which has been since first implementation of pixel scrolling.

> PRInt32
> nsEventStateManager::ComputeWheelActionFor(nsMouseScrollEvent* aMouseEvent,
>                                            PRBool aUseSystemSettings)
> {
>   PRInt32 action = GetWheelActionFor(aMouseEvent);
>   if (aUseSystemSettings &&
>       (aMouseEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage)) {
>     action = MOUSE_SCROLL_PAGE;
>   }
> 
>   if (aMouseEvent->message == NS_MOUSE_PIXEL_SCROLL) {
>     if (action == MOUSE_SCROLL_N_LINES || action == MOUSE_SCROLL_PAGE ||
>         (aMouseEvent->scrollFlags & nsMouseScrollEvent::kIsMomentum)) {
>       action = MOUSE_SCROLL_PIXELS;
>     } else {
>       // Do not scroll pixels when zooming
>       action = -1;
>     }
>   } else if (aMouseEvent->scrollFlags & nsMouseScrollEvent::kHasPixels) {
>     if (aUseSystemSettings ||
>         action == MOUSE_SCROLL_N_LINES || action == MOUSE_SCROLL_PAGE ||
>         (aMouseEvent->scrollFlags & nsMouseScrollEvent::kIsMomentum)) {
>       // Don't scroll lines when a pixel scroll event will follow.
>       // Also, don't do history scrolling or zooming for momentum scrolls.
>       action = -1;
>     }
>   }
> 
>   return action;
> }

The final if should check MOUSE_SCROLL_PAGE. I had realized this at creating non-adhoc patch for bug 605648 but I forgot it.

Comment 7

[Masayuki Nakano [:masayuki] (JST, +0900)]

(In reply to comment #3)
> This is getting complicated.

Basically, most changes are moving code, not writing newly.

> Does the patch which disables pixel scrolling on Windows fix this problem
> temporarily?

Do you mean "the patch" is for bug 605648? If so, yes, all regressions which have been found are hidden by the patch.

Comment 8

[Olli Pettay [:smaug]]

Comment on attachment 533608 [details] [diff] [review]
Patch v1.1

We have so many prefs for mousewheel handling that we really should have good tests.
I wonder how to write such.


Please add a comment how ComputeWheelActionFor and GetWheelActionFor differ.


> nsEventStateManager::DoQueryScrollTargetInfo(nsQueryContentEvent* aEvent,
>                                              nsIFrame* aTargetFrame)
> {
>   nsMouseScrollEvent* msEvent = aEvent->mInput.mMouseScrollEvent;
>+
>+  // Don't use high resolution scrolling when user customize the scrolling
>+  // speed.
>+  if (!UseSystemScrollSettingFor(msEvent)) {
>+    return;
>+  }
>+
>   nsIScrollableFrame::ScrollUnit unit;
>-  if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage) {
>-    unit = nsIScrollableFrame::PAGES;
>-  } else {
>-    unit = nsIScrollableFrame::LINES;
>+  PRBool allowOverrideSystemSettings;
>+  switch (ComputeWheelActionFor(msEvent, PR_TRUE)) {
>+    case MOUSE_SCROLL_N_LINES:
>+      unit = nsIScrollableFrame::LINES;
>+      allowOverrideSystemSettings = PR_TRUE;
>+      break;
>+    case MOUSE_SCROLL_PAGE:
>+      unit = nsIScrollableFrame::PAGES;
>+      allowOverrideSystemSettings = PR_FALSE;
>+      break;
>+    default:
>+      // Don't use high resolution scrolling when the action doesn't scroll
>+      // contents.
>+      return;
What if the action is MOUSE_SCROLL_PIXELS?
In that case allowOverrideSystemSettings should PR_FALSE and
unit = nsIScrollableFrame::DEVICE_PIXELS , right?

Those fixed, r=me

Comment 9

[Masayuki Nakano [:masayuki] (JST, +0900)]

(In reply to comment #8)
> We have so many prefs for mousewheel handling that we really should have
> good tests.
> I wonder how to write such.

I think that we should create a new API for nsDOMWindowUtils for calling native mouse wheel event handler in nsWindow or nsChildView.

I think we should create it before we start to work for DOM3 mouse wheel event implementation.

> > nsEventStateManager::DoQueryScrollTargetInfo(nsQueryContentEvent* aEvent,
> >                                              nsIFrame* aTargetFrame)
> > {
> >   nsMouseScrollEvent* msEvent = aEvent->mInput.mMouseScrollEvent;
> >+
> >+  // Don't use high resolution scrolling when user customize the scrolling
> >+  // speed.
> >+  if (!UseSystemScrollSettingFor(msEvent)) {
> >+    return;
> >+  }
> >+
> >   nsIScrollableFrame::ScrollUnit unit;
> >-  if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage) {
> >-    unit = nsIScrollableFrame::PAGES;
> >-  } else {
> >-    unit = nsIScrollableFrame::LINES;
> >+  PRBool allowOverrideSystemSettings;
> >+  switch (ComputeWheelActionFor(msEvent, PR_TRUE)) {
> >+    case MOUSE_SCROLL_N_LINES:
> >+      unit = nsIScrollableFrame::LINES;
> >+      allowOverrideSystemSettings = PR_TRUE;
> >+      break;
> >+    case MOUSE_SCROLL_PAGE:
> >+      unit = nsIScrollableFrame::PAGES;
> >+      allowOverrideSystemSettings = PR_FALSE;
> >+      break;
> >+    default:
> >+      // Don't use high resolution scrolling when the action doesn't scroll
> >+      // contents.
> >+      return;
> What if the action is MOUSE_SCROLL_PIXELS?
> In that case allowOverrideSystemSettings should PR_FALSE and
> unit = nsIScrollableFrame::DEVICE_PIXELS , right?

Yes, but is it needed? I don't have a plan which I use this event with pixel scroll event. But it's easy, I'll add it before landing.

Comment 10

[Masayuki Nakano [:masayuki] (JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/d4746fb15403

If you have some suggestion about the documents of each method, I'll modify it on another regression bug's patch.

Comment 11

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0b3