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

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

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.)

Comment 1

[Botond Ballo [:botond]]

Attachment: Bug 1458063 - Propagate event modifiers for wheel events in APZInputBridge::ReceiveInputEvent.

Review commit: https://reviewboard.mozilla.org/r/241232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241232/

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1458063 - Refactor WillHandleWheelEvent() to also indicate the type of action APZ should take.

Review commit: https://reviewboard.mozilla.org/r/241234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241234/

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1458063 - Propagate the APZWheelAction to APZCTreeManager.

Review commit: https://reviewboard.mozilla.org/r/241236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241236/

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1458063 - Give a name to the special (-1,-1) value of a pinch gesture event's focus point.

Review commit: https://reviewboard.mozilla.org/r/241238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241238/

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1458063 - Support constructing a PinchGestureInput with a focus point in Screen coordinates.

Review commit: https://reviewboard.mozilla.org/r/241240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241240/

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1458063 - Introduce a new wheel action for pinch-zooming.

Review commit: https://reviewboard.mozilla.org/r/241242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241242/

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8972708 [details]
Bug 1458063 - Refactor WillHandleWheelEvent() to also indicate the type of action APZ should take.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241234/diff/1-2/

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8972709 [details]
Bug 1458063 - Propagate the APZWheelAction to APZCTreeManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241236/diff/1-2/

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8972710 [details]
Bug 1458063 - Give a name to the special (-1,-1) value of a pinch gesture event's focus point.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241238/diff/1-2/

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8972711 [details]
Bug 1458063 - Support constructing a PinchGestureInput with a focus point in Screen coordinates.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241240/diff/1-2/

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8972712 [details]
Bug 1458063 - Introduce a new wheel action for pinch-zooming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241242/diff/1-2/

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8972710 [details]
Bug 1458063 - Give a name to the special (-1,-1) value of a pinch gesture event's focus point.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241238/diff/2-3/

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8972711 [details]
Bug 1458063 - Support constructing a PinchGestureInput with a focus point in Screen coordinates.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241240/diff/2-3/

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8972712 [details]
Bug 1458063 - Introduce a new wheel action for pinch-zooming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241242/diff/2-3/

Comment 15

[Botond Ballo [:botond]]

(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.

Comment 16

[Botond Ballo [:botond]]

Attachment: Flip prefs to enable pinch-zooming on desktop (with Alt+mousewheel as trigger)

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.

Comment 17

[Botond Ballo [:botond]]

(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 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8972707 [details]
Bug 1458063 - Propagate event modifiers for wheel events in APZInputBridge::ReceiveInputEvent.

https://reviewboard.mozilla.org/r/241232/#review247118

Comment 19

[Kartikaya Gupta (email:[e-mail])]

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

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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

Comment 21

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

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! :)

Comment 23

[Kartikaya Gupta (email:[e-mail])]

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 :)

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8972708 [details]
Bug 1458063 - Refactor WillHandleWheelEvent() to also indicate the type of action APZ should take.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241234/diff/2-3/

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8972709 [details]
Bug 1458063 - Propagate the APZWheelAction to APZCTreeManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241236/diff/2-3/

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8972710 [details]
Bug 1458063 - Give a name to the special (-1,-1) value of a pinch gesture event's focus point.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241238/diff/3-4/

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8972711 [details]
Bug 1458063 - Support constructing a PinchGestureInput with a focus point in Screen coordinates.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241240/diff/3-4/

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8972712 [details]
Bug 1458063 - Introduce a new wheel action for pinch-zooming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241242/diff/3-4/

Comment 29

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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:[e-mail]) 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:[e-mail]) 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:[e-mail]) 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.

Comment 30

[Botond Ballo [:botond]]

I don't want to land this without a Try push, and bug 1458703 is preventing me from doing Try pushes.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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

Comment 32

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/cab1fa46efe0
https://hg.mozilla.org/mozilla-central/rev/8a09824019b7
https://hg.mozilla.org/mozilla-central/rev/cb713d188609
https://hg.mozilla.org/mozilla-central/rev/b0921ba1cfa7
https://hg.mozilla.org/mozilla-central/rev/201d209f0796
https://hg.mozilla.org/mozilla-central/rev/34a5f8bd932c