media_vimeo and media_wysiwyg tokenhandling broken except when using the dev version of wysiwyg

yikes

Did you try 7.x-2.x dev before downgrading? Recent fixes for token issues

I was seeing something similar when using media_ckeditor and media_oembed to insert YouTube videos. Even with the rich-text being encoded and decoded properly, the placeholder HTML was generated like:

<video controls="controls" preload="auto" alt="alt text for my video" class="media-element file-large cke_widget_element" data-delta="1" data-fid="130" data-media-element="1" data-cke-widget-data="%7B%22classes%22%3A%7B%22file-large%22%3A1%2C%22media-element%22%3A1%7D%7D" data-cke-widget-upcasted="1" data-cke-widget-keep-attr="0" data-widget="mediabox" width="480"><source src="https://youtu.be/some_video_id" type="video/oembed"></video>

When swapped to source view (or saved), this was NOT properly converted to a JSON macro, which meant that the page and associated media could never be edited without working only in source mode. Worse, and as I suspect you found with media_vimeo, it's not even valid video HTML, because the OEmbed call to retrieve the actual HTML hasn't happened yet. This means that the content never works, and never can.

I had similar problems when trying to embed PDF documents that use PDFPreview in their view mode: *those* use div containers with nested divs, which were also failing to be converted to macros, and failed even more spectacularly when swapped between source and wysiwyg mode.

So: there *is* a workaround for this if you're using media_wysiwyg, which is to specify a special view mode for use in Wysiwyg editors which just renders a plain image as the placeholder for each file type. That image can be successfully swapped to and from the file macro, which means the page works OK when viewed, and the file can be edited with the media browser. It does make checking the layout tricky in edit mode, though.

The actual problem seems to be the replacePlaceholderWithToken function in media_wysiwyg.filter.js. It is trying to use regular expressions to parse media placeholders out of the wysiwyg HTML, and this is set up to only find placeholders that are img or span tags! Div, video, audio and any other container tags are not recognised at all, causing macro substitution to be skipped. If you have something like nested spans, then the regex will not identify the complete container: only up to the end of the first , which is not going to be the last one: it then substitutes the macro for only *part* of the placeholder, which makes the generated HTML not parse at all (this is arguably worse). A comment in the script notes that this is not an ideal approach.

I've attached a patch that uses JQuery to parse the wysiwyg content and identify the media placeholders, instead of the regex. It recognises *any* placeholder element which has class="media-element"

New version of the patch with proper whitespace handling (the last one had some rogue tabs which misaligned a couple of statements).

it'd be nice to get this fix into beta11 .

@fuzzy76 you reported in #11 that this worked, but then in #12 state that YouTube is broken, can you please confirm what exactly was fixed as described in #11? (vimeo fixed? anything else fixed that was broken?)

