436057 - Popup Blocker

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Christian Sejersen]

Comment 1

[Johnathan Nightingale [:johnath]]

Looking at this now - a straight port is the obvious approach, but the existing functionality is very dependent on a menu of options.  For popuptest.com, this looks like:

Allow popups for popuptest.com
Edit Pop-up Blocker Preferences
Don't show this message when popups are blocked
----
Show 'http://www.popuptest.com/index1.html'
Show 'http://www.popuptest.com/index2.html'
Show 'http://www.popuptest.com/index3.html'
Show 'http://www.popuptest.com/index4.html'
Show 'http://www.popuptest.com/index5.html'
Show 'http://www.popuptest.com/index6.html'

I would propose that the use case for showing the 4th of 6 popups is vanishingly small, certainly for a 1.0 mobile browser.  I'd suggest narrowing things down to the top 3 actions, and turning those into buttons on the bar instead of this "popup out of a button" approach.

Madhava - what do you think?

Comment 2

[Madhava Enros [:madhava]]

I agree about not showing a list of URLs from which people can pick.  That's more nuance (being generous about it's usefulness) than we need here, I think.

It's another question altogether, actually, what it will even mean to have a pop-up open.  Is it another fennec window?  Another tab?

If we're going with buttons, and so shortening the description of each option, it's probably worth making explicit the difference between allowing once and allowing forever (for the site).

Something along these lines?:

---------------------------------------------------------------------
Fennec prevented this site from opening n pop-ups                [x]

    [ Never tell me ]              [ Always Allow ]  [ Allow Once ]
---------------------------------------------------------------------

Comment 3

[Johnathan Nightingale [:johnath]]

Attachment: WIP Patch

This adds the (ugly) notification box when popups happen, with a functional "Don't warn" button and a non-functional "Always allow".  It also needs some code cleanup, but provides the basic framework.

Comment 4

[Johnathan Nightingale [:johnath]]

Attachment: Working patch

