The alt and title fields have some special needs handling in their implementation. The idea is that we need these fields to also be reflected as the alt and title attributes on the html element.

When the user changes the title and/or alt field in the Wysiwyg via the media browser popup, those changes need to immediately be reflected in the macro. CKEditor's image attributes browser allows the user to manage the alt and title attributes directly. Should a change be made via the media browser, those changes should be reflected in the CKEditor browser too.

Later, when the image is rendered, those fields need some extra handling as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sylus’s picture

Status: Needs review » Needs work

First wanted to say really appreciate all of your hard work.

I have noticed though that with the last few commits alt tags have reverted in their support for entity translation.

However this is only for the CKEditor + Media Popup. The field level media selector still works as expected. Additionally if I go to /admin/content/file and inspect the alt + title fields there (in my case french + english) I can see that all of the values are appropriate.

Was hoping we could address this reversion in this issue. Additionally after looking at the patch I am not sure where the $file entity is being found as it isn't part of the media_element_validate function.

Setting back to needs work if that is okay :)

sylus’s picture

Priority: Major » Critical

Updating this to critical as is a regression.

sylus’s picture

It looks like part of the problem is the introduction of:

  $replace_options = array(
    'clear' => TRUE,
    'sanitize' => FALSE,
  );

  // Load alt and title text from fields.
  if (!empty($alt)) {
    $file->alt = token_replace($alt, array('file' => $file), $replace_options);
  }

Could this be because $replace_options in token_replace can take a language parameter, which we are not passing?

kaidjohnson’s picture

Status: Needs work » Needs review
FileSize
3.68 KB

I'm not sure about the translation stuff, but the proposed patch was not meant to have that snippet in it. I was extracting the patch above from a larger patch in #2067063: Wysiwyg integration is broken and it looks like the code from media.module was included here, when it shouldn't have been. That code has already been committed to media in its proper place (NOT where it is in the patch above...)

This patch is meant to specifically handle the syncing of alt/title fields with html alt/title attributes when working with media in a Wysiwyg environment and should not require any php updates. I've added two-way syncing:

  1. When a user updates the alt or title field from the media popup, it syncs the value to the appropriate html attribute.
  2. When a user updates the alt or title field from a wysiwyg image editor (CKEditor double-click on the image), that value is syned to the appropriate file_entity field.
ph08n1x’s picture

Omg yEssss!! This has finally fixed my issue that I was having with alternative and title attributes not being added. Awesome patch!!!

briand44’s picture

Component: WYSIWYG integration » Code
FileSize
3.88 KB

I've updated the patch to apply against the latest dev release. Also confirming that it does what it says it does. Thanks!

DamienMcKenna’s picture

Component: Code » Media WYSIWYG
aaron’s picture

Status: Needs review » Reviewed & tested by the community

Looks good -- great work!

micbar’s picture

im wondering if this needs a reroll.

micbar’s picture

SandraVdv’s picture

The patch in #6 doesn't work for the latest dev release 7.x-2.0-alpha3

SandraVdv’s picture

Oh sorry, my mistake, I didn't download the latest dev, but the alpha3 version

aaron’s picture

jwilson3’s picture

Patch in #6 RTBC++. This is an essential piece needed to finalize the wysiwyg + media integration with alt/title handling (hopefully once and for all)!

aaron’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Needs work

The last submitted patch, 6: media-wysiwyg-alt-title-handling-2126697-6.patch, failed testing.

The last submitted patch, 6: media-wysiwyg-alt-title-handling-2126697-6.patch, failed testing.

Peacog’s picture

I'm not sure if this comment belongs here or in Improve our macro handling but I'm getting Uncaught TypeError: Cannot read property 'field_file_image_title_text[und][0][value]' of undefined in syncAttributesToFields because file_info.fields is not set.
I'm using latest dev versions of Media, File Entity and CKEditor modules with CKEditor library version 4.3.1. What am I doing wrong?

Devin Carlson’s picture

I can confirm #18, on a fresh install of Media WYSIWYG with either WYSIWYG or the stand-alone CKEditor module, this commit prevents the format form from being submitted due to a JS error.

Reverted with http://drupalcode.org/project/media.git/commit/0d39e26.

kaare’s picture

Patch in #6 applies cleanly, but is otherwise broken. Alt and title fields isn't updated in the returned HTML. I think this should be postponed on #2126755: Improve Media's WYSIWYG Macro handling as this refactors and touches the same code.

Meanwhile, as a quick fix this interdiff makes this patch not choke. syncAttributesToFields() or rather extract_file_info() hands over all kinds of versions of file_info. Sometimes 'attributes' is set, sometimes not, ditto for 'fields'.

jwilson3’s picture

Rerolled #20, it no longer applies to head.

