Last Comment Bug 686228 - Ability to scroll using mouse wheel on Android
: Ability to scroll using mouse wheel on Android
Status: RESOLVED FIXED
[mentor=kats][lang=java]
: helpwanted, mobile, relnote
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P3 normal with 1 vote (vote)
: Firefox 20
Assigned To: groodt
:
Mentors:
: 723800 764744 817224 829879 (view as bug list)
Depends on: 845747
Blocks: 723800
  Show dependency treegraph
 
Reported: 2011-09-11 11:13 PDT by Antoine Turmel [:GeekShadow]
Modified: 2013-04-09 00:50 PDT (History)
20 users (show)
teodora.vermesan: in‑litmus-
teodora.vermesan: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+


Attachments
Simple scrolling using PanZoomController (5.22 KB, patch)
2012-11-29 12:48 PST, groodt
bugmail: feedback+
Details | Diff | Splinter Review
Usable scrolling. Missing state checks. (7.32 KB, patch)
2012-11-29 15:53 PST, groodt
no flags Details | Diff | Splinter Review
Basic scrolling. Amount determined by DPI. (7.35 KB, patch)
2012-12-05 12:36 PST, groodt
bugmail: feedback+
Details | Diff | Splinter Review
Basic scrolling. (5.95 KB, patch)
2013-01-04 15:40 PST, groodt
bugmail: review+
Details | Diff | Splinter Review
Basic scrolling. Consumes scroll event. (5.98 KB, patch)
2013-01-05 12:38 PST, groodt
bugmail: review+
Details | Diff | Splinter Review
Simple mouse scrolling. Patch includes author information. (6.19 KB, patch)
2013-01-06 14:45 PST, groodt
bugmail: review+
Details | Diff | Splinter Review

Description Antoine Turmel [:GeekShadow] 2011-09-11 11:13:13 PDT
User Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Build ID: 20110908135051

Steps to reproduce:

On Fennec 9 running on Honeycomb 3.2 on ASUS Transformer with Keyboard dock :
Plugged a USB Mouse, and use the scroll on the mouse.


Actual results:

The web page didn't scroll.


Expected results:

The web page should scroll using a USB Mouse, just like the Honeycomb 3.2 browser.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-11 11:22:49 PDT
Sounds a lot like bug 675291 which was spun off from bug 674591
Comment 2 Antoine Turmel [:GeekShadow] 2011-12-30 08:03:22 PST
It's missing on Fennec Native as well
Comment 3 Kevin Brosnan [:kbrosnan] 2012-03-05 09:12:20 PST
Tablet will be receiving the XUL ui for the initial release.
Comment 4 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-05 10:14:13 PDT
Confirmed.  XUL and Native do not respond to mouse scroll events.
Toshiba Thrive Nightly 6/5/2012 and ESR 10.0.5
Comment 5 Aaron Train [:aaronmt] 2012-06-14 06:56:23 PDT
*** Bug 764744 has been marked as a duplicate of this bug. ***
Comment 6 corporate.joe 2012-09-14 12:11:37 PDT
ICS 4.0.4 Latest version of FF mobile still doesn't support scroll wheel from external device.

The Firefox start page displayed after opening the application is scrollable with the mouse wheel.  Browsing web pages with scroll wheel is a no-go. Other applications and the launcher also work correctly with the wheel.

Mouse is a Logitech tablet Bluetooth variety.  Android device is a Galaxy Nexus.
Comment 7 thanhdieugtv 2012-09-27 12:43:08 PDT
Mouse scrolling within the latest Firefox on an Android ICS 4.0.3 Google TV device (Geniatech ATV310) does not work as well
Logitech Wireless Keyboard and Mouse Combo MK220.
Comment 8 Matt Brubeck (:mbrubeck) 2012-10-01 11:32:38 PDT
If someone wants to help fix this, I can help out.  The main thing to do is add an OnGenericMotionListener to the LayerView object:
http://developer.android.com/reference/android/view/View.html#setOnGenericMotionListener%28android.view.View.OnGenericMotionListener%29
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/LayerView.java

As a start, the listener could just call PanZoomController methods to scroll the content directly at this point.  That would be acceptable as a first step.  But I think that ideally we would generate a Gecko event that can be handled by content scripts and by built-in scrolling code.

