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.

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.)

(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.

(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.

(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.

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.

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 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.

(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.

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.

(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...

>Ok, seems like the string should make this clearer. Perhaps: "Don't show info bar when pop-ups are blocked."

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.

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)