Clean up theme_image() to use drupal_attributes() for all attributes and revisit defaults for "alt" and "title"

.

Looks like we're mixing isset() and !empty() in that line. Is that intentional?

HEAD is broken.

theme_image() did change since the patch. Now we have

return '<img src="' . check_url($url) . '" alt="' . check_plain($alt) . '" title="' . check_plain($title) . '" ' . (isset($image_attributes) ? $image_attributes : '') . $attributes . ' />';

If both are empty there still might be two spaces.
But I can't run this through a debugger right now to see what happens there.

how's this?

#10: theme_image_minor_cleanup-555830-10.patch queued for re-testing.

Moving to "markup" queue. Trivial clean up.

Let's do this properly, please.

/me blinks ;)

Variable clean-up.

+++ includes/theme.inc	13 Mar 2010 20:20:22 -0000
@@ -1499,10 +1499,21 @@ function theme_image($variables) {
+    $attributes['src'] = check_url(file_create_url($path));
+    if (!empty($alt)) {
+      $attributes['alt'] = check_plain($alt);
+    }
+    if (!empty($title)) {
+      $attributes['title'] = check_plain($title);
+    }

drupal_attributes() and check_url() call check_plain(), so this results in 'src', 'alt', and 'title' being check_plain()'d twice.

Powered by Dreditor.

The "quick fix" tag is a curse.

Obviously, I've lost track of which functions already call check_plain() and which functions not. :(

+++ includes/theme.inc	14 Mar 2010 18:38:35 -0000
@@ -1499,10 +1499,21 @@ function theme_image($variables) {
+    $attributes['src'] = check_url(file_create_url($path));

check_url() calls check_plain() too, so this results in double check_plain(). Do we need to check_url() the result of file_create_url()? Do we have this situation anywhere else: setting a value in an $attributes array to the result of a check_url()?

[EDIT: see also #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain()]

142 critical left. Go review some!

I'm really happy to see this patch! Was going to do the same thing over in: #520704: How should theme_image() deal with screen readers not reading the image title attribute?.

Much better.

Im not rly sure if I'm helping here but this is a try (just deleted check_url)

Very odd. Further findings now in #715142-6: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain()

Postponing this until we reach a conclusion about what check_url() should do in #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain().

Hmm, I think we need special handling for the alt attribute. It needs to be there (even if it's empty) for accessibility purposes because if it's not, the screen reader will announce "Image with no alt text" and then proceed to read the actual file name. See this page for details.

I did a very quick read through the comments here. I agree with @jacine, that if an image is clrearly for presentation (e.g. a spacer) that the alt attribute should = "". However, if an image conveys meaning or purpose than the alt attribute should eiter convey the meaning and purpose or not exist at all. There are cases where the name of the image file, absent an appropriate alt attribute, is more useful that alt = "".

tagging

I think this will eliminate those instances where the alt tags are the same as the filename, but I don't have an easy way to pull up this function. Is there a recommended way to see the output?

This patch applied fine. I like the additional checks for attributes.

@mgifford this is the function that takes care of outputting all images. It will take care of outputting empty attributes (especially title), and in general it's a very nice clean-up, but it can't solve the these kinds of issues, as it has no context. It just prints out what it's told. Those kinds of issues need to be dealt with at the source. I made a list of possible places that we can improve in that regard here: #520704: How should theme_image() deal with screen readers not reading the image title attribute?.

That said, a good way to test this one is to look at the Syndicate block. The RSS Icon is output using this function.

Before the patch you get:
<img src="http://sky.d7/misc/feed.png" alt="Subscribe to Syndicate" title="" width="16" height="16" />

After:
<img src="http://sky.d7/misc/feed.png" alt="Subscribe to Syndicate" width="16" height="16" />

@Jacine, sorry. I was just about to send a follow up note to say that the patch above in #29 is rubbish. The RSS Icon or themes/seven/template.php's sort ascending/descending.

Just started looking at this too late in the evening. The goal in my patch (as rubbish as it was) was simply to do a check to see that the filename wasn't being used as an alt attribute. Thinking about it after looking at #520704: How should theme_image() deal with screen readers not reading the image title attribute? it seemed that it's unlikely that someone would take the time to insert "$theme_path . '/images/arrow-asc.png'" into an alt tag. Seems most likely that someone would absentmindedly just add "arrow-asc.png".

However, I can't see someone doing that anywhere at the code level, even in the contrib directories. It's much more important to check for this in user contributed content. I'm not sure that adding a check for bad alt tag definitions (other than empty or perhaps a space " ") is useful.

I'm in full agreement though about eliminating blank tags! So +1 on #23!

Based on recent discussion in the other issue, I think we're safe to apply this change. That is, because this function returns a HTML string, so it is the last theme function in a rendering stack and therefore responsible for sanitizing the URL.

Therefore, re-uploading the last known good patch.

Adding special case for alt specified by @Jacine in #26.

+++ includes/theme.inc	10 Apr 2010 11:50:59 -0000
@@ -1532,10 +1532,24 @@ function theme_image($variables) {
-    $url = file_create_url($path);
-    return '<img src="' . check_url($url) ...
+    $attributes['src'] = file_create_url($path);

HEAD currently has a test (testLXSS()) requiring that l() be safe from XSS attacks. If l() needs to be safe from XSS, then so should theme_image(). Changing this to a requirement that bad protocols be stripped before theme('image') is called should go through a security review. Let's do that review on the other issue, and once the other issue lands, pursue this otherwise nice clean-up of theme_image().

Powered by Dreditor.

I was wrong in #34. l() calls check_plain(), not check_url(). Therefore, it's ok to change theme_image() to do the same. Patch looks good except:

+++ includes/theme.inc	10 Apr 2010 11:50:59 -0000
@@ -1532,10 +1532,24 @@ function theme_image($variables) {
+    if (!empty($alt)) {
+      $attributes['alt'] = $alt;
+    } ¶
+    else {
+      $attributes['alt'] = '';
+    }

Please remove trailing whitespace from lines, but also, can we shorten this to 1 line: $attributes['alt'] = !empty($alt) ? $alt : '';

Powered by Dreditor.

Thanks, optimized that.

How do you feel about this instead? empty() is the wrong check for $alt and $title, as there's no reason to prevent '0'. Also, I'm not crazy about making it impossible to remove the 'alt'. With this patch, passing an explicit 'alt' => NULL removes it, but leaving it unspecified defaults to it being an empty string.

I don't think this is the issue to make the #37 tests pass as a result of a decision to change what 'title' defaults to (from empty string to not being output). Let's do that in #520704: How should theme_image() deal with screen readers not reading the image title attribute?. This patch removes that, tests should pass.

I'm against introducing new @todos at this stage. Since !empty() is indeed the wrong condition, we have to implement the isset() and default argument change here.

I haven't followed all of the comments on this issue. However, I will state here that unless alt or title are explicitly set (including being explicitly set to null "") that the attributes should not be generated. When a screen-reader comes across an image with alt="" it performs differently than when the alt attribute is not present. That is, alt="" and an omitted alt attribute are not equivalents as far as accessibility is concerned. Not sure about title, but I would suspect that depending on the screen-reader it would be similar.

The scope of this issue has evolved, so updating title to match.

@Everett: Not sure whether I understood your direction regarding the alt attribute. If none has been defined, should one be output or not? Is a missing alt attribute valid XHTML?

Aside from that, attached patch should finally pass.

@Sun

There are three options for an image in xhtml

1. alt="description"
2. alt="" this is effectively saying presentation only (separator image, etc.)
3. No alt attribute, this is an image without a description, but which is not explicitly defined as presentational only.

I believe that in xhtml an alt attribute is required for validation, but I am not certain about this. Marking images with alt="" where no alt text is available is very bad for accessibility.

Example:

<img src="myDog.png" alt="" />

This would be ignored by a screen-reader.

<img src="myDog.png" />

This would be read as myDog.png by a screen-reader (more meaningful than no text at all).

<img src="r1c2.png">
This would be read as r1c2.png by a screen-reader, meaningless, but worthwhile to know that there is something with meaning that the user cannot perceive.  That is, it is valuable to know that there is information that a screen-reader reader cannot perceive, instead of simply not knowing that there was any information at all.  Gives the user the opportunity to ask someone about the image, rather than believing there to be no image.

W3C validator says that a missing alt attribute is invalid for XHTML+RDFa 1.0, so I fear there's little that we can do on that.

Given that technical doctype limitation and requirement for an alt attribute, I think we're back to RTBC here.

I agree with the RTBC. This changes 'title' from default of empty string to default of being omitted: this is in line with Everett's recommendation in #44 and has no down side AFAICT. This leaves 'alt' as defaulting to empty string (no change relative to HEAD), which is not Everett's recommendation with respect to accessibility, but is required for W3C validation.

I can appreciate the argument that 'alt' should be omitted by default, requiring code that calls theme('image') to always pass in something for 'alt' (even if empty string), and allowing a W3C validator to catch when that's not done. But that's a change from HEAD that perhaps should be debated in a separate issue? The rest of what's being done in #46 makes a lot of sense, so let's not hold it up.

Committed to CVS HEAD. Thanks.

+++ includes/common.inc	22 Apr 2010 23:18:22 -0000
@@ -5455,7 +5455,7 @@ function drupal_common_theme() {
-      'variables' => array('path' => NULL, 'alt' => '', 'title' => '', 'attributes' => array(), 'getsize' => TRUE),
+      'variables' => array('path' => NULL, 'alt' => '', 'title' => NULL, 'attributes' => array(), 'getsize' => TRUE),

As a follow-up patch, it might be nice to document why we sometimes use '' vs NULL. We can re-purpose the text from #50 for that.

Some duplication, but given how often this issue is discussed, perhaps this will help cut down on support requests. I'm not an expert on this topic or good at writing concise documentation, so anyone who is: please feel free to suggest edits.

@Everett: I'd love your feedback on this.

Awesome.

Committed to CVS HEAD.

Follow-up for D8: #999338: theme_image() alt attribute cannot be passed in $variables['attributes']