Closed Bug 76177 Opened 23 years ago Closed 22 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
675 bytes, text/html
Details
fix
1.40 KB, 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) — — 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)
        — 
          — 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
        — 
          — 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: 22 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: