737758 - Improve bug 206438 to also work on OSX and Linux

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Avi Halachmi (:avih)]

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.

Comment 1

[Avi Halachmi (:avih)]

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?

Comment 2

[Markus Stange [:mstange]]

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

Comment 3

[Timothy Nikkel (:tnikkel)]

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

Comment 4

[Timothy Nikkel (:tnikkel)]

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

Comment 5

[Markus Stange [:mstange]]

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.

Comment 6

[Timothy Nikkel (:tnikkel)]

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?

Comment 7

[Markus Stange [:mstange]]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 9

[Markus Stange [:mstange]]

Sounds good.

Comment 10

[Avi Halachmi (:avih)]

Attachment: Part 1: Propagate mouse wheel event origin from ESM

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

Comment 11

[Avi Halachmi (:avih)]

Attachment: Part 2: Address Windows abnormality (detect "faked" pixel scrolls)

Comment 12

[Avi Halachmi (:avih)]

Attachment: Part 3: Cleanups and consistency

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 14

[Avi Halachmi (:avih)]

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 15

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 16

[Avi Halachmi (:avih)]

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

[Olli Pettay [:smaug][[e-mail]]]

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 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 21

[Avi Halachmi (:avih)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 24

[Avi Halachmi (:avih)]

(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)?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree with your response to my previous comments.

Comment 26

[Avi Halachmi (:avih)]

Attachment: Part 1 V2: Propagate mouse wheel event origin from ESM

Adds {} around if bodies.

Comment 27

[Avi Halachmi (:avih)]

Attachment: Part 3 V2: Cleanups and semantic consistency

{} around the if body.

Comment 28

[Jim Mathies [:jimm]]

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.

Comment 29

[Avi Halachmi (:avih)]

Attachment: Part 2 V2: Address Windows abnormality (identify _faked_ pixel scrolls)

Readability.

Comment 30

[Avi Halachmi (:avih)]

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 31

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 32

[Avi Halachmi (:avih)]

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.

Comment 33

[Scott Johnson (:jwir3)]

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

[Avi Halachmi (:avih)]

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.

Comment 35

[Avi Halachmi (:avih)]

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.

Comment 36

[Scott Johnson (:jwir3)]

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 37

[Alex Keybl [:akeybl]]

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.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 40

[:Gavin Sharp [email: [e-mail]]]

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

Comment 41

[Mihaela Velimiroviciu (:mihaelav)]

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.