Closed Bug 76177 Opened 24 years ago Closed 23 years ago

Cannot change "SRC" attribute on an image element (<img>)

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: sujay, Assigned: pavlov)

References

(
URL
)

Details

(Keywords: regression, Whiteboard: [eapp])

Attachments

(2 files, 2 obsolete files)

Simple page to demonstrate if "GetNaturalWidth" and "GetNaturalHeight" are working
(deleted), text/html
Details
fix
(deleted), patch
cmanske
: review+
jst
: superreview+
shaver
: approval+
Details | Diff | Splinter Review
regression using 4/16 build 1) launch netscape 2) launch composer 3) insert image 4) choose image(use image above in URL or any image) now notice it doesn't inline the image in the image props window. although I do see it flash briefly. this is a regression.
What is "inline the image"? Display in the small preview window in the dialog? Could this be related to Note that the XUL and JS for the preview feature in Image Dialog has not changed in quite some time, but new imagelib code has been causing new problems.
yes, display in the small preview window in the dialog...
Clarifying Summary.
Summary: image doesn't inline in image props dialog → Preview image doesn't display in image props dialog
This was broken very recently!
Assignee: brade → cmanske
Target Milestone: --- → mozilla0.9.1
The preview image in the Image Properties dialog worked in a 4-14(7am) build, but then didn't show up in a 4-15(6am) build. I used OS/2 builds to find when the preview image got lost.
Thanks, Jessica.
It's hard to pin down exactly what's happening, but in the Image Properties Dialog, we are getting "0" values for "naturalWidth" and "naturalHeight", which is why the preview image doesn't show up. The attached test page seems to work, however, so it isn't simple bustage in nsHTMLImageElement methods as I had hoped. Tracing into nsHTMLImageElement ::GetNaturalWidth(), I noticed that nsHTMLImageElement::GetImageFrame returns a null frame. Note that we use the following JS to wait for the completion of image loading before trying to get the 'natural' width and height. // Get the origin width from the image or setup timer to get later if (previewImage.complete) { GetActualSize(); } else { // Start timer to poll until image is loaded //dump("*** Starting timer to get natural image size...\n"); timeoutId = window.setInterval("GetActualSize()", interval); intervalSum = 0; } ... function GetActualSize() { if (intervalSum > intervalLimit) { dump(" Timeout trying to load preview image\n"); CancelTimer(); return; } if (!previewImage) { CancelTimer(); } else { if (previewImage.complete) { // Image loading has completed -- we can get actual width ... Maybe something is wrong with nsHTMLImageElement::GetComplete()?
Assignee: cmanske → pavlov
Severity: normal → major
Changing summary and component
Component: Editor → ImageLib
Summary: Preview image doesn't display in image props dialog → Failure to get "naturalWidth" and "naturalHeigh" (Preview image doesn't display in image props dialog)
oh, yeah. getcomplete isn't right.. i've got a patch for that somewhere around here.
Status: NEW → ASSIGNED
ok, getcomplete isn't the problem... i'm not entirely sure what is but I have a few questions: first, why don't you just create an img frame inside the inital document? second, you should be able to attach an onload handler to it to avoid the timer stuff. I don't know enough about the js dom stuff you are doing to tell anything else. your first call to nsHTMLImageElement::GetComplete, I assume from: 292 // Get the origin width from the image or setup timer to get later 293 if (previewImage.complete) 294 GetActualSize(); does get the right frame, but the rest of the calls that come from your timer get back a frame of type "InlineFrame" which seems to be a nsHTMLContainerFrame. I don't think this is a problem with imagelib
Status: ASSIGNED → NEW
Create an img frame? This is in a dialog, and we need a real HTML DOM Image element so we can call GetComplete, GetNaturalWidth, GetNaturalHeight (via JS attributes). I did notice the "InlineFrame" as well and was confused about that. I'd like to use an onload handler -- can point me to an example of doing that in JS?
in the xul you have <html .../> couldn't you have <html ...><img src="about:blank"></html> or somesuch? I don't really know. I don't know where nay examples of doing onload on an <img>, but i'm told it can be done.
Yes, I see what you mean -- I tried that when I first wrote it but I had to remove the element and create/append a new one because if the image URL ever failed to load, any further attempts to set "src" would fail. I'll experiment with that again to see if it's still true with your new imagelib code. Note that simply putting an "onload" attribute on the <img> does work fine! I now can get the correct naturalWidth and naturalHeight. What I did with the ".complete" timers still seems valid, however, and did work before, so there is a real problem lurking ( probably in layout?) Since I have a JS solution for the original problem, should we bother to investigate this any more? If no, reassign back to me.
pavlov, waiting for your reply to: Since I have a JS solution for the original problem, should we bother to investigate this any more?
if there is any further investigation, it would probably need to go over to the DOM guys like jst :-)
Is libpr0n maybe not feeding us the image size information as quickly as the old imagelib did, or something? FWIW, you could use image.onload too instead of spinning with a timer and checkin for image.complete...
we should be sending the info sooner than the old imagelib was...
Terri Preston: could you please attach your "JS solution"? I worked on using an "onload" handler also but I had problems making it work. I'd like to see if your's is different. The "onload" wouldn't fire when you changed the "src" attribute, even if a new <img> element was created and reappended to the <html> element in the XUL. It still seems like the basic libimg "oncomplete" and 'GetNaturalWidth/Height' should work correctly, though.
looking at the image element code, there is stuff in there that won't allow you to change the src= more than once.. over to jst.... should we remove this? there is a comment about 4.x compatibility as i recall.
Assignee: pavlov → jst
Component: ImageLib → DOM HTML
QA Contact: sujay → desale
*Please* let us change the 'src' on an img element > once! You might also look into bug 72922, which is related: Composer needs to be able to reload an image from the 'src' url after user changes it (e.g., with an image editor.) We need a DOM interface way to do this.
I'm fine with making the change to let you change image.src more than once and still preloading files (if the image is displayed there's no issue here, it's only if it's done with new Image() from JS). I don't have any time to work on this right now tho, let me know how urgent this is.
Urgency is a "must have" for 0.9.1, with some lead time to make sure it works as it should for Composer's Image Properties dialog preview window and (more importantly) to get the natural width and height info for the image. Adding bug to for the Composer side of this work.
Blocks: 78351
Moving bugs that do not have "important" keywords or fixes in hand to the next milestone.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
This is a description of what I've done in editor's EdImageProperties.xul and .js code in connection with this problem (see bug 78351). Using XUL like this: <html id="preview-image-holder"> <html:img id="preview-image" onload="PreviewImageLoaded();"/> </html> for the HTML preview img, the onload the onload handler *is* called the very first time you choose a file. But it is not called after you change the SRC to a different URL. Note that changing the SRC *does* load the new image, which wasn't working before the latest XPCDOM landing. I also tried creating a new <img> element, setting the "onload" like this: image.onload = "PreviewImageLoaded();"; then deleting existing img, and appending the new one at runtime. But that didn't help in triggering the onload firing. In fact, when that was used, the onload handler wasn't called at all. So the main issue here seems to be that the "onload" method isn't being called after changing the "src" on the <img> element.
Depends on: 81210
Keywords: correctness, nsCatFood, regression
okay in today's 5/22 commercial build, I see an image in the preview area, but its still cropped, and not optimizing that space like it used to... go back to old bulds to see what it used to look like.
This is current state: The first image you select via the "Choose" button is loaded correctly, and we get the correct natural width and height. The image in the Preview box is scaled if it exceeds the fixed-size box, but the actual width and height are reported. After OK, the image in the document displays correctly. The remaining problem the same as orginally reported here: Before using OK, if you select another image using "Choose" button or typing an new filename, the image displays in the preview box, but it uses the previous images width and height. That is because the "onload" method is not called when the "SRC" attribute is changed for the preview image.
Attached patch Proposed fix. (obsolete) (deleted) — — Splinter Review
Pavlov and I came up with the attached patch, Pavlov said he'll get it checked in so reassigning.
Assignee: jst → pavlov
Keywords: patch
Unfortunately, we are not "out of the woods" quite yet. Now still have a problem -- probably cache-related? Use Composer and click on Image button to bring up dialog. Click on "Choose File" button. (If it isn't visible, Cancel dialog and bring it up again -- that's a known XPFE problem.) Select a local image file -- it displays in preview box and its "natural width and height" are reported next to the image. Click on "Choose File" button again and select a different image. Again, the image displays correctly and shows the width and height. Here's the bug: If you select an image that you've already selected before, The "onload" handler is called, but "naturalWidth" and "naturalHeight" are returned as "0" for both. So it seems that a cached image is failing to return its size. This might be related to bug 7019. Since the problem described in current summary has been fixed, we probably should file a new bug for this new problem?
I filed bug 82435 on the new issue, so this bug may be closed with the current patch. r=cmanske
Pav: can you supply sr?
Whiteboard: FIX IN HAND need SR=
nope, get jst to sr= it
Attached patch Better fix (obsolete) (deleted) — — Splinter Review
I don't think jst can sr his own fix, can he?
Nope
Blocks: 82435
this patch needs to be revisted. it will break mouseovers since it no longer sets mFailureReplace.
Whiteboard: FIX IN HAND need SR=
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
Priority: -- → P2
Changing summary to better describe what is being fixed in this bug. The "natural width and height" issue is covered by bug 82435.
Summary: Failure to get "naturalWidth" and "naturalHeigh" (Preview image doesn't display in image props dialog) → Cannot change "SRC" attribute on an image element (<img>)
Blocks: 72922
No longer blocks: 78351
pushing out. 0.9.2 is done. (querying for this string will get you the list of the 0.9.2 bugs I moved to 0.9.3)
Target Milestone: mozilla0.9.2 → mozilla0.9.3
No longer blocks: 72922
-> 0.9.4 per Pavlov
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Attachment #36116 - Flags: needs-work+
.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.8
Adding nsbeta1 keyword to all regressions so they *get some love* and attention.
Keywords: nsbeta1
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Whiteboard: [eapp]
Major corporations depend on eapp bugs, and need them to be fixed before they can recommend Mozilla-based products to their customers. Adding nsbeta1+ keyword and making sure the bugs get re-evaluated if they are targeted beyond 1.0.
Keywords: nsbeta1 → nsbeta1+
I don't currently see the problem of the wrong naturalwidth/height. i currently only see the first onload, which is what jst's patch fixes. it seems to have bitrotted though, so we probably need something slightly more up-to-date
eapp was incorrectly used to change this to nsbeta1+. Resetting to nsbeta1 to nominate. This is an important issue and deserves to be nsbeta1+ by the ADT.
Keywords: nsbeta1+ → nsbeta1
Attached patch fix (deleted) — — Splinter Review
here's an updated fix that will cause the onload's to get fired
Comment on attachment 69558 [details] [diff] [review] fix sr=jst
Attachment #69558 - Flags: superreview+
Target Milestone: mozilla0.9.9 → mozilla1.0
can someone please r= this?
Comment on attachment 69558 [details] [diff] [review] fix r=cmanske
Attachment #69558 - Flags: review+
Attachment #35780 - Attachment is obsolete: true
Attachment #36116 - Attachment is obsolete: true
Keywords: nsbeta1+
Keywords: nsbeta1
checked in
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
verfied fixed
Status: RESOLVED → VERIFIED
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: