9796 - Viewing bad images gives bad feedback

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Eli Goldberg]

* TITLE/SUMMARY
Bad PNG displayed as a single black pixel, rather than file name

[broken out of bug #924]

* STEPS TO REPRODUCE
0) Launch Apprunner
1) View http://www.cdrom.com/pub/png/PngSuite/xlfn0g04.png (an invalid PNG file)

* RESULT
 - What happened

A single black pixel appears.

 - What was expected

Image file name (per bug #1994)

* REGRESSION

 - Occurs On
        Mac OS Apprunner (7.13.99 AM optimized build)
        Linux Apprunner (7.13.99 AM optimized build)
        Win32 Apprunner (7.13.99 AM optimized build [NT 4, Service Pack 3])


* CONFIGURATIONS TESTED

- [Mac] Power Mac 8500/120 (233 Mhz 604e), 64 MB RAM (VM on; 1 MB of VM used),
1024x768 (Thousands of Colors), Mac OS 8.6

- [Win32] Vectra VL (233 Mhz P2), 96 MB RAM, 800x600 (True Color), NT 4.0 SP3.

- [Linux] Vectra VL (266 Mhz P2), 96 MB RAM.

Comment 1

[Greg Roelofs]

This is not specific to PNGs; broken GIFs and JPEGs do the same thing.  (I
tested by truncating a known good example of each to 8 bytes; you get the same
triangular dot in both cases.)  Tested with viewer only, using a build mostly
dating from 20 June.

As noted above, it's not a single pixel (at least under Unix); it's six pixels,
five of which are gray-176 and one of which is black.  They're arranged as a
stack of four on the left and a stack of two (centered vertically) on the right.
The black one is the lower of the two right pixels.  I.e.:

	G
	GG
	GB
	G

I always assumed that was the intended "broken image" image for Mozilla.

Comment 2

[Eli Goldberg]

[Thanks for doing the additional testing that I should have done, were I not
running out the door to catch a bus while writing the bug report. ;-]

Comment 3

[Hixie (not reading bugmail)]

What should happen is that when you are viewing an image, as opposed to an
HTML, TXT or XML document, and it turns out to be broken, some feedback should
be provided to inform you of the problem.

Currently, only the console informs us of any problems.

This is not an alt text bug, BTW. Alt text shouldn't be drawn here.

Comment 4

[Eli Goldberg]

Ian,

To clarify, is it your opinion that the browser should provide specific feedback
when a bad image is encountered (as opposed to an image that the browser simply
cannot load), or is this stated at any point in a specification that you could
cite?

Thanks.

Comment 5

[Hixie (not reading bugmail)]

Oops, sorry, my message wasn't very clear... :-(

This bug related to the specific instance of viewing a graphic directly, that
is, by typing the URI to a graphic image into the location bar (or clicking a
link directly to an image, not to an image contained in a wrapper HTML doc).

Two scenarios can come up when this happens:
  1. The file doesn't exist: A 404 is returned, and nothing special occurs,
     since an image's 404 is indistinguishable from an HTML or XML's 404.
  2. The file is broken/invalid: The viewer should say so. Currently it just
     displays six pixels, as described above. _This_ is the problem.

Comment 6

[Eli Goldberg]

Sure.

In this case, the bad image returns bad feedback both in the scenario in which
it's loaded directly, or in which it's loaded as part of the page. (quick test at
http://www.prometheus-music.com/gecko/badpng.html, in this case, a single pixel.)

In the case in which it's loaded as part of a web page, are you suggesting that
the user feedback should differ from that of any other invalid/unloadable image,
or are we exclusively referring to the scenario in which the image is loaded
independently of a web page?

Thanks!

Comment 7

[Hixie (not reading bugmail)]

The picture-in-a-file issue should be covered by bug 1995 -- I don't think bug
1995 has looked at the issue of _invalid_ pictures (as opposed to missing
pictures) though, so you may wish to investigate further. (In brief: missing
or broken pictures should result in the alt text being used instead.)

User feedback of broken and missing images that are part of web pages should
be given as per bug 6211.

This bug should refer, IMHO, exclusively to the case of the graphic being loaded
directly, independently of a web page.

Comment 8

[Greg Roelofs]

This is fixed, I think by the changes Pamela made for 9336.  Tested with viewer
under Linux on the referenced PNG image and two broken GIF and JPEG images.

Greg

Comment 9

[Hixie (not reading bugmail)]

The current feedback is not appropriate.

NOTE. This bug is exclusively about the case where no XML or HTML document is
involved and the image is viewed directly! e.g., by entering an image's URI into
the 'location' field. The cases of broken/missing images in documents are all
covered by bug 1995!

We now display the URL of the image. Presumably this is caused by the "alt text"
behaviour of a broken <img> element in HTML. We should instead be telling the
user that the image is broken (either by an icon or by a message). That is to
say, the broken/missing-image handling of <img>s in HTML docs SHOULD NOT be
involved when showing images directly.

Just giving the URI of the image, as now, is not useful in any way. It would be
much better if the image code was to say what was wrong with the image - for
example, "the image is missing its palette section". This information should
be the same as that given on the errors/information page (see bug 6211) when a
broken image is present in an HTML/XML+CSS document.

I am reopening this bug.

[BTW, if anyone has some broken JPEGs, GIFs or PNGs, could they attach them to
the bug report? Thanks. That will aid with testing. Don't forget to specify the
correct MIME type...]

Comment 10

[Eli Goldberg]

ASCII and ye shall receive. Pam has a whole collection of 'em, and I'll attach a
few.

Comment 11

[Eli Goldberg]

Attachment: Bad GIF image (corrupted frame)

Comment 12

[Eli Goldberg]

Hmm...the only other examples I can find crash today's build. ;)

Greg probably has a few invalid PNG images that he may be interested in
appending.

Comment 13

[Hixie (not reading bugmail)]

elig: I hope you filed bugs for those that crash our builds... ;-)

Comment 14

[Eli Goldberg]

Yup. ;) Better yet, I think Pam is fixing them, too. ;)

