Note: There are a few cases of duplicates in user autocompletion which are being worked on.

No indication of blocked popups when the notification bar is disabled

RESOLVED FIXED in Firefox 4.0b9

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: dao, Assigned: Margaret)

Tracking

({regression})

Trunk
Firefox 4.0b9
regression
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [strings])

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Keywords: regression
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
(Assignee)

Comment 1

7 years ago
Created attachment 496971 [details] [diff] [review]
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.)
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attachment #496971 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Whiteboard: [strings]
(Reporter)

Comment 2

7 years ago
(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.
(Reporter)

Comment 3

7 years ago
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+
(Assignee)

Comment 4

7 years ago
(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?
(Reporter)

Comment 5

7 years ago
(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.
(Reporter)

Comment 6

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #496971 - Flags: review?(dolske)
(Assignee)

Comment 7

7 years ago
(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.
(Assignee)

Comment 8

7 years ago
Created attachment 497610 [details] [diff] [review]
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.
Attachment #496971 - Attachment is obsolete: true
Attachment #497610 - Flags: feedback?(dao)
(Assignee)

Updated

7 years ago
Keywords: icon
(Assignee)

Comment 9

7 years ago
Created attachment 497614 [details] [diff] [review]
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.
Attachment #497610 - Attachment is obsolete: true
Attachment #497614 - Flags: feedback?(dao)
Attachment #497610 - Flags: feedback?(dao)
Created attachment 497625 [details]
PopUp Blocked Glyph - Mac
Created attachment 497627 [details]
PopUp Blocked Glyph - Windows
Keywords: icon
(Assignee)

Comment 12

7 years ago
Created attachment 497658 [details] [diff] [review]
patch v4
Attachment #497614 - Attachment is obsolete: true
Attachment #497658 - Flags: review?(dao)
Attachment #497614 - Flags: feedback?(dao)
(Reporter)

Comment 13

7 years ago
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-
(Assignee)

Comment 14

7 years ago
(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).
(Reporter)

Comment 15

7 years ago
(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.
(Assignee)

Comment 16

7 years ago
Created attachment 497803 [details] [diff] [review]
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.
Attachment #497658 - Attachment is obsolete: true
Attachment #497803 - Flags: review?(dao)
(Assignee)

Comment 17

7 years ago
(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."
(Assignee)

Comment 20

7 years ago
Created attachment 497938 [details] [diff] [review]
patch v5 (with new string)
Attachment #497803 - Attachment is obsolete: true
Attachment #497938 - Flags: review?(dao)
Attachment #497803 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Whiteboard: [strings] → [strings][needs review dao]
(Reporter)

Comment 21

7 years ago
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+
(Assignee)

Comment 22

7 years ago
Created attachment 498101 [details] [diff] [review]
patch v6 (to land)
Attachment #497938 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [strings][needs review dao] → [strings][can land]
(Assignee)

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/rev/18a8c936ce33
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [strings][can land] → [strings]
(Reporter)

Updated

7 years ago
Keywords: uiwanted
Target Milestone: --- → Firefox 4.0b9

Comment 24

7 years ago
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

Updated

7 years ago
Blocks: 574688
Keywords: regression
You need to log in before you can comment on or make changes to this bug.