47475 - Need "Show(View) Image" / "Reload Image" on context menu

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement, P3)

Description

[INOUE Seiichiro]

From Bugzilla Helper:
User-Agent: Mozilla/4.72 [en] (X11; I; Linux 2.2.17 i686; Nav)
BuildID:    2000073008

Netscape4x has a preference setting as follows,
"Automatically load images and other data types
(Otherwise, click the images button to load when needed)".
I like this feature, i.e. I can load images by clicking button on the images,
only when I want to see them.

Mozilla has a similar setting, "Do not load any images".
But it lacks the above feature.
When I choose this setting, I have no way to load images.


Reproducible: Always
Steps to Reproduce:
1.Choose "Do not load any images" setting.
2.Open an HTML file including some images.
3.Click button on an image.

Actual Results:  Nothing happens.
			

Expected Results:  I expect it to load the image.(only that image).

I think "Do not load any images" should be "Do not load any images
automatically".

Comment 1

[sairuh (rarely reading bugmail)]

morse, this sounds like something in your area --is it?

Comment 2

[Stephen P. Morse]

Certainly a nice feature and something I wasn't even aware of.  I now see that 
if you have image-loading disabled, then the right-click context menu has a 
show-image choice which will cause that individual image to get loaded.

Note that this is similar to (but not the same as) the view-image choice in the 
right-context menu.  However, in that case the image is brought up in a separate 
window.  Furthermore, it has a slight problem (see bug 34165).

Unfortunately this is a request for a new feature.  All new-feature work has 
been frozen until after the product finally ships.  So I have no choice but to 
"future" this bug, same as was done for 34165.  Now if someone out on the web 
wanted to implemented this, then we could get this incorporated prior to product 
ship.

Comment 3

[Stephen P. Morse]

Corrected the wording on the summary line.  Previous wording was confusing. 

Comment 4

[sairuh (rarely reading bugmail)]

this sounds almost identical to bug 34165. any reason why it isn't a dup? other
than the fact that in here all images are turned off...

Comment 5

[Stephen P. Morse]

No, it's not a dup.  As I described above, show-image and view-image are two 
different features.  View image causes the image being pointed at to be 
displayed in a separate window.  Show image causes the image to appear in its 
designated place on the page.  This bug is about show image and bug 34165 is 
about view image.

Comment 6

[sairuh (rarely reading bugmail)]

okay.

Comment 7

[Rob Hodges]

"Now if someone out on the web wanted to implemented this, then we could get
this incorporated prior to product ship."

Alright, I am someone out on the web!  And this one is a showstopper for me.  
I've been looking at it lately but I'm not quite sure where this would fit into
the code.  Most of the other UI stuff seems to utilise the DOM but I'm not sure
that's enough here, and I'm not sure how deep I need to go to get this in.

The questions that come to mind are:
1a) Is it currently possible to load an image into its placeholder from
Javascript?
1b) If not, what is the appropriate code to look at (where do the js bindings
come from?)
2) Should I be looking at nsIPresContext::StartLoadImage() and if so, where do I
get the nsIPresContext object to invoke it?  Or am I totally off track?
3) Am I going to need to temporarily override the "don't load images" flag to
force that load?  (if so, how?)

