1721601 - Double tap with two fingers to zoom works on map websites such as google maps/bing maps while mousehover'ing the map itself

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Daniel Cicas [:dcicas], Release QA]

Affected versions

• Fx 91.0b5
  Fx 92.0a2 (BuildID: 20210721092353 )

Affected platforms

• macOS 11

Steps to reproduce

1. Start Firefox.
2. Reach google maps/bing maps.
3. Double tap to zoom on the map.

Expected result

• Double tap to zoom does not work on specific sites like google maps.

Actual result

• You can double tap to zoom in on map websites.

Regression range

• First bad: 2021-06-04
• Last good: 2021-06-03
• Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dc056be018fe8400010ef3169ff13ff739758a33&tochange=3d302847f025b332190a747a21bb1fedaa5e7b98
  Found commit message:
  Bug 1713589. If the double tap default zoom in pref is 1 make sure to fully disable the functionality. r=botond
  And then use this to disable the default zoom in in a couple of tests. The tablecell test specifically checks that we don't zoom in on tablecells. The hscrollable test checks that we don't scroll when double tapping and it would like to zoom out.

Comment 1

[Daniel Cicas [:dcicas], Release QA]

Hello, Timothy!

It seems that Bug 1713589 might have caused this regression, what do you think?

Comment 2

[Timothy Nikkel (:tnikkel)]

Thanks, I'll take a look.

Comment 3

[Pascal Chevrel:pascalc]

Tracking for 91 as this is S2 and a regression.

Comment 4

[Timothy Nikkel (:tnikkel)]

We are relying on not finding anything to zoom in on in the maps case so that we don't zoom in. However bug 1713589 changed it so that we zoom in a small amount if we can't find anything to zoom in on, in order to provide feedback to the user.

Chrome also does this small zoom in behaviour, however they support preventDefault on a wheel listener to prevent a double tap from zooming (maps web apps preventdefault wheel events because they generally do their own zooming), we do not, this is tracked in bug 1697766.

We can:

1. leave things as they are
2. implement preventDefault for double taps
3. backout bug 1713589 and no longer have this small zoom in behaviour.

Number 2) would require adding a bunch of fairly boilerplate code to apz, but I think it would be mostly straightforward.

Comment 5

[Pascal Chevrel:pascalc]

Thanks for the additional context, given that we are mid beta cycle, I think we should go with option 1 for 91, marking 91 as wontfix.

Comment 6

[Karl Dubost💡 :karlcow]

What do we mean by double tap here?

1. a double tap with two fingers
   this creates zoom in/zoom out of the full viewport
2. a double tap with one finger.
   this zooms in the map without issues
   with Firefox 92.0a1 (2021-07-28) (64-bit) on macOS 11.5 (20G71) [edited the version]

So this works for me as expected. Or maybe I'm not testing the right thing.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Karl Dubost💡 :karlcow from comment #6)

What do we mean by double tap here?

1. a double tap with two fingers
   this creates zoom in/zoom out of the full viewport

That's not what we want to happen. We don't want any zooming to happen.

2. a double tap with one finger.
   this zooms in the map without issues
   with Firefox 92.0a1 (2021-07-28) (64-bit) on macOS 11.5 (20G71) [edited the version]

That's what we want.

Comment 8

[Karl Dubost💡 :karlcow]

Attachment: double-tap.jpg

OK about 1. Double tap with two fingers.

• On Edge, it does nothing.
• On Safari, it does nothing.
• On Firefox, it zooms the viewport (not the map) This is a feature. It does the same thing for any articles. And it is working as expected.

For example, see the screenshot, before and after double tapping on the paragraph.
if I double tap again, it goes back to the full map.

That's cool. no?

Comment 9

[Karl Dubost💡 :karlcow]

ok so… indeed Dennis showed me the rationale wisdom. Indeed when on the map it should probably not do zoom at all. That makes sense.

Comment 10

[Timothy Nikkel (:tnikkel)]

Yeah, on the map itself. We don't want to zoom then because then we get two forms of zoom going on which is confusing.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Found a Chromium issue that they send a wheel event.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

