177747 - Don't show "Alternate text" under Element Properties and Page Info for standalone images

Status: VERIFIED WORKSFORME

(SeaMonkey :: Page Info, defect)

Description

[Hanspeter Niederstrasser]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2b) Gecko/20021031
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2b) Gecko/20021031

When an image is viewed directly within the browser, selecting "Properties" from
the context menu gives the above error message for the "Alternate text"
property.  This is incorrect, as the image displays correctly in the browser and
does not have errors.

Reproducible: Always

Steps to Reproduce:
1. Load an image into the browser.  Can either be through a remote URL or drag
and drop from a local file.
2. Open the context menu on the image and select "Properties"
3. Look at the text given for "Alternate text."

Actual Results:  
"Alternate text" says 'The image [image_name] cannot be displayed, because it
contains errors.'

Expected Results:  
Image Properties should either not show the entire Alternate Text line or it
should leave the value blank.

An image viewed by itself does not have any alt-text, since that is a property
that gets assigned by a web page.  Therefore, the displayed value for the
Alternate Text property should be blank.

This should not be a dupe of Bug 121084 [cache: Images requested twice leads to
error] because 1) the images are shown without any problems in the browser and
2) this bug also appears when viewing local images and should not affect the
cache.  This is reproducible with all cache settings under Advanced Preferences.

Finally, the properties for the image from _within_ a web page behave as
expected.  Alternate Text shows the correct alt-text.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, that _is_ the alt text for that image.  This way _if_ the image were
to have errors and not be displayable the alt text would be displayed instead.

Granted, that's an implementation detail, and we should probably not be showing
it in properties....

Comment 2

[Todd Cohn]

Confirming, changing summary a bit.  It's not the "Alternative Text" we need
changed but the fact we show it in the properties window in this case. 

Comment 3

[Torben]

Sending to ImageLib, since I need help assigning a particular imaging bug, and
they'll accept it in ImageLib and reassign it for me :-) Also changing HW/OS to
All/All, this happens on windows too (build 2002110708).

Comment 4

[Hixie (not reading bugmail)]

Eventually we'll fix this by using <object> instead of <img>, so this will
become a non-issue, so let's not worry about it...

Comment 5

[Christopher Hoess (gone)]

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

Comment 6

[Chris Lyon]

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

Comment 7

[Felix Miata]

Why does properties show a filename fragment instead of a complete filename as
the "alt" text?

Comment 8

[Hixie (not reading bugmail)]

it shows the alternate text attribute, not the filename. They may be similar on
certain pages...

Comment 9

[Felix Miata]

I was referring to an image file loaded directly from local storage, as the
summary describes. How can it have any attributes besides name, size, timestamp,
etc? I thought an alt attribute exists only when assigned by markup code, which
is bypassed using a file picker to load an image file.

Comment 10

[Kevin Stange]

I see this as well, go load an image directly in the browser, right click it and
hit properties.  The Alternate text attribute should be blank!

Also, if you drag the image off to the desktop, for example, you'll get a
shortcut with the filename: The image [image_name] cannot be displayed, because it
contains errors..url

Comment 11

[Hixie (not reading bugmail)]

Oh, you are just repeating the bug report.

Yes, we know.

Thanks for confirming it though. :-)

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Jo Hermans]

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

Comment 14

[Matthias Versen [:Matti]]

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

Comment 15

[Dave Edwards]

Could we change the summary from "Don't show "Alternate text" under properties
for images loaded directly" to "Don't show "Alternate text" under element
properties for images loaded directly"? I tried searching for this bug using
"element properties" and ended up submitting a duplicate.

Comment 16

[Torben]

Summary changed as requested in comment 15.

Comment 17

[Aaron Leventhal]

Changing summary back. It's not okay to not show the alt text. We use the ALT
text for screen reader accessibility.

Comment 18

[Aaron Leventhal]

Nevermind, I'm going to deal with screen reader stuff on my end. I'll have to
circumvent this fake alt text by detecting the image document.

Comment 19

[Jonathan Wakely]

aaronl, if a user loads http://example.org/ting.gif how do you plan to get an
ALT text from it, for accessibility, or any other admirable purpose ?

Why has this bug's summary changed from the remarkably clear "don't do X when Y
is true" to "X is broken" ?