I'd really appreciate some guidance as to the Right Way to go about this; any
documents/source you think I should read etc.  I'm having a hard time getting a
high-level view of the code, but I *really* want this feature.  (And a lot of
people are going to be spitting chips if it isn't in the first release.)  

(We also need a "Load all images" button on the toolbar when the "don't load
images" pref is enabled, although I guess that's up to the individual skin
authors.  Are the prefs exposed to xul?)

Comment 8

[pnunn]

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

Comment 9

[Paul Chen]

nav triage team:

Nice to have, but not a beta1 stopper, marking nsbeta1-

Comment 10

[Stephen P. Morse]

Selective image blocking is done in the nsCookie.cpp file and so would get 
classified as a cookie bug.  Total image blocking is done in the image library.

This bug involves total image blocking.  So reassigning.

Comment 11

[Igor Nekrestyanov]

Attachment: Proposed solution

Comment 12

[Igor Nekrestyanov]

I just attached archive with proposed solution.

Overall idea:
  - Have separate service for ImagePermissions
     (unlike cookies they are volatile, not persistent)
  - When user explicitly request image displaying then
     add permissions for this image (or whole page) and
     trigger image(s) update
  - Use trick to trigger image update from javascript:
     clear and then restore location.href of corresponding image object
  - Move permission checks from image library to nsFrameImageLoader
     because we need to know to which document this image belongs
  - If image is prohibitted to be loaded for some reason
     (blocked of image disabled) then show it if it is already in cache
    (but do fetch it from the net)

See some additional comments in README.

Comment 13

[Rich Burridge]

tpreston, our engineers in Russia have a potential fix for this "bug".
Can you review it and give it r= for checking into the trunk please?
Thanks.

Comment 14

[sairuh (rarely reading bugmail)]

->pavlov, while pnunn's on sabbatical. rich, i cannot seem to view the
attachment --could it pls be reattached as a patch so that it can be reviewed?
thx!

Comment 15

[Rich Burridge]

Igor, could you please do as sairuh requests. Thanks.

Comment 16

[Igor Nekrestyanov]

Attachment: Proposed solution (as ZIP file)

Comment 17

[Igor Nekrestyanov]

I attached same set of patches again as zip archive
(first time it was .tar.gz) 
It is quite big - affects 14 files (3 are new) so i do not think that 
it is worth attaching each file separately.

To view it first save patch as zip file (say 47475.zip) and then unzip it.

Comment 18

[sairuh (rarely reading bugmail)]

pavlov [or someone else], could you pls review the attached patch? need to save
as zip, then unzip 'em...

Comment 19

[timeless]

cvs server: warning: 
mozilla/xpfe/browser/resources/content/Attic/nsContextMenu.js is not (any longe
r) pertinent

I have a long list of comments, i'll attach a cvs diff -N -u -w

Comment 20

[timeless]

Attachment: [e-mail]'s patch

Comment 21

[timeless]

line 33/34 of nsFrameImageLoader.cpp.patch
>   nsCOMPtr<nsIURI> url;
>   url = doc->GetDocumentURL();
instead
>   nsCOMPtr<nsIURI> url(doc->GetDocumentURL());
[recently scc seems to have changed his mind about this one, but we've been 
encouraging this style for a while].

>   (void)url->GetSpec(&spec);
why (void)?

>     NS_PRECONDITION(nsnull != mGroup, "null ImageGroup ptr");
>     if (nsnull == mGroup) {
>       return NS_ERROR_NULL_POINTER;
>     }
>     NS_PRECONDITION(nsnull == mImageRequest, "double init of ImageRequest");
>     if (nsnull != mImageRequest) {
>       return NS_ERROR_ALREADY_INITIALIZED;
>     }

instead you could write:
>     NS_PRECONDITION(mGroup, "null ImageGroup ptr");
>     if (!mGroup) {
>       return NS_ERROR_NULL_POINTER;
>     }
>     NS_PRECONDITION(!mImageRequest, "double init of ImageRequest");
>     if (mImageRequest) {
>       return NS_ERROR_ALREADY_INITIALIZED;
>     }
-

>           switch(permission) {
>             case nsIImagePermission::LOAD_WHOLE_GROUP :
>               mGroup->SetImgLoadAttributes(grouploading_attribs |
nsImageLoadFlags_kExplicitlyRequested);
>             case nsIImagePermission::LOAD_ALLOWED :
>               break;
>             case nsIImagePermission::LOAD_FORBIDDEN :
>               attribs |= nsImageLoadFlags_kOnlyFromCache;
>               mImageLoadStatus = NS_IMAGE_LOAD_STATUS_NOT_PERMITTED;
>               break;
>             default:
>               //by default just load image
>               break;
>           }
>         } else {
>         return rv;
>         }

is ::LOAD_ALLOWED similar to default?
>           switch(permission) {
>             case nsIImagePermission::LOAD_FORBIDDEN :
>               attribs |= nsImageLoadFlags_kOnlyFromCache;
>               mImageLoadStatus = NS_IMAGE_LOAD_STATUS_NOT_PERMITTED;
>               break;
>             case nsIImagePermission::LOAD_WHOLE_GROUP :
>               mGroup->SetImgLoadAttributes(grouploading_attribs |
nsImageLoadFlags_kExplicitlyRequested);
>             case nsIImagePermission::LOAD_ALLOWED :
>             default:
>               //by default just load image
>               break;
>           }
>         } else {
>           return rv;
>         }

