The function theme_file_icon($variables) returns code which includes an empty alt attribute. I believe it would correctly return an alt attribute such as "PDF" for a PDF file, etc.

If it is determined that this icon is purely decorative (and if it is, then why is the title attribute filled in?) and is properly a null alt attribute, then add a class "decorative" so that automated checking for missing/null alt attributes can be taught to ignore the lack of an alt attribute, to distinguish it from content where site content providers neglected to provide an alt attribute.

Files: 
CommentFileSizeAuthor
#75 file-icon-alt-text-2163209-75-D7.patch2.45 KBDaniel.Moberly
FAILED: [[SimpleTest]]: [MySQL] 41,756 pass(es), 0 fail(s), and 343 exception(s). View
#3 theme_file_icon-alt-text-2163209-3.patch547 bytesmgifford
PASSED: [[SimpleTest]]: [MySQL] 59,839 pass(es). View
#7 theme_file_icon-alt-text-2163209-7.patch1.24 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 60,097 pass(es), 0 fail(s), and 249 exception(s). View
#8 theme_file_icon-alt-text-2163209-8.patch1.16 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/file.module. View
#10 theme_file_icon-alt-text-2163209-9.patch1.16 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 64,495 pass(es). View
#11 Screen Shot 2014-01-16 at 4.56.58 PM.png62.34 KBmgifford
#21 file-icon-alt-text-2163209-21.patch1.36 KBandrewmacpherson
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,650 pass(es). View
#24 file-icon-alt-text-2163209-24.patch1.47 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,761 pass(es). View
#24 file-icon-alt-text-2163209-24.patch1.47 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-icon-alt-text-2163209-24_0.patch. Unable to apply patch. See the log in the details link for more information. View
#29 file-icon-alt-text-2163209-29.patch1.47 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,166 pass(es). View
#35 file-icon-alt-text-2163209-35-D7.patch1.97 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 41,033 pass(es). View
#35 Screen Shot 2014-07-14 at 12.35.42 AM.png146.01 KBmgifford
#58 file-icon-alt-text-2163209-58-D7.patch2.72 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,033 pass(es). View
#58 interdiff.txt1.87 KBDavid_Rothstein
#60 file-icon-alt-text-2163209-60-D7.patch2.72 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 41,504 pass(es). View

Comments

Charles Belov’s picture

Issue summary: View changes
mgifford’s picture

Version: 7.24 » 8.x-dev

Note that this is still a problem with this function.
https://api.drupal.org/api/drupal/core!modules!file!file.module/function...

mgifford’s picture

Status: Active » Needs review
FileSize
547 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,839 pass(es). View

Here's the patch. I'm actually in favour of nixing the title tag here.

More info on the ideal here #1919940: Build API to Replace Links using Title Attributes with Proper Accessible, Themable Tooltips

Charles Belov’s picture

Actually, returning the mime isn't particularly friendly as alternate text. I'm guessing a non-technical person would not find it helpful to hear, say, "application slash pdf". It would probably be more useful to hear "PDF icon" or "PDF".

Hope this helps.

mgifford’s picture

There are about 350 mime types that Drupal manages here https://api.drupal.org/api/drupal/core!includes!file.mimetypes.inc/funct...

Any case for a more human friendly string would involve checking the mime type and then providing a human friendly alternate.

See:
www.freeformatter.com/mime-types-list.html#mime-types-list
http://stackoverflow.com/questions/18038676/getting-mime-type-descriptio...

Charles Belov’s picture

However, there are only 14 icons in \modules\file\icons\. I believe 14 alternate texts would be quite manageable.

text-x-generic.png Text icon
text-x-script.png Script icon
video-x-generic.png Video icon
x-office-document.png Office document icon
x-office-presentation.png Office presentation icon
x-office-spreadsheet.png Office spreadsheet icon
application-octet-stream.png Binary code icon
application-pdf.png PDF icon
application-x-executable.png Program icon
audio-x-generic.png Audio icon
image-x-generic.png Image icon
package-x-generic.png Package icon
text-html.png HTML icon
text-plain.png Plain text icon

mgifford’s picture

FileSize
1.24 KB
FAILED: [[SimpleTest]]: [MySQL] 60,097 pass(es), 0 fail(s), and 249 exception(s). View

Ok, so if we restrict it to just those it can look a bit more like this.

mgifford’s picture

FileSize
1.16 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/file.module. View

These are easy:
txt, pdf, doc, html, jpg, gif, png, xls, ppt

