1050130 - clean up popup blocking code

Status: NEW

(Firefox :: General, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Current Nightly.

1) Go to http://www.popuptest.com/popuptest2.html
2) Click icon in awesomebar, Allow pop ups from this site
3) Close pop ups.
4) Switch tabs, see pop up icon in other tabs that have no pop ups.
5) Switch to original site. Icon is still displayed, despite us having allow this site and no popups remaining blocked.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

I had some problems bisecting this, but after ending up at a range of about 20 commits, this stood out greatly:

changeset:   194557:37184d8682db
user:        Bill McCloskey <[e-mail]>
date:        Wed Jul 16 23:05:17 2014 -0700
summary:     Bug 1024391 - Fix popup blocking bug (r=felipe)

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I tried this STR and it doesn't quite work for me. When I click "Allow pop ups from this site", we don't actually show the popups, so there's nothing to close. Another weird thing is that when I choose this option, then the "notification box" goes away but the URL bar icon stays around. However, when I switch tabs, I see the behavior you're reporting: the URL bar icon comes back when I switch back to the original tab. I tried the same STR in FF31 and you're right that it no longer shows the URL bar icon when we switch back. The reason for the change appears to be bug 1047111. I could fix this, which would return us to the former behavior, but I'm pretty uncomfortable with the current state of the code.

I'm starting to wonder if our pre-existing behavior makes any sense at all. Gavin, I'm needinfoing you for some ideas on how pop-up blocking is supposed to work. Here's what seems odd to me about the original FF31 behavior:

- Why doesn't "Allow pop ups from this site" show the pop ups that have already been hidden? The only way to see them is to select "Allow pop ups" and then to ask to see each individual pop up.

- Once you've chosen "Allow pop ups", why do we remove the notification box but not the URL bar item? If you're inexperienced and you use the notification box to choose "Allow pop ups", then the box disappears and there's no obvious way to ask to see the pop up that we blocked. You just have to reload.

- If we keep the URL bar notification around even after selecting "Allow pop ups", why does it go away when you switch tabs and then switch back? I can sort of see that we want people to be able to ask for individual pop ups, and once they switch tabs they probably don't care anymore. But it's still a little odd.

- As briefly mentioned in bug 1047111, the pageshow and pagehide handlers in the pop up code are really weird. As far as I can tell, the pageshow handler is totally useless. I think it filters out windows that have navigated. However, if the window navigated, then presumably we'd get a pagehide event for it, and that throws away *everything*. Is that reasonable?

I'd like to propose that we simplify this code somewhat:

Get rid of the menu items for individual pop ups. When you select "Allow pop ups", show them all. At that time, hide the notification box and the URL bar icon. I also think we should get rid of the pageshow handler and make the pagehide handler specific to the top-level frame.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I unfortunately am not really able to dig deeply into the details of this, but the "old" code is old enough that I have little faith in it being sensible, and I definitely support simplifying the code overall. A list of the UX-affecting changes (sounds like comment 2 is a fairly complete one) should be reviewed by UX folk (which should happen quickly now via the process outlined at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Requesting_feedback_and_ui-review_for_desktop_Firefox_front-end_changes), and I can find someone to review said simplification.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, sounds good. I'd prefer if we could agree on the UX before I start implementing. It's pretty easy to get a sense of our existing behavior by visiting http://www.popuptest.com/. The last paragraph of comment 2 describes what I'd like to do. The pageshow/pagehide stuff is just a question of how we handle navigations. I'm proposing that only a top-level navigation should throw away all our pop up blocking state; currently we throw everything away for sub-frame navigations as well.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Here's corrected/tighter STR:

1) Go to http://www.popuptest.com/popuptest2.html
2) Click icon in awesomebar, Allow pop ups from this site.
3) Refresh page.
4) (optional) Close popups.
5) Switch tabs.

You now have the popup icon in the awesomebar for a tab that never had any popups. Regardless of what the expected behavior of a tab with popups is, this can't be right.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> Here's corrected/tighter STR:
> 
> 1) Go to http://www.popuptest.com/popuptest2.html
> 2) Click icon in awesomebar, Allow pop ups from this site.
> 3) Refresh page.
> 4) (optional) Close popups.
> 5) Switch tabs.
> 
> You now have the popup icon in the awesomebar for a tab that never had any
> popups. Regardless of what the expected behavior of a tab with popups is,
> this can't be right.

Oh, I see. That's fixed by the patch in bug 1045826. I guess I should dupe this against that, but I'd rather leave it open for this other stuff.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Sounds like we should resummarize this bug then.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Philipp, can you (or another UX person) sign off on the UX from comment 2?

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Redirecting needinfo to Sevaan since I'm on PTO this week.

Comment 10

[Sevaan Franks [:sevaan]]

I'm good with the proposition in Comment 2 (definitely think the popups should just be shown when allowed), but I am hesitant to remove the icon completely as I can see a use-case where a user may want to temporarily allow popups then reblock after performing whatever actions they need to do. By remove the icon completely, it makes it impossible to quickly allow/disallow and I don't think just refreshing the page to get the icon back is an obvious solution.

Instead, what if we kept the icon and just changed it to reflect the state (allowed/disallowed)? Something like this maybe?:
http://cl.ly/image/161K251L3E1b

Thoughts?
Thoughts on this?

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.