Closed
Bug 985867
Opened 11 years ago
Closed 11 years ago
Regression: Tapping in the add-ons page opens the same page in a new tab, unable to search for add-ons
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox28 wontfix, firefox29 wontfix, firefox30 verified, firefox31 verified, firefox32 verified, fennec31+)
VERIFIED
FIXED
Firefox 32
People
(Reporter: csuciu, Assigned: wesj)
References
Details
(Keywords: regression)
Attachments
(3 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Nightly 31.0a1 (2014-03-19)
Samsung Galaxy Tab (4.0)
Asus Transformer Pad (4.2)
HTC Desire HD (2.3)
1. Start Nightly with a clean profile
2. Open add-ons page via Menu > Tools > Add-ons
3. Tap anywhere on the page
Result: A new add-ons page opens in a new tab
Reporter | ||
Comment 1•11 years ago
|
||
Last good revision: 8abc76dedec2 (2014-03-01)
First bad revision: 0085a162499f (2014-03-02)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8abc76dedec2&tochange=0085a162499f
Comment 2•11 years ago
|
||
I'm not able to reproduce this on my Nexus 5.
Comment 3•11 years ago
|
||
I can reproduce this on LG Nexus 4 (Android 4.4.2).
For example. I wanted to install "Stylish" and after tapping it, a new tab opened with "addons.mozilla.org/en-US/android". In the end I had over 20 tabs opened and couldn't install it, only after closing the tabs opened.
Updated•11 years ago
|
Flags: needinfo?(teodora.vermesan)
Keywords: regression,
regressionwindow-wanted
Comment 4•11 years ago
|
||
Regression window wanted:
good build: 01-03-2014
bad build: 02-03-2014
pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8abc76dedec2&tochange=0085a162499f
Flags: needinfo?(teodora.vermesan)
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Updated•11 years ago
|
tracking-fennec: --- → ?
Comment 5•11 years ago
|
||
This changeset looks suspicious:
http://hg.mozilla.org/mozilla-central/rev/ad1574c13ebf
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Summary: Tapping in the add-ons page opens the same page in a new tab → Regression: Tapping in the add-ons page opens the same page in a new tab, unable to search for add-ons
Comment 8•11 years ago
|
||
Wes - This is already on Aurora. Is bug 973348 to blame? If so, why is it happening with about:addons? Are we doing something in about:addons that triggers this?
Reporter | ||
Comment 9•11 years ago
|
||
Keywords: regressionwindow-wanted
Updated•11 years ago
|
tracking-fennec: ? → 30+
Assignee | ||
Comment 10•11 years ago
|
||
This was not a regression from bug 973348.
No longer blocks: 973348
Assignee | ||
Comment 11•11 years ago
|
||
AFAICT, This is happening in the platform. For some reason the target of the first touchstart is set to this div (even though you are far away from it), which we then use as our touch event target. Trying to figure out why...
Assignee | ||
Comment 12•11 years ago
|
||
Two things here. We're opening a new tab on "click". Something's changed recently so that we're dispatching events as:
touchstart
mousemove
mousedown
mouseup
touchend
That means the click fires before the touchend. Then, when touchend does run the getContentFrame() at:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7117
returns null. I'm not sure why exactly. Still looking. But that causes it to never really fire and the event isn't removed from our list of pending events. When you return to the page and click again we fire the event (that happens to have the same id) at the same target.
I suspect the change in event ordering is because of the new doubletap handling, but I'm not quite sure why. You know kats?
Our tests for touchevents this fire events use DOMWindowUtils, so we don't really test this code. We could and should though :)
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 13•11 years ago
|
||
Ahh, so to reproduce this my STR are:
1.) Open about:apps
2.) Tap the "Browse all Add-ons" row
3.) Wait for the page to load. Hit back.
4.) Tap anywhere on the screen.
I can reproduce with those steps even on release builds. I think the problem with click events firing between touchevents has been present ever since we supported quick clicking in pages with user scalable turned off (our GestureDetectors fire before we send the events to Gecko).
In onclick, we open a new tab (via BrowserApp.addTab()). The touchend event then fires after that new tab has opened, so it fires in the new tab, which angers the PresShell because the event is trying to fire in a different frame than its target.
I think we need to fix this event ordering bug. I can reproduce it with a test page, but
1.) Any touch listeners in the page make it disappear, and
2.) the page uses window.open, which doesn't trigger the same bad behavior in presshell.
I'm still not sure why these events fire in order sometimes and out-of-order others. I don't see any way that events can get sent in the right order...
Comment 14•11 years ago
|
||
Thanks, that explanation makes sense, somewhat. Within the regression range, it was probably introduced by the backout of bug 960146, because that backout fixed touch listeners. Likely another bug (maybe 941995) which landed while 960146 was still in the tree caused this to happen in this case.
But yes, I agree that we need to fix the event ordering. There was a similar issue on B2G a few months ago (bug 965381) where I did the equivalent of setTimeout(,0) so that the touch event was guaranteed to be handled before the click event. We can do that here too.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 15•11 years ago
|
||
This posts the click as a new task to the PanZoomTarget (which should post to the UI Thread). I really wanted to post to the "current" thread here, but I think this code should be running on the UIThread, so this is the same.
Attachment #8405533 -
Flags: review?(bugmail.mozilla)
Updated•11 years ago
|
Attachment #8405533 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Reporter | ||
Comment 18•11 years ago
|
||
I can not reproduce the bug on Nightly 31.0a1 (2014-04-13) using the steps from comment #0 or comment #13
The bug is present on all the other branches and can be reproduced only by following the steps from comment #13.
Assignee | ||
Comment 19•11 years ago
|
||
backed out while I try to repro/fix bug 997289:
https://hg.mozilla.org/integration/fx-team/rev/1c075c91c7bf
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 20•11 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #19)
> backed out while I try to repro/fix bug 997289:
> https://hg.mozilla.org/integration/fx-team/rev/1c075c91c7bf
FYI, this had the wrong bug number in the commit message (bug 985967). If you aren't using the qbackout extension for these kind of things, I recommend you give it a try :)
Target Milestone: Firefox 31 → ---
Comment 21•11 years ago
|
||
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/1c075c91c7bf
Comment 22•11 years ago
|
||
Backout fixed bug 997289.
Comment 23•11 years ago
|
||
Is this a dupe of bug 890237?
Updated•11 years ago
|
Status: REOPENED → ASSIGNED
status-firefox32:
--- → affected
Comment 24•11 years ago
|
||
Do we have an update here? This is currently on Beta 4.
Comment 25•11 years ago
|
||
I am able to reproduce this on Marketplace too at first time I try to install a webapp (make a tap in the page)
PS: This was fixed with the previous patch in April 14 that was back out .
Comment 27•11 years ago
|
||
> 11:43 sawrubh: are addons supported for Fx-Android?
> 11:43 andym: sawrubh: yes
> 11:43 sawrubh: I try installing, get directed to the addons site, when I tap in the search box, the page just keeps on reloading
> 11:44 sawrubh: also when I do Tools->Addons->Your Addons, I still get directed to the addons website
> 11:45 sawrubh: andym: and I'm on the latest Nightly
> 11:46 sawrubh: and in fact the page doesn't reload when I tap on the search box on the addons website, it opens a new tab :o
User's are running into this still
Comment 29•11 years ago
|
||
Comment 13 mentions some things that make this bug go away. We could add a hack to the about:addons page to at least make this issue go away for the one case where it is currently happening. We can keep a testcase around to fix the event ordering.
Wes?
Comment 30•11 years ago
|
||
An in-page workaround: long-press select the text in the button you're about to click, then tap the arrow on the right hand side. Reliably causes AMO to function for me.
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #29)
> Comment 13 mentions some things that make this bug go away. We could add a
> hack to the about:addons page to at least make this issue go away for the
> one case where it is currently happening. We can keep a testcase around to
> fix the event ordering.
>
> Wes?
The workaround is basically to throw yourself out of the fast-click path. i.e. either Registering touch event listeners means we have to listen for them to reply before clicking or changing the viewport to... whatever it is now that disables fast-clicks. None of them are particularly great.
This bug is already shipping on release and has been since NativeFennec 1.0. I'd prefer not to push for a hack to fix it. I let it slip off my radar, but I can bump it up in priority. I haven't been able to reproduce the Google bugs on any devices I use regularly. Digging into my stache today to see if it happens on any of them.
Flags: needinfo?(wjohnston)
Assignee | ||
Comment 32•11 years ago
|
||
Here's the hacky fix. Touch event listeners should be slightly faster than just a viewport fix... I think.
Attachment #8430292 -
Flags: review?(mark.finkle)
Updated•11 years ago
|
Attachment #8430292 -
Flags: review?(mark.finkle) → review+
Updated•11 years ago
|
tracking-fennec: 30+ → 31+
Assignee | ||
Comment 33•11 years ago
|
||
Comment 34•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Assignee | ||
Comment 35•11 years ago
|
||
Comment on attachment 8430292 [details] [diff] [review]
Hack fix
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Always. Lucas showed me this today. I think on release it presents slightly differently because of the change in bug 973348.
User impact if declined: Clicks just open amo
Testing completed (on m-c, etc.): Landed on mc last week.
Risk to taking this patch (and alternatives if risky): This is the simplest solution I could come up with. Its very low risk. It basically does nothing, but it does throw us out of our fast click path on about:addons into a slower, but not really slow, path
String or IDL/UUID changes made by this patch: none
Attachment #8430292 -
Flags: approval-mozilla-beta?
Attachment #8430292 -
Flags: approval-mozilla-aurora?
Comment 36•11 years ago
|
||
Comment on attachment 8430292 [details] [diff] [review]
Hack fix
Since it's super confusing to try to hit a text entry field and get a hyper link reaction I think we should take this low risk patch into our final beta.
Attachment #8430292 -
Flags: approval-mozilla-beta?
Attachment #8430292 -
Flags: approval-mozilla-beta+
Attachment #8430292 -
Flags: approval-mozilla-aurora?
Attachment #8430292 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 37•11 years ago
|
||
Assignee | ||
Comment 38•11 years ago
|
||
Updated•11 years ago
|
Comment 39•11 years ago
|
||
Verified as fixed in builds:
32.0a1 (2014-06-03)
31.0a2 (2014-06-03)
30.0b10
Device: Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
Comment 40•11 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•