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

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: botond, Assigned: tanushree)

Tracking

(Blocks: 1 bug)

Trunk
mozilla62
Points:
---

Firefox Tracking Flags

(firefox61 wontfix, firefox62 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
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.
Posted patch proof of concept (obsolete) — Splinter Review
(Reporter)

Updated

10 months ago
Blocks: 688990
(Reporter)

Updated

10 months ago
No longer blocks: 1245183
Comment hidden (mozreview-request)
(Assignee)

Comment 3

10 months ago
mozreview-review
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.
(Reporter)

Comment 4

10 months ago
mozreview-review
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.
Attachment #8975962 - Flags: review?(botond)
(Reporter)

Comment 5

10 months ago
mozreview-review
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!
(Assignee)

Comment 6

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Reporter)

Comment 8

10 months ago
mozreview-review
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."
Attachment #8975962 - Flags: review?(botond) → review+
(Reporter)

Updated

10 months ago
Assignee: nobody → tpodder
(Reporter)

Updated

10 months ago
Attachment #8973392 - Attachment is obsolete: true

Comment 9

10 months ago
mozreview-review
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.
Attachment #8975962 - Flags: review?(mstange) → review+
Comment hidden (mozreview-request)

Comment 11

10 months ago
Pushed by mstange@themasta.com:
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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8adad3e36890
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
status-firefox61: affected → wontfix
You need to log in before you can comment on or make changes to this bug.