Closed
Bug 47475
Opened 25 years ago
Closed 18 years ago
Need "Show(View) Image" / "Reload Image" on context menu
Categories
(Core :: Graphics: ImageLib, enhancement, P3)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: inoue, Assigned: csthomas)
References
(Blocks 1 open bug)
Details
Attachments
(6 files, 3 obsolete files)
6.68 KB,
application/octet-stream
|
Details | |
10.44 KB,
application/zip
|
Details | |
23.91 KB,
patch
|
Details | Diff | Splinter Review | |
42.42 KB,
patch
|
Details | Diff | Splinter Review | |
65.19 KB,
patch
|
Details | Diff | Splinter Review | |
16.14 KB,
patch
|
bzbarsky
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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•25 years ago
|
||
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•25 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•25 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
Comment 4•25 years ago
|
||
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•25 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.
Comment 7•25 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•24 years ago
|
Target Milestone: Future → M25
Updated•24 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 → ---
Updated•24 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]
nav triage team:
Nice to have, but not a beta1 stopper, marking nsbeta1-
Keywords: nsbeta1-
Comment 10•24 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•24 years ago
|
Whiteboard: [y]
Comment 11•24 years ago
|
||
Comment 12•24 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•24 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.
Updated•24 years ago
|
Assignee: pnunn → pavlov
Comment 14•24 years ago
|
||
->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•24 years ago
|
||
Igor, could you please do as sairuh requests. Thanks.
Comment 16•24 years ago
|
||
Comment 17•24 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.
Comment 18•24 years ago
|
||
pavlov [or someone else], could you pls review the attached patch? need to save
as zip, then unzip 'em...
Comment 19•24 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•24 years ago
|
||
Comment 21•24 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•24 years ago
|
||
Comment 23•24 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•24 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•24 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•24 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•24 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•24 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•24 years ago
|
||
Comment 30•24 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•24 years ago
|
Target Milestone: --- → Future
Comment 31•24 years ago
|
||
pavlov: Is anybody going to review and check in the patch atteched by Andrew
Brygin?
Comment 32•24 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•24 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 ?
Comment 34•23 years ago
|
||
*** Bug 121786 has been marked as a duplicate of this bug. ***
Comment 35•23 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.
Comment 36•23 years ago
|
||
*** Bug 140811 has been marked as a duplicate of this bug. ***
Comment 37•23 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•23 years ago
|
||
*** Bug 155384 has been marked as a duplicate of this bug. ***
Comment 39•23 years ago
|
||
*** Bug 35130 has been marked as a duplicate of this bug. ***
Comment 40•23 years ago
|
||
*** Bug 161786 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Keywords: mozilla0.9.9 → patch
![]() |
||
Comment 41•22 years ago
|
||
*** Bug 101058 has been marked as a duplicate of this bug. ***
Comment 42•22 years ago
|
||
*** Bug 175712 has been marked as a duplicate of this bug. ***
Comment 43•22 years ago
|
||
*** Bug 47792 has been marked as a duplicate of this bug. ***
Comment 44•22 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•22 years ago
|
||
*** Bug 176357 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Summary: Need "show-image" item on right-click menu similar to 4.x → Need "Show Image" item on context menu
Comment 46•22 years ago
|
||
Anyone else think bug 185518 should be duped to this?
Comment 47•22 years ago
|
||
*** Bug 185518 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Summary: Need "Show Image" item on context menu → Need "Show Image" / "Reload Image" on context menu
Comment 48•22 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
![]() |
||
Comment 49•22 years ago
|
||
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•22 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 52•22 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
Comment 53•22 years ago
|
||
*** Bug 209262 has been marked as a duplicate of this bug. ***
Comment 54•22 years ago
|
||
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•22 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
Comment 56•22 years ago
|
||
Eh, I don't think that we need an entire submenu for image-related commands
Comment 57•22 years ago
|
||
Maybe :) but Reload command for loaded images seems to be more handly that View
command.
Comment 58•22 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•21 years ago
|
||
Isn't this fixed? Suggest to mark as RESOLVED.
Comment 60•21 years ago
|
||
>Isn't this fixed?
no.
Comment 61•21 years ago
|
||
*** Bug 220910 has been marked as a duplicate of this bug. ***
Comment 62•21 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•21 years ago
|
||
an important bug being fixed? dream on
Comment 64•21 years ago
|
||
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•21 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.
Comment 66•21 years ago
|
||
well after that comment, my inclination to fix this bug is no longer existing.
Comment 67•21 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•21 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•21 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•21 years ago
|
||
*** Bug 232041 has been marked as a duplicate of this bug. ***
Comment 71•21 years ago
|
||
*** Bug 237334 has been marked as a duplicate of this bug. ***
Comment 72•21 years ago
|
||
*** Bug 252594 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 73•21 years ago
|
||
*** Bug 255600 has been marked as a duplicate of this bug. ***
Comment 74•20 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•20 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.
Comment 76•20 years ago
|
||
*** Bug 268063 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Summary: Need "Show Image" / "Reload Image" on context menu → Need "Show(View) Image" / "Reload Image" on context menu
![]() |
||
Comment 77•20 years ago
|
||
*** Bug 283776 has been marked as a duplicate of this bug. ***
Comment 78•20 years ago
|
||
*** Bug 284006 has been marked as a duplicate of this bug. ***
Comment 79•20 years ago
|
||
Similar bug?:
https://bugzilla.mozilla.org/show_bug.cgi?id=218142
Comment 80•20 years ago
|
||
Ack, I'm an idiot...Bugzilla needs an edit button/link.
Comment 81•20 years ago
|
||
*** Bug 285952 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Flags: blocking-aviary1.1?
Comment 82•20 years ago
|
||
The firefox bug depending on this was already minused.
Flags: blocking-aviary1.1?
Assignee | ||
Comment 83•19 years ago
|
||
Assignee: pavlov → cst
Status: NEW → ASSIGNED
Attachment #196930 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196930 -
Flags: review?(cbiesinger)
Comment 84•19 years ago
|
||
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-
Assignee | ||
Updated•19 years ago
|
Attachment #196930 -
Attachment is obsolete: true
Attachment #196930 -
Flags: superreview?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 85•19 years ago
|
||
Attachment #197106 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #197106 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•19 years ago
|
Keywords: helpwanted
Whiteboard: [cst: r? sr?]
Assignee | ||
Updated•19 years ago
|
Attachment #197106 -
Flags: review?(bzbarsky)
![]() |
||
Comment 86•19 years ago
|
||
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 87•19 years ago
|
||
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•19 years ago
|
||
(I also failed to notice the "Componenets" typo :)
Comment 89•19 years ago
|
||
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•19 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•19 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.
Assignee | ||
Updated•19 years ago
|
Whiteboard: [cst: r? sr?] → [cst: r? sr?] [cst: COMPONENENTS]
Comment 92•19 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•19 years ago
|
||
Is there any chance to see this bug resolved in MoFo/SeaMonkey Trunk versions?
Updated•19 years ago
|
Flags: blocking-aviary2?
Assignee | ||
Updated•19 years ago
|
Whiteboard: [cst: r? sr?] [cst: COMPONENENTS] → [cst: what key doesnt conflict?, COMPONENENTS, r+ sr+]
Assignee | ||
Comment 94•19 years ago
|
||
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•19 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+]
Comment 96•19 years ago
|
||
It wasn't a typo :)
Whiteboard: [cst: what key doesn't conflict?, COMPONENTS, r+ sr+] → [cst: what key doesn't conflict?, Componenets, r+ sr+]
Comment 97•19 years ago
|
||
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•19 years ago
|
||
Question about patch: will Shift + "Reload Image" do forced reloading (similar as Shift+Reload for whole page)?
Assignee | ||
Comment 99•19 years ago
|
||
(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•19 years ago
|
||
*** Bug 323416 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Attachment #197106 -
Flags: approval1.8.1? → branch-1.8.1?(neil)
Comment 101•19 years ago
|
||
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-
Comment 102•19 years ago
|
||
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-
Assignee | ||
Comment 103•19 years ago
|
||
Neil, what key should I use?
Comment 104•19 years ago
|
||
(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.
Assignee | ||
Comment 105•19 years ago
|
||
Attachment #197106 -
Attachment is obsolete: true
Attachment #233526 -
Flags: superreview?(neil)
Attachment #233526 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 106•19 years ago
|
||
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?]
Assignee | ||
Comment 107•19 years ago
|
||
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 108•19 years ago
|
||
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 109•19 years ago
|
||
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+
Assignee | ||
Comment 110•19 years ago
|
||
(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/.
Assignee | ||
Comment 111•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Whiteboard: [cst: r? sr?]
Target Milestone: Future → mozilla1.9alpha
Assignee | ||
Comment 112•18 years ago
|
||
Resolving based on comment 111 (apparently I forgot to click the button?)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 113•18 years ago
|
||
This didn't actually implement 'Show image', does it? (forceReload doesn't work with images turned off.)
Comment 114•18 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?
Assignee | ||
Comment 115•18 years ago
|
||
(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•18 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.
Comment 117•18 years ago
|
||
That's now part of SeaMonkey. File a new bug for Firefox I guess.
Comment 118•18 years ago
|
||
This isn't fixed in SM 1.1.1, menu item doesn't appear.
You need to log in
before you can comment on or make changes to this bug.
Description
•