There's an open webkit issue to provide a way to disable double tap zoom.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, dispatching wheel event sounds odd to me. Instead, Chromium guys should propose new event like beforezoom etc. wheel events whose all delta values are 0 may make web developers confused since it's hard to guess what the event means.

Usually, non-passive wheel event listeners is not used by documents (meaning non-web-apps). So, how about to stop the new feature of bug 1713589 if double tapped on elements whose ancestors have non-passive wheel event listeners for now?

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #13)

Usually, non-passive wheel event listeners is not used by documents (meaning non-web-apps). So, how about to stop the new feature of bug 1713589 if double tapped on elements whose ancestors have non-passive wheel event listeners for now?

It would fix this bug, but it feels a little "hacky" in that why would the presence of a non-passive wheel listener affect the behaviour (how much we decide to zoom) of a double tap, but not whether or not we do a double tap zoom at all?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I've assumed that he proposed disabling double tap zoom if there's any non-passive wheel event listeners?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not sure which one is better whether non-passive wheel event listeners affect to outside the double tapped elements or limited into the elements because 3rd party's script might add it to their own element.

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)

I've assumed that he proposed disabling double tap zoom if there's any non-passive wheel event listeners?

Okay, if we are talking about disabling double tap zoom entirely when there are non-passive wheel listeners. That would disable double tap in cases where the page preventDefaults wheel events that have ctrl modifier, but it would also disable double tap for any non-passive wheel listener, even if it does nothing with ctrl+wheel events. We wouldn't send a potentiall confusing wheel event to content, but it's another special case.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Simon Fraser told me on the WebKit slack, he'd oppose the zoom event to prevent double tap zooming, to be precise he'd oppose the way to prevent by using preventDefault() in the event handlers, he suggested that a right way is using something similar to touch-action property. That sounds more promising to me.

Anyway, that would be a long-term solution.

(Note that Safari started firing wheel events on pinch zoom since this changeset, but doesn't fire on double tap zoom)

Comment 19

[Timothy Nikkel (:tnikkel)]

This also affects things like google sheets.

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1721601. Move "zoom in if can't zoom out" from being a flag to part of ZoomTarget for AsyncPanZoomController::ZoomToRect. r?botond

Instead of passing this flag all of the time for double taps we want to decide to do this or not in CalculateRectToZoomTo. Moving it into ZoomTarget seems like a good way to do this (which is done in the next patch).

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1721601. If an element has a non-passive wheel listener don't do the "zoom in if can't zoom out" behaviour for macOS double tap zooms. r?botond

With our current double tap zoom behaviour some sites like google maps or google sheets will zoom in when double tapping. This happens because of the behaviour added in bug 1713589. The goal of this change was to provide some user feedback (by zooming 1.2x) that they had double tapped when we couldn't find anything on the page to zoom in on. Chrome had this behaviour and it seemed desirable (Safari does not have this). The reason that Chrome does not zoom in on the above mentioned sites is because Chrome implements sending content a wheel event that it can preventDefault as a way to prevent the zoom from the double tap from happening (similar to pinch zooming) (Safari does not implement this, and we currently do not have this) and those sites do this preventDefaulting. That means we are the only browser that suffers from this problem.

The behaviour of sending a wheel event to be potentially preventDefaulted by content is not favoured by all browser vendors. Other solutions have been proposed, but movement on this does not seem imminent.

So this work around was proposed to fix this for now: disable the "zoom in if can't find something to zoom in on" behaviour if there is a non-passive wheel event listener on the target element or some ancestor.

Depends on D128435

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1721601. Add test. r?botond,hiro

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fa95cb8ad3a
Move "zoom in if can't zoom out" from being a flag to part of ZoomTarget for AsyncPanZoomController::ZoomToRect. r=botond
https://hg.mozilla.org/integration/autoland/rev/fc0a680bc208
If an element has a non-passive wheel listener don't do the "zoom in if can't zoom out" behaviour for macOS double tap zooms. r=botond
https://hg.mozilla.org/integration/autoland/rev/3e844ac540bd
Add test. r=botond

Comment 24

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/9fa95cb8ad3a
https://hg.mozilla.org/mozilla-central/rev/fc0a680bc208
https://hg.mozilla.org/mozilla-central/rev/3e844ac540bd

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.