1. Start Composer and insert a valid image, or edit any page with a valid image. 2. Double click on the image to launch Image Properties dialog. 3. Alter the text in the "Image Location" input field so the image name is now wrong. (You will see the small preview image disappear when the image is no longer correct -- the preview image always clears the image cache so it always correct.) 4. Click ok to close the dialog. Expected: The image in the page should now show the broken image icon. Actual: The previous image is still displayed. Note that in a debug window, you will see the message "Error reading file..." (output from nsFileTransport.cpp, line 807), so the image in the page is attempting to reload. If you save the file to local disk, close the window, and reopen the file, the image will show as the broken image icon. It seems there's a failure to trigger redrawing of the image in the page.
Severity: normal → major
Hardware: PC → All
Reassigning to Alex
Assignee: attinasi → alexsavulov
Bug 134986 might be related. This problem is very important for Composer. Editing with images that become broken is very confusing right now. I will help debug it as soon as I can, but we'd appreciate someone more expert in layout/image code to look as well.
not enough rights to access bug 134986!!!
That's weird! Brade didn't think it had any connection anyway.
Created attachment 87401 [details] [diff] [review] workaround patch This is the workaround patch in Composer that we should use if this isn't fixed in time in the layout code. It detects when we fail to load the image into the preview window in the image properties dialog. For just that case, it reinserts the image to trigger redisplaying it using the "broken" icon.
Daniel: Could you please help investigating this?
Although Charley's patch is fine for users who will encounter this problem in 1 scenario, it is incomplete in that users might encounter this problem after saving or publishing (in error conditions). It also doesn't help browser users (I believe Kevin said that this would likely affect JS in browser which changes an image's src also). Daniel--can you help investigate why this is broken?
cmanske: are you working on this? if yes, feel free to take it. i have others too, so i'm covered for a while. :-) thx!
This bug has a much wider effect than only on Composer... Create a simple html document with a local image, view it in browser, open document inspector and change the src attribute of the image so it contains a wrong path after a correct one. Same effect. Will investigate tomorrow (11:30pm here...)
Alexandru: As Daniel and Kathy point out, this is a wider issue and my patch is in no way a good fix. It is only there as a last-ditch hack since I don't think we should ship with the current state. I'm not familiar enough with the image/layout/css code to find the real problem. Daniel: thanks for helping.
adding 'rtm' to elevate importance to RTM release
Whiteboard: EDITORBASE+ [adt2] → EDITORBASE+ [adt2 rtm]
Created attachment 87670 [details] [diff] [review] test case showing that same problem appears in browser and affects rollovers and dynamically changed images
Created attachment 87904 [details] test case with good mime type...
Attachment #87670 - Attachment is obsolete: true
i think i figured out why we don't display the broken image icon. i need some time to find out if i can fix it. the problem lays in the nsresult imgRequest::NotifyProxyListener method that does not send a proxy->OnDataAvailable(frame, &r); so that the region gets invalidated and repainted how it should be. the fix for it might be tricky. working on it...
ah, not "send" but "call" is the right word. nice tescases anyway and a very good catch. is definitively not editor but layout. thanks daniel.
*** Bug 126486 has been marked as a duplicate of this bug. ***
Created attachment 88089 [details] [diff] [review] le patch. is correcting the behaviour by setting the mLoads.mRequest = null the patch changes the behaviour of the image frame in nsImageFrame::OnStopDecode in case that the load failed. it does reset both mLoad elements to get a broken icon from the Paint and sets imageFailedToLoad to invalidate correctly. need r=/sr= por favor
Attachment #87401 - Attachment is obsolete: true
after applying the patch we run into bug 22820. i think i know why that bug happens. we ned to find a way to change the inline frame back to an image frame when this happens. we need some work on the frame manager i think.
ah forgot to mention: bug 22820 only occurs when no size is specified for the img element
Created attachment 88099 [details] [diff] [review] correct patch (the previous one had some garbage in it) see comment for the previous patch
Attachment #88089 - Attachment is obsolete: true
Comment on attachment 88099 [details] [diff] [review] correct patch (the previous one had some garbage in it) r=glazman Tested on win2k and RH72. Thanks a lot Alex !!!
Attachment #88099 - Flags: review+
I tested this as well. Thanks, this is great.
BTW, bug 22820 doesn't affect changing image SRC in Composer. There's special code to always show broken image in Composer, and we *always* write out the width and height attributes for an image when changing anything using the Image dialog.
Comment on attachment 88099 [details] [diff] [review] correct patch (the previous one had some garbage in it) sr=waterson
Attachment #88099 - Flags: superreview+
Pls land this on the trunk, and have QA verify the fix. thanks!
oops forgot to mark it fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: EDITORBASE+ [adt2 rtm] → EDITORBASE+ [adt2 rtm] [FIXED ON TRUNK]
*** Bug 112183 has been marked as a duplicate of this bug. ***
it would be nice if QA could verify this soon so we can check it into the branch since it affects the editor too.
I've tested this over and over during the past 3 day's builds. Works great. I tested both in Composer and with the attached test case, "test case showing that same problem appears in browser..."
Status: RESOLVED → VERIFIED
I am also able to Verify that this has been fixed on the 06-20 trunk build.
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check this in asap, then add the "fixed1.0.1" keyword.
Keywords: adt1.0.1 → adt1.0.1+
Whiteboard: EDITORBASE+ [adt2 rtm] [FIXED ON TRUNK] → EDITORBASE+ [adt2 rtm] [FIXED ON TRUNK] [ETA 06/24]
The patch for this bug seems to have caused bug 154218 Can anybody verify?
OK, after backing out this patch, bug 154218 is fixed. So it looks like the fix for this bug definately caused that bug to appear.
do not backout this patch yet!!! see discussion on the new reported bug 154218
verified using branch build 20020731 on winxp , adding KW:verified1.0.1
You need to log in before you can comment on or make changes to this bug.