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

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Charles Manske]

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.

Comment 1

[Kevin McCluskey (gone)]

Reassigning to Alex

Comment 2

[Charles Manske]

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

[Alexandru Savulov]

not enough rights to access bug 134986!!!

Comment 4

[Charles Manske]

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

Comment 5

[Charles Manske]

Attachment: 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

[Charles Manske]

Daniel: Could you please help investigating this?

Comment 7

[Kathleen :Brade]

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

[Alexandru Savulov]

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!

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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

[Charles Manske]

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

[Syd Logan]

adding 'rtm' to elevate importance to RTM release

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: test case showing that same problem appears in browser and affects rollovers and dynamically changed images

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: test case with good mime type...

Comment 14

[Alexandru Savulov]

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

[Alexandru Savulov]

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

[Charles Manske]

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

Comment 17

[Alexandru Savulov]

Attachment: 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

Comment 18

[Alexandru Savulov]

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

[Alexandru Savulov]

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

Comment 20

[Alexandru Savulov]

Attachment: correct patch (the previous one had some garbage in it)

see comment for the previous patch

Comment 21

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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 !!!

Comment 22

[Charles Manske]

I tested this as well. Thanks, this is great.

Comment 23

[Charles Manske]

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 24

[Chris Waterson]

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

sr=waterson

Comment 25

[Jaime Rodriguez, Jr.]

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

Comment 26

[Alexandru Savulov]

oops forgot to mark it fixed

Comment 27

[Marc Boullet]

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

Comment 28

[Alexandru Savulov]

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

[Charles Manske]

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

[michael wendell]

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

Comment 31

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 32

[Jaime Rodriguez, Jr.]

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.

Comment 33

[WD]

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

Comment 34

[WD]

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

[Alexandru Savulov]

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

Comment 36

[Moied]

verified using branch build 20020731 on winxp , adding KW:verified1.0.1