Last Comment Bug 657634 - Don't use high resolution scrolling when scrolling speed is customized by prefs
: Don't use high resolution scrolling when scrolling speed is customized by prefs
Status: VERIFIED FIXED
nominated by Alice0775 at comment 1
: regression
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All Windows 7
: -- normal (vote)
: mozilla6
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London)
:
Mentors:
data:text/html;charset=utf-8,<!DOCTYP...
Depends on: 658155
Blocks: 605648 657935 658111
  Show dependency treegraph
 
Reported: 2011-05-17 08:16 PDT by Alex
Modified: 2013-12-27 14:30 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Patch v1.0 (15.31 KB, patch)
2011-05-18 01:14 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London)
no flags Details | Diff | Review
Patch v1.0.1 (15.32 KB, patch)
2011-05-18 23:56 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London)
masayuki: review-
Details | Diff | Review
Patch v1.1 (15.52 KB, patch)
2011-05-19 05:10 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London)
bugs: review+
Details | Diff | Review

Description Alex 2011-05-17 08:16:09 PDT
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 2011-05-17 09:23:56 PDT
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] (Mozilla Japan) (not in London) 2011-05-18 01:14:09 PDT
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] (high review load, please consider other reviewers) 2011-05-18 12:09:15 PDT
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] (Mozilla Japan) (not in London) 2011-05-18 23:56:53 PDT
Created attachment 533549 [details] [diff] [review]
Patch v1.0.1

fix a nit, sorry for the spam.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London) 2011-05-19 02:57:06 PDT
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] (Mozilla Japan) (not in London) 2011-05-19 05:10:32 PDT
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] (Mozilla Japan) (not in London) 2011-05-19 05:14:01 PDT
(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] (high review load, please consider other reviewers) 2011-05-21 13:52:33 PDT
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] (Mozilla Japan) (not in London) 2011-05-21 17:09:25 PDT
(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] (Mozilla Japan) (not in London) 2011-05-21 22:43:17 PDT
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] 2011-07-27 05:03:22 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0b3

Note You need to log in before you can comment on or make changes to this bug.