1150284 - [Browser] Unable to zoom in/out on Google Maps

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20150401_1639.txt

Description:
The Google Maps page cannot be zoomed in or out when the user pinches the screen to zoom. The user is still able to pan around the map page, though.

Pre-requisite: Have Wi-Fi or Data Connection connected.

Repro Steps:
1) Update a Flame to 20150401010204.
2) Go to Google Maps page.
3) When the page is fully loaded, pinch to zoom in or out on the screen.

Actual:
The webpage does not zoom in or out.

Expected:
The webpage does zoom in or out as expected.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150401010204
Gaia: 03164bd160809747e6a198e0dba1b7c3ee7789f5
Gecko: 18a8ea7c2c62
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Repro frequency: 10/10
See attached: video clip, logcat
https://youtu.be/07l-SEAOknw

Comment 1

[Yeojin Chung [:YeojinC]]

This issue does NOT reproduce on Flame 2.2.

Result: The webpage does zoom in or out as expected.

Device: Flame 2.2 (KK, 319mb, full flash)
Gaia: 8b3086ad3963f1707e2bee9094baccafffe161c4
Gecko: 20b67213a047
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

This is a regression so nominating this 3.0? The user should be able to zoom in on maps without issue.

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

Mozilla-Inbound Regression Window:

Last working Mozilla-Inbound build:
Environmental Variables:
Device: Flame 3.0
BuildID: 20150313035721
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 2b9f5019abf1
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

First broken Mozilla-Inbound build:
Environmental Variables:
Device: Flame 3.0
BuildID: 20150313045428
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 4100738d662d
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Working Gaia with Broken Gecko issue DOES reproduce:
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 4100738d662d

Working Gecko with Broken Gaia issue does NOT reproduce:
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 2b9f5019abf1

Mozilla-Inbound Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2b9f5019abf1&tochange=4100738d662d

This looks to be caused by the landings for either bug 1141252 or bug 69787

