Closed
Bug 513817
Opened 15 years ago
Closed 15 years ago
Switch scrolling to 6 lines in the default case for 3.6 on windows
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 3.6b1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: faaborg, Assigned: masayuki)
References
(Depends on 2 open bugs)
Details
Attachments
(2 files, 16 obsolete files)
38.01 KB,
patch
|
Details | Diff | Splinter Review | |
38.01 KB,
patch
|
Details | Diff | Splinter Review |
A lot of discussion has gone into the changes proposed in bug 462809, including: Bug 509189 - The application level mouse wheel scrolling acceleration should be disabled in default settings Bug 508785 - Extremely high speed of mouse-wheel scrolling Bug 508747 - mousewheel.withnokey.numlines overrides the OS setting and the follow up bug: Bug 509651 - Disable Firefox's acceleration based scrolling model if the mouse driver already provides it. We still believe that the ideal state is solving bug 509651. However, this requires additional research on the best way to detect an existing scrolling model, and it won't be ready for 3.6. So if we back out the acceleration model, we are left with the problem that Chrome is perceptually twice as fast as us at scrolling. To address that for 3.6, I propose we just copy their behavior, and double the OS setting for lines to scroll. For the vast majority of users this will be 6 lines.
Reporter | ||
Updated•15 years ago
|
Flags: blocking-firefox3.6?
Comment 1•15 years ago
|
||
Margaret: any chance you'll have time for this? We're happy to have someone else handle it, but wanted to give you right of first refusal! :)
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Assignee | ||
Comment 2•15 years ago
|
||
Please wait bug 512235. This will be fixed easily after the patch.
Depends on: 512235
Assignee | ||
Comment 3•15 years ago
|
||
I think that you should reduce the default pref value of |mousewheel.withnokey.numlines|. And you add new pref |mousewheel.scroll_speed_factor| and you use this value in |nsMouseWheelTransaction::AccelerateWheelDelta|.
Assignee | ||
Comment 4•15 years ago
|
||
checking on tryserver...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•15 years ago
|
||
Probably, this is Alex Faaborg wanted patch. https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug513817-3/ The acceleration is disabled, and the scrolling speed is always doubled. Check the behavior. And also this fixes a nit: * Fix a bug of nsMouseWheelTransaction::ComputeWheelDelta, if the factor is 0, the users cannot scroll by the wheel. And I wonder can we accelerate the scrolling speed when the wheel event is pixel scrolling. I'm not sure this mode's preferred behavior.
Attachment #398402 -
Attachment is obsolete: true
Attachment #398625 -
Flags: review?(roc)
Attachment #398625 -
Flags: review?(faaborg)
Comment 6•15 years ago
|
||
Umm. If you want to ignore the system pref and use 6, do that. We shouldn't do (system pref)*2. If, for instance, someone had set that pref to 6 in order to speed up scrolling, we'd now do 12, which is not what that person wants.
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #6) > Umm. If you want to ignore the system pref and use 6, do that. We shouldn't do > (system pref)*2. If, for instance, someone had set that pref to 6 in order to > speed up scrolling, we'd now do 12, which is not what that person wants. My patch always multiplies the scroll speed. I.e., both the value from system settings and the our prefs. If you think we should multiply the speed only when we honor the system settings, we can do it.
Comment 8•15 years ago
|
||
I was referring to the system setting when I said "if someone had set that pref to 6".
Assignee | ||
Comment 9•15 years ago
|
||
How about this approach? This patch has the limitation of the multiplied speed (8). And this patch doesn't multiply the non-system settings scrolling.
Attachment #398625 -
Attachment is obsolete: true
Attachment #398625 -
Flags: review?(roc)
Attachment #398625 -
Flags: review?(faaborg)
Comment 11•15 years ago
|
||
[Removing "on windows" from summary, since scroll-acceleration affects all platforms now, after bug 512235.]
Summary: Switch scrolling on Windows to (system pref)*2 for 3.6 → Switch scrolling to (system pref)*2 for 3.6
Assignee | ||
Comment 12•15 years ago
|
||
fix some nits. I think that we shouldn't accelerate when the wheel event isn't trusted event.
Attachment #398704 -
Attachment is obsolete: true
Assignee | ||
Comment 13•15 years ago
|
||
Comment on attachment 398821 [details] [diff] [review] Patch v3.2 I didn't think enough whether the default prefs values are useful or not. If you have better suggestions, I'll use them in next patch. test builds: https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug513817-5.1/
Attachment #398821 -
Flags: review?(faaborg)
Attachment #398821 -
Flags: review?(dao)
Comment 14•15 years ago
|
||
Comment on attachment 398821 [details] [diff] [review] Patch v3.2 there's nothing for me to review here...
Attachment #398821 -
Flags: review?(dao) → review?(Olli.Pettay)
Assignee | ||
Comment 15•15 years ago
|
||
(In reply to comment #14) > (From update of attachment 398821 [details] [diff] [review]) > there's nothing for me to review here... Ah, I didn't want the actual code review, I wanted the behavior review of the patch.
Comment 16•15 years ago
|
||
Limiting the multiplied speed at 8 seems like a stopgap solution. But before we go into details, I'd like to understand the motivation for (system pref)*2. Why is 4 the right choice if the system pref is 2? Why is 6 the right choice if the system pref is 3? Why is 8 the right choice the system pref is 4?
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16) > Limiting the multiplied speed at 8 seems like a stopgap solution. But before we > go into details, I'd like to understand the motivation for (system pref)*2. Why > is 4 the right choice if the system pref is 2? Why is 6 the right choice if the > system pref is 3? Why is 8 the right choice the system pref is 4? I tested with Microsoft Explorer Mouse and with Alps Touchpad of VAIO, both on Vista. The mouse wheel events come with the system setting value. However, if the system is delayed (or by very fast operation?), the mouse wheel events often come with larger value. So, in most cases, the scrolling speed is multiplied but some times, it's not so. However, I don't think this is a problem because I cannot know the non-multiplied scrolling (i.e., slow scrolling), actually. # If the reason is the system delayed, the non-multiplied scrolling can suppress the unexpected high-speed scrolling. So, I think that to multiply the system settings is good idea for me, if we want to provide faster scroll settings. Note that I'm not sure the 2 is best value. 1.5 might be better. But we cannot know the best value without the feedback of the testers. The reason of 2 in my patch is that Alex suggested the value in this report.
Comment 18•15 years ago
|
||
(In reply to comment #17) > The mouse wheel events come with the system setting value. However, if the > system is delayed (or by very fast operation?), the mouse wheel events often > come with larger value. So, in most cases, the scrolling speed is multiplied > but some times, it's not so. However, I don't think this is a problem because I > cannot know the non-multiplied scrolling (i.e., slow scrolling), actually. > # If the reason is the system delayed, the non-multiplied scrolling can > suppress the unexpected high-speed scrolling. I don't understand this paragraph. :( If somebody does, please paraphrase it for me...
Assignee | ||
Comment 19•15 years ago
|
||
Sorry. I meant that the delta value of the mouse wheel event isn't always the system setting value on Windows. Sometimes, the value is lager than the system setting value. I guess that the cause of the most cases is the system is busy by other works, e.g., HDD swapping, processing a loop in the process (or another process) and painting a window. At this time, user might turn extra the mouse wheel. Then, the extra operation might cause the large delta mouse wheel event. Fortunately, my patch doesn't accelerate the delta value if the delta value is larger than the limitation. This can suppress the unexpected (extra) scrolling. For example, when the events comes with 3, 3, 6. Then, the computed delta value in my patch is 6, 6, 8. This is better than 6, 6, 12 because the user might turn extra the mouse wheel by non-reaction by the system busy.
Comment 20•15 years ago
|
||
OK, this is an argument for using a limit, but I'd like to understand the motivation for (system pref)*2 in the first place.
Assignee | ||
Comment 21•15 years ago
|
||
(In reply to comment #20) > OK, this is an argument for using a limit, but I'd like to understand the > motivation for (system pref)*2 in the first place. I guess that the stuff think that the most system's default scrolling speed is 3 lines on Windows. (In fact, MS's Intelli Point's default value and Alps's touchpad's default value are so. I'm not sure the Logitec's default value.) It seems that the Fx UI team decided that the preferred value for Fx is 6 in bug 462809. But the bug's patch ignored the system prefs, but of course we shouldn't do so. Therefore, they want to use '2', I guess.
Comment 22•15 years ago
|
||
In that case, syspref*2 doesn't make sense. What would make sense is syspref == 3 ? 6 : syspref. Unfortunately, this will still ignore users who were happy with 3 (like me). 3 with acceleration allows me to scroll both accurately and fast, so I'd prefer that over 6 without acceleration.
Assignee | ||
Comment 23•15 years ago
|
||
(In reply to comment #22) > 3 with acceleration allows me to scroll both accurately and > fast, so I'd prefer that over 6 without acceleration. I agree too. We shouldn't ignore the system settings. I think that the backing out the all related patches are best for accessibility and usability. But if Fx UI team still thinks that they want to change the scrolling speed, my patch is a good point for both Fx UI team and the users. I believe that the platform team honors the system setting and the system behavior on each platforms. But Fx UI team didn't think so and they changed the platform code to so.
Assignee | ||
Comment 24•15 years ago
|
||
And note that if we speed up the scrolling speed, we need more some works. E.g., it scrolls over one page in small scrollable box, e.g., a scrollable box height may be 4 lines, so, if we scrolls this box 6 lines by one action, the user cannot look the 5th and 6th lines. And also it accelerate the scrolling speed in all widgets, e.g., textarea, listbox and treeview. But they should be scrolled by the system settings. because such non-system-standard behavior could break other XUL application's behavior.
Comment 25•15 years ago
|
||
Whoever reviewed bug 512235 should probably review this too.
Comment 26•15 years ago
|
||
Comment on attachment 398821 [details] [diff] [review] Patch v3.2 >@@ -2389,16 +2433,17 @@ nsresult > nsEventStateManager::DoScrollText(nsPresContext* aPresContext, > nsIFrame* aTargetFrame, > nsMouseScrollEvent* aMouseEvent, > ScrollQuantity aScrollQuantity) > { > nsIScrollableView* scrollView = nsnull; > nsIFrame* scrollFrame = aTargetFrame; > PRInt32 numLines = aMouseEvent->delta; >+ PRBool isSystemSpeed = aMouseEvent->deltaIsSystemValue; You could add a new parameter to DoScrollText, then there was no need for deltaIsSystemValue. PostHandleEvent knows the same thing what deltaIsSystemValue is. >@@ -2515,17 +2561,21 @@ nsEventStateManager::DoScrollText(nsPres > scrollView->ScrollByPages(scrollX, scrollY, > (noDefer ? NS_VMREFRESH_IMMEDIATE : NS_VMREFRESH_SMOOTHSCROLL)); > } > else if (aScrollQuantity == eScrollByPixel) { > scrollView->ScrollByPixels(scrollX, scrollY, overflowX, overflowY, > (noDefer ? NS_VMREFRESH_IMMEDIATE : NS_VMREFRESH_DEFERRED)); > } > else { >- nsMouseWheelTransaction::AccelerateWheelDelta(scrollX, scrollY); >+ // If the event is trusted event, we should accelerate the speed. >+ if (NS_IS_TRUSTED_EVENT(aMouseEvent)) { How could an untrusted event reach ::DoScrollText? No need for NS_IS_TRUSTED_EVENT. >+// max value of the multiplied system scroll speed >+pref("mousewheel.sysscrollspeed.vertical.max", 8); >+pref("mousewheel.sysscrollspeed.horizontal.max", 4); >+ I have no idea why these values. I think vertical and horizontal scrolling should be always as fast. > nsMouseScrollEvent(PRBool isTrusted, PRUint32 msg, nsIWidget *w) > : nsMouseEvent_base(isTrusted, msg, w, NS_MOUSE_SCROLL_EVENT), >- scrollFlags(0), delta(0), scrollOverflow(0) >+ scrollFlags(0), delta(0), scrollOverflow(0), deltaIsSystemValue(PR_TRUE) > { > } > > PRInt32 scrollFlags; > PRInt32 delta; > PRInt32 scrollOverflow; >+ >+ PRPackedBool deltaIsSystemValue; This looks a bit like a hack, and as far as I see, there is no need for it. For the usability etc. faaborg should review.
Attachment #398821 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 27•15 years ago
|
||
Thank you, Olli. I agree, the patch should be reviewed by roc because the mouse wheel related code was always reviewed by him. But I need the faaborg's review, first.
Attachment #398821 -
Attachment is obsolete: true
Attachment #399077 -
Flags: review?(faaborg)
Attachment #398821 -
Flags: review?(faaborg)
Assignee | ||
Comment 28•15 years ago
|
||
oops, I posted a wrong patch, sorry for the spam.
Attachment #399077 -
Attachment is obsolete: true
Attachment #399078 -
Flags: review?(faaborg)
Attachment #399077 -
Flags: review?(faaborg)
Reporter | ||
Comment 29•15 years ago
|
||
>OK, this is an argument for using a limit, but I'd like to understand the
>motivation for (system pref)*2 in the first place.
We are trying to balance the following things:
1) Appear as fast as chrome
2) Give the user some level of control over the settings
3) Avoid adding too many prefs to Firefox
the *2 hack achieves all three, albeit with the second goal being kind of obfuscated (less is still less, and more is more, but they don't have direct control any more).
If you can find a clean solution that gives us all three goals, then I think we should go for that instead.
Other approaches could include set our scrolling to 6 lines if the user has the default value of 3, and otherwise just honoring exactly what they specified in the OS prefs. However this seemed even less deterministic than the *2 hack.
Reporter | ||
Comment 30•15 years ago
|
||
>[Removing "on windows" from summary, since scroll-acceleration affects all
>platforms now, after bug 512235.]
what effect will this patch have on Linux, OS X, and other platforms?
Comment 31•15 years ago
|
||
(In reply to comment #29) > We are trying to balance the following things: > > 1) Appear as fast as chrome Can you please "allow the user to scroll in small steps" to that list? Scrolling slowly while reading a text seems like a common enough use case which apparently is supported on OS X. That seems to have been forgotten in bug 462809.
Reporter | ||
Comment 32•15 years ago
|
||
If they set the scroll amount to 1, we will set it to 2. Do you think 2 is small enough, or should we forget about doubling and just have 3 as the "special case we forget about your setting" option? I'm actually indifferent between the two approaches.
Comment 33•15 years ago
|
||
2 would be one third slower than 3, which is a slowdown I wouldn't want. This would be even worse for other apps that don't double the pref. I'm looking for a balance between controlled slow scrolling and fast scrolling, but it seems that only acceleration could solve that completely.
Assignee | ||
Comment 34•15 years ago
|
||
(In reply to comment #32) > If they set the scroll amount to 1, we will set it to 2. Do you think 2 is > small enough, or should we forget about doubling and just have 3 as the > "special case we forget about your setting" option? I'm actually indifferent > between the two approaches. It doesn't make sense that the users should change the system scrolling settings from 3 to 2 for Firefox scrolling.
Comment 35•15 years ago
|
||
(In reply to comment #34) > It doesn't make sense that the users should change the system scrolling > settings from 3 to 2 for Firefox scrolling. It's worse: from 3 to 1 in order to get 2 in Firefox.
Reporter | ||
Comment 36•15 years ago
|
||
>It's worse: from 3 to 1 in order to get 2 in Firefox.
yeah, so here are the two options
OS - Firefox
---------------
1 - 2
2 - 4
3 - 6
4 - 8
----------------
1 - 1
2 - 2
3 - 6
4 - 4
Perhaps the later option is more understandable (although really both of them are pretty confusing).
Comment 37•15 years ago
|
||
Can we do 3 - 4 rather than 3 - 6? That would be less surprising and take the slow-scrolling use case better into account.
Assignee | ||
Comment 38•15 years ago
|
||
Why only 3 is accelerated? There might be non-3 mouse drivers. # And also, on Linux and on Mac, what is the default value?
Assignee | ||
Comment 39•15 years ago
|
||
(In reply to comment #37) > Can we do 3 - 4 rather than 3 - 6? That would be less surprising and take the > slow-scrolling use case better into account. How about x1.3? I.e., NS_round(3.0 * 1.3) == 4.0.
Comment 40•15 years ago
|
||
(In reply to comment #38) > Why only 3 is accelerated? Because a user who changed the system pref doesn't want a different value.
Assignee | ||
Comment 41•15 years ago
|
||
(In reply to comment #40) > (In reply to comment #38) > > Why only 3 is accelerated? > > Because a user who changed the system pref doesn't want a different value. Other values can be default setting, too. The mouse driver can change the value for its characteristics.
Comment 42•15 years ago
|
||
Right. I don't think we want to blindly overrule these mouse drivers.
Reporter | ||
Comment 43•15 years ago
|
||
The vast majority of users won't know about the existence of the system pref, and matching the speed of Chrome is the highest priority goal. If the user really wants to dial something in specific (like 2, or 4) perhaps we should just give it to them and stop messing around with their settings. I'm assuming that 3 is the 99% case, but do we have any evidence that this isn't true?
Comment 44•15 years ago
|
||
I would recommend dropping the "match Chrome" argument and replace it with "support fast scrolling". The latter needs to be able to stand on its own against "support slow scrolling". It's not clear to me that Chrome gets this right.
> I'm assuming that 3 is the 99% case, but do we have any evidence that this
> isn't true?
This isn't quite the right question. I have 3 and knew about the system preference.
Assignee | ||
Comment 45•15 years ago
|
||
(In reply to comment #43) > The vast majority of users won't know about the existence of the system pref, > and matching the speed of Chrome is the highest priority goal. Yeah, so, I suggest that: * We should multiply the system scrolling speed in default settings (it is implemented in my patch). * We should provide an UI in the option dialog for disabling the multiplying for the minority of users.
Comment 46•15 years ago
|
||
(In reply to comment #45) > (In reply to comment #43) > > The vast majority of users won't know about the existence of the system pref, > > and matching the speed of Chrome is the highest priority goal. > > Yeah, so, I suggest that: > * We should multiply the system scrolling speed in default settings (it is > implemented in my patch). Wait. Even if only a minority is affected, this isn't an argument for multiplying all settings instead of only 3.
Reporter | ||
Comment 47•15 years ago
|
||
> 1) Appear as fast as chrome
> 2) Give the user some level of control over the settings
> 3) Avoid adding too many prefs to Firefox
Achieving all three of these is literally impossible as far as I can tell, but I recommend that we go with only doubling on the specific setting of 3, and only on Windows (where 3 is set by the default mouse driver, which is the most common case).
Reporter | ||
Updated•15 years ago
|
Attachment #399078 -
Flags: review?(faaborg)
Reporter | ||
Comment 48•15 years ago
|
||
Comment on attachment 399078 [details] [diff] [review] Patch v3.3 Turning the review flag off based on the previous comment (only double on 3, only on windows). Also note that I can only provide a ui-review.
Reporter | ||
Updated•15 years ago
|
Summary: Switch scrolling to (system pref)*2 for 3.6 → Switch scrolling to 6 lines in the default case for 3.6
Reporter | ||
Updated•15 years ago
|
OS: All → Windows XP
Summary: Switch scrolling to 6 lines in the default case for 3.6 → Switch scrolling to 6 lines in the default case for 3.6 on windows
Assignee | ||
Comment 49•15 years ago
|
||
(In reply to comment #47) > and only on Windows Why? I guess Mac has speedy scrolling settings (or acceleration), so, it seems ok. However, how about Linux?
OS: Windows XP → All
Reporter | ||
Comment 50•15 years ago
|
||
For linux we don't know for sure that 3 is the 99% case, so the change could be strange an arbitrary. Ideally we will get acceleration working well for Windows and Linux in the future (OS X already has a great model). In the meantime this bug is literally a hack.
Assignee | ||
Comment 51•15 years ago
|
||
Hrm, I installed Microsoft's IntelliPoint (for Explorer Mouse) to clean Windows environment. Then, the default value is Horizontal: 3, Vertical: 4 in registry settings. (Their default values on Windows are 3 and 3.) Then, the mouse driver sent 1 (or sometimes 2) in the computed value by nsWindow at vertical scrolling. So, the "only 3" approach is not be enabled with Microsoft mouse... Is it minority?
Comment 52•15 years ago
|
||
Doesn't IntelliPoint support acceleration? Modifying the values we get from these drivers doesn't make sense anyway.
Assignee | ||
Comment 53•15 years ago
|
||
(In reply to comment #52) > Doesn't IntelliPoint support acceleration? Modifying the values we get from > these drivers doesn't make sense anyway. Ah, IntelliPoint support it and enables it in default settings. However, even if I disable the acceleration (I usually disables it), the mouse wheel events are sent more frequent. So, the each mouse wheel events has small delta, but the frequency helps the scrolling speed. O.K. I'll post a patch.
Assignee | ||
Comment 54•15 years ago
|
||
This has a bool pref which is "mousewheel.system_scroll_override". And also the mapping for the "system value" to the actual "scrolling value" is defined by prefs: > pref("mousewheel.system_scroll_override.vertical.(system value)", (scrolling value); > pref("mousewheel.system_scroll_override.horizontal.(system value)", (scrolling value); Additionally, this patch fixes the problems of comment 24. I'm going to test this patch on tryserver, and I'm going to clean-up the code before review. # Unfortunately, I'm going to offline tomorrow.
Attachment #399078 -
Attachment is obsolete: true
Assignee | ||
Comment 55•15 years ago
|
||
test builds: https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug513817-6/
Assignee | ||
Comment 56•15 years ago
|
||
(In reply to comment #53) > (In reply to comment #52) > > Doesn't IntelliPoint support acceleration? Modifying the values we get from > > these drivers doesn't make sense anyway. > > Ah, IntelliPoint support it and enables it in default settings. Oh, wait, but if the users changed the system settings to the faster value and the mouse driver sends the delta '3' by high frequency, then, we scroll very fast. So, we should also check the system setting directly, shouldn't check only the delta value. I'll fix this issue in next patch.
Assignee | ||
Comment 57•15 years ago
|
||
Looks our gtk2 code doesn't use system settings for the wheel delta value. http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#3238 We always set 3 at vertical, and set 1 at horizontal. So, we *can* change the values. # Looks like Linux doesn't support the system wide settings for the wheel. http://www.gtk.org/api/2.6/gtk/GtkSettings.html
Assignee | ||
Comment 58•15 years ago
|
||
(In reply to comment #57) > http://www.gtk.org/api/2.6/gtk/GtkSettings.html oops, this is latest stable. http://library.gnome.org/devel/gtk/stable/GtkSettings.html
Assignee | ||
Comment 59•15 years ago
|
||
Attachment #399546 -
Attachment is obsolete: true
Assignee | ||
Comment 60•15 years ago
|
||
O.K. Faaborg, I'd like you to like this approach. When "mousewheel.system_scroll_override_on_contents" is TRUE and the scrolling target is NOT a control (like textarea, listbox and treeview), this patch tries to override the system scrolling speed. The reason of that I hate like to override the system setting on the controls is that the scrolling speed on controls isn't important for the user feeling, however, they are really system widget emulation. So, the scrolling speed should be same as the system native widget's speed. This patch checks whether the BOTH wheel speed settings (i.e., vertical and horizontal) are customized or not. If one of them or both of them are customized, this patch doesn't override the system scrolling speed. On Vista, the delta value isn't same as the system settings because Vista sends the wheel events immediately. (Windows of before Vista, they wait to send the wheel events until that the user turns enough amount for the settings.) Therefore, the delta value doesn't same as the system setting values. So, we still need to multiply the delta value instead of replacing it. The factors can be customized by "mousewheel.system_scroll_override_on_contents.vertical.factor" and "mousewheel.system_scroll_override_on_contents.horizontal.factor". If (the delta value * factor) is larger than (the system setting value * factor), the delta value is replaced by the latter value instead of the former. By this, we can suppress the unexpected high speed scrolling, e.g., the delta value might be accelerated by the mouse driver. And also if the computed delta value is larger than the scrolling page size, this patch scrolls just one page at that time. On Linux the user can use the override. But I tested on Linux actually on VMware, the wheel events are sent higher frequency than the Windows. Therefore, if we enable the override, the scrolling is too fast. So, this patch disables it on Linux. The test builds are coming soon.
Attachment #400248 -
Attachment is obsolete: true
Attachment #400254 -
Flags: ui-review?(faaborg)
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 61•15 years ago
|
||
test builds for the latest patch: https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug513817-10/
Assignee | ||
Comment 62•15 years ago
|
||
Alex Faaborg: Would you review quickly if we land this before 3.6b1? Unfortunately, I might be offline during from Friday to Sunday in JST.
Assignee | ||
Comment 63•15 years ago
|
||
Nominating b1 blocker. -> P1 -> target to 3.6b1 Bug 462809 changed the DOM wheel event behavior on Windows. And bug 512235 fixed it (the current trunk behavior is same as Fx3.5 and former). But bug 512235 isn't landed to 3.6 branch yet because it doesn't have the approval and it makes a regression which is the high speed acceleration on all platforms. But this patch fixes the high speed scrolling issue. So, both this patch and bug 512235's patch are needed. Even if we cannot land this patch, we must land bug 512235 and need to disable the acceleration setting on the branch at least.
Priority: -- → P1
Target Milestone: --- → Firefox 3.6b1
Reporter | ||
Comment 64•15 years ago
|
||
Comment on attachment 400254 [details] [diff] [review] Patch v6.0 sorry for the delay in getting the review, I only had time to test on Vista, but I think we should get this landed for broader testing and feedback. When I tested it things worked great.
Attachment #400254 -
Flags: ui-review?(faaborg) → ui-review+
Assignee | ||
Updated•15 years ago
|
Attachment #400254 -
Flags: review?(roc)
Assignee | ||
Comment 65•15 years ago
|
||
Comment on attachment 400254 [details] [diff] [review] Patch v6.0 Thanks, roc, would you review this? The details of this patch is written is comment 60.
Assignee | ||
Comment 66•15 years ago
|
||
fix a nit. I used aIsVertical in some places but the current ESM uses aIsHorizontal. I replaced my aIsVertical to aIsHorizontal for the consistency. Note that I added some values to the nsMetricID in nsILookAndFeel. However, I didn't change the uuid of it because some other checkins didn't change too and my values are appended to its tail. So, my patch didn't change any other values.
Attachment #400254 -
Attachment is obsolete: true
Attachment #401172 -
Flags: review?(roc)
Attachment #400254 -
Flags: review?(roc)
+ // Override the system scrolling speed if the user didn't customoze the customize Instead of IsControlFrame, would it be simpler and more general to just check if the frame to be scrolled is the root scroll frame for its document? Do we want overflow:auto elements to be accelerated or not? Would it make sense for IsScrollSpeedOverrideAllowed to be a theme or widget API? It seems to me that this is really quite platform-specific: we only need or want this on Windows.
Assignee | ||
Comment 68•15 years ago
|
||
(In reply to comment #67) > Instead of IsControlFrame, would it be simpler and more general to just check > if the frame to be scrolled is the root scroll frame for its document? Do we > want overflow:auto elements to be accelerated or not? How about this case? A web page has only one scrollable content which is not a root element. I.e., <html> <body> <div id="sidebar" style="float:left;width: 100px;">the contents of sidebar</div> <div id="contents" style="margin-left:120px;overflow:auto;"> There are very long contents which needs scrollbars. </div> </body> </html> > Would it make sense for IsScrollSpeedOverrideAllowed to be a theme or widget > API? O.K., I'll move it to nsIWidget. > It seems to me that this is really quite platform-specific: we only need > or want this on Windows. On tire1 platforms, it's correct.
Assignee | ||
Comment 69•15 years ago
|
||
> <div id="contents" style="margin-left:120px;overflow:auto;">
<div id="contents" style="margin-left:120px;overflow:auto;height:80%;">
^^^^^^^^^^^
It's true that there are pages like that, but they're pretty rare. On the other hand, there are pages that use overflow:auto elements to emulate listboxes.
Assignee | ||
Comment 71•15 years ago
|
||
ok, I'll post a new patch after testing. I added an API to nsIWidget in the new patch. NS_IMETHOD GetMouseWheelScrollingSpeed(PRBool aIsHorizontal, PRInt32 &aScrollLines, PRBool &aIsSystemDefault) = 0;
Comment 72•15 years ago
|
||
Whatever we do, we should be careful to not cause similar problems what Chrome has. If you have <select> which size is >1 but <6 and you scroll it using mouse wheel on chrome, you easily skip several items. We do have the same problem when the size is 2, but that is a bug to fix IMO, not a feature.
Assignee | ||
Comment 73•15 years ago
|
||
Olli: Yeah, you're right. Therefore I added RoundToOnePageScroll and checked the page size at the final step of the acceleration.
Assignee | ||
Comment 74•15 years ago
|
||
Attachment #401172 -
Attachment is obsolete: true
Attachment #402099 -
Flags: review?(roc)
Attachment #401172 -
Flags: review?(roc)
+ // XXX hack for Windows. Before Vista, Windows doesn't support the + // horizontal scrolling by mouse wheel on the system level. Vista and + // later use same values for both vertical and horizontal scrolling + // in the default settings. Seems to me this should be in widget too. Why not move all of nsMouseWheelTransaction::OverrideSystemScrollSpeed (except for the frame type check) into an nsIWidget API? E.g. call it PRInt32 nsIWidget::GetAccelerateMouseScrollAmount(PRInt32 aScrollLines, PRBool aIsHorizontal) ? Or maybe it would be even better to have nsMouseScrollEvent have two delta values, one for "normal" and one for "accelerated", and ESM picks which one to use based on the frame type? I think RoundToOnePageScroll should be named LimitToOnePageScroll.
Assignee | ||
Comment 76•15 years ago
|
||
ok, I added nsIWidget::OverrideSystemMouseScrollSpeed. And nsBaseWidget only set aOriginalDelta * aFactor to the result. nsWindow of Windows only override this method for the system settings checking. Therefore, non-Windows can override always the scrolling speed if "mousewheel.system_scroll_override_on_root_content.enabled" is true.
Attachment #402099 -
Attachment is obsolete: true
Attachment #402288 -
Flags: review?(roc)
Attachment #402099 -
Flags: review?(roc)
+ * @param aOverriddenDelta The overridden mosue scrolling speed. This value "mouse" + double aFactor, Why do we need this as a parameter? This method should be responsible for computing it. In other words: + if (!nsContentUtils::GetBoolPref( + "mousewheel.system_scroll_override_on_root_content.enabled", + PR_FALSE)) { + return aScrollLines; + } + + nsCAutoString prefName("mousewheel.system_scroll_override_on_root_content."); + if (aIsHorizontal) { + prefName.AppendLiteral("horizontal."); + } else { + prefName.AppendLiteral("vertical."); + } + prefName.AppendLiteral("factor"); + double factor = (double)nsContentUtils::GetIntPref(prefName.get(), 0) / 100; + // The pref value must be larger than 100, otherwise, we don't override the + // delta value. + if (factor <= 1.0) { + return aScrollLines; + } + This code can all live in OverrideSystemMouseScrollSpeed.
Assignee | ||
Comment 78•15 years ago
|
||
trying to test on tryserver...
Attachment #402288 -
Attachment is obsolete: true
Attachment #402288 -
Flags: review?(roc)
Assignee | ||
Comment 79•15 years ago
|
||
fix a comment bug.
Attachment #402410 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #402414 -
Flags: review?(roc)
+ PRBool aIsSystemScrollSpeed) I think this should be called aAllowScrollSpeedOverride. I think nsBaseWidget::OverrideSystemMouseScrollSpeed should only be in nsWindow.cpp for Windows. I don't think other platforms need or want this code. Other than that, looks good.
Assignee | ||
Comment 81•15 years ago
|
||
(In reply to comment #80) > I think nsBaseWidget::OverrideSystemMouseScrollSpeed should only be in > nsWindow.cpp for Windows. I don't think other platforms need or want this code. Roc, I don't think so. If I use a BlueTooth mouse of MS on Mac and install IntelliPoint for Mac, I look same scrolling speed as Windows' on Mac too. And on Linux, if there are some mouse which fires the wheel events slower than MS mouse (The MS mouse wheels are turned smoothly. And it sends very many wheel events, but IntelliPoint reduce the delta values of each events on Windows), the scrolling speed is same as Windows' default. We cannot detect these cases now. However, if we can detect them, it could be great (if the Windows' users are glad this patch). So, some users might want to enable the setting on other platforms.
I don't think we should expect *users* to modify any preferences, especially ones as complicated as these. We wouldn't add features that were only accessible via about:config. However, here it's not a big deal, I guess, since the code needs to exist for Windows anyway. So I'll let it go.
Comment on attachment 402414 [details] [diff] [review] Patch v9.0.1 Just change that parameter name to aAllowScrollSpeedOverride
Attachment #402414 -
Flags: review?(roc) → review+
Assignee | ||
Comment 84•15 years ago
|
||
thanks. I'll land this patch several hours later.
Attachment #402414 -
Attachment is obsolete: true
Assignee | ||
Comment 85•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8013abd85baa landed to trunk, I'll land it to 1.9.2 branch if there is no problem.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 86•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aa0cb2aff229
status1.9.2:
--- → beta1-fixed
Assignee | ||
Comment 87•15 years ago
|
||
Assignee | ||
Comment 88•15 years ago
|
||
Attachment #402763 -
Attachment is obsolete: true
Comment 89•15 years ago
|
||
Just so you guys know, if the goal was "match Chrome behavior", doubling the system scroll lines value isn't Chrome's behavior at all. (I wrote Chrome's behavior.) Take a look at http://trac.webkit.org/browser/trunk/WebCore/platform/win/WheelEventWin.cpp for a nicely-commented block about scroll rates if you want to know what we do (on Windows). In short, we use 100 px per "three lines", and the system ticks->lines mapping (which defaults to three lines per tick, so by default we do 100 px per tick).
Assignee | ||
Comment 90•15 years ago
|
||
(In reply to comment #89) > Just so you guys know, if the goal was "match Chrome behavior", doubling the > system scroll lines value isn't Chrome's behavior at all. (I wrote Chrome's > behavior.) No, the goal was "provides faster scroll (like WebKit) to the users who don't customize the scroll speed". But thank you for the information.
Reporter | ||
Comment 91•15 years ago
|
||
Hey Peter, it seems like I've been getting higher values than 100px in Chrome instances, but obviously there are a lot of variables in play here, including the mouse driver and the way the physical mouse hardware reports events. I have a stack of mice under my desk but haven't had a chance to open them yet.
>Just so you guys know, if the goal was "match Chrome behavior", doubling the
>system scroll lines value isn't Chrome's behavior at all.
I was in fact worried that a larger scroll distance was making Chrome feel faster, although a more generic goal could be phrased as a "small perceptual trick to make Firefox more responsive."
We are still collecting feedback on the Firefox betas to see if user's like the change, and I should also get to the bottom of why my mouse events are doubling in Chrome if that wasn't the intended behavior (either way, it does feel more responsive).
You need to log in
before you can comment on or make changes to this bug.
Description
•