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

RESOLVED FIXED in Firefox 3 beta4

Status

()

Firefox
Menus
--
enhancement
RESOLVED FIXED
14 years ago
3 years ago

People

(Reporter: alanjstr, Assigned: Ehsan)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 3 beta4
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-IE])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

14 years ago
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:
(Reporter)

Updated

14 years ago
Depends on: 47475

Comment 1

14 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: asa → bugzilla
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

14 years ago
"Reload image" would be very useful to reload dynamic images (CGI-scripted
counters or diagrams, webcam pictures etc.) without reloading whole page.
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).

Updated

13 years ago
Flags: blocking1.0?
Flags: blocking1.0? → blocking1.0-

Comment 5

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

Comment 6

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

Comment 8

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

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

Comment 11

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

Comment 12

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

Updated

12 years ago
Hardware: All → PC

Comment 13

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

Updated

12 years ago
Blocks: 163993

Updated

12 years ago
No longer blocks: 163993

Updated

12 years ago
Blocks: 164421
Flags: blocking-aviary1.1?
Assignee: firefox → nobody
Component: General → Menus
QA Contact: bugzilla → menus
Hardware: PC → All
Version: unspecified → Trunk

Updated

12 years ago
Flags: blocking-aviary1.1? → blocking-aviary1.1-

Comment 14

12 years ago
*** Bug 306776 has been marked as a duplicate of this bug. ***
*** Bug 269875 has been marked as a duplicate of this bug. ***

Comment 16

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

Comment 17

11 years ago
When you plane Fix thish BUG ??

Comment 18

11 years ago
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.
Flags: blocking-firefox2?
(Reporter)

Comment 19

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