Robert, can you take a look at this please? We may need the offending commit to be backed out.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's probably bug 69787, but that's not something we just want to back out, since sites depend on it. I'll try to figure out exactly what the issue is and if we can work around it.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, commenting offsetX/Y out of MouseEvent.webidl fixes the bug, so it's definitely bug 69787.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is that when offsetX is defined, we go down a different code path in Google Maps which relies on MouseEvent.target not being null. But for some reason, in Firefox it is null :-(. That doesn't seem right.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The target is null because the event never gets dispatched. Obfuscated function "Uu" in common.js is this:
function Uu(a, b, c) {
    c = c.changedTouches;
    var d = c[oo](c[H] - 1);
    c = ca[Wo]("MouseEvents");
    c.initMouseEvent(b, !0, !0, k, 1, d[vn], d[un], d[xo], d[yo], !1, !1, !1, !1, 1, null);
    c.j = !0;
    (d = a.Ka) && d[wn] && d[wn](c);
    R[n](a, b, c)
}

d[wn](c) would call dispatchEvent but it doesn't run because a.Ka is null. I don't know what a.Ka is yet.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

.Ka is the current target element. It gets set to non-null when obfuscated function "Su[I].Ab" generates a mousedown for a touchstart.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, looking at this code, I think I understand the problem now! The relevant code is:
Su[I].Ab = function(a) {
    this.F && this.F(a);
    if (!bp(a)) {
        this.ra.msSetPointerCapture && this.ra.msSetPointerCapture(a[fo]);
        var b = a[Jo];
        this.Ka = null;
        1 == b[H] && (this.Ka = b[oo](0)[jd]);
        Tu(this) && a[Db]();
        Uu(this, "mousedown", a);

When there's more than one touch (i.e. during a pinch-zoom), this code sets Ka to null. Then later we get mousemove created but not dispatched, triggering the bug.

It's not clear to me how this code is supposed to work, or why it would work in other browsers.

I need to try reproducing this on a desktop machine with touch events. Next week. Assuming it reproduces there, I can maybe try comparing how Chrome behaves.

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: affects Firefox for Android

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Rick, perhaps you have some input here, or can forward this to Google Maps team?

Comment 12

[Rick Byers]

Weird.  Chrome has MouseEvent.offsetX/offsetY too so I'm not sure why this would trigger a different codepath.  Presumably there's something different about how Firefox and Chrome handle multi-touch events, but I don't see anything obvious.  If it helps, I believe maps avoids calling preventDefault on the touchstart, cancelling touchmove only after it's seen one outside a certain bounding box (though not sure what they do on pinch).  I'll see if I can track down someone in the maps team who might be able to help.

Comment 13

[Rick Byers]

Oh actually I was making a mistake in my testing.  Just tried again.  On FF 37.0.2 for Android, if I call preventDefault only on touchmove (not touchstart) then I don't get touchmove events for a pinch gesture (instead it tries to pinch).  Test using http://rbyers.net/eventTest.html.  Both Safari and Chrome give JS a chance to suppress the pinch by cancelling the touchmove (so that a site can disable scroll/pinch without also disabling tap).  Not sure how this would be connected to offsetX/Y but it seems like it could explain why Maps zoom code wouldn't be working at all.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Thanks, that sounds like an issue on Gecko's Android touch event handling.

The spec says 
"If the preventDefault method is called on the first touchmove event of an active touch point, it should prevent any default action caused by any touchmove event associated with the same active touch point, such as scrolling. "

Comment 15

[Rick Byers]

Right.  It looks like it works fine for scrolling, but I'm seeing really odd behavior in the zoom case.  Often I get contextmenu events and text selection when doing a pinch zoom gesture that cancels only touchmove, not touchstart.  It also seems inconsistent whether I'll get touchmove events in that case or not.

In case it helps, we went through some effort to try to formulate precisely how cancelling different touch events affects the gestures we create.  Details here: https://docs.google.com/document/d/176xYUC3WbiSl7qd08SBW4NiI4_gO0N4PN6tPAq49-68/edit with a nearly literal translation into code here: https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_detection/touch_disposition_gesture_filter.h&q=TouchDisposition&sq=package:chromium&type=cs&l=17.

Comment 16

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

In Fennec (and B2G, for that matter) we don't distinguish between a preventDefault on the touchstart vs a preventDefault on the first touch-move. In both cases the block of events is thrown away. If you're doing a preventDefault only on the touch block from the second finger, then I could see how the first finger might end up triggering long-tap events (contextmenu or text selection). We should probably fix that.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Unfortunately I don't see how different preventDefault handling could lead to the issues I described in steps 6-9. They may be caused by an unrelated bug. I'm going to keep digging into that.

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

Adding mfinkle as a NI as wesj is completely on iOS these days.

Comment 19

[Wesley Johnston (:wesj)]

kats knows more than me or finkle do about how this works anymore, and he's pretty involved here. If I can help you (kats) feel free to ping me (but just glancing at the platform code, its changed completely since I was involved and is changing more).

Comment 20

[Gregor Wagner [:gwagner]]

Any updates here? Thats a pretty annoying bug if you dogfood the phone.

Comment 21

[Doug Sherk (:drs) (inactive)]

Bhavana, this should block Spark.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Kats, it sounds like you're the right assignee for this. Please let me know if not.

Comment 23

[Rick Byers]

Sounds like you've got a handle on the problem and don't need me anymore.  Feel free to cc me again if there's anything I can do from the Chrome/Google side.

Comment 24

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

Roc, did you look into this any further per comment 17?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm working on it.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> I need to try reproducing this on a desktop machine with touch events. Next
> week. Assuming it reproduces there, I can maybe try comparing how Chrome
> behaves.

Unfortunately, on Linux desktop, embedded Google Maps don't listen for touch events in either Chrome or Firefox.

However, the maps.google.com app listens for touch events, and zoom gestures work in desktop Linux Firefox! (This is with the patch in bug 978679, to enable touch events on desktop Linux.) Originally I had selectioncaret.enabled set to true, and it didn't work with that, so maybe that's the problem. Need to try that on the phone.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> The problem is that when offsetX is defined, we go down a different code
> path in Google Maps which relies on MouseEvent.target not being null. But
> for some reason, in Firefox it is null :-(. That doesn't seem right.

On desktop Firefox, with touch events enabled, this doesn't happen; offsetX is never called when target is null. I'll recheck, but I'm guessing FirefoxOS is being sent a different "mobile version" that has this problem.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm kinda stuck here. I'll try our contacts with the Maps team.

Comment 30

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

If you hit a dead end feel free to send it back to me and I can try to do something about this in gecko.

Note that the secondary issue with the contextmenu popping up mentioned in comment 15 should be fixed with bug 1158617.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> If you hit a dead end feel free to send it back to me and I can try to do
> something about this in gecko.

Thanks for the offer but I kinda doubt we can do anything here. Google engineers have acknowledged this as a Maps bug. I have the Google bug number if anyone needs it.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Robert, what is the google bug number? Do you think they will fix this in time for us to make sure it's fixed before 39 moves to release? If not, is there any sort of temporary fix we can put in place?

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The Google bug is 20820906. I've just pinged them for an update.

The only temporary fix I can think of is to disable the offsetX/Y attributes on FirefoxOS only. This will work around the Google Maps bug but break other sites.

Comment 34

[Kevin Brosnan [Ex-Mozilla]]

Our concern is Firefox for Android for the 39 release which is 16 work days from now.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can do the same thing there I guess. Google says "we assigned someone to work on it last week".

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> We can do the same thing there I guess. Google says "we assigned someone to
> work on it last week".

Roc, any update? At this point I'd like to get a patch ready assuming that Google won't get this fix out in time for release.

Comment 37

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

When I talked to Rick last week he mentioned he was working with the maps team on this issue, perhaps he can provide an update.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=835e258eeb4d

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Disable offsetX/offsetY properties in FxOS/Fennec

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8624036 [details] [diff] [review]
Disable offsetX/offsetY properties in FxOS/Fennec

Review of attachment 8624036 [details] [diff] [review]:
-----------------------------------------------------------------

I really hate having to do this, but I don't think we have a better option.

Of course, once we do this, it'll be hard for the Maps team to test their fix. Maybe they can use the current version of Fennec?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8624036 [details] [diff] [review]
Disable offsetX/offsetY properties in FxOS/Fennec

I would add [Pref="dom.mouseEvent.offsetXY.enabled"] to the properties in .webidl and then enable the pref in firefox.js.
(we have all these great annotations in .webidl, so I'd prefer to use those rather than preprocessing)

Change to dom/events/test/mochitest.ini would be still needed, but
dom/webidl/moz.build wouldn't be.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Disable offsetX/offsetY properties in FxOS/Fennec

Comment 43

[Rick Byers]

I'm really sorry for the delay on this guys, its unacceptable as far as I'm concerned.  I've escalated my attempts to get this fixed ASAP.  Internally my argument has been that if an app can't justify supporting a particular browser, then they should remove any code paths specific to that browser UA string so that it goes down the same path as for a browser that is supported (ideally the spec compliant path).  Is that fair?

If these sorts of issues continue to be a problem for you I'm open to chatting about more radical options.  Eg. if we can get our interoperability up high enough, then perhaps Chrome should randomly choose to send a Mozilla UserAgent string sometimes (and vice versa) <grin>?

Comment 44

[Kevin Brosnan [Ex-Mozilla]]

Should we consider landing this only on release leaving Firefox for Android Beta as the platform to test against?

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

We should land the patch everywhere to get as much testing as possible, and once
Google has fixed maps, then back it out.


And thanks Rick!

Comment 46

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

(In reply to Rick Byers from comment #43)
> Internally
> my argument has been that if an app can't justify supporting a particular
> browser, then they should remove any code paths specific to that browser UA
> string so that it goes down the same path as for a browser that is supported
> (ideally the spec compliant path).  Is that fair?

Thanks Rick, that sounds fair to me. App authors should generally assume unknown/not-explicitly-supported browsers are spec compliant. After all, that's the whole point of having a spec! :)

Comment 47

[Rick Byers]

BTW, I assume this issue is the same between Firefox OS and Firefox for Android.  I can reproduce on FF for Android Beta (39.0) but not stable (38).

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e97f3b44653

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/953f8ac1e47e

Comment 50

[Bob Owen (:bobowen)]

(In reply to Pulsebot from comment #49)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/953f8ac1e47e

I just added mulet to the skip-if, as it was failing on inbound.
Hopefully that was the right course of action.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, thanks. Sorry!

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e97f3b44653
https://hg.mozilla.org/mozilla-central/rev/953f8ac1e47e

Comment 53

[Liz Henry (:lizzard) (relman/hg->git project)]

It sounds like we need this in Fennec but it just missed the beta 8 build. 

Can you request uplift please?

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8624138 [details] [diff] [review]
Disable offsetX/offsetY properties in FxOS/Fennec

Approval Request Comment
[Feature/regressing bug #]: 69787
[User impact if declined]: Google Maps zoom not working
[Describe test coverage new/current, TreeHerder]: automated tests
[Risks and why]: low risk, just hiding some DOM attributes on mobile
[String/UUID change made/needed]: none

Comment 55

[Liz Henry (:lizzard) (relman/hg->git project)]

roc, what about 40?

Comment 56

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8624138 [details] [diff] [review]
Disable offsetX/offsetY properties in FxOS/Fennec

[Triage Comment]
I am pretty sure we want that also in 40.

Comment 57

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8624138 [details] [diff] [review]
Disable offsetX/offsetY properties in FxOS/Fennec

We should take this for 39 and so need to land it on both m-b and m-r.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f1107d829946
https://hg.mozilla.org/releases/mozilla-release/rev/7ba0c0811b23

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/470f4457f01d

Comment 60

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed on the latest 3.0 Nightly Flame build.

Actual Results: Google Maps can be zoomed in and out.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150624160209
Gaia: eb0d4aefa62b20420d6fa0642515a110daca5d97
Gecko: 7b0df70e27ea
Gonk: a4f6f31d1fe213ac935ca8ede7d05e47324101a4
Version: 41.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Comment 61

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is also fixed with the latest Spark build on Aries.

Actual Results: Google maps can be zoomed in and out when desired.

Environmental Variables:
Device: Aries 3.0
BuildID: 20150625095718
Gaia: 038e917076271d304b906a41b4de670e505c67ae
Gecko: 0b2f5e8b7be5
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 41.0a1 (3.0) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Comment 62

[Kevin Brosnan [Ex-Mozilla]]

This needs to be verified on Firefox for Android 39 and later versions.

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 1204841 reenabled the offsetX and offsetY properties.