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

VERIFIED FIXED in mozilla6

Status

()

Core
Event Handling
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Alex, Assigned: masayuki)

Tracking

({regression})

Trunk
mozilla6
All
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6-)

Details

(Whiteboard: nominated by Alice0775 at comment 1, URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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
Blocks: 605648
Status: UNCONFIRMED → NEW
tracking-firefox6: --- → ?
Component: General → Event Handling
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: general → events
Version: unspecified → Trunk

Updated

6 years ago
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #533201 - Flags: review?(Olli.Pettay)
Blocks: 657935

Comment 3

6 years ago
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.
Blocks: 658111
Summary: Ignores the replacement values ​​for the parameters mousewheel.withnokey.numlines and sysnumlines → Don't use high resolution scrolling when scrolling speed is customized by prefs
OS: All → Windows 7
Depends on: 658155
Created attachment 533549 [details] [diff] [review]
Patch v1.0.1

fix a nit, sorry for the spam.
Attachment #533201 - Attachment is obsolete: true
Attachment #533201 - Flags: review?(Olli.Pettay)
Attachment #533549 - Flags: review?(Olli.Pettay)
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.
Attachment #533549 - Flags: review?(Olli.Pettay) → review-
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.
Attachment #533549 - Attachment is obsolete: true
Attachment #533608 - Flags: review?(Olli.Pettay)
(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

6 years ago
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
Attachment #533608 - Flags: review?(Olli.Pettay) → review+
(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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6

Updated

6 years ago
Whiteboard: nominated by Alice0775 at comment 1

Updated

6 years ago
tracking-firefox6: ? → +

Updated

6 years ago
tracking-firefox6: + → -

Comment 11

6 years ago
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0b3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.