The default bug view has changed. See this FAQ.

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
3 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
(Assignee)

Comment 2

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)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 658111
(Assignee)

Updated

6 years ago
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
(Assignee)

Updated

6 years ago
OS: All → Windows 7
(Assignee)

Updated

6 years ago
Depends on: 658155
(Assignee)

Comment 4

6 years ago
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)
(Assignee)

Comment 5

6 years ago
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-
(Assignee)

Comment 6

6 years ago
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)
(Assignee)

Comment 7

6 years ago
(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+
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

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