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

Files: 
CommentFileSizeAuthor
#14 media_filter_1970370_14.patch6.14 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch media_filter_1970370_14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 ckeditor-3-visual.png604.03 KBbrantwynn
#13 ckeditor-3-actual.png58.02 KBbrantwynn
#7 Screen Shot 2013-08-15 at 3.25.14 PM.png84.93 KBbrantwynn
#5 media_filter_1970370_3.patch11.09 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 92 pass(es).
[ View ]
#3 media_filter_1970370_3.patch11.52 KBdouggreen
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch media_filter_1970370_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 media_filter_1504696_35.patch10.48 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 92 pass(es).
[ View ]
media.filter-js-1.x.patch19.23 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch media.filter-js-1.x_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
media.filter-js-2.x.patch15.09 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 92 pass(es).
[ View ]

Comments

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new10.48 KB
PASSED: [[SimpleTest]]: [MySQL] 92 pass(es).
[ View ]

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.

StatusFileSize
new11.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch media_filter_1970370_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

updated patch

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new11.09 KB
PASSED: [[SimpleTest]]: [MySQL] 92 pass(es).
[ View ]

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

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.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new84.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

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

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

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

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.

StatusFileSize
new604.03 KB

double post

StatusFileSize
new58.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

StatusFileSize
new6.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch media_filter_1970370_14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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)

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.

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

Status:Needs review» Fixed

Status:Fixed» Closed (fixed)

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

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