456077 - No mechanism to edit popup preferences

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Al Billings [:abillings - ex-MoCo]]

If a user goes to a site that tries to open a popup, they will get a popup blocker notification. If they choose "Always Allow" or "Never Tell Me", they can go on their merry way. The problem is that there is no way to change this choice later. If they choose "Always Allow" for a site, they will never be able to turn on popup blocking on the selected site ever again. One mistake and they are stuck with it forever...

Steps to Reproduce
1. Go to http://www.popuptest.com/popuptest1.html
2. Look at notification UI.
3. Select "Always Allow"
4. Reload site.

What do you do now if you want to block popups for the site?

This was seen in Stuart's build 20080918185520.

Comment 1

[Daniel Brooks [:db48x]]

There's always about:config, but you do have a good point. We should add another item to the side panel, as a sibling to the prefs.

Comment 2

[Stuart Parmenter]

imho this should just be in prefs:

block popups:  [yes, no]

madhava?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

actually there is no option in about:config or prefs.js related to the popup blocker.  I tried to figure this out today, even created seperate profiles and diff'd them after selecting each option.  

In addition, the popup preference is global on/off, not site specific like firefox.  This point could be by design, but I thought it useful to make note of.

Comment 4

[Madhava Enros [:madhava]]

How about this --

given that popups are generally annoying, and even harder to handle on a one-window at a time UI like Fennec, AND that management of per-site popups settings is complicated, we do the following...

1. There's a pref that says "Block Pop-ups [yes/no]" -- by default it's "yes."  When this is on, we block pop-ups silently -- no notification bar or anything.  Downside - if a site actually has a useful pop-up, you won't see it AND you won't know that the site is trying.  Upside - you never have to deal with popups.

2. When you change the pref to "no," and you encounter a site that tries to pop something up, we throw up the notification bar -- it says something like "[larry content] This site wants to pop up n windows. (Allow once)"  You can tap outside of it to dismiss and carry on.  The "Allow once" means that sites that really require the use of a pop-up can still function, though you may have to go and toggle your "Block Pop-ups" pref first.  Pop-ups emerge as new tabs, not new windows.  Focus goes to the first of the opened tabs, but the transition effect shows the tab area, so the user can see how many there are.  We can possibly do some additional decoration here to identify the new ones.


This is more aggressively anti-popup than Firefox on the desktop.  The rationale, though, is that by doing this we can (a) leave out a whole pop-up management interface and (b) avoid popups as a matter of routine on a platform where they're harder to deal with to begin with.

Thoughts?

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, I'd agree. For version 1 of Fennec, I think just saying "we block popups that aren't opened by direct user action". If this affects Top 100 sites, bugs will be opened, and likely those bugs will end up being informative to our heuristics used for determining which windows we allow vs. disallow in terms of opening popups.

For a future version, this sounds like the sort of thing where both notification and function (open blocked popups / always allow popups / never allow popups) can be safely stowed in the site-specific button. I don't think that should be in scope as a 1.0 blocker or issue, though.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

I think that if we take an approach where we open new tabs the user needs to know that.  Right now it happens and it is unclear how to go back.  One method for letting the user know is to flash the tab bar or display a warning (maybe yellow bar) that says a new tab has been created.  They can click "ok" or "never show again" to ignore the warning in the future.

I think a method to make this more friendly and similar to a user who comes from the desktop is to add an option that is more along the lines of:
 Allow Popups?
 [always ask, yes, no]


One danger in this is we could be spamming the user with popups their first time using the product (Allow Popups, New tabs notification, password manager, etc.) which could leave a bad taste of fennec.

Another thing I would like to have access to is a UI where I could change these settings once I have set them.  I am only talking about the settings that I have set via clicking a button on a display message.  If there is a way to have two "options" pages, maybe a "global" (plugins, javascript, images) and a "behavior" or "content" (popups, pwdmgr, *whatever else we add*).  One other thought I had was to somehow put this config option on or near the url bar (since we drop the warning down from the bar).  I could see a small icon that when clicked drops down the different user config options (think larry or awesomebar) and the user can adjust their browser behavior.

Comment 7

[Ben Combee (:bcombee)]

