Bug 206438 (smooth scrolling should be similar to SmoothWheel) distinguishes mouse wheel events from the rest by identifying "pixels" unit at the scroll request. This only works on windows, because on Linux/OSX normal mouse wheel scroll generate "lines" unit scroll requests. This bug aims to fix it for those platforms.

Summary of some IRC discussions from today: [ESM = Events state manager] tn: On Linux, ESM only receives "lines" scroll [for normal mouse wheel]. tn: On OSX, ESM receives "lines" scrolls for normal external mice, and "pixels" scrolls for proper "pixel devices" (trackpad, magic mouse, etc). (*) avih: On Windows, even for normal mouse wheel events, the platform-specific code sends ESM a pixels scroll event ("faked", because it didn't originate in a true pixels scroll request). (*) avih: Pixels scrolls on both OSX and Windows (faked or not) are preceded with a lines scroll event which has the kHasPixels flag on. masayuki: The windows behavior will change with bug 719320 and bug 672175 [this will probably fix the Windows abnormality in this context]. @mstange, tn is not 100% sure of his OSX info, could you please confirm?

Yes, I'm pretty sure this is all correct.

Masayuki, so we never got pixel scrolling hooked up to what some hardware was sending? What is the plan for pixel scrolling on Windows?

Also, why do we want to always send pixel scroll events even if we didn't get pixel level scroll messages from the OS?

So that a consumer could choose to handle only pixel scroll events without missing any events. The same works for line scroll events: Existing web contents expected only line scrolls from DOMMouseScroll events, so handling only those needed to work, too. Also, the kHasPixels flag isn't published on the scroll DOM events, so web content can't conditionally handle either line scrolls or pixel scroll because it doesn't know the original source unit. Looks like smaug and I made that decision on IRC, see bug 350471 comment 51. In any case, the new DOM wheel events (bug 719320) probably have a much better API.

Oh sorry, I specifically meant how the Windows handles it where it always send both types of events even if it doesn't have any pixel data. On OS X we only send pixel events if we get pixel data, no?

Oh, right. What I was thinking of were the DOM events. You're right about OS X, and I don't know the reasons for the Windows behavior.

(In reply to Markus Stange from comment #5) > In any case, the new DOM wheel events (bug 719320) probably have a much > better API. I'm thinking that: 1. D3E wheel event is fired. 2. If D3E wheel event isn't consumed by preventDefault(), DOMMouseScroll is fired. 3. If DOMMouseScroll event isn't consumed by preventDefault(), MozMousePixelScroll is fired. 4. If MozMousePixelScroll event isn't consumed by preventDefault(), ESM does its default action.

Sounds good.

This _should_ fix OSX+Linux (not tested), but will break Windows, because on Windows it'll ens at the MOUSE_SCROLL_PIXELS case.

Comment on attachment 608658 [details] [diff] [review] Part 1: Propagate mouse wheel event origin from ESM roc: Can you review the /layout and /modules/libpref changes? smaug: Can you review the /content changes?

FWIW, even after landing bug 672175, part 2 here (address windows abnormality) is still required, and in general, this patch doesn't create hg conflicts with latest m-c, and the parts of this patch are as valid as before.

Comment on attachment 608658 [details] [diff] [review] Part 1: Propagate mouse wheel event origin from ESM > nsresult > nsEventStateManager::DoScrollText(nsIFrame* aTargetFrame, > nsMouseScrollEvent* aMouseEvent, > nsIScrollableFrame::ScrollUnit aScrollQuantity, > bool aAllowScrollSpeedOverride, >- nsQueryContentEvent* aQueryEvent) >+ nsQueryContentEvent* aQueryEvent, >+ nsIAtom *aOrigin) I would use an enum for aOrigin.

(In reply to Olli Pettay [:smaug] from comment #15) >... > I would use an enum for aOrigin. Me too, but roc really wanted atoms instead. The main reason, as far as I understand, is to later on compose a preference name on the fly from this string, instead of using a big case statement for each origin. If you wish to follow this discussion, see bug 206438 comment 52 onwards.

Comment on attachment 608658 [details] [diff] [review] Part 1: Propagate mouse wheel event origin from ESM Ok, I see. A bit ugly but should work.

Comment on attachment 608658 [details] [diff] [review] Part 1: Propagate mouse wheel event origin from ESM Review of attachment 608658 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/events/src/nsEventStateManager.cpp @@ +3218,5 @@ > break; > > case MOUSE_SCROLL_PAGE: > DoScrollText(aTargetFrame, msEvent, nsIScrollableFrame::PAGES, > false); Why not pass an origin for all calls to DoScrollText? ::: content/events/src/nsEventStateManager.h @@ +353,5 @@ > nsMouseScrollEvent* aMouseEvent, > nsIScrollableFrame::ScrollUnit aScrollQuantity, > bool aAllowScrollSpeedOverride, > + nsQueryContentEvent* aQueryEvent = nsnull, > + nsIAtom *aOrigin = nsnull); I don't think this parameter should be optional.

Comment on attachment 608660 [details] [diff] [review] Part 3: Cleanups and consistency Review of attachment 608660 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1407,5 @@ > */ > void > +nsGfxScrollFrameInner::AsyncScroll::InitDuration(nsIAtom *aOrigin) { > + if (!aOrigin) > + aOrigin = nsGkAtoms::other; Who's passing a null origin? Let's make everything pass an explicit origin.

(In reply to Avi Halachmi (:avih) from comment #16) > Me too, but roc really wanted atoms instead. The main reason, as far as I > understand, is to later on compose a preference name on the fly from this > string, instead of using a big case statement for each origin. If you wish > to follow this discussion, see bug 206438 comment 52 onwards. The main reason is modularity. This way you can add new scroll origins without modifying nsGfxScrollFrame.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > Comment on attachment 608658 [details] [diff] [review] > Part 1: Propagate mouse wheel event origin from ESM > > Review of attachment 608658 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/events/src/nsEventStateManager.cpp > @@ +3218,5 @@ > > break; > > > > case MOUSE_SCROLL_PAGE: > > DoScrollText(aTargetFrame, msEvent, nsIScrollableFrame::PAGES, > > false); > > Why not pass an origin for all calls to DoScrollText? My original (unsubmitted) patch actually did this, however, when I rewrote it, I changed it because: 1. We don't need other origins right now (and we can add them later if/when that changes). Bug 206438 comment 62 defines our current "requirement" as mouse wheel scroll only. I added other origins (lines/pages) because I think they're very useful, AND the code changes were very local and safe. 2. If we really want it complete, we should also use origin for all other scroll requests/propagations (e.g. usages of ScrollTo, ScrollBy, etc - a LOT of places around the code). Also, for some/many of these places, origin might not be as obvious as it is here, and also, most scroll calls don't even know if it might end in smooth scroll - which is where origin is used. 3. "Fragmentation" of scroll origins will, IMHO, make it less useful. Right now origin is mouseWheel/scrollbars/lines/pages(/pixels/other), which covers nicely all scroll triggers (and after minimal testing, smooth scroll currently always ends with one of first 4). If we use "high resolution" origin, we could end up with MANY different origins (e.g. VK_UP/VK_DOWN/VK_LEFT/...), which will overall make this system cumbersome, and therefore not as useful IMO. So, taking into account the negatives stated above, compared to the other approach of minimal required code changes (even if generally "incomplete"), I chose the latter as I see it as less evil. > > ::: content/events/src/nsEventStateManager.h > @@ +353,5 @@ > > nsMouseScrollEvent* aMouseEvent, > > nsIScrollableFrame::ScrollUnit aScrollQuantity, > > bool aAllowScrollSpeedOverride, > > + nsQueryContentEvent* aQueryEvent = nsnull, > > + nsIAtom *aOrigin = nsnull); > > I don't think this parameter should be optional. See next reply. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19) > Comment on attachment 608660 [details] [diff] [review] > Part 3: Cleanups and consistency > > Review of attachment 608660 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/generic/nsGfxScrollFrame.cpp > @@ +1407,5 @@ > > */ > > void > > +nsGfxScrollFrameInner::AsyncScroll::InitDuration(nsIAtom *aOrigin) { > > + if (!aOrigin) > > + aOrigin = nsGkAtoms::other; > > Who's passing a null origin? Let's make everything pass an explicit origin. Part 2 of this patch does. And I used null and not nsGkAtoms::other as default value because IMO it's easier and less error prone in usage. Making all scroll calls use an explicit (and correct) origin is not easy, requires MANY code changes all over the place, cumbersome, and error prone (see answers 2,3 above).

Comment on attachment 608658 [details] [diff] [review] Part 1: Propagate mouse wheel event origin from ESM Review of attachment 608658 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +2327,4 @@ > { > nsSize deltaMultiplier; > + if (!aOrigin) > + aOrigin = nsGkAtoms::other; {} @@ +2334,5 @@ > nscoord appUnitsPerDevPixel = > mOuter->PresContext()->AppUnitsPerDevPixel(); > deltaMultiplier = nsSize(appUnitsPerDevPixel, appUnitsPerDevPixel); > + if (isGenericOrigin) > + aOrigin = nsGkAtoms::pixels; {} @@ +2340,5 @@ > } > case nsIScrollableFrame::LINES: { > deltaMultiplier = GetLineScrollAmount(); > + if (isGenericOrigin) > + aOrigin = nsGkAtoms::lines; {} @@ +2346,5 @@ > } > case nsIScrollableFrame::PAGES: { > deltaMultiplier = GetPageScrollAmount(); > + if (isGenericOrigin) > + aOrigin = nsGkAtoms::pages; {}

Comment on attachment 608660 [details] [diff] [review] Part 3: Cleanups and consistency Review of attachment 608660 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1407,5 @@ > */ > void > +nsGfxScrollFrameInner::AsyncScroll::InitDuration(nsIAtom *aOrigin) { > + if (!aOrigin) > + aOrigin = nsGkAtoms::other; {} around the if body

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > {} around the if body OK, and what about your previous comments (and my replies)?

I agree with your response to my previous comments.

Adds {} around if bodies.

{} around the if body.

Comment on attachment 608659 [details] [diff] [review] Part 2: Address Windows abnormality (detect "faked" pixel scrolls) Review of attachment 608659 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/events/src/nsEventStateManager.cpp @@ +3226,5 @@ > DoScrollText(aTargetFrame, msEvent, nsIScrollableFrame::DEVICE_PIXELS, > + false, nsnull, > + (msEvent->scrollFlags & nsMouseScrollEvent::kFromLines) ? > + nsGkAtoms::mouseWheel : nsnull > + ); nit - wrap the conditional statement in paren so you don't have to fold that ending ');' over.

Readability.

I also suggest to land this for Aurora as well. Otherwise, bug 206438 (already in Aurora) is not fixed for OSX/Linux, and it will also not work for Windows if mousewheel.enable_pixel_scrolling == false (not sure when that's the case, but mouse wheel roll will NOT trigger longer animation duration when enable_pixel_scrolling == false).

Comment on attachment 609268 [details] [diff] [review] Part 1 V2: Propagate mouse wheel event origin from ESM I'm clearing the checkin flag on the attachments since all parts should land at the same time and it will be easier for the person landing this to only update one place stating that they have checked this in.

Removing checkin-needed for now: Patch V2 might not work on all Linux systems. jwir3 and dholbert compiled and tested this patch on Linux (jwir3 on Ubuntu 11.04, and dholbert on Ubuntu 12.04), and it only worked for dholbert. For jwir3 it seems that the new pref: general.smoothScroll.mouseWheel(.*) doesn't affect mouse wheel smooth scrolling. dholbert and jwir3 will try to figure out what happened, and post the conclusion here.

(In reply to Avi Halachmi (:avih) from comment #32) > dholbert and jwir3 will try to figure out what happened, and post the > conclusion here. It was a build configuration issue on my part. Sorry for holding this up. I pushed to inbound now: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f10a8447a5b https://hg.mozilla.org/integration/mozilla-inbound/rev/28ab6cd9175d https://hg.mozilla.org/integration/mozilla-inbound/rev/6442d65f75f6 Thanks for the contribution, Avi!

Comment on attachment 609268 [details] [diff] [review] Part 1 V2: Propagate mouse wheel event origin from ESM [Aurora Approval Request Comment] Regression caused by (bug #): 736251 (the new behavior doesn't work on OSX/Linux). User impact if declined: OSX/Linux users won't get the fix for bug 736251. Testing completed (on m-c, etc.): Tested locally - Aurora compiled with the patch applied (cleanly) and behaves as expected: Ubuntu 11.10(32), Win7(64), XP-SP3(32). Risk to taking this patch (and alternatives if risky): No concrete risks that I can think of. Worst theoretical risk is probably: slightly longer-than-expected smooth scroll animation for some input devices. String changes made by this patch: None. @roc, I'd appreciate if you could review the "Risks" part.

Oops, Correction [Aurora Approval Request Comment]: Regression caused by (bug#): 206438 (won't work on OSX/Linux) Impact: OSX/Linux users won't see the fix for bug 206438. Sorry.

https://hg.mozilla.org/mozilla-central/rev/5f10a8447a5b https://hg.mozilla.org/mozilla-central/rev/28ab6cd9175d https://hg.mozilla.org/mozilla-central/rev/6442d65f75f6

Comment on attachment 609268 [details] [diff] [review] Part 1 V2: Propagate mouse wheel event origin from ESM [Triage Comment] Approving for Aurora 13 to complete the smooth scrolling feature for OS X and Linux, since we expect that any fallout will become quickly apparent through user feedback and our own testing. If that's the case, we may need to consider backing out this change later.

https://hg.mozilla.org/releases/mozilla-aurora/rev/993e301d9bdb https://hg.mozilla.org/releases/mozilla-aurora/rev/5096a64c4c39 https://hg.mozilla.org/releases/mozilla-aurora/rev/4e89e0ae8354

Changed target milestone to mozilla13 as this has been uplifted to Aurora.

Target milestone tracks trunk landing, the status-firefox* flags track "backports".

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0 Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0 Verified that smooth scroll is applied to KB arrows, mouse scroll, PgUp/PgDn, Space, scrollbar buttons/empty space and magic mouse scroll.