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 |
#8 | media-alt-attribute.patch | 1.87 KB | effulgentsia |
#5 | media-alt-markup-fix-7.x-2.x-1242884-5.patch | 1005 bytes | kofi |
#4 | media-alt-fix-7.x-2.x-1242884-4.patch | 1.18 KB | tsvenson |
Comments
Comment #1
tsvenson CreditAttribution: tsvenson commentedMade #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@tsvenson you are right title works for me too.
Changed the code to:
Comment #3
kofi CreditAttribution: kofi commentedTo make more clear why the alt tag is stripped:
The function theme_image() in theme.inc looks like this:
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 commentedNice. Tested and it works. Rolled a diff patch against current 2.x for it.
Comment #5
kofi CreditAttribution: kofi commentedThanks 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@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 commentedHow about this? It's a more targeted version of #5.
Comment #9
tsvenson CreditAttribution: tsvenson commentedPatch in #8 works fine. Tested with todays 2.x dev release so its ready to be committed.
Comment #10
skyredwangCan someone also commit the fix to 7.x-1.x?
Comment #11
Dave ReidWhy are we using array_key_exists() here rather than !isset($element['#alt'])?
Comment #12
bryancasler CreditAttribution: bryancasler commentedsubscribe
Comment #13
slashrsm CreditAttribution: slashrsm commentedI changed array_key_exists() for isset().
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedMy 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 commentedsubscribing, same issue here
Comment #16
tsvenson CreditAttribution: tsvenson commentedThis 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 commentedJust 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 commentedAttaching image to make my point:)
Comment #19
tsvenson CreditAttribution: tsvenson commented@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 commentedCommitted #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.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commented@effulgentsia well seems that textfield is now removed:/
check #1251468: Hide Confusing Field 'Description' When Embedding Media
Comment #22
HyperGlide CreditAttribution: HyperGlide commentedsubscribe