Closed Bug 605648 Opened 14 years ago Closed 13 years ago

Support high resolution scrolling on Windows

Categories

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

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6
Tracking Status
blocking2.0 --- .x+

People

(Reporter: faaborg, Assigned: masayuki)

References

()

Details

Attachments

(3 files, 11 obsolete files)

23.20 KB, image/gif
Details
60.62 KB, patch
Details | Diff | Splinter Review
40.50 KB, patch
jimm
: review+
smaug
: review+
Details | Diff | Splinter Review
Windows Vista introduced standard mouse events that contain a higher level of scrolling precision for mouse wheel movement.   

Non discrete mouse wheels have been in the marketplace for awhile now, and mouse drivers are starting to enable sending these events to applications.

Note that this bug is different than bug 590022.  Our smooth scrolling preference simply draws 10 frames when we receive an event that the user has moved their mouse wheel a single discrete step.  This bug would enable users on windows to scroll with pixel level precision (when using a non-discrete wheel), similar to the fine grained level of control that users currently have on OS X.  The two bugs are related in that we would obviously not want to draw 10 frames if the user only moved the scrollable area by a single pixel.

I'll attach a white paper describing the new events to this bug for people to review.
So this is basically: "Implement pixel scrolling on Windows"?
If Windows works reasonable same way as OSX, all the bits to enable
this in dom/layout are there.
And IIRC we support pixel scrolling on Windows when panning using a touchscreen.
Yes, we have pixel scrolling for touch panning, which uses the same support as OSX. Depending on how compatible these new wheel events are this should be straightforward.
Note that bug 590022 will turn on smooth scrolling on by default (drawing 10 frames per scroll event).  This works well with mice with finite notches, but for these events or touch input, we need to make sure there aren't any unintentional interactions.  Have we tried turning on smooth scrolling on a touch enabled windows machine yet?
Requesting blocking for two reasons:

1) users scroll more than they do anything else in our interface, including using the location bar
2) this is a perception of performance issue. Users can feel the literal tactile responsiveness of the product more directly than they can feel the speed of our javascript engine. (that that JS speed isn't super important, it's just not as easy to literally touch).
blocking2.0: --- → ?
(In reply to comment #4)
> Note that bug 590022 will turn on smooth scrolling on by default (drawing 10
> frames per scroll event).  This works well with mice with finite notches, but
> for these events or touch input, we need to make sure there aren't any
> unintentional interactions.  Have we tried turning on smooth scrolling on a
> touch enabled windows machine yet?

That's a good question. I just checked and smooth scrolling doesn't interfere with the touch-enabled pixel scrolling on Windows.
>I just checked and smooth scrolling doesn't interfere
>with the touch-enabled pixel scrolling on Windows.

Ok, hopefully our implementation for these events won't interfere as well, making this bug easier for us to resolve. (As an aside, turning on smooth scrolling on OS X and using an attached mouse seemed to cause significant problems, we'll need to figure out what is going on there).
Does anyone have a link to the relevant Windows API ?