'application/msword' => t('Microsoft Office document icon'),
'x-office-presentation' => t('Office presentation icon'),
'x-office-spreadsheet' => t('Office spreadsheet icon'),
'application/pdf' => t('PDF icon'),
'image/jpeg' => t('Image icon'),
'image/png' => t('Image icon'),
'image/gif' => t('Image icon'),
'text/html' => t('HTML icon'),
'text/plain' => t('Plain text icon'),

But there are alternates http://filext.com/faq/office_mime_types.php

Not sure about:
mp4, mov, wav, mp3, bin, exe

Or if there are others, but don't think these are useful:

// 'text-x-generic' => t('Text icon'),
// 'text-x-script' => t('Script icon'),
// 'video-x-generic' => t('Video icon'),
// 'application-octet-stream' => t('Binary code icon'),
// 'application-x-executable' => t('Program icon'),
// 'audio-x-generic' => t('Audio icon'),
// 'package-x-generic' => t('Package icon'),

Again, not sure which variations or for that matter what the default should be:
http://www.php.net/manual/en/function.mime-content-type.php

Where did you get your list?

Status: Needs review » Needs work

The last submitted patch, 8: theme_file_icon-alt-text-2163209-8.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
PASSED: [[SimpleTest]]: [MySQL] 64,495 pass(es). View

arg

mgifford’s picture

This needs to be extended, but is the results from a WAVE scan of one of the pages showing the alt text for the images.

The last submitted patch, 7: theme_file_icon-alt-text-2163209-7.patch, failed testing.

The last submitted patch, 7: theme_file_icon-alt-text-2163209-7.patch, failed testing.

dawehner’s picture

It seems to be possible to reuse information coming from file_default_mimetype_mapping()

Charles Belov’s picture

#8 Where did you get your list?

I got my list of MIME-type icons from the docroot/modules/file/icons/ folder. You only need alternate text for images that exist, and there are only those 14 images.

mp4, mov, wav, mp3, bin, exe

Those other mime types don't have images in that folder, so they don't need to have alternate text added. However, if you do add images for those other MIME types, then alternate text needs to be added as well.

That does bring up the larger issue that someone could add images. In theory, they aren't supposed to modify core. I don't know whether the Files module supports looking in some other location for additional images this, but if they did add images, they would also need to add alternate text, presumably by overriding the theme_file_icon function, same as I have until this issue is fixed and pushed to production.

This points to the question, presumably for Drupal 8, not Drupal 7, whether mime type icons and their corresponding alternate text belong in configuration rather than docroot/modules/file/icons/ (for the icons) and code (for the alternate text).

In terms of scope control for the current issue though, there are only 14 images and therefore only 14 alternate texts are needed.

mgifford’s picture

Thanks @Charles Belov. It's good to keep it focused if we can.

@dawehner file_default_mimetype_mapping() doesn't get us any closer to a human readable string like "Microsoft Word Document".

mgifford’s picture

There is a duplicate here #1507754: Give file icons alt text

Not sure which should be closed.

mgifford’s picture

mgifford’s picture

Keeping this issue open.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

No longer applies.

andrewmacpherson’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,650 pass(es). View

