Set As Desktop Background... Block Images from %s... I hear from flickr folks that they are getting lots of customer support calls from Firefox users who tried (reasonably enough) to set a pretty picture as wallpaper but instead bonked the block images from item. With no confirmation dialog, and no clue except looking at the change to the second item once you *have* blocked images from the site, the poor user has no clue why "flickr broke". This could be fixed by separating the items. A confirming dialog for blocking is just my babbling, don't take it as a UE suggestion ;-). /be

Seems like what we might really want is a confirmation dialog for image blocking, since probably most users don't understand what image blocking is and wouldn't have any idea how to undo it.

Low bar: add a separator Higher bar: add a confirmation dialog Highest bar: add a notification message with an "undo" button

This is the lowbar patch, which I'm putting here so I can clear my tree and actually try for one of the better fixes. If we get to a critical timeframe, let me know and I'll ask for review on this.

Patch adds a notification bar for undoing blocking images from a particular site when the user chooses to block images. My concern here is that blocking images doesn't take effect until the site is refreshed or reloaded at another time, so there's a bit of discontinuity here. A couple new strings here, so I'm asking for l10n approval. Is that the right procedure for that flag?

Comment on attachment 230208 [details] [diff] [review] notification bar Great start. Couple of things wrong: - the "Block images from %S" context menu seems to have moved a few pixels to the right (at least on Mac). - the text should be "Firefox will now always block images from this site" - should we reload the page to block the images immediately? I'd almost think that would be the desired effect ...

Comment on attachment 230208 [details] [diff] [review] notification bar Will review the final patch.

Patch updated to reflect the previous UI review comments. The strings have been tweaked and we now reload the page after images have been blocked. Note that the misaligned menuitem is not caused by this patch, but is an earlier regression.

Comment on attachment 230447 [details] [diff] [review] notification bar v2 > permissionmanager.add(uri, "image", > aBlock ? nsIPermissionManager.DENY_ACTION : nsIPermissionManager.ALLOW_ACTION); >+ var notificationBox = gBrowser.getNotificationBox(); >+ var notification = notificationBox.getNotificationWithValue("images-blocked"); move this below the function declaration just to keep this logically grouped (defining it here, then not using it for another ten lines is kinda odd. r=me

Landed on trunk. Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.667; previous revision: 1.666 done Checking in browser/locales/en-US/chrome/browser/browser.properties; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v <-- browser.properties new revision: 1.28; previous revision: 1.27 done

Comment on attachment 230447 [details] [diff] [review] notification bar v2 >Index: browser/base/content/browser.js >=================================================================== >+ if (notification) >+ notification.label = message; >+ else { >+ var buttons = [{ >+ label: bundle_browser.getString("undo"), >+ accesskey: bundle_browser.getString("undo.accessKey"), The accesskey for this button is currently stuck on "U" no matter what you have defined in browser.properties. It's fixed by using: >+ accessKey: ...

Makes it possible for localizers to choose accesskey.

Comment on attachment 230776 [details] [diff] [review] accesskey fix trivial correctness fix, we should just take this.

Comment on attachment 230447 [details] [diff] [review] notification bar v2 We need to take this, before we can take the correctness fix. has been in on trunk since wednesday. very low risk as all the new code follows after the actual functionality is done.

Comment on attachment 230447 [details] [diff] [review] notification bar v2 Well tested fix, we should take this.

Comment on attachment 230776 [details] [diff] [review] accesskey fix and, of course, this!

patches checked in to brach Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.479.2.174; previous revision: 1.479.2.173 done Checking in browser/locales/en-US/chrome/browser/browser.properties; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v <-- browser.properties new revision: 1.20.2.8; previous revision: 1.20.2.7 done

+imageWarningBlocked=Firefox will now always block images from this site +imageWarningAllowed=Firefox will now allow images from this site Is there are reason why you're explicitly using "Firefox" here instead of "%S" which should be replaced by brandShortName?

Comment on attachment 230776 [details] [diff] [review] accesskey fix This patch still needs to land on trunk. Can someone please help out with that?

(In reply to comment #18) > (From update of attachment 230776 [details] [diff] [review] [edit]) > This patch still needs to land on trunk. Can someone please help out with that? mozilla/browser/base/content/browser.js 1.670

(In reply to comment #16) > patches checked in to brach > Checking in browser/base/content/browser.js; > /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js > new revision: 1.479.2.174; previous revision: 1.479.2.173 > done Incorrect checkin to MOZILLA_1_8_BRANCH. Should be patched to toggleImageBlocking() function, not to copyEmail(). http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&file=browser.js&rev1=1.479.2.173&rev2=1.479.2.174&root=/cvsroot

(In reply to comment #20) > Incorrect checkin to MOZILLA_1_8_BRANCH. Should be patched to > toggleImageBlocking() function, not to copyEmail(). > http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&file=browser.js&rev1=1.479.2.173&rev2=1.479.2.174&root=/cvsroot > Fixed bad merge Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.479.2.175; previous revision: 1.479.2.174 done

> Firefox will now always block images from this site Shouldn't there be a period at the end of that sentence

And shouldn't the site being blocked be indicated. bug 346728

Hello All. I ended up here after doing a google search to find some way to disable the notification bar and automatic page refresh. The automatic refresh is particularly annoying. Sometimes I want to block images from several different servers that are called from the same page. This causes multiple refreshes of the page, whereas before, I would block all of the image servers I wanted and then refresh once.

(In reply to comment #24) > The automatic refresh is particularly annoying. Sometimes I want to block > images from several different servers that are called from the same page. This > causes multiple refreshes of the page, whereas before, I would block all of the > image servers I wanted and then refresh once. First of all, see the rationale for the reload in Comment #4. Also, in my experience people don't pay much attention to seperate issues raised in bugs where fixes are already checked in and bug is resolved. If you want to see this changed, you're probably best off searching for an existing bug that reverts the refresh behavior or if you can't find one, filing a new one. fyi: bugs on bugzilla.mozilla.org are not indexed by google. (see robots.txt) The best way to search is using the built in search features.

See bug 187636, "blocking/unblocking images should take effect immediately (without reload)".