[Bug 1228250]

[R-bugs-h]

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 Mozilla
Bugs, which is subscribed to Mozilla Firefox.
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