Comment 15

[pnunn]

sorry gang. Providing 'bad image' diagnostics is low
on the priority scale. Is more of a wishlist feature than
a bug.
-pn

Comment 16

[Hixie (not reading bugmail)]

As agreed with Beth, assigning myself as QA contact for all 'alt text' bugs...

Comment 17

[Hixie (not reading bugmail)]

Fair enough; in the meantime, could we just replace the filename with this?:

   "Mozilla could not display this image as the file is corrupted."

The current feedback is not just sub-ideal, it is plain wrong. We are displaying
the full path to the file, and not saying that there is a problem. (Well, unless
you count the diagnostic message printed on the console.)

pn: Internally, how do you get layout to draw the image? Is it by creating a
virtual HTML document with just an <img> tag?

Comment 18

[Hixie (not reading bugmail)]

(Moving milestone back to M11, as this should definitely be fixed.)

Comment 19

[Eli Goldberg]

Adding German to CC: list, as I believe this is a UE-relevant issue.

I'm not aware of any precedent of including error messages as textual web content
within Mozilla or its forefathers.

I also don't understand the argument for presenting a different error response to
the user when a bad image is displayed independently of a web page, as opposed to
when it's displayed as part of a web page.

It strikes me as proposing inconsistent application behavior, in that it requires
the user to learn different application responses for the same problem (of a bad
image) depending on the mode (HTML page with images versus just an image).

I agree that providing the URL for broken images is completely useless --- just
as it is when presented on pages with content and images [as it requires users to
understand that images are specified by URLs, and/or that they have such as a
thing as "ALT text" in order to understand that the image is broken.]

Comment 20

[Hixie (not reading bugmail)]

Eli:
> I'm not aware of any precedent of including error messages as textual web
> content within Mozilla or its forefathers.

We do it for broken XML pages.


> I also don't understand the argument for presenting a different error
> response to the user when a bad image is displayed independently of a web
> page, as opposed to when it's displayed as part of a web page.

