Problem/Motivation

The JavaScript part to deal with the media token filter depends quite a bit on the WYSIWYG module.
This makes it harder than necessary to integrate proper media token support into other modules (e.g. ckeditor module).
Further the JavaScript to use changed between the major media versions, what makes it even more complex to provide proper media token support for other modules.

Proposed resolution

I propose to split the JavaScript into two parts:
* A Media token library
* A WYSIWYG module integration

At the same time we could streamline the media token library to expose the same functions for Media 1.x and 2.x.

Remaining tasks

reviews needed - some testing was already done in the related issue: #1504696: Integration with CKEditor module

User interface changes

none

API changes

As there isn't an proper API as of now (as far as I know) we should document how to use the media token javascript library.

#1504696: Integration with CKEditor module

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, media.filter-js-1.x.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
10.48 KB

media.filter.js was hardcoded to Only local images are allowed. tags which completely breaks inserting video. In fact any regexp breaks because the formatter could be anything.

There's already the tagmap which has all of these in it, so we can just use that instead.

douggreen’s picture

FileSize
11.52 KB

updated patch

Status: Needs review » Needs work

The last submitted patch, media_filter_1970370_3.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
11.09 KB

Fixed paths in patch. Patch was created in the docroot instead in the module folder.

Dave Reid’s picture

I think this is working, I don't see any regressions in using the WYSIWYG. I'm still nervous about committing something which changes a large chunk of JS without any peer review. If someone else can confirm this is good, that would be appriciated.

saltednut’s picture

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

Looks like it is not quite working with CKEditor module and CKEditor 4.1 - I had to apply the patch for CKeditor #1504696: Integration with CKEditor module and also the patch from #5.

I get as far as uploading or picking a media asset from the modal and setting up embed settings, but when submitting, it appears the full markup is not making it into CKEditor. This seems to be an issue with the CKEditor module or CKEDitor 4.1 not supporting the media tags.

Screen Shot 2013-08-15 at 3.25.14 PM.png

I also attempted this with CKEditor library version 3.6 and the WYSIWYG module - in this case, everything works as expected. I don't see any regression and I think it is safe to move forward and follow up with the CKEditor module/CKEditor 4.1/Media 2.x integration in #1504696: Integration with CKEditor module

Dave Reid’s picture

Status: Reviewed & tested by the community » Fixed

Ok committed #5 to 7.x-2.x. Let's follow-up in the CKEditor issue queue then.

http://drupalcode.org/project/media.git/commit/3cb8d2c

das-peter’s picture

Thank you, thank you so much! Committing that was a very important signal. I'll provide feedback & support in the other issue asap!

das-peter’s picture

While I tried to reproduce the issue mentioned in #7, which I couldn't, I came across another issue.
Follow-up and patch created: #2066859: Removed media are restored on save in WYSIWYG

Anonymous’s picture

Status: Fixed » Needs work

This actually breaks WYSIWYG + CKeditor 3. The Media filter token is not inserted into the tagmap because the attach callback is called while CKeditor is inserting the placeholder HTML.

The output in the saved text field is like this:

<p><img alt="" class="media-element file-default" data-file_info="%7B%22fid%22:%223%22,%22view_mode%22:%22default%22,%22fields%22:%7B%22format%22:%22default%22,%22field_file_image_alt_text%5Bund%5D%5B0%5D%5Bvalue%5D%22:%22%22,%22field_file_image_title_text%5Bund%5D%5B0%5D%5Bvalue%5D%22:%22%22%7D,%22type%22:%22media%22%7D" height="199" src="http://media.localhost:8080/sites/default/files/IMG_0176.JPG" width="519" /></p>

This makes it look like it's working, but it's not actually rendering a file entity.

saltednut’s picture

FileSize
604.03 KB

double post

saltednut’s picture

FileSize
58.02 KB

Yep - I can confirm. THought this was working per visual inspection but did not realize the code snippet was incorrect.

Visually, looks correct:
ckeditor-3-visual.png

Oops, code is wrong:
ckeditor-3-actual.png

das-peter’s picture

Here's a patch to fix that (Attention: patch also contains #2066859: Removed media are restored on save in WYSIWYG).
Changes:

  • Drupal.media.filter.getWysiwygHTML() returns the wrapped HTML to insert into the WYSIWYG and registers macro and markup. You can do this still by yourself but why should you.... I'll update the ckeditor patch to use this new function asap.
  • "Redirected" functions in wysiwyg-media.js to the new ones provided by Drupal.media.filter
  • Contains fix from #2066859: Removed media are restored on save in WYSIWYG

Tested with:
- WYSIWYG latest dev
- CKEditor 3.6.6.1 (>=4.1 doesn't work as of this #1956778-14: Ckeditor 4.1 ACF)

Anonymous’s picture

Status: Needs work » Needs review

I'm confirming that the patch in #14 fixed 3.6.6.1.

The patch plays with the tag map before inserting the HTML. When the HTML is inserted, CKEditor was firing another event which was causing the "attach" function in the WYSIWYG plugin to run before the tag map was up to date.

Dave Reid’s picture

Commmitted #14 to 7.x-2.x. Thanks again everyone! http://drupalcode.org/project/media.git/commit/b5e1141

Dave Reid’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

ParisLiakos’s picture

this commit seems to be the reason for a number of regressions/bugs #2067063: Wysiwyg integration is broken :(