If we ever get some kind of tooltip for menuitems we should show the server that the user is gonna block when moving over the "block images from this server" content menuitem. Otherwise it's impossible to see what you're actually blocking without opening the Image Manager afterwards. Perhaps we could also show it in the statusbar...

I think what is being suggested is a suboptimal solution. I suggest reformulating the menu item itself to "Block Images from Server <server name>".

--> morse

Patch adds server name to "Block images..." and "Unblock images..." commands in context menu, e.g. "Block Images from mozilla.org". This is my first attempt at a patch, using patchmaker. So in case I got it wrong, this is what I tried to do: 1. Create new file locale/cookie/cookieContext.properties 2. Insert code into content/cookie/cookieContextOverlay.xul, line 110 The content is in the attachment.

I have several comments about this patch: 1. This is going to significantly increase the width of the context menu. How does UE feel about that? cc'ing marlon for that. 2. You are doing your own parsing of URL's. That could lead to problems for cases you didn't consider. For example, what about file: url's. If the URL was file://c|/a/b/c.html, the context menu would say "block image for c|". Is that what you expected. Worse, what about platforms that have no drive letters. In that case you'll get a url like file:///a/b/c.html and the context menu would be "block image for ". 3. You'll need to modify the jar.mn file, otherwise your .properties file won't get into the jar. Did you actually test out the patch?

1. I don't think this will significantly increase the width of the context menu. A long server name is probably no longer than a long image name and the static text is only a little longer than "save image ()...". But I could certainly add a cutoff point on name length if necessary, e.g. "Block Images from reallylongsevern...". 2. Is there a standard function for parsing out the server? I couldn't find one. If not, are local files the only exception? 3. I haven't tried applying the patch (I don't know how), but I have tested the code, and it works as described.

How did you test it if you didn't modify the jar.mn file?

Yes, there are standard url parsing routines. Take a look at the code in nsCookies.cpp and search for ExtractURLPart.

BTW, to apply the patch, simply put it into a file, for example foo.bar, and then from a dos prompt do: patch <foo.bar But I didn't mean to ask if you actually "applied the patch." What I was asking if you tested out the change, because without changing the jar.mn file I wouldn't have expected that it worked.

I ran Mozilla from the bin directory above the unjarred source. It worked fine, without any changes to jar.mn. I assumed you meant applying the patch because I can't imagine submitting code without at least trying it out first. I'm afraid the cpp functions aren't going to help much except as a reference for creating js code. A few more thoughts on the file:// problems: Image blocking doesn't work for local files anyway. I don't know if this is a bug or a feature. If it's a bug (and will be fixed in future releases), maybe the best thing would be to just make the message "Block Images from localhost". If it's a feature, it would be better to eliminate "Block Images..." from the context menu for local images. What do you think?

The point I was making is that your patch will also need changes to the jar.mn file. If image blocking doesn't work on local files, then that is a bug. Forget the cpp files then. Do an LXR search for ExtractURLPart and see which .js files are using it. You can make the same sort of calls. This is a much better approach than trying to do your own parsing.

Changes: 1. Uses standard parsing functions from editorUtilities.js. 2. Handles local files as special case. 3. Limits length of host name to 25 characters 4. Message text stored in contentAreaCommands.properties instead of creating new properties file. Morse: I think I've addressed all the problems. Thanks for all your help and patience with this.

You can't put cookies stuff (which is an extension) into contentAreaCommands.properties (which is a core module). Use your original idea of having a new file called cookieContext.properties and put a reference to that into the jar.mn file.

<server name> in the menu sounds like a space problem.. it also qualifies as "too much info", for our users, and generally goes against the design principles for menus. i am not yet convinced we need to make an exception for such a marginal feature. we are in the process of reorging the menus, so marking dependency. tooltips in menus is also bad practice, we've already had the debate over them before in another bug.

sorry, i was under the impession this was for menu items, not context menu items. remarking dependency. the summary should reflect that. context menus are suffering even more than menu items, and the same can pretty much be said for them. please, we must no longer assume we can squeeze in new contextual menu items in without checking with the responsible UE person, or spec/guidelines. I am currently working on redesigning contextual menus with MPT, so this addition should be delayed until we have a design finalized. thank you

