Zooming on duckduckgo maps zooms fixed position elements too

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: jnicol, Assigned: kats)

Tracking

65 Branch
mozilla67
Unspecified
Android
Points:
---

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

()

Attachments

(3 attachments)

Reporter

Description

3 months ago

STR:

  1. visit duck duck go map page on android device - https://duckduckgo.com/?q=london&t=hk&ia=news&iax=about&iaxm=about
  2. attempt to zoom map, by moving two fingers apart over the map

Expected: the map zooms in, loads more details and the scale adjusts, etc.
Actual: the entire page is zoomed by firefox, including the fixed-position information panel

This behaviour was changed by bug 1465616. It wasn't correct prior to that either, just differently wrong. Chrome works as expected.

Reporter

Comment 1

3 months ago

That link doesn't work correctly. Add step 1.5 - click "Maps" in the menu under the search bar

Reporter

Comment 2

3 months ago

We also have another problem here which might be worthy of a separate bug - when zoomed far in the fixed position layer becomes very large. And since it's not scrollable we use a SingleTiledContentClient for it, which doesn't get clipped by the critical display port and we OOM quite easily.

The issue here is whether the pinch gesture is handled by the browser (APZ), or the page.

By default, pinch gestures are handled by the browser. The page has several mechanisms of disabling that behaviour, e.g. preventDefault()ing the second touch-down event, or by using touch-action. Mapping applications generally want to use one of these mechanisms.

It appears that in Chrome, the page succeeds at preventing the browser from handling the pinch gesture. In Firefox, it does not, and so we get the browser's pinch-zoom behaviour (which since bug 1465616 includes zooming fixed-position elements).

So, the thing to investigate here, is what mechanism the page is using to tell the browser not to handle the pinch, and why that mechanism is breaking down in Firefox.

(In reply to Jamie Nicol [:jnicol] from comment #2)

We also have another problem here which might be worthy of a separate bug - when zoomed far in the fixed position layer becomes very large. And since it's not scrollable we use a SingleTiledContentClient for it, which doesn't get clipped by the critical display port and we OOM quite easily.

I agree that this is worth tracking as a separate bug. Even though for this site the fix we want is to make sure the browser doesn't handle the pinch (which will avoid the OOM problem), there could be other sites that do want the browser to handle a pinch and that also have large fixed-position elements, where we can run into this OOM.

Based on some brief investigation, it looks like the site is preventDefault()ing the first touch-start, but not the second?

That should also prevent browser pinch zooming.

Reporter

Updated

3 months ago
See Also: → 1529892

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)

That should also prevent browser pinch zooming.

Actually that's not specced anywhere. It appears that we currently do not prevent browser pinch zooming if just the first touch-start is prevented, whereas other browsers do. We could probably modify our behaviour to match.

In terms of implementation I think APZEventState.cpp should track whether or not the first touchstart got a preventDefault, and then || that into the response for the second touchstart.

Another alternative is to do it on the APZ side, where we copy the flag from the first TouchInputBlock to the second. But that has a couple of problems: one is that APZ can't distinguish between the cases where the touchstart was prevented vs the first touchmove was prevented. And I think if the first touchmove is prevented that has a slightly different semantics, and I wouldn't expect the pinch to be skipped. There's probably some other bugs that are a result of this failure to distinguish scenarios. The other problem is that we might create the second touch block before we get a content response from the first touch block, and in that scenario we'd have to go looking for the second touch block when we do get a content response for the first one, to make sure to propagate the preventDefault flag. This seems a little icky, and I think it would cleaner in APZEventState.cpp.

Priority: -- → P3

I have a WIP for this. Writing a test is giving me headaches though.

Assignee: nobody → kats

Other mobile browsers disallow browser-based pinch zooming when the
first touchstart is preventDefaulted, even if the second one is not. We
allowed pinch zooming in that scenario. This patch makes it so that if
the first touchstart is preventDefaulted, then subsequent touchstart
events are also preventDefaulted, which brings our behaviour in line
with that of other browsers.

Comment 12

3 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd326f5e4eb8
Move TouchCounter to apz/util/ and allow counting WidgetTouchEvent too. r=botond
https://hg.mozilla.org/integration/autoland/rev/77625f533af6
Propagate preventDefault from first to subsequent touchstarts. r=botond
https://hg.mozilla.org/integration/autoland/rev/ee394d0b085d
Add a test for preventDefault on pinch zooms. r=botond

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=3fac96a70b5977b4351ce2d1b223a05f97369229 looks better

I wasn't waiting for the touch sequence to actually get processed by APZ which caused some weirdness. Also it turns out the WidgetTouchEvent semantics is different from MultitouchInput so I had to tweak the TouchCounter code a bit.

Flags: needinfo?(kats)

Comment 15

3 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea87977f029e
Move TouchCounter to apz/util/ and allow counting WidgetTouchEvent too. r=botond
https://hg.mozilla.org/integration/autoland/rev/0ff8915bda3d
Propagate preventDefault from first to subsequent touchstarts. r=botond
https://hg.mozilla.org/integration/autoland/rev/89e9e71a6b0c
Add a test for preventDefault on pinch zooms. r=botond

Comment 17

3 months ago

I am still able to reproduce this while the page is loading. I wonder if that is a factor.

After loading the page can't zoomed out. After pressing back the page is still zoomed in.

(In reply to csheany from comment #17)

I am still able to reproduce this while the page is loading.

Seems to happen in Chrome as well. Most likely the page is not registering the touch listeners until load is complete.

Comment 19

3 months ago

Does the zoom persist also?

Yes. Although it's not clear to me what scenario you're referring to here. I tested both of the ones I thought you might be referring to:

A) Paste URL in URL bar, zoom in, and then click on the "Maps" tab to go to the maps tab. The maps tab loads in-place with the zoom. Clicking back retains the zoom.
B) Paste URL in URL bar, click on the "Maps" tabs, zoom in while it's loading, click on a link to navigate away, and then click back to go back to the maps page. The zoom is retained.

Comment 21

3 months ago

C? :)

  1. Paste URL in URL bar
  2. Click on the "Maps tab
  3. Zoom in while it's loading
  4. Click < Search Results

Comment 22

3 months ago

A doesn't seem right either.

At least when the "Maps" tab initially loads.

The zoom is retained in (C) as well. Based on the way the page is behaving I'm pretty sure clicking on the maps tab doesn't navigate to a new page, it just repopulates the DOM with the maps tab. So whatever zoom you had before is what you're going to stay at. This will be the same across all browser. Hitting back on the maps or the little arrow to go back to the search results also stays on the same page.

Comment 24

3 months ago

Ok.

It was starting to sound like Bug 1511823 but maybe not.

FWIW, it does seem to create a new history entry.

What's unfortunate is that the panel can be scrolled off screen.

You need to log in before you can comment on or make changes to this bug.