Follow-up to issue https://www.drupal.org/node/1470018#comment-9338279

When adding an image within the wysiwyg, fields values of the entity are shown in default languages. They need to show the translated values if available.

CommentFileSizeAuthor
#22 media3x-media_wysiwyg_et_integration-2378463-22.patch8.62 KBjoseph.olstad
#14 media-media_wysiwyg_et_integration-2378463-14.patch8.64 KBjoseph.olstad
#12 media-media_wysiwyg-et_integration-2378463-12.patch8.75 KBpeximo
#7 media-media_wysiwyg-et_integration-2378463-7.2.20.patch8.33 KBpeximo
#7 media-media_wysiwyg-et_integration-2378463-7.patch8.29 KBpeximo
#5 2378463_5.patch8.59 KBchx
#3 interdiff.txt2.61 KBDavid_Rothstein
#3 media-alt-title-language-2378463-3-7.x-2.0-alpha3-do-not-test.patch6.07 KBDavid_Rothstein
#3 media-alt-title-language-2378463-3.patch6.15 KBDavid_Rothstein
#2 media-alt-title-language-2378463-2-7.x-2.0-alpha3-do-not-test.patch5.74 KBDavid_Rothstein
#2 media-alt-title-language-2378463-2.patch5.82 KBDavid_Rothstein
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pmusaraj’s picture

Fixing this properly would require passing the language of the current node to the media browser. Without knowing the language of the current node, there is no way to load the correct file entity language.

There is a workaround, though. We can use two hooks to load the correct alt and title fields for the file entity when displaying the image. This would bypass the form in the popup, therefore, a second hook allows us to disable those two fields in the popup window.

Code below assume it's in a module called testmodule:

<?php 

function testmodule_media_wysiwyg_token_to_markup_alter(&$element, $tag_info, $settings) {
  // Display ALT and TITLE using file's entity translation fields, if available
  if (arg(0)=='node' && is_numeric(arg(1))) {
    global $language;
    $lang = $language->language;
    $file = file_load($tag_info['fid']);
    $alt = $file->field_file_image_alt_text[$lang][0]['value'];
    $title = $file->field_file_image_title_text[$lang][0]['value'];
    if ($alt != '')
      $element['content']['file']['#alt'] = $alt;
    if ($title != '')
      $element['content']['file']['#title'] = $title;
  }
}

function testmodule_media_wysiwyg_format_form_prepare_alter(&$form, &$form_state, $file) {
  // Disable fields since updated values are overridden by values in file
  $form['options']['fields']['field_file_image_alt_text']['#disabled'] = TRUE;
  $form['options']['fields']['field_file_image_title_text']['#disabled'] = TRUE;
}

David_Rothstein’s picture

Here's a patch that's a partial fix (I thought of creating a separate issue for this, but seems to make more sense in this one).

It makes things work correctly on display, although it's pretty hacky.

It does not solve the problem of being able to add overridden title/alt text for a particular language when embedding an image within the media browser; I believe if you type something in there it will still always be used as the English value. But it does handle the case where the image file entity has translated alt/title fields attached to it, and it makes sure that the correct language is used for those when the embedded image is being rendered. There are two fixes:

  1. The first is relatively simple; when the Media module is overriding field values on the file entity during display based on alt/title text that was added at the time the image was embedded, it clobbers all languages with the overrides - instead, it should only override the correct language and leave the others alone.
  2. The second is a lot more complicated and has to do with the fact that the Media module sometimes stores an override for the "alt" or "title" HTML attribute directly as part of the embed (separate from the overridden fields, and therefore devoid of any language information). There doesn't seem to be any good way to prevent it from ever doing that during embeds (at least not without a very large refactoring) so I dealt with it on display instead. This is explained in the code comments.

I've only tested this patch against an older version of the Media module (alpha3) and am attaching a version of the patch for that too, but the version for 7.x-2.x was a pretty straightforward reroll of that.

David_Rothstein’s picture

New patches fixing a logic error - the previous version compared the 'alt' and 'title' overrides to the alt/title text stored on the entity itself, but instead should be comparing to the field overrides that were embedded in the WYSIWYG (which is basically what the documentation said it was already doing).

Without this fix, the patch essentially stopped working if someone went back and edited the original English version of the file to change the alt/title text there to something else.

mgifford’s picture

Issue tags: +Accessibility

Would be great if this were included @David_Rothstein's patch still works.

chx’s picture

@David_Rothstein's patch is great but with the latest dev it does not seem to be enough any more as the return value from media_wysiwyg_filter_field_parser is set completely on the field instead of merging in. Ie the English version was entered in the WYSIWYG editor and then later a German version was added so after file load you have $file->some_alt['en'] $file->some_alt['de'] but the override only contains $file->some_alt['en'] and the current code sets that to $file->some_alt destroying the German translation.

Reviewing media_wysiwyg_filter_field_parser found that the merging is basically happening there already so I passed in $file then rewrote the merging. I also found traces of this wanting to happen as media_wysiwyg_format_form already passed in $file to this function.

sylus’s picture

Just for my own edification is this related to this issue? I am thinking not but wanted to make sure not duplicating any logic with langcode handling. Thx!

#2129273: Pass langcode to media_wysiwyg_token_to_markup resolving media alt attributes in interface

peximo’s picture

Rerolled. Testing the old patch I found that overrides works fine with source language but it seems to me there are some problems with translated content.
Rerolled patch doesn’t fix this problem, it just updates the changes to the last version.

The last submitted patch, 7: media-media_wysiwyg-et_integration-2378463-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work
joseph.olstad’s picture

Issue summary: View changes

@peximo, I'm kind of surprised to see this issue as I haven't noticed it. I'm not sure any patch is needed, I'll have to have a closer look myself. Meanwhile I've got some steps for correctly enabling file_entities (media) as entity_translate enabled:

See these instructions.
#2838315: Support for translating file entities

Have you configured file_entity to be entity translatable as per these instructions?

joseph.olstad’s picture

Issue summary: View changes

in addition:
7.x-4.x is really good from what I can tell, I haven't noticed any problems with it yet.
Kaare from the University of Oslo developed the 7.x-4.x version and he did an excellent job. You might want to try it out and report your results.
Please review my previous comment with configuration instructions for file_entity and entity_translation enabling/configuring of file entities.

peximo’s picture

@joseph.olstad, thanks for your reply. I've a site with Media 2.x and unfortunately I can't upgrade this module to 4.x.
I've Media 7.x-2.0-beta1 with this patch applied and all works well, I've read the provided instructions and everything seems to me correctly configured.
After the update to 7.x-2.20 the translated alt and title are showed in a wrong language (the source language).
With this rerolled patch everything works good again.

joseph.olstad’s picture

Status: Needs work » Active

the last patch applies correctly (with fuzz) on the current HEAD of the media 7.x-2.x branch

Something must be up with the testbot, I retriggered just now.

joseph.olstad’s picture

test runner didn't like the fuzz, the new patch has no fuzz.

joseph.olstad’s picture

Status: Active » Needs review
joseph.olstad’s picture

Assigned: Unassigned » joseph.olstad

joseph.olstad’s picture

Patch needs to use array() syntax, not []
array() is not deprecated yet so it's still good.

**EDIT** looks like the issue was just a glitch with the testbot

joseph.olstad’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Category: Bug report » Feature request
FileSize
8.62 KB

needs a port to 4x, it's conflicts everywhere.

reroll for 3x, same patch, not sure why the testbot is being so picky about fuzz

joseph.olstad’s picture

Issue tags: +patch needs port to media4x
steinmb’s picture