Closed Bug 1150613 Opened 5 years ago Closed 5 years ago

Doorhanger should not reappear after being dismissed

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(1 file)

STR:
1. Attempt to log into a site with a new login, doorhanger pops up
2. Dismiss by tapping outside the doorhanger
3. Switch tabs.
4. Switch back.

Expected: Tab is switched, no doorhanger shows
Actual: Doorhanger is shown again, despite being dismissed

We don't necessarily want to remove the doorhanger entirely, because we will want to summon it again in bug 700437. Just don't re-show the doorhanger popup.
Ok, this is actually worse than I thought, I was able to repro a login save/update doorhanger following you around forever, across indefinite url changes, on Firefox 35, and 38.0.5.

1. Attempt to log into a site with a new login, doorhanger pops up
2. Tap on urlbar to go to awesome screen
3. Click on a link from top sites, or type in the url and hit enter

Expected: Doorhanger no longer shows
Actual: Login save/update doorhanger keeps showing up
Assignee: nobody → liuche
Blocks: 1173887
Bug 1150613 - Doorhanger should not reappear after being dismissed. r=margaret
Attachment #8624510 - Flags: review?(margaret.leibovic)
Comment on attachment 8624510 [details]
MozReview Request: Bug 1150613 - Doorhanger should not reappear after being dismissed. r=margaret

https://reviewboard.mozilla.org/r/11643/#review10179

r+ with comments addressed.

::: mobile/android/base/DoorHangerPopup.java:330
(Diff revision 1)
> +    public void onDismiss() {

What happens if this gets called while the selected tab is switching? I just want to make sure that we can't run into an edge case where we remove doorhangers from the wrong tab.

::: mobile/android/base/DoorHangerPopup.java:247
(Diff revision 1)
> +                && (!isTransient || (isTransient && dh.shouldRemove(isShowing())))) {

This `isTransient` parameter name is a bit confusing... what we're really doing here is maybe keeping a doorhanger if the persistence/timeout parameters say so, so maybe this should be something more like `checkShouldRemove`. Or alternately, flip the true/false values to say `forceRemove` or something like that.

Also, you should update the method documentation to explain this parameter.
Attachment #8624510 - Flags: review?(margaret.leibovic) → review+
https://reviewboard.mozilla.org/r/11643/#review10185

> What happens if this gets called while the selected tab is switching? I just want to make sure that we can't run into an edge case where we remove doorhangers from the wrong tab.

I think the case where the user dismisses a doorhanger and then is able to switch a tab fast enough to do something in between the dismiss() call and the onDismiss callback is really, really low. That being said, if onDismiss is a really delayed callback or something, then we'll have some concurrency problems. This might look like the case where for some location pages, the doorhanger displays and then gets hidden again. However, I just filed bug 1176138 for that, and include this edge case in the consideration.

This is blocking bug 1173887, and this seems like a really small edge case, so I'd rather land it, and then deal with this case if problems arise.
https://hg.mozilla.org/mozilla-central/rev/8c6f31ca0635
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.