I disagree that placing the server name in the menu is "too much information." This bug was filed because the current menu item has too little information. In addition, the current behavior is confusing. If I'm on cnn.com, and choose to block an ad, if the ad is from a cnn.com server, I'll end up blocking all of the images from that site. I think this is worse behaviour than giving the end user "too much information" about what they're doing. Also, I doubt the typical user will have trouble understanding what "Block images from ads.doubleclick.net" means. Thing two: a fix for bug 91482 might also solve this bug. If, when the menu item is selected, the confirmation dialog lists the site name, then the changes for this bug _might_ not be needed. "Really block images from ads.doubleclick.net?"

not sure if this is the right place to make this comment, but... i agree that something needs to be done to indicate the server being blocked. if i'm at www.coolsite.com, and i see a banner advert, i want to know if clicking "block images from this server" is going to block images from images.coolsite.com, or just images from ad.nastymarketers.com (which is what i want) my current method is to do view | page info to display the list of images and see where they are coming from, and then go back to the page to use the context menu on whichever images come from. the current context menu option isn't very useful - having the server name may be "too much info" for many users, but i'd imagine those users wouldn't figure out how to work the option as it is either. maybe the way to go is move this off the image context menu altogether and handle it (with server name info) somewhere else? hope that adds something...

Beth, is there a chance you can improve you patch to make everybody happy? pi

This bug should be invalid. Giving a context menu item a tooltip is bizarre.

mpt: the tooltip suggestion is admittedly a bit weird. however, there is a problem here which needs a solution: "block images from this server" is pretty much useless when there is no way for the user to know which server is being blocked. if indicating which server is about to be blocked is confusing, then surely it's going to be even more confusing for those users when the blocking does not work as expected. if both the context menu and the tooltip are out, then either there needs to be a dialog to give the information and explanation (yuck), or this should be somewhere other than on the image context menu. updating summary so it doesn't refer to a particular solution for this problem...

suppose for the sake of argument the text was 'block images like this...' and clicking it brought up a dialog prefilled with the server class being blocked. the user could make changes to the server address or decide that they can't afford to block images from the server (cancel). Could someone please start a thread in npm.ui explaining the problem, the proposed (and discarded) ideas and try to build a consensus around a sane proposal? -- please get a regular from irc.mozilla.org #mozillazine to agree that you have come to a consensus around a sane proposal instead of just reporting that your proposal won (because you won the flamewar by annoying the other side until they gave up in disgust).