If the image is displayed as part of a web page and is broken, _it_is_not_the_
problem_of_the_reader_. The reader wants to know if the _page_ is broken
(because then Mozilla can't display it), but if an image on the page has a
problem, then that is the problem of the _author_, not the reader.

HTML has a mechanism to cope specifically with the case of an image being broken
or unavailable: alternative text.

However, when the reader wants to view an image directly, then _that_ *is* the
document. And so if it is broken, the reader wants to know, because they are
expecting to see an image, and if it's broken, we can't give them one.

So if we cannot display an image, then the least we can do is say so.


> It strikes me as proposing inconsistent application behavior, in that it
> requires the user to learn different application responses for the same
> problem (of a bad image) depending on the mode (HTML page with images versus
> just an image).

No, because if an image is on the page, then the reader is not trying to see the
image, they are trying to see the page. So if the browser can substitute some
alternative, such as the content of the 'alt' attribute, then that is adequate.
The meaning is not lost.

Another way of looking at this is: the two problems you list are _not_ the same
problem *to the user* (although internally they may well be). The equivalent
problem of a broken image viewed directly is a broken XML page viewed directly.
And our response should be equivalent.


> I agree that providing the URL for broken images is completely useless ---
> just as it is when presented on pages with content and images [...].

Agreed. And on a web page, we never display the full URI of the image if the
image is broken. (We may occasionally, as a last ditch effort, display the file
_name_, minus extension, as that is sometimes more useful than nothing at all.
This only happens if the author of the HTML page has made an error and omitted
to give alt text -- for decorative images, alt="" should be used.)


> ...it requires users to understand that images are specified by URLs, and/or
> that they have such as a thing as "ALT text" in order to understand that the
> image is broken.

In the case of an inline image in a document, there is no need for readers to
even be aware of the fact that an image was expected, let alone that it was
broken. So there is no need for them to learn about URIs and alt text.


Pam:
Internally, how do you get layout to draw the image? Is it by creating a
virtual HTML document with just an <img> tag? If this is the case, then you need
only change what you are providing as the alt attribute to instead read:

   Mozilla could not display this image as the file is corrupted.

And that should address the issue adequately enough for 5.0. (For 5.1 we will
want to have the better feedback, of course. See also bug 6211.)

Comment 21

[pnunn]

Due to workload, target changed.

Comment 22

[Hixie (not reading bugmail)]

Pam: I cannot verify this LATER. This is a _bug_, and it really must be fixed
before final release. Shall we mark it REMIND instead?

Comment 23

[Hixie (not reading bugmail)]

Reopening. See comments dated 1999-09-01.

Comment 24

[pnunn]

sure, ian. You want to take this one?
-p

Comment 25

[pnunn]

here you go.

Comment 26

[Hixie (not reading bugmail)]

Pavlov: This bug is probably yours.

This bug is related to the specific instance of viewing a graphic directly, that
is, by typing the URI to a graphic image into the location bar (or clicking a
link directly to an image, not to an image contained in a wrapper HTML doc).

Two bad scenarios can come up when this happens:
  1. The file doesn't exist: A 404 is returned, and nothing special occurs,
     since an image's 404 is indistinguishable from an HTML or XML's 404.
  2. The file is broken/invalid: The viewer should say so. Currently it just
     displays six pixels, as described above. _This_ is the problem.

What should happen is that when you are viewing an image, as opposed to an
HTML, TXT or XML document, and it turns out to be broken, some feedback should
be provided to inform you of the problem.

Currently, only the console informs us of any problems.

(User feedback of broken and missing images that are part of web pages should
be given as per bug 6211. See also bug 1995.)

Comment 27

[Stuart Parmenter]

well, since the broken image is technically (in our system) a html image frame, 
it will reduce to 0x0, since there isn't any alt text.  perhaps we should set 
alt text to the filename if no alt text is specified.. *shrug*

Comment 28

[timeless]

nc4 had a broken image icon, could you paint that instead?

Comment 29

[Stuart Parmenter]

no, i have to paint the alt text.. or at least that is what hixie told me :-)

