Bug 3869 – Should use HTML Image element inplace of JS Image object

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Samrod]

The URL provides documentation rather than an example.

The .complete image property always returns "undefined" regardless of image load
status.  Not sure how else to check image load status in WebKit/Safari.

[Mark Rowe (bdash)]

This property is supported by both Internet Explorer and Mozilla.

[Anders Carlsson]

Created attachment 2931 [details]
Implement image.complete

This patch implements image.complete in the same way as Image.complete (the
javascript image object) works.

[Maciej Stachowiak]

Comment on attachment 2931 [details]
Implement image.complete

r=me

[Maciej Stachowiak]

Comment on attachment 2931 [details]
Implement image.complete

oh, wait, almost forgot - this needs a layout test

[Vicki Murley]

I think this is also in Radar as <rdar://problem/3852987>

[Guillaume Outters]

Shouldn't a lot more code be shared between img HTML elements and JavaScript Image objects? While 
trying to do some king of image preloader, I found those pretty annoying differences:

- the 'complete' property as explained in this bug.
- if you give a JS Image object an onload function, the 'this' in this function will not be the Image, 
instead it is the container Window. On the other hand, the onload of a document.createElement('img') 
gets the correct 'this'.

Last difference: after the image is loaded, the JS one gets a width and a height, while the DOM one gets 
none; I believe this could be considered an expected behavior (although different from Firefox), as for 
any DOM object not inserted in the rendering tree (temporary workaround for us JavaScript coders: on 
the DOM img onload, create a JS Image, set its src to this.src, and obtain its width and height. Quite 
ugly, isn't it?).

I haven't already looked at the code, so I'm not sure if this is the right thing to do, but I think letting 
this as one bug (as opposed to creating one for each difference, particularly for the 'this' problem) 
reflects the fact that JS and DOM should be unified, not patched separately. Severity increase anyone?

[Anders Carlsson]

Created attachment 3225 [details]
Use HTMLImageElementImpl instead of Image

Looking at Opera, Mozilla and MacIE, they all seem to create a HTML img element
instead of a custom Image object. Here's a patch that makes height and width
work better for img elements and replaces the custom Image object with
HTMLImageElementImpl

[Darin Adler]

Comment on attachment 3225 [details]
Use HTMLImageElementImpl instead of Image

This is a change that has huge implications -- do we perhaps need more than
just the one layout test?

Looks great to me, r=me.

[Maks Orlovich]

http://bugs.kde.org/show_bug.cgi?id=64702   
http://bugs.kde.org/show_bug.cgi?id=77085  
http://bugs.kde.org/show_bug.cgi?id=98461  
   
have some potential things that should be fixed by this switch; so might be a  
good source for testcases, and illustrate why it's overall a right thing.  
Anyway, thanks for doing this, I am probably gonna merge this patch (after  
some prerequisites are in)...  
  
   

[Maks Orlovich]

Created attachment 3305 [details]
one more test

Here is a test (khtmltests/regression/tests/ecma/image2.html) based on the
first 2 bugs linked above. This one clearly demonstrates why Image needs to be
a DOM element. 

(the third bug is broken due to ERROR_EVENT vs. KHTML_ERROR_EVENT weirdness)

[Darin Adler]

Comment on attachment 3225 [details]
Use HTMLImageElementImpl instead of Image

This patch doesn't compile.

There are references to Image::info in kjs_events.cpp and kjs_html.cpp that
need to be converted to the new scheme.

[Darin Adler]

Created attachment 3372 [details]
updated cut at the patch, but still doesn't address use of Image::info

This is the patch as it stands right now in my tip of tree (I'm about to roll
it out), in case that helps finishing the job.

[Anders Carlsson]

*** Bug 5367 has been marked as a duplicate of this bug. ***

[Anders Carlsson]

Created attachment 5803 [details]
Patch

[Darin Adler]

Comment on attachment 5803 [details]
Patch

r=me

[Maciej Stachowiak]

The patch seems to include some layout test results but not the actual tests. Make sure those get committed when landing please.