--
>     char* cp = mURL.ToNewCString();
i think we're moving to a different syntax...
[mURL.get() which is a pointer you don't need to delete]

...navigator.xul.patch
>                   accesskey=""
since you're defining accesskeys as "" in the dtd, you should reference them.
accesskey="&showImageCmd.accesskey;" and 
accesskey="&showAllImagesCmd.accesskey;" respectively.  So if
someone later wants to set an accesskey they don't need to change the xul.

nsContextMenu.js.patch
>       //cludge to force image to be redisplayed
i think mozilla.org is using XXX to represent things that are bad/broken
so:
>       //XXX cludge to force image to be redisplayed

*** Patching a file in /Attic/ is bad. Attic means it was deleted. ***

nsImagePermission.cpp
please include a modeline and a license (mpl1.1 is suggeested)

function
PRBool nsImagePermission::permitLoad
has 4spaces for indentation, the rest of the file seems to use 2spaces.  It's 
your call [i like 2 spaces,
but consistency is more important]

// static data initialization
COOKIE_BehaviorEnum nsImagePermission::mImageBehavior = COOKIE_Accept;
nsVoidArray* mDocPermission = nsnull;
nsVoidArray* mImgPermission = nsnull;

Statics are sometimes risky, i'll defer to someone else who will say they're 
ok.

if.cpp.patch
while rewriting please switch from NULL to nsnull

Please don't license files under NPL unless you are a netscape employee.

Comment 22

[timeless]

Attachment: patch with style changes

Comment 23

[timeless]

if people find my changes to Igor's patch acceptable, we can include them. 

Igor: if you don't want to do the extra cleanup, I can file another bug for it.

Comment 24

[Igor Nekrestyanov]

timeless: Thank you for usefull comments. I have no idea on 
recent style/syntax changes and tried to follow the way things were 
done in mozilla sources.
I am working on updated version of patch. 
Anyway i want to remove some dead stuff from nsCookie.cpp 
(that is now in nsImagePermission) (so i will make some cleanup)

Soory about Attic - this is because i am mostly working with not with 
main branch and i did not realise that nsContextMenu.js was moved.

Comment 25

[Stuart Parmenter]

thanks for the patch, however most of this code is being rewritten, so checking
this in now will only cause some new problems.  The javascript/xul stuff will be
useful, but I want to hold off checking this in until later.

Comment 26

[Igor Nekrestyanov]

Proposed solution was based not on the main branch
(and this was my mistake, sorry). I will try to update 
it to new changes and resubmit.

BTW, both timeless's patches will not work. 
they both do not include changes to nsContextMenu.js (which was moved 
and therefore my patch went to /Attic/ :( ). 
Also patch with style changes does not contain any newly added files
as well as change to nsIFrameImageLoader.h
 

Comment 27

[Stuart Parmenter]

the new code isn't on tip yet.  in about 2 weeks it should all be there and at
that point we should get this working.

Comment 28

[timeless]

arg. i thought i did cvs diff -N -u, sorry. i can rediff, but at this point it 
doesn't matter.  i'll send igor the diff -N -u 's because i did make changes to 
his files.

Comment 29

[Andrew Brygin]

Attachment: Patch updated to work with main mozilla trunk

Comment 30

[Andrew Brygin]

I just attached newer version of proposed solution                              
that was updated to work with current state of main mozilla trunk.              
We tried to follow style changes proposed by timeless.

Comment 31

[Gregory Labzovsky]

pavlov:  Is anybody going to review and check in the patch atteched by Andrew
Brygin?

Comment 32

[Stuart Parmenter]

unfortunatly, a large part of the patch is no longer valid since the new 
imagelib landed.  some of it may still be however I think there are some better 
ways to do it, such as using nsIContentPolicy, and just resetting the src to 
the same thing.

however, I havn't thought about it too much, so don't hold me to what I just 
said :-)

Comment 33

[stefan livens]

I almost filed a new bug for this one, but I stumbled on this one. 
How is the situation on this one ?
I definitely want this one in the browser asap, because it's much needed. 
why ? browse the apple site, and others, they use the akamai server to deliver
content. I have 'only load images from originating server' to block ads, but I
can't load the images on the apple site, so the site is basically useless. no
'unblocking images from this site' doesn't cut it.
So back to main question : when ?

Comment 34

[Dimitrios]

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

Comment 35

[Manko]

It's important that Gecko unlike NN 4.x allows show image without reloading the
whole document.

More, when image is loaded, context menu item "Show image" could be changed to
"Reload image" - this is very useful for dynamic CGI-scripted images such as
diagrams, counters or informers.

Comment 36

[Matthias Versen [:Matti]]

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

Comment 37

[Jesse Ruderman]

We might want to rename "show image" to "load image".  "Load image" is more
similar to "reload image" and less likely to be confused with "view image".

(After writing this comment, I noticed that mpt made the same suggestion in
http://www.mozilla.org/projects/ui/menus/shortcut/.)

Comment 38

[Jesse Ruderman]

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

Comment 39

[Jesse Ruderman]

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

Comment 40

[Daniel Wang]

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

Comment 42

[Jesse Ruderman]

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

Comment 43

[Jesse Ruderman]

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

Comment 44

[wayne harper]

this is a must have for those of us still using dial-up. with it you can keep
images turned off & only view the ones you want.

Comment 45

[Daniel Wang]

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

Comment 46

[Felix Miata]

Anyone else think bug 185518 should be duped to this?

Comment 47

[Kai Lahmann (is there, where MNG is)]

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

Comment 48

[Eugene Savitsky]

Nominating. Seen thoused times questions about this feature in discussions on
the net in all kind of forums (russian, estonian, international). Very needed
function for notebook/PDA internet users thrue GPRS (an GSM internet access).

Comment 49

[Boris Zbarsky [:bzbarsky]]

Once bug 83774 lands this may not be that hard to do.... (just have to be
careful for the security issues).

Comment 50

[Joachim Noreiko]

This context-menu option should be available at all times, irrespective of the
"Do not load any images" preferences setting.

here's why: on a modem, a page with many images may take a long time. But a user
can often tell which images will interest them more - from reading ALT text, or
from the surrounding text. 
It should be possible to right-click the image placeholder, select "Load this
image" to move it to the front of the "image-getting queue".

Comment 51

[Samir Gehani]

adt: nsbeta1-

Comment 52

[rstupp]

Hi!

Just an issue, why people may want to disable image loading in Mail&News:
Spam Mails often embed image locations to track, whether an email-address is
valid and used (read by someone).

So disabling automatic image loading in mail & news is a nice feature.

Greetings,
Robert

Comment 53

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 54

[Christian :Biesinger (don't email me, ping me on IRC)]

my suggestion from bug 209262:
Well, if you ask me, we should use the current "view image" option for this:
o) If the image is not currently loaded, load the image and display it in place
o) If it is, show only the image (=current behaviour)

Comment 55

[Manko]

Christian, it seems to me your suggestion isn't handly. If image is already
loaded it will be nice use "Load/Reload Image" context menu item for reload
image. See comment 35 and comment 37.

Showing only selected image (without HTML document) is, I guess, less essential
feature. If you consider it to be necessary we could add another item in context
menu named e.g. "Show Only This Image" by analogy with iframe context menu.

More, with same analogy, for reducing context menu we could hide all image menu
items in a submenu, e.g.:
This Image >
  ---------------------
  Load/Reload Image
  ---------------------
  Block Images from this Server
  Block Images and Cookies from this Server
  ---------------------
  Show Only This Image
  Open Image in New Window
  Open Image in New Tab
  ---------------------
  Copy Image Location
  Save Image as...
  Send Image...
  Set As Wallpaper
  ---------------------
  View Image Info
  Open Long Description
  Open Long Description in New Window
  Open Long Description in New Tab

Comment 56

[Christian :Biesinger (don't email me, ping me on IRC)]

Eh, I don't think that we need an entire submenu for image-related commands

Comment 57

[Manko]

Maybe :) but Reload command for loaded images seems to be more handly that View
command.

Comment 58

[JonStipe]

My ISP has had a problem lately where, when downloading a web page with lots of
images, the download of some images might get corrupted sometimes, causing them
to appear all messed up (it's a dial-up connection). Since it only seems to
happen when lots of images are being downloaded at once, a normal reload will
just reproduce the problem. Currently, the only solution is to right-click a
damaged image, select "View image", reload just that image, and go back. Having
a "reload image" option would help a lot for this.

Comment 59

[Brett Taylor]

Isn't this fixed?  Suggest to mark as RESOLVED.

Comment 60

[Christian :Biesinger (don't email me, ping me on IRC)]

>Isn't this fixed?

no.

Comment 61

[Jo Hermans]

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

Comment 62

[Stefek Zaba]

Pretty please: this is a usability and privacy feature which is clunky to
work around. In the absence of a single-click-action whose effect is "show me
the page populated with images, temporarily overriding my
image-blocking-preferences", Joe Luser - me, in this case - either wanders off
to Edit->Prefs->Priv&Sec->Images
and selcts the "take me now, big boy" option - "Accept All Images", *intending*
(but probably, nay, provably) forgetting to reset it afterwards; or, far worse,
fires up A.N.Other browser (Exploder, mayhap), with loss of loyalty/mindshare to
Mozilla. It's one neat idea from Netscape 4.x which is, IMNSHO, worth carrying
across. But what do I know...

It's obviously non-trivial, or the request wouldn't still be in the Bugzilla DB
three years after first being raised; and it's in the Wishlist FAQ, so I'm not
the only Netscape refugee hankering after it. All I can offer in further support
is "think of the undying gratitude of the userbase, and the growth in karma"... 

Thanks, Stefek

Comment 63

[James Carr]

an important bug being fixed? dream on

Comment 64

[Christian :Biesinger (don't email me, ping me on IRC)]

can you people STOP ADDING USELESS COMMENTS, they make it even less likely that
a bug will be fixed (because it's harder to spot the relevant information).
thank you.

Comment 65

[James Carr]

clearly additional comments are interupting all the technical discussions one
how this bug is going to be fixed. whats that you say? nothing has been done for
how long? oh well never mind. hail the comment hitler.

Comment 66

[Christian :Biesinger (don't email me, ping me on IRC)]

well after that comment, my inclination to fix this bug is no longer existing.

Comment 67

[flii (cj)]

re: comment #66
you mean someone's actually working on it?  awesome!  i'm not sure how much help
i would be since i use firebird instead of mozilla, but when you come out with a
patch i will sure test it out if it would do any good.

i know this would be a particularly nice feature when you block ads by blocking
key terms in usercontent, such as "banner", and then a site you actually want to
see pictures on has named their main logo "topbanner" or something similar. 
i've been waiting for mozilla to get it implemented so it will trickle down to
firebird, and it's heartening to know someone is taking it seriously.  best of luck!

Comment 68

[Manko]

Please, do not argue about necessity of fixing this bug here. Your activity
affects only negatively.

Could anybody see attachment 258038 [details] [diff] [review] and adapt it to current Mozilla trunk?

And, please, add DO NOT SPAM to Status Whiteboard.

Comment 69

[MAX]

Browsing without images still do not work.
So option "do not load any images" - senseless.
Turn off images and go to
http://www.tomshardware.com
 We see wrong page layout so we can not read headlines. IE-> OK.
http://www.pornstaraction.com/
 We do not see image borders and alttext so we can not right-click on image
 placeholder to load and view DESIRED image. IE -> OK.
So browsing without images DO NOT WORK! Is this bug???
PLEASE do browsing without images:
1 Turn off images (may be from customized toolbar)
2 Load page with:
  CORRECT layout
  Image borders (it MUST exist for right-clicking, as i understand if IE can 
  not determine image size it make default minimal square ~100x100mm -> so 
  borders always exist for right-click)
  Image alt-text (it MUST exist, for desision view or not)
3 Right-click menu with option view image. And load ONLY DESIRED image WITHOUT
  open new or reload page.

Comment 70

[Alfonso Martinez]

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

Comment 71

[R.K.Aa.]

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

Comment 72

[Jo Hermans]

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

Comment 73

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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

Comment 74

[Dimitrios]

Users interested in this bug may look at ShowImage extension at
http://showimage.mozdev.org/. It is made for Firefox. It didn't work very well
for me (maybe it needs updating) but it seems a good approach (no further
cluttering of the context menu for users with fast connections).

Comment 75

[Justin]

Just a small note: This functionality exists in IE 6. Set IE to not load images.
Right click on an image placeholder, 5th option down "Show Picture". And it
loads the image without reloading the page.

Comment 76

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 77

[Boris Zbarsky [:bzbarsky]]

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

Comment 78

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

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

Comment 79

[Dan Fischbach]

Similar bug?:
https://bugzilla.mozilla.org/show_bug.cgi?id=218142

Comment 80

[Dan Fischbach]

Ack, I'm an idiot...Bugzilla needs an edit button/link.

Comment 81

[R.K.Aa.]

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

Comment 82

[Mike Connor [:mconnor]]

The firefox bug depending on this was already minused.

Comment 83

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: SeaMonkey patch

Comment 84

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 196930 [details] [diff] [review]
SeaMonkey patch

	 // View Image depends on whether an image was clicked on.
	 this.showItem( "context-viewimage", this.onImage &&
!this.onStandaloneImage);
-

why remove that line?

I'm not really happy about showing this for images that are not broken. I'd
prefer it if reload for those images actually used a VALIDATE_ALWAYS load flag,
which this patch doesn't. r=biesi on this patch if you make it only appear for
broken images (this code has a check for those already, iirc)

Comment 85

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: better patch

Comment 86

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197106 [details] [diff] [review]
better patch

Just looking at the core changes here....

>Index: mozilla/content/base/public/nsIImageLoadingContent.idl

>+  /**
>+   * forceReload reloads the last image that was attempted to be loaded
>+   * (as indicated by currentURI), using VALIDATE_ALWAYS.

How about "forceReload forces reloading of the image pointed to by currentURI"?
 VALIDATE_ALWAYS is, imo, an implementation detail that consumers of this API
should not have to know about....

>+   * @throws NS_ERROR_NOT_AVAILABLE if this isn't an image or no load was
>+   * attempted yet

How about "throws NS_ERROR_NOT_AVAILABLE if there is no current URI to reload"?

With that, looks good to me.

Comment 87

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

Comment on attachment 197106 [details] [diff] [review]
better patch

>Index: mozilla/xpfe/communicator/resources/content/nsContextMenu.js

>+        this.showItem( "context-reloadimage", this.onImage);

>+    // Reload image
>+    reloadImage : function () {
>+        urlSecurityCheck( this.imageURL, document );
>+        if (this.target instanceof Componenets.interfaces.nsIImageLoadingContent)
>+          this.target.forceReload();

If you're here, this.onImage should be true, and this.onImage already checks
instanceof nsIImageLoadingContent (and currentURI != null).

http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/nsC
ontextMenu.js#287

Comment 88

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

(I also failed to notice the "Componenets" typo :)

Comment 89

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 197106 [details] [diff] [review]
better patch

despite gavin's observations, I'd rather keep the check... not only does this
way make it clearer that forceReload is a function of nsIImageLoadingContent,
but the code is also separated in time and space from the one that sets
onImage.

very nice. r=biesi

Comment 90

[[e-mail]]

Comment on attachment 197106 [details] [diff] [review]
better patch

>+<!ENTITY reloadImageCmd.label         "Reload Image">
>+<!ENTITY reloadImageCmd.accesskey     "R">
This currently conflicts with "Save Link Target". However should
https://bugzilla.mozilla.org/show_bug.cgi?id=146825#c40 get implemented it will
then conflict with "Set As Wallpaper" but I might possibly be able to change
that to use "p".

Comment 91

[[e-mail]]

Comment on attachment 197106 [details] [diff] [review]
better patch

It took me some time to work out what this code did (basically by testing it on
a hit counter), as it appears to be of no use on blocked images.

Comment 92

[[e-mail]]

Comment on attachment 197106 [details] [diff] [review]
better patch

Well, I don't really see the utility of this particular patch, but please get the context menu access key conflicts fixed before checking this in.

Comment 93

[Artem S. Tashkinov]

Is there any chance to see this bug resolved in MoFo/SeaMonkey Trunk versions?

Comment 94

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 197106 [details] [diff] [review]
better patch

Requesting approval on 1.8 branch for SM1.1 due to (minor) core change.

Comment 95

[Lloyd Wood]

fixing status whiteboard typo's.

Comment 96

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

It wasn't a typo :)

Comment 97

[Mike Connor [:mconnor]]

tossing to 1.8.1 list, I don't think we want this particular patch for Firefox, and this is contains an API change so it shouldn't be approved as-is for 1.8.1.

Comment 98

[Manko]

Question about patch: will Shift + "Reload Image" do forced reloading (similar as Shift+Reload for whole page)?

Comment 99

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #98)
> Question about patch: will Shift + "Reload Image" do forced reloading (similar
> as Shift+Reload for whole page)?
> 

I wrote another version that supports shift just like the reload button, but for some reason it doesn't work at all.

Comment 100

[Ria Klaassen (not reading all bugmail)]

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

Comment 101

[[e-mail]]

Comment on attachment 197106 [details] [diff] [review]
better patch

My understanding is that we're not allowed to change nsIImageLoadingContent.idl

Comment 102

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

blocking-: we wouldn't hold for this RFE.  (Neil could certainly approve a patch for the branch for SM, and mconnor or someone for FF.)

Comment 103

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Neil, what key should I use?

Comment 104

[[e-mail]]

(In reply to comment #103)
>Neil, what key should I use?
No idea offhand; the current set of keys are all wrong anyway, see bug 146825.

Comment 105

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: unrotted

Comment 106

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Neil, for the XPFE side, let's say I used "G" as the access key.

Comment 107

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: unrotted, addressing bz's review comments from before

Comment 108

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233530 [details] [diff] [review]
unrotted, addressing bz's review comments from before

>Index: content/base/src/nsImageLoadingContent.cpp
>+  if (!currentURI)
>+    return NS_ERROR_NOT_AVAILABLE;

Curlies around the body, please.

With that the content parts look good.  I didn't look at the XUL/JS changes...

Comment 109

[[e-mail]]

Comment on attachment 233530 [details] [diff] [review]
unrotted, addressing bz's review comments from before

only xul/js change was typo fix right?

Comment 110

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #109)
> (From update of attachment 233530 [details] [diff] [review] [edit])
> only xul/js change was typo fix right?
> 

Yes, and I now patch both xpfe/ and suite/.

Comment 111

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Checked in on trunk (SeaMonkey 1.5).  Not going on 1.8 branch as discussed before.
Marking fixed because the core component of this bug is fixed.  UI for Firefox is in bug 218142.

Comment 112

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Resolving based on comment 111 (apparently I forgot to click the button?)

Comment 113

[Nickolay_Ponomarev]

This didn't actually implement 'Show image', does it? (forceReload doesn't work with images turned off.)

Comment 114

[Worcester12345]

(In reply to comment #0)
> From Bugzilla Helper:
> User-Agent: Mozilla/4.72 [en] (X11; I; Linux 2.2.17 i686; Nav)
> BuildID:    2000073008
> 
> Netscape4x has a preference setting as follows,
> "Automatically load images and other data types
> (Otherwise, click the images button to load when needed)".
> I like this feature, i.e. I can load images by clicking button on the images,
> only when I want to see them.
> 
> Mozilla has a similar setting, "Do not load any images".
> But it lacks the above feature.
> When I choose this setting, I have no way to load images.
> 
> 
> Reproducible: Always
> Steps to Reproduce:
> 1.Choose "Do not load any images" setting.
> 2.Open an HTML file including some images.
> 3.Click button on an image.
> 
> Actual Results:  Nothing happens.
> 			
> 
> Expected Results:  I expect it to load the image.(only that image).
> 
> I think "Do not load any images" should be "Do not load any images
> automatically".

Where do we enable this feature in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061001 SeaMonkey/1.5a?

Comment 115

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #114)
> Where do we enable this feature in Mozilla/5.0 (Windows; U; Windows NT 5.1;
> en-US; rv:1.9a1) Gecko/20061001 SeaMonkey/1.5a?

The scope of the bug I fixed was: if images are not blocked, when right-clicking an image or broken image, it should be possible to force a reload.  The justification for this behavior is dailup users who stop a page load early and only want to load specific images (or broadband users dealing wth slow servers).  There is no way to "enable" the feature - just right-click a [non-css-applied] image.

Comment 116

[Nebojsa]

I think that if I hit right-click on the broken image it shouldn't be reloaded in new window or tab. I need to see it on it's place. Not to mention the sites with so many little pics close to one another as a part of website's design. Developers sould reconsider putting this option as an add-on (plug-in) and who wants it...
If something like that exist, please reply here.

Comment 117

[Christian :Biesinger (don't email me, ping me on IRC)]

That's now part of SeaMonkey. File a new bug for Firefox I guess.

Comment 118

[Manko]

This isn't fixed in SM 1.1.1, menu item doesn't appear.