I think that tabs that have been created but not visited by the user should be highlighted in some way, similar to the way Windows highlights new Programs entries in the start menu.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> I think that tabs that have been created but not visited by the user should be
> highlighted in some way, similar to the way Windows highlights new Programs
> entries in the start menu.

Sounds like bug 465281

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

ran into this again today, have to blow away my whole profile to see popups again.  Very annoying

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

if this is not going in 1.0, will it go in shortly thereafter?

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

(for the UI of this patch see the screenshot)

This patch add two buttons to the identity-popup, both are for removing a previous choice, which can be 'Always Show' or 'Never Show'.

But,... "Never Show" _globally_ turns off notification of popups by toggling the privacy.popups.showBrowserMessage preference (the 'Always Allow' is _not_ a pref and it is per-site).

is it considered as a bug? I mean, do we want per-site 'Never Show'?

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)

> But,... "Never Show" _globally_ turns off notification of popups by toggling
> the privacy.popups.showBrowserMessage preference (the 'Always Allow' is _not_ a
> pref and it is per-site).
> 
> is it considered as a bug? I mean, do we want per-site 'Never Show'?


IMO it is a bug and we should file a new blocking bug on it and fix it for 1.0

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 397145 [details] [diff] [review]
Patch

Patch looks good.

Madhava, is the style OK with you?

Stuart, this patch would be really useful to users and is low risk. We should land this if Madhava is OK with the style.

The sooner the better as it adds two strings.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #13)
> (In reply to comment #11)
> 
> > But,... "Never Show" _globally_ turns off notification of popups by toggling
> > the privacy.popups.showBrowserMessage preference (the 'Always Allow' is _not_ a
> > pref and it is per-site).
> > 
> > is it considered as a bug? I mean, do we want per-site 'Never Show'?
> 
> 
> IMO it is a bug and we should file a new blocking bug on it and fix it for 1.0

ok,filled bug 513214 (with a patch) for that.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

there is a typo in this patch : 
+      else if(capability == this._kIPM.DENY_ACTION) {
+        allowButton.host = host;

allowButton should be denyButton.

Should i have to upload an other patch for that?

Comment 17

[Madhava Enros [:madhava]]

There are some issues around understandability of the buttons and scalability that make me uncomfortable about this particular design.  Until we can fully sort out what we want to do here, a very useful and safer first step for getting rid of site-specific prefs would be the following:

In the place of the set of buttons for each type of remembered site-specific pref, let's just have one button:

[ Clear Site Preferences ]

That will make fennec forget what the user has told it about geolocation, pop-up blocking, and saved passwords (are there others?).  This more general and simpler interaction means the user has less fine grained control, but covers the basic use case.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 397145 [details] [diff] [review]
Patch

I think we want a simpler option, detailed above by Madhava.

Comment 19

[Madhava Enros [:madhava]]

It should also clear out exceptions the user has made for expired/etc. certificates.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Should we also delete the offline data with the [Clear Site Preferences] button?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #21)
> Should we also delete the offline data with the [Clear Site Preferences]
> button?

We should remove the permission settings. I'm not sure about deleting the actual data here or in the "Clear recent browsing history" preference. Let's look at Firefox behavior:

Advanced > Network > Offline Storage

Options for space for cache, clearing the cache and each site's stored data

Madhava?

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-1

Updated patch which handle "offline-data", "popup", and "geolocation" but not the password since the passsword are not stored into the nsIPermissionManager (I think).

New screenshot is coming, but I already know that Madhava may want something else

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot landscape

Screenshot of landscape site menu, which working only for Hildon while I'm not sure that the skin is correct

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot portrait

Screenshot of portrait site menu

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-2

This one also handle password through the nsILoginManager.
I guess the .properties translation I've add need to be reworked. (Madhava?)

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

The newest designs for this feature simplify the UI:
* [Clear Site Preferences] button clears all preferences at once. We won't have individual buttons for each preference.
* [Forget Password] button will clear any remembered passwords.

Both of these buttons will only be visible if there is a preference or password to clear.

Comment 28

[Axel Hecht [:Pike]]

... from yesterday's Firefox delivery meeting, the AMO whitelist etc is stored as a site-specific pref, i.e., when you reset site prefs like fx does, you can't install addons from AMO anymore (unless you manually add back the pref for it)

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-3

This one implement the last design as described per Mark Finkle in comment 27.
It still need 2 things:
 * a final layout/UI implementation for both hildon and wince. I'm waiting for the final decision to make the wince CSS part
* Also i need some help to disambiguate the fact that in 'Clear Site Preferences' it can also appear "password" under the main title (for the 'Never Remember Password') which can seems in opposition with the "Forget Password" menu item.

See the coming screenshot for an idea where this can appear when i said "above the title".

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot wireframe

Wireframe screenshot

Comment 31

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This patch revert back the style button and also add some css for handling the winmo case.
I've create bug 554280 for styling the site menu which I guess can also be considered as styling the whole identity panel.

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 434205 [details] [diff] [review]
Patch

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+  <binding id="sitepref" display="xul:hbox">

I think we should call this "pageaction" since the items that appear on the panel will be more than preferences.

Can we move this binding to it's own file? chrome/content/bindings/pageaction.xml

>+    <content>
>+      <xul:hbox align="center">
>+        <xul:image xbl:inherits="src=image" class="prefimage"/>
>+      </xul:hbox>
>+      <xul:vbox pack="center" flex="1">
>+        <xul:label class="preftitle" xbl:inherits="value=title" crop="end"/>
>+        <xul:label class="prefdesc" value="" xbl:inherits="value=description" crop="end"/>

Let's use "pageaction-title" and "pageaction-desc" for the class names. I don't want to conflict with settings.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+    // Site menu
>+    PermsHelper.resize();
>+

Let's call PermsHelper -> PageActions

>+var PermsHelper = {

PermsHelper -> PageActions

The rest of the "permission" names in the code are fine, unless I mention a problem



>+  resize: function() {

Why do we need to manually resize? I thought we set the "window-width" class and we set the scrollbox as "inline-block".


>diff --git a/chrome/content/browser.css b/chrome/content/browser.css

>+sitepref {
>+  -moz-binding: url("chrome://browser/content/bindings.xml#sitepref");
>+}

Change this based on <pageaction> and location of the pageaction.xml file

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+    // clean all the previous result
>+    let prefs = document.getElementById("site-preferences");
>+    while(prefs.hasChildNodes())
>+      PermsHelper.removeItem(prefs.lastChild);
>+

Add a "removeAllItems()" to PageActions

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>     <!-- popup for site identity information -->
>     <vbox id="identity-container" hidden="true" class="panel-dark window-width" mode="unknownIdentity">
>-      <hbox id="identity-popup-container" flex="1" align="top">
>+      <hbox id="identity-popup-container" align="top">

Why this change?


>+      <arrowscrollbox id="site-preferences" orient="vertical" class="window-width" hidden="true"/>

Use id="pageactions-container"

>+# Site preferences
>+siteprefs.search.addNew=Add a new Search Engine

"Add a New Search Engine"

>diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css
>diff --git a/themes/wince/browser.css b/themes/wince/browser.css

Update the CSS based on <pageaction> and "pageaction-title" and "pageaction-desc"


Code looks good, this r- is mainly for name changes

Comment 33

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

>+  resize: function() {

>Why do we need to manually resize? I thought we set the "window-width" class
>and we set the scrollbox as "inline-block".

Yes that is what we do but the arrowscrollbox doesn't seem to update its height based on its content. So, I've to manually update the height to fit its children minus a margin left for being able to click the content if the pageactions-container is very long.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Trying to play this morning by adding a new pageaction item, it is still difficult because of the removeAllItems call.

I want to address that by adding a way to register/unregister to PageActions, but probably in a followup bug.

Comment 35

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 5265285 means we need to use .getAllLogins({}) on m-192

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 434247 [details] [diff] [review]
Patch v0.2

I added the missing images and made the getAllLogins change

Comment 37

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/3bca60979f8e

Comment 38

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.2pre) Gecko/20100325 Namoroka/3.6.2pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a3pre) Gecko/20100325 Namoroka/3.7a3pre Fennec/1.1a2pre

Comment 39

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=9692 updated for the l10n string guide

Comment 40

[Aakash Desai [:aakashd]]

litmus testcases:

https://litmus.mozilla.org/show_test.cgi?id=11769
https://litmus.mozilla.org/show_test.cgi?id=11770
https://litmus.mozilla.org/show_test.cgi?id=11771

have been added to regression test this bug