Closed Bug 456077 Opened 11 years ago Closed 10 years ago

No mechanism to edit popup preferences

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(fennec1.0-)

VERIFIED FIXED
fennec1.0b1
Tracking Status
fennec 1.0- ---

People

(Reporter: abillings, Assigned: vingtetun)

References

Details

(Keywords: uiwanted, Whiteboard: [fennec l10n])

Attachments

(4 files, 6 obsolete files)

79.79 KB, image/png
Details
38.41 KB, image/png
Details
57.35 KB, image/png
Details
24.12 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → db48x
Depends on: 436077
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.
Keywords: uiwanted
imho this should just be in prefs:

block popups:  [yes, no]

madhava?
Target Milestone: --- → Fennec A2
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.
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?
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.
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.
Target Milestone: Fennec A2 → Fennec A3
Blocks: 477628
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.
(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
tracking-fennec: --- → ?
Assignee: db48x → nobody
tracking-fennec: ? → 1.0-
ran into this again today, have to blow away my whole profile to see popups again.  Very annoying
Flags: wanted-fennec1.0?
if this is not going in 1.0, will it go in shortly thereafter?
Attached patch Patch (obsolete) — Splinter Review
(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'?
(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
Attachment #397145 - Flags: ui-review?(madhava)
Attachment #397145 - Flags: superreview?(pavlov)
Attachment #397145 - Flags: review+
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.
(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.
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?
Whiteboard: [fennec l10n]
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 on attachment 397145 [details] [diff] [review]
Patch

I think we want a simpler option, detailed above by Madhava.
Attachment #397145 - Flags: ui-review?(madhava)
Attachment #397145 - Flags: ui-review-
Attachment #397145 - Flags: superreview?(pavlov)
It should also clear out exceptions the user has made for expired/etc. certificates.
Duplicate of this bug: 494218
Should we also delete the offline data with the [Clear Site Preferences] button?
(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?
Attached patch wip-1 (obsolete) — Splinter Review
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
Attachment #397145 - Attachment is obsolete: true
Attached image screenshot landscape
Screenshot of landscape site menu, which working only for Hildon while I'm not sure that the skin is correct
Attachment #397147 - Attachment is obsolete: true
Attached image screenshot portrait
Screenshot of portrait site menu
Attached patch wip-2 (obsolete) — Splinter Review
This one also handle password through the nsILoginManager.
I guess the .properties translation I've add need to be reworked. (Madhava?)
Attachment #426366 - Attachment is obsolete: true
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.
... 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)
Flags: in-testsuite?
Flags: in-litmus?
Attached patch wip-3 (obsolete) — Splinter Review
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".
Attachment #426417 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #432554 - Attachment is obsolete: true
Attachment #434205 - Flags: review?(mark.finkle)
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
Attachment #434205 - Flags: review?(mark.finkle) → review-
Attached patch Patch v0.2Splinter Review
>+  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.
Attachment #434205 - Attachment is obsolete: true
Attachment #434247 - Flags: review?(mark.finkle)
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.
Bug 5265285 means we need to use .getAllLogins({}) on m-192
Comment on attachment 434247 [details] [diff] [review]
Patch v0.2

I added the missing images and made the getAllLogins change
Attachment #434247 - Flags: review?(mark.finkle) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/3bca60979f8e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=9692 updated for the l10n string guide
Flags: wanted-fennec1.0?
You need to log in before you can comment on or make changes to this bug.