1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref)

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

In bug 1458063, we added a way to trigger pinch-zooming on desktop using the mousewheel and a modifier key of the user's choosing (behind a pref).

On Mac, it would be even more convenient for testing to hook up trackpad gestures to trigger pinch-zooming. This will also be behind a pref for now, though we'll want to ship it later when desktop zooming is sufficiently well-behaved.

Comment 1

[Markus Stange [:mstange]]

Attachment: proof of concept

Comment 2

[Tanushree Podder [:tanushree]]

Attachment: Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

Previously, a feature was added to trigger pinch-zooming on desktop using the
mousewheel and a modifier key of the user's choosing (behind a pref).

To make testing easier on Mac desktops, we wanted to trigger pinch-zooming
through mac trackpad gestures. This patch enables pinch zomming through
trackpad gestures when the prefs apz.allow-zooming and
dom.meta-viewport.enabled are set to true. Currently, tests for this feature
were performed manually.

We will be using this feature only for internal testing. Later on when zooming
on desktops is more well-behaved, we will create automated tests and ship it.

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

Comment 3

[Tanushree Podder [:tanushree]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

https://reviewboard.mozilla.org/r/244186/#review250108

::: widget/cocoa/nsChildView.mm:4210
(Diff revision 1)
> +    NSEventPhase eventPhase = [anEvent phase];  
> +    PinchGestureInput::PinchGestureType pinchGestureType;  
> +
> +    switch (eventPhase) {
> +	case NSEventPhaseBegan: 
> +	{

I will remove the trailing white spaces along with other modifications suggested in comments.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

https://reviewboard.mozilla.org/r/244186/#review250116

Thanks! This generally looks good, just a few small nits.

Please flag Markus for a second round of review after addressing these comments.

::: commit-message-4303d:6
(Diff revision 1)
> +Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref); r?botond
> +
> +Previously, a feature was added to trigger pinch-zooming on desktop using the
> +mousewheel and a modifier key of the user's choosing (behind a pref).
> +
> +To make testing easier on Mac desktops, we wanted to trigger pinch-zooming

"on Mac desktops" -> "on Mac"

::: commit-message-4303d:7
(Diff revision 1)
> +
> +Previously, a feature was added to trigger pinch-zooming on desktop using the
> +mousewheel and a modifier key of the user's choosing (behind a pref).
> +
> +To make testing easier on Mac desktops, we wanted to trigger pinch-zooming
> +through mac trackpad gestures. This patch enables pinch zomming through

"mac" -> "Mac", "zomming" -> "zooming"

::: widget/cocoa/nsChildView.mm:4209
(Diff revision 1)
> +    TimeStamp eventTimeStamp = nsCocoaUtils::GetEventTimeStamp([anEvent timestamp]);
> +    NSEventPhase eventPhase = [anEvent phase];  
> +    PinchGestureInput::PinchGestureType pinchGestureType;  
> +
> +    switch (eventPhase) {
> +	case NSEventPhaseBegan: 

Please increase indent by 1 level (2 spaces) only, as per style guide

::: widget/cocoa/nsChildView.mm:4210
(Diff revision 1)
> +    NSEventPhase eventPhase = [anEvent phase];  
> +    PinchGestureInput::PinchGestureType pinchGestureType;  
> +
> +    switch (eventPhase) {
> +	case NSEventPhaseBegan: 
> +	{

Opening brace goes on same line as "case"

::: widget/cocoa/nsChildView.mm:4211
(Diff revision 1)
> +    PinchGestureInput::PinchGestureType pinchGestureType;  
> +
> +    switch (eventPhase) {
> +	case NSEventPhaseBegan: 
> +	{
> +	    pinchGestureType = PinchGestureInput::PINCHGESTURE_START; 

Likewise here, only increase indent by 1 level

::: widget/cocoa/nsChildView.mm:4219
(Diff revision 1)
> +	case NSEventPhaseChanged:
> +	{ 
> +	    pinchGestureType = PinchGestureInput::PINCHGESTURE_SCALE;
> +	    break;
> +	}
> +	default:

Are there any NSEventPhase\* values that we might receive for which we shouldn't generate any event?

(This is probably a question for Markus.)

::: widget/cocoa/nsChildView.mm:4235
(Diff revision 1)
> +	position,
> +	100.0,
> +	100.0 * (1.0 - [anEvent magnification]),
> +	nsCocoaUtils::ModifiersForEvent(anEvent)};
> +
> +    if (pinchGestureType == PinchGestureInput::PINCHGESTURE_END) 

Please put braces around if statement body (even if it's a single line) per style guide

::: widget/cocoa/nsChildView.mm:4240
(Diff revision 1)
> +    if (pinchGestureType == PinchGestureInput::PINCHGESTURE_END) 
> +      event.mFocusPoint = PinchGestureInput::BothFingersLifted<ScreenPixel>();
> +    mGeckoChild->DispatchAPZInputEvent(event);
> +  } else {
> +    
> -  if (!anEvent || !mGeckoChild ||
> +    if (!anEvent || !mGeckoChild ||

The !mGeckoChild check here is redundant now that we check it at the top of the function.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

https://reviewboard.mozilla.org/r/244186/#review250132

::: widget/cocoa/nsChildView.mm:4209
(Diff revision 1)
> +    TimeStamp eventTimeStamp = nsCocoaUtils::GetEventTimeStamp([anEvent timestamp]);
> +    NSEventPhase eventPhase = [anEvent phase];  
> +    PinchGestureInput::PinchGestureType pinchGestureType;  
> +
> +    switch (eventPhase) {
> +	case NSEventPhaseBegan: 

Actually, it looks like there may be a tab character on this line and some of the lines below.

Please remove the tab characters and use spaces only for indentation.

Thanks!

Comment 6

[Tanushree Podder [:tanushree]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

https://reviewboard.mozilla.org/r/244186/#review250116

> The !mGeckoChild check here is redundant now that we check it at the top of the function.

Good catch! I'll fix that.

Comment 7

[Tanushree Podder [:tanushree]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

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

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

https://reviewboard.mozilla.org/r/244186/#review250460

Thanks, looks good to me!

::: widget/cocoa/nsChildView.mm:4222
(Diff revisions 1 - 2)
> -	{ 
> -            pinchGestureType = PinchGestureInput::PINCHGESTURE_END;
> +        pinchGestureType = PinchGestureInput::PINCHGESTURE_END;
> -            break;
> +        break;
> -        }
> +      }
> -    } 
> -   
> +      default: {
> +        NS_WARNING("Couldn't find matching phase for pinch gesture event.");

I would suggest phrasing this as "Unexpected phase for pinch gesture event."

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

https://reviewboard.mozilla.org/r/244186/#review250470

Looks good to me, thanks!

::: widget/cocoa/nsChildView.mm:4244
(Diff revision 2)
> +
> +    mGeckoChild->DispatchAPZInputEvent(event);
> +  } else {
> +
> +    if (!anEvent ||
> -      [self beginOrEndGestureForEventPhase:anEvent]) {
> +	      [self beginOrEndGestureForEventPhase:anEvent]) {

I think there's a tab on this line. Mozreview's way of displaying this doesn't make it completely obvious though.

Comment 10

[Tanushree Podder [:tanushree]]

Comment on attachment 8975962 [details]
Bug 1458711 - Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref);

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8adad3e36890
Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref); r=botond,mstange

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8adad3e36890