Closed Bug 657634 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla6
Tracking Status
firefox6 - ---

People

(Reporter: black0swamp, Assigned: masayuki)

References

()

Details

(Keywords: regression, Whiteboard: nominated by Alice0775 at comment 1)

Attachments

(1 file, 2 obsolete files)

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.
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
Component: General → Event Handling
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: general → events
Version: unspecified → Trunk
Attached patch Patch v1.0 (obsolete) — Splinter Review
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)
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.
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
Attached patch Patch v1.0.1 (obsolete) — Splinter Review
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-
Attached patch Patch v1.1Splinter Review
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 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
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Whiteboard: nominated by Alice0775 at comment 1
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0b3
Status: RESOLVED → VERIFIED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: