844326 - Cannot scroll the Lanyrd Mobile app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 2/22/2013
Device: Unagi

Steps:

1. Install https://marketplace.firefox.com/app/lanyrd-mobile
2. Launch it
3. Try scrolling it

Expected:

It scrolls.

Actual:

It doesn't scroll. Works fine on FF Android. Definitely think this is a regression.

Comment 1

[Jason Smith [:jsmith]]

Broken tier 1 app. Needs to block.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Josh / SC, can one of you guys take a look?

Comment 3

[Angela Hubenya]

I was working on finding the regression window for this issue. The last time the app could be scrolled was on 2013-01-05-03-09-00, master build. The app does not scroll on any builds after that.

Comment 4

[Jason Smith [:jsmith]]

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-01-05&enddate=2013-01-06

Comment 5

[Jason Smith [:jsmith]]

I'm guessing this is a fallout of bug 823619.

Comment 6

[Jason Smith [:jsmith]]

Hmm...this is almost like the maps equivalent of bug 827270.

Do we do evangelism outreach here to fix this? Or is this a platform bug?

Comment 7

[Jason Smith [:jsmith]]

Answered in IRC:

jsmith	cjones: So is bug 844326 then an evangelism problem or a platform problem. It looks like the maps equivalent of bug 827270.

cjones	jsmith, need to find out what the bug is first, but seems like a platform issue

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

touchstart events have been |stopPropagation()| by the javascript loaded in this app, which is pretty similar to bug 827270. Here is the the JS stack:

(gdb) p xpc_PrintJSStack(cx, true, true, false)
$1 = 0x42890000 "0 anonymous() [\"https://s3.amazonaws.com/static.lanyrd.net/mobile-web-bundle.95f2235b.gz.js\":2]\n    this = [object Object]\n1 anonymous() [\"https://s3.amazonaws.com/static.lanyrd.net/mobile-web-bundle.95f2235b.gz.js\":2]\n    this = [object Object]\n"

Comment 9

[Andrew Overholt [:overholt]]

Shih-Chiang, please let me know if you're not the best owner here.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Andrew Overholt [:overholt] from comment #9)
> Shih-Chiang, please let me know if you're not the best owner here.
This bug is most likely to be an evangelism problem from my observation in comment 8. Do we have any contact window to the developers of Lanyrd app?

Comment 11

[Jason Smith [:jsmith]]

(In reply to Shih-Chiang Chien [:schien] from comment #10)
> (In reply to Andrew Overholt [:overholt] from comment #9)
> > Shih-Chiang, please let me know if you're not the best owner here.
> This bug is most likely to be an evangelism problem from my observation in
> comment 8. Do we have any contact window to the developers of Lanyrd app?

I thought Chris Jones indicated in IRC that this was definitely a platform problem.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

stopPropagation() isn't supposed to cancel default pan/zoom behavior.  That's the role of preventDefault().

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

BrowserElementPannning will not be able to receive touchstart event if the content javascript invokes |stopPropagation()|.

@cjones, we probably need to handle touchstart at capturing phase like what we discussed in bug 827270 previously. https://bugzilla.mozilla.org/show_bug.cgi?id=827270#c49

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: detemine scrolling target at capturing phase

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://tbpl.mozilla.org/?tree=Try&rev=48fa1b09fd88

Comment 16

[Andrew Overholt [:overholt]]

Comment on attachment 718370 [details] [diff] [review]
detemine scrolling target at capturing phase

Vivien, can you review this (not sure if cjones will be able to do so today)?

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 718370 [details] [diff] [review]
detemine scrolling target at capturing phase

What about using a systemEventListener instead http://mxr.mozilla.org/mozilla-central/source/content/events/public/nsIEventListenerService.idl#72 that would ensure we receive the event event if the content has called stopPropagation.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 718370 [details] [diff] [review]
detemine scrolling target at capturing phase

Please re-ask review when you the previous question is answered.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 718370 [details] [diff] [review]
detemine scrolling target at capturing phase

>diff --git a/dom/browser-element/BrowserElementPanning.js b/dom/browser-element/BrowserElementPanning.js

>+  onTouchStart_defaultPrevented: function cp_onTouchStart_defaultPrevented(evt) {

>+    this._resetActive();
>+    this.dragging = false;
>+    this.detectingScrolling = false;
>+    this._activationTimer.cancel();

We should share this code with onTouchEnd().

Looks good, r=me with that.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use addSystemEventListener to receive all touch events without affecting by stopPropagation.

Taking the advice in comment 17 from Vivien, we can avoid all the wired mouse/touch event sequences caused by stopPropagation().

https://tbpl.mozilla.org/?tree=Try&rev=5a2bbaf14ae6

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 718820 [details] [diff] [review]
use addSystemEventListener to receive all touch events without affecting by stopPropagation.

Seems like a better idea to me, but I don't know how system event listeners fit into DOM event semantics.

Are you sure this patch isn't going to leak the global?  (I honestly don't know.)

Out of my league, will defer to experts :).

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 718820 [details] [diff] [review]
use addSystemEventListener to receive all touch events without affecting by stopPropagation.

r+ But I would like to heard from Justin about leaking the global (I tend to think that this will be freed when the process will be killed in the worst case ?)

Comment 23

[Justin Lebar (not reading bugmail)]

We use the same idiom in BrowserElementChildPreload, so I don't see how this could be bad if that code is fine.  And hopefully we'd have noticed by now if that code was leaking.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3d3cdc8632

Comment 25

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/8f3d3cdc8632

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/937cae49de2e
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/bc28c44df88d

Comment 27

[Angela Hubenya]

This issue is verified as fixed. The application scrolls now.
Build: 20130301030548 
Gecko: http://hg.mozilla.org/mozilla-central/rev/993d7aff3109
Gaia: 7f0a1d5341ac8870e70f64f543ec79a729bf717e