1663731 - FNX-14556 ⁃ [Bug] pinch to zoom doesn't work on https://www.lightpollutionmap.info/ with simultaneous touch due to pointer events

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Amedyne Moya [:amoya]]

From github: https://github.com/mozilla-mobile/fenix/issues/13376.

Steps to reproduce

So this is an issue which has been reported in two cases with different sites using maps, but they don't seem to be using the same library.

Exhibit A
https://github.com/webcompat/web-bugs/issues/56090

1. Go to https://leafletjs.com/
2. Pinch to zoom

Exhibit B
https://github.com/webcompat/web-bugs/issues/54615

1. Go to https://www.lightpollutionmap.info/
2. Pinch to zoom

In both cases, this is working only if you put one finger, wait, and put a second finger, then making the gesture.

Device information

• Android device: 10, Pixel 2
• Fenix version: Firefox Preview 81

Comment 1

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

So far I'm not able to reproduce the problem.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

The lightpollutionmap.info seems quite busted in Fenix Nightly. Sometimes zooming will zoom all the elements of the page. Pinch zooming out will nearly always zoom in on the page. Tested using a Pixel 2XL and a Moto G5.

Comment 3

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

Hm. Now I can repro the lightpollutionmap problem. Seems kind of like putting down two fingers together triggers a double-tap zoom event.

For leaflet.js, I can sort of repro on release but not Nightly. It may have been fixed by bug 1648491.

Comment 4

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

Can you confirm that the leaflet.js site works for you on Nightly? Also, if you set dom.w3c_pointer_events.multiprocess.android.enabled and dom.w3c_pointer_events.enabled to false on Fenix (browser restart needed to be safe), do you still see the issues on lightpollutionmap.info? I don't, which makes me suspect it's a pointer events bug. The site has some slightly-obfuscated JS but it looks like it does some gesture detection with pointer events, so maybe we're sending bad info on the pointer events.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Leaflet is working mostly perfectly on nightly. Might have had a missed touch once or twice over a few minutes of interacting with the map box. Though it might have been a pinch outside the box border. Zooming in and out, double tap to zoom in, the buttons and using a pointing stick (s-pen) all functioned as expected. Used a Galaxy Note 9.

Yes, with touch events disabled the lightpollutionmap.info page behaves as I would expect. With a few minutes of usage I do not see any of the bad behavior I was seeing with pointer events enabled.

Comment 6

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

Great, thanks for confirming! I'm going to modify this bug to track just the lightpollutioninfo.map page, and move it to DOM events.

Comment 7

[David Lawrence [:dkl]]

Changed the default type to 'defect' for the component.

Comment 8

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

I assume you meant to post that to bug 1666562?

Comment 9

[David Lawrence [:dkl]]

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

I assume you meant to post that to bug 1666562?

Oops. yeah. sorry bout that :)

Comment 10

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

This is also reproducible in Windows build and is regressed by:

Comment 11

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Turning off apz.allow_zooming fixes the issue on both Android and Windows. :kats, mind taking a look?

Comment 12

[Botond Ballo [:botond]]

The site uses the OpenLayers library. We have an existing issue on file related to pointer events involving the OpenLayers library: bug 1607463. This may be another manifestation of the same isssue.

Comment 13

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

allow_zooming does not affect the mentioned DuckDuckgo map and Bing Maps, so I think they are different.

Comment 14

[Botond Ballo [:botond]]

The issue is related to us getting into this code block (and therefore sending a pointercancel event) when we shouldn't (or at least, when the site / library is not expecting us to).

Comment 15

[Botond Ballo [:botond]]

Unlike the diagnosis in bug 1607463 comment 3, here the site does not appear to be preventDefault()ing the pointerdown event in the first place, suggesting a possible site issue.

Comment 16

[Botond Ballo [:botond]]

Possibly the sites in question have since upgraded to a version of OpenLayers which includes this PR, titled "Do not preventDefault on pointerdown".

Comment 17

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

Calling preventDefault() on pointerdown should not be relevant here; that should only prevent some mouse compat events from being dispatched.

That being said, if we are triggering a pointercancel via the block of code you linked in comment 14, then it's because APZ determines it can consume the events with some default action. This makes sense when apz.allow_zooming=true because the pinch action is permitted. If the page wants to override that behaviour it should really be setting a touch-action property that disallows pinching. So touch-action:none or maybe touch-action: pan-x pan-y.

I'm not entirely sure why it works in Chrome though.

Comment 18

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

I filed https://github.com/openlayers/openlayers/issues/11614 to hopefully arrive at a solution.

Comment 19

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

Looking at our code a bit more it's possible that we're erroneously entering that pointercancel block too early. Normally we wait for both the touchstart and (if the touchstart wasn't preventDefaulted) the first touchmove before notifying APZ of the content response. However what I'm seeing here is that we send the pointercancel right after the second touchstart, instead of waiting for the touchmove on that block. This might be because sentContentResponse gets set to true here but the condition at this line is really wanting to know if the current event triggered a content response. In this case the sentContentResponse was set to true for the previous event and so that might be a bit of a mismatch.

Try push with that changed to see if it breaks anything and/or fixes the bug: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=d3cf499902514bcfafc10ac71ad6f58dccd26b6b

Comment 20

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

(Parking with me for now)

Comment 21

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

Patch seems to work, both on Windows and in GVE. Also doesn't seem to fail any existing tests, so it should be good.

Comment 22

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

Final patch with test: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=f5e793fb52db30bdd48deebfad50534ceee0d55c

Comment 23

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

Attachment: Bug 1663731 - For two-touch gestures, don't send the pointercancel too early. r?botond

The existing code would send the pointercancel to web content as soon as the
second touchstart event was triggered for a pinch gesture. Instead, we want
to wait and see if the subsequent touchmove is cancelled before doing that.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7006f2f19b52
For two-touch gestures, don't send the pointercancel too early. r=botond

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7006f2f19b52

Comment 26

[Laurentiu Apahidean]

Verified as fixed on Firefox Preview Nightly 201025 (Build #2015771787) using a Pixel 3 XL (Android 9) and Samsung Galaxy S9 (Android 8.0.0).