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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
ImageLib
P3
enhancement
RESOLVED FIXED
17 years ago
3 years ago

People

(Reporter: INOUE Seiichiro, Assigned: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com])

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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".
morse, this sounds like something in your area --is it?
Assignee: dveditz → morse
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: Preferences: Backend → XP Apps
Ever confirmed: true
Keywords: 4xp
QA Contact: sairuh → elig

Comment 2

17 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: --- → Future

Comment 3

17 years ago
Corrected the wording on the summary line.  Previous wording was confusing. 
Summary: No preference for "click the images button to load when needed" → Need "show-image" item on right-click menu similar to 4.x
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

17 years ago
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.
okay.
Keywords: helpwanted

Comment 7

17 years ago
"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?)

Updated

17 years ago
Target Milestone: Future → M25

Updated

17 years ago
Summary: Need "show-image" item on right-click menu similar to 4.x → [y]Need "show-image" item on right-click menu similar to 4.x
Target Milestone: M25 → ---

Comment 8

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

Updated

17 years ago
Summary: [y]Need "show-image" item on right-click menu similar to 4.x → Need "show-image" item on right-click menu similar to 4.x
Whiteboard: [y]

Updated

17 years ago
OS: Linux → All
QA Contact: elig → sairuh
Hardware: PC → All

Comment 9

17 years ago
nav triage team:

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

Comment 10

17 years ago
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.
Assignee: morse → pnunn
Status: ASSIGNED → NEW
Component: XP Apps → ImageLib
QA Contact: sairuh → tpreston

Updated

17 years ago
Whiteboard: [y]

Comment 11

17 years ago
Created attachment 24923 [details]
Proposed solution

Comment 12

17 years ago
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

17 years ago
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.
Assignee: pnunn → pavlov
->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

17 years ago
Igor, could you please do as sairuh requests. Thanks.

Comment 16

17 years ago
Created attachment 25173 [details]
Proposed solution (as ZIP file)

Comment 17

17 years ago
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.
pavlov [or someone else], could you pls review the attached patch? need to save
as zip, then unzip 'em...
Keywords: patch, review

Comment 19

17 years ago
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

17 years ago
Created attachment 25320 [details] [diff] [review]
nis@sparc.spb.su's patch

Comment 21

17 years ago
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

17 years ago
Created attachment 25339 [details] [diff] [review]
patch with style changes

Comment 23

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Created attachment 25803 [details] [diff] [review]
Patch updated to work with main mozilla trunk

Comment 30

17 years ago
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.

Updated

17 years ago
Depends on: 75338

Updated

17 years ago
Target Milestone: --- → Future

Comment 31

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

Comment 32

17 years ago
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

16 years ago
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 ?

Updated

16 years ago
Keywords: patch, review → mozilla0.9.9

Comment 34

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

Comment 35

16 years ago
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.
*** Bug 140811 has been marked as a duplicate of this bug. ***

Comment 37

15 years ago
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

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

Comment 39

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

Comment 40

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

Updated

15 years ago
Keywords: mozilla0.9.9 → patch

Updated

15 years ago
Blocks: 164421
*** Bug 101058 has been marked as a duplicate of this bug. ***

Comment 42

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

Comment 43

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

Comment 44

15 years ago
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

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

Updated

15 years ago
Summary: Need "show-image" item on right-click menu similar to 4.x → Need "Show Image" item on context menu

Comment 46

15 years ago
Anyone else think bug 185518 should be duped to this?
*** Bug 185518 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Summary: Need "Show Image" item on context menu → Need "Show Image" / "Reload Image" on context menu

Comment 48

15 years ago
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).
Keywords: nsbeta1

Updated

15 years ago
Blocks: 193867
Once bug 83774 lands this may not be that hard to do.... (just have to be
careful for the security issues).
Depends on: 83774

Comment 50

15 years ago
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

15 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Comment 52

14 years ago
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

Updated

14 years ago
Blocks: 163993
*** Bug 209262 has been marked as a duplicate of this bug. ***
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

14 years ago
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
Eh, I don't think that we need an entire submenu for image-related commands

Comment 57

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

Comment 58

14 years ago
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

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

Updated

14 years ago
Blocks: 218142
>Isn't this fixed?

no.

Comment 61

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

Comment 62

14 years ago
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

14 years ago
an important bug being fixed? dream on
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

14 years ago
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.
well after that comment, my inclination to fix this bug is no longer existing.

Comment 67

14 years ago
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

14 years ago
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

14 years ago
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

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

Comment 71

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

Comment 72

13 years ago
*** Bug 252594 has been marked as a duplicate of this bug. ***
*** Bug 255600 has been marked as a duplicate of this bug. ***

Comment 74

13 years ago
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

