Closed
Bug 1458063
Opened 6 years ago
Closed 6 years ago
Allow pinch-zooming to be tested on non-touchscreen devices (behind a pref)
Categories
(Core :: Panning and Zooming, enhancement, P3)
Core
Panning and Zooming
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)
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
2.68 KB,
patch
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Updated•6 years ago
|
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee | ||
Updated•6 years ago
|
Blocks: desktop-zoom
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
(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.
Assignee | ||
Comment 16•6 years ago
|
||
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.
Assignee | ||
Comment 17•6 years ago
|
||
(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•6 years ago
|
||
mozreview-review |
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 19•6 years ago
|
||
mozreview-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 20•6 years ago
|
||
mozreview-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 21•6 years ago
|
||
mozreview-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 22•6 years ago
|
||
mozreview-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 23•6 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•6 years ago
|
||
(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.
Assignee | ||
Comment 30•6 years ago
|
||
I don't want to land this without a Try push, and bug 1458703 is preventing me from doing Try pushes.
Depends on: 1458703
Comment 31•6 years ago
|
||
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
Comment 32•6 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Updated•6 years ago
|
Blocks: desktop-zoom-xp
Assignee | ||
Updated•6 years ago
|
No longer blocks: desktop-zoom
You need to log in
before you can comment on or make changes to this bug.
Description
•