Ability to scroll using mouse wheel on Android

RESOLVED FIXED in Firefox 20

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: GeekShadow, Assigned: groodt)

Tracking

({helpwanted, mobile, relnote})

Trunk
Firefox 20
ARM
Android
helpwanted, mobile, relnote
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus -
in-moztrap +

Firefox Tracking Flags

(blocking-fennec1.0 -, fennec+)

Details

(Whiteboard: [mentor=kats][lang=java])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Keywords: helpwanted, mobile
OS: Linux → Android
Hardware: x86 → ARM
Sounds a lot like bug 675291 which was spun off from bug 674591

Updated

6 years ago
Priority: -- → P3
(Reporter)

Comment 2

6 years ago
It's missing on Fennec Native as well
Component: General → General
Product: Fennec → Fennec Native
tracking-fennec: --- → 11+
Tablet will be receiving the XUL ui for the initial release.
blocking-fennec1.0: --- → -
Confirmed.  XUL and Native do not respond to mouse scroll events.
Toshiba Thrive Nightly 6/5/2012 and ESR 10.0.5
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
Duplicate of this bug: 764744

Comment 6

5 years ago
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

5 years ago
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.
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!
Summary: Ability to scroll webpage using physical mouse on Honeycomb → Ability to scroll using mouse wheel on Android
Whiteboard: [mentor=mbrubeck][lang=java][lang=cpp]
Blocks: 723800
tracking-fennec: 11+ → ?
tracking-fennec: ? → +
(Assignee)

Comment 9

5 years ago
(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.
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.
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 11

5 years ago
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?
(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

5 years ago
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
(Assignee)

Comment 14

5 years ago
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).
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
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?
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.
(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?
(Assignee)

Comment 19

5 years ago
Yes, that makes sense. I'll give it a try tonight.
(Assignee)

Comment 20

5 years ago
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.
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Comment 22

5 years ago
Created attachment 686754 [details] [diff] [review]
Simple scrolling using PanZoomController
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.
Attachment #686754 - Flags: feedback+
(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.
(Assignee)

Comment 25

5 years ago
(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.
(Assignee)

Comment 26

5 years ago
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.
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.
(Assignee)

Comment 28

5 years ago
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.
(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.
Duplicate of this bug: 817224
(Assignee)

Comment 31

5 years ago
Created attachment 688910 [details] [diff] [review]
Basic scrolling. Amount determined by DPI.

Amount scrolled is determined by device DPI
Attachment #688910 - Flags: feedback?(bugmail.mozilla)
(Assignee)

Comment 32

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #686754 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #686847 - Attachment is obsolete: true
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).
Attachment #688910 - Flags: feedback?(bugmail.mozilla) → feedback+
Looks like Kats is mentoring here.
Whiteboard: [mentor=mbrubeck][lang=java][lang=cpp] → [mentor=kats][lang=java][lang=cpp]
Whiteboard: [mentor=kats][lang=java][lang=cpp] → [mentor=kats][lang=java]
Assignee: nobody → groodt
Are you still working on this bug?
(Assignee)

Comment 36

5 years ago
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.
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.
(Assignee)

Comment 38

5 years ago
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?
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 :)
(Assignee)

Comment 40

5 years ago
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.
Attachment #688910 - Attachment is obsolete: true
Attachment #698124 - Flags: review?(bugmail.mozilla)
Attachment #698124 - Attachment is patch: true
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!
Attachment #698124 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 42

5 years ago
Created attachment 698329 [details] [diff] [review]
Basic scrolling. Consumes scroll event.
Attachment #698124 - Attachment is obsolete: true
Attachment #698329 - Flags: review?(bugmail.mozilla)
Attachment #698329 - Flags: review?(bugmail.mozilla) → review+
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.
(Assignee)

Comment 44

5 years ago
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.
Attachment #698329 - Attachment is obsolete: true
Attachment #698479 - Flags: review?(bugmail.mozilla)
Attachment #698479 - Flags: review?(bugmail.mozilla) → review+
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>
https://hg.mozilla.org/mozilla-central/rev/43ceabecd922
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Duplicate of this bug: 829879

Updated

4 years ago
Depends on: 845747
Should we mention this new feature in the Firefox 20 release notes?
Keywords: relnote

Updated

4 years ago
Duplicate of this bug: 723800
Flags: in-litmus?(fennec)
Test Case added in the Full Functional Tests Suite:
https://moztrap.mozilla.org/manage/case/7405/
Flags: in-moztrap+
Flags: in-litmus?(fennec)
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.