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 Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1228250 Title: [Shift + Mouse-Scroll-Wheel] Does NOT Scroll Horizontally To manage notifications about this bug go to: https://bugs.launchpad.net/firefox/+bug/1228250/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
