93390 - [RFE]"Block images from this server" should identify "this server"

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement, P4)

Description

[Henrik Gemal]

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

Comment 1

[Henri Sivonen (:hsivonen)]

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

Comment 2

[Blake Ross]

--> morse

Comment 3

[Beth J.]

Attachment: Fix for bug #93390 per comment 1

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.

Comment 4

[Stephen P. Morse]

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?

Comment 5

[Beth J.]

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.

Comment 6

[Stephen P. Morse]

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

Comment 7

[Stephen P. Morse]

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

Comment 8

[Stephen P. Morse]

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.

Comment 9

[Beth J.]

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?

Comment 10

[Stephen P. Morse]

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.

Comment 11

[Beth J.]

Attachment: Revised patch

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.

Comment 12

[Stephen P. Morse]

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.

Comment 13

[marlon bishop]

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

Comment 14

[marlon bishop]

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

Comment 15

[ltskinol]

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?"

Comment 16

[Michael Lefevre]

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

Comment 17

[Boris 'pi' Piwinger]

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

pi

Comment 18

[Matthew T (active 1999-2002)]

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

Comment 19

[Michael Lefevre]

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

Comment 20

[timeless]

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

Comment 21

[Mikko Kinnunen]

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. 

Comment 22

[Travis Saling]

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.

Comment 23

[Peter]

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.

Comment 24

[:glob ✱]

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

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Reassigning to new module owner

Comment 26

[SineSwiper]

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

Comment 27

[SineSwiper]

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

Comment 28

[Mitchell Stoltz (not reading bugmail)]

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

Comment 29

[benc]

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

Comment 30

[SineSwiper]

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

Comment 31

[Mogens Isager]

Attachment: Updated patch working with 20040129

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.

Comment 32

[Mike Connor [:mconnor]]

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"

Comment 33

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 34

[Phillip Stewart :P]

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

Comment 35

[OstGote!]

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

Comment 36

[zug_treno]

Attachment: SeaMonkey 2.0a1pre version of Mogens Isager's patch

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 37

[Ian Neal]

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.

Comment 38

[zug_treno]

Attachment: SeaMonkey 2.0a1pre version of Mogens Isager's patch v.2

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

[Ian Neal]

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 40

[[e-mail]]

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.

Comment 41

[zug_treno]

Attachment: SeaMonkey 2.0a1pre version of Mogens Isager's patch v.2.5

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

Comment 42

[[e-mail]]

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

Comment 43

[zug_treno]

Attachment: SeaMonkey 2.0a1pre version of Mogens Isager's patch v.3 (ready for sr)

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

Comment 44

[[e-mail]]

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

[[e-mail]]

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?

Comment 47

[Vlado Valastiak [:wladow]]

Attachment: v4

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 48

[[e-mail]]

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

Comment 49

[Vlado Valastiak [:wladow]]

Attachment: v5

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 50

[[e-mail]]

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

Comment 51

[Vlado Valastiak [:wladow]]

Attachment: for checkin

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

Comment 52

[[e-mail]]

Fix checked in, I hope I credited everyone correctly!