theme_file_icon() has been removed as part of the twig effort, replaced by template_preprocess_file_link() (see #1898070: file.module - Convert theme_ functions to Twig).

This patch just re-implements the list of text-alternatives from comment #9. It also fixes the text for spreadsheets and presentation (they were the wrong way round).

However, it doesn't work for some mime-types. If an MS-word ".doc" file is uploaded, the icon gets alt text, but if an OpenDocument ".odt" file is uploaded, there is no alt text.

The approach we're using so far is to determine the alt text based on the mime-type, but some generic icons get used for several mimetypes (notably the office ones, see file_map_icon()). If we determine the alt text by mime-type, then we don't have nearly enough human-names. Perhaps we need an equivalent file_map_alt_text()? Do mime-types have "official" human-readable names we can use as our source?

Alternatively, we could keep the list of text-alternatives short if we map them to the icon image instead of the mime-type.

andrewmacpherson’s picture

Sorry, I missed the links referenced in comment #5.

The list at http://www.freeformatter.com/mime-types-list.html looks like a good start if we decide to go for a big list of human-friendly names for mime-types, although the author says it not an "official" list.

andrewmacpherson’s picture

If this is committed, we might mention it in the D8 accessibility features blurb at
https://www.drupal.org/drupal-8.0/features#accessibility

mgifford’s picture

FileSize
1.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,761 pass(es). View
1.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-icon-alt-text-2163209-24_0.patch. Unable to apply patch. See the log in the details link for more information. View

Thanks for patch and link to #1898070: file.module - Convert theme_ functions to Twig and fixes.

I do like the idea of adding more icons, especially for OpenDocument (OpenOffice or LibreOffice), but that should be a different issue. We've only got to contend with the following in this issue:

$ ls core/modules/file/icons/
application-octet-stream.png audio-x-generic.png text-html.png text-x-script.png x-office-presentation.png application-pdf.png image-x-generic.png text-plain.png video-x-generic.png x-office-spreadsheet.png application-x-executable.png package-x-generic.png text-x-generic.png x-office-document.png

I like the idea of extending the icons to include a more full range of extensions and mime types, putting them in a file_map_alt_text() function would be great. But I'd like to see that extended list be a new issue. Right now we just have to worry about providing alt tags for the images we actually use.

I modified the patch to include binary files and also provide a default if the mimetype isn't described.

If we don't have images for the big list, we don't need human readable alt text for it. That's what I figure in any case.

davidhernandez’s picture

I tested this and achieved the desired result on the node edit form and the node display. Do you want to RTBC this or do any other mime types need adding?

mgifford’s picture

I don't think we need to address the other mime types at this time. We can save that for D9. This will hit 90% of use cases I think.

Just need someone to mark as RTBC as far as I'm concerned.

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: file-icon-alt-text-2163209-24.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,166 pass(es). View

Reroll..

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Oooh, this is really nice. The only thing we might want to do here is pull that into a helper function so that you can easily get that list that from other places, but we could always do that in a follow-up.

Committed and pushed to 8.x. Thanks!

  • webchick committed d14ea92 on 8.x
    Issue #2163209 by andrewmacpherson, mgifford | Charles Belov: Add...
mgifford’s picture

Is this something that can be backported to D7?

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

I don't see why not.

mgifford’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.97 KB
PASSED: [[SimpleTest]]: [MySQL] 41,033 pass(es). View
146.01 KB

Ok, I think this is a good first start. Seems to be working here:

alt text for file images.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

#35 looks good to me. The change is the same as what went into 8.x, with respect to the differences in the theme system between the two versions. I get the same result as is shown in #35's attached image. After applying the patch and clearing the cache the alt attribute is added to file download link icons.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: file-icon-alt-text-2163209-35-D7.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: file-icon-alt-text-2163209-35-D7.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: file-icon-alt-text-2163209-35-D7.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: file-icon-alt-text-2163209-35-D7.patch, failed testing.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

bad bot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: file-icon-alt-text-2163209-35-D7.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: file-icon-alt-text-2163209-35-D7.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: file-icon-alt-text-2163209-35-D7.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.72 KB
PASSED: [[SimpleTest]]: [MySQL] 41,033 pass(es). View
1.87 KB

Hm, I think there are a few problems here (see attached patch and interdiff):

  1. No default value for 'alt' in the theme function (will cause PHP notices with several contrib modules that use this theme function).
  2. No documentation of the new parameter to the theme function.
  3. No check_plain() call before printing this to HTML (although all the particular alt values used in this patch are OK, they aren't necessarily always going to be).
  4. (minor) Slightly wasteful double call to file_get_mimetype() - this looks like it has the same issue in Drupal 8 too.
David_Rothstein’s picture

I'm also not sure file_get_mimetype($file->filename) is correct... shouldn't it be file_get_mimetype($file->uri)?

I believe $file->filename can sometimes be a human readable name (like "My PDF File") rather than something with an extension, like my-pdf-file.pdf...

mgifford’s picture

FileSize
2.72 KB
PASSED: [[SimpleTest]]: [MySQL] 41,504 pass(es). View

I don't know, looks better from the API, but it should be easy to test. This patch includes file_get_mimetype($file->uri).

They both seem to work fine. Would be good to know what test case one would fail.

ruha’s picture

This patch addresses an issue I'm having with a site right now. I'm having trouble getting it to work on 7.37 with PDFs. I am getting the alt tags on the node edit page but not on the node view page.

mgifford’s picture

@ruha - want to mark it RTBC?

jasonflaherty’s picture

Hello, was just looking into this issue. Is the idea to add a patch to drupal core for this or should I begin looking to a preprocess function? Thanks!

mgifford’s picture

@buildakicker the goal was to add the patch to core. Although perhaps there's a better way to do this.

jasonflaherty’s picture

Thanks @mgifford. This is a good for accessibility, not perfect, but a good fix. I ended up using a preprocess in the time being:

function bootstrap_file_icon($variables) {
  $file = $variables['file'];
  $icon_directory = $variables['icon_directory'];

  $mime = check_plain($file->filemime);

  $icon_url = file_icon_url($file, $icon_directory);
  return 'Only local images are allowed.';
}

Not sure if this is the best solution... but it does describe the icon :).

Thanks!

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
talhaparacha’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
90.33 KB

The patch at #60 applied cleanly. Not need for a re-roll. Tested a few file types after applying patch, works as expected. Screenshot after applying patch

joewhitsitt’s picture

#60 Applied cleanly to core 7.39. Tested with PDF file icons and got the expected alt="PDF icon" result.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.40 release notes

Committed to 7.x - thanks!

+  $mimetype = file_get_mimetype($file->uri);

We should consider a followup to see if $file->mimetype can simply be used here - not sure why it wouldn't work.

David_Rothstein’s picture

I misspoke on the name of the file object property above, but I think I had the idea right :) See related issue.

