345349 - Add notification bar for undoing "Block images from server"

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Brendan Eich [:brendan]]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 3

[Joey Minta]

Attachment: lowbar

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.

Comment 4

[Joey Minta]

Attachment: notification bar

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 5

[Mike Beltzner [:beltzner, not reading bugmail]]

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 6

[Mike Connor [:mconnor]]

Comment on attachment 230208 [details] [diff] [review]
notification bar

Will review the final patch.

Comment 7

[Joey Minta]

Attachment: notification bar v2

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 8

[Mike Connor [:mconnor]]

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

Comment 9

[Joey Minta]

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 10

[u60234]

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

Comment 11

[u60234]

Attachment: accesskey fix

Makes it possible for localizers to choose accesskey.

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 230776 [details] [diff] [review]
accesskey fix

trivial correctness fix, we should just take this.

Comment 13

[Joey Minta]

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 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230447 [details] [diff] [review]
notification bar v2

Well tested fix, we should take this.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230776 [details] [diff] [review]
accesskey fix

and, of course, this!

Comment 16

[Joey Minta]

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

Comment 17

[Marek Stępień [:marcoos, inactive]]

+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 18

[u60234]

Comment on attachment 230776 [details] [diff] [review]
accesskey fix

This patch still needs to land on trunk. Can someone please help out with that?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(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

Comment 20

[Anbo Motohiko]

(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

Comment 21

[Joey Minta]

(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

Comment 22

[Jesse Ruderman]

> Firefox will now always block images from this site

Shouldn't there be a period at the end of that sentence

Comment 23

[Brian Polidoro]

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

Comment 24

[Lordargent]

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.

Comment 25

[Jeremy Baron]

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

Comment 26

[Jesse Ruderman]

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