393120 - Make use of notification bar in SeaMonkey for popup blocking

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Teune van Steeg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6

When bug #270443 has been fixed, SeaMonkey can use the notification bar to
notify when a popup has been blocked. Like the way it is handled in Firefox.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Teune van Steeg]

Attachment: fix, also fixes bug 278831 and bug 393108

Full patch, shows notification bars for:
 - blocked popups
 - blocked xpinstall (bug 393108)
 - plugins missing
it also makes seamonkey use the plugin finder service (bug 278831)
and it adds a better layout for the sidebar notification.

It needs bug 400764 fixed, though

Comment 2

[Teune van Steeg]

 (In reply to bug 270443 comment #91)
> The second shows when a notification is showing, and you press the back button.
> Then when the notification disappears, the page doesn't stretch to the
> statusbar (it only moves up to fill the space left by the notification area).
(In reply to bug 270443 comment #102)
> (From update of attachment 279368 [details] [diff] [review])
> >-                this.mBrowser.parentNode.removeAllNotifications(true);
> >+                this.mBrowser.parentNode.removeAllNotifications(false);
> I guess this is OK for now but check back later e.g. when 363707 is fixed.
> 

The fix for bug 363707 did not fix this problem. This patch reverts the above change anyhow: I'll investigate the problem further, and when I have enough information I'll file a new bug for this.

Comment 3

[Teune van Steeg]

(In reply to comment #2)
> The fix for bug 363707 did not fix this problem. This patch reverts the above
> change anyhow: I'll investigate the problem further, and when I have enough
> information I'll file a new bug for this.
> 

I tested this also on Windows and with Firefox (moved code from nsBrowserStatusHandler in browser.js to tabbrowser.xml to duplicate SeaMonkey's behavior) and found that this bug appears there too. Setting "browser.sessionhistory.max_total_viewers" to 0 (disable bfcache) fixes this issue. Looking at dependencies to the bfcache bug I found bug 303327 which describes I'm reasonably sure the same issue.

Comment 4

[[e-mail]]

Comment on attachment 287420 [details] [diff] [review]
fix, also fixes bug 278831 and bug 393108

Well, I thought this worked quite nicely for installation and plugins, but unfortunately I got no notifications for popups.

>+  var notificationbox = event.rangeParent.control;
This looks odd... what does it mean?

>-                this.mBrowser.parentNode.removeAllNotifications(false);
>+                this.mBrowser.parentNode.removeAllNotifications(true);
This still doesn't work :-(

>+                var missingPluginsArray = new Object();
Nit: I prefer {} and [] to new Object/Array.

>+                var doc = pluginElement.ownerDocument;
>+                var docShell = this._findChildShellForDocument(doc, this.activeBrowser.docShell);
>+                try {
>+                  pluginsPage = makeURI(pluginsPage, doc.characterSet, docShell.currentURI).spec;
>+                } catch (ex) {
>+                  pluginsPage = "";
>+                }
You should be able to use doc.documentURIObject to avoid the docShell.

>+                window.openDialog("chrome://mozapps/content/plugins/pluginInstallerWizard.xul",
>+                                  "PFSWindow", "modal,chrome,resizable=yes",
>+                                  {plugins: missingPluginsArray, browser: this.notificationbox.activeBrowser});
Please don't name modal windows.

Comment 5

[Teune van Steeg]

Attachment: show notification bar for blocked popups

(In reply to comment #4)
> unfortunately I got no notifications for popups.
remember that the patch for bug 400764 is still needed. 

> >+  var notificationbox = event.rangeParent.control;
> This looks odd... what does it mean

I hope I don't have to explain event.rangeParent. It happens to return the status bar for popup menus opened from a status bar panel or it returns the notification if a popup menu is opened from a button on that notification, to name a few examples.

control is just a property on a notification that references its notificationbox.

Comment 6

[[e-mail]]

(In reply to comment #5)
>(In reply to comment #4)
>>>+  var notificationbox = event.rangeParent.control;
>>This looks odd... what does it mean
>I hope I don't have to explain event.rangeParent. It happens to return the
>status bar for popup menus opened from a status bar panel or it returns the
>notification if a popup menu is opened from a button on that notification, to
>name a few examples.
Ah, then I think you want event.currentTarget

Comment 7

[Teune van Steeg]

(In reply to comment #6)
> Ah, then I think you want event.currentTarget
> 

Actually, no: This is a popupshowing event; event.currentTarget just returns the popup menu. I want the opener of that popup menu (the button), or rather the notificationbox containing that button. But the parent node (the notification) of that button conveniently happens to be referenced by event.rangeParent.

Comment 8

[[e-mail]]

Comment on attachment 290478 [details] [diff] [review]
show notification bar for blocked popups

>+    if (pm.testPermission(uri, "popup") ==
>+        Components.interfaces.nsIPermissionManager.ALLOW_ACTION) {
>+      // Offer an item to block popups for this site, if a whitelist entry exists
>+      // already for it.
How can this happen? I didn't think we supported blacklisting anyway.

>+  dontShowMessage.checked = !pref.getBoolPref("privacy.popups.showBrowserMessage");
Again, how can this be true?

>-  var browser = getBrowser();
>+  // rangeParent is currently either <notification> or null
>+  var browser = rangeParent ? rangeParent.control.activeBrowser : gBrowser.selectedBrowser;
Can you pass the browser in as a parameter instead?

>+function toggleShowPopupBlockerNotifications()
>+{
>+  var showMessage = pref.getBoolPref("privacy.popups.showBrowserMessage");
This is always true.

>+  // FF uses this function to show a dialog, explaining the icon in the status bar
>+  // when popups are blocked.
>+  // This can be done in SeaMonkey too, but the text of the dialog is currently
>+  // explaining that popups can be blocked.
File a bug?

>+            const CiPM = Components.interfaces.nsIPermissionManager;
const nsIPermissionManager

>+      <handler event="DOMUpdatePageReport" phase="capturing">
This is the wrong event; it only triggers on the current tab, and never in the sidebar. You may be better off writing your own DOMPopupBlocked handler.

>+          if (browser.pageReport.length > 100)
>+            browser.pageReport.shift();
It would be nice to have a separate variable to count all the blocked popups, not just the first 100 distinct popups.

Comment 9

[[e-mail]]

Comment on attachment 290478 [details] [diff] [review]
show notification bar for blocked popups

>     browser.popupDomain = null;
>-    browser.popups = [];
Do we still need popupDomain?

>+  var notificationbox = event.rangeParent.control;
So, it looks like event.rangeParent isn't reliable.

Comment 10

[Teune van Steeg]

(In reply to comment #8)
> [...] I didn't think we supported blacklisting anyway.
Blacklisting is automatically supported for popup blocking, but there is no UI to add items to the blacklist.

> >+      <handler event="DOMUpdatePageReport" phase="capturing">
> This is the wrong event; it only triggers on the current tab, and never in the
> sidebar. You may be better off writing your own DOMPopupBlocked handler.
Yes, currently. But that is why the patch for bug 400764 is needed; Why should we not use something that is already available in toolkit? It may not be perfect, but at least it is a base we can improve on in this part of the code (remove duplicate entries for example).

(In reply to comment #8)
> Do we still need popupDomain?
yes, for the status bar notification icon (when it should be shown).

Comment 11

[[e-mail]]

Comment on attachment 290478 [details] [diff] [review]
show notification bar for blocked popups

>   <popupset>
>     <menupopup id="popupBlockerMenu"
>                oncommand="popupBlockerMenuCommand(event.target);"
>                onpopupshowing="return popupBlockerMenuShowing(event)"/>
>-    <!-- Items are generated, see popupBlockerMenuShowing() -->
>+      <!-- Items are generated, see popupBlockerMenuShowing() -->
>+    <popup id="popupNotificationMenu"
>+           oncommand="popupBlockerMenuCommand(event.target);"
>+           onpopupshowing="return popupNotificationMenuShowing(event)">
In Windows, putting the popup in a popupset has a strange side-effect - if you click outside of the popup, the click still has its usual effect. This includes clicking on the button the opened the popup. This isn't normal behaviour for menubuttons, which this button is pretending to be.

(In reply to comment #10)
>(In reply to comment #8)
>>[...] I didn't think we supported blacklisting anyway.
>Blacklisting is automatically supported for popup blocking, but there is no UI
>to add items to the blacklist.
No, and I think the existing UI to remove items from the whitelist suffices.

>But that is why the patch for bug 400764 is needed
Sorry, I keep forgetting to look at that patch at the same time.

>>Do we still need popupDomain?
>yes, for the status bar notification icon (when it should be shown).
Ah, but what's the use when we've navigated away from the page with the popup to another page on the same site? We've lost the popup list by then.

Comment 12

[[e-mail]]

(In reply to comment #9)
>>+  var notificationbox = event.rangeParent.control;
>So, it looks like event.rangeParent isn't reliable.
I've just fixed it so that you can use document.popupNode.parentNode.parentNode

Comment 13

[Teune van Steeg]

Attachment: show notification bar for blocked popups (v2)

(In reply to comment #11)
> In Windows, putting the popup in a popupset has a strange side-effect - if you
> click outside of the popup, the click still has its usual effect. This includes
> clicking on the button the opened the popup. This isn't normal behaviour for
> menubuttons, which this button is pretending to be.
I've placed the popup out of a popupset.

> (In reply to comment #10)
> >(In reply to comment #8)
> >>[...] I didn't think we supported blacklisting anyway.
> >Blacklisting is automatically supported for popup blocking, but there is no UI
> >to add items to the blacklist.
> No, and I think the existing UI to remove items from the whitelist suffices.
I have removed that part.

> >>Do we still need popupDomain?
> >yes, for the status bar notification icon (when it should be shown).
> Ah, but what's the use when we've navigated away from the page with the popup
> to another page on the same site? We've lost the popup list by then.
OK, I have stripped out the use of popupDomain, and updated the code where it was used.

(In reply to comment #12)
> (In reply to comment #9)
> >>+  var notificationbox = event.rangeParent.control;
> >So, it looks like event.rangeParent isn't reliable.
> I've just fixed it so that you can use document.popupNode.parentNode.parentNode
Thanks! Will document.popupNode.parentNode.control do too?

> >+  // FF uses this function to show a dialog, explaining the icon in the status bar
> >+  // when popups are blocked.
> >+  // This can be done in SeaMonkey too, but the text of the dialog is currently
> >+  // explaining that popups can be blocked.
> File a bug?
onPopupWindow is actually useless now, so that can be converted to what Firefox does. I will file a follow-up bug, when this bug has been (nearly) fixed.

Comment 14

[[e-mail]]

Comment on attachment 291568 [details] [diff] [review]
show notification bar for blocked popups (v2)

This is excellent stuff! Just a few simple questions, I hope:

>-pref("privacy.popups.first_popup",                true);
>+pref("privacy.popups.first_popup",                false);
What's the deal with this? (Maybe I didn't understand your comment below.)

>+function UpdateStatusBarPopupIcon(aEvent)
>+{
>+  if (aEvent && aEvent.originalTarget != gBrowser.getNotificationBox())
>+    return;
>+
>+  var showIcon = pref.getBoolPref("privacy.popups.statusbar_icon_enabled");
>+  if (showIcon) {
>+    var popupIcon = document.getElementById("popupIcon");
>+    popupIcon.hidden = !gBrowser.getNotificationBox().popupCount;
>+  }
>+}
This means that if you clear the pref, any existing icons hang around... with the old code the icon would go away on the next domain; I don't mind if you leave it until the next page load or remove it immediately via a pref listener.

> function popupBlockerMenuShowing(event)
> {
>   var separator = document.getElementById("popupMenuSeparator");
>+  var browser = gBrowser.selectedBrowser;
> 
>   if (separator)
>-    separator.hidden = !createShowPopupsMenu(event.target);
>+    separator.hidden = !createShowPopupsMenu(event.target, browser);
>+}
Nit: might as well inline browser here, it's not worth a separate variable.

>+  try {
>+    // uri.host generates an exception on nsISimpleURIs,
>+    // but not on other URIs with no host. Then we don't
>+    // want to show this menuitem: so throw something here.
>+    if (!uri.host)
>+      throw "no host";
>+
>+    // Offer a menu item to allow popups for this site
>+    allowPopupsForSite.hidden = false;
>+    var allowString = gNavigatorBundle.getFormattedString("popupAllow", [uri.host]);
>+    allowPopupsForSite.setAttribute("label", allowString);
>+  } catch (ex) {
>+    allowPopupsForSite.hidden = true;
>+  }
>+
>+  var showPopupManager = document.getElementById("showPopupManager");
>+  try {
>+    // this generates an exception on nsISimpleURIs
>+    showPopupManager.hostport = uri.hostPort;
>+  } catch (ex) {
>+    showPopupManager.hostport = "";
>+  }
Can't you merge these try/catch blocks? Also I'm not that keen on using throw, but fortunately for you I can't think of anything better ;-)

>+function toggleShowPopupBlockerNotifications()
Nit: this only disables them (I guess at some point someone might want to add a pref pane to enable them). s/toggle/disabled/ please?

>+{
>+  // FF uses this function to show a dialog, explaining the icon in the status bar
>+  // when popups are blocked.
>+  // This can be done in SeaMonkey too, but the text of the dialog is currently
>+  // explaining that popups can be blocked.
>+  // see onPopupWindow(), which currently has no use.
>+  pref.setBoolPref("privacy.popups.showBrowserMessage", false);
I don't understand this comment. The code seems to have nothing to do with showing a dialog.

>+                if (aData == "dom.disable_open_during_load") {
>+                  // we don't support adding items to the blacklist,
>+                  // so we don't need to handle turning off popup blocking.
>+                  if (!this._prefs.getBoolPref(aData) || !this.popupCount)
>+                    return;
>+
>+                  var uri = this.activeBrowser.currentURI;
>+                  const nsIPermissionManager = Components.interfaces.nsIPermissionManager
>+                  var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>+                                              .getService(nsIPermissionManager);
>+
>+                  if (pm.testPermission(uri, "popup") == nsIPermissionManager.ALLOW_ACTION) {
>+                    var popupNotification = this.getNotificationWithValue("popup-blocked");
>+                    if (popupNotification)
>+                      this.removeNotification(popupNotification);
>+                    this.popupCount = 0;
>+                    this.notifyPopupCountChanged();
>+                  }
>+                }
I don't think most of this is necessary. The idea is to remove notifications if popup blocking is disabled, right? You also didn't check for an existing zero popup count like you did before, although I guess it's not so essential here.

>+                if (host.length > hostport.length)
>+                  return; // can't be a match
>+
>+                if (host == hostport.substr(hostport.length - host.length)) {
So what you're saying is "hostport ends with host"? You can improve it using .slice(-host.length), and you don't need the extra length check. (There's a neat optimisation for starts with, but I can't think of one for ends with.)

>+              if (soundUrlSpec.substr(0, 7) == "file://") {
>+                var soundUrl = Components.classes["@mozilla.org/network/standard-url;1"]
>+                                         .createInstance(Components.interfaces.nsIFileURL);
>+                soundUrl.spec = soundUrlSpec;
>+                var file = soundUrl.file;
>+                if (file.exists)
>+                  sound.play(soundUrl);
You copied some bad code from navigator.js ;-) Is there a chance you can fix it to use getFileFromURISpec() instead?

>+      <method name="togglePopupPermissionForSite">
I think this would me more accurately named allowPopupsForSite?

Comment 15

[Teune van Steeg]

(In reply to comment #14)
> (From update of attachment 291568 [details] [diff] [review])
> This is excellent stuff! Just a few simple questions, I hope:
> 
> >-pref("privacy.popups.first_popup",                true);
> >+pref("privacy.popups.first_popup",                false);
> What's the deal with this? (Maybe I didn't understand your comment below.)
This preference is currently used to open a dialog showing that popups can be blocked when the first popup window is opened (onPopupWindow()). Now popup blocking is on by default, users will already know that popups can be blocked when they turn off popup blocking: that dialog is not necessary anymore *here*.

> [...] 
> >+{
> >+  // FF uses this function to show a dialog, explaining the icon in the status bar
> >+  // when popups are blocked.
> >+  // This can be done in SeaMonkey too, but the text of the dialog is currently
> >+  // explaining that popups can be blocked.
> >+  // see onPopupWindow(), which currently has no use.
> >+  pref.setBoolPref("privacy.popups.showBrowserMessage", false);
> I don't understand this comment. The code seems to have nothing to do with
> showing a dialog. 
That is correct, that comment doesn't need to be there actually. It is only that Firefox at this point (now notifications for popups have been turned off) show a dialog explaining that the status bar icon will still tell when popups are blocked. They use the same dialog with the same preference as mentioned above (in reply to your first question).

So the purpose of this comment is actually to ask the questions: Do you want the same dialog as Firefox at this point? Can onPopupWindow() be removed? I don't see the use for it anymore. Or can the whole dialog with the preference and onPopupWindow() be removed? 
The first and last questions are actually answered with "file a bug?" (comment #8)

> 
> >+                if (aData == "dom.disable_open_during_load") {
> >+                  // we don't support adding items to the blacklist,
> >+                  // so we don't need to handle turning off popup blocking.
> >+                  if (!this._prefs.getBoolPref(aData) || !this.popupCount)
> >+                    return;
> >+
> >+                  var uri = this.activeBrowser.currentURI;
> >+                  const nsIPermissionManager = Components.interfaces.nsIPermissionManager
> >+                  var pm = Components.classes["@mozilla.org/permissionmanager;1"]
> >+                                              .getService(nsIPermissionManager);
> >+
> >+                  if (pm.testPermission(uri, "popup") == nsIPermissionManager.ALLOW_ACTION) {
> >+                    var popupNotification = this.getNotificationWithValue("popup-blocked");
> >+                    if (popupNotification)
> >+                      this.removeNotification(popupNotification);
> >+                    this.popupCount = 0;
> >+                    this.notifyPopupCountChanged();
> >+                  }
> >+                }
> I don't think most of this is necessary. The idea is to remove notifications if
> popup blocking is disabled, right? You also didn't check for an existing zero
> popup count like you did before, although I guess it's not so essential here.
> 
I tried to copy the code that I removed from navigator.js (gPopupPrefListener), and adjust it here. To tell you the truth I really don't understand the purpose of that code. 

As I interpreted it it will remove the status bar icon (in this case also the notification) when the host of one of the browsers is whitelisted, when popup blocking is turned on. And when popup blocking is turned off it will remove the status bar icon when at least one host of a browser is not blacklisted. 

So I tried to copy the part for turning on popupblocking. But maybe it is better to do here what you think I intended to do? :)

I actually do check for a zero popup count here ;)

Comment 16

[Teune van Steeg]

Attachment: show notification bar for blocked popups (v3)

Comment 17

[[e-mail]]

Comment on attachment 291790 [details] [diff] [review]
show notification bar for blocked popups (v3)

:-)

Comment 18

[[e-mail]]

Bah, I discovered a problem - sidebars don't work in other windows. This patch makes things worse because even with the fixed CSS the popup menu only work in the browser; I guess we need to move the popup and its code to utilityOverlay. I don't mind if you want to do the work in a separate bug.

Comment 19

[Teune van Steeg]

Attachment: popup blocking moved to utilityOverlay

Let's fix this right now: 
This is the same patch as v3, with the following differences:
- the popup menu for the notifications has been moved from navigator.xul to utilityOverlay.xul

- the strings have been moved from navigator.dtd/properties to utilityOverlay.dtd/properties

- the functions that are used by the popup menu have been moved from navigator.js to utilityOverlay.js

- the binding of notificationbox has been moved to communicator.css from navigator.css

- in navigator.xul the popup element remains, so it can be overlaid by utilityOverlay.xul

- everywhere else where sidebarOverlay.xul is being used, a popup element is added which can be overlaid by utilityOverlay.xul: editor, messengerCompose and debugQATextEditorShell (although the sidebar doesn't seem to work there).

- were we first did: throw "no host", I have found another way to not show the menu item when there is no host ;).

Comment 20

[[e-mail]]

Comment on attachment 291895 [details] [diff] [review]
popup blocking moved to utilityOverlay

8-)

I think the CSS changes should go in now, the other changes obviously need to wait...

Comment 21

[Teune van Steeg]

Attachment: previous patch without the css changes

Comment 22

[Teune van Steeg]

Attachment: Patch with only the CSS changes

Comment 23

[[e-mail]]

I checked in the CSS changes.

Comment 24

[[e-mail]]

I updated the patch for minor bitrot and checked it in.

Comment 25

[Stefan [:stefanh]]

Because of bug 359597 the notification bar now updates the number of blocked popups everytime I try to show a popup fron the tool menu on mac...

Comment 26

[Stefan [:stefanh]]

>show a popup fron the tool menu on mac...

show a popup from the Tools menu on mac...