ParisLiakos’s picture

Status: Needs work » Needs review

or should it be postponed according to #20?

The last submitted patch, 20: media-wysiwyg-alt-title-handling-2126697-20.patch, failed testing.

johnennew’s picture

Isn't the problem here that nothing implements hook_tokens to provide a token for the alt and title fields?

This could be fixed by changing the token format to that used by entity token and then enabling entity_token. I've supplied a patch which does this but it does put a dependency on entity token. Given the size and scope of this module I suggest this is OK to do (maybe?!)

Status: Needs review » Needs work

The last submitted patch, 24: media-wysiwyg-alt-title-handling-2126697-21.patch, failed testing.

johnennew’s picture

I don;t get the failing tests - is it because of the dependency on entity token?

dsnopek’s picture

@ceng: I don't think that's the issue. This is about the Javascript failing to copy the info from the media token into the title/alt fields when selecting an existing image and clicking the Media button.

I've re-rolled the patch from #21 to apply against the latest Git. It works great for me!

This appears to fix this issue in Panopoly: #2307007: When inserting Image in WYSIWYG, the second set of alt/title fields is ignored

dsnopek’s picture

Status: Reviewed & tested by the community » Needs review

Heh, I take it back! While this patch doesn't cause any harm, it's actually upgrading to the latest Media commit that fixes the issue we were having in Panopoly:

http://cgit.drupalcode.org/media/commit/?id=63824297d512b1993d4448eb851d...

Sorry for the noise!

dsnopek’s picture

Status: Needs review » Reviewed & tested by the community

Blergh! Sorry for all the reversals - it's really difficult to test this stuff. :-/ It appears this fix is, in fact, needed to make sure the alt/title actually make it to get actually displayed when viewing the content outside of the WYSIWYG.

TwoD’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.18 KB

