Zoom gesture can not be performed at the same time as a pan gesture
Categories
(Core :: Panning and Zooming, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox105 | --- | fixed |
People
(Reporter: sawyerbergeron, Assigned: rzvncj, Mentored)
References
Details
(Whiteboard: [lang=c++])
Attachments
(1 file)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:98.0) Gecko/20100101 Firefox/98.0
Steps to reproduce:
- Start a zoom gesture in
- Stop zoom gesture but keep fingers on trackpad
- Perform a pan gesture
Actual results:
Page does not pan
Expected results:
Page pans after the zoom gesture has been recognized, so that if my cursor was not directly centered over the area I want to zoom in on I can make small corrections while zooming or correct without having to start a new gesture
Reporter | ||
Comment 1•3 years ago
|
||
Test was performed on a wlroots compositor (wayland) using Libinput 1.19.3
Expected behavior is visible using Gnome Web (Epiphany) on the same platform
Reporter | ||
Updated•3 years ago
|
Comment 2•3 years ago
•
|
||
First step to investigating this would be to understand what sequence of events do we get at the GTK level.
- Do we get a
GDK_TOUCHPAD_GESTURE_PHASE_END
at the transition?- If not, are the events during the "pan" phase
GDK_TOUCHPAD_GESTURE_PHASE_UPDATE
?
- If not, are the events during the "pan" phase
- Or are the events during the "pan" phase now
GdkEventScroll
?
Comment 3•3 years ago
|
||
I can reproduce this. It's pretty clear from the observed behaviour that we continue to get GdkEventTouchpadPinch
throughout the entire gesture, since we continue to zoom in/our in response to small changes in the finger span even during the "pan" phase of the gesture.
APZ does support this use case (called "two-finger panning"), if it gets PinchGestureInput
events whose focus point (on touchscreens, the midpoint of the two touch points) is changing.
In the GTK widget code, we set the focus point based on the x
and y
(or x_root
and y_root
) fields of GdkEventTouchpadPinch
. It follows that these properties are not changing over the course of the gesture (they probably correspond to the cursor position on screen).
I see that the event also has dx
and dy
fields; I'm going to venture a guess that these do change to reflect the touchpad gesture's focus point.
So, I think this can be fixed by revising our code to use something like x + dx
and y + dy
(possibly with a transformation applied to dx
and dy
to get them into the same coordinate system as x
and y
, or at least magnitudes suitable for addition) as the focus point.
Marking as S3 for now. Also making this a mentored bug. Sawyer, let me know if you're interested in trying this!
Reporter | ||
Comment 4•3 years ago
|
||
I'd be happy to work on this! I'm currently having some trouble with my moz build repo (mozboot/mach throw a really angry stack trace) and I have a thesis proposal to complete over the next couple weeks, but can put more time into this after that.
Assignee | ||
Comment 5•3 years ago
|
||
I've attempted the following template specialization for GdkEventTouchpadPinch
:
diff --git a/widget/gtk/nsWindow.cpp b/widget/gtk/nsWindow.cpp
--- a/widget/gtk/nsWindow.cpp
+++ b/widget/gtk/nsWindow.cpp
@@ -4071,6 +4071,23 @@ static LayoutDeviceIntPoint GetRefPoint(
aWindow->WidgetToScreenOffset();
}
+template <>
+static LayoutDeviceIntPoint GetRefPoint(nsWindow* aWindow,
+ GdkEventTouchpadPinch* aEvent) {
+ if (aEvent->window == aWindow->GetGdkWindow()) {
+ // we are the window that the event happened on so no need for expensive
+ // WidgetToScreenOffset
+ return aWindow->GdkEventCoordsToDevicePixels(aEvent->x + aEvent->dx,
+ aEvent->y + aEvent->dy);
+ }
+ // XXX we're never quite sure which GdkWindow the event came from due to our
+ // custom bubbling in scroll_event_cb(), so use ScreenToWidget to translate
+ // the screen root coordinates into coordinates relative to this widget.
+ return aWindow->GdkEventCoordsToDevicePixels(aEvent->x_root + aEvent->dx,
+ aEvent->y_root + aEvent->dy) -
+ aWindow->WidgetToScreenOffset();
+}
+
void nsWindow::OnMotionNotifyEvent(GdkEventMotion* aEvent) {
if (!mGdkWindow) {
return;
Unfortunately, it doesn't seem to work. I thought that no transformations are needed for dx
and dy
, since they're added directly to x
and y
(and x_root
and y_root
), but maybe I'm wrong. I'm also not much of a gestures person, so maybe my touchpad navigation skills have something to do with the results, but I've been unable to properly switch from zooming to panning without lifting my (two) fingers from the touchpad.
Assignee | ||
Comment 6•3 years ago
|
||
Actually, it kind of works... If I just keep my fingers on the touchpad after I zoom but wait for a few seconds before bringing them together to try to pan, then panning appears to work as expected. Could somebody confirm that this is how I'm supposed to switch from zooming to panning without lifting my fingers off the touchpad? If the diff above works properly I'm happy to make it into a patch and submit it.
Assignee | ||
Comment 7•3 years ago
|
||
Actually I can't reproduce it working so I must have lifted a finger or done something else to break the gesture. Nevermind.
Assignee | ||
Comment 8•3 years ago
|
||
I've logged what happens while zooming, and here it is:
x: 485, y: 511, dx: 0.533646, dy: -10.0116
x: 485, y: 511, dx: 0.637009, dy: -9.05539
x: 485, y: 511, dx: 2.41373, dy: -8.86981
x: 485, y: 511, dx: 4.99988, dy: -9.45335
x: 485, y: 511, dx: -4.13783, dy: -7.00249
x: 485, y: 511, dx: -4.31024, dy: -2.91769
x: 485, y: 511, dx: -0.862045, dy: -8.86981
x: 485, y: 511, dx: -1.03445, dy: -4.90173
x: 485, y: 511, dx: 0, dy: -3.26782
x: 485, y: 511, dx: -0.172409, dy: -2.45087
x: 485, y: 511, dx: 0.689636, dy: -2.80099
x: 485, y: 511, dx: 1.55168, dy: -4.43491
x: 485, y: 511, dx: -0.172409, dy: -3.96808
x: 485, y: 511, dx: -1.03445, dy: -3.96808
x: 485, y: 511, dx: -3.10336, dy: -3.96808
x: 485, y: 511, dx: -3.27579, dy: -3.61795
x: 485, y: 511, dx: -5.17229, dy: -2.45087
x: 485, y: 511, dx: -4.82747, dy: -1.63391
x: 485, y: 511, dx: -3.4482, dy: 0.233414
x: 485, y: 511, dx: -2.06891, dy: -0.116699
x: 485, y: 511, dx: -0.748291, dy: -0.101303
x: 485, y: 511, dx: 0.176315, dy: 0.298386
x: 485, y: 511, dx: 0.3871, dy: 0.0436707
x: 485, y: 511, dx: 0.341812, dy: -0.0578308
x: 485, y: 511, dx: 1.02119, dy: -0.259216
x: 485, y: 511, dx: 12.183, dy: -1.83264
x: 485, y: 511, dx: 1.55168, dy: -2.21745
x: 485, y: 511, dx: 10.3446, dy: -1.28378
x: 485, y: 511, dx: 7.58603, dy: -0.700241
x: 485, y: 511, dx: 7.58603, dy: -1.05037
x: 485, y: 511, dx: 5.3447, dy: 0.466827
x: 485, y: 511, dx: 3.62061, dy: 0.350113
x: 485, y: 511, dx: 2.06891, dy: 0.116699
x: 485, y: 511, dx: 2.06891, dy: 0.233414
x: 485, y: 511, dx: 0.432541, dy: 0.390396
x: 485, y: 511, dx: 0, dy: 0.336899
x: 485, y: 511, dx: 0.345901, dy: 0.702469
x: 485, y: 511, dx: 0.166336, dy: 2.58983
x: 485, y: 511, dx: 2.24132, dy: 2.80099
x: 485, y: 511, dx: 1.55168, dy: 2.10074
x: 485, y: 511, dx: 4.13783, dy: 1.5172
x: 485, y: 511, dx: 2.06891, dy: 1.05037
x: 485, y: 511, dx: 2.24132, dy: -0.350113
x: 485, y: 511, dx: 2.75854, dy: -0.583527
x: 485, y: 511, dx: 2.58614, dy: -0.583527
x: 485, y: 511, dx: 1.35304, dy: -0.34346
x
and y
do not change at all. Changing the focus to CONSTANT_X + dx, CONSTANT_Y + dy
does do something, but the effect is barely noticeable. It's not clear to me what conversion (and from what) dx
and dy
would need to undergo to make this work.
Assignee | ||
Comment 9•3 years ago
|
||
I believe I've figured it out, dx
and dy
need to be continuously updated. That is, it's not x + dx
, but rather static_dx += dx; use x + static_dx;
. The deltas are relative to the last event, not to the beginning of the zoom gesture. I'll mess around with it a bit more and submit a patch when it's ready.
Assignee | ||
Comment 10•3 years ago
|
||
Accumulating dx
and dy
(as described above) does work for panning, however I've compared the behaviour to the one exhibited by the Gnome browser, and that one allows for simultaneous panning and zooming, while with these changes Firefox doesn't seem to want to ever return to zooming once the panning-while-zooming starts. I've not looked at the code in any great detail, but it feels that once the focus point changes (however little) we're in pan-only mode - and it's very difficult to have the focus point not change at all.
Looks like this might require subtler changes than originally envisioned?
Comment 11•3 years ago
|
||
We do have a feature called "pinch locking", where we try to detect if a two-finger touchscreen gesture is a pan rather than a pinch, and lock into panning in that case.
To verify that this is what you're seeing, could you try setting the pref apz.pinch_lock.mode
to 0, and see if the issue goes away?
If it is indeed pinch locking that's causing this, the next step would be to see why it's so sensitive. The condition for entering pinch lock is found here and it requires both that the focus change over the lifetime of the gesture be above a threshold, and that the span distance (the amount by which the distance between the fingers has changed) over the lifetime of the gesture be below a threshold.
Assignee | ||
Comment 12•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #11)
We do have a feature called "pinch locking", where we try to detect if a two-finger touchscreen gesture is a pan rather than a pinch, and lock into panning in that case.
To verify that this is what you're seeing, could you try setting the pref
apz.pinch_lock.mode
to 0, and see if the issue goes away?If it is indeed pinch locking that's causing this, the next step would be to see why it's so sensitive. The condition for entering pinch lock is found here and it requires both that the focus change over the lifetime of the gesture be above a threshold, and that the span distance (the amount by which the distance between the fingers has changed) over the lifetime of the gesture be below a threshold.
Yes, it was indeed the pinch locking. With apz.pinch_lock.mode
set to 0 I'm seeing Firefox behave like Epiphany. I'll look into the threshold later today or tomorrow. Thanks!
Assignee | ||
Comment 13•3 years ago
|
||
And another thing about this patch, I'm seeing this comment in nsWindow::SynthesizeNativeTouchPadPinch()
:
// We only set the fields of GdkEventTouchpadPinch which are
// actually used in OnTouchpadPinchEvent().
// GdkEventTouchpadPinch has additional fields (for example, `dx` and `dy`).
// If OnTouchpadPinchEvent() is changed to use other fields, this function
// will need to change to set them as well.
Should I set dx
and dy
there too? If so, where would I pull them from?
Assignee | ||
Comment 14•3 years ago
|
||
(Also, if this helps anything - clarity? - you can assign this bug to me, I intend to see it through eventually. :))
Comment 15•3 years ago
•
|
||
(In reply to Razvan Cojocaru from comment #13)
Should I set
dx
anddy
there too? If so, where would I pull them from?
Very good point! Yes, we should. I would suggest the following:
- When receiving the
PHASE_BEGIN
event, save the value ofpointInWindow
in a new field ofnsWindow
(e.g.mLastTouchpadPinchBegin
). Thedx
anddy
values here can be 0. - For
PHASE_UPDATE
andPHASE_END
events, set theirx
andy
tomLastTouchpadPinchBegin
, and set theirdx
anddy
to the difference(pointInWindow - mLastTouchpadPinchBegin)
.
(In reply to Razvan Cojocaru from comment #14)
(Also, if this helps anything - clarity? - you can assign this bug to me, I intend to see it through eventually. :))
Done -- thanks!
Assignee | ||
Comment 16•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #15)
(In reply to Razvan Cojocaru from comment #13)
Should I set
dx
anddy
there too? If so, where would I pull them from?Very good point! Yes, we should. I would suggest the following:
- When receiving the
PHASE_BEGIN
event, save the value ofpointInWindow
in a new field ofnsWindow
(e.g.mLastTouchpadPinchBegin
). Thedx
anddy
values here can be 0.- For
PHASE_UPDATE
andPHASE_END
events, set theirx
andy
tomLastTouchpadPinchBegin
, and set theirdx
anddy
to the difference(pointInWindow - mLastTouchpadPinchBegin)
.
Right, but that's not how GDK sets them according to my tests. Your assumption is that on each PHASE_UPDATE
event dx
and dy
are "full" deltas with respect to the original point (specified as x
and y
in the event). But what I'm seeing is that on each PHASE_UPDATE
event, the delta is relative to the position of the previous PHASE_UPDATE
event.
Anyway, I got the idea, I'll update the code to reflect the GDK behaviour. :)
Assignee | ||
Comment 17•3 years ago
|
||
Comment 18•3 years ago
•
|
||
We ran into a complication while working on a test for this in bug 1780121.
The native events produced by the widget and processed by APZ need to be mapped onto web platform events which are dispatched to the web content, and the web content needs to have an opportunity to override the browser's default handling of the events by calling preventDefault()
.
As there are no events directly representing touchpad touches or pinch gestures in the web platform, for touchpad pinch gestures we map them onto wheel
events with the Ctrl
modifier set, with their deltaY
set based on the amount of zooming (i.e. based on the change in finger span relative to the last event).
We have some code to drop pinch gesture events where the span change is zero, because in such cases the wheel
events would have deltaY=0
and not get dispatched to content and, on pages which want to handle the zooming themselves, content would not get a chance to preventDefault()
those events. This has led to bugs such as bug 1700819.
But dropping events with a zero span change means that pan-only movement during the pinch gesture won't work. (During manual testing, I'm guessing it worked because at least some events actually had a small amount of span change; however, events that did have a zero span change got dropped and this probably contibuted to the two-finger pan feeling slow. In our test, we tried to synthesize events which consistently had zero span change and those did not result in panning at all.)
Timothy, do you know if we support two-finger panning as a part of a pinch gesture on Windows touchpads? If so, how do we handle this, i.e. what sort of events do we send to APZ and to content during the pan phase of the gesture?
Assignee | ||
Comment 19•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #18)
But dropping events with a zero span change means that pan-only movement during the pinch gesture won't work. (During manual testing, I'm guessing it worked because at least some events actually had a small amount of span change; however, events that did have a zero span change got dropped and this probably contibuted to the two-finger pan feeling slow. In our test, we tried to synthesize events which consistently had zero span change and those did not result in panning at all.)
Yes, this is indeed exactly why it worked during manual testing. Not only that, but it's why it appeared to work in the initial mochitest I wrote: I was varying not only the focus point coordinates, but also the scale slightly. I also agree (and actually was planning to write about that here or in the diff) that the dropped events contribute to the less-than-ideal panning, once it begins.
Comment 20•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #18)
We have some code to drop pinch gesture events where the span change is zero, because in such cases the
wheel
events would havedeltaY=0
and not get dispatched to content and, on pages which want to handle the zooming themselves, content would not get a chance topreventDefault()
those events. This has led to bugs such as bug 1700819.
I would say that we accumulate the event deltas until they are big enough to generate a wheel event with deltaY != 0, rather than just dropping them.
Timothy, do you know if we support two-finger panning as a part of a pinch gesture on Windows touchpads? If so, how do we handle this, i.e. what sort of events do we send to APZ and to content during the pan phase of the gesture?
No, once we start a pinch gesture, the gesture has to end before we can get back into panning. A gesture can start as panning and transition to pinching. In that case we send the pan end before the pinch start.
The code is here, where we can only enter panning if we had no previous state, but we can enter pinching if we were previously panning:
https://searchfox.org/mozilla-central/rev/42a517a4fd9d0c43486f1b20ffca06ce8876d3d7/widget/windows/DirectManipulationOwner.cpp#301
Direct manipulation just gives us a transform, so we could implement panning in the middle of pinching if we wanted, it would make the code in that file more complicated though.
Comment 21•3 years ago
•
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #20)
Direct manipulation just gives us a transform, so we could implement panning in the middle of pinching if we wanted, it would make the code in that file more complicated though.
Do you have an idea of how that could work at the level of web platform events / preventDefault()
?
Comment 22•3 years ago
|
||
I don't have any specific insight into that. Could we partition the gesture into distinct pan and pinch parts so that only one is changing at a time (one stops and then the other starts) and send the content events for each of those parts as if the other part of the gesture isn't happening and the preventDefault would only cancel that part of the gesture? Or if we didn't partition into distinct parts we'd have to send two content events for both things that are going on, handling preventDefault for one but not the other would be complicated. Or we could pack both the pan and pinch into a ctrl wheel event abusing the deltax/y/z fields. But that might confuse web content, the current setup (ctrl wheel event with deltay is already pretty weird), and web content would need to change to preventDefault these new types of pans it can get.
Comment 23•3 years ago
|
||
Markus, I'd be curious to know whether/how Mac handles this as well:
- After you start a touchpad pinch-zoom gesture, if you then transition into a pan-only movement (two fingers moving in the same direction) without lifting your fingers, can you scroll that way?
- If so, what kind of events do we dispatch to content during the pan movement?
- Do we handle
preventDefault()
correctly in such cases (e.g. avoid activating browser zoom on a site like Bing Maps during a combined gesture like this)? (Do we pan the map instead?)
Comment 24•3 years ago
|
||
No, macOS does not allow you to transition a touchpad pinch zoom gesture into a scroll gesture, neither in Safari nor in Apple Maps.
In Apple maps you can zoom and rotate in the same gesture, but not zoom and pan. It seems that "pan or not pan" is decided at the start of the gesture, and then either only panning or only non-panning is allowed until both fingers are lifted.
Comment 25•3 years ago
|
||
Thank you all for the feedback.
It looks like this combined pinch + pan gesture is not something that's currently supported on either of our other desktop platforms (Windows or Mac), and a proper implementation (meaning, no events / deltas are dropped, the view responds to touchpad movements promptly, and sites with custom handling of zoom like mapping sites work well) would need some additional thought and potentially be fairly involved.
At the same time, we have an existing browser on Linux (Gnome Web) which supports this feature.
I think that an implementation that avoids dispatching events (to APZ or content) with a zero span change is better than not supporting panning at all, even if it's a bit less responsive / some events are dropped. So, I'm going to suggest that we land our current implementation, with one change: rather than dropping the delta for an event with zero span change, accumulate it into mCurrentTouchpadFocus
such that it will be reflected as part of the next event with a nonzero span change.
For the test, we can work around the issue by making small changes to the span even during the "pan" part of the gesture. Perhaps we can also move the test patch from bug 1780121 to this bug, as the test and the fix are closely related and should probably land together.
Comment 26•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #25)
At the same time, we have an existing browser on Linux (Gnome Web) which supports this feature.
Out of curiosity how does Gnone Web handle sending events to web content to potentially preventDefault in a pinch + pan gesture?
Comment 27•3 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #26)
(In reply to Botond Ballo [:botond] from comment #25)
At the same time, we have an existing browser on Linux (Gnome Web) which supports this feature.
Out of curiosity how does Gnone Web handle sending events to web content to potentially preventDefault in a pinch + pan gesture?
Good question. I'm not sure, but if we want to pursue a more proper solution in the future, then looking into that could turn up some useful ideas.
Comment 28•3 years ago
|
||
Comment 29•3 years ago
|
||
bugherder |
Description
•