218142 - Add "Show Image" to context menu of not-successfully-loaded images

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement)

Description

[alanjstr]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030803 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030803 Mozilla Firebird/0.6.1

Need context menu items for images in FB, like
http://bugzilla.mozilla.org/show_bug.cgi?id=47475

Reproducible: Always

Steps to Reproduce:

Comment 1

[Simon Paquet [:sipaq]]

Confirming for developer review and taking QA Contact. IMO we should reuse the
"view image"-option instead of introducing two new entries in the context menus.
This is also dicsussed in the referenced bug.

Comment 2

[Mike Connor [:mconnor]]

We're already getting into bloat territory with image context menu stuff

if the image isn't shown, a show image option makes sense, but Reload Image is
not needed...

Comment 3

[Manko]

"Reload image" would be very useful to reload dynamic images (CGI-scripted
counters or diagrams, webcam pictures etc.) without reloading whole page.

Comment 4

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040126 Firebird/0.8.0+
(.:MrC:.)

Firebird already has UI to prevent the loading of images - having a Show Image
context entry would allow a user to load each image individually (this may be
useful for dial-up users).

Comment 5

[Chris Wood]

A Reload Image feature should be in an extension; but Show Image should be included.

Comment 6

[Beren Sanders]

4/5 people I have gotten to try out firefox have been annoyed at the lack of a
"Show Image in current page" feature.  I think it is a very useful and necessary
feature for people who want to minimize their bandwidth, and don't like images.
  They want to be able to right click where the image would be and go like "Show
Image" and have that image then be rendered in the page.

Comment 7

[Phil Ringnalda (:philor)]

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

Comment 8

[Ben Basson]

For those interested, there is an extension that does this:
http://showimage.mozdev.org/

This would be nice and easy to port over to native code. Please consider setting
this as a target for Firefox 1.1 or 1.5.

Comment 9

[Justin Kerk]

There's no reason not to have Reload Image, you could have it occupy the same
menu position as Show Image, and which one is shown would depend on whether or
not the image has been loaded.

Comment 10

[Peter van der Woude [:Peter6]]

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

Comment 11

[José Jeria]

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

Comment 12

[José Jeria]

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

Comment 13

[David Everett]

In MS IE I use the Show Image all the time, for this reason.  I am active on
many forum boards, and if I am trying to pull up multiple boards at once I will
"stop" the browser when text is loaded, so I dont have to wait for the pictures.
 Well, ocassionaly, a ppicture will be posted that pertains to the post which
must be seen.  In this case I am able to right-clikc the picture and choose
"show image" which in a sense 'reloads' the image.  Firefox doesnt do this, it
should.

Comment 14

[Kevin Brosnan]

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

Comment 15

[Peter van der Woude [:Peter6]]

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

Comment 16

[Jo Hermans]

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

Comment 17

[Rohan]

When you plane Fix thish BUG ??

Comment 18

[Ian Macfarlane]

This is really needed for feature parity with IE. Suggesting we put 'show image' in the context menu of an image only where the image has not been loaded. It's not needed where the image has been loaded, or anywhere else (i.e. make it context-smart).

Reload image is obviously controversial and an extension is already available to do it for power-users who want this functionality.

As mentioned in comment 4, this is a big benefit to dial-up users, and having it context-smart should mean it would not be menu-bloat..

This should be an easy (few lines?) fix, and is clearly desirable.

Comment 19

[alanjstr]

There is no attached patch, no assignee, and beta 1 is already out.  I doubt this will make Firefox 2.0, so please remove your request.

Comment 20

[Rohan]

