618437 - No indication of blocked popups when the notification bar is disabled

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

From bug 588317:

> users
> who have disabled the "popup blocked" notification bar (via its checkbox) no
> longer have any UI indication that a popup was blocked, or ability to open
> those popups... The icon was in the status bar, and the status bar is gone now.

Bug 588317 is one way to address this. Another one would be to show the icon in the location bar when the notification bar is disabled.

Comment 1

[:Margaret Leibovic]

Attachment: patch

This patch adds the icon that was in the status bar to the location bar.

To address bug 588317 comment 21, it doesn't look like the original icon had keyboard/screen reader access. Maybe that should be in a follow-up. It seems like that's a bigger concern if we get rid of the notification bar.

(For reference, the status bar icon was removed in this changeset: http://hg.mozilla.org/mozilla-central/rev/eec9a82ad740.)

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> To address bug 588317 comment 21, it doesn't look like the original icon had
> keyboard/screen reader access. Maybe that should be in a follow-up. It seems
> like that's a bigger concern if we get rid of the notification bar.

Right, as long as this is secondary UI, this isn't really a concern.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 496971 [details] [diff] [review]
patch

>@@ -533,17 +558,20 @@ const gPopupBlockerObserver = {
>     if (foundUsablePopupURI)
>       blockedPopupsSeparator.removeAttribute("hidden");
>     else
>       blockedPopupsSeparator.setAttribute("hidden", true);
> 
>     var blockedPopupDontShowMessage = document.getElementById("blockedPopupDontShowMessage");
>     var showMessage = gPrefService.getBoolPref("privacy.popups.showBrowserMessage");
>     blockedPopupDontShowMessage.setAttribute("checked", !showMessage);
>-    blockedPopupDontShowMessage.setAttribute("label", gNavigatorBundle.getString("popupWarningDontShowFromMessage"));
>+    if (aEvent.target.localName == "popup")

What's this supposed to test really?

As opposed to the status bar, the location bar isn't guaranteed to exist, so you need to take this into account.

Comment 4

[:Margaret Leibovic]

(In reply to comment #3)
> Comment on attachment 496971 [details] [diff] [review]
> patch
> 
> >@@ -533,17 +558,20 @@ const gPopupBlockerObserver = {
> >     if (foundUsablePopupURI)
> >       blockedPopupsSeparator.removeAttribute("hidden");
> >     else
> >       blockedPopupsSeparator.setAttribute("hidden", true);
> > 
> >     var blockedPopupDontShowMessage = document.getElementById("blockedPopupDontShowMessage");
> >     var showMessage = gPrefService.getBoolPref("privacy.popups.showBrowserMessage");
> >     blockedPopupDontShowMessage.setAttribute("checked", !showMessage);
> >-    blockedPopupDontShowMessage.setAttribute("label", gNavigatorBundle.getString("popupWarningDontShowFromMessage"));
> >+    if (aEvent.target.localName == "popup")
> 
> What's this supposed to test really?

I just copied this over from the original status bar code. It's supposed to test whether or not the popup we're showing is part of the popup that comes up in the notification bar (rather than the icon).

> As opposed to the status bar, the location bar isn't guaranteed to exist, so
> you need to take this into account.

Would it be sufficient to just add a check to onUpdatePageReport so that we don't try to show the icon when the location bar doesn't exist?

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> > >+    if (aEvent.target.localName == "popup")
> > 
> > What's this supposed to test really?
> 
> I just copied this over from the original status bar code. It's supposed to
> test whether or not the popup we're showing is part of the popup that comes up
> in the notification bar (rather than the icon).

Does it still do what it was supposed to do? <popup> shouldn't be used anymore (bug 578322).

> > As opposed to the status bar, the location bar isn't guaranteed to exist, so
> > you need to take this into account.
> 
> Would it be sufficient to just add a check to onUpdatePageReport so that we
> don't try to show the icon when the location bar doesn't exist?

Should be sufficient as far as I can see.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 496971 [details] [diff] [review]
patch

> const gPopupBlockerObserver = {
>+  _reportButton: null,
>+  
>+  onReportButtonClick: function (aEvent)
>+  {
>+    if (aEvent.button != 0)
>+      return;
>+
>+    if (!this._reportButton)
>+      this._reportButton = document.getElementById("page-report-button");

_reportButton should always be set here already.

Comment 7

[:Margaret Leibovic]

(In reply to comment #6)
> Comment on attachment 496971 [details] [diff] [review]
> patch
> 
> > const gPopupBlockerObserver = {
> >+  _reportButton: null,
> >+  
> >+  onReportButtonClick: function (aEvent)
> >+  {
> >+    if (aEvent.button != 0)
> >+      return;
> >+
> >+    if (!this._reportButton)
> >+      this._reportButton = document.getElementById("page-report-button");
> 
> _reportButton should always be set here already.

That's what I initially thought, but for some reason it isn't. I'm not sure why.

Comment 8

[:Margaret Leibovic]

Attachment: patch v2

You were right, that popup check wasn't working properly. I couldn't figure out a way to use the popupshowing event to tell whether the menupopup came from the location bar or the notification, so I decided to set a flag in onReportButtonClick.

Also, I tried getting rid of the check for this._reportButton in onReportButtonClick, but it wasn't set when the method was called, so it would fail. I'm still not sure why.

Stephen is working on new icons, and they're going to introduce hover/pressed states, so I'll upload a new patch when those are ready.

Comment 9

[:Margaret Leibovic]

Attachment: patch v3

I got rid of the flag in favor of using aEvent.target.anchorNode.id to check if the menupopup came from the location bar.

Comment 10

[Stephen Horlander [:shorlander]]

Attachment: PopUp Blocked Glyph - Mac

Comment 11

[Stephen Horlander [:shorlander]]

Attachment: PopUp Blocked Glyph - Windows

Comment 12

[:Margaret Leibovic]

Attachment: patch v4

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 497658 [details] [diff] [review]
patch v4

>+  onReportButtonClick: function (aEvent)
>+  {
>+    if (aEvent.button != 0 || aEvent.originalTarget != this._reportButton)

s/originalTarget/target/

>+      aEvent.target.anchorNode.setAttribute("pressed", "true");

Please rename this attribute to "open".

>-  gBrowser.addEventListener("DOMUpdatePageReport", gPopupBlockerObserver.onUpdatePageReport, false);
>+  gBrowser.addEventListener("DOMUpdatePageReport", gPopupBlockerObserver.onUpdatePageReport.bind(gPopupBlockerObserver), false);

Should add a line break here.

>+popupWarningDontShowFromLocationbar=Don't show info message when pop-ups are blocked

Calling the icon a message doesn't seem quite right to me...

>+#page-report-button {
>+  list-style-image: url("chrome://browser/skin/urlbar-popup-blocked.png");
>+  -moz-image-region: rect(0px, 16px, 16px, 0px);  
>+}
>+
>+#page-report-button:hover {
>+  -moz-image-region: rect(0px, 32px, 16px, 16px);  
>+}
>+
>+#page-report-button:hover:active,
>+#page-report-button[pressed="true"] {
>+  -moz-image-region: rect(0px, 48px, 16px, 32px);  
>+}

nit: s/0px/0/

There are trailing spaces too.

>--- a/browser/themes/winstripe/browser/jar.mn
>+++ b/browser/themes/winstripe/browser/jar.mn
>@@ -42,16 +42,17 @@ browser.jar:
>         skin/classic/browser/section_collapsed-rtl.png
>         skin/classic/browser/section_expanded.png
>         skin/classic/browser/setDesktopBackground.css
>         skin/classic/browser/menu-back.png                           (menu-back.png)
>         skin/classic/browser/menu-forward.png                        (menu-forward.png)
>         skin/classic/browser/monitor.png
>         skin/classic/browser/monitor_16-10.png
>         skin/classic/browser/urlbar-favicon-glow.png
>+        skin/classic/browser/urlbar-popup-blocked.png

Need to add this to the aero section too.

Comment 14

[:Margaret Leibovic]

(In reply to comment #13)
> >+popupWarningDontShowFromLocationbar=Don't show info message when pop-ups are blocked
> 
> Calling the icon a message doesn't seem quite right to me...

This string is referring to the notification bar. It's used in the menupopup instead of "Don't show this message when pop-ups are blocked" (that's the string used when the menupopup is in the notification bar).

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> (In reply to comment #13)
> > >+popupWarningDontShowFromLocationbar=Don't show info message when pop-ups are blocked
> > 
> > Calling the icon a message doesn't seem quite right to me...
> 
> This string is referring to the notification bar.

Ok, seems like the string should make this clearer.

Comment 16

[:Margaret Leibovic]

Attachment: patch v5

I removed some other trailing whitespace, since copy/pasting it is what made it get into my patch.

I left the new string as-is, since it's the string that was in the menupopup for the status bar icon.

Comment 17

[:Margaret Leibovic]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > >+popupWarningDontShowFromLocationbar=Don't show info message when pop-ups are blocked
> > > 
> > > Calling the icon a message doesn't seem quite right to me...
> > 
> > This string is referring to the notification bar.
> 
> Ok, seems like the string should make this clearer.

Alex, what do you think the string should be?

"Don't show notification bar when pop-ups are blocked"

Comment 18

[Benjamin Smedberg]

strings block beta9...

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

>Ok, seems like the string should make this clearer.

Perhaps: "Don't show info bar when pop-ups are blocked."

Comment 20

[:Margaret Leibovic]

Attachment: patch v5 (with new string)

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 497938 [details] [diff] [review]
patch v5 (with new string)

>+    if (!this._reportButton)
>+      this._reportButton = document.getElementById("page-report-button");

if (!this._reportButton && gURLBar)

>+    if (aEvent.target.anchorNode.id == "page-report-button") {
>+      aEvent.target.anchorNode.setAttribute("open", "true");
>+      blockedPopupDontShowMessage.setAttribute("label", gNavigatorBundle.getString("popupWarningDontShowFromLocationbar"));
>+    } else
>+      blockedPopupDontShowMessage.setAttribute("label", gNavigatorBundle.getString("popupWarningDontShowFromMessage"));
>+  },
>+  
>+  onPopupHiding: function (aEvent) {

Trailing space on the empty line.

> function prepareForStartup() {
>-  gBrowser.addEventListener("DOMUpdatePageReport", gPopupBlockerObserver.onUpdatePageReport, false);
>+  gBrowser.addEventListener("DOMUpdatePageReport",
>+                            gPopupBlockerObserver.onUpdatePageReport.bind(gPopupBlockerObserver), false);

You should probably make this just gBrowser.addEventListener("DOMUpdatePageReport", gPopupBlockerObserver, false); and rename the onUpdatePageReport method to handleEvent.

Comment 22

[:Margaret Leibovic]

Attachment: patch v6 (to land)

Comment 23

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/18a8c936ce33

Comment 24

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)