Closed Bug 1150284 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla41
blocking-b2g 2.5+
Tracking Status
firefox38 --- unaffected
firefox39 + fixed
firefox38.0.5 --- unaffected
firefox40 + fixed
firefox41 --- fixed
firefox-esr38 --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- verified
fennec 39+ ---

People

(Reporter: ychung, Assigned: roc)

References

()

Details

(Keywords: dogfood, regression, Whiteboard: [3.0-Daily-Testing], gfx-noted)

Attachments

(2 files, 1 obsolete file)

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
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
[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.
blocking-b2g: --- → 3.0?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [3.0-Daily-Testing][systemsfe] → [3.0-Daily-Testing][systemsfe], gfx-noted
Blocks: spark
QA Contact: ktucker
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.
QA Whiteboard: [QAnalyst-Triage+]
Depends on: 1141252, 69787
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Yeah, commenting offsetX/Y out of MouseEvent.webidl fixes the bug, so it's definitely bug 69787.
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.
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.
.Ka is the current target element. It gets set to non-null when obfuscated function "Su[I].Ab" generates a mousedown for a touchstart.
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.
[Tracking Requested - why for this release]: affects Firefox for Android
tracking-fennec: --- → ?
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Component: Panning and Zooming → DOM: Events
tracking-fennec: ? → 39+
Rick, perhaps you have some input here, or can forward this to Google Maps team?
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.
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.
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. "
Flags: needinfo?(wjohnston)
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.
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.
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.
Adding mfinkle as a NI as wesj is completely on iOS these days.
Flags: needinfo?(mark.finkle)
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).
Flags: needinfo?(wjohnston)
Flags: needinfo?(mark.finkle)
Any updates here? Thats a pretty annoying bug if you dogfood the phone.
Keywords: dogfood
Severity: normal → major
blocking-b2g: 3.0? → 3.0+
Whiteboard: [3.0-Daily-Testing][systemsfe], gfx-noted → [3.0-Daily-Testing], gfx-noted
Bhavana, this should block Spark.
Flags: needinfo?(bbajaj)
blocking-b2g: 3.0+ → spark+
Flags: needinfo?(bbajaj)
Kats, it sounds like you're the right assignee for this. Please let me know if not.
Assignee: nobody → bugmail.mozilla
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.
Roc, did you look into this any further per comment 17?
Flags: needinfo?(roc)
I'm working on it.
Flags: needinfo?(roc)
(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.
(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.
I'm kinda stuck here. I'll try our contacts with the Maps team.
No longer blocks: spark
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.
Assignee: bugmail.mozilla → roc
Status: NEW → ASSIGNED
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Our concern is Firefox for Android for the 39 release which is 16 work days from now.
We can do the same thing there I guess. Google says "we assigned someone to work on it last week".
(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.
Flags: needinfo?(roc)
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.
Flags: needinfo?(rbyers)
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?
Attachment #8624036 - Flags: review?(bugs)
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.
Attachment #8624036 - Flags: review?(bugs) → review-
Attachment #8624138 - Flags: review?(bugs) → review+
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>?
Should we consider landing this only on release leaving Firefox for Android Beta as the platform to test against?
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!
(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! :)
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).
(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.
Yes, thanks. Sorry!
Flags: needinfo?(roc)
https://hg.mozilla.org/mozilla-central/rev/5e97f3b44653
https://hg.mozilla.org/mozilla-central/rev/953f8ac1e47e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
It sounds like we need this in Fennec but it just missed the beta 8 build. 

Can you request uplift please?
Flags: needinfo?(roc)
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
Flags: needinfo?(roc)
Attachment #8624138 - Flags: approval-mozilla-beta?
roc, what about 40?
Flags: needinfo?(roc)
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.
Attachment #8624138 - Flags: approval-mozilla-aurora+
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.
Attachment #8624138 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
This needs to be verified on Firefox for Android 39 and later versions.
Flags: needinfo?(fennec)
blocking-b2g: spark+ → 2.5+
Flags: needinfo?(rbyers)
Bug 1204841 reenabled the offsetX and offsetY properties.
Flags: needinfo?(fennec)
You need to log in before you can comment on or make changes to this bug.