(In reply to comment #19)
> There is no attached patch, no assignee, and beta 1 is already out.

You can use source code from this extension http://showimage.mozdev.org/
Please add it in FireFox 2.0

Comment 21

[Mike Connor [:mconnor]]

We'd like to get this, but not going to block on it at this stage.

Comment 22

[Jonathan Haas]

Attachment: Patch

Now "Show Image" shows the image inline (instead of a new page) if the image is currently unloaded. This is useful if loading images is disabled or there was a connection error while loading the image.

I did not add a new menuitem because that would cause l10n impact.

Comment 23

[Simon Bünzli]

Comment on attachment 229867 [details] [diff] [review]
Patch

>+          if (this.target.naturalWidth != 0)
>+            openUILink( this.imageURL, e );
>+          else
>+            this.loadImage(e);

I doubt that having the same menu item for different behavior will be accepted.

>+        	var pref = Components.classes[ "@mozilla.org/preferences-service;1" ]

Nit: No tabs please!

>+        	var oriImageBehavior = 0;

Nit: What's wrong with "originalImageBehavior"?

>+        	if( pref.getIntPref( "permissions.default.image") != 0 )

Nit: Always put a space after if. OTOH having spaces inside parentheses is against the style used in browser.js.

>+        	this.target.setAttribute( "src","" );

Nit: Spaces after commas.

>+        	return;

Nit: Unneeded.

Finally, the whole patch is obviously a wall paper patch. Not sure whether we'd prefer to rather wait for a correct implementation instead though.

Comment 24

[Jason Chu]

Looks like the loadImage function was copied/pasted from the Show Image extension.  :)  Another nitpick if this patch is used, why not set oriImageBehavior equal to the pref in the first place?  Then it doesn't have to be read twice.

Comment 25

[Jonathan Haas]

Attachment: fixed intendation etc.

I agree with you that putting different functions on the same menuitem is probably not a very good idea. I can provide an additional patch with a new menuitem if needed.

Comment 26

[Nickolay_Ponomarev]

This looks like a hack (wouldn't work with custom content-policy implementation), and I would instead modify nsContentUtils::CanLoadImage to make the content policy check optional, then use that from nsImageLoadingContent.

Anyway, you have more chances getting this reviewed by gavin.sharp

Comment 27

[Jo Hermans]

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

Comment 28

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

Not a blocker, but we'd take a patch for "Show Image" for when images are hidden.

Comment 30

[(no longer active)]

Taking over...

Comment 31

[(no longer active)]

Comment on attachment 229879 [details] [diff] [review]
fixed intendation etc.

This patch is now obsolete...

Comment 32

[(no longer active)]

Attachment: Patch (v1)

This patch does two things:

1) It fixes the |gContextMenu.onLoadedImage| variable to actually reflect whether the image is loaded or not.  The existing code sets |gContextMenu.onLoadedImage| to true if the size of the image is available.  This may happen for example when the browser has received the Content-Length HTTP hearder, but the actual image contents have not been received yet.

2) It adds the menu item Show Image above View Image and displays it only on partially-loaded or not-loaded images.  This menu item forces the image to reload using |nsIImageLoadingContent::forceReload()|.  This code is based on attachment 233530 [details] [diff] [review] from bug 47475.

Note: the current code hides the Show Image menu item for fully-loaded images.  If desired, I can modify the patch to change the title to Reload Image and have it reload such images, but for now I left out this functionality based on previous comments on this bug.

I've tested this patch on the trunk, and it works smoothly.

Comment 33

[alanjstr]

Can we tell if an image was partially loaded and then loading stopped (incomplete  data stream, someone hit stop, etc?)  In other words, do we know that it is considered partial vs complete?

Do you want to change bug status to Assigned?

Comment 34

[(no longer active)]

(In reply to comment #33)
> Can we tell if an image was partially loaded and then loading stopped
> (incomplete  data stream, someone hit stop, etc?)  In other words, do we know
> that it is considered partial vs complete?

Yes, we can.

Every image implements the nsIImageLoadingContent interface <http://www.xulplanet.com/references/xpcomref/ifaces/nsIImageLoadingContent.html>.  You can obtain an imgIRequest interface <http://www.xulplanet.com/references/xpcomref/ifaces/imgIRequest.html> from this interface's |getRequest()| method, and check its |imageStatus| property.  If it's STATUS_LOAD_COMPLETE, then the image is loaded completely.  If it's STATUS_LOAD_PARTIAL, the image is partially loaded (it might have been canceled by hitting Stop for example).  There are other status values as well: <http://www.xulplanet.com/references/xpcomref/ifaces/imgIRequest.html>

The proposed patch shows the Show Image item both for images that have not been loaded at all, *and* for images that are partially loaded.  IE has the same behavior, and it makes sense, because users may try to reload a partially loaded image as well.

> Do you want to change bug status to Assigned?

Yes, thanks for mentioning this!

Comment 35

[(no longer active)]

Attachment: Patch (v1) (strings only) (checked in)

String changes to land before the l10n freeze...

Comment 36

[(no longer active)]

Beltzner: can you review the string changes please? (attachment 298773 [details] [diff] [review])

This is a highly requested feature (78 votes) and it's so trivial to implement that it's a shame we don't have it in Firefox 3.  And it's marked as wanted-firefox3+ as well.

We need to land the strings at least before the l10n freeze.  Thanks!

(I'll ask Mano for a code review right away)

Comment 37

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

Comment on attachment 295241 [details] [diff] [review]
Patch (v1)

I'm wary of changing the definition of onLoadedImage (iirc that's had security implications in the past). Are you sure all it's users can handle the change? A look through CVS blame might be a good idea here, I don't remember all the details.

Comment 38

[(no longer active)]

(In reply to comment #37)
> (From update of attachment 295241 [details] [diff] [review])
> I'm wary of changing the definition of onLoadedImage (iirc that's had security
> implications in the past). Are you sure all it's users can handle the change? A
> look through CVS blame might be a good idea here, I don't remember all the
> details.

You're right, maybe there are extensions which rely on onLoadedImage in some way.  I can roll my own variable here, I suppose.  But first let's wait and see if I can get Beltzner's ui-r on the string change in time...

Comment 39

[(no longer active)]

(In reply to comment #37)
> (From update of attachment 295241 [details] [diff] [review])
> I'm wary of changing the definition of onLoadedImage (iirc that's had security
> implications in the past). Are you sure all it's users can handle the change? A
> look through CVS blame might be a good idea here, I don't remember all the
> details.

Gavin: were you referring to bug 293527?  From bug 293527 comment 6, I guess STATUS_SIZE_AVAILABLE was used to make sure that the content downloaded was a valid image.  So, I think we should add a new variable for this bug, and check it in addition to onLoadedImage.

Here's the checkin from that bug on browser.js:

<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=/mozilla/browser/base/content&command=DIFF_FRAMESET&file=browser.js&rev2=1.455&rev1=1.454>

Comment 40

[(no longer active)]

Comment on attachment 295241 [details] [diff] [review]
Patch (v1)

Holding review as per comment 39...

And another shameless ping to Beltzner...  :-)

Comment 41

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

Yeah, that's almost surely what I was thinking of. Thanks for looking into it!

Comment 42

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

Comment on attachment 298773 [details] [diff] [review]
Patch (v1) (strings only) (checked in)

r+uir+a=beltzner

Comment 43

[(no longer active)]

Need to check-in attachment 298773 [details] [diff] [review] before the l10n freeze

Comment 44

[Reed Loden [:reed]]

Checking in browser/locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <--  browser.dtd
new revision: 1.92; previous revision: 1.91
done

Comment 45

[(no longer active)]

Attachment: Patch (v2)

Gavin: addressed your comment 37.  Carrying over the ui-r request from beltzner.

Comment 46

[Cheng Khoon]

+    // Show image depends on an image that's not fully loaded
+    this.showItem("context-showimage", (this.onImage && !this.onLoadedImage));
+

I may be wrong, but somehow I feel that what you intended is !this.onCompletedImage instead.

Comment 47

[(no longer active)]

Attachment: Patch (v2.1) (checked in)

