Closed
Bug 1139232
Opened 9 years ago
Closed 9 years ago
Door hanger shows up in the tab over view
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox36 affected, firefox37 affected, firefox38 affected, firefox39 verified, firefox40 verified, fennec39+)
RESOLVED
FIXED
Firefox 40
People
(Reporter: kbrosnan, Assigned: ally)
References
Details
Attachments
(4 files)
135.50 KB,
image/png
|
Details | |
175.59 KB,
image/png
|
Details | |
183.47 KB,
image/png
|
Details | |
1.14 KB,
patch
|
liuche
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
STR: * Open several tabs that contain content that would trigger door hangers (plugins, geolocation, etc) * Open the tabs overview mode * Close the current focused tab * when the newly selected tab loads the door hanger appears Expect: no door hanger
Updated•9 years ago
|
Assignee: nobody → ally
tracking-fennec: ? → 39+
Assignee | ||
Comment 1•9 years ago
|
||
This is not believed to be related to https://bugzilla.mozilla.org/show_bug.cgi?id=1124711 ? Initial digging suggests that it is not.
If I had to guess, onLocationChange probably forces the doorhanger to appear and, since I doubt it's aware of the tabs tray state, will just make the doorhanger appear over it.
Reporter | ||
Comment 3•9 years ago
|
||
I believe that this is fallout from the new tablet tabs tray. Bug 1124711 is fixed on 38 so it cannot be the cause.
Assignee | ||
Comment 4•9 years ago
|
||
I am unable to reproduce this. Perhaps liuche's doorhanger refactor fixed this.
Flags: needinfo?(kbrosnan)
Assignee | ||
Comment 5•9 years ago
|
||
kbrosnan has confirmed that he cannot reproduce either.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(kbrosnan)
Resolution: --- → WORKSFORME
Comment 6•9 years ago
|
||
I can still reproduce this on my devices. The doorhanger changes don't fix this bug, and both phone and tablet are affected.
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment 7•9 years ago
|
||
Yeah, this should not happen. If there is a notification that comes through from the site while the user is in the Tabs tray, we should not show it until the user returns to that tab.
Assignee | ||
Comment 8•9 years ago
|
||
Chenxia, could you post an str & what device you used? I can't seem to get this to reproduce.
Flags: needinfo?(liuche)
Comment 9•9 years ago
|
||
1. Go to maps.yahoo.com 2. Before it loads, switch to tabs tray view Expected: doorhanger pops up only when you load the tab Actual: doorhanger pops up in tab view
Flags: needinfo?(liuche)
Reporter | ||
Comment 10•9 years ago
|
||
That is different than the STR in comment 0.
Comment 11•9 years ago
|
||
Hmm, okay, but I think it's exhibiting the same bug - that the doorhanger shouldn't be showing in the tabs tray view. This just eliminates the extra steps of opening additional tabs and closing them to kick off the load of a tab that shows a doorhanger. This happens on a Nexus 4, and also a Nexus 9. I expect it happens on pretty much all other devices too.
Assignee | ||
Comment 12•9 years ago
|
||
Actually this looks even worse on my test phone. :/ Appears to be the same problem
Assignee | ||
Comment 13•9 years ago
|
||
most relevant bit of logging so far 19569-19569/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup onTabChanged called 19569-19569/org.mozilla.fennec_mozilla W/anaaktge﹕ TabsPanel show() called 19569-19569/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup onTabChanged called 19569-19967/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup handle message called 19569-19569/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup addDoorhanger called 19569-19569/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup called possibly of note, when the bug does not reproduce, the logs always show tabspanel.show() following all the tabschanged() calls
Assignee | ||
Comment 14•9 years ago
|
||
Nulling out selected tab does get rid of the bug here, but is not a valid solution since the tab tray needs to know what is selected. It appears that the doorhanger.java has no knowledge of the tab tray's existence. So, I propose the following: during show() in tabspanel, message the doorhanger class and set mDisabled to true, when the tabspanel is dismissed, messge to flip mDisabled to false. full log 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup ctor called 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup onTabChanged() SELECTED 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup called 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup() bailing 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup onTabChanged() LOCATION_CHANGE 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup called 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup() bailing 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup onTabChanged() LOCATION_CHANGE 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup called 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup() bailing 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ TabsPanel show() called 21348-21461/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup handle message called 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup addDoorhanger called 21348-21348/org.mozilla.fennec_mozilla W/anaaktge﹕ DoorhangerPopup updatePopup called //does not bail out early, completes & popup appears
Comment 15•9 years ago
|
||
This sounds similar to what liuche had to deal with in bug 1151505. As much as it's kinda gross, I think that we should just control all this from BrowerApp, since that's what we're already doing. So, when BrowserApp decides to show the TabsPanel, it can also tell DoorhangerPopup to hide itself.
Updated•9 years ago
|
Summary: Door hanger on tablet shows up in the tab over view → Door hanger shows up in the tab over view
Comment 17•9 years ago
|
||
I would agree with Margaret - setting boolean flags for disabled/enabled just seems risky because then the Doorhanger is maintaining its own internal model/state of what it thinks the situation is - it's better to just react to messages or notifications, or have a centralized place to manage state.
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8600657 -
Flags: review?(liuche)
Updated•9 years ago
|
Attachment #8600657 -
Attachment is patch: true
Comment 19•9 years ago
|
||
Comment on attachment 8600657 [details] [diff] [review] doorhangsovertabviewReviewCopy Review of attachment 8600657 [details] [diff] [review]: ----------------------------------------------------------------- Tried it on a few doorhangers and it seems to work well (logins, geolocation).
Attachment #8600657 -
Flags: review?(liuche) → review+
Assignee | ||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c99917c92085
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8600657 [details] [diff] [review] doorhangsovertabviewReviewCopy Approval Request Comment [Feature/regressing bug #]: doorhangers showing in tab tray view [User impact if declined]: popups show up blocking tab thumbnails [Describe test coverage new/current, TreeHerder]: [Risks and why]: [String/UUID change made/needed]: no
Attachment #8600657 -
Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c99917c92085
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 23•9 years ago
|
||
Comment on attachment 8600657 [details] [diff] [review] doorhangsovertabviewReviewCopy Approved for uplift to aurora.
Attachment #8600657 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 25•9 years ago
|
||
Trying the steps from comment 9 and description, doorhanger does not show up in the tab tray, so: Verified as fixed using: Devices: Motox X (Android 4.4) and Asus Transformer (Android 4.0.3). Build: Firefox for Android 40.0a1 (2015-05-07)
Comment 26•9 years ago
|
||
Trying the steps from comment 9 and description, doorhanger does not show up in the tab tray, so: Verified as fixed using: Devices: Motox X (Android 4.4) and Asus Transformer (Android 4.0.3). Build: Firefox for Android 39.0a2 (2015-05-08)
Updated•3 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
•