Closed
Bug 844326
Opened 12 years ago
Closed 12 years ago
Cannot scroll the Lanyrd Mobile app
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:tef+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
People
(Reporter: jsmith, Assigned: schien)
References
Details
(Keywords: regression, Whiteboard: [target 28/2])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
vingtetun
:
review+
justin.lebar+bug
:
feedback+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
Keywords: regression,
regressionwindow-wanted
Josh / SC, can one of you guys take a look?
Comment 3•12 years ago
|
||
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.
Updated•12 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 4•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-01-05&enddate=2013-01-06
Reporter | ||
Comment 6•12 years ago
|
||
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?
Reporter | ||
Comment 7•12 years ago
|
||
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
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
Shih-Chiang, please let me know if you're not the best owner here.
Assignee: nobody → schien
blocking-b2g: tef? → tef+
Updated•12 years ago
|
Whiteboard: [target 28/2]
Assignee | ||
Comment 10•12 years ago
|
||
(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?
Reporter | ||
Comment 11•12 years ago
|
||
(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.
stopPropagation() isn't supposed to cancel default pan/zoom behavior. That's the role of preventDefault().
Assignee | ||
Comment 13•12 years ago
|
||
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
Updated•12 years ago
|
status-b2g18:
--- → affected
status-b2g18-v1.0.1:
--- → affected
Comment 16•12 years ago
|
||
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)?
Attachment #718370 -
Flags: review?(21)
Comment 17•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 718370 [details] [diff] [review] detemine scrolling target at capturing phase Please re-ask review when you the previous question is answered.
Attachment #718370 -
Flags: review?(21)
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.
Attachment #718370 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 20•12 years ago
|
||
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
Attachment #718370 -
Attachment is obsolete: true
Attachment #718820 -
Flags: review?(21)
Attachment #718820 -
Flags: feedback?(jones.chris.g)
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 :).
Attachment #718820 -
Flags: feedback?(jones.chris.g) → feedback?(justin.lebar+bug)
Comment 22•12 years ago
|
||
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 ?)
Attachment #718820 -
Flags: review?(21) → review+
Comment 23•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #718820 -
Flags: feedback?(justin.lebar+bug) → feedback+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 24•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3d3cdc8632
Keywords: checkin-needed
Updated•12 years ago
|
QA Contact: ahubenya
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8f3d3cdc8632
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 26•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/937cae49de2e https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/bc28c44df88d
status-b2g18-v1.0.0:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
Comment 27•12 years ago
|
||
This issue is verified as fixed. The application scrolls now. Build: 20130301030548 Gecko: http://hg.mozilla.org/mozilla-central/rev/993d7aff3109 Gaia: 7f0a1d5341ac8870e70f64f543ec79a729bf717e
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•