This bug has nothing to do with accessibility, or ALT text for that matter. The
element properties window is wrong to try to show "Alternate text".

Comment 20

[Hanspeter Niederstrasser]

As the reporter, I'm changing the summary back to the description of the problem.  

This bug is not about hiding the ALT text from browsers (graphical/screen
readers/etc).  This bug is about incorrectly showing an internal Mozilla
ALT-text string (used for when a directly loaded image is broken) in the
"Element Properties" dialog (see comment #1).

Comment 21

[Brian 'netdragon' Bober]

Putting URL http://www.mozilla.org/images/mozilla-banner.gif as an example.

"Not applicable" would be adequate. Can Page Info just detect somehow that the
nsIImageDocument is being used and use "Not applicable" instead of the real ALT
string?

Comment 22

[Brian 'netdragon' Bober]

* Changing "images loaded directly" to "standalone images" which is what
developers use.
* Adding Page Info to the summary.
* Removing WONTFIX?? from status whiteboard
* Sending over to Page Info product. This can be fixed without modification to
ImageLib or nsIImageDocument, afaik using what I'll mention below. Modifying how
standalone images are shown when not found can be done in a seperate bug.
* Changing Severity to Trivial

Implementation Details:

http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/nsContextMenu.js#255

I think this is like what we'll need to do in Page Info and Element Properties:

var documentType = window._content.document.contentType;
if ( documentType.substr(0,6) == "image/" ) {

/* Change the ALT text to "Not applicable for standalone images." or something */

}

Comment 23

[Aaron Kaluszka]

The more proper fix should be the ALT text itself, NOT another hack to get
around that hack...

Comment 24

[Brian 'netdragon' Bober]

Aaron, then the other thing we could do is change the alt text to "Not
Applicable" and modify nsIImageDocument somehow when the picture didn't load,
but I am not sure if there is a way to know if the image didn't load properly
and then we get into a whole lot of work for something so small. The
communication between Imglib and nsIImageDocument is going to be improved
someday anyway, so this isn't really a huge hack, its just a 10 line temporary hack.

Comment 25

[Bill Mason]

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

Comment 26

[Owen Marshall (Not reading bugmail)]

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

Comment 27

[Stefan Borggraefe]

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

Comment 28

[Adam Hauner]

xref: bug bug 135902 - Alternate text and Type has wrong value

Comment 29

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

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

Comment 30

[Matthias Versen [:Matti]]

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

Comment 31

[Frank Wein [:mcsmurf]]

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

Comment 32

[Serge Gautherie (:sgautherie)]

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

Comment 33

[Mattias]

How about applying this special alt-text only if the picture doesn't load? 
Right now, if you drag a picture into a Thunderbird message window, then it is
possible to email it off perfectly fine, except the file name says "blah blah...
cannot be displayed, because it contains errors."  If there is no alt-text then
Thunderbird uses the file name wich looks much better (try the same thing using
Internet Explorer).

Comment 34

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050825 SeaMonkey/1.0a]
(nightly) (W98SE)

Bug 292503 may have partially fixed the current bug:
*Drag and Drop from Windows Explorer: fixed.
*<about:logo>: still broken, but Reload fixes it...
*<http://www.mozilla.org/images/mozilla-banner.gif> (any http/https !!?):
remains broken.

Comment 35

[Phil Ringnalda (:philor)]

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

Comment 36

[Mats Palmgren (inactive)]

Attachment: wip (diff -w)

Like so?

Comment 37

[Mats Palmgren (inactive)]

Attachment: Screenshot, Page Info

Comment 38

[Mats Palmgren (inactive)]

Attachment: Screenshot, Properties

Comment 39

[[e-mail]]

I can't reproduce comment #34; I thought bug 292503 had fixed this.

Comment 40

[Mats Palmgren (inactive)]