Great work! The behavior is very confusing without this change.
A few notes:

  1. +++ b/modules/media_wysiwyg/js/media_wysiwyg.filter.js
    @@ -61,6 +61,63 @@
    +      for (field in options) {
    

    Missing "var".

  2. +++ b/modules/media_wysiwyg/js/wysiwyg-media.js
    @@ -110,12 +110,15 @@ InsertMedia.prototype = {
    +    var attributes = Drupal.media.filter.parseAttributeFields(formatted_media.options);
    +
         var element = Drupal.media.filter.create_element(formatted_media.html, {
               fid: this.mediaFile.fid,
               view_mode: formatted_media.type,
    -          attributes: formatted_media.options,
    +          attributes: $.extend(this.mediaFile.attributes, attributes),
               fields: formatted_media.options
    

    I moved this into the create_element() method so it'll also be run when editors get attached, and it'll automatically work for the standalone CKEditor plugin.
    The formatted_media.options to this.mediaFile.attributes change stays though, good catch!

I also reworded/expanded the comments a bit to try to clarify why this is done.

TwoD’s picture

FileSize
5.9 KB

Found an issue where it wouldn't sync correctly if you changed the alt/title when first inserting media.

Status: Needs review » Needs work

The last submitted patch, 31: media_wysiwyg.2126697.31.patch, failed testing.

micbar’s picture

@@ -17,6 +17,9 @@ function media_wysiwyg_format_form($form, &$form_state, $file) {
   // Allow for overrides to the fields.
   $query_fields = isset($_GET['fields']) ? drupal_json_decode($_GET['fields']) : array();
   $fields = media_wysiwyg_filter_field_parser(array('fields' => $query_fields), $file);
+  dpm($_GET['fields'], 'query');
+  dpm($query_fields, 'query_fields');
+  dpm($fields, 'fields');
 
TwoD’s picture

Oh, thought I left out that part, sorry, will reroll when I get back home... ;P

micbar’s picture

FileSize
5.13 KB

New patch.

micbar’s picture

Status: Needs work » Needs review
micbar’s picture

I tested the patch in #35 on
-drupal 7.31
-media dev
-file_entity dev
-wysiwyg dev
-ckeditor 4.4.3
-Mac
-Safari

1. If you enter alt and title attributes in the wysiwyg embed form, they are correctly displayed in the browser.
2. If you change the alt and title attributes via the image attributes dialog, the attributes of the embedded image are updated when saving the node.
3. The field values, provided by the file entity itself get overridden by the values entered in the wysiwyg media embed form or in the image dialog.

I think, that this patch works as expected.

bkosborne’s picture

Is the expectation that this would only work with the WYSIWYG module and not CKEditor standalone? Because I can't get this syncing to work with the standalone editor. Trying to dig in.

micbar’s picture

It only works with the WYSIWYG module. The integration of the ckeditor standalone module is handled by other issues.
Ckeditor standalone module integration is provided by the media_wysiwyg submodule. I still uses the buggy old macro handling with attributes forced into the image class.

TwoD’s picture

Thanks for the reroll!

It should theoretically work with standalone CKEditor as well, though I haven't tested it.

@micbar, the media_wysiwyg module is also responsible for the Wysiwyg integration. In fact, all the changes in this patch are made in that module. They essentially use the same code for the filter/placeholder substitution, which is where the new functions get called from, with just some extra glue code in the media_ckeditor module (located inside media_wysiwyg).

micbar’s picture

@TwoD
yes, the media_wysiwyg module is also responsible for the wysiwyg module integration.
the media_ckeditor submodule needs an update. I tried this patch with ckeditor dev. Alt and title attributes are empty. If i click the embedded media file and open the media dialog, the alt and title fields are empty and the view mode is "Default". I think, this has no connection with this patch. It needs a follow-up issue.

tanc’s picture

I've tested the patch at #35 and confirm it doesn't work with CKEditor module. I've created a related case to track progress and fixes here: #2340007: CKEditor -- Alt and Title fields require some special handling.

ec-mdecker’s picture

Patch from #35 worked for for my set up using WYSIWYG module with CKEditor library.

Current Setup
WYSIWYG: 7.x-2.2+46-dev
CKEditor: 4.2.0.f74e558

heddn’s picture

Status: Needs review » Needs work
+++ b/modules/media_wysiwyg/js/media_wysiwyg.filter.js
@@ -61,6 +61,99 @@
+        if (field.match(/^field_file_image_alt_text/)) {
...
+        if (field.match(/^field_file_image_title_text/)) {

These field names are configurable in file_entity module. See the variables file_entity_alt & file_entity_title. The names shoud probably be sent up via a js settings object.

mgifford’s picture

Issue tags: +Accessibility

@micbar are you able to re-roll the patch?

kaare’s picture

FileSize
5.32 KB

Patch applied with some fuzz. Re-rolled without change.

micbar’s picture

Status: Needs work » Needs review
osopolar’s picture

#47 works well for me, thanks.

mgifford’s picture

@osopolar - Think it is RTBC?

@heddn's concerns in #45 don't look like they have been addressed "The names should probably be sent up via a js settings object."

osopolar’s picture

@mgifford It looks fine to me, I am using the patch in development and didn't notice any problems so far.

heddn’s picture

Status: Needs review » Needs work

But the field names are hard-coded. They should be retrieved via a variable_get() and sent up via js settings. Not everyone uses the hard-coded names for alt & title text.

justanothermark’s picture

FileSize
5.48 KB
1 KB

Using the patch in #47 I could override fields to another value but I couldn't override them to empty. When the field was set to override to empty it would always use the field value from the file entity.

The attached patch allows any attribute (in wysiwyg_allowed_attributes) to be overwritten, even if the value evaluates to false, and converts boolean false values to an empty string when copying the values from fields to attributes.alt and attributes.title.

dsnopek’s picture

Issue tags: +panopoly

The patch in #53 works for me!

Leaving at 'Needs work' because @heddn's review in #52 still isn't addressed.

mgifford’s picture

Status: Needs work » Needs review

Just changing the status.

drasgardian’s picture

Status: Needs review » Needs work

I agree with the concerns in #52 about hard-coding the field names.

Prior to applying this patch we were able to use hook_media_wysiwyg_format_form_prepare_alter to add custom fields to the form. e.g. for applying classes.

  $form['options']['class'] = array(
    '#title' => t('Alignment'),
    '#type' => 'select',
    '#options' => array(
      'align-none' => 'none',
      'align-left' => 'left',
      'align-right' => 'right'
    ),
    '#default_value' => isset($query_fields['class'])?$query_fields['class']:'',
  );

This won't work now because parseAttributeFields only allows 'field_file_image_alt_text' and 'field_file_image_title_text'

chrisgross’s picture

It has just occurred to me how critical this issue truly is.

I've been running a distribution using the old dev version of media that panopoly was using until just recently. We had been using patch #27, because without it, alt and title text simply did not work. The tradeoff here is that is that this patch seems to make it so changing the view mode of an image does not wipe out the previous image style class, which means using view modes, which seemed like the only feasible way to float images left and right, is not possible, because the float classes conflict with one another.

After reading comment #56, it has occurred to me that I had tried to do this very same thing, only by adding a field to the image entity, which doesn't work either.

So as it stands, you have to choose between having alt/title text or floating (or adding fields of any kind).

dddbbb’s picture

@chrisgross See https://www.drupal.org/node/2018075 for a related issue regarding image floating - hopefully it may be of some help.

chrisgross’s picture

@danbohea thanks for the link, but it doesn't seem to work, though I also am not using extra fields.

Despite being a stopgap solution, I've found that #53 works fairly well, but is incompatible with the latest patch in 2308487, which references this issue.

chrisgross’s picture

See #16 from the other issue for a compatible patch.

chrisgross’s picture

Also, @drasgardian based on my testing of adding fields to file entities, only fields that are of the list type seem to have the problem where the values don't stick. Text fields and their variants seem to work just fine, which is a bummer as far as trying to float images this way.

chrisgross’s picture

As of beta1, I am no longer having issues with alt and title text when viewing nodes. Alt and title text do not seem to exist as markup within ckeditor, but I don't think that really matters too much. Can anyone else verify this? Also, without the patches in this thread, I don't have issues with old image style classes sticking around when switching styles.

Ah, I see the alt text on the file entity is still overriding the value on the instance. I don't know about you guys, but I'm just going to do a hook_form_alter and disable access to those fields on the base entity form, since the default fields or useless to me. In my use case, only the instance values matter. If the base entity fields are empty, the instance values win every time.

I know it's a workaround and not a fix, but hook_form_alter() is my best friend right now. The form fields are all still perfectly visible in the media browser when editing an existing instance. If your use case is anything like mine, and instance values matter more than base values, this is a more satisfactory solution than the above patches, because they introduce more problems than they solve, the biggest being that they cause the image style/display mode CSS classes to accumulate in the editor when you change them, instead of wiping the the old ones out. I've always found that having these field values editable on two different levels is confusing to users anyway.

/*
 * Implements hook_form_alter().
 */
function YOUR_MODULE_form_alter(&$form, &$form_state, $form_id) {
  if (in_array($form_id, array('file_entity_edit', 'file_entity_add_upload'))) {
    $hide_fields = array(
      'field_file_image_alt_text',
      'field_file_image_title_text',
      'field_file_description',
      'field_CUSTOM_FIELD',
    );
    foreach ($hide_fields as $field) {
      if (isset($form[$field])) {
        $form[$field]['#access'] = FALSE;
      }
    }
  }
}

Additionally, the description might make more sense to include only on the base entity and not the instance, in which case, you might want something like this:

 if ($form_id == 'media_wysiwyg_format_form' && isset($form['options']['fields']['field_file_description'])) {
    $form['options']['fields']['field_file_description']['#access'] = FALSE;
 }

Hopefully this is useful for someone. Even if this bug does get fixed, I sort of like this method better since it reduces apparent redundancy from the perspective of less technical users and allows you to decide which fields make more sense being effectively "attached" to either the fields base or instance. Not necessarily a one-size-fits all solution and I know why it may not be desirable for everyone but it works great for me.

Note that this is not ideal if you plan on accessing the base file entity directly, such as building a view, and want there to be alt and title text fields available.

osopolar’s picture

@chrisgross: The patch from #53 didn't work for you?

rwohleb’s picture

I've re-rolled the patch from #53 with support for variable alt/title fields. The challenge is that the file_entity module lets you specify strings, with optional tokens, for those alt and title variables. As a workaround, the code now looks at those variables and tries to extract the field name if they only contain a single token and no extraneous text. Documentation will have to be written explaining how to set those file_entity settings correctly.

The potential downside is that if someone wants to use the file_entity handling of alt/title somewhere, they don't have the room to maneuver. However, If they are using the media module, I'm not sure why they would.

chrisgross’s picture

@osopolar it works for the most part, but as I mentioned above, it also introduces a bug where old display mode classes aren't removed when you switch to a new one, which for my purposes, is a deal breaker.

thtas’s picture

Here is my attempt to fix the issue raised in #65

thtas’s picture

vistree’s picture

Hi thtas,
great job. It seems to work for me. Even custom view modes are prevented!

rooby’s picture

Is this still needs work due to #52 or has that been fixed?

rwohleb’s picture

We are running the fix in #64 on a large production site without issue, other than the view-mode class duplication. We've been manually correcting that at present. Thtas adjustment in #66 seems to have fixed that remaining issue based on my testing so far.

vzblk’s picture

Fixes for the latest version

  • joseph.olstad committed cf8e119 on 7.x-2.x authored by vzblk
    Issue #2126697 by thtas, kaare, TwoD, kaidjohnson, justanothermark,...
joseph.olstad’s picture

fixed in 7.x-2.x dev branch

joseph.olstad’s picture

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

Note,

@Skylord on Cannot override fields after beta3 update

found a regression with patch #71

here's the interdiff between #66 and #71

here's the related issue, I'll commit the fix in the related issue.
#2823547: Cannot override fields after beta3 update

so basically saying, #66 was the better one as the fix is basically to undo the interdiff between 71 and 66.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.