1648489 - [touchpad] Pinch zooming on Google Maps moves the fixed position elements

Status: RESOLVED FIXED

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

Description

[Gyula Palko[:gpalko]]

Affected versions
Nightly 79.0a1

Affected platforms
Devices with precision touchpad (tested on Lenovo yoga)

Preconditions
apz.allow_zooming = true
apz.windows.use_direct_manipulation = true

Steps to reproduce

1. Open https://maps.google.com
2. On map- Initiate pinch zooming from the touchpad

Expected result
The map is zoomed, other elements on the page remain displayed(buttons, search field)

Actual Result
The buttons from the page are moved as zooming is performed
The user needs to zoom out, in order to use the search box from the page or the control buttons on the bottom right side
Zoom out performance is poor

Note
Pinch zoom initiated from the touchscreen is working as expected
The issue looks similar to bug 1619187

Comment 1

[Timothy Nikkel (:tnikkel)]

Seems like for every pinch gesture (pinch gesture being user touches touch pad, moves fingers then lifts fingers) one event that should be preventdefault-ed by content is not and zooms the page, but the rest of the pinch gesture inputs work correctly.

Comment 2

[Timothy Nikkel (:tnikkel)]

Seems like the problem might be that we get a InputQueue::ReceivePinchGestureInput call before we get InputQueue::SetConfirmedTargetApzc. At least that is a difference on mac (where it works) vs Windows (where it doesn't) for me.

Comment 3

[Timothy Nikkel (:tnikkel)]

The problem is that this code

https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/widget/WidgetEventImpl.cpp#492

prevents us from dispatching the wheel event that we create from the first PinchGestureInput. When we send a pinch start or end from direct manipulation we send mPreviousSpan == mCurrent span here

https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/widget/windows/DirectManipulationOwner.cpp#394

and that creates a wheel event with all zero deltas. On mac we always seem to get a positive magnification even if we are sending a pinch start so we avoid this problem. And so on Windows we never dispatch because of this if

https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/layout/base/PresShell.cpp#8188

Comment 4

[Timothy Nikkel (:tnikkel)]

Looks like sending mPreviousSpan == mCurrentSpan is used in several other places. Should we be avoiding doing that in general? Or find a different way to solve this?

Comment 5

[Timothy Nikkel (:tnikkel)]

In the mean time I'll change direct manipulation to be like mac so we can fix this.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1648489. Send pinch starts events with some amount of scale instead of none with direct manipulation. r=kats

Comment 7

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

Looks like sending mPreviousSpan == mCurrentSpan is used in several other places. Should we be avoiding doing that in general? Or find a different way to solve this?

I think the choice to send mPreviousSpan == mCurrentSpan pre-dates any use of wheel events generated based on the pinch gestures so this consideration hasn't come up before.

APZ itself only seems to pay attention to the spans for PINCHGESTURE_SCALE events, so it should be safe to tweak places that send PINCHGESTURE_START events to make sure they have a nonzero span change and thus get dispatched to content. (The alternative would be to check for generated-from-pinch events specifically in IsAllowedToDispatchDOMEvent(), but I don't know if there's enough info on the wheel event to determine that, and it's probably not worth plumbing an extra field in.)

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #7)

(The alternative would be to check for generated-from-pinch events specifically in IsAllowedToDispatchDOMEvent(), but I don't know if there's enough info on the wheel event to determine that, and it's probably not worth plumbing an extra field in.)

There is a pre-existing mInputSource field on WidgetMouseEventBase (which is a superclass of WidgetWheelEvent) that we use for similar things, but I'm not sure it's a good idea to actually do this. Presumably web content isn't expecting to handle zero-delta wheel events and if we start sending those it might cause unexpected side-effects.

I think for now making PINCHGESTURE_START contain a nonzero span change seems like the right thing to do. It seems like this is going to be a trackpad-specific thing, because we only generate wheel events from trackpad pinches. So it's a fairly limited-scope problem, and we can revisit this later if we discover other cases where the solution could be unified with this one.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b21421230463
Send pinch starts events with some amount of scale instead of none with direct manipulation. r=kats

Comment 10

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/b21421230463

Comment 11

[Timothy Nikkel (:tnikkel)]

ni me to file a follow up.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)

I think for now making PINCHGESTURE_START contain a nonzero span change seems like the right thing to do. It seems like this is going to be a trackpad-specific thing, because we only generate wheel events from trackpad pinches. So it's a fairly limited-scope problem, and we can revisit this later if we discover other cases where the solution could be unified with this one.

So this means that we don't actually need a followup as the patch here has fixed all the cases that matter.