So showing the address in the the context menu wastes space and a tooltip is ackward, but how about the status bar? When the mouse pointer is moved over the "Block images from this server"-option, show the server address in the status bar (possibly with the text "Block images from" repeated, so the full status bar text becomes "Block images from ads.imageserv.news.com". It's not perfect because the status bar can be far away from the image & menuitem, but users should be pretty used to the idea of relevant stuff showing in the status bar.

Galeon does this, and it is quite helpful (and doesn't make the menu over-wide). Currently, when I use Mozilla I first have to "view image" to see where the image is coming from, and then block it (or not) depending on the source. With Galeon the info is right there with the right-click - I can immediately tell if the image is coming from doubleclick.net, for instance, or the actual current server (in which case I definitely do not want to block it!). As a user I'd think this would be a major improvement in the functionality. The current state of affairs seems rather sub-optimum. It is a great feature to have, in any case.

See also Bugzilla Bug 174687 which is a very similar entry for Phoenix, which also has a patch. It does not handles local files as special case, or limits length of host name to 25 characters.

how about keeping the name of the menu item as is, but have a confirmation dialog .. "are you sure you want to block all images from <server name>." ?

Reassigning to new module owner

What is the status on this bug? Is Beth still working on it, or did Moore scare him off? Since this is in two other major Mozilla-based browsers already (and thus an embarassing situation to the main Mozilla branch), obviously this is a problem issue. (Popup and image blocking are one of the primary reasons why a majority of Mozilla users use Mozilla over IE.) Hell, since both of those are also OSS, can't you just rip the code for it from there and put it here? Also voting for this bug. (The speed in which very simple things like this don't get fixed never ceases to amaze me...)

> how about keeping the name of the menu item as is, but have a confirmation > dialog .. "are you sure you want to block all images from <server name>." ? I somewhat agree with the WONTFIX reason on this issue in bug 91482. Besides, it's one more thing to click on, and I already do that when I show the properties for every image, just to make sure that it's not from the actual server. A simple right-click to see which server it's on is an easier way of showing things. On the space issue, I don't think it's anything major. Holders of domains don't want 100 character domains, as a general rule, and even spam kings are realizing the value of the new (and shorter) name spaces in .cc, .info, etc., etc. Again, like Beth said, there can also be a block for long names with "[blahblahblahblah.foobar.blahbla]". (The use of ellipses "..." in menus has been always known to mean "this will bring up another dialog", so that wouldn't work.)

> (The speed in which very simple things like this > don't get fixed never ceases to amaze me...) Ask not what Mozilla can do for you... No one here has had the time or resources to work on Image Blocking lately. If this bug is something you care about, contribute a patch, or find someone who can.

There is some discussion about making "this server" work better for cookies, so you might get some logic for free.

> Ask not what Mozilla can do for you... I know, I know. Though, it's hard than it looks to muck through the Mozilla code if you don't know jack about it, and especially if C++ is not your mother's tongue. > No one here has had the time or resources to work on Image Blocking lately. If > this bug is something you care about, contribute a patch, or find someone who > can. That's a shame, considering the importance of trying to get rid from popups, ad images, and annoying advertizing once and for all. Again, what's the status of the current patch? Can anybody build off of it? Better yet, where's the source to Galeon? Maybe I could try to find the code for it in Galeon. (How close is the Galeon code compared to Mozilla?)

I am not sure how this is implemented in Galeon but the functionality for http URLs is exactly the same as with this patch applied. I have updated the patch to work with the current trunk. I removed the dependency on editorUtilities.js since that apparently were causing problems. I also limited the max host length to 20 chars which should be more than enough IMHO.

Firefox does this, its a nice usability win, in cases where the domain overflows, they just trim it and show ... that's still better than "this server"

*** Bug 313633 has been marked as a duplicate of this bug. ***

Shouldn't this be a Mozilla Application Suite bug instead of a Core bug?

*** Bug 315393 has been marked as a duplicate of this bug. ***

Pretty much identical to attachment 140241 [details] [diff] [review], changes: removed the check for the user pref, used "\u2026" instead of "..." and the page is reloaded after images have been blocked.

Comment on attachment 305244 [details] [diff] [review] SeaMonkey 2.0a1pre version of Mogens Isager's patch Code only review to start with: >Index: suite./common/permissions/imageContextOverlay.xul >=================================================================== > var ioService = Components.classes["@mozilla.org/network/io-service;1"] > .getService(Components.interfaces.nsIIOService); > uri = ioService.newURI(gContextMenu.imageURL, null, null); > permissionmanager.remove(uri.host, "image"); >+ Non-empty line. >+ // Reload the page to show the effect instantly >+ BrowserReload(); > }, > > initImageBlocking : function () { > try { > // Block image depends on whether an image was clicked on >+ // If block or unblock is needed, parse server from url >+ if (gContextMenu.onImage) { >+ var bundle = srGetStrBundle("chrome://communicator/locale/contentAreaCommands.properties"); >+ var IOService = Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService); Line up .getService with .classes >+ var scheme = ""; just var scheme; here >+ try { >+ // This fails if there's no scheme >+ scheme = IOService.extractScheme(gContextMenu.imageURL); >+ } catch (e) {} Set scheme = ""; in the catch >+ >+ // Local file is special case >+ var serverLabel = ""; just var serverLabel; here >+ if (scheme == "file") { >+ serverLabel = "localhost"; >+ } else { >+ // Get server name >+ try { >+ serverLabel = IOService.newURI(gContextMenu.imageURL, null, null).host; >+ } catch (e) {} Set serverLabel = ""; in the catch >+ Non-empty line. >+ // Limit length to max 20 characters >+ if (serverLabel.length > 20) { >+ serverLabel = serverLabel.slice(0,18); >+ serverLabel += "\u2026"; >+ } >+ } Remove tab in the 2 lines above. >+ Declare saveImageMenuItem and caption before the if statement (strict JS errors otherwise). >+ // Set label for appropiate action >+ if (cookieContextMenu.isBlockingImages()) { Remove tabs in the 2 lines above. >+ var saveImageMenuItem = document.getElementById("context-blockimage"); >+ var caption = bundle.formatStringFromName("blockImage",[serverLabel],1); See comment above. >+ } else { >+ var saveImageMenuItem = document.getElementById("context-unblockimage"); >+ var caption = bundle.formatStringFromName("unblockImage",[serverLabel],1); See comment above. >+ } >+ saveImageMenuItem.setAttribute("label", caption); >+ } On testing: With page reloading happening automatically at what stage would the user see the unblock image menuitem in the context menu? Two options: 1. Not have reloading. 2. Adopt the way Firefox does it, only show block and use the info bar to allow an undo.

(In reply to comment #37) > With page reloading happening automatically at what stage would the user see > the unblock image menuitem in the context menu? Two options: > 1. Not have reloading. OK, I removed the automatic reload after blocking.

Comment on attachment 307048 [details] [diff] [review] SeaMonkey 2.0a1pre version of Mogens Isager's patch v.2 r=me thanks all you need now is an sr=

Comment on attachment 307048 [details] [diff] [review] SeaMonkey 2.0a1pre version of Mogens Isager's patch v.2 >+ if (scheme == "file") { >+ serverLabel = "localhost"; >+ } else { >+ // Get server name >+ try { >+ serverLabel = IOService.newURI(gContextMenu.imageURL, null, null).host; >+ } catch (e) { >+ serverLabel = ""; >+ } I don't like this. I think we should hide the menuitems if we can't get a host. Or start by hiding them, then try to get a host, then unhide the appropriate item. Or possibly even borrow from the Firefox code which I believe uses a single menuitem, and works out whether it needs to block or unblock. >+ serverLabel += "\u2026"; Ideally you would use the value of the intl.ellipsis localised pref. >+ // Set label for appropiate action Typo: appropriate >+ var saveImageMenuItem; Surely this should be blockImageMenuItem? >+ if (cookieContextMenu.isBlockingImages()) { You could probably inline the code, since you already have a URI. >+blockImage=Block Images from %S >+unblockImage=Unblock Images from %S You should also remove the old strings.

(In reply to comment #40) > I don't like this. I think we should hide the menuitems if we can't get a > host. I think it would be better to show '(Un)Block Images from Unknown Server' instead of hiding the menuitem. That way users can still block the images despite the fact that SeaMonkey can't show them the hostname. > Or possibly even borrow from the Firefox code which I believe uses a > single menuitem, and works out whether it needs to block or unblock. SeaMonkey uses a single changing (un)block menuitem, Firefox uses the notification bar to undo blocking (see Firefox bug 345349). > >+ if (cookieContextMenu.isBlockingImages()) { > You could probably inline the code, since you already have a URI. I'm not sure what you mean by this, could you explain what I should do here? > >+blockImage=Block Images from %S > >+unblockImage=Unblock Images from %S > You should also remove the old strings. It seems that the old strings are still used further down.

(In reply to comment #41) >(In reply to comment #40) >>I don't like this. I think we should hide the menuitems if we can't get a host. >I think it would be better to show '(Un)Block Images from Unknown Server' >instead of hiding the menuitem. That way users can still block the images >despite the fact that SeaMonkey can't show them the hostname. Unfortunately you can't actually block images without a hostname... >>Or possibly even borrow from the Firefox code which I believe uses a >>single menuitem, and works out whether it needs to block or unblock. >SeaMonkey uses a single changing (un)block menuitem, Firefox uses the >notification bar to undo blocking (see Firefox bug 345349). That's not quite true... although it can be used to undo accidental blocking. >>>+ if (cookieContextMenu.isBlockingImages()) { >>You could probably inline the code, since you already have a URI. >I'm not sure what you mean by this, could you explain what I should do here? Just call permissionmanager.testPermission directly on your URI. >>>+blockImage=Block Images from %S >>>+unblockImage=Unblock Images from %S >>You should also remove the old strings. >It seems that the old strings are still used further down. Sorry for being unclear, but I was referring to unblockImageCmd.label which is no longer needed now you're setting the label from script.

(In reply to comment #40) > I don't like this. I think we should hide the menuitems if we can't get a > host. OK, the menuitems aren't shown if we can't get a host. > >+blockImage=Block Images from %S > >+unblockImage=Unblock Images from %S > You should also remove the old strings. The two accesskey strings in imageContextOverlay.dtd seem to be unused, should those (and maybe the then empty file) also be removed?

(In reply to comment #41) > (In reply to comment #40) > > I don't like this. I think we should hide the menuitems if we can't get a > > host. > I think it would be better to show '(Un)Block Images from Unknown Server' > instead of hiding the menuitem. That way users can still block the images > despite the fact that SeaMonkey can't show them the hostname. Sorry for taking so long to get back to you on this. I asked around and apparently bug 400097 removed the ability to block hostless schemes, so while Morgan's patch was correct at the time, that bit isn't correct any more...

Comment on attachment 312640 [details] [diff] [review] SeaMonkey 2.0a1pre version of Mogens Isager's patch v.3 (ready for sr) So, if you could fix the code to only show the menuitem for schemes with hosts, and while you're there completely remove imageContextOverlay.dtd?

This is updated v3 patch with neil's comments addressed: 1) shows menu-item only for schemes with hosts (FX approach) 2) adds accesskeys for menu-items 3) removes imageContextOverlay.dtd file

Comment on attachment 327181 [details] [diff] [review] v4 OK, so this patch is almost ready, but I checked with L10N and we do want separate accesskeys for block and unblock. While you're fixing that, I'd appreciate if you could tweak the menu showing code; I think it would be simpler if we a) hid both menuitems first b) show the menuitem that we update the label and accesskey for, if we're on an image with a host. Having done that, you can then move all the menu code inside the try/catch, beginning try { var serverLabel = uri.host; Just one more nit: we like to sprinkle spaces around in the code to space things out a bit, e.g. after commas (but not at the end of a line of course). (I notice that the Firefox code has this problem too, so I guess it got copied that way.)

This adds separate accesskeys and as requested changes the way how we hide/unhide both menu-items (hide both first, unhide appropriate). Asking for review again due to rewritten and simplified "Set label and accesskey" part.

Comment on attachment 327455 [details] [diff] [review] v5 Wow, that code looks much better now :-) >- gContextMenu.showItem >- ("context-blockimage", >- gContextMenu.onImage && cookieContextMenu.isBlockingImages()); >- >- gContextMenu.showItem >- ("context-unblockimage", >- gContextMenu.onImage && !cookieContextMenu.isBlockingImages()); I think we can probably get rid of isBlockingImages() ;-) >+ var serverLabel = uri.host; >+ if (serverLabel) { >+ >+ // Limit length to max 15 characters >+ var shortenedserverLabel = serverLabel.replace(/^www\./i, ""); >+ var serverLabelEllipsis = ""; >+ if (shortenedserverLabel.length > 15) >+ serverLabelEllipsis = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch) >+ .getComplexValue("intl.ellipsis", >+ Components.interfaces.nsIPrefLocalizedString).data; >+ serverLabel = shortenedserverLabel.substr(0,15) + serverLabelEllipsis; Having another look at this again, I think having the two variables is confusing. Would you mind a) using serverLabel throughout b) including the substr/ellipsis line in the if block? You may also want to split up the preference code into separate statements to improve readability. >+ blockImageMenuItem.setAttribute("label", bundle.formatStringFromName(action + "Image",[serverLabel],1)); It would be nice if you could add a few more spaces here and other places ;-)

This patch compared to previous: 1) removes isBlockingImages() completely 2) uses serverLabel only (gets rid of shortenedserverLabel)

Fix checked in, I hope I credited everyone correctly!