Closed Bug 1458063 Opened 2 years ago Closed 2 years ago

Allow pinch-zooming to be tested on non-touchscreen devices (behind a pref)

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(7 files)

As we're going to be working on pinch-zooming on desktop in the near future, it would facilitate testing if pinch-zoom gestures could be triggered on non-touchscreen devices as well.

I'm thinking of hooking up something like Alt+mousewheel to generate pinch-zoom events, behind a pref. (Alt+mousewheel is currently hooked up to do page navigation, but I'm happy to stomp on that for testing purposes.)
Priority: -- → P3
Whiteboard: [gfx-noted]
Blocks: desktop-zoom
(In reply to Botond Ballo [:botond] from comment #0)
> I'm thinking of hooking up something like Alt+mousewheel to generate
> pinch-zoom events, behind a pref. (Alt+mousewheel is currently hooked up to
> do page navigation, but I'm happy to stomp on that for testing purposes.)

I ended up doing something a little more flexible: I hooked into the existing framework for customizing wheel actions by adding a new wheel action, ACTION_PINCH_ZOOM (numerical value 5), which users can set as the value of e.g. "mousewheel.with_alt.action" to get Alt+mousewheel to do pinch-zooming, or "mousewheel.with_shift.action" for Shift+mousewheel, and so on.
This is a patch that demonstrates what prefs need to be flipped to enable pinch-zooming on desktop (with Alt+mousewheel as the trigger in this example). I don't intend to land this.
(The first patch isn't really necessary, it's a relic from an older implementation strategy where I would check the modifiers in APZCTreeManager directly, but it still feels more correct to propagate the modifiers, so I'm leaving it.)
Comment on attachment 8972707 [details]
Bug 1458063 - Propagate event modifiers for wheel events in APZInputBridge::ReceiveInputEvent.

https://reviewboard.mozilla.org/r/241232/#review247118
Attachment #8972707 - Flags: review?(bugmail) → review+
Comment on attachment 8972708 [details]
Bug 1458063 - Refactor WillHandleWheelEvent() to also indicate the type of action APZ should take.

https://reviewboard.mozilla.org/r/241234/#review247120

::: gfx/layers/apz/public/APZInputBridge.h:94
(Diff revision 2)
> -  // Returns whether or not a wheel event action will be (or was) performed by
> -  // APZ. If this returns true, the event must not perform a synchronous
> -  // scroll.
> +  // Returns the kind of wheel event action, if any, that will be (or was) 
> +  // performed by APZ. If this returns true, the event must not perform a 
> +  // synchronous scroll.

nit: trailing ws

::: gfx/layers/apz/src/APZInputBridge.cpp:33
(Diff revision 2)
>           aEvent.mMessage == eMouseUp ||
>           aEvent.mMessage == eDragEnd ||
>           (gfxPrefs::TestEventsAsyncEnabled() && aEvent.mMessage == eMouseHitTest);
>  }
>  
> -/* static */ bool
> +/* static */ Maybe<APZWheelAction> 

nit: trailing ws
Attachment #8972708 - Flags: review?(bugmail) → review+
Comment on attachment 8972709 [details]
Bug 1458063 - Propagate the APZWheelAction to APZCTreeManager.

https://reviewboard.mozilla.org/r/241236/#review247122

::: widget/InputData.h:623
(Diff revision 2)
>  
>    // Sometimes a wheel event input's wheel delta should be adjusted. This member
>    // specifies how to adjust the wheel delta.
>    WheelDeltaAdjustmentStrategy mWheelDeltaAdjustmentStrategy;
> +
> +  APZWheelAction mAPZAction = APZWheelAction::Scroll;

I'd prefer to initialize this in the constructors rather than here
Attachment #8972709 - Flags: review?(bugmail) → review+
Comment on attachment 8972710 [details]
Bug 1458063 - Give a name to the special (-1,-1) value of a pinch gesture event's focus point.

https://reviewboard.mozilla.org/r/241238/#review247124

::: gfx/layers/apz/test/gtest/InputUtils.h:137
(Diff revision 3)
> +  ScreenIntPoint endFocus = TruncatedToInt(
> +      PinchGestureInput::BothFingersLifted<ScreenPixel>());

The TruncatedToInt here makes me a little uneasy. In the long run it would probably be better to make CreatePinchGestureInput take a ScreenPoint instead of a ScreenIntPoint so that we don't truncate down and then convert it back to a float inside the function. But you don't need to do that in this bug.
Attachment #8972710 - Flags: review?(bugmail) → review+
Comment on attachment 8972711 [details]
Bug 1458063 - Support constructing a PinchGestureInput with a focus point in Screen coordinates.

https://reviewboard.mozilla.org/r/241240/#review247126

::: widget/InputData.cpp:627
(Diff revision 3)
>  {
>  }
>  
>  bool
>  PinchGestureInput::TransformToLocal(const ScreenToParentLayerMatrix4x4& aTransform)
> -{ 
> +{

yay removal of pre-existing trailing whitespace! :)
Attachment #8972711 - Flags: review?(bugmail) → review+
Comment on attachment 8972712 [details]
Bug 1458063 - Introduce a new wheel action for pinch-zooming.

https://reviewboard.mozilla.org/r/241242/#review247128

Nice work! This turned out much cleaner than I thought it would be :)
Attachment #8972712 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> nit: trailing ws

Sigh. I made the Eclipse project generator turn on the "Remove trailing whitespace on edited lines" setting in bug 1432919, but it looks like I still have the occasional Eclipse instance lying around that doesn't have that set.

Fixed.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> ::: widget/InputData.h:623
> > +
> > +  APZWheelAction mAPZAction = APZWheelAction::Scroll;
> 
> I'd prefer to initialize this in the constructors rather than here

Done, though I don't really see the point: we went from specifying the initial value in one place, to specifying it in three places (as there are three constructors), and if someone adds a new constructor, there is a risk of them forgetting to initialize it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> ::: gfx/layers/apz/test/gtest/InputUtils.h:137
> (Diff revision 3)
> > +  ScreenIntPoint endFocus = TruncatedToInt(
> > +      PinchGestureInput::BothFingersLifted<ScreenPixel>());
> 
> The TruncatedToInt here makes me a little uneasy. In the long run it would
> probably be better to make CreatePinchGestureInput take a ScreenPoint
> instead of a ScreenIntPoint so that we don't truncate down and then convert
> it back to a float inside the function. But you don't need to do that in
> this bug.

Fixed. It was a very easy change to make since IntPointTyped converts implicitly to PointTyped.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> Nice work! This turned out much cleaner than I thought it would be :)

Yeah, the existing infrastructure for wheel action prefs came in very handy for this change.
I don't want to land this without a Try push, and bug 1458703 is preventing me from doing Try pushes.
Depends on: 1458703
See Also: → 1458711
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cab1fa46efe0
Propagate event modifiers for wheel events in APZInputBridge::ReceiveInputEvent. r=kats
https://hg.mozilla.org/integration/autoland/rev/8a09824019b7
Refactor WillHandleWheelEvent() to also indicate the type of action APZ should take. r=kats
https://hg.mozilla.org/integration/autoland/rev/cb713d188609
Propagate the APZWheelAction to APZCTreeManager. r=kats
https://hg.mozilla.org/integration/autoland/rev/b0921ba1cfa7
Give a name to the special (-1,-1) value of a pinch gesture event's focus point. r=kats
https://hg.mozilla.org/integration/autoland/rev/201d209f0796
Support constructing a PinchGestureInput with a focus point in Screen coordinates. r=kats
https://hg.mozilla.org/integration/autoland/rev/34a5f8bd932c
Introduce a new wheel action for pinch-zooming. r=kats
Depends on: 1458783
No longer blocks: desktop-zoom
Regressions: 1601936
You need to log in before you can comment on or make changes to this bug.