Closed Bug 78351 Opened 24 years ago Closed 24 years ago

Image Properties dialog should show a preview image and get the natural width and height of images

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.3

People

(Reporter: cmanske, Assigned: cmanske)

References

Details

(Whiteboard: [dialog]DEPENDS FIX IN HAND, checked in, waiting for dependent fix to clear)

Attachments

(6 files)

Patch to use "onload" handler on an HTML <img> tag in XUL for preview image and getting natural width and height
4.70 KB, patch
Details | Diff | Splinter Review
Use "_moz-bypass-cache" to force loading of preview image from source.
2.48 KB, patch
Details | Diff | Splinter Review
Patch to fix some image dialog problems and prepare for fix to 82435
3.74 KB, patch
Details | Diff | Splinter Review
Patch to use new "removeEntry" method that fixes bug 82435.
4.73 KB, patch
Details | Diff | Splinter Review
Updated patch: made changes suggested by kin
2.68 KB, patch
Details | Diff | Splinter Review
Updated patch: Fixes lots of problems and removes dependencies on other bugs.
6.19 KB, patch
Details | Diff | Splinter Review
Currently, the small preview image in the Image dialog uses JS timers and tests for "img.complete" to detect when image is loaded. We can then get the "natural" width and height of the image. This depends on being able to set the "src" attribute on a single img element more than once, causing it to trigger an image load (bug 76177).
Depends on: 76177
Didn't quite complete the description: The work to do is replace the current timer code with an "onload" handler instead.
Severity: normal → major
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
The editor works is done, but won't work until bug 76177 is fixed.
Whiteboard: DEPENDS
moving to the same milestone as the depends bug
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Changing to using an "onload" handler is done and checked in. In today's build, setting the image does show up in the preview window. But that handler isn't getting called at all, so we still aren't getting the natural width and height correctly. I tried using XUL like this: <html id="preview-image-holder"> <html:img id="preview-image" onload="PreviewImageLoaded();"/> </html> for the HTML preview img I also triedcreating 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 at all in triggering the onload firing.
Patch cleans up old "timer" strategy for preview image and uses "onload" handler. These changes are needed, but complete fix still depends on fixing bug 76177. On 5/14, the preview image loaded correctly and the natural width and height were correct after selecting an image for the first time. If you then select another image, the preview image changes, but the onload handler is not fired again, thus the natural width and height is not updated for the new image selected.
Keywords: patch, review
Whiteboard: DEPENDS → DEPENDS FIX IN HAND need r= sr=
-var gPreviewImage; +//var gPreviewImage; Just delete it. Same here: +dialog.ChooseFile = document.getElementById( "ChooseFile" ); +dump(" ***** ChooseFile button: "+dialog.ChooseFile+"\n"); + sr=sfraser on the rest.
Depends on: 81210
The other debug dump() calls in the patch should also be removed before checkin. r=kin@netscape.com
Ooops! All extra junk deleted. Was trying to debug the "missing Choose button" problem.
Whiteboard: DEPENDS FIX IN HAND need r= sr= → DEPENDS FIX IN HAND
Editor work checked in. Still blocked by dependent bugs.
Keywords: correctness
Priority: -- → P3
Whiteboard: DEPENDS FIX IN HAND → [dialog]DEPENDS FIX IN HAND
Depends on: 82435
Patch on 76177 fixes that problem, but there's still a problem getting naturalWidth and naturalHeight for images already loaded (new bug 82435).
Whiteboard: [dialog]DEPENDS FIX IN HAND → [dialog]DEPENDS
Changing summary to turn this bug into a tracking bug for preview image and getting natural width and height issues.
Summary: Use "onload" handler to get preview image in Image Properties dialog → Image Properties dialog should show a preview image and get the natural width and height of images
Patch from 5/26/01 adds "_moz-bypass-cache" attribute to EdImageProps.xul, part of fix to dependent bug 82435. Additional changes in EdImageProps.js protects existing "width" and "height" attributes on <img> tag when preview image fails to load (e.g., when SRC is a remote URL and user is offline.)
Whiteboard: [dialog]DEPENDS → [dialog]DEPENDS FIX IN HAND, need r=, sr=
If the ImgLib work required as suggested by JST in bug 82435 isn't done, then I suggest we use the "_moz-bypass-cache" strategy at least for an interim fix.
No longer depends on: 81210
Changes in th 6/13/01 patch do the following: 1. Saves the existing image src URL, so if we can't load the image and thus don't get the natural width and height, we don't remove the existing HTML "width" and "height" attributes. 2. Adds the "_moz-bypass-cache" attribute to the <img> tag in the dialog, so if we do decide to use my suggested fix to bug 82435, this attribute is all ready. If we don't do that fix, having an attribute that is ignore won't affect anything. I won't be around from 6/15 through 7/2 to follow up on the dependent bug issues, which is why I'd like to do these editor changes now.
We're making progress! nsIImageCache now has a new method to remove a URL from the image cache, fixing bug 82435. So the patch on 6/13/01 19:36 now uses that as well as doing the "save original url and don't trash width and height when image doesn't load" fix described in last comment. Unfortunately, there's a new problem we discovered: After designating the Image SRC once (either typing a URL or selecting an image with the "Choose File" button), the size from that first image "sticks" for the preview image. I.e., any image you type or select after that will display in the preview window with the first image's size. Note that the actual (natural) width and height are reported correctly, but the nsImageFrame code doesn't redraw the image correctly in the dialog. A new bug will be filed on that issue. So this should wrap up the editor changes need for this dialog.
Filed new bug 85911 for the image size problem described above.
Depends on: 85911
Whiteboard: [dialog]DEPENDS FIX IN HAND, need r=, sr= → [dialog]DEPENDS FIX IN HAND, need r=, sr=, a=
sr=kin@netscape.com with the changes we discussed over aim: 1. Move all the cache uri setup code into the try/catch block. 2. Add a comment bracketing this cache code describing why it's neccessary.
Blocks: 72922
Whiteboard: [dialog]DEPENDS FIX IN HAND, need r=, sr=, a= → [dialog]DEPENDS FIX IN HAND, need r=, a=
r=brade
Whiteboard: [dialog]DEPENDS FIX IN HAND, need r=, a= → [dialog]DEPENDS FIX IN HAND, need a=
a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)
Blocks: 83989
Depends on: 85981
After thorough testing, I'm adding some changes to the patch to fix lots of problems. This actually goes back to a strategy we used before: delete the existing preview image element and create a new one. This removes the dependency on bug 76177 and bug 85911. Mostly, it also solves a bad problem: as soon as user entered a URL that didn't exist, it killed the preview image. I.e., any further attempt to set the SRC to a valid URL failed. This is what is new in the 06/14/01 16:24 patch relative to the previous one that was already reviewed and approved: + if(dialog.PreviewImage) + removeEventListener("load", PreviewImageLoaded, true); + + if (dialog.ImageHolder.firstChild) + dialog.ImageHolder.removeChild(dialog.ImageHolder.firstChild); + + dialog.PreviewImage = document.createElementNS("http://www.w3.org/1999/xhtml", "html:img"); + if(dialog.PreviewImage) + { + dialog.ImageHolder.appendChild(dialog.PreviewImage); + dialog.PreviewImage.addEventListener("load",PreviewImageLoaded,true); + dialog.PreviewImage.src = imageSrc;
No longer depends on: 76177, 85911
Whiteboard: [dialog]DEPENDS FIX IN HAND, need a= → [dialog]DEPENDS FIX IN HAND, need r=, sr=, a= for updated patch
Looks good. sr=sfraser
r=blake
a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)
Ok, last patch checked in. Just waiting for fix to bug 82435 to mark this fixed.
Keywords: patch, review
Whiteboard: [dialog]DEPENDS FIX IN HAND, need r=, sr=, a= for updated patch → [dialog]DEPENDS FIX IN HAND, checked in, waiting for dependent fix to clear
move to 0.9.3 since the fix is checked in
because this fix is checked in and we're just waiting for a dependent bug to be fixed, moving to 0.9.3 milestone
OS: Windows NT → All
Hardware: PC → All
Target Milestone: mozilla0.9.2 → mozilla0.9.3
wow -- this works great, marking fixed tested using the build from June 26, 2001 trunk build on win98
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified in 9-03 build
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: