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)
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)
15.52 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•14 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•14 years ago
|
Assignee | ||
Comment 2•14 years ago
|
||
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•14 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•14 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•14 years ago
|
OS: All → Windows 7
Assignee | ||
Comment 4•14 years ago
|
||
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•14 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•14 years ago
|
||
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•14 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•14 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•14 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•14 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
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Updated•14 years ago
|
Whiteboard: nominated by Alice0775 at comment 1
Updated•14 years ago
|
Updated•14 years ago
|
Comment 11•14 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
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•