To implement that part, the generic motion event listener would create and send a GeckoEvent, similar to this touch event listener:
http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/mobile/android/base/GeckoApp.java#l2533

You'll need to add a new type of GeckoEvent, or extend the MOTION_EVENT type to include wheel scroll info:
http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/mobile/android/base/GeckoEvent.java#l269
http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/widget/android/AndroidJavaWrappers.cpp#l514

The event will be received here:
http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/widget/android/nsWindow.cpp#l828

Instead of calling OnMultiTouchEvent to generate an nsTouchEvent, for wheel motion we should add a new nsWindow method that generates a WheelEvent, similar to this code from the GTK version:
http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/widget/gtk2/nsWindow.cpp#l3050

There are a lot of pieces there, but I think it should be fairly straightforward once you get started.  Please let us know if you want to work on this, and if you have any questions!
Comment 9 groodt 2012-11-12 14:17:37 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #8)
> If someone wants to help fix this, I can help out.  The main thing to do is
> add an OnGenericMotionListener to the LayerView object:
> http://developer.android.com/reference/android/view/View.
> html#setOnGenericMotionListener%28android.view.View.
> OnGenericMotionListener%29
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/
> LayerView.java
> 
> As a start, the listener could just call PanZoomController methods to scroll
> the content directly at this point.  That would be acceptable as a first
> step.  But I think that ideally we would generate a Gecko event that can be
> handled by content scripts and by built-in scrolling code.
> 
> To implement that part, the generic motion event listener would create and
> send a GeckoEvent, similar to this touch event listener:
> http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/mobile/android/base/
> GeckoApp.java#l2533
> 
> You'll need to add a new type of GeckoEvent, or extend the MOTION_EVENT type
> to include wheel scroll info:
> http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/mobile/android/base/
> GeckoEvent.java#l269
> http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/widget/android/
> AndroidJavaWrappers.cpp#l514
> 
> The event will be received here:
> http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/widget/android/
> nsWindow.cpp#l828
> 
> Instead of calling OnMultiTouchEvent to generate an nsTouchEvent, for wheel
> motion we should add a new nsWindow method that generates a WheelEvent,
> similar to this code from the GTK version:
> http://hg.mozilla.org/mozilla-central/file/9e8a91dfbc7e/widget/gtk2/nsWindow.
> cpp#l3050
> 
> There are a lot of pieces there, but I think it should be fairly
> straightforward once you get started.  Please let us know if you want to
> work on this, and if you have any questions!

Im very interested in tackling this.
Comment 10 Kevin Brosnan [:kbrosnan] 2012-11-12 15:43:43 PST
Do you have a build system setup yet? That is the first step in this progress. https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec has the relevant information. It may have errors or omissions if you hit any issues with the setup anyone active in www.mibbit.com/chat/?server=irc.mozilla.org&channel=%23mobile or #mobile on irc://irc.mozilla.org should be able to assist.
Comment 11 groodt 2012-11-13 11:03:22 PST
Yes, I've managed to build and deploy Fennec successfully today. Now I need to figure out how to reproduce this bug. I'll also start getting familiar with the code mentioned above.

