Bug 17589 – Scroll wheel sensitivity ignored

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Matt Lilek]

On Vista, no matter how many lines I tell the machine to scroll at a time with my scroll wheel, it still scrolls the same amount (which is painfully slow).

[Adam Roben (:aroben)]

<rdar://problem/5770893>

[Marcus]

Created attachment 20747 [details]
patch attempt

[Adam Roben (:aroben)]

Comment on attachment 20747 [details]
patch attempt

Thanks for the patch! This is a good step in the right direction.

This patch won't fix the issue for overflow areas (e.g., <div style="overflow: scroll">). Overflow areas use RenderLayer::scroll to do their scrolling. We'll want to fix both at the same time, and hopefully in a way that avoids duplicating the code you've added.

+        if (!::SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0, &linesToScroll, 0))

On Vista there's also an SPI_GETWHEELSCROLLCHARS value that we should support for horizontal scrolling. Since this value isn't present in pre-Vista headers, you'll have to declare it at the top of this file, like this:

static const UINT SPI_GETWHEELSCROLLCHARS = <the value that Vista uses>;

+            linesToScroll = 3;  // Windows default value 

This should be placed into a named constant at the top of the file, like this:

static const unsigned defaultLinesToScroll = 3;

+            // Like Mozilla, use default font to determine the number of pixels per line.
+            const FrameView* frameView = static_cast<const FrameView*>(this);
+            const Frame* frame = frameView->frame();
+            int lineStep;
+            if (frame && frame->contentRenderer() && frame->contentRenderer()->style())
+                lineStep = linesToScroll * frame->contentRenderer()->style()->font().lineSpacing();
+            else
+                lineStep = linesToScroll * LINE_STEP / 2; // fallback value
+
+            scrollBy(-e.deltaX() * lineStep, -e.deltaY() * lineStep);

This is a layering violation. Classes in platform/ should not in general be aware of classes outside of platform/, so it seems unfortunate to be introducing RenderView usage here. We'd also want this change to be made on all platforms, not just Windows, I believe. This change seems logically distinct from supporting the system preferences for scrolling amount, so this should be turned into a separate patch.

[Adam Roben (:aroben)]

This affects XP as well.

[Marcus]

(In reply to comment #3)
Thanks for the review! You guys are great :)

[Maxime Britto]

Created attachment 22015 [details]
Patch attempt

I already explained some things in the changelog.
Also about the Screen Scroll option in windows, I hesitated to use a boolean instead of the value 150. I didn't know if it was better to simplify the reading or to limit the number of attributes/variables.
I've finally chosen to keep only one variable and to test its value so see if it's a LineScroll or a ScreenScroll.

[Darin Adler]

Comment on attachment 22015 [details]
Patch attempt

Looks good!

Is "sensitivity" really the right name for this? The Windows APIs don't seem to call it that. Is that the end-user name for this feature? Where does the term come from.

The magic number 150 appears multiple times in this patch. It needs to be a named constant if we're going to keep it.

But I don't think that using a magic number is a clean way to communicate that we're in page scroll mode -- I suggest a separate boolean for that.

 1364             //if (deltaX && node->renderer()->scroll(deltaX < 0 ? ScrollRight : ScrollLeft, e.isContinuous() ? ScrollByPixel : ScrollByLine,
 1365             //                                           deltaX < 0 ? -deltaX : deltaX))
 1366             //    e.accept();

