Comment on attachment 8915954 Bug 143038 Make users can scroll contents horizontally with vertical wheel operation with a modifier
https://reviewboard.mozilla.org/r/186794/#review192674 This is complicated enough, that I think I should re-read this after those small nits are fixed. ::: commit-message-19b32:26 (Diff revision 1) > +restoring. > + > +So, this patch does NOT change any wheel event information on web apps. Only > +changes its default action. This is same behavior as Chromium. > + > +Note that with this patch, users cannot navigate the tab's history with Could you explain this. Currently shift+vertical wheel is back-forward. Does this new behavior override that feature always or only when it is enabled explicitly? How is shift+horizontal supposed to work for back-forward if shift+vertical is for scrolling? One often gets both horizontal and vertical scrolling when using touchpad. ::: dom/events/EventStateManager.h:652 (Diff revision 1) > * If an .override_x value is -1, same as the > * corresponding mActions value. > */ > Action mOverriddenActionsX[COUNT_OF_MULTIPLIERS]; > > + // XXX Modifier is better than Modifiers. However, it's defined in That seems like a silly reason to not use Modifier. If you're really worried about the build time change, why not just move Modifier to its own header? ::: dom/events/EventStateManager.cpp:5691 (Diff revision 1) > + } > + > + Index index = GetIndexFor(aEvent->mModifiers & > + > ~mModifierToTreatVertialWheelAsHorizontalScroll); > + Init(index); > + // We need to cache this result in the widget. Some methods of this class Cache the result in the widget? I don't see anything stored in the widget. ::: dom/events/EventStateManager.cpp:5710 (Diff revision 1) > return INDEX_DEFAULT; > } > + if (!NeedToTreatAsHorizontalScroll(aEvent)) { > + return GetIndexFor(aEvent->mModifiers); > + } > +#if 0 No ifdef 0 code, please ::: dom/events/EventStateManager.cpp:5837 (Diff revision 1) > Index index = GetIndexFor(aEvent); > Init(index); > > + // If the event should be treated as horizontal wheel operation, deltaY > + // should be applied mMultiplierX. Note that deltaX and deltaZ are always > + // 0 in such case. Therefore, we only need to use temporary variable only Why they are 0? ::: dom/events/EventStateManager.cpp:5842 (Diff revision 1) > + // 0 in such case. Therefore, we only need to use temporary variable only > + // for deltaY. Additionally, if the event is being handled by default > + // handler, the deltaX and deltaY may be swapped. Therefore, we need to > + // use mMultiplierX for deltaY only when the event should be treated as > + // horizontal scroll and mDeltaY isn't 0. > + auto multiplierForDeltaY = mMultiplierY[index]; Could you not use auto here. With auto I need to go to the mMultiplierY definition to see what the type of multiplierForDeltaY is. ::: dom/events/EventStateManager.cpp:5886 (Diff revision 1) > aEvent->mOverflowDeltaX /= mMultiplierX[index]; > } > - if (mMultiplierY[index]) { > - aEvent->mOverflowDeltaY /= mMultiplierY[index]; > + > + // If the event should be treated as horizontal wheel operation, deltaY > + // should be applied mMultiplierX. Note that deltaX and deltaZ are always > + // 0 in such case. Therefore, we only need to use temporary variable only Again, why they are 0? ::: dom/events/EventStateManager.cpp:5891 (Diff revision 1) > + // 0 in such case. Therefore, we only need to use temporary variable only > + // for deltaY. Additionally, if the event is being handled by default > + // handler, the deltaX and deltaY may be swapped. Therefore, we need to > + // use mMultiplierX for deltaY only when the event should be treated as > + // horizontal scroll and mDeltaY isn't 0. > + auto multiplierForDeltaY = mMultiplierY[index]; Don't use auto, pretty please. ::: dom/events/WheelHandlingHelper.h:217 (Diff revision 1) > + */ > +class MOZ_STACK_CLASS AutoTemporarilyWheelDeltaSwapper final > +{ > +public: > + /** > + * @param aWheelEvent A wheel event. Must not be nullptr. If so, perhaps make the ctor take WidgetWheelEvent& and store using that type too. That self-documents that it must not ever-never be null -- You received this bug notification because you are a member of Desktop Packages, which is subscribed to firefox in Ubuntu. https://bugs.launchpad.net/bugs/1228250 Title: [Shift + Mouse-Scroll-Wheel] Does NOT Scroll Horizontally Status in Mozilla Firefox: Fix Released Status in firefox package in Ubuntu: Fix Released Bug description: In Chromium, I can scroll horizontally using my mouse-wheel by holding down the shift key while scrolling the mouse wheel. Firefox is missing this very convenient page-navigation short-cut. Use Case: As my eyes age, I find myself always scaling up the web pages I read (by holding down crtl and scrolling my mouse wheel). Doing this, often makes the page exceed the width of my monitor (hiding the right-side of the text I want to read) and produces a horizontal scroll bar at the bottom of the page. At this point, since I've already used ctrl-scroll-mouse-wheel to magnify the page, it would be wonderful if I could use shift-scroll- mouse-wheel to horizontally-scroll the magnified page and therefore center the (previously cropped) text that I am wanting to read. ProblemType: Bug DistroRelease: Ubuntu 13.04 Package: firefox 24.0+build1-0ubuntu0.13.04.1 ProcVersionSignature: Ubuntu 3.8.0-30.44-generic 3.8.13.6 Uname: Linux 3.8.0-30-generic x86_64 NonfreeKernelModules: wl AddonCompatCheckDisabled: False ApportVersion: 2.9.2-0ubuntu8.3 Architecture: amd64 AudioDevicesInUse: USER PID ACCESS COMMAND /dev/snd/controlC0: lonnie 2161 F.... pulseaudio /dev/snd/pcmC0D0p: lonnie 2161 F...m pulseaudio BrokenPermissions: sessionstore.bak (0o600, wrong owner) BuildID: 20130911155223 Channel: Unavailable Date: Fri Sep 20 11:02:07 2013 ForcedLayersAccel: False IfupdownConfig: # interfaces(5) file used by ifup(8) and ifdown(8) auto lo iface lo inet loopback InstallationDate: Installed on 2013-09-06 (14 days ago) InstallationMedia: Ubuntu 13.04 "Raring Ringtail" - Release amd64 (20130424) IpRoute: default via 192.168.24.1 dev eth0 proto static 192.168.24.0/24 dev eth0 proto kernel scope link src 192.168.24.198 metric 1 MarkForUpload: True PrefSources: prefs.js [Profile]/extensions/{b9db16a4-6edc-47ec-a1f4-b86292ed211d}/defaults/preferences/prefs-dwhelper.js ProcEnviron: TERM=xterm PATH=(custom, no user) XDG_RUNTIME_DIR=<set> LANG=en_US.UTF-8 SHELL=/bin/bash Profiles: Profile0 (Default) - LastVersion=24.0/20130911155223 (In use) RelatedPackageVersions: google-talkplugin 4.5.3.0-1 icedtea-7-plugin 1.3.2-1ubuntu1.1 totem-mozilla 3.6.3-0ubuntu6 rhythmbox-mozilla 2.98-0ubuntu5 RunningIncompatibleAddons: False SourcePackage: firefox UpgradeStatus: No upgrade log present (probably fresh install) dmi.bios.date: 04/14/2011 dmi.bios.vendor: Dell Inc. dmi.bios.version: A09 dmi.board.name: 0P792H dmi.board.vendor: Dell Inc. dmi.board.version: A09 dmi.chassis.type: 8 dmi.chassis.vendor: Dell Inc. dmi.chassis.version: A09 dmi.modalias: dmi:bvnDellInc.:bvrA09:bd04/14/2011:svnDellInc.:pnStudio1737:pvrA09:rvnDellInc.:rn0P792H:rvrA09:cvnDellInc.:ct8:cvrA09: dmi.product.name: Studio 1737 dmi.product.version: A09 dmi.sys.vendor: Dell Inc. To manage notifications about this bug go to: https://bugs.launchpad.net/firefox/+bug/1228250/+subscriptions -- Mailing list: https://launchpad.net/~desktop-packages Post to : desktop-packages@lists.launchpad.net Unsubscribe : https://launchpad.net/~desktop-packages More help : https://help.launchpad.net/ListHelp
