Closed
Bug 1458711
Opened 6 years ago
Closed 6 years ago
Allow pinch-zooming to be triggered by Mac trackpad gestures (behind a pref)
Categories
(Core :: Panning and Zooming, enhancement, P3)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla62
People
(Reporter: botond, Assigned: tanushree)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 1 obsolete file)
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•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Blocks: desktop-zoom-mac
Reporter | ||
Updated•6 years ago
|
No longer blocks: desktop-zoom
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years 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•6 years 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•6 years 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•6 years 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•6 years 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•6 years ago
|
Assignee: nobody → tpodder
Reporter | ||
Updated•6 years ago
|
Attachment #8973392 -
Attachment is obsolete: true
Comment 9•6 years 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•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8adad3e36890
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•