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)

All
Android
defect
Not set
normal

Tracking

(firefox36 affected, firefox37 affected, firefox38 affected, firefox39 verified, firefox40 verified, fennec39+)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- verified
firefox40 --- verified
fennec 39+ ---

People

(Reporter: kbrosnan, Assigned: ally)

References

Details

Attachments

(4 files)

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
Assignee: nobody → ally
tracking-fennec: ? → 39+
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.
I believe that this is fallout from the new tablet tabs tray. Bug 1124711 is fixed on 38 so it cannot be the cause.
I am unable to reproduce this. Perhaps liuche's doorhanger refactor fixed this.
Flags: needinfo?(kbrosnan)
kbrosnan has confirmed that he cannot reproduce either.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(kbrosnan)
Resolution: --- → WORKSFORME
I can still reproduce this on my devices. The doorhanger changes don't fix this bug, and both phone and tablet are affected.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
Chenxia, could you post an str & what device you used? I can't seem to get this to reproduce.
Flags: needinfo?(liuche)
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)
That is different than the STR in comment 0.
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.
Actually this looks even worse on my test phone. :/  Appears to be the same problem
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
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
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.
Summary: Door hanger on tablet shows up in the tab over view → Door hanger shows up in the tab over view
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.
Attachment #8600657 - Flags: review?(liuche)
Attachment #8600657 - Attachment is patch: true
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+
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 ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8600657 [details] [diff] [review]
doorhangsovertabviewReviewCopy

Approved for uplift to aurora.
Attachment #8600657 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
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)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.