(I'm not taking this, since this is a Windows specific bug, but would be
great to know what kind of API should be used to implement this.)
(In reply to comment #8)
> Does anyone have a link to the relevant Windows API ?

From: http://www.microsoft.com/whdc/device/input/wheel.mspx

"Windows applications that handle window messages for vertical scrolling should not require modification to support smooth scrolling if the application acts on the mouse wheel delta and does not assume a message of 120. However, hardware and applications should be thoroughly tested on the latest release of Windows Vista to verify that they behave correctly. For more information, see "Best Practices for Supporting Microsoft Mouse and Keyboard Devices."

Then it points to the Platform SDK article about mouse input: http://msdn.microsoft.com/en-us/library/ms645533.aspx which specifically defines the WM_MOUSEWHEEL event: http://msdn.microsoft.com/en-us/library/ms645614.aspx

So unless we assume a message of 120, I'm not clear on the action we're meant to take.
nsWindow::OnMouseWheel does some calculations involving WHEEL_DELTA which it says is 120.
(In reply to comment #10)
> nsWindow::OnMouseWheel does some calculations involving WHEEL_DELTA which it
> says is 120.

WHEEL_DELTA is defined by the sdk headers. We don't assume it has the value of 120. We also use SPI_GETWHEELSCROLLLINES & SPI_GETWHEELSCROLLCHARS in our calculations. We use these in calculating a discrete scroll step.
Someone smarter than me should read the document, but I can't figure out what more we need to be doing as an application. It seems to basically say that if a mouse driver supports the new higher definition scrolling, then the appropriate information will be passed to applications.

Alex: did you get any additional details from the Logitech team about what changes need to happen? Can we add one of them to this bug?
(In reply to comment #12)
> Someone smarter than me should read the document, but I can't figure out what
> more we need to be doing as an application. It seems to basically say that if a
> mouse driver supports the new higher definition scrolling, then the appropriate
> information will be passed to applications.
> 
> Alex: did you get any additional details from the Logitech team about what
> changes need to happen? Can we add one of them to this bug?

It sounds like some mice might send an event where zDelta isn't in multiples of WHEEL_DELTA. So for example if one line of scroll is equal to WHEEL_DELTA, a mouse might send smaller incremental values in zDelta providing finer grained (smooth) scrolling.

We currently deal with this by accumulating scroll data up to WHEEL_DELTA, at which point we scroll a line.

Maybe we should be detecting smaller increments and if present, flip to pixel scrolling. I'm not sure. FWIW, my ms laser mouse sends zDelta values of +/-WHEEL_DELTA and I have our internal implementation of smooth scrolling enabled which looks fine.
(In reply to comment #13)
> Maybe we should be detecting smaller increments and if present, flip to pixel
> scrolling. I'm not sure.

Sounds to me like that's the right way to fix this bug. Do you see any problems with that approach?
cc'ing Masatoshi Kimura, he implemented the original delta work we did when Vista came out in bug 347875.

(In reply to comment #14)
> (In reply to comment #13)
> > Maybe we should be detecting smaller increments and if present, flip to pixel
> > scrolling. I'm not sure.
> 
> Sounds to me like that's the right way to fix this bug. Do you see any problems
> with that approach?

That was just a guess. :) Maybe Alex can ping the Logitech folks for confirmation (and a list of mice that support this?). MSDN is failing me, and since my mouse seems to be non-high precision, I really can't test my idea.
My MS Comfort Optical Mouse 3000 with IntelliPoint 8.0 supports high precision wheel scroll (it's the reason I had to accumulate wheel deltas).
>Alex: did you get any additional details from the Logitech team about what
>changes need to happen? Can we add one of them to this bug?

That information literally just arrived.  Posting it below.

--------------------

Hi Alex,
Here is the documentation about Enhanced Scrolling we discussed last week :

The first is about the application implementation. It's very short and doesn't highlight some of the pitfalls (such as what to do with the remainder). The PDF document attached below has details about that.
- http://msdn.microsoft.com/en-us/library/ms645617(v=VS.85).aspx

This second document is the hardware implementation. It's not super relevant for you but it shows that this mechanism is not a proprietary implementation : it's based on standard windows messages and can be implemented by any mouse manufacturer.
- http://www.microsoft.com/whdc/device/input/wheel.mspx

This PDF document is an extract from a document Logitech and Microsoft are currently working on. This document is mainly intended for applications which have scrolling bugs (i,e. not Firefox) but it still contains information on the "ideal" implementation of Hi-Res scrolling. Please do not distribute as it's still being finalized. [faaborg: fair enough, file will be sent in private email to the cc list of the bug, if joining later contact me directly for it]

Finally this is a pseudo code algorithm we were working on. We decided to have a 3 lines version in the document to keep it as crisp as possible but I thought this may be interesting to you.

OnScroll( delta )
{
if( ScrollDirectionChanged( delta, cummulatedDelta ))
{
cummulatedDelta = delta
// Clear the remainder
}
else
{
cummulatedDelta += delta
// Add the new delta to the remainder
}

deltaToPixelScrollFactor = ONE_LINE_IN_PIXELS * GetSystemLinePerScroll( ) / WHEEL_DELTA
// This factor determines the scrolling sensitivity. It depends on the global
// system setting "line per scroll" which is 3 by default.

pixelsToScroll = cummulatedDelta * deltaToPixelScrollFactor

if( pixelsToScroll <= -1 || pixelsToScroll >= 1 )
{
ScrollWindowContentByPixel( pixelsToScroll )

cummulatedDelta -= pixelsToScroll / deltaToPixelScrollFactor
// Remove the scrolled distance from the cumulated delta
}
}

I still owe you the software / drivers we showed you to test Hi-Res with the devices we gave you. I will need a few more days to get this ready for you. Stay tuned :-)

Please do not hesitate to e-mail or call me [faaborg: removed number since this bug is public] if you need any additional information.

Sincerely,
Eric
>since my mouse seems to be non-high precision, I really can't test my idea.

Logitech was nice enough to give us a few hi-res mice, I can mail you one if you want.
(In reply to comment #18)
> >since my mouse seems to be non-high precision, I really can't test my idea.
> 
> Logitech was nice enough to give us a few hi-res mice, I can mail you one if
> you want.

Naa, I can pick up an ms mouse maybe from best buy. I need a new one anyway. I think my original suggestion will work. We need to figure out how to identify these mice and figure out how we map delta values to pixels.
I don't think this blocks... it's a nice feature, but we need to actually ship.
blocking2.0: ? → -
Renominating for blocking. 

Rationale: scrolling with the mousewheel is a common user action, especially on Windows. I think this is worth investigating to understand the size of the fix. It would be a strong win for perception of responsiveness.
blocking2.0: - → ?
Adding to beltzner's rationale, it's these very tactile aspects of the product that most directly determine the user's assessment of the product's speed.
NOTE:

With some MS mice, I cannot scroll any contents on Flash when I turn the mouse wheel normally. We're redirecting the native mouse wheel messages to plugins, however, I guess that Flash discards the wheel messages which delta value is less than 120. So, if we support the high-precision scrolling, we should notice it to plugin venders because most mouse venders will send the high-precision delta value to us if mouse vendors know this fix, then, many users will not be able to scroll Flash (or other plugin's) contents newly.
I take it that's something specific to NPAPI? Does IE9 have this problem, too?
We redirect the mouse wheel messages when the mouse cursor is on a plugin window. Other browsers don't redirect the messages to plugin in such case. Therefore, if mouse drivers send low-precision wheel messages to plugin window of each browsers, this problems have never happened except us.
Blocking to figure out if this is something we can still do this late in the release cycle. Masayuki, can you investigate further?
Assignee: nobody → masayuki
blocking2.0: ? → betaN+
Sorry for the delay. I'll check our implementation especially the relation between our internal event and DOM event.
Attached image Logitech SetPoint
This came in through private email:

Just a quick follow up with you regarding Hi-Res scrolling.

As of last week Logitech has posted the new version of our Mouse and Keyboard software for PC (SetPoint 6.2) which enables Hi-Resolution scrolling for the mice we gave you during our visit. Once SetPoint 6.2 is installed, you will observe that the delta value in the WM_MOUSEVSCROLL messages will be 8 times more frequent with increments of +/- 15 instead of a single message of +/- 120 previously.

To install it, follow the link below and click on "Downloads" :
http://www.logitech.com/en-us/584/3131?wt.mc_id=usym_/setpoint_global&bit=32&osid=14&strf=Universal_Symlink

On my side, I tried SetPoint 6.2 with the Beta version of Firefox 4.0 (build 7). I do see that Firefox 4.0 now responds to smaller scrolling increments where Firefox 3.x did not. However I also observe that the window scrolls by 3 steps despite receiving 8 increments from the mouse. The current implementation seem to wait for an accumulation of the scrolling delta of 40 to scroll its content. Did you decide to not allow the scrolling increment to go below a certain size or is it still too early to test this ?

Sincerely,
Eric

PS : With Setpoint 6.2 you can easily go back to the "standard" scrolling by un-checking the "Enable Smooth Scrolling" in the Pointer settings of the mouse (See screenshot below [attached])
We can't block on this any more. But we should definitely try for next release.
blocking2.0: betaN+ → .x
Attached patch Patch v0.7 (WIP) (obsolete) — Splinter Review
Hmm, current trunk's full page scroll logic is wrong. This patch fixes the bug.

And I think that we should only dispatch new native event to Gecko, and Gecko should dispatch both MozMouseScroll and MozMousePixelScroll to DOM.
Attachment #505663 - Attachment is obsolete: true
Attached patch Patch v0.9 (WIP)Splinter Review
Test builds will be here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/masayuki@d-toybox.com-2384b0f0d986

But the patch needs to replace a lot of mouse wheel handling code. So, I'm worry the risk.

Faaborg, do you (and UX team members) think that this bug should be fixed on Fx4 by this patch?

# I'll add DOM event testing for the new internal event...
Attachment #505750 - Attachment is obsolete: true
This sounds like a great feature for FF4+1
Given the similarities with our OS X implementation, how risky do we think this is?  I think at the very least we would need to get beta coverage with beta 11, and would want to back this out at the first sign of unexpected regressions.
The patch seems to change also the cross-platform mouse scroll handling 
quite a bit, so that just reviewing the patch would take some time.

Masayuki, was there some reason why you couldn't use the same
approach as what we have on OSX?
We cannot use pixel scroll event for Windows because the delta value means 1/n line or 1/n page. So, the delta value doesn't mean pixels. And we cannot compute the pixel values on widget level. Therefore, we need to define a new high resolution mouse scroll event internally and widget dispatches the event and nsEventStateManager needs to dispatch DOMMouseScroll event and MozMousePixelScroll event to DOM tree. And if the DOM events are not consumed, we need to scroll a scrollable element. At this time, we can compute the line height or page size of the scroll target. And also, the computed scroll amount may have subpixel. Then, we need to store the subpixel and add the remaining delta to next high resolution event.

For these requirements, we need to:
* replace the WM_MOSUEWHEEL and WM_MOUSEHWHEEL handler with new high resolution mouse scroll event dispatcher
* update NS_MOUSE_PIXEL_SCROLL handler in nsEventStateManager which dispatches DOMMouseScroll event
* update NS_MOUSE_SCROLL handler in nsEventStateManager which dispatches MozMousePixelScroll event
* update nsGfxScrollFrame::ScrollBy() for computes the remaining delta value

So, the most code of mouse wheel event handler will be modified.
If the reviewers feel this change is reasonable given how late in the process we are, from a UX perspective we would really love to land it.  But they should make the call on how risky we think this is.
Masayuki, if you think the patch is ready for review, I could take a better
look at it tomorrow (for the cross-platform part). But my current guess is that
this is too risky.
Less risky approach might be to make windows widget code to query the current
pixels-pre-line information from ESM and then use the same code path as what
is used for OSX.
(In reply to comment #39)
> Masayuki, if you think the patch is ready for review, I could take a better
> look at it tomorrow (for the cross-platform part). But my current guess is that
> this is too risky.

I think it's risky too. And I found some bugs in the patch, I'm working on them. I cannot recommend the patch will be landed on this timing.

> Less risky approach might be to make windows widget code to query the current
> pixels-pre-line information from ESM and then use the same code path as what
> is used for OSX.

It's less risky but I don't like the approach because we cannot get the scrollable element's information *at* scrolling. I.e., the scroll target or its information might be changed by JS. However, I understand it's rare case. Therefore, I think it might be reasonable for now. I'll try to write the patch tomorrow, and I'll test it. But even if we land the patch, I want to refactor the code by the latest patch's approach on Fx4.next.
And I think that we should suggest the high resolution scroll event for DOM to W3C.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-WheelEvent-deltaX
The delta properties are long but I think they should be float or double.
What kind of high resolution scroll event should DOM Events have? And why?
delta properties could sure be double, but why should anything else be changed?

But we can discuss about spec issue elsewhere. If you have suggestions for the 
spec, could you please email to me. Or to www-dom@w3.org
(In reply to comment #42)
> What kind of high resolution scroll event should DOM Events have? And why?
> delta properties could sure be double, but why should anything else be changed?

I meant the delta value type change. E.g., a half line scroll cannot indicate with long delta. If we convert it to pixels by the target element's line height, it changes the meaning of the event.

> But we can discuss about spec issue elsewhere. If you have suggestions for the 
> spec, could you please email to me. Or to www-dom@w3.org

Yeah, I'll post it.
Attached patch Adhoc patch v1.0 (obsolete) — Splinter Review
This works fine for me with Logitech mice.

I needed to update logic in ESM, unfortunately. The logic in PostHandleEvent() is wrong.

> 3048         if (useSysNumLines) {
> 3049           if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage)
> 3050             action = MOUSE_SCROLL_PAGE;
> 3051         }
> 3052 
> 3053         if (aEvent->message == NS_MOUSE_PIXEL_SCROLL) {
> 3054           if (action == MOUSE_SCROLL_N_LINES ||
> 3055               (msEvent->scrollFlags & nsMouseScrollEvent::kIsMomentum)) {
> 3056              action = MOUSE_SCROLL_PIXELS;
> 3057           } else {
> 3058             // Do not scroll pixels when zooming
> 3059             action = -1;
> 3060           }
> 3061         } else if (msEvent->scrollFlags & nsMouseScrollEvent::kHasPixels) {
> 3062           if (action == MOUSE_SCROLL_N_LINES ||
> 3063               (msEvent->scrollFlags & nsMouseScrollEvent::kIsMomentum)) {
> 3064             // Don't scroll lines when a pixel scroll event will follow.
> 3065             // Also, don't do history scrolling or zooming for momentum scrolls.
> 3066             action = -1;
> 3067           }
> 3068         }

First, see #3049, when the system setting scroll amount is full page scroll, even if the event is followed by pixel events, it does full page scrolling. I.e., we cannot do high resolution scrolling in such case.

And also #3061 is strange. If an NS_MOUSE_SCROLL is followed by pixel events, it does nothing ONLY when the computed action is scrolling per line or it's momentum. I update the logic as:

>         switch (aEvent->message) {
>           case NS_MOUSE_SCROLL:
>             if (msEvent->IsFollowedByPixelEvents()) {
>               // When this event is followed by some pixel scroll events,
>               // this event should do nothing.
>               action = -1;
>             } else if (useSysNumLines) {
>               if (msEvent->IsFullPage()) {
>                 action = MOUSE_SCROLL_PAGE;
>               } else {
>                 action = MOUSE_SCROLL_N_LINES;
>               }
>             } else if (msEvent->IsMomentum()) {
>               // If the event was dispatched by momentum, this event shouldn't
>               // do history scrolling and zooming.
>               if (action == MOUSE_SCROLL_HISTORY ||
>                   action == MOUSE_SCROLL_ZOOM) {
>                 action = -1;
>               }
>             }
>             break;
>           case NS_MOUSE_PIXEL_SCROLL:
>             if (useSysNumLines ||
>                 action == MOUSE_SCROLL_N_LINES || action == MOUSE_SCROLL_PAGE) {
>               // If the action is scrolling contents, this event should do it
>               // by pixels.
>               action = MOUSE_SCROLL_PIXELS;
>             } else {
>               // Do nothing when the action isn't scrolling contents.
>               // I.e., zooming or history scrolling.
>               action = -1;
>             }
>             break;
>         }

Test builds are coming soon:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/masayuki@d-toybox.com-f27fd73a7272

The test result can check here:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=f27fd73a7272
Version: unspecified → Trunk
Could we just not care about page events too much at this point?
I mean, just keep them working the same way as always.
Once we implement DOM 3 Events wheel event, then we could have support
for real page scroll events.
Attached patch Adhoc patch v1.0.1 (obsolete) — Splinter Review
Sorry for the spam, v1.0 was changed for debugging code (changed the message result).
Attachment #507191 - Attachment is obsolete: true
(In reply to comment #45)
> Could we just not care about page events too much at this point?
> I mean, just keep them working the same way as always.

Hmm, I think the high resolution scrolling is more useful with page scrolling... By the patch, 1/8 page is scrolled by every native wheel event if the mouse is Logitech's.

The Mac's pixel scroll implementer didn't think about page scroll because Mac doesn't have page scroll by mouse wheel. Therefore, I need to change the logic for Windows.

> Once we implement DOM 3 Events wheel event, then we could have support
> for real page scroll events.

I'm not sure what you mean. We already support the page scrolling by mouse wheel on Windows. Mac doesn't have page scroll settings for mouse wheel. The change is needed for the high-resolution scrolling.
The risk vs reward on this patch is insane for FF4. How many people have these mice with the proper drivers on vista+ (likely fractions of a percent) vs how many people would be affected by a scrolling regression (potentially everyone). Couple that with comment 23 and comment 41 and this really seems out of scope for FF4.

It is my opinion that no work should be done on this until post-FF4 and I am bummed that two of our awesome devs are spending cycles on it.
(In reply to comment #48)
> The risk vs reward on this patch is insane for FF4. How many people have these
> mice with the proper drivers on vista+ (likely fractions of a percent) vs how
> many people would be affected by a scrolling regression (potentially everyone).

Both Logitech mice driver and Microsoft mice driver support high resolution scroll. I guess that these two vendors have big share in the world. And both drivers have automatic updater themselves.

> Couple that with comment 23 and comment 41 and this really seems out of scope
> for FF4.

The patch doesn't affect about comment 23 issue. We're redirecting raw mouse wheel messages to plug-ins without any work. So, mouse driver changes to use high resolution scrolling for our window, the comment 23 issue happens both on current trunk build and patched build. And we cannot choose whether the drivers sends low resolution message or high resolution message.

And comment 41 is just my idea, not related to this bug. Until we implement DOM3 mouse wheel event, we dispatch DOMMouseScroll event without any changes, even if my patch is applied.
(In reply to comment #49)
> (In reply to comment #48)
> > The risk vs reward on this patch is insane for FF4. How many people have these
> > mice with the proper drivers on vista+ (likely fractions of a percent) vs how
> > many people would be affected by a scrolling regression (potentially everyone).
> 
> Both Logitech mice driver and Microsoft mice driver support high resolution
> scroll. I guess that these two vendors have big share in the world. And both
> drivers have automatic updater themselves.
> 

Ah, I wasn't aware they have their own updaters. That changes things a bit, but I still feel this should be punted and would be a great feature (self contained, PR can talk about it, clear test plan and criterion for success, etc) for future quarterly/mini releases.
>Ah, I wasn't aware they have their own updaters

Yeah, it was actually that update to all of their mice already in the marketplace that prompted us to start work on this bug.  Either way, I agree that this sounds outside of our tolerance for risk at this stage.
(In reply to comment #51)
> I agree that this sounds outside of our tolerance for risk at this stage.

I have worked for this bug due to the request of jst in comment 26. And you hoped that this bug is fixed by b11 if it's possible.

Now, I have a simplest patch which changes few lines in XP level and replaces native mouse wheel event handler of Windows.
http://hg.mozilla.org/try/rev/03d76e5e041c

You should decide whether we should go review process for to fix on Fx4b11 or will put it off to Fx4.next.
I definitely want this fixed if we feel it isn't scary, but I'm concerned by comment #39:

>But my current guess is that this is too risky.
(In reply to comment #47)
> I'm not sure what you mean. We already support the page scrolling by mouse
> wheel on Windows. 
Sure, but what we report to web pages isn't exactly anything too great.
DOM 3 Events defines what kind of event to dispatch.
(In reply to comment #53)
> I definitely want this fixed if we feel it isn't scary, but I'm concerned by
> comment #39:
> 
> >But my current guess is that this is too risky.

Yes, but the latest patch is based on the smaug's idea in the comment #39. If you still think that the smaug's approach is risky too for now, I have no reason to keep working on this bug in this cycle.

(In reply to comment #54)
> (In reply to comment #47)
> > I'm not sure what you mean. We already support the page scrolling by mouse
> > wheel on Windows. 
> Sure, but what we report to web pages isn't exactly anything too great.
> DOM 3 Events defines what kind of event to dispatch.

Sure. I keep it in my mind for next work.
>If you still think that the smaug's approach is risky too for now

I'm really not the right person to gauge the overall risk of a core > event handling patch.  In terms of UX this is very much wanted, but the reviewers should make the final call on if we can land it.
Masayuki, is the patch ready for a review?
Does it pass tests on tryserver?

I wonder if we could take the fix (hopefully asap if nothing major is 
found in review), but back it out immediately if some regression is found.
And if regression is found, we would just fix the issues for FFnext.
Attached patch Adhoc patch v1.1 (obsolete) — Splinter Review
http://tbpl.mozilla.org/?tree=MozillaTry&rev=03d76e5e041c

Yes, this patch passed all tests.

> @@ -3046,25 +3066,25 @@ nsEventStateManager::PostHandleEvent(nsP
>            nsContentUtils::GetBoolPref(sysNumLinesKey.get());
>  
>          if (useSysNumLines) {
>            if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage)
>              action = MOUSE_SCROLL_PAGE;
>          }
>  
>          if (aEvent->message == NS_MOUSE_PIXEL_SCROLL) {
> -          if (action == MOUSE_SCROLL_N_LINES ||
> +          if (action == MOUSE_SCROLL_N_LINES || action == MOUSE_SCROLL_PAGE ||
>                (msEvent->scrollFlags & nsMouseScrollEvent::kIsMomentum)) {
>               action = MOUSE_SCROLL_PIXELS;
>            } else {
>              // Do not scroll pixels when zooming
>              action = -1;
>            }

The if condition should mean that the action is "scroll contents" or "momentum". Mac doesn't have page scrolling, therefore, this adds it in the condition. This doesn't affect to other platforms because full page scroll is supported only on Win and OS/2. OS/2 doesn't dispatch NS_MOZ_MOUSE_PIXEL_SCROLL.

>          } else if (msEvent->scrollFlags & nsMouseScrollEvent::kHasPixels) {
> -          if (action == MOUSE_SCROLL_N_LINES ||
> +          if (useSysNumLines || action == MOUSE_SCROLL_N_LINES ||
>                (msEvent->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;
>            }
>          }

And this if condition should mean that the event is NS_MOUSE_SCROLL which is followed by NS_MOUSE_PIXEL_SCROLL and the computed action is "scroll contents" or "momentum", it's scrolled by the followed pixel scroll events. Therefore, the event doesn't need to cause scroll.

useSysNumLines means that the action is line scroll or page scroll, and the action isn't overridden by us with modifier key. I.e., if page scroll is caused by user's choice, we shouldn't perform the followed pixel scroll event. Therefore, action == MOUSE_SCROLL_PAGE is wrong here. We should check useSysNumLines for page scroll.
Attachment #507193 - Attachment is obsolete: true
Attachment #507882 - Flags: review?(Olli.Pettay)
Comment on attachment 507882 [details] [diff] [review]
Adhoc patch v1.1

This patch needs to rewrite the nsWindow::OnMouseWheel() method for dispatching NS_MOUSE_PIXEL_SCROLL event for each WM_MOUSE(H)WHEEL.

First, I separated the system setting caching part to new method for simple implementation of OnMouseWheel().

Until HandleScrollingPlugins(), the logic isn't changed.

Next, if we need to forget the remaining delta value of last wheel event, this calls ResetRemainingWheelDelta(). Check the condition by the if condition.

Next, we need to know content's line height or page height or page width for NS_MOUSE_PIXEL_SCROLL. For querying them, we need to set sample mouse scroll event. It is the testEvent. the delta value isn't important except the sign.

Even if the query event is failed, we should dispatch NS_MOUSE_SCROLL event. This prevents regression. Therefore, the error checking (!queryEvent.mSucceeded) will be after dispatching NS_MOUSE_SCROLL event.

The NS_MOUSE_SCROLL event's delta value is computed by current event's delta value and remaining delta value. And for computing remaining delta value for next WM_MOUSE(H)WHEEL, we need to compute the difference between the raw delta value (including last remaining delta) and current dispatching delta value.

Note that I changed the computation of page scroll because current implementation is wrong. Per WHEEL_DELTA (120), we should scroll one page but current trunk scrolls one page every WM_MOUSE(H)WHEEL.

If the delta is 0 (this means that the delta value is less than 1 line or page), we shouldn't dispatch NS_MOSUE_SCROLL event because if we dispatch it at that time, more and more DOMMouseScroll event will be handled by Web applications.

Finally, we dispatch NS_MOUSE_PIXEL_SCROLL. The logic is similar to NS_MOUSE_SCROLL initialization. The only difference is, the computing delta is in lines/pages or pixels.

For example: If we receive 15 native delta value 6 times and system settings say scroll 1 line per 40 delta and its line height is 15px, the dispatched events are:

1st message:
NS_MOUSE_SCROLL       delta:  0line, remaining-native-delta: 15, not dispatched.
NS_MOUSE_PIXEL_SCROLL delta:  5px,   remaining-native-delta:  1, dispatched.

2nd message:
NS_MOUSE_SCROLL       delta:  0line, remaining-native-delta: 30, not dispatched.
NS_MOUSE_PIXEL_SCROLL delta:  6px,   remaining-native-delta:  1, dispatched.

3rd message:
NS_MOUSE_SCROLL       delta:  1line, remaining-native-delta:  5, dispatched.
NS_MOUSE_PIXEL_SCROLL delta:  6px,   remaining-native-delta:  1, dispatched.

4th message:
NS_MOUSE_SCROLL       delta:  0line, remaining-native-delta: 20, not dispatched.
NS_MOUSE_PIXEL_SCROLL delta:  6px,   remaining-native-delta:  1, dispatched.

5th message:
NS_MOUSE_SCROLL       delta:  0line, remaining-native-delta: 35, not dispatched.
NS_MOUSE_PIXEL_SCROLL delta:  6px,   remaining-native-delta:  1, dispatched.

6th message:
NS_MOUSE_SCROLL       delta:  1line, remaining-native-delta: 10, dispatched.
NS_MOUSE_PIXEL_SCROLL delta:  6px,   remaining-native-delta:  1, dispatched.

On trunk, this scrolls 2lines, i.e., 30px. By this patch scrolls 35px. The remaining native delta value for NS_MOUSE_SCROLL is 10, that means about 4px. So, there is an error by decimal fraction but it's small.
Attachment #507882 - Flags: review?(jmathies)
I don't really know much about this feature, so keep that in mind. That said, given where we are in the release cycle, the question really should not be "is it safe to take this fix", but rather "will it suck too much if we don't take this fix". Any risk that we can avoid for FF4 is IMHO risk we should avoid.
Attached patch Adhoc patch v1.2 (obsolete) — Splinter Review
fix a nit in nsWindow::OnMouseWheel().
Attachment #507882 - Attachment is obsolete: true
Attachment #507922 - Flags: review?(jmathies)
Attachment #507922 - Flags: review?(Olli.Pettay)
Attachment #507882 - Flags: review?(jmathies)
Attachment #507882 - Flags: review?(Olli.Pettay)
Attached patch Adhoc patch v1.3 (obsolete) — Splinter Review
The new event must not be NS_IS_IME_RELATED_EVENT. (nsGUIEvent.h)
Attachment #507922 - Attachment is obsolete: true
Attachment #507926 - Flags: review?(jmathies)
Attachment #507926 - Flags: review?(Olli.Pettay)
Attachment #507922 - Flags: review?(jmathies)
Attachment #507922 - Flags: review?(Olli.Pettay)
Attached patch Adhoc patch v1.4 (obsolete) — Splinter Review
Sorry for the spam.

In nsWindow::OnMouseWheel(), the new events (testEvent and queryEvent) should be initialized by nsWindow::InitEvent() which initialize the eventRef by cursor position. This is important when user tries to scroll non-focused window's scrollable element and its font-size is different from the focused window's scrollable element.
Attachment #507926 - Attachment is obsolete: true
Attachment #507964 - Flags: review?(jmathies)
Attachment #507964 - Flags: review?(Olli.Pettay)
Attachment #507926 - Flags: review?(jmathies)
Attachment #507926 - Flags: review?(Olli.Pettay)
(In reply to comment #60)
> I don't really know much about this feature, so keep that in mind. That said,
> given where we are in the release cycle, the question really should not be "is
> it safe to take this fix", but rather "will it suck too much if we don't take
> this fix". Any risk that we can avoid for FF4 is IMHO risk we should avoid.

Indeed. I think we should just not take this for FF4.
FYI:

I posted the type issue of delta value of DOM3 WheelEvent to W3C.
http://lists.w3.org/Archives/Public/www-dom/2011JanMar/0021.html
I don't feel we should take either of these for 4.0, our scroll wheel code is touchy and we usually get regressions from changes. Masayuki, can you setup reviews for the whole fix (Patch v0.9?), we'll review that and shoot to get those changes in at some point.

Is there any way we can put together some tests for this code in widget?
(In reply to comment #66)
> I don't feel we should take either of these for 4.0, our scroll wheel code is
> touchy and we usually get regressions from changes. Masayuki, can you setup
> reviews for the whole fix (Patch v0.9?), we'll review that and shoot to get
> those changes in at some point.

No, the strict way is risky. And if I will work for it after Fx4.0, I can improve it simpler (using high resolution delta value in nsMouseScrollEvent, not DOMMouseWheel event).

> Is there any way we can put together some tests for this code in widget?

I think that I cannot write it with current adhoc patch because we need the line-height value or page height/width value for checking the result in js level but I'm not sure the way. nsIScrollableFrame isn't scriptable interface and it computes the line-height from font metrics. We don't have a way to get the actual font metrics from js.

If we consider we implement a high resolution delta value for DOMMouseWheel event, we can test it easily.
Comment on attachment 507964 [details] [diff] [review]
Adhoc patch v1.4

Masayuki, which patch should we take for FF5?
This is something which should land asap when the tree opens and
there should be a plan to either pref off or back out the patch.
Attachment #507964 - Flags: review?(Olli.Pettay)
Comment on attachment 507964 [details] [diff] [review]
Adhoc patch v1.4

I'll rewrite the patch with the original approach.
Attachment #507964 - Attachment is obsolete: true
Attachment #507964 - Flags: review?(jmathies)
Um, I don't have much time for Fx5. Therefore, I changed my mind. I suggest that we should use the adhoc patch for now. I'll check it on current trunk and request review soon.
Attached patch Adhoc Patch v1.5 (obsolete) — Splinter Review
Let's use the adhoc (lower risk) approach for Fx5.
Attachment #524259 - Flags: review?(jmathies)
Attachment #524259 - Flags: review?(Olli.Pettay)
This needs to be able to be preffed off or easily backed out btw.
I think that backout is only the way because this needs to replace most part of mouse wheel handling code of nsWindow for Windows.
Would other code depend on the new behavior or code? We don't want cascading dependencies if we do in fact have to back it out.
If somebody changes the mouse wheel handling after the patch, it depends on the patch. However, such conflict cannot be avoided by using pref even if it's possible. (I.e., if I *add* the new handler (without removing current handler), another developer needs to fix their own bug on both code, and test *them*. I don't think that it will be correctly done.)

Note that the patch supports new platform feature, but it's actually expanding to support the existing feature on Windows.
That doesn't really change the fact that if we find any problems in this patch after Firefox 5 branch (April 21st iirc) we need to back this out. If other dependencies are checked in on top of it we'll need to back out or disable those too.

I'm fine with using this as the plan. As long as we're prepared to actually execute that plan if the need arises.
Any update on when we will be able to get reviews?  I'm eager for us to get this landed on nightly so that we can start testing.
Sorry for the delay.
Starting to review asap
Masayuki, could you do me a huge favor and merge this to trunk?
Attached patch Adhoc Patch v1.6 (obsolete) — Splinter Review
here.
Attachment #524259 - Attachment is obsolete: true
Attachment #528233 - Flags: review?(jmathies)
Attachment #528233 - Flags: review?(Olli.Pettay)
Attachment #524259 - Flags: review?(jmathies)
Attachment #524259 - Flags: review?(Olli.Pettay)
(In reply to comment #81)
> Created attachment 528233 [details] [diff] [review] [review]
> Adhoc Patch v1.6
> 
> here.

This broke smooth scrolling for my mouse wheel, I get line scroll with the patch applied. Was that intended?
If there's something you want me to debug specifically Masayuki let me know. Here are a few data points:

queryEvent.mReply.mLineHeight = 19

scrollEvent.scrollFlags = kHasPixels

nativeDeltaForScroll = -120

sMouseWheelScrollLines = 3

sMouseWheelScrollChars = 3

deltaPerUnit = 40

delta = 3

The mouse is a microsoft optical comfort mouse 1000.
Comment on attachment 528233 [details] [diff] [review]
Adhoc Patch v1.6

Clearing review request since the issue Jimm has with the patch must be fixed.
Attachment #528233 - Flags: review?(Olli.Pettay)
(In reply to comment #82)
> This broke smooth scrolling for my mouse wheel, I get line scroll with the
> patch applied. Was that intended?

Hi, sorry for the delay. Today is final national holiday Japan. So, I'm going to look this tomorrow.

I guess that the autoscrolling is the function of Gecko's, right? If so, the kHasPixels kills the autoscrolling. Hmm, it's difficult issue...
(In reply to comment #85)
> (In reply to comment #82)
> > This broke smooth scrolling for my mouse wheel, I get line scroll with the
> > patch applied. Was that intended?
> 
> Hi, sorry for the delay. Today is final national holiday Japan. So, I'm going
> to look this tomorrow.
> 
> I guess that the autoscrolling is the function of Gecko's, right? If so, the
> kHasPixels kills the autoscrolling. Hmm, it's difficult issue...

I think that's correct, I believe we implemented that for touch screens, so the auto scroll would turn off.
Attached patch Adhoc Patch v1.7 (obsolete) — Splinter Review
I added kAllowSmoothScroll flag to the mouse scroll event.
Attachment #528233 - Attachment is obsolete: true
Attachment #530586 - Flags: review?(jmathies)
Attachment #528233 - Flags: review?(jmathies)
Comment on attachment 530586 [details] [diff] [review]
Adhoc Patch v1.7

Review of attachment 530586 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks like good cleanup of our horrendously complex scrolling code.

::: widget/public/nsGUIEvent.h
@@ +414,5 @@
>  // Query if the DOM element under nsEvent::refPoint belongs to our widget
>  // or not.
>  #define NS_QUERY_DOM_WIDGET_HITTEST     (NS_QUERY_CONTENT_EVENT_START + 9)
> +// Query for some information about mouse wheel event's target
> +// XXX This is used only for bug 605648 temporary.  DON'T use this event.

No need to mention the bug # we have blame for that. I would just say this is a private event used by mouse scrolling and should not be used.

@@ -1608,1 @@
>  

What was the purpose of this change? Are these not content events anymore?

::: widget/src/windows/nsWindow.cpp
@@ +6332,5 @@
> +  ResetRemainingWheelDelta();
> +
> +  BOOL ret = ::SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0,
> +                                    &sMouseWheelScrollLines, 0);
> +  if (!ret) {

nit - Lets just nix the BOOL here and wrap the call in the if. Same below.

@@ +6403,5 @@
>      return quit; // return immediately if it's not our window
> + }
> +
> +  PRInt32 nativeDelta = (short)HIWORD(aWParam);
> +  if (nativeDelta == 0) {

nit - !nativeDelta

@@ +6427,5 @@
> +      (sLastMouseWheelWnd != mWnd ||
> +       sLastMouseWheelDeltaIsPositive != (nativeDelta > 0) ||
> +       sLastMouseWheelOrientationIsVertical != isVertical ||
> +       sLastMouseWheelUnitIsPage != isPageScroll ||
> +       now - sLastMouseWheelTime > 1500)) {

Do we really need the timeout here? Wheel delta is usually pretty small. What were you trying to fix here? (Also, why 1500?)

@@ +6517,5 @@
> +      (PRInt32)(scrollEvent.delta * orienter * deltaPerUnit);
> +    sRemainingDeltaForScroll = nativeDeltaForScroll - recomputedNativeDelta;
> +  }
> +
> +  if (scrollEvent.delta != 0) {

nit - nix the |!= 0|

::: widget/src/windows/nsWindow.h
@@ +666,1 @@
>  };

Nit - can you move these up above AutoForgetRedirectedKeyDownMessage and try to match the formatting of the other statics.
(In reply to comment #88)
> 
> @@ -1608,1 @@
> >  
> 
> What was the purpose of this change? Are these not content events anymore?
> 

I was referring to the changes to 

#define NS_IS_QUERY_CONTENT_EVENT(evnt) \
...
(In reply to comment #88)
> @@ -1608,1 @@
> >  
> 
> What was the purpose of this change? Are these not content events anymore?

No, the patch checks the struct type instead of message. nsQueryContentEvent is used only for them. Therefore, this is quicker.

> @@ +6427,5 @@
> > +      (sLastMouseWheelWnd != mWnd ||
> > +       sLastMouseWheelDeltaIsPositive != (nativeDelta > 0) ||
> > +       sLastMouseWheelOrientationIsVertical != isVertical ||
> > +       sLastMouseWheelUnitIsPage != isPageScroll ||
> > +       now - sLastMouseWheelTime > 1500)) {
> 
> Do we really need the timeout here? Wheel delta is usually pretty small.
> What were you trying to fix here? (Also, why 1500?)

Mr. Eric Tissot-Dupont who is a manager of the Logitech device driver team for mice and keyboards lectured me by email. I'll forward it to you.

In the email, he said that we should reset it when the wheel is idle for more than 1 or 2 seconds.
Attached patch Adhoc Patch v1.8Splinter Review
See above comment.
Attachment #530586 - Attachment is obsolete: true
Attachment #530586 - Flags: review?(jmathies)
Comment on attachment 532112 [details] [diff] [review]
Adhoc Patch v1.8

r+ -> widget parts
Attachment #532112 - Flags: review+
Comment on attachment 532112 [details] [diff] [review]
Adhoc Patch v1.8

Thank you, Jim!
Attachment #532112 - Flags: review?(Olli.Pettay)
Comment on attachment 532112 [details] [diff] [review]
Adhoc Patch v1.8

r+ for the non-widget part.
Attachment #532112 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/0a1e7ec7e268

Thank you, everyone.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Did this patch turn off scrolling acceleration ?  Used to be the faster you rolled the wheel the further the page would scroll, now it seems linear/flat no matter how fast you roll the wheel.  For those that used this feature it 'feels' like scrolling is now way slower.
Using cset just before this landed and pref's set at:
http://hg.mozilla.org/mozilla-central/rev/3e7a21049b6c

mousewheel.acceleration.factor;5
mousewheel.acceleration.start;3

I can scroll page in one roll of the wheel.  With this patch those settings seem to be ignored or over-written, as it takes 2-3 rolls to scroll the same page.
Depends on: 657634
Masayuki, should you perhaps back this out and fix the regression before
relanding? Or do you think the regressions are easy to fix?
(In reply to comment #98)
> Masayuki, should you perhaps back this out and fix the regression before
> relanding? Or do you think the regressions are easy to fix?

I read these bug reports but I don't think they are valid for high-resolution scrolling. We decided that we should use pixel scroll for Windows too. Then, we need to discard all functions for line scrolling when we enable the high resolution scrolling.

I guess that both bugs could be fixed if we made NS_QUERY_SCROLL_TARGET_INFO event fail when these prefs are used. I'll post the patch soon.
This is a new 'feature', and as I understand things new features are supposed to be able to be 'turned off', or backed out on regressions. 

Not 'everyone' wants/needs support of HD scrolling, especially on pages that are mostly 'text'.
(In reply to comment #100)
> This is a new 'feature', and as I understand things new features are
> supposed to be able to be 'turned off', or backed out on regressions. 

Coming patch refers mousewheel.enable_pixel_scrolling. So, users can kill the high resolution scrolling.

I'll post it to bug 657634.
(In reply to comment #101)
> I'll post it to bug 657634.

I changed my mind. I'll post separated patches.

I already posted a patch to bug 657865 for disabling high resolution scrolling on Windows by pref and in default settings.
I'm going to land the patch for bug 657865 tomorrow morning (JST). Then, you cannot reproduce all regressions in default settings. But DON'T close the regression bugs at that time. They are just hidden by the landing, not fixed yet.
Slightly off topic, apologies.

Jim, I'd highly recommend using SmoothWheel. Scrolling is greatly improved while using the addon, and it has all the configuration options you would want, including a couple of tweaks for the acceleration.

https://addons.mozilla.org/en-US/firefox/addon/smoothwheel/

For what it's worth, I agree with him; not implementing acceleration with high resolution scrolling seems to be a mistake. It's one of those things that, once you've become accustomed to it, it's very hard to revert.
Depends on: 662191
How can this be tested?
Thanks
If you're using Logitech's mouse with SetPoint, you can see half line scrolling in default settings.

Otherwise, I'm not sure how to test it. If your mouse sends always 30 or 40 delta value and you set vertical scroll amount to only one line, you might be able to see scrolling less than a line.
I'll also reach out to Logitech to see if they caught anything in testing.
I have a Logitech G500 and smooth scrolling doesn't work. Should I be complaining here or at Logitech because they haven't allowed all-around per-pixel scrolling on the G* line of mice preferring instead to only give a plugin for IE? Based on my initial poking around, it seems Logitech have messed up by splitting the drivers into two branches...
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.