Images that are "broken" during document editing don't redisplay using broken image icon.

VERIFIED FIXED in mozilla1.0.1



16 years ago
16 years ago


(Reporter: Charles Manske, Assigned: Alexandru Savulov)



Windows 2000

Firefox Tracking Flags

(Not tracked)


(Whiteboard: EDITORBASE+ [adt2 rtm] [FIXED ON TRUNK] [ETA 06/24])


(2 attachments, 3 obsolete attachments)



16 years ago
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
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.


16 years ago
Severity: normal → major
Keywords: nsbeta1+
Hardware: PC → All
Whiteboard: EDITORBASE+


16 years ago
Target Milestone: --- → mozilla1.0.1
Reassigning to Alex
Assignee: attinasi → alexsavulov


16 years ago
QA Contact: petersen → moied


16 years ago
Whiteboard: EDITORBASE+ → EDITORBASE+ [adt2]


16 years ago
Keywords: testcase
Priority: -- → P2

Comment 2

16 years ago
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.

Comment 3

16 years ago
not enough rights to access bug 134986!!!

Comment 4

16 years ago
That's weird! Brade didn't think it had any connection anyway.

Comment 5

16 years ago
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.

Comment 6

16 years ago
Daniel: Could you please help investigating this?

Comment 7

16 years ago
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?

Comment 8

16 years ago

are you working on this? if yes, feel free to take it. i have others too, so i'm
covered for a while. :-)

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...)

Comment 10

16 years ago
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.

Comment 11

16 years ago
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

Comment 14

16 years ago
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...

Comment 15

16 years ago
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.

Comment 16

16 years ago
*** Bug 126486 has been marked as a duplicate of this bug. ***

Comment 17

16 years ago
Created attachment 88089 [details] [diff] [review]
le patch. is correcting the behaviour by setting the mLoads[0].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


16 years ago
Attachment #87401 - Attachment is obsolete: true

Comment 18

16 years ago
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.

Comment 19

16 years ago
ah forgot to mention: bug 22820 only occurs when no size is specified for the
img element

Comment 20

16 years ago
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)


Tested on win2k and RH72.
Thanks a lot Alex !!!
Attachment #88099 - Flags: review+

Comment 22

16 years ago
I tested this as well. Thanks, this is great.

Comment 23

16 years ago
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

Comment 24

16 years ago
Comment on attachment 88099 [details] [diff] [review]
correct patch (the previous one had some garbage in it)

Attachment #88099 - Flags: superreview+


16 years ago
Keywords: adt1.0.1, mozilla1.0.1

Comment 25

16 years ago
Pls land this on the trunk, and have QA verify the fix. thanks!

Comment 26

16 years ago
oops forgot to mark it fixed
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: EDITORBASE+ [adt2 rtm] → EDITORBASE+ [adt2 rtm] [FIXED ON TRUNK]

Comment 27

16 years ago
*** Bug 112183 has been marked as a duplicate of this bug. ***

Comment 28

16 years ago
it would be nice if QA could verify this soon so we can check it into the branch
since it affects the editor too.

Comment 29

16 years ago
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..."

Comment 30

16 years ago
I am also able to Verify that this has been fixed on the 06-20 trunk build.

Comment 31

16 years ago
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+

Comment 32

16 years ago
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]

Comment 33

16 years ago
The patch for this bug seems to have caused bug 154218
Can anybody verify?

Comment 34

16 years ago
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.

Comment 35

16 years ago
do not backout this patch yet!!! see discussion on the new reported bug 154218


16 years ago
Keywords: mozilla1.0.1+ → fixed1.0.1

Comment 36

16 years ago
verified using branch build 20020731 on winxp , adding KW:verified1.0.1
Keywords: verified1.0.1
You need to log in before you can comment on or make changes to this bug.