11 years ago
(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
We'd like to get this, but not going to block on it at this stage.
Flags: blocking-firefox2? → blocking-firefox2-
Keywords: helpwanted

Comment 22

11 years ago
Created attachment 229867 [details] [diff] [review]
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.
Attachment #229867 - Flags: review?(mconnor)

Comment 23

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

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

11 years ago
Created attachment 229879 [details] [diff] [review]
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.
Attachment #229867 - Attachment is obsolete: true
Attachment #229879 - Flags: review?(mconnor)
Attachment #229867 - Flags: review?(mconnor)

Comment 26

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

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

Updated

11 years ago
Flags: blocking-firefox3?
Not a blocker, but we'd take a patch for "Show Image" for when images are hidden.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Duplicate of this bug: 410616
Taking over...
Assignee: nobody → ehsan.akhgari
Keywords: helpwanted
Whiteboard: [wanted-firefox3] → [wanted-firefox3][ie parity]
Target Milestone: --- → Firefox 3 M11
Comment on attachment 229879 [details] [diff] [review]
fixed intendation etc.

This patch is now obsolete...
Attachment #229879 - Flags: review?(mconnor)
Attachment #229879 - Attachment is obsolete: true
Created attachment 295241 [details] [diff] [review]
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.
Attachment #295241 - Flags: ui-review?(beltzner)
Attachment #295241 - Flags: review?(gavin.sharp)
(Reporter)

Comment 33

10 years ago
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?
(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!
Status: NEW → ASSIGNED
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3][ie parity] → [ie parity]
Created attachment 298773 [details] [diff] [review]
Patch (v1) (strings only) (checked in)

String changes to land before the l10n freeze...
Attachment #298773 - Flags: ui-review?(beltzner)
Attachment #298773 - Flags: review?(beltzner)
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)
Attachment #295241 - Flags: review?(gavin.sharp) → review?(mano)
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.
(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...
(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 on attachment 295241 [details] [diff] [review]
Patch (v1)

Holding review as per comment 39...

And another shameless ping to Beltzner...  :-)
Attachment #295241 - Flags: review?(mano)
Whiteboard: [ie parity] → [ie parity][has patch][needs ui-review beltzner][string changes need to land before l10n freeze]
Yeah, that's almost surely what I was thinking of. Thanks for looking into it!
Comment on attachment 298773 [details] [diff] [review]
Patch (v1) (strings only) (checked in)

r+uir+a=beltzner
Attachment #298773 - Flags: ui-review?(beltzner)
Attachment #298773 - Flags: ui-review+
Attachment #298773 - Flags: review?(beltzner)
Attachment #298773 - Flags: review+
Attachment #298773 - Flags: approval1.9+
Need to check-in attachment 298773 [details] [diff] [review] before the l10n freeze
Keywords: checkin-needed
Whiteboard: [ie parity][has patch][needs ui-review beltzner][string changes need to land before l10n freeze] → [ie parity][has patch][string changes need to land before l10n freeze]
Attachment #298773 - Attachment description: Patch (v1) (strings only) → Patch (v1) (strings only) (for check-in before the l10n freeze)
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
Keywords: checkin-needed
Whiteboard: [ie parity][has patch][string changes need to land before l10n freeze] → [ie parity][has patch]
Attachment #298773 - Attachment description: Patch (v1) (strings only) (for check-in before the l10n freeze) → Patch (v1) (strings only) (checked in)
Created attachment 299520 [details] [diff] [review]
Patch (v2)

Gavin: addressed your comment 37.  Carrying over the ui-r request from beltzner.
Attachment #295241 - Attachment is obsolete: true
Attachment #299520 - Flags: ui-review?(beltzner)
Attachment #299520 - Flags: review?(gavin.sharp)
Attachment #295241 - Flags: ui-review?(beltzner)
Whiteboard: [ie parity][has patch] → [ie parity][has patch][needs ui-review beltzner][needs review gavin]

Comment 46

10 years ago
+    // 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.
Created attachment 301103 [details] [diff] [review]
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.
Attachment #299520 - Attachment is obsolete: true
Attachment #301103 - Flags: ui-review?(beltzner)
Attachment #301103 - Flags: review?(gavin.sharp)
Attachment #299520 - Flags: ui-review?(beltzner)
Attachment #299520 - Flags: review?(gavin.sharp)
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4

Comment 48

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

Right.  Done.
Whiteboard: [ie parity][has patch][needs ui-review beltzner][needs review gavin] → [parity-IE][has patch][needs ui-review beltzner][needs review gavin]
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...
Attachment #301103 - Flags: review?(gavin.sharp) → review?(mano)
Whiteboard: [parity-IE][has patch][needs ui-review beltzner][needs review gavin] → [parity-IE][has patch][needs ui-review beltzner][needs review mano]
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.
Attachment #301103 - Flags: review?(mano) → review+
Whiteboard: [parity-IE][has patch][needs ui-review beltzner][needs review mano] → [parity-IE][has patch][needs ui-review beltzner]
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 on attachment 301103 [details] [diff] [review]
Patch (v2.1) (checked in)

ui-r+a=beltzner, make sure to mark it late-l10n
Attachment #301103 - Flags: ui-review?(beltzner)
Attachment #301103 - Flags: ui-review+
Attachment #301103 - Flags: approval1.9+
Flags: blocking-firefox2-
Flags: blocking-aviary1.5-
Flags: blocking-aviary1.0-
It's not late-l10n, as the strings landed before string freeze.
Keywords: checkin-needed
Whiteboard: [parity-IE][has patch][needs ui-review beltzner] → [parity-IE]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #301103 - Attachment description: Patch (v2.1) → Patch (v2.1) (checked in)

Comment 56

10 years ago
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
(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.
Created attachment 303275 [details] [diff] [review]
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.
Attachment #303275 - Flags: superreview?(jonas)
Attachment #303275 - Flags: review?(jonas)
Attachment #303275 - Flags: review?(gavin.sharp)
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.
Depends on: 417545
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
Attachment #303275 - Attachment is obsolete: true
Attachment #303275 - Flags: superreview?(jonas)
Attachment #303275 - Flags: review?(jonas)
Attachment #303275 - Flags: review?(gavin.sharp)
Flags: in-litmus?

Updated

10 years ago
Summary: Need "Show Image" / "Reload Image" on context menu → Add "Show Image" to context menu of not-successfully-loaded images

Comment 61

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

10 years ago
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).

Updated

10 years ago
Duplicate of this bug: 422600

Comment 64

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

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

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

9 years ago
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).
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!

Updated

9 years ago
Depends on: 436483

Comment 71

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

Updated

9 years ago
Depends on: 439133
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.)
Flags: in-testsuite?
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.
Flags: in-litmus? → in-litmus-
Blocks: 715399

Updated

3 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.