Bug 3869 - Should use HTML Image element inplace of JS Image object
Summary: Should use HTML Image element inplace of JS Image object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Enhancement
Assignee: Anders Carlsson
URL: http://docs.sun.com/source/816-6408-1...
Keywords: HasReduction, InRadar
: 5367 (view as bug list)
Depends on:
Blocks: 6519
  Show dependency treegraph
 
Reported: 2005-07-06 03:13 PDT by Samrod
Modified: 2006-01-21 16:03 PST (History)
2 users (show)

See Also:


Attachments
Implement image.complete (5.04 KB, patch)
2005-07-12 12:38 PDT, Anders Carlsson
mjs: review-
Details | Formatted Diff | Diff
Use HTMLImageElementImpl instead of Image (11.01 KB, patch)
2005-08-04 09:06 PDT, Anders Carlsson
darin: review-
Details | Formatted Diff | Diff
one more test (470 bytes, text/html)
2005-08-09 17:54 PDT, Maks Orlovich
no flags Details
updated cut at the patch, but still doesn't address use of Image::info (12.68 KB, patch)
2005-08-14 00:12 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (19.45 KB, patch)
2006-01-20 15:39 PST, Anders Carlsson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Samrod 2005-07-06 03:13:34 PDT
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.
Comment 1 Mark Rowe (bdash) 2005-07-06 13:56:59 PDT
This property is supported by both Internet Explorer and Mozilla.
Comment 2 Anders Carlsson 2005-07-12 12:38:51 PDT
Created attachment 2931 [details]
Implement image.complete

This patch implements image.complete in the same way as Image.complete (the
javascript image object) works.
Comment 3 Maciej Stachowiak 2005-07-13 23:57:46 PDT
Comment on attachment 2931 [details]
Implement image.complete

r=me
Comment 4 Maciej Stachowiak 2005-07-13 23:58:45 PDT
Comment on attachment 2931 [details]
Implement image.complete

oh, wait, almost forgot - this needs a layout test
Comment 5 Vicki Murley 2005-07-14 00:53:19 PDT
I think this is also in Radar as <rdar://problem/3852987>
Comment 6 Guillaume Outters 2005-07-15 05:22:03 PDT
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?
Comment 7 Anders Carlsson 2005-08-04 09:06:20 PDT
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
Comment 8 Darin Adler 2005-08-04 11:26:23 PDT
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.
Comment 9 Maks Orlovich 2005-08-05 08:00:23 PDT
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)...  
  
   
Comment 10 Maks Orlovich 2005-08-09 17:54:07 PDT
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)
Comment 11 Darin Adler 2005-08-14 00:09:04 PDT
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.
Comment 12 Darin Adler 2005-08-14 00:12:40 PDT
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.
Comment 13 Anders Carlsson 2006-01-16 15:25:57 PST
*** Bug 5367 has been marked as a duplicate of this bug. ***
Comment 14 Anders Carlsson 2006-01-20 15:39:58 PST
Created attachment 5803 [details]
Patch
Comment 15 Darin Adler 2006-01-21 10:59:30 PST
Comment on attachment 5803 [details]
Patch

r=me
Comment 16 Maciej Stachowiak 2006-01-21 13:15:55 PST
The patch seems to include some layout test results but not the actual tests. Make sure those get committed when landing please.