Alt and Title double encoded in WYSIWYG

This patch looks good. I'm testing it locally. It solved the issue and I haven't seen any unintended effects.

This looks like a duplicate of #2291527: Content not properly escaped for use in JSON when inserting media in WYSIWYG.

Entities became double-encoded for me because the WYSIWYG editor interpreted parts (field values in my case) of the macros as markup, because they weren't properly escaped to begin with. This started a sort of chain reaction where the editor would insert closing tags to compensate for broken markup, the Media WYSIWYG module would (badly) escape the closing tags which ended up where field values were encoded, the editor again thought the markup was broken because the closing tag was now escaped so it inserted a new one, and the problem grew as the user toggled the editor on and off.

Note, when testing the patch I posted in the above issue, please make sure you insert a new image (and thus get a "fresh" macro generated), as old macros may still contain badly escaped markup. When running editors through Wysiwyg module, this may not be an issue since Media Wysiwyg module is allowed to transle macro tags to placeholder images before the editor notices the "bad" markup, but start out fresh just to be sure.

My patch in the other issue also decodes the attributes - but also fields - like @dsnopek's patch does (but using Drupal's decode_entities()), and makes sure the macro won't ever be interpreted as markup.

Sorry for the delay, missed your comment.

The other issue is poorly named. They just mean the serialized contents of the media token/macro.

If you add a format enabled field to a Media type and put some markup in there, the markup itself won't be escaped/encoded when printed as part of the Media token. This confuses the heck out of browsers and WYSIWYG editors because they don't know to ignore <p>[[{type:"media",fid:123,fields:{some_field:"<p>My <em>weird</em> caption with markup &amp; stuff in it</p>";},attributes:{title:"Foo",alt:"Bar"}}]]</p> (leaving out a few quotes and things for clarity) as a plain piece of textual content inside a paragraph but sees another paragraph in the middle with some text nodes around it. The ampersand was likely already properly escaped if you're using a WYSIWYG editor, but even if it wasn't it wouldn't actually hurt the markup parsing here. The paragraph tags aren't escaped because WYSIWYG editors are unlikely to be expected to output escaped markup unless the user is actually posting example code.

Whenever a string like that gets inserted into a DOM it gets parsed and actually split into those nodes. Later the editor serializes it back out to markup looking something like <p>[[{type:"media",fid:123,fields:{some_field:"</p><p>My <em>weird</em> caption with markup &amp; stuff in it</p><p>";},attributes:{title:"Foo",alt:"Bar"}}]]</p>.

When the Media plugin - parsing the contents as a plain string - rips out the token (anything it sees wrapped in [[{...}]]) the next time content is being edited and puts in a placeholder image, it parses anything it found between those markers as JSON object to populate the dialog box fields. That object ends up looking something like this:

{
  type: "media",
  fid: 123,
  fields: {
    some_field: {
        "</p><p>My <em>weird</em> caption with markup &amp; stuff in it</p><p>"
    }
  },
  attributes: {
    title: "Foo",
    alt: "Bar",
  }
}

For every time the Media token gets serialized and parsed without proper escaping, extra "layers" of paragraph tags will be added to fix "broken" elements.

If all the field (and attribute) values are properly escaped before being output in the token, this doesn't happen.
The above example should look like [[{type:"media",fid:123,fields:{some_field:"<p>My <em>weird</em> caption with markup &amp; stuff in it</p>";},attributes:{title:"Foo",alt:"Bar"}}]]

. Note how the ampersand is now double-escaped because the whole field was escaped before being written to the token.

This prevents the browser and editors from parsing any part of the token as markup, so only one node is created when inserting it into the DOM. The parsed JSON object stored in memory while the placeholder is used now looks like this:

{
  type: "media",
  fid: 123,
  fields: {
    some_field: {
        "<p>My <em>weird</em> caption with markup & stuff in it</p>"
    }
  },
  attributes: {
    title: "Foo",
    alt: "Bar",
  }
}

Just unescape the fields before writing them to the textarea in the Media dialog and the WYSIWYG editor (or user) sees normal markup.
(Hmm, technically, I think I opted for storing the unescaped version in memory after parsing token values, for convenience, but that's just an implementation detail.)

I don't think this issue should be closed I needed the following patches to resolve issues around encoding and it also looks like Panopoly (and derivatives like Open Atrium) also need these patches:

projects[media][patch][2126697] = http://drupal.org/files/issues/media-wysiwyg-alt-title-handling-2126697-27.patch
projects[media][patch][2308487] = http://drupal.org/files/issues/media-alt-title-double-encoded-2308487-1.patch

Attaching patch for alpha4 compatibility.

This might be a duplicate of #1987568: Alt and Title Text &quot;. Can we retest and see if it is still an issue with the latest version of file_entity?

The patch from the mentioned issue doesn't resolve the issue. Nor does the one here either. Let's try something a little more aggressive. Note, I had to do the nasty js stuff so the markup would get html rendered, otherwise the double encoding appears in the WYSIWYG.

I found this problem is resolved with the most recent file_entity version = "7.x-2.0-beta1", and for us it still existed with file_entity version = "7.x-2.0-alpha3+30-dev".

I can still verify this issue exists in both the latest stable and dev of file entity.

@sylus does the patch address this issue? @heddn's patch still applies nicely.

It works for the title as attribute of the <img> tag, but when showing also the title beside the image (enabled in manage display settings of the file field, format plain text) the special characters like <>&where removed instead of encoded.

FYI, this patch no longer plays nice with the latest patch in #2126697

Here is an updated patch that is compatible with #53 from #2126697. This will only work if the other patch is applied first.

Ok.. Still marking it as needs review.

FYI, #16 failed testing, but that is because it relies on another patch. It's still safe to use in conjunction with the other. Obviously, the better solution is to get these changes to a state where they can safely and reasonable be committed.

@chrisgross Absolutely. Would it be better to merge the two issues? I'm really not sure how to get around @drasgardian concerns in the other issue #2126697-56: Wysiwyg -- Alt and Title fields require some special handling.

@mgifford perhaps. However, the issue @drashardian is having is not specific to the form fields added programmatically. It also happens when adding fields with file_entity. It only seems to happen with non-text based fields (select, checkboxes, etc). See my comment here: https://www.drupal.org/node/2126697#comment-10106758

fixed in 7.x-2.x dev branch

@heddn: This issue is closed, but in another issue, #2833205: Allow for some escaping when overriding fields in WYSIWYG, I have a need to revert one of the lines from your original patch in #9 here. This is the line:

element.attr(a, $('<textarea />').html(info.attributes[a]).text());

Which I need to change back to:

element.attr(a, info.attributes[a]);

Just wanted to get your take on that change. It's needed because otherwise special characters can't be escaped before going into alt/title or other attributes. So for example, a & character can make it into an alt unescaped, which prevents CKEditor from recognizing the token as a widget. Any thoughts about this?

re #26: The nasty js stuff on that line could be improved. I seem to recall facing this as an issue in ckeditor for something else and there is another method to do the same thing but not break things. I don't recall what that approach was off my head.

Cool, thanks @heddn.