Should I be using any sort of IDE for the Java stuff? Or is a simple text editor the way to go?
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-11-13 11:35:35 PST
(In reply to groodt from comment #11)

> Should I be using any sort of IDE for the Java stuff? Or is a simple text
> editor the way to go?

No IDE required. We do have coding style guidelines, but any text editor can make it work.
Comment 13 rikcatau 2012-11-25 03:59:50 PST
I've been keeping an eye on this bug for some time, good to see that people are looking into it. I have no IDE system ready but here are my 2 cents:

Scrolling with the mouse wheel does work in the adress bar (when it's expanded and has many pages showing in the browser history). Also it works in the webpages overview that can be shown in the left of the screen when >6 or so pages are loaded. It just does not work in the main screen.

BTW this is the main thing making me revert to the stock browser in most occasions
Comment 14 groodt 2012-11-25 09:44:39 PST
Thanks for the info. 

I've had a busy couple of weeks, so not had a chance to work on this yet. Also, my bluetooth mouse died, so not been ideal. I hope to start work on this properly this week (I've got a new mouse on the way).
Comment 15 groodt 2012-11-26 11:55:17 PST
I've got a working bluetooth mouse again. I'm ready to try and tackle this.

:mbrubeck I'm looking to add the simple OnGenericMotionListener as a starting point.
Comment 16 groodt 2012-11-26 12:59:14 PST
I've been able to grab the ACTION_SCROLL MotionEvent and I'm able to extract the X and Y axis values, so in theory, I should be able to implement scrolling of the view.

:mbrubeck I'm not sure what to do now. You mention I can scroll by using the PanZoomController, but I don't follow.

I've found that Class, but the only public methods seem to deal with TouchEvents. Does anybody have any pointers for me?
Comment 17 Wesley Johnston (:wesj) 2012-11-26 13:01:58 PST
You may have to expose something new, but kats is a good guy to talk to about that stuff. In order to make the animation smooth, we might want to try and use animatedZoomTo(rect) or a modified version of it for scrolling.
Comment 18 Kartikaya Gupta (email:kats@mozilla.com) 2012-11-26 13:40:45 PST
(In reply to groodt from comment #16)
> I've been able to grab the ACTION_SCROLL MotionEvent and I'm able to extract
> the X and Y axis values, so in theory, I should be able to implement
> scrolling of the view.
> 
> :mbrubeck I'm not sure what to do now. You mention I can scroll by using the
> PanZoomController, but I don't follow.
> 
> I've found that Class, but the only public methods seem to deal with
> TouchEvents. Does anybody have any pointers for me?

It sounds like what you want to be doing is passing the MotionEvent directly into the PanZoomController using the same code path that the existing MotionEvents take (i.e. pass it into mTouchEventHandler from LayerView). Then, once you have the events coming in to PanZoomController's onTouchEvent method, add another case to that switch statement to handle the ACTION_SCROLL event type. As a first cut I would just try calling the scrollBy(dx, dy) method in PanZoomController and see if that has the desired effect. Once that's working we can look at making sure it works in all cases (i.e. for all values of PanZoomController's mState variable). Does that make sense?
Comment 19 groodt 2012-11-27 01:58:07 PST
Yes, that makes sense. I'll give it a try tonight.
Comment 20 groodt 2012-11-29 12:39:11 PST
Ok, I've got very basic scrolling working. Should I post a diff somewhere for people to look at?

Two problems with it right now:
1. It is possible to scroll the content off the screen.
2. The amount scrolled seems very small. Requires a lot of scrolling to move the content.
Comment 21 groodt 2012-11-29 12:41:45 PST
If I should rather take a look at generating a GECKO_EVENT as mbrubeck mentions above, let me know.

I figured baby steps would be best for me, since Im new to the codebase and pretty new to Android, that's why I went for the PanZoomController approach.
Comment 22 groodt 2012-11-29 12:48:37 PST
Created attachment 686754 [details] [diff] [review]
Simple scrolling using PanZoomController
Comment 23 Kartikaya Gupta (email:kats@mozilla.com) 2012-11-29 13:39:43 PST
Comment on attachment 686754 [details] [diff] [review]
Simple scrolling using PanZoomController

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

This is looking pretty good! About the being able to pan the page off-screen, that should be fixable with a call to bounce() after the scrollBy call. The bounce() call will figure out if the new viewport is in overscroll and do an animation to bring it back out of overscroll.

The other thing that will need to be done is more state-checking in the onScroll function. I don't have a bluetooth mouse (or an android device that supports it) so I can't really play around with the interaction between touch panning actions and mouse wheel actions. What should be the expected behaviour, for example, if you start panning with your finger and while you're in the middle of that, you roll the mouse wheel? I would imagine that the mouse wheel action should do nothing there, because otherwise the page will slide under your finger unexpectedly. So for this case, if mState is TOUCHING, PANNNING, PANNING_LOCKED, PANNING_HOLD, PANNING_HOLD_LOCKED, or PINCHING then you want onScroll to do nothing. See if you can figure out what the best behaviour is for the other states as well. Hopefully it won't be too complicated - the hardest part will be understanding what the different states mean - see the documentation on the PanZoomState enum for some help there, and feel free to ask questions if anything is unclear.

::: mobile/android/base/gfx/LayerView.java
@@ +105,5 @@
>          mInputConnectionHandler = null;
>  
>          setFocusable(true);
>          setFocusableInTouchMode(true);
> +        setOnGenericMotionListener(new GenericMotionListener());

You should just be able to override View.onGenericMotionEvent instead of doing it this way. That is simpler and also has the advantage of gracefully falling back to Android API levels before 12, which is when this API was introduced.

::: mobile/android/base/gfx/TouchEventHandler.java
@@ +259,5 @@
>  
> +    private boolean isScrollEvent(MotionEvent event) {
> +        int action = (event.getAction() & MotionEvent.ACTION_MASK);
> +        return (action == MotionEvent.ACTION_SCROLL);
> +    }

This will need an API level check, since Motion.ACTION_SCROLL is only in API level 12 and higher. Basically you want this function to return false if Build.VERSION.SDK_INT <= 11.
Comment 24 Kartikaya Gupta (email:kats@mozilla.com) 2012-11-29 13:44:27 PST
(In reply to groodt from comment #20)
> 2. The amount scrolled seems very small. Requires a lot of scrolling to move
> the content.

According to the AXIS_VSCROLL and AXIS_HSCROLL documentation, the value is a normalized value between -1.0 and 1.0. So you probably want to multiply this value by something (maybe screen height/width?) to get more meaningful scroll amounts.
Comment 25 groodt 2012-11-29 13:50:48 PST
(In reply to Kartikaya Gupta (:kats) from comment #23)
> Comment on attachment 686754 [details] [diff] [review]
> Simple scrolling using PanZoomController
> 
> Review of attachment 686754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking pretty good! About the being able to pan the page
> off-screen, that should be fixable with a call to bounce() after the
> scrollBy call. The bounce() call will figure out if the new viewport is in
> overscroll and do an animation to bring it back out of overscroll.
> 
> The other thing that will need to be done is more state-checking in the
> onScroll function. I don't have a bluetooth mouse (or an android device that
> supports it) so I can't really play around with the interaction between
> touch panning actions and mouse wheel actions. What should be the expected
> behaviour, for example, if you start panning with your finger and while
> you're in the middle of that, you roll the mouse wheel? I would imagine that
> the mouse wheel action should do nothing there, because otherwise the page
> will slide under your finger unexpectedly. So for this case, if mState is
> TOUCHING, PANNNING, PANNING_LOCKED, PANNING_HOLD, PANNING_HOLD_LOCKED, or
> PINCHING then you want onScroll to do nothing. See if you can figure out
> what the best behaviour is for the other states as well. Hopefully it won't
> be too complicated - the hardest part will be understanding what the
> different states mean - see the documentation on the PanZoomState enum for
> some help there, and feel free to ask questions if anything is unclear.
> 
> ::: mobile/android/base/gfx/LayerView.java
> @@ +105,5 @@
> >          mInputConnectionHandler = null;
> >  
> >          setFocusable(true);
> >          setFocusableInTouchMode(true);
> > +        setOnGenericMotionListener(new GenericMotionListener());
> 
> You should just be able to override View.onGenericMotionEvent instead of
> doing it this way. That is simpler and also has the advantage of gracefully
> falling back to Android API levels before 12, which is when this API was
> introduced.
> 
> ::: mobile/android/base/gfx/TouchEventHandler.java
> @@ +259,5 @@
> >  
> > +    private boolean isScrollEvent(MotionEvent event) {
> > +        int action = (event.getAction() & MotionEvent.ACTION_MASK);
> > +        return (action == MotionEvent.ACTION_SCROLL);
> > +    }
> 
> This will need an API level check, since Motion.ACTION_SCROLL is only in API
> level 12 and higher. Basically you want this function to return false if
> Build.VERSION.SDK_INT <= 11.

Thanks. I'll add bounce(), inherit from View.onGenericMotionEvent and add API level checks, then take a look at more sophisticated state checks.
Comment 26 groodt 2012-11-29 15:53:29 PST
Created attachment 686847 [details] [diff] [review]
Usable scrolling. Missing state checks.

Added bounce() and API Level checks. Using magic number of 16 for scroll factor as I couldn't think of a sensible way to calculate it based on screen dimensions.

I would copy the approach from Androids ScrollView, but I don't understand what it does:
    /**
     * Gets a scale factor that determines the distance the view should scroll
     * vertically in response to {@link MotionEvent#ACTION_SCROLL}.
     * @return The vertical scroll scale factor.
     * @hide
     */
    protected float getVerticalScrollFactor() {
        if (mVerticalScrollFactor == 0) {
            TypedValue outValue = new TypedValue();
            if (!mContext.getTheme().resolveAttribute(
                    com.android.internal.R.attr.listPreferredItemHeight, outValue, true)) {
                throw new IllegalStateException(
                        "Expected theme to define listPreferredItemHeight.");
            }
            mVerticalScrollFactor = outValue.getDimension(
                    mContext.getResources().getDisplayMetrics());
        }
        return mVerticalScrollFactor;
    }


I still need to add some state-checking in onScroll to handle the interactions between scrolling and panning.
Comment 27 Kartikaya Gupta (email:kats@mozilla.com) 2012-11-29 17:01:39 PST
The Android ScrollView code looks like it basically uses the height of a list item as the scroll amount. It pulls it from the theme data. Using a constant instead is fine by me, but I would rather the constant be relative to the screen size than just a fixed number of pixels. This is because different displays have different pixel densities, and so 16 pixels might be a lot on one device but very little on another (e.g. the Nexus 10). I would probably define MAX_SCROLL to be some multiple of GeckoAppShell.getDpi() which will make it scale a little better to different screen densities.
Comment 28 groodt 2012-11-30 08:20:09 PST
I agree that something like DPI as a factor is a good approach.

Now, any ideas for what a suitable base value would be? Or a way to calculate one based on screen dimensions? em-height or something?

I kind of like the ScrollView approach of using the height of a list item too.
Comment 29 Kartikaya Gupta (email:kats@mozilla.com) 2012-11-30 10:10:25 PST
(In reply to groodt from comment #28)
> I agree that something like DPI as a factor is a good approach.
> 
> Now, any ideas for what a suitable base value would be? Or a way to
> calculate one based on screen dimensions? em-height or something?

The way I would do it is set MAX_SCROLL = x * GeckoAppShell.getDpi() and then adjust x until it is a reasonable amount on your device. (If MAX_SCROLL = 16 was working well for you, then set x to whatever 16 / GeckoAppShell.getDpi() is). That way it should scroll by around the same amount (in real-world units, like cm) on all devices.

You could also grab a DisplayMetrics object and make it some percentage of the screen height, but the screen height can change (e.g. on rotation) and so that is probably less reliable.
Comment 30 Kartikaya Gupta (email:kats@mozilla.com) 2012-11-30 23:27:12 PST
*** Bug 817224 has been marked as a duplicate of this bug. ***
Comment 31 groodt 2012-12-05 12:36:48 PST
Created attachment 688910 [details] [diff] [review]
Basic scrolling. Amount determined by DPI.

Amount scrolled is determined by device DPI
Comment 32 groodt 2012-12-05 12:41:59 PST
Scrolling now works well on my Nexus 7. 

I still need to take a look at state management so things don't get confused when swiping and scrolling at the same time. I imagine this is an edge case though. Im on vacation for a week from tomorrow, so will be going off the grid. I should be able to take a look at the state management when I get back, but others are welcome to take a look and use my patch as a starting point.

Otherwise, I guess it would be possible to use the patch as is, and implement the state management later. I guess this would be a judgement call made by one of the committers. I imagine this bug isn't very high priority anyway, so we may as well fix it properyly.
Comment 33 Kartikaya Gupta (email:kats@mozilla.com) 2012-12-05 13:12:49 PST
Comment on attachment 688910 [details] [diff] [review]
Basic scrolling. Amount determined by DPI.

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

Looking good so far. And I agree that this bug isn't super high priority so I'm fine waiting a week for you to come back and finish it. Have a good vacation!

::: mobile/android/base/gfx/LayerView.java
@@ +41,5 @@
>   * A view rendered by the layer compositor.
>   *
>   * Note that LayerView is accessed by Robocop via reflection.
>   */
> +public class LayerView extends FrameLayout implements View.OnGenericMotionListener {

The View base class already has an onGenericMotionEvent function, so I don't think we need this extra "implements" clause or the setOnGenericMotionListener call (also these pieces will be troublesome on API level <= 11).
Comment 34 Mark Finkle (:mfinkle) (use needinfo?) 2012-12-05 21:03:15 PST
Looks like Kats is mentoring here.
Comment 35 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-04 12:45:37 PST
Are you still working on this bug?
Comment 36 groodt 2013-01-04 12:57:33 PST
Yes, unless somebody else wants to finish the last bit. I should get some time to work on this over the weekend. Been busy catching up with things at the beginning of the new year.
Comment 37 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-04 13:00:05 PST
Ok, thanks. I just wanted to check since it's been a while. I'd like to get this in soonish as I'm also planning to add support for another input device and I'll be able to reuse this code.
Comment 38 groodt 2013-01-04 13:59:53 PST
Yes, I agree. I've left this far too long.

As far as I can tell, the only thing left is to sort out the state management. Now I might be misunderstanding things, but I think it could be as simple as only allowing scrolling when mState = NOTHING. If mState is in any other condition, then we know something else is happening and we probably don't want to allow mouse scrolling as well. Or am I completely missing something?
Comment 39 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-04 14:34:32 PST
That sounds right. We may also want to allow mouse scrolling in the FLING state (this is when the page is still moving after you do a pan). That's just my impression based on how long flings sometimes take to finish; I can imagine myself starting a fling with my finger and then moving to the mouse to scroll some more, and having to wait until the fling is done would be annoying. Either way is fine by me though; I don't have a device to try this out so I'll defer to your judgement on that :)
Comment 40 groodt 2013-01-04 15:40:27 PST
Created attachment 698124 [details] [diff] [review]
Basic scrolling.

Only allow scrolling when mState is NOTHING or FLING to prevent state confusion during touch events like panning, zooming, touching.

I actually found that on my Nexus 7, when touch events were active that I wasn't able to use the mouse scroll anyway, but I've added the guard anyway.
Comment 41 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-05 06:51:25 PST
Comment on attachment 698124 [details] [diff] [review]
Basic scrolling.

>diff --git a/mobile/android/base/ui/PanZoomController.java b/mobile/android/base/ui/PanZoomController.java
>+    private boolean onScroll(MotionEvent event) {
>+        if (mState == PanZoomState.NOTHING || mState == PanZoomState.FLING) {
>+            float scrollX = event.getAxisValue(MotionEvent.AXIS_HSCROLL);
>+            float scrollY = event.getAxisValue(MotionEvent.AXIS_VSCROLL);
>+
>+            scrollBy(scrollX * MAX_SCROLL, scrollY * MAX_SCROLL);
>+            bounce();

I don't think the return value here is used but it's probably better to return true if we do the scroll to indicate we consumed the event. Other than that this patch looks great!
Comment 42 groodt 2013-01-05 12:38:17 PST
Created attachment 698329 [details] [diff] [review]
Basic scrolling. Consumes scroll event.
Comment 43 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-06 14:06:23 PST
Just realized your patch is missing author info/commit message. Please follow the instructions at https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in to create a patch and I can land it for you.
Comment 44 groodt 2013-01-06 14:45:10 PST
Created attachment 698479 [details] [diff] [review]
Simple mouse scrolling. Patch includes author information.

Sorry about that. The Mercurial MQ stuff fries my brain. I can see it is powerful, but Im used to git. I get into all sorts of weird states with MQ. It looks like I managed to create a patch correctly though.
Comment 45 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-06 15:31:29 PST
Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/43ceabecd922

Btw, if you prefer working in git you can export patches from git in MQ-style with this command:

git show -M -C --binary --format="# HG changeset patch%n# User %an <%ae>%n%B" -U8 <cset>
Comment 46 Phil Ringnalda (:philor) 2013-01-06 21:53:24 PST
https://hg.mozilla.org/mozilla-central/rev/43ceabecd922
Comment 47 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-11 21:32:13 PST
*** Bug 829879 has been marked as a duplicate of this bug. ***
Comment 48 Matt Brubeck (:mbrubeck) 2013-03-07 17:13:56 PST
Should we mention this new feature in the Firefox 20 release notes?
Comment 49 Brion Vibber 2013-03-15 17:09:05 PDT
*** Bug 723800 has been marked as a duplicate of this bug. ***
Comment 50 Teodora Vermesan (:TeoVermesan) 2013-04-09 00:50:36 PDT
Test Case added in the Full Functional Tests Suite:
https://moztrap.mozilla.org/manage/case/7405/

Note You need to log in before you can comment on or make changes to this bug.