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)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3.6b1
Tracking Flags:
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)

Patch v9.1
38.01 KB, patch
Details | Diff | Splinter Review
Patch v9.1 for 1.9.2 branch
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.
Flags: blocking-firefox3.6?
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+
Please wait bug 512235. This will be fixed easily after the patch.
Depends on: 512235
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|.
Attached patch Patch v1.0 (obsolete) — — Splinter Review 
checking on tryserver...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attached patch Patch v2.0 (obsolete) — — Splinter Review 
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)
Blocks: 462809
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.
(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.
I was referring to the system setting when I said "if someone had set that pref to 6".
Attached patch Patch v3.0 (obsolete) — — Splinter Review 
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)
Attached patch Patch v3.1 (obsolete) — — Splinter Review 
fix a nit.
Attachment #398695 - Attachment is obsolete: true
Blocks: 514783
[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
Attached patch Patch v3.2 (obsolete) — — Splinter Review 
fix some nits.

I think that we shouldn't accelerate when the wheel event isn't trusted event.
Attachment #398704 - Attachment is obsolete: true
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 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)
(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.
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?
(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.
(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...
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.
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.
(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.
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.
(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.
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.
Whoever reviewed bug 512235 should probably review this too.
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-
Attached patch Patch v3.3 (obsolete) — — Splinter Review 
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)
Attached patch Patch v3.3 (obsolete) — — Splinter Review 
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)
>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.
>[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?
(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.
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.
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.
(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.
(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.
>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).
Can we do 3 - 4 rather than 3 - 6? That would be less surprising and take the slow-scrolling use case better into account.
Why only 3 is accelerated? There might be non-3 mouse drivers.

# And also, on Linux and on Mac, what is the default value?
(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.
(In reply to comment #38)
> Why only 3 is accelerated?

Because a user who changed the system pref doesn't want a different value.
(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.
Right. I don't think we want to blindly overrule these mouse drivers.
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?
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.
(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.
(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.
> 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).
Attachment #399078 - Flags: review?(faaborg)
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.
Summary: Switch scrolling to (system pref)*2 for 3.6 → Switch scrolling to 6 lines in the default case for 3.6
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
(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
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.
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?
Doesn't IntelliPoint support acceleration? Modifying the values we get from these drivers doesn't make sense anyway.
(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.
Attached patch Patch v4.0 (obsolete) — — Splinter Review 
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
(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.
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
Attached patch Patch v5.0 (obsolete) — — Splinter Review 

        Attachment #399546 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v6.0 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
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+

        Attachment #400254 -
        Flags: review?(roc)

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v6.1 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
    

    
  
(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.

    
    

    
  
> <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.

    
    

    
  
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;

    
    

    
  
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.

    
    

    
  
Olli:

Yeah, you're right. Therefore I added RoundToOnePageScroll and checked the page size at the final step of the acceleration.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v7.0 (obsolete)
        — 
          — Splinter Review
      

    


  

        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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v8.0 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v9.0 (obsolete)
        — 
          — Splinter Review
      

    


  
trying to test on tryserver...

        Attachment #402288 -
        Attachment is obsolete: true

        Attachment #402288 -
        Flags: review?(roc)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v9.0.1 (obsolete)
        — 
          — Splinter Review
      

    


  
fix a comment bug.

        Attachment #402410 -
        Attachment is obsolete: true

        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.

    
    

    
  
(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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v9.1
        — 
          — Splinter Review
      

    


  
thanks. I'll land this patch several hours later.

        Attachment #402414 -
        Attachment is obsolete: true

    
    

    
  
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

    
  
Depends on: 518745

    
    

    
  
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).

    
    

    
  
(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.

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: