No indication of blocked popups when the notification bar is disabled

RESOLVED FIXED in Firefox 4.0b9

Status

()

defect
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: dao, Assigned: Margaret)

Tracking

({regression})

Trunk
Firefox 4.0b9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [strings])

Attachments

(3 attachments, 6 obsolete attachments)

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.
Keywords: regression
blocking2.0: --- → ?
Posted patch patch (obsolete) — Splinter Review
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.)
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attachment #496971 - Flags: review?(dolske)
Whiteboard: [strings]
(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 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.
Attachment #496971 - Flags: review-
blocking2.0: ? → final+
(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?
(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 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.
Attachment #496971 - Flags: review?(dolske)
(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.
Posted patch patch v2 (obsolete) — Splinter Review
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.
Attachment #496971 - Attachment is obsolete: true
Attachment #497610 - Flags: feedback?(dao)
Keywords: icon
Posted patch patch v3 (obsolete) — Splinter Review
I got rid of the flag in favor of using aEvent.target.anchorNode.id to check if the menupopup came from the location bar.
Attachment #497610 - Attachment is obsolete: true
Attachment #497614 - Flags: feedback?(dao)
Attachment #497610 - Flags: feedback?(dao)
Posted patch patch v4 (obsolete) — Splinter Review
Attachment #497614 - Attachment is obsolete: true
Attachment #497658 - Flags: review?(dao)
Attachment #497614 - Flags: feedback?(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.
Attachment #497658 - Flags: review?(dao) → review-
(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).
(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.
Posted patch patch v5 (obsolete) — Splinter Review
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.
Attachment #497658 - Attachment is obsolete: true
Attachment #497803 - Flags: review?(dao)
(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"
strings block beta9...
blocking2.0: final+ → beta9+
Keywords: uiwanted
>Ok, seems like the string should make this clearer.

Perhaps: "Don't show info bar when pop-ups are blocked."
Posted patch patch v5 (with new string) (obsolete) — Splinter Review
Attachment #497803 - Attachment is obsolete: true
Attachment #497938 - Flags: review?(dao)
Attachment #497803 - Flags: review?(dao)
Whiteboard: [strings] → [strings][needs review 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.
Attachment #497938 - Flags: review?(dao) → review+
Attachment #497938 - Attachment is obsolete: true
Whiteboard: [strings][needs review dao] → [strings][can land]
http://hg.mozilla.org/mozilla-central/rev/18a8c936ce33
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [strings][can land] → [strings]
Keywords: uiwanted
Target Milestone: --- → Firefox 4.0b9
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)
No longer blocks: 574688
blocking2.0: beta9+ → betaN+
Keywords: regression
Blocks: 574688
Keywords: regression
You need to log in before you can comment on or make changes to this bug.