(In reply to comment #39)
> I can't reproduce comment #34; I thought bug 292503 had fixed this.
> 

SeaMonkey and Firefox trunk on Linux still says:
Alternate Text: The image “http://www.mozilla.org/projects/minefield/minefield-icon.png” cannot be displayed, because it contains errors.

in Page Info and Properties for a standalone image.

Comment 41

[[e-mail]]

(In reply to comment #40)
>SeaMonkey and Firefox trunk on Linux still says:
>Alternate Text: The image
>“http://www.mozilla.org/projects/minefield/minefield-icon.png” cannot be
>displayed, because it contains errors.
Weird. My linux trunk is OK. ajschult's linux trunk is OK. My Windows 1.8.1a is OK. But my Windows trunk is not ok...

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to comment #41)
> Weird. My linux trunk is OK. ajschult's linux trunk is OK. My Windows 1.8.1a is
> OK. But my Windows trunk is not ok...

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060614 SeaMonkey/1.1a] (nightly) (W98SE)

Bug still there:
1. Open (in a new tab) the link from comment 34.
2. Image context menu > Properties.

Comment 43

[Serge Gautherie (:sgautherie)]

(In reply to comment #36)
> Created an attachment (id=225815) [edit]
> wip (diff -w)
> 
> Like so?

(The idea seems right.)
Nit: I suggest to swap the test and the else order in the <pageInfo.js> file(s).

Comment 44

[Mats Palmgren (inactive)]

Attachment: Patch

Comment 45

[Mats Palmgren (inactive)]

Attachment: Patch (diff -w)

Comment 46

[Serge Gautherie (:sgautherie)]

Comment on attachment 226137 [details] [diff] [review]
Patch

Nits:
once you have review on th -w patch, I suggest to:

>Index: browser/base/content/metaData.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/metaData.js,v
>retrieving revision 1.9
>diff -p -u -1 -2 -r1.9 metaData.js
>--- browser/base/content/metaData.js	2 Jun 2006 04:28:48 -0000	1.9
>+++ browser/base/content/metaData.js	19 Jun 2006 12:37:03 -0000
>@@ -261,25 +261,26 @@ function checkForImage(elem, htmllocalna
>   if (!ismap) {
>-   if (imgType == "img" || imgType == "input") {
>+   if ((imgType == "img" || imgType == "input") &&
>+       !(elem.ownerDocument instanceof ImageDocument)) {
>      setAlt(img);
>    } else {
>      hideNode("image-alt");
>    }
>   }

move the if '{' to the next line,
and add a missing space to this if+else block.

>Index: xpfe/browser/resources/content/metadata.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/metadata.js,v
>retrieving revision 1.24
>diff -p -u -1 -2 -r1.24 metadata.js
>--- xpfe/browser/resources/content/metadata.js	18 Mar 2005 10:38:52 -0000	1.24
>+++ xpfe/browser/resources/content/metadata.js	19 Jun 2006 12:37:04 -0000
>@@ -210,25 +210,26 @@ function checkForImage(elem, htmllocalna
>     if (!ismap) {
>-       if (imgType == "img" || imgType == "input") {
>+       if ((imgType == "img" || imgType == "input") &&
>+           !(elem.ownerDocument instanceof ImageDocument)) {
>            setAlt(img);
>        } else {
>            hideNode("image-alt");
>        }
>     }

move the if '{' to the next line,
and remove the extra spaces from this if+else block, to get a two-space indentation only.

Comment 47

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 48

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3 (diff -w)

Comment 49

[[e-mail]]

I'm sorry but I can't reproduce the "error" text bug on any of my builds today.

Comment 50

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060623 SeaMonkey/1.1a] (nightly) (W98SE)

Sometimes, I have to try more (2+) than one time,
but I can still get the _error_ text with both the <banner.gif> and <about:logo>.

The easiest seems to be to
1. Load the <banner.gif> in the current tab,
2. Context-click to open it in a new tab.

NB: In the cases when I don't get the "error" text, I get the "location" url.

Comment 51

[[e-mail]]

Comment on attachment 226182 [details] [diff] [review]
Patch rev. 3 (diff -w)

When imagelib finds the image is already in the image cache it cancels the original load request with an error code (<biesi> it can't use a success code <biesi> necko won't let it). Unfortunately this prevents the image document from clearing the ALT text, so we need to tell the image document that NS_IMAGELIB_ERROR_LOAD_ABORTED is really a success code.

Comment 52

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4 (CHECKED IN: comment 57)

(In reply to comment #51)
> ... so we need to tell the image document that
> NS_IMAGELIB_ERROR_LOAD_ABORTED is really a success code.

Something like this? (The Alt. text with this patch is
"http://www.mozilla.org/projects/minefield/minefield-icon.png")

What about other errors, e.g. a corrupt image would still have Alt. text in
Properties/PageInfo for a image document with this patch.
As I see it, a standalone image doesn't conceptually have Alt. text because
that is an attribute only for markup documents, so Properties/PageInfo
should not list Alt. text at all. So, I prefer the last patch (rev. 3).

Comment 53

[Serge Gautherie (:sgautherie)]

As I understand it,
'rev 3' would fix the initial report,
'rev 4' would be an extra (welcomed) fix.

Comment 54

[[e-mail]]

Comment on attachment 227029 [details] [diff] [review]
Patch rev. 4 (CHECKED IN: comment 57)

I'm not a content peer; please find another reviewer.

Note: if you don't think successful image documents should have alt text then you could submit a patch to remove line 580.

Comment 55

[[e-mail]]

(In reply to comment #54)
>if you don't think successful image documents should have alt text then
>you could submit a patch to remove line 580.
Ah, it seems that the alt text is necessary for Select All/Copy to pick it up.

With this fix I no longer have an issue with the alt text in page info or properties, but if you are still unhappy then I suggest you continue the discussion with the module owner.

Comment 56

[Mats Palmgren (inactive)]

Comment on attachment 227029 [details] [diff] [review]
Patch rev. 4 (CHECKED IN: comment 57)

See comment 51, comment 52. This fixes the bogus Alt. text in
Properties/PageInfo. I think we should remove it from the UI completely
(for image docs) eventually, but that's a different patch.

Comment 57

[Mats Palmgren (inactive)]

Comment on attachment 227029 [details] [diff] [review]
Patch rev. 4 (CHECKED IN: comment 57)

Checked in at 2006-06-28 19:42 PDT.

Leaving the bug open to remove the Alt.text field in Properties/PageInfo.

Comment 58

[Serge Gautherie (:sgautherie)]

Looking forward to get this first patch into 1.8 branch...

Comment 59

[Daniel Brooks [:db48x]]

Comment on attachment 227029 [details] [diff] [review]
Patch rev. 4 (CHECKED IN: comment 57)

in that case, set the flag :)

Comment 60

[Darin Fisher]

Comment on attachment 227029 [details] [diff] [review]
Patch rev. 4 (CHECKED IN: comment 57)

Not comfortable making a chnage to nsImageDocument for FF2 beta1 to fix a minor UI issue.

It also seems to me that the correct fix would be to make ImageLib not report an error when it successfully sends data to its consumer.  The current patch appears to be a bandaid for bad ImageLib behavior :-(

Comment 61

[[e-mail]]

(In reply to comment #60)
>It also seems to me that the correct fix would be to make ImageLib not report
>an error when it successfully sends data to its consumer.  The current patch
>appears to be a bandaid for bad ImageLib behavior :-(
I'm no ImageLib cache expert but apparently it's unable to succesfully cancel the original channel, instead it is required to report an error.

Comment 62

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

Well you can't pass a success code to nsIRequest::cancel.

Comment 63

[Darin Fisher]

I don't understand.  Why doesn't ImageLib pass a dummy error code to Necko and then map that error code back to something like NS_OK before reporting it to its consumer?  Why must ImageLib's consumer know about this?  Do we have to go and hunt down every ImageLib consumer and fix them in the same way as nsImageDocument?

Comment 64

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

hm, that'd probably be a good idea

Comment 65

[Robin Whittleton]

This WFM on Firefox 3 rc1. Right-click / Properties on an image has the alt text set as the image URL which I think is acceptable. Tools / Page Info doesn't show the alt text field at all.

Comment 66

[Florian Quèze [:florian]]

(In reply to comment #65)

> Properties on an image has the alt
> text set as the image URL which I think is acceptable.

I think displaying the URL as alt text is wrong both because it's redundant and because it is not the content of an alt attribute of an img tag sent by a server.
It's a minor issue though.

> Tools / Page Info doesn't show the alt text field at all.

It was fixed (at least for Firefox) in Page Info in another bug.

Comment 67

[Philip Chee]

> Actual Results:  
> "Alternate text" says 'The image [image_name] cannot be displayed, because it
> contains errors.'
The original problem is fixed as it now shows the url of the image. As to whether this is correct or if this attribute should be hidden for images is gist for another bug.

Comment 68

[:Hb]

VERIFIED Windows NT 5.1; en-US; rv:1.9.1.5pre) Gecko/20091019