We don't want to check in commented-out code.

 1368                 int pixelsToScroll = abs(deltaX);
 1369                 if (!e.isContinuous() && abs(deltaX) != 150) {

You're repeating abs(deltaX) a number of times here. Can you use pixelsToScroll instead?

You say "sensibility" in a few places where you mean "sensitivity".

 74     //TODO: retrieve the user setting for the number of lines to scroll on each wheel event

We don't normally use "TODO:" -- consider FIXME instead.

 39     , m_charScrollSensitivity(1) //Values needed on other platforms to let the user to choose the numer of lines to scroll on each wheel event

I don't think this comment is helpful. You should just leave it out.

 68         if(! ::SystemParametersInfo(SPI_GETWHEELSCROLLCHARS, 0, &scrollChars, 0))
 69             scrollChars = 1;

There should be a space after the if. There should not be a space after the "(". We don't use the "::" when calling global functions.

 71         //If we are in a page scroll mode we choose an arbitrary high value 
 72         //which can be recongnized easily later when we'll choose the scroll granularity.

There should be spaces after the "//" and before the comments. Typo here: "recongnized" instead of "recognized".

 82         if(! ::SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0, &scrollLines, 0))
 83             scrollLines = 3;

Where does this number 3 come from?

 76         m_charScrollSensitivity = (int) scrollChars;

We normally use static_cast rather than C-style casts.

[Maxime Britto]

Created attachment 22075 [details]
correctd patch

I changed the variable names for them to be more explicit and I added 2 bool for the page scroll (vertical and horizontal)

Here are the chosen names :

+        int m_charsToScrollPerDelta;
+        int m_linesToScrollPerDelta;
+        bool m_pageXScrollMode;
+        bool m_pageYScrollMode;

[Eric Seidel (no email)]

Comment on attachment 22075 [details]
correctd patch

The deltaX clause is incorrect.  Copy/paste bug.  Horizontal scroll events will cause vertical scrolling.

We talked over IRC about possibly abstracting these scroll + accept clauses into some nice static inline function(s).

Even stuff like this I would almost consider abstracting:

if (e.isPageXScrollModeEnabled())
+            deltaX = (deltaX > 0 ? visibleWidth() : -visibleWidth()) / LINE_STEP_WIN;

adjustDeltaForPageScrollMode(deltaX, e.isPageScrollModeEnabled(), visibleWidth());

static inline adjustDeltaForPageScrollMode(int& delta, bool pageScrollModeEnabled, int pageScroll)
{
  if (pageScrollModeEnabled)
          delta = (delta > 0 ? pageScroll) : -pageScroll) / LINE_STEP_WIN;
}

But that is probably not worth it.  Just depends on your copy/paste code tolerance level... mine is extremely low in general. :)

is there any way we can test this with DRT?

We can fake scroll events, no?  The question would be getting the system settings to be the same on all machines...  or maybe DRT could hook in some special way to override the page scroll settings?

r- for the copy/paste error.

[Maxime Britto]

Created attachment 22284 [details]
Corrected patch

Applyed the corrections, added the static inline functions.
For the DRT I couldn't find anything to override this system settings call : ::SystemParametersInfo(SPI_GETWHEELSCROLLCHARS, 0, &scrollChars, 0)

It's Win32 generic system call which retrieves the user setting passed as parameter in the other parameter. This kind of call have to be implemented in the DRT since it's not possible right now in there.

[Eric Seidel (no email)]

Comment on attachment 22284 [details]
Corrected patch

Looks great!

I suggest you make:
if (deltaX)

part of the 
scrollAndAcceptEvent inline
as an early return:

if (!delta)
   return;

Also, there are some spacing issues here:

+static inline void adjustDeltaForPageScrollMode(float& delta, bool pageScrollEnabled, int visibleWidthOrHeight)
+{
+    if (pageScrollEnabled)
+            delta = (delta > 0 ? visibleWidthOrHeight : -visibleWidthOrHeight) / LINE_STEP_WIN;
+}

Land away!

[Timothy Hatcher]

Landed in r35633.

[Timothy Hatcher]

Wrong bug. Reopening.

[Timothy Hatcher]

This was landed.

[Gavin Sherlock]

I suspect this has resulted in bug 20331

[Bhu]

Hi 

Im new here and have suffered with this problemn for ever. Seeing this solution is great news with what apple had done! Nothing!

Only problem is I dont know how to apply this patch. How? Where do I apply this code to fix my safari mouse lag that we all get from the scrol whell...? 

Sorry if its obvious but any links how to apply this code much appredciated.

Thank you

Bhu