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

VERIFIED FIXED in mozilla1.0

Status

()

Core
DOM: Core & HTML
P2
major
VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: sujay, Assigned: Stuart Parmenter)

Tracking

({regression})

Trunk
mozilla1.0
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [eapp], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

17 years ago
yes,  display in the small preview window in the dialog...

Comment 3

17 years ago
Clarifying Summary.
Summary: image doesn't inline in image props dialog → Preview image doesn't display in image props dialog

Comment 4

17 years ago
This was broken very recently!
Assignee: brade → cmanske
Target Milestone: --- → mozilla0.9.1

Comment 5

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

Comment 6

17 years ago
Thanks, Jessica.

Comment 7

17 years ago
Created attachment 31490 [details]
Simple page to demonstrate if "GetNaturalWidth" and "GetNaturalHeight" are working

Comment 8

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

Comment 9

17 years ago
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)
(Assignee)

Comment 10

17 years ago
oh, yeah.  getcomplete isn't right.. i've got a patch for that somewhere around
here.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

Comment 15

17 years ago
pavlov, waiting for your reply to: Since I have a JS solution for the original 
problem, should we bother to investigate this any more?
(Assignee)

Comment 16

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

Comment 18

17 years ago
we should be sending the info sooner than the old imagelib was...

Comment 19

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

Comment 20

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

Comment 21

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

Comment 23

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

Comment 25

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

Updated

17 years ago
Depends on: 81210

Updated

17 years ago
Keywords: correctness, nsCatFood, regression
(Reporter)

Comment 26

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

Comment 27

17 years ago
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.
Pavlov and I came up with the attached patch, Pavlov said he'll get it checked
in so reassigning.
Assignee: jst → pavlov
(Assignee)

Updated

17 years ago
Keywords: patch

Comment 30

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

Comment 31

17 years ago
I filed bug 82435 on the new issue, so this bug may be closed with the current
patch.
r=cmanske

Comment 32

17 years ago
Pav: can you supply sr?
Whiteboard: FIX IN HAND need SR=
(Assignee)

Comment 33

17 years ago
nope, get jst to sr= it

Comment 35

17 years ago
I don't think jst can sr his own fix, can he?
Nope

Updated

17 years ago
Blocks: 82435
(Assignee)

Comment 37

17 years ago
this patch needs to be revisted.  it will break mouseovers since it no longer 
sets mFailureReplace.
Whiteboard: FIX IN HAND need SR=

Comment 38

17 years ago
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
(Assignee)

Updated

17 years ago
Priority: -- → P2

Comment 39

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

Updated

17 years ago
Blocks: 72922

Updated

17 years ago
No longer blocks: 78351

Comment 40

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

Updated

17 years ago
No longer blocks: 72922

Comment 41

17 years ago
-> 0.9.4 per Pavlov
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Attachment #36116 - Flags: needs-work+
(Assignee)

Comment 42

17 years ago
.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.8

Comment 43

16 years ago
Adding nsbeta1 keyword to all regressions so they *get some love* and attention.
Keywords: nsbeta1
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

16 years ago
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+
(Assignee)

Comment 45

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

Comment 46

16 years ago
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
(Assignee)

Comment 47

16 years ago
Created attachment 69558 [details] [diff] [review]
fix

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+
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0
(Assignee)

Comment 49

16 years ago
can someone please r= this?

Comment 50

16 years ago
Comment on attachment 69558 [details] [diff] [review]
fix

r=cmanske
Attachment #69558 - Flags: review+
(Assignee)

Updated

16 years ago
Attachment #35780 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #36116 - Attachment is obsolete: true

Updated

16 years ago
Keywords: nsbeta1+

Updated

16 years ago
Keywords: nsbeta1
(Assignee)

Comment 52

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 53

16 years ago
verfied fixed
Status: RESOLVED → VERIFIED

Updated

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