|
5.13 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Android; Tablet; rv:19.0) Gecko/19.0 Firefox/19.0 Build ID: 20130215125331 Steps to reproduce: I am using a mouse on my Asus TF300T. Actual results: The scrolling wheel does nothing. Expected results: It should scroll the page.
|
(Reporter) | |
Comment 1•5 years ago
|
||
It does work in Firefox Beta (20) but the scrolling is inversed.
|
||
Comment 2•5 years ago
|
||
Let's accurately change the summary then
|
||
Comment 3•5 years ago
|
||
Greg, does the mouse wheel scrolling still work fine for you? I'm wondering if our code is negated or if the device Pierre is using is sending events that are negated compared to yours.
It is working fine for me on my Nexus 7 with a bluetooth mouse attached. I'll do a build of HEAD, but I don't imagine anything has changed in this area. I also suspect a device issue. It could also be a personal preference I guess. The Asus TF300T has a 2 finger touch scroll I think. So maybe natural scrolling feels "wrong"? Or alternatively my perspective of what scroll wheel should do is inverted because I've been using natural scroll on a Mac for so long. Pierre, are you scrolling with your fingers or a physical mouse? When you move your 2 fingers up, in which direction would you expect the page to scroll? Is Firefox behaving differently to other applications on this device?
|
|
(Reporter) | |
Comment 5•5 years ago
|
||
When I scroll with the touchpad, the page moves in the same direction as my fingers. This is the same behavior as with other applications. When I scroll with an USB mouse, the page does not move in the same direction as the wheel. Firefox seems to be the only application to do it this way.
|
|
||
Comment 6•5 years ago
|
||
On my Linux desktop when I scroll with a mouse wheel, rolling the mouse wheel "up" (rolling the top away from me) makes the page go "down" (in that I end up at the top of the page if I go far enough). This is the opposite of what I would expect on a touch screen interaction mode, where the page would move in the same direction as my fingers. So it sounds to me like this is working as expected.
|
|
||
Comment 7•5 years ago
|
||
> So it sounds to me like this is
> working as expected.
Well, as *I* expect it to work, without having actually used it. Maybe upon actually using it I would find it weird.|
|
(Reporter) | |
Comment 8•5 years ago
|
||
The thing is I expect my mouse to work the same way on my computer and on my tablet (moreover I expect all my Android applications to behave in the same way.). But if people have different expectations, I guess the best would be to have this as a preference.
My expectations. Mouse scroll wheel: Wheel scrolled away (up) -> scrollbar travels up -> page travels down -> end up at top of page. 2 finger scroll gestures: Fingers pushed away (up) -> scrollbar travels down -> page travels up -> end up at bottom of page. I wonder if the device treats the touchpad differently to a mouse, or if they are all touch events? Did your touchpad work before Firefox 20?
|
|
(Reporter) | |
Comment 10•5 years ago
|
||
No, it does not work at all with Firefox 19.
|
|
(Reporter) | |
Comment 11•5 years ago
|
||
Sorry, I did not read you correctly. The touchpad does work with Firefox 19 but the mouse wheel does not.
|
|
||
Comment 12•5 years ago
|
||
Making it a pref sounds like a reasonable approach. It should be simple enough and would make a good first bug for somebody to tackle.
|
||
Comment 13•5 years ago
|
||
Sounds sensible. If enough people think I've got it inverted, then we can change the default to be the other way around as well I guess.
|
||
Comment 14•4 years ago
|
||
Do you have to be able to reproduce these bugs to work on them? I don't have a tablet I could test this or possible fixes on but would love to be able to work on this.
|
|
||
Comment 15•4 years ago
|
||
It would be good if you could test the patch you write, yes. However I don't think you need a tablet for it - I think if you have a bluetooth mouse and an android device you should be able to pair the mouse to the device and use it as an input and test the patch that way.
|
||
Comment 16•4 years ago
|
||
Is anyone working on this at the moment? Can I take it on?
|
|
||
Comment 17•4 years ago
|
||
If so, can anyone suggest a possible emulator? I have a Sony Xperia that doesn't have a mouse.
|
|
||
Comment 18•4 years ago
|
||
You're welcome to take on this bug. The first step is to get your build environment set up as described at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec. If you're testing on an emulator then there are instructions on how to do that at https://wiki.mozilla.org/Mobile/Fennec/Android/Emulator However for this bug it would be useful to be able to hook up a mouse to your android device, as I don't think mouse inputs are supported in the emulator. If you want you can go through the steps above and build Fennec and run it on the emulator to see - you'll have to do most of that anyway to work on any Fennec bugs so even if you end up switching to another bug it won't be wasted work.
|
|
||
Comment 19•4 years ago
|
||
Do I need any particular mouse? Would any USB mouse do?
|
|
||
Comment 20•4 years ago
|
||
As long as you can use it on your Android device, any mouse should do fine.
|
|
||
Comment 21•4 years ago
|
||
Ok, do I need a specific cable or connector to get a normal USB mouse to work on my phone.
|
|
||
Comment 22•4 years ago
|
||
Ok, do I need a specific cable or connector to get a normal USB mouse to work on my phone?
|
|
||
Comment 23•4 years ago
|
||
Please don't 'bump' bugs by repeating the same comment twice.
|
|
||
Comment 24•4 years ago
|
||
I've never connected a USB mouse to an Android device, so I can't help you there. Sorry.
|
|
||
Comment 25•4 years ago
|
||
I am now no longer working on this bug.
|
||
Comment 26•4 years ago
|
||
The code which handles the scroll wheel on Android is in mobile/android/base/gfx/JavaPanZoomController.java handlePointerScroll(MotionEvent event) These three lines read the scroll wheel event values, and pass it to scrollBy: float scrollX = event.getAxisValue(MotionEvent.AXIS_HSCROLL); float scrollY = event.getAxisValue(MotionEvent.AXIS_VSCROLL); scrollBy(scrollX * MAX_SCROLL, scrollY * MAX_SCROLL); Documentation on those values are found here: http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_HSCROLL -1.0 (left) to 1.0 (right) http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_VSCROLL -1.0 (down) to 1.0 (up) Notice that scroll down is a NEGATIVE value, so scrollY needs to be negated before being passed to scrollBy. scrollBy(scrollX * MAX_SCROLL, -scrollY * MAX_SCROLL);
|
|
||
Comment 27•4 years ago
|
||
See the discussion in comments 4-9. People can have different expectations when using different scroll devices (not to mention that different hardware manufacturers can have their own ideas about what actions should map to what), so making the scroll direction controlled by a pref seems like the best option.
|
|
||
Comment 28•4 years ago
|
||
Mozilla normally adopts whatever the platform standard is, not the arbitrary preference of some developers who are more familiar with other platforms. Android Browser, Chrome, settings pages, other system apps and even Android Firefox settings all scroll the opposite direction from the current Firefox for Android. Configuration is nice, but to Android centric users the current direction appears broken. So far, I've seen only mention of tablets where a mouse may be a secondary pointer device. Where this is a serious issue is for all those hot selling Android stick computers where a mouse is often the primary pointing device. Leaving it broken to someday add a configuration option when the fix is known and trivial is disappointing.
|
|
||
Comment 29•4 years ago
|
||
(In reply to John Bridges from comment #28) > Leaving it broken to someday add a configuration option when the fix is > known and trivial is disappointing. If you feel strongly about this issue you're more than welcome to contribute a patch for it. I'd be happy to accept a patch that flips the default direction in addition to adding the pref, which should satisfy your requirements as well as that of others who have commented in this bug. I'd be happy to guide you through building Fennec and contributing the patch.
|
||
Comment 30•4 years ago
|
||
Hello, I am a complete newbie to open source software, and I was wondering if you could tell me how to get started. I have a working knowledge of Java, and would love to help however possible. I am also currently learning Objective C and iPhone development. Please point me in the right direction! Thanks a lot!
|
|
||
Comment 31•4 years ago
|
||
Hi Arjun! Glad to see you're interested in working on this bug. I also noticed you posted over in bug 893289 - I would recommend picking one of the two bugs to work on rather than trying to tackle both. Whichever one you choose to go with, the first steps are pretty much the same - follow the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android to get your build environment set up and make sure you can load a local build onto a device. Once you can do that post here (or in bug 893289) and we can point you to the relevant code and how to get started with the actual bug-fixing!
|
(Assignee) | |
Comment 32•4 years ago
|
||
I would like to agree with comment 28, I believe the current scroll direction is broken for the mouse scroll wheel in Firefox for Android. Reversing the action of the scroll wheel in Firefox for Android would bring it in line with the other apps I have installed on my Droid Razr Maxx. As an added comment: While using click-hold-drag with the mouse the screen scrolls as I would expect, dragging any direction drags the screen in the direction of travel.
|
|
(Assignee) | |
Comment 33•4 years ago
|
||
I have gone to: https://wiki.mozilla.org/Mobile/Fennec/Android and believe I am close to getting things set up to build. I have not tried yet. I have installed: Java jdk-1.7.0_25-fcs.i586 android-ndk-r8e-linux-x86 android-sdk_r22.0.4-linux (and updated a ton of files) Updated: mercurial to 2.6.3-97.1 ia32-libs and ccache are installed Have pulled the source with: hg clone http://hg.mozilla.org/mozilla-central/ src I have read everything relating to setting up the environment to getting the source. Now I have a couple questions. The Wiki page cited above is geared more to Ubuntu. I am using openSuse 12.3. Question 1: Is there an equivalent for the line: sudo apt-get build-dep firefox Question 2: Do I need more of a mozconfig than: # Add the correct paths here: ac_add_options --with-android-ndk="$HOME/opt/android-ndk-r8e" ac_add_options --with-android-sdk="$HOME/opt/android-sdk-linux/platforms \ /android-16" # android options ac_add_options --enable-application=mobile/android ac_add_options --target=arm-linux-androideabi mk_add_options MOZ_OBJDIR=./objdir-droid (a working mozconfig with notes form someone might be very beneficial to me) Question 3: is android-16 the correct platform in the above mozconfig? Or should it be android-18 I will have some build questions after the items above have been resolved. Is this the right place for these questions or should I be asking somewhere else?
|
|
||
Comment 34•4 years ago
|
||
I suggest contacting developers such as Kartikaya (kats) over in #mobile on irc.mozilla.org (https://wiki.mozilla.org/IRC) to discuss build troubleshooting and to keep comments in this bug related towards resolution of the issue described in comment #0.
|
||
Comment 35•4 years ago
|
||
For the build dependencies see -> https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Linux_Prerequisites#openSUSE.2FSUSE_Linux_Enterprise
|
|
||
Comment 36•4 years ago
|
||
(In reply to Dennis from comment #33) > The Wiki page cited above is geared more to Ubuntu. I am using openSuse 12.3. > > Question 1: Is there an equivalent for the line: > sudo apt-get build-dep firefox See comment 35 for the equivalent. > Question 2: Do I need more of a mozconfig than: > > # Add the correct paths here: > ac_add_options --with-android-ndk="$HOME/opt/android-ndk-r8e" > ac_add_options --with-android-sdk="$HOME/opt/android-sdk-linux/platforms \ > /android-16" > > # android options > ac_add_options --enable-application=mobile/android > ac_add_options --target=arm-linux-androideabi > > mk_add_options MOZ_OBJDIR=./objdir-droid > > (a working mozconfig with notes form someone might be very beneficial to me) That should be sufficient, provided you fill in the correct paths as needed. The mozconfig I use for debug android builds is at https://github.com/staktrace/moz-scripts/blob/master/mozconfig.Linux-android-debug if you want to see, but it has stuff that you shouldn't strictly need. > Question 3: is android-16 the correct platform in the above mozconfig? Or > should it be android-18 If you have android-18 installed you should be able to use that. The mozconfig was probably last updated when 16 was the latest version. > I will have some build questions after the items above have been resolved. > Is this the right place for these questions or should I be asking somewhere > else? IRC is better, but you can also email me if you like.
|
|
(Assignee) | |
Comment 37•4 years ago
|
||
My build environment is set up and I am now able to build fennec (thank you kats) and run the .apk on my Motorola Razr Maxx. During another IRC session kats suggested I negate scrollY (as outlined in comment 26 , thank you John Bridges). I have edited mobile/android/base/gfx/JavaPanZoomController.java and negated scrollY, rebuilt the java code, rebuilt the fennec .apk and installed it on my Motorola Razr Maxx. The mouse scroll wheel now functions just as it would in a desktop environment. Everything else seems to work as it did before, ie: the click and drag drags the screen in the direction of movement, touch and drag on the touchscreen drags the screen in the direction of movement. I have found no ill effects from negating scrolly. This change now brings the fennec scroll wheel function in line with the other apps mouse scroll wheel function on my razr maxx. If the negated scrollY could be made the default I think it would be good. Please direct me to what, if anything, I should do next.
|
|
||
Comment 38•4 years ago
|
||
That's great! I'm glad you were able to make that change and verify that it has the expected behaviour. For the actual patch though we want to make that direction preffable so that anybody can change it at runtime from their about:config options. To do this, you'll need to add a new pref and hook it up. The code changes you will need to make are: 1) Add a new boolean pref to the file at [1]. Call it something like "ui.scrolling.negate_wheel" or something similar. When this boolean is set to true, we will invert the scroll direction from mouse wheels. 2) Update the code at [2] so that it fetches the new pref value as well. Currently the code is calling the PrefsHelper function at [3] but you'll have to change it to call the one at [4] and update the callback implementation to handle the new pref as well. Have the callback store the pref value in a field in JavaPanZoomController 3) Do the negation of scrollY based on the value of the pref. Does that make sense? If you can code that up, follow the instructions at [5] to generate a patch and attach it to this bug. If you have any questions feel free to ask here or on IRC/email. [1] https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/app/mobile.js#l654 [2] https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/base/gfx/JavaPanZoomController.java#l153 [3] https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/base/PrefsHelper.java#l29 [4] https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/base/PrefsHelper.java#l35 [5] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
|
|
||
Comment 39•4 years ago
|
||
Why would anyone ever want the scroll wheel to function the opposite of other parts of FireFox for Android that use Android UI controls? For example the "top sites" and "Settings" pages both scroll the correct direction with the mouse wheel because they do not use the FireFox's broken handlePointerScroll. This is a bug. The default should be changed to be consistent with the rest of Firefox. Adding a feature to enable this bug to persist as an option makes no sense.
|
|
||
Comment 40•4 years ago
|
||
There are devices other that mouse scroll wheels that go through our handlePointerScroll code. Unless you have personally tested every Android device in existence with every possible peripheral device please do not make claims about what other people may or may not want.
|
|
||
Comment 41•4 years ago
|
||
The results of AXIS_VSCROLL are ONLY for scrolling devices, not for position. So this has no effect on peripheral devices that do not have a scroll wheel or emulate a scroll wheel.
To quote from the Android documentation:
> Some pointing devices such as mice may support vertical and/or
> horizontal scrolling. A scroll event is reported as a generic
> motion event with ACTION_SCROLL that includes the relative scroll
> offset in the AXIS_VSCROLL and AXIS_HSCROLL axes.
Do you have proof some some exotic device that somehow modifies the standard UI controls so they use a reverse value from AXIS_VSCROLL, but do not reverse the AXIS_VSCROLL value provided to FireFox? That sounds rather exotic. The only reason I can think of such a exotic device would be to work around Firefox's current bug.
For "top sites" and "Settings" pages both to scroll the opposite direction of the rest of Firefox is a bug. A bug that affects every single mouse user on Android.
I see no point in leaving this bug in, or adding a configuration option to keep this buggy behavior when there is not any hint of who specifically wants this "scroll differently in different parts of FireFox" feature.|
|
||
Comment 42•4 years ago
|
||
The Asus Transformer dock as I recall was the first reason we implemented this feature. It has the reverse scrolling that you believe to be non-existent so we do need to support both cases.
|
|
||
Comment 43•4 years ago
|
||
(In reply to Kevin Brosnan [:kbrosnan] from comment #42) > The Asus Transformer dock as I recall was the first reason we implemented > this feature. It has the reverse scrolling that you believe to be > non-existent so we do need to support both cases. The ASUS just inverted the scroll direction everywhere, not specifically Firefox or specific UI elements. The inverted nature was a bug ASUS fixed in a firmware update in 2011: http://www.transformerforums.com/forum/asus-transformer-tf101-general-discussions/5329-how-do-you-fix-inverted-scrolling-touchpad.html Even on a ASUS transformer without the updated firmware or other device with an inverted scroll it is still inconsistent in "top sites" and "Settings" vs the main browser. Why would you ever want the main browser to scroll the opposite of "top sites" and "settings" on any device?
|
|
||
Comment 44•4 years ago
|
||
John, why do you care so much if we have a pref for it or not? As long as the default direction is flipped what difference does it make to you if there is a pref to flip it back? There are hundreds of prefs in Gecko for all sorts of things that I'm sure you never use, why are you spending so much effort (and delaying this bug from even getting fixed at all) to argue against this one pref?
|
|
||
Comment 45•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44) > John, why do you care so much if we have a pref for it or not? Because the fix is known, and the current behavior is clearly broken (works one way in some parts, the other direction in other parts of Firefox). Right now, any part of Firefox where the scrolling is handled by Android UI scrolls one way (such as settings), and any part where handlePointerScroll is called scrolls the other direction. Delaying a known fix to add an option to preserve a bug, adding more cruft to the Firefox codebase is a waste. Who are all these potential Android FireFox users with mice running into the flaky scroll wheel support? Android stick computers like the MK802 series https://www.google.com/#bav=&q=android+stick http://en.wikipedia.org/wiki/Android_Mini_PC_MK802
|
|
||
Comment 46•4 years ago
|
||
(In reply to John Bridges from comment #45) > Delaying a known fix to add an option to preserve a bug, adding more cruft > to the Firefox codebase is a waste. It's also a waste to continue arguing about this, and that delays getting this fixed just as much. I offered you the option of writing the patch yourself back in comment 29 but instead you chose to ignore it and continue arguing. Now you'll just have to wait for Dennis to finish his patch.
|
|
||
Comment 47•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #46) > It's also a waste to continue arguing about this, and that delays getting > this fixed just as much. So far, pointing out the obvious (it's a bug, not a feature) in this discussion has taken much less time than setting a VM with a supported OS for development, installing all the tools/source, figuring out the build instructions and proving the fix was correct. Thankfully Dennis did this in Comment 37.
|
||
Comment 48•4 years ago
|
||
I'm seeing this bug too when using Firefox Android on an Android TV stick. On the about:home page the mouse wheel scrolling is working correctly, though.
|
|
(Assignee) | |
Comment 49•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #38) > That's great! I'm glad you were able to make that change and verify that it > has the expected behaviour. For the actual patch though we want to make that > direction preffable so that anybody can change it at runtime from their > about:config options. To do this, you'll need to add a new pref and hook it > up. The code changes you will need to make are: > > 1) Add a new boolean pref to the file at [1]. Call it something like > "ui.scrolling.negate_wheel" or something similar. When this boolean is set > to true, we will invert the scroll direction from mouse wheels. > 2) Update the code at [2] so that it fetches the new pref value as well. > Currently the code is calling the PrefsHelper function at [3] but you'll > have to change it to call the one at [4] and update the callback > implementation to handle the new pref as well. Have the callback store the > pref value in a field in JavaPanZoomController > 3) Do the negation of scrollY based on the value of the pref. > > Does that make sense? If you can code that up, follow the instructions at > [5] to generate a patch and attach it to this bug. If you have any questions > feel free to ask here or on IRC/email. > > [1] > https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/app/ > mobile.js#l654 > [2] > https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/base/ > gfx/JavaPanZoomController.java#l153 > [3] > https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/base/ > PrefsHelper.java#l29 > [4] > https://hg.mozilla.org/mozilla-central/file/612f8a4c8178/mobile/android/base/ > PrefsHelper.java#l35 > [5] > https://developer.mozilla.org/en-US/docs/Developer_Guide/ > How_to_Submit_a_Patch I am sure I am not qualified to handle this with my present skills, but I am willing to learn if you have time to help me. (The following is at line #658, my line numbers are off by 3 from your link) For 1) I have this: // Negate scrollY, true will make the mouse scroll wheel move the screen the same direction as with most desktops or laptops. pref("ui.scrolling.negate_wheel_scrollY", true); For 2) I think you mean to change line #153 to something like this: PrefsHelper.getPrefs("ui.scrolling.axis_lock_mode" "ui.scrolling.negate_wheel_scrollY", new PrefsHelper.PrefHandlerBase() { I changed it to PrefsHelper.getPrefs and added the second prefName. If that is not correct then I need help with that part. But now I am lost as to how to update the callback and store the new pref value. And also, where/how do I negate scrollY
|
|
||
Comment 50•4 years ago
|
||
(In reply to Dennis from comment #49) > I am sure I am not qualified to handle this with my present skills, but I am > willing to learn if you have time to help me. I can certainly help you with finding your way around our code, but unfortunately if you are not familiar enough with Java to do this then I won't be able to help you with that. > If that is not correct then I need help with that part. But now I am lost as > to how to update the callback and store the new pref value. You can create a member variable in the class and store the value of the pref there. The callback function gets called with both the pref name and value, so you need to check the pref name to see which pref value you are getting and handle it appropriately. > And also, where/how do I negate scrollY This needs to be done in the same way that you did before (i.e. comment 37) but only when the member variable that holds the pref value is true.
|
|
(Assignee) | |
Comment 51•4 years ago
|
||
I need to do some studying. It will be several days or more before I will be able to do anything with this. So if anyone else wants to work on this it is fine with me.
|
|
||
Updated•4 years ago
|
||
|
|
(Assignee) | |
Comment 52•4 years ago
|
||
Created attachment 795119 [details] [diff] [review] negateScrollY_Prefable.patch This patch makes scrollY prefable in about:config. If the pref is true then scrollY will be negated and a mouse scroll wheel will scroll the page/screen in the same direction as a mouse scroll wheel attached to a desktop or laptop. A special thank you to Chris Double for his coding skills and help.
|
|
(Assignee) | |
Comment 53•4 years ago
|
||
I have tested these changes by compiling, creating the .apk package, and installing and testing on my Motorola Razr Maxx using a connected wheel mouse. All previously existing mouse, scroll, touch and drag functionality continues to work. I have found no problems while using it. The ui.scrolling.negate_wheel_scrollY pref in about:config toggles (true/false) with true negating scrollY.
|
|
||
Comment 54•4 years ago
|
||
Comment on attachment 795119 [details] [diff] [review] negateScrollY_Prefable.patch This looks really good, thanks for taking the time to figure out how to do this! (Also thanks to Chris Double for stepping in to help!) There's just a few minor formatting issues to fix up (see below), and then we should be able to get this landed. When you fix the formatting, please upload a new patch with a commit message that describes what the patch is doing, and flag me for review on it. There are instructions on how to generate a patch ready for landing at https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F > @Override public void prefValue(String pref, String value) { >- if (value.equals("standard")) { >- mMode = AxisLockMode.STANDARD; >- } else if (value.equals("free")) { >- mMode = AxisLockMode.FREE; >- } else { >- mMode = AxisLockMode.STICKY; >+ if (pref.equals("ui.scrolling.axis_lock_mode")) { >+ if (value.equals("standard")) { Please make sure the code is 4-space indented; here and later in the patch you have a 2-space indent. > float scrollX = event.getAxisValue(MotionEvent.AXIS_HSCROLL); > float scrollY = event.getAxisValue(MotionEvent.AXIS_VSCROLL); >+ >+ if (mNegateWheelScrollY) { scrollY *= -1.0;} Please indent this code to line up with the code around it. Also please move the body of the if condition down to be on a line of its own. On the line above the if, there is a trailing whitespace, please remove it (this shows up highlighted in the Splinter view on Bugzilla).
|
|
(Assignee) | |
Comment 55•4 years ago
|
||
Created attachment 795512 [details] [diff] [review] negateScrollY_Prefable.patch Formatting cleaned up and commit message added.
|
|
(Assignee) | |
Comment 56•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54) > Comment on attachment 795119 [details] [diff] [review] > negateScrollY_Prefable.patch > When you fix the formatting, please upload a new > patch with a commit message that describes what the patch is doing, and flag > me for review on it. I don't know how to flag you for review.
|
||
Comment 57•4 years ago
|
||
(In reply to Dennis from comment #56) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54) > > Comment on attachment 795119 [details] [diff] [review] > > negateScrollY_Prefable.patch > > > > When you fix the formatting, please upload a new > > patch with a commit message that describes what the patch is doing, and flag > > me for review on it. > > I don't know how to flag you for review. See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
|
|
||
Comment 58•4 years ago
|
||
Comment on attachment 795512 [details] [diff] [review] negateScrollY_Prefable.patch Review of attachment 795512 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +568,5 @@ > if (mState == PanZoomState.NOTHING || mState == PanZoomState.FLING) { > float scrollX = event.getAxisValue(MotionEvent.AXIS_HSCROLL); > + float scrollY = event.getAxisValue(MotionEvent.AXIS_VSCROLL); > + if (mNegateWheelScrollY) > + { scrollY *= -1.0;} The formatting here still needs to be fixed - the if statement should be aligned with the lines above (i.e. 12 blank spaces at the start of the line, not 8), and the braces around the body of the if statement should be positioned like all the other braces in the file (opening curly on the same line as the if statement, closing curly on a line by itself after the body). There is still some trailing whitespace on the line with the scrollY variable assignment.
|
|
(Assignee) | |
Comment 59•4 years ago
|
||
I just noticed that half the patch is missing. The changes for mobile.js got lost. I will fix it and resubmit.
|
|
(Assignee) | |
Comment 60•4 years ago
|
||
Created attachment 795808 [details] [diff] [review] negateScrollY_Prefable.patch Reworked, recompiled, retested. Hopefully this is how it should be.
|
|
||
Comment 61•4 years ago
|
||
Comment on attachment 795808 [details] [diff] [review] negateScrollY_Prefable.patch Review of attachment 795808 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!
|
||
Comment 63•4 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9ada8ad0d4f
|
|
||
Comment 64•4 years ago
|
||
| useful | ||
For anybody else that stumbles across this bug, note that the name of the pref to invert the scroll direction when scrolling with a wheel is "ui.scrolling.negate_wheel_scrollY". Flip that pref and the scroll wheel should invert direction right away. You shouldn't need to restart Firefox.
Description
•