Reviewer's notes (since there's a mix of stolen-from-ff and novel code here):

 - mobile.js - added the prefs to allow new windows in the first place (apparently that wasn't working...), as well as the prefs to then block them by default, and inform the user by default

 - browser.js
    - onLocationChange got some more code mostly copied from firefox around dismissing transient notifications.  The delta is nearly syntactic - changing gBrowser.selectedBrowser to just gBrowser since we aren't a tabbrowser
    - gPopupBlockerObserver - copied from Firefox as a start, but then pruned of several methods that have to do with the reportButton in the status bar, and with the complicated "allow popup 4 of 6" support Firefox has.  Different buttons added to the notification bar too.

 - browser.xul  - just registering the brand bundle since we didn't have it before.  (WIP patch contained notificationbox stuff which has since been landed separately).

 - browser.properties - subset of the popup* strings in firefox, plus new button labels and access keys.

I think we should land this version for M5 (post-review, natch), but I will file follow-up bugs to:

 - create a multi-line notification binding that is closer to madhava's designs, putting buttons on a second row, and replace our notifications with those (should be code-transparent assuming the new ones just extend the old ones)

 - add support for an "allow once" button, which probably means stealing more of the Firefox code around "fillPopupList" in browser.js

Comment 5

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

Comment on attachment 328954 [details] [diff] [review]
Working patch

>diff --git a/app/mobile.js b/app/mobile.js
>--- a/app/mobile.js
>+++ b/app/mobile.js
>@@ -38,6 +38,7 @@
> 
> pref("toolkit.defaultChromeURI", "chrome://browser/content/browser.xul");
> pref("general.useragent.extra.mobile", "@APP_UA_NAME@/@APP_VERSION@");
>+pref("browser.chromeURL", "chrome://browser/content/");
> 
> pref("browser.startup.homepage", "http://www.mozilla.org/");
> pref("browser.ui.cursor", false);
>@@ -124,4 +125,8 @@
> pref("browser.display.focus_background_color", "#ffffa0");
> pref("browser.display.focus_text_color", "#00000");
> 
>+/* block popups by default, and notify the user about blocked popups */
>+pref("dom.disable_open_during_load", true);
>+pref("privacy.popups.showBrowserMessage", true);
>+
> pref("snav.enabled", true);
>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -45,6 +45,8 @@
> 
> Cu.import("resource://gre/modules/SpatialNavigation.js");
> 
>+var gPrefService = null;
>+
> function getBrowser() {
>   return Browser.content.browser;
> }
>@@ -83,7 +85,7 @@
> 
>     this._content = document.getElementById("content");
>     this._content.addEventListener("DOMTitleChanged", this, true);
>-
>+    this._content.addEventListener("DOMUpdatePageReport", gPopupBlockerObserver.onUpdatePageReport, false);
>     BrowserUI.init();
> 
>     this._progressController = new ProgressController(this.content);
>@@ -251,9 +253,38 @@
>   },
> 
>   // This method is called to indicate a change to the current location.
>-  onLocationChange : function(aWebProgress, aRequest, aLocation) {
>+  onLocationChange : function(aWebProgress, aRequest, aLocationURI) {
> 
>+    var location = aLocationURI ? aLocationURI.spec : "";
>     this._hostChanged = true;
>+
>+    // This code here does not compare uris exactly when determining
>+    // whether or not the message(s) should be hidden since the message
>+    // may be prematurely hidden when an install is invoked by a click
>+    // on a link that looks like this:
>+    //
>+    // <a href="#" onclick="return install();">Install Foo</a>
>+    //
>+    // - which fires a onLocationChange message to uri + '#'...
>+    gBrowser = getBrowser();
>+    if (gBrowser.lastURI) {
>+      var oldSpec = gBrowser.lastURI.spec;
>+      var oldIndexOfHash = oldSpec.indexOf("#");
>+      if (oldIndexOfHash != -1)
>+        oldSpec = oldSpec.substr(0, oldIndexOfHash);
>+      var newSpec = location;
>+      var newIndexOfHash = newSpec.indexOf("#");
>+      if (newIndexOfHash != -1)
>+        newSpec = newSpec.substr(0, newSpec.indexOf("#"));
>+      if (newSpec != oldSpec) {
>+        // Remove all the notifications, except for those which want to
>+        // persist across the first location change.
>+        var nBox = Browser.getNotificationBox();
>+        nBox.removeTransientNotifications();
>+      }
>+    }
>+    gBrowser.lastURI = aLocationURI;
>+
> 
>     if (aWebProgress.DOMWindow == this._browser.contentWindow) {
>       BrowserUI.setURI();
>@@ -614,3 +645,88 @@
>     gIdentityHandler = new IdentityHandler();
>   return gIdentityHandler;
> }
>+
>+
>+/**
>+ * Handler for blocked popups, triggered by DOMUpdatePageReport events in browser.xml
>+ */
>+const gPopupBlockerObserver = {
>+  _kIPM: Components.interfaces.nsIPermissionManager,
>+
>+  onUpdatePageReport: function (aEvent)
>+  {
>+    gBrowser = getBrowser();
>+    //if (aEvent.originalTarget != gBrowser.selectedBrowser)
>+    //  return;

Keep the aEvent.originalTarget, remove the gBrowser.selectedBrowser ?

>+
>+  toggleAllowPopupsForSite: function (aEvent)
>+  {
>+    var currentURI = gBrowser.webNavigation.currentURI;
      var currentURI = getBrowser().webNavigation.currentURI;

or needs a var gBrowser = ...

>+    var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>+                       .getService(this._kIPM);
>+    pm.add(currentURI, "popup", this._kIPM.ALLOW_ACTION);
>+
>+    Browser.getNotificationBox().removeCurrentNotification();
>+  },
>+
>+  dontShowMessage: function ()
>+  {
>+    var showMessage = gPrefService.getBoolPref("privacy.popups.showBrowserMessage");

You're assuming gPrefService has been inited


Tweaks aside, this looks ok to me

Comment 6

[Johnathan Nightingale [:johnath]]

Attachment: Updated based on mfinkle's nits, irc discussion

- gPrefService is now a memoized getter on the global object
 - Browser now has a .currentBrowser getter
 - Code is rewritten to use it
 - commented-out code about event.originalTarget is now alive again (whoops)

Comment 7

[Johnathan Nightingale [:johnath]]

changeset:   51:ebd0f56b637d
tag:         tip
user:        Johnathan Nightingale <[e-mail]>
date:        Fri Jul 11 14:51:28 2008 -0400
summary:     Add popup blocker UI.  b=436057 r=mfinkle

Comment 8

[Aakash Desai [:aakashd]]

Litmus has a BFT for this feature with the following testcases:

https://litmus.mozilla.org/show_test.cgi?id=7152
https://litmus.mozilla.org/show_test.cgi?id=7222
https://litmus.mozilla.org/show_test.cgi?id=7159
https://litmus.mozilla.org/show_test.cgi?id=7131
https://litmus.mozilla.org/show_test.cgi?id=7360

Also, verified FIXED using popuptest.com with build:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090817
Fennec/1.0b3pre

I've found a blocker bug with the pop-up blocker found with this ID:

https://bugzilla.mozilla.org/show_bug.cgi?id=510977