Comment 30

[timeless]

i'm confused, i read this part of hixie's comment:
> This bug is related to the specific instance of viewing a graphic directly,
> that is, by typing the URI to a graphic image into the location bar 
if you do this, there is no html, and there can't be any alt text.

if i missed something, then just ignore my comments. -- sorry

Comment 31

[Hixie (not reading bugmail)]

What should happen is that when you are viewing an image, as opposed to an
HTML, TXT or XML document, and it turns out to be broken, some feedback should
be provided to inform you of the problem.

Something like (inline, instead of the image):

    /\  This image is not valid.
   /__\ URI: http://www.pavlov.net/pr0n/catherine/00055.jpeg
        Content-Type: image/jpeg

        Error: The JPEG header was missing the JPEG signature. This is probably 
        not a JPEG image.

Of course, mpt could give you better text.

Comment 32

[Hixie (not reading bugmail)]

Pavlov: btw, you can probably implement that by using <object> instead of <img>
and by putting the error message in the <object> element's alternate content.
You'd have to poke the error message into the document's DOM if you detected
an error, dunno how hard that would be. Also note that when doing things like 
this, you absolutely have to make sure you ignore the user's "show images" 
pref... clearly you know how to do this already since you currently ignore the
said pref for all the ^%$#(*^(#@*$ images... :-P

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

the URL from this bug is not valid anymore. Can somebody give another one?

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

Taking

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: draft patch

ok, this is the easiest way to do it. The displayed message looks like this
with the patch:
The image is not valid and could therefore not be loaded

I would have put URL & content type in there, but newlines didn't work.

Comments? (I know, the text needs to be made localizable)

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: draft patch

ok, this is the easiest way to do it. The displayed message looks like this
with the patch:
The image is not valid and could therefore not be loaded

I would have put URL & content type in there, but newlines didn't work.

Comments? (I know, the text needs to be made localizable)

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch, localizable string

(hm, why did the previous patch appear twice?)

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch with better wording

now uses the wording suggested by mpt ("The image http://foo/bar cannot be
displayed, because it is corrupted.").

Comment 39

[Hixie (not reading bugmail)]

Did you consider using an <object> element as suggested in comment 31?

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

How would the <object> approach be better?

Comment 41

[Hixie (not reading bugmail)]

You could got an icon, a link, multiline text, etc, as shown in comment 31.

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

ok; <object> can not be used because of bugs in Mozilla (bug 96976, bug 127963).

I will file a new bug for converting this code to use <object> after they are
resolved.

In the meantime - pavlov, could you review this patch?

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch with even better wording

ok, finally timeless and mpt could agree on a wording, which I'm now using.

Also putting the XPIDLString and attribute setting into the if block

Comment 44

[timeless]

Comment on attachment 76407 [details] [diff] [review]
patch with even better wording

code: r=timeless
property: r=mpt, r=timeless

Comment 45

[tor]

What's wrong with the current broken icon and URL alt text (besides not
invalidating/repainting the region when the error happens, which this
patch doesn't address)?

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

tor: nothing, except that this is not for images in webpages (which have an alt
text), but when an image itself is displayed. In that case, we currently show
the URL of the file, which is not very helpful.

Comment 47

[tor]

Would it be hard to get the broken icon added in that case?

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

tor: actually... I wonder why it isn't there? The code for displaying such
images basically creates an HTML Document that looks like this:
<html>
 <body>
   <p>
    <img src="foo" alt="the error message">
   </p>
  </body>
</html>

so the image icon should really be there. strange...

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

tor: however, when <object> will be used (see comment 42), this can easily be fixed.

could you super-review?

Comment 50

[tor]

Comment on attachment 76407 [details] [diff] [review]
patch with even better wording

sr=tor

Comment 51

[Asa Dotzler [:asa]]

Comment on attachment 76407 [details] [diff] [review]
patch with even better wording

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

filed bug 133933 for using <object>

Comment 53

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 54

[Josh Birnbaum]

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