(In reply to comment #46)
> +    // Show image depends on an image that's not fully loaded
> +    this.showItem("context-showimage", (this.onImage && !this.onLoadedImage));
> +
> 
> I may be wrong, but somehow I feel that what you intended is
> !this.onCompletedImage instead.
> 

Oops!  Your completely right.  The new patch fixes this issue.  Thanks for catching this.

Comment 48

[Victor Bielawski]

Hmm... I think [ie parity] should be [parity-ie] or [parity-IE] like other bugs.

Comment 49

[(no longer active)]

(In reply to comment #48)
> Hmm... I think [ie parity] should be [parity-ie] or [parity-IE] like other
> bugs.

Right.  Done.

Comment 50

[(no longer active)]

Comment on attachment 301103 [details] [diff] [review]
Patch (v2.1) (checked in)

Trying mano for the code review.  mano: please see gavin's concerns in comment 37, my investigations on it in comment 39, which are addressed in the latest patch.

Beltzner: ping for ui-r...

Comment 51

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

Comment on attachment 301103 [details] [diff] [review]
Patch (v2.1) (checked in)

It's generally best to not change review requestees without poking first, especially when the reviewer has already taken a first look at the patch :)

>Index: browser/base/content/nsContextMenu.js

>+  showImage: function(e) {
>+    urlSecurityCheck(this.imageURL,
>+                     this.browser.contentPrincipal,
>+                     Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);

I think this security check is redundant - nsImageLoadingContent::ForceReload just calls  nsImageLoadingContent::LoadImage, which is the normal image loading path. It calls nsContentUtils::CanLoadImage which does a checkLoadURI and content policy check. I suppose it doesn't really hurt to leave it to protect against the forceReload implementation ever changing, though.

Comment 52

[(no longer active)]

Thanks for the review, gavin, and sorry for the hasty review request change.  :-)

Now let's see if the UI is OK with Beltzner or not.  Since this is a simple in code/UI but huge in functionality for dialup users, I hope we get a ui-r from Beltzner soon, so that this can get in for beta4.

Comment 53

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

Comment on attachment 301103 [details] [diff] [review]
Patch (v2.1) (checked in)

ui-r+a=beltzner, make sure to mark it late-l10n

Comment 54

[Reed Loden [:reed]]

It's not late-l10n, as the strings landed before string freeze.

Comment 55

[Reed Loden [:reed]]

Checking in browser/base/content/browser-context.inc;
/cvsroot/mozilla/browser/base/content/browser-context.inc,v  <--  browser-context.inc
new revision: 1.39; previous revision: 1.38
done
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.35; previous revision: 1.34
done

Comment 56

[PikeUK]

This doesn't work (nothing happens, no error msgs) if "Load images automatically" is disabled in Options->Content, if I add the hack from the 2006 patch it does work.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008021304 Minefield/3.0b4pre

Comment 57

[(no longer active)]

(In reply to comment #56)
> This doesn't work (nothing happens, no error msgs) if "Load images
> automatically" is disabled in Options->Content, if I add the hack from the 2006
> patch it does work.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008021304
> Minefield/3.0b4pre

My first reaction was something along the lines of "we must respect user prefs if they don't want to load an image, but we present this option in the UI as "Load images _automatically_".  So, a logical use case would be for a user to disable this pref, and then use the new Show Image menu item on image placeholders to individually load the ones she may be interested in.  This actually looks like a valid point to me, so I'll provide a followup patch soon based on comment 26.

Comment 58

[(no longer active)]

Attachment: Ignore image blocking status

This patch adds the checkContentPolicy attribute to the nsIImageLoadingContent interface, and skips the content policy check for the image blocking status if it's set to false in nsImageLoadingContent::LoadImage().  This is implemented in nsContentUtils::CanLoadImage().

Requesting review and super-review on the content/ part from sicking, and review on the browser/ part from gavin.

Comment 59

[Reed Loden [:reed]]

Comment on attachment 303275 [details] [diff] [review]
Ignore image blocking status

Please file a new bug for this patch and mark this one obsolete. Having multiple patches per bug only creates problems with tracking if a bug is fixed or not.

Comment 60

[(no longer active)]

Comment on attachment 303275 [details] [diff] [review]
Ignore image blocking status

(In reply to comment #59)
> (From update of attachment 303275 [details] [diff] [review])
> Please file a new bug for this patch and mark this one obsolete. Having
> multiple patches per bug only creates problems with tracking if a bug is fixed
> or not.

-> bug 417545

Comment 61

[mmortal03]

Did this bug break the old behavior of "View Image"?  I think "View Image" is now working like "Load/Reload Image".  "View Image" should allow one to view just that image in the current window, or if middle clicked, in a new window, as it used to be.  A new option should be created for loading/reloading an image in the current page, and that other option needs left alone. It would say "Load Image" if the image didn't initially load, and "Reload Image" if the image did load.  Please refer me to the right bug report if this one isn't it.  I searched all over for bugs regarding "View Image", and found nothing relevant.

Comment 62

[mmortal03]

I apologize for the previous post.  "View Image" is in fact not working like "Show Image".  I see that they are in fact separate options.  Instead, it looks as if "View Image" is just broken (for me).

Comment 64

[SEN]

Ok, "Show Image" is work, but only when "Load images automatically" is on... and image broken, but when "Load images automatically" is off, "Show Image" do nothing. Its not correct.

Comment 65

[Kim Sullivan]

I don't see a "show image" context menu item on images that don't finish loading due to a network timeout. Should this bug be reopened, or should I file a new bug? I'm using FF3 RC1, and haven't tweaked any image loading preferences.

Comment 66

[(no longer active)]

Can you reproduce this consistently?  Does it happen only sometimes, or have you never seen the Show Image menu item at all?  Can you provide a set of steps to reproduce the problem?

Comment 67

[Kim Sullivan]

Yes, this happens consistently - whenever I open several tabs with lots of images at once, some of the images don't finish loading in time (I only have 512kbit/s and the conection gets congested easily), and are only halfway finished. In that case, the show image menu doesn't appear (simply reloading the page doesn't help either, only view image and then ctrl-f5 to refresh).

The "Show Image" menu item appears consistently on images that don't get loaded at all (due to a 404 or 403 http error).

I was thinking of how to provide a reliable testcase to demonstrate this, is there any way to artificially force a timeout on an image (for example, a script that sends only part of the image and then waits, or something like that)?

Comment 68

[(no longer active)]

(In reply to comment #67)
> Yes, this happens consistently - whenever I open several tabs with lots of
> images at once, some of the images don't finish loading in time (I only have
> 512kbit/s and the conection gets congested easily), and are only halfway
> finished. In that case, the show image menu doesn't appear (simply reloading
> the page doesn't help either, only view image and then ctrl-f5 to refresh).

That's weird.  I'm on a 256kbps connection and I see the Show Image menu on all of the partially loaded images.  In fact, this menu item appears on images that are loading as well.

> The "Show Image" menu item appears consistently on images that don't get loaded
> at all (due to a 404 or 403 http error).

That's the expected behavior.

> I was thinking of how to provide a reliable testcase to demonstrate this, is
> there any way to artificially force a timeout on an image (for example, a
> script that sends only part of the image and then waits, or something like
> that)?

Yes, but that requires you to have access to a server.  Another possible option would be to find a we page with very large images, and cancel the loading of the page at some point, and try to see if the Show Image menu appears on the half-loaded images.

Also, what platform are you using?

Comment 69

[Kim Sullivan]

I'm using FF3 on Windows XP (I probably won't be able to get to it during the weekend, my Ubuntu notebook hasn't been upgraded yet).

On linux it could be possible to simulate network congestion using trickle http://monkey.org/~marius/pages/?page=trickle or tc http://www.kdedevelopers.org/node/1878 even without having access to a remote server (but a local server would still be required).

I have a feeling that FF thinks that the images have actually finished loading, even though they haven't. Could this happen when the image host doesn't send the filesize in the reply, and FF isn't able to determine if it has the whole image after the data stops coming in? (this would probably mean that it doesn't happen as consistently as I think, but I haven't tried running some packet sniffer yet... I just saw that a lot of pictures didn't finish loading and the menuitem was missing).

Comment 70

[(no longer active)]

Hmm, I suspect that this might be a problem in netwerk code, because all I do in this patch is query the backend to see if the image has finished loading, and show the Show Image menu item in case it hasn't.  It would be great if you can provide a set of steps to reproduce, and file a new bug and making it block this one.

Thanks!

Comment 71

[Kim Sullivan]

Didn't find a "network" module, so i reported it in the "menu" module, it's bug 436483.

Comment 72

[Dão Gottwald [:dao]]

from bug 439133:
> If one was inclined to add tests for this,
> browser/base/content/test/test_contextmenu.html provides most of the framework
> for doing so. (You'd probably have to add a bit to make some element display
> this context menu.)

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, I don't think that a Litmus test makes sense here. How should we find a website which doesn't finish loading of images over time? I strongly believe that this should go into an automated test. Therefor the httpd.js server could be used.