@fuzzy76 do your youtube placeholder elements have the class 'media-element' ? (see comment #10) also @LittleRedHen says there is a workaround for this.

So: there *is* a workaround for this if you're using media_wysiwyg, which is to specify a special view mode for use in Wysiwyg editors which just renders a plain image as the placeholder for each file type. That image can be successfully swapped to and from the file macro, which means the page works OK when viewed, and the file can be edited with the media browser. It does make checking the layout tricky in edit mode, though.

I guess we still need some feedback on this one, but in the meantime, @LittleRedHen: Do you think there is any way to do this without requiring jQuery 1.8? Even if the code is clunkier, I think it would be worth it to avoid having to require a minimum version of jQuery.

@brockfanning , the patch that @LittleRedHen wrote is compatible with lower jQuery versions , have another look at the patch, the line in question is //commented out , I think he left that line there intentionally so that others would not try to use the >= 1.8 function.

Ah, my bad, cool.

@brockfanning, @joseph.olstad: It definitely works on JQuery 1.7, but I haven't tested lower versions than that.

The $.parseHTML line is in there because I did test its use: it's preferable in many ways because it doesn't execute any embedded scripts by default, but for patching purposes, I figured it wasn't appropriate to increase the installation complexity. There's therefore a possibility in that someone could potentially use source mode to add scripts that break the submission, since the source is parsed here before the server-side text filters get applied (assuming those have been properly set to strip out embedded script tags).

It's a client-side vulnerability rather than a server-side one, though: as far as I can tell, it's possible to break the page, but the filters *will* still get applied before anything is saved, and so there's no additional risk to the system. In addition to that, if the CKEditor Advanced Content Filter is on (media_ckeditor install guidelines current recommend that it be turned off), then that could filter out script tags before reaching this function.

I can re-roll the patch without the comment if that's misleading?

@LittleRedHen: I believe out-of-the-box Drupal is at jQuery 1.4.4, so if possible let's try to get this to work there. (If not possible, we may have to add jquery_update as a dependency for media_wysiwyg). It's not liking the prop() method. I manually changed "prop" to "attr" and it worked OK. I'm not sure if that would cause an issue in more recent jQuery versions.

With that change, though, this is working for me in all three of the "streaming" use-cases I know of: media_oembed, media_youtube, and media_vimeo. @fuzzy76 we may have to investigate what's going on with your media_youtube testing. Can you try clearing your browser's "Cached images and files" and see if that helps? If not, any idea if there are javascript errors going on?

@brockfanning: there is an existing open issue about jQuery versions and use of 'attr' which should probably be taken into account as well: https://www.drupal.org/node/2416839

My site requires jQuery 1.7 as a minimum due to use of an unrelated module, so I'm not set up for testing anything less than that, but I can confirm that $(this).attr('outerHTML') does NOT work with 1.7: which makes sense, since both of those are really accessing the element.outerHTML DOM property, rather than an actual 'outerHTML' attribute on the markup (jQuery < 1.6 was hazy about the difference, but 1.6+ are not).

Here's a re-rolled version of the patch that fetches the outerHTML the olde-school way: via the DOM element property if possible (for performance reasons) and by cloning and wrapping the object, then fetching the content of the wrapper if that property doesn't exist (mostly very, very old browsers but better safe than sorry).

If you *do* end up choosing a dependency on jquery_update, then this chould be changed back to use the simpler .prop() syntax. And maybe consider going to 1.8+, so that parseHTML is available as well!

test results by @fuzzy76 on a vanilla installation show that this patch is backwards compatible with out of the box drupal 7.x jQuery, meaning it IS compatible with >= jQuery 1.4

Excellent work @LittleRedHen

Any objections?

in 7.x-2.x dev

@fuzzy76
I am having another closer look at: replacePlaceholderWithToken
it is called "when a rich text editor detaches"

thanks for your description, should help locate the annoyance

Hi @fuzzy76 ,

In this patch I've backed off/reverted all changes in the related function replacePlaceholderWithToken since beta2

Not sure if the older regex is still compatible but it is worth testing.

@LittleRedHen any ideas?

Before we revert anything let's make sure the issue can be recreated. I'm not seeing the same thing, @fuzzy76, when I try putting content after the embed. For me it's working fine. Here is a drush make recipe for the setup I'm using to test:

---
api: 2
core: "7.52"

projects:

  # The Media module.
  media:
    download:
      type: git
      branch: dfd1968
 
  # The WYSIWYG module.
  wysiwyg:
    download:
      type: git
      branch: 507c503

  # Other required modules.
  media_oembed: "2.7"
  ctools: "1.12"
  file_entity: "2.0-beta3"
  token: "1.6"
  views: "3.14"

# The Ckeditor library.
libraries:
  ckeditor:
    download:
      type: "file"
      url: "http://download.cksource.com/CKEditor/CKEditor/CKEditor%204.6.0/ckeditor_4.6.0_full.zip"

Does any of that look different from what you have? This is what I'm doing to set up the testing site: (NOTE THAT THIS WIPES THE SITE, SO DON'T DO THIS ON AN ACTUAL SITE)

drush site-install
drush en media_wysiwyg wysiwyg token media_oembed

And then these manual steps:

1. Configure a text format so that only this checkbox is checked: "Convert Media tags to markup"
2. On "/admin/config/content/wysiwyg" switch that format to "CKEditor".
3. After saving, click "Edit" to configure the profile.
4. Check "Media Browser" and save.

@fuzzy76 , the ball is in your court for now. I'll follow up later.

ok @fuzzy76, someone needs to honk some big loud horns and get wysiwyg maintainers to publish a new version asap, meanwhile if we wanted we could write some code in the .install file for media_vimeo or 'media.install' to check that IF you are using wysiwyg module that it MUST be the dev version and NOT the 4 year old "stable" (sic) release.

hiding the 'revert' patch as I don't think it is necessary. I am unable to reproduce the tokenhandling issue as reported by @fuzzy76

A wysiwyg release is coming very soon from email I got from the maintainer. Hope he is true to his word.

Please open a new issue if there's any new concerns.