13 years ago
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.
*** Bug 268063 has been marked as a duplicate of this bug. ***
Summary: Need "Show Image" / "Reload Image" on context menu → Need "Show(View) Image" / "Reload Image" on context menu
*** Bug 283776 has been marked as a duplicate of this bug. ***
*** Bug 284006 has been marked as a duplicate of this bug. ***

Comment 79

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

Comment 80

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

Comment 81

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

Updated

12 years ago
No longer blocks: 163993

Updated

12 years ago
Flags: blocking-aviary1.1?
The firefox bug depending on this was already minused.
Flags: blocking-aviary1.1?
Created attachment 196930 [details] [diff] [review]
SeaMonkey patch
Assignee: pavlov → cst
Status: NEW → ASSIGNED
Attachment #196930 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196930 - Flags: review?(cbiesinger)
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)
Attachment #196930 - Flags: review?(cbiesinger) → review-
Attachment #196930 - Attachment is obsolete: true
Attachment #196930 - Flags: superreview?(neil.parkwaycc.co.uk)
Created attachment 197106 [details] [diff] [review]
better patch
Attachment #197106 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #197106 - Flags: review?(cbiesinger)
Keywords: helpwanted
Whiteboard: [cst: r? sr?]
Attachment #197106 - Flags: review?(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.
Attachment #197106 - Flags: review?(bzbarsky) → review+
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
(I also failed to notice the "Componenets" typo :)
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
Attachment #197106 - Flags: review?(cbiesinger) → review+

Comment 90

12 years ago
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

12 years ago
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.
Whiteboard: [cst: r? sr?] → [cst: r? sr?] [cst: COMPONENENTS]

Comment 92

12 years ago
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.
Attachment #197106 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Comment 93

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

Updated

12 years ago
Flags: blocking-aviary2?
Whiteboard: [cst: r? sr?] [cst: COMPONENENTS] → [cst: what key doesnt conflict?, COMPONENENTS, r+ sr+]
Comment on attachment 197106 [details] [diff] [review]
better patch

Requesting approval on 1.8 branch for SM1.1 due to (minor) core change.
Attachment #197106 - Flags: approval1.8.1?

Comment 95

12 years ago
fixing status whiteboard typo's.
Whiteboard: [cst: what key doesnt conflict?, COMPONENENTS, r+ sr+] → [cst: what key doesn't conflict?, COMPONENTS, r+ sr+]
It wasn't a typo :)
Whiteboard: [cst: what key doesn't conflict?, COMPONENTS, r+ sr+] → [cst: what key doesn't conflict?, Componenets, r+ sr+]
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.
Flags: blocking-aviary2? → blocking1.8.1?

Comment 98

12 years ago
Question about patch: will Shift + "Reload Image" do forced reloading (similar as Shift+Reload for whole page)?
(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.
*** Bug 323416 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Attachment #197106 - Flags: approval1.8.1? → branch-1.8.1?(neil)
Comment on attachment 197106 [details] [diff] [review]
better patch

My understanding is that we're not allowed to change nsIImageLoadingContent.idl
Attachment #197106 - Flags: branch-1.8.1?(neil) → branch-1.8.1-
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.)
Flags: blocking1.8.1? → blocking1.8.1-
Neil, what key should I use?
(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.
Created attachment 233526 [details] [diff] [review]
unrotted
Attachment #197106 - Attachment is obsolete: true
Attachment #233526 - Flags: superreview?(neil)
Attachment #233526 - Flags: review?(bzbarsky)
Neil, for the XPFE side, let's say I used "G" as the access key.
Whiteboard: [cst: what key doesn't conflict?, Componenets, r+ sr+] → [cst: r? sr?]
Created attachment 233530 [details] [diff] [review]
unrotted, addressing bz's review comments from before
Attachment #233526 - Attachment is obsolete: true
Attachment #233530 - Flags: superreview?(neil)
Attachment #233530 - Flags: review?(bzbarsky)
Attachment #233526 - Flags: superreview?(neil)
Attachment #233526 - Flags: review?(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...
Attachment #233530 - Flags: review?(bzbarsky) → review+
Comment on attachment 233530 [details] [diff] [review]
unrotted, addressing bz's review comments from before

only xul/js change was typo fix right?
Attachment #233530 - Flags: superreview?(neil) → superreview+
(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/.
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.
Whiteboard: [cst: r? sr?]
Target Milestone: Future → mozilla1.9alpha
Resolving based on comment 111 (apparently I forgot to click the button?)
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 113

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

Comment 114

11 years ago
(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?
(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

11 years ago
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.
That's now part of SeaMonkey. File a new bug for Firefox I guess.

Comment 118

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

Updated

3 years ago
Duplicate of this bug: 715399
Blocks: 715399
You need to log in before you can comment on or make changes to this bug.