Image alt and title tags filled in via the wysiwyg editor are lost [#1242884]

Using the wysiwyg module together with the media module I insert an image in a node. If I put something in the image's alternative text and title and save the node the tags are present in the database. So far everything works as expected.
The problem is the tags are only in the database but invisible in the node's view and edit mode afterwards. The image is rendered with an empty alt tag.
I tracked the problem down to the file modules/media/includes/media.filter.inc.
The function media_get_file_without_label() inserts the alt and title tag in the attributes array of the image's render markup. Due to the design of the function theme_image() in includes/theme.inc alt and title tags are ignored if they are in the attributes array. See #999338: theme_image() alt attribute cannot be passed in $variables['attributes']

A workaround would be to insert the alt and title tags in the markup directly in media_get_file_without_label():

Before:

function media_get_file_without_label($file, $view_mode, $settings = array()) {

  [...]

  if (!isset($element['#attributes']) && isset($settings['attributes'])) {
    $element['#attributes'] = $settings['attributes'];
  }

  [...]

After:

function media_get_file_without_label($file, $view_mode, $settings = array()) {

  [...]

  if (!isset($element['#attributes']) && isset($settings['attributes'])) {
    $element['#attributes'] = $settings['attributes'];
  }
  
  if(isset($settings['attributes'])) {
	  if(isset($settings['attributes']['alt'])) {
    	$element['#alt'] = $settings['attributes']['alt'];	 
	  }
	  if(isset($settings['attributes']['title'])) {
    	$element['#title'] = $settings['attributes']['title'];	 
	  }
  }

  [...]

The problem exists in the 1.x branch too. The workaround fixes the problem for me.
To prevent overhead the alt and title tag shouldn't be include in the attributes array I think. Not sure how to achieve that.

Sorry that I can't provide a patch file. Maybe someone can provide a patch file to simplify the testing process.

Comment	File	Size	Author
#18	wysiywg_alt.jpg	42.76 KB	ParisLiakos
#13	media-alt-attribute.patch	1.86 KB	slashrsm
#13
#8	media-alt-attribute.patch	1.87 KB	effulgentsia
#8
#5	media-alt-markup-fix-7.x-2.x-1242884-5.patch	1005 bytes	kofi
#5
#4	media-alt-fix-7.x-2.x-1242884-4.patch	1.18 KB	tsvenson
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tsvenson CreditAttribution: tsvenson commented 8 August 2011 at 15:35

Made #1242760: Alt text added uses CKEditor image properties stripped a dupe of this.

Title works in the current 2.x versions, alt still doesn't. See dupe issue for more info.

Comment #2

kofi CreditAttribution: kofi commented 8 August 2011 at 15:43

@tsvenson you are right title works for me too.

Changed the code to:

function media_get_file_without_label($file, $view_mode, $settings = array()) {

  [...]

  if (!isset($element['#attributes']) && isset($settings['attributes'])) {
    $element['#attributes'] = $settings['attributes'];
  }
  
   if(isset($settings['attributes']) && isset($settings['attributes']['alt'])) {
   	$element['#alt'] = $settings['attributes']['alt'];	 
  }

  [...]

Comment #3

kofi CreditAttribution: kofi commented 8 August 2011 at 15:56

To make more clear why the alt tag is stripped:

The function theme_image() in theme.inc looks like this:

function theme_image($variables) {
  $attributes = $variables['attributes'];
  $attributes['src'] = file_create_url($variables['path']);

  foreach (array('width', 'height', 'alt', 'title') as $key) {

    if (isset($variables[$key])) {
      $attributes[$key] = $variables[$key];
    }
  }

  return '<img' . drupal_attributes($attributes) . ' />';
}

The documentation says that $variables['alt'] defaults to an empty string.
http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_image/7

The problem is, that the function isset() returns true even if a string is empty. Therefore the content of the alt tag in $attributes/$variables['attributes'] is overwritten with an empty string during the foreach cycle all the time.

Comment #4

tsvenson CreditAttribution: tsvenson commented 8 August 2011 at 17:01

Status:

Active

» Needs review

File	Size
media-alt-fix-7.x-2.x-1242884-4.patch	1.18 KB

Nice. Tested and it works. Rolled a diff patch against current 2.x for it.

Comment #5

kofi CreditAttribution: kofi commented 8 August 2011 at 21:21

File	Size
media-alt-markup-fix-7.x-2.x-1242884-5.patch	1005 bytes

Thanks tsvenson for providing the patch! After hours of reading I figured out how to create patch files myself :)
I think you should not touch the other if statements to keep the legacy support for styles modules etc. working.
Therefore I created another patch that is simply adding the #alt variable to the elements render markup.

Comment #6

tsvenson CreditAttribution: tsvenson commented 8 August 2011 at 22:25

Status:

Needs review

» Reviewed & tested by the community

@kofi: I know it can be a bit tricky to create patches. Just learned it myself using Netbeans 7.1 and its git plugin.

Tried your patch in #5 and it does the trick for me. Great work.

Comment #8

effulgentsia CreditAttribution: effulgentsia commented 16 August 2011 at 01:09

Status:

Reviewed & tested by the community

» Needs review

File	Size
media-alt-attribute.patch	1.87 KB

How about this? It's a more targeted version of #5.

Comment #9

tsvenson CreditAttribution: tsvenson commented 16 August 2011 at 21:09

Status:

Needs review

» Reviewed & tested by the community

Patch in #8 works fine. Tested with todays 2.x dev release so its ready to be committed.

Comment #10

skyredwang

Shanghai

CreditAttribution: skyredwang commented 22 August 2011 at 11:32

Can someone also commit the fix to 7.x-1.x?

Comment #11

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 22 August 2011 at 21:17

Status:

Reviewed & tested by the community

» Needs work

+++ b/includes/media.filter.incundefined
@@ -388,6 +388,17 @@ function media_get_file_without_label($file, $view_mode, $settings = array()) {
+    if (isset($settings['attributes']['alt']) && !array_key_exists('#alt', $element) && isset($element['#theme']) && in_array($element['#theme'], array('image', 'image_style'))) {

Why are we using array_key_exists() here rather than !isset($element['#alt'])?

Comment #12

bryancasler CreditAttribution: bryancasler commented 23 August 2011 at 16:24

Comment #13

slashrsm CreditAttribution: slashrsm commented 28 August 2011 at 12:51

Status:

Needs work

» Needs review

File	Size
media-alt-attribute.patch	1.86 KB

I changed array_key_exists() for isset().

Comment #14

effulgentsia CreditAttribution: effulgentsia commented 29 August 2011 at 15:17

My thought for using array_key_exists() is that whatever is in $element should take precedence, and setting $element['#alt'] to NULL is the only way to omit the alt attribute (since it otherwise defaults to empty string). That said, I don't know of an actual use-case for setting $element['#alt'] to NULL when there's a value in $settings['attributes']['alt'], so it probably doesn't matter.

Comment #15

ddanier CreditAttribution: ddanier commented 9 September 2011 at 13:37

subscribing, same issue here

Comment #16

tsvenson CreditAttribution: tsvenson commented 26 September 2011 at 00:59

Issue tags:

+Media Sprint 2011

This should really be sorted during the sprint.

Easy enough to also be backported to 1.x, probably without any modifications needed.

Comment #17

ParisLiakos CreditAttribution: ParisLiakos commented 26 September 2011 at 02:00

Just tested and works.if you manually edit the alt attribute in the wysiwyg editor the alt is displayed correctly.
but maybe we should expand it a bit more and add the description the user enters when selects the image to the alt?manually editing the alt before saving a node is kinda not user friendly.

Actually this is the expected behavior.since that's the description in the Description textfield (after image is selected from library):
Alternate text a user will see if the image is not available
and use this for both title and alt attributes?

Comment #18

ParisLiakos CreditAttribution: ParisLiakos commented 26 September 2011 at 02:07

File	Size
wysiywg_alt.jpg	42.76 KB

Attaching image to make my point:)

Comment #19

tsvenson CreditAttribution: tsvenson commented 26 September 2011 at 08:53

Status:

Needs review

» Reviewed & tested by the community

@rootatwc: I've tried the patch with success as well, however effulgentsia's concerns in comment #14 needs to be looked at before it can be committed.

Comment #20

effulgentsia CreditAttribution: effulgentsia commented 7 October 2011 at 00:47

Status:

Reviewed & tested by the community

» Fixed

Committed #13 to 1.x and 2.x. I'm willing to forego my concern in #14 until a use-case comes up for it. Seems like bad practice to set alt to NULL anyway, I think.

Re #17: can you open a new issue for that? Thanks.