David_Rothstein’s picture

Also, I should mention, I should have found and committed this issue earlier in the release cycle (since it adds translatable strings, and we want to give translators time to translate them).

But in the end, I think untranslated alt text for these images is likely better than no alt text at all (maybe?) so I went ahead and committed it now anyway.

Daniel.Moberly’s picture

Status: Fixed » Needs review
FileSize
2.45 KB
FAILED: [[SimpleTest]]: [MySQL] 41,756 pass(es), 0 fail(s), and 343 exception(s). View

Why are we only allowing end users to modify the alt tag for theme_file_icon? Doesn't it make more sense to just allow users to do whatever they want with the tags like in theme_image? I don't see why there is a case to be made that users should be able to add alt text but not their own classes or height/width values (which are flagged as missing by many code-review systems such as Google Developer Tools). Personally I know I have cases where I am manually adding height/width tags into the output string after rendering since theme_file_icon doesn't give any options to do so.

Allowing users to add their own attributes also allows users to choose to make the icon "decorative" as described in the original post by passing the appropriate class names, etc.

Attaching a patch to make this function much more like theme_image while still allowing for default MIME values in the title field, etc. for others' thoughts.

Status: Needs review » Needs work

The last submitted patch, 75: file-icon-alt-text-2163209-75-D7.patch, failed testing.

mgifford’s picture

I think this would be better as a new issue for 8.1. Please start a new issue and link to it from here.

David_Rothstein’s picture

Status: Needs work » Fixed

I suspect it's not relevant for Drupal 8, but yes, a new issue for that sounds like a good idea.

It's an interesting proposal, but not directly related to this issue since all we were trying to do here was add the alt text, which is now done.

Daniel.Moberly’s picture

I agree that its not relevant for Drupal 8 as the render array can be manipulated - the image tag is output directly in the function in Drupal 7.

New issue here: https://www.drupal.org/node/2594281

pfrenssen’s picture

Why was this committed without a test? This is now throwing notices in existing sites that call this theme function without supplying the 'alt' key.

See #2599180: Make alt and icon_directory properties really optionals.

pfrenssen’s picture

Status: Fixed » Needs work
Issue tags: +Needs tests

This got committed to both D7 and D8 without having test coverage.

David_Rothstein’s picture

Tests would definitely be nice. However, there shouldn't be any PHP notices unless you forgot to clear caches (or run update.php) when updating a site to the newest release. I'll comment on the other issue.

pfrenssen’s picture

@David_Rothstein, you're correct, there are no notices thrown, #2599180: Make alt and icon_directory properties really optionals appears to have been caused by an incomplete core update.

fallenturtle’s picture

Is there a way to map multiple file extensions to a single text-alternative? For example the default array supplied in this patch seems to only catch .doc as a MS Word doc and not the newer .docx type. I'm assuming the same applies to all the other MS Office apps which have added "x" to the end of their extensions. Do I just need to add extra entries like so:

'application/msword' => t('Microsoft Office document icon'),
'application/vnd.openxmlformats-officedocument.wordprocessingml.document' => t('Microsoft Office document icon')

Or is it possible to be like (I know this isn't correct code):

'application/msword', 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' => t('Microsoft Office document icon')

  • webchick committed d14ea92 on 8.3.x
    Issue #2163209 by andrewmacpherson, mgifford | Charles Belov: Add...

  • webchick committed d14ea92 on 8.3.x
    Issue #2163209 by andrewmacpherson, mgifford | Charles Belov: Add...

  • webchick committed d14ea92 on 8.4.x
    Issue #2163209 by andrewmacpherson, mgifford | Charles Belov: Add...

  • webchick committed d14ea92 on 8.4.x
    Issue #2163209 by andrewmacpherson, mgifford | Charles Belov: Add...