I have been poking around at wysiwyg-media.js in an attempt to get inline audio embeds working.

It seems inline media is broken for all media types except for images. This has been documented/discussed in a couple of places already (#1283844: [meta] Improve WYSIWYG integration and #1062948: Issue getting Media, Styles and WYSIWYG working together with MediaElement.js). There also appears to have been a few proposals to overhaul the inline media approach #1291518: Use Field Formatters for Embedding Options and http://drupal.org/sandbox/dman/1075470.

I think the above proposals are definitely worth discussing further however, in the meantime it would great if this would "just work".

The breakage appears to be caused in a couple of places where things are very image-specific, particularly insertMediaFile() and createTag(). It doesn't look like it'll be much work to clean these up. In the interest of not breaking stuff that currently works, I am wondering if anyone who is familiar with the history of these methods could weigh in on why they do some of the things they do.

Particularly:

  • stripDivs(): Seems like this exists to work around some wysiwyg eccentricities, though has been written to be specific to images. Should this apply to other elements?
  • createTag(): provides special handling for IE and token replacements for positioning inline images and setting width/height attributes - maybe this can be moved into a image handler?
  • attach() and addImageAttributes(): It looks like all of the inline macro code has been written with only images in mind, the macro format should probably be generalized a little bit.

I have made a start on this, will post patches tomorrow.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tnightingale’s picture

I have finally reviewed all of the wysiwyg integration code. From what I can see in the issue queue and git log it seems that it is mostly legacy code that hasn't had much attention throughout the development of media module.

Attached is a patch with a full overhaul of the javascript that I hope achieves the following goals:

- Simplify the WYSIWYG attach/detach (macro -> markup -> macro) process.
The media item's placeholder markup needs to track data relating to the file that it represents so that it can be easily be replaced with an inline JSON macro. Currently this information is being manually serialized into a set of custom attributes and 'special' class names.

This patch takes a different approach by urlencoding the JSON object and attaching it to the element as a html5 data-* attribute. This makes the format of the file info data more flexible and will hopefully make implementing Dave Reid's field formatter proposal (http://drupal.org/node/1291518#comment-5611652) a little easier.

- Clean up the code
wysiwyg-media.js is a mess of code trying to do several different things at once.
1) Register a wysiwyg plugin.
2) Invoke the media browser and handle the user's file selection.
3) Manage placeholder markup for display within the wysiwyg and generate JSON macro's for replacement.
The patch tries to separate these tasks and make it a little easier to understand from a developer's perspective. In doing so, some bugs around variable scope and variable hoisting have been resolved.

A couple of notes:
I tried very hard to figure out a way of allowing any markup to be used as a media item placeholder in the WYSIWYG. I eventually gave up with the conclusion that this is simply not possible without using DOM parser provided by one of the WYSIWYG editors (both TinyMCE and CKEditor provide parsers). Parsing html with regular expressions is just not cool (see http://stackoverflow.com/a/1732454/854985).
As a result I have hard-coded 'media_preview' to be the view-mode used as a placeholder in the WYSIWYG. The code assumes that the 'media_preview' view-mode has been configured to output a image representation of the media item.

Scattered through the current code are references to image attributes (width, height, positioning) and the storing/fetching of these attributes. I have removed this code as it is specific to images and should be managed by either: some sort of specific media-type handler mechanism or configured in the field formatter settings (http://drupal.org/node/1291518#comment-5611652).
I am yet to remove any image attribute handling from the PHP side of things (media.filter.inc), though I did add some simple error handling as if breaks if the expected values don't exist.

This patch still needs to be tested on oldIE, the only thing I can see that might be problematic is the storage and fetching of data attributes.

tnightingale’s picture

Status: Active » Needs review
becw’s picture

Assigned: tnightingale » becw

This exact task was my DrupalCon goal, so I'll review this patch during the code sprint tomorrow.

becw’s picture

The patch from #1 no longer applies; here's a re-roll.

Also, the first issue I'm seeing with this is that the WYSIWYG placeholders for media don't render with the correct view mode--this is a regression from the current behavior.

tnightingale’s picture

the first issue I'm seeing with this is that the WYSIWYG placeholders for media don't render with the correct view mode

While I agree, it's not ideal, this was deliberate. As I mentioned above, I spent a long time looking at ways to allow for any arbitrary html markup to be used as a placeholder in the WYSIWYG (e.g: <video> and <audio> tags) while also remaining WYSIWYG library agnostic.

By staying WYSIWYG agnostic we are limited to using regular expressions to target placeholder markup in order to swap out for macros on editor detach. Parsing HTML with regular expressions is an exercise in insanity. I did experiment a little bit with the DOMParser object available in webkit and dev versions of firefox along with the polyfill included here https://developer.mozilla.org/en/DOMParser but found browser support was extremely patchy.

Another issue with using anything but img tags as placeholders, is that there is nothing stopping the editor from injecting text/markup into or rewriting your placeholders. Both CKEditor and TinyMCE will automatically place the "cursor" inside div or span elements and to make things even trickier, if you then insert a paragraph (by pushing 'return') they will duplicate the wrapping elements and all attributes applied to them.

As a result of all of this I hard-coded 'media_preview' to be the view-mode used as a placeholder in the WYSIWYG and it assumes that 'media_preview' has been configured to output only an image tag. I'm totally welcome to better suggestions :)

Thanks for the review, I am looking forward to getting this resolved once and for all!

becw’s picture

So, the WYSIWYG placeholder should be an image and nothing else--no markup, no caption text. Ultimately we want to render it to real markup via the filter, and users should not be able to directly alter that markup in the WYSIWYG. However, the WYSIWYG placeholder can approximate the size of the media with the particular view mode, and we also have existing code to pass media dimensions (the height/width attributes, or the style="height: y; width: x;") set via WYSIWYG through to the file display renderer. This behavior is present in the current WYSIWYG integration, so I would prefer to keep it.

I think the main issues with the patch in #4 are:

  • WYSIWYG placeholders don't render with the correct view mode. Image placeholders should use the selected view mode's image style, video previews should use the thumbnail with an appropriate view mode, and other file types should use media's media type icons.
  • TinyMCE and CKEditor allow users to interactively resize images. These changes need to be reflected when the file is displayed.
  • Video gets inserted in the WYSIWYG with the file name.
  • For videos, the file view mode set when inserting in the WYSIWYG doesn't get passed through to render.

Otherwise, I think the js in particular is a vast improvement on the current implementation :) I'm going to work on addressing these things this afternoon.

tnightingale’s picture

Keeping ability to resize images via WYSIWYG++. Though as the WYSIWYG allows this to be done to any image, I'm not sure how we would prevent other file type's placeholder images from being similarly resized (we may just ignore the alterations in when rendering).

For these two points:

  • Video gets inserted in the WYSIWYG with the file name.
  • For videos, the file view mode set when inserting in the WYSIWYG doesn't get passed through to render.

From what I recall in the initial patch, the placeholder markup inserted into WYSIWYG is whatever has been configured to display for that media type's 'media_preview' view mode, for videos I think this is the file name by default - but this is configureable - I think the issue is whether it should be configurable or not.
Regarding the second point, not sure why that would be the case only for videos as the code doesn't discriminate by type anywhere.

becw’s picture

Status: Needs review » Needs work
FileSize
25.54 KB
  • Video gets inserted in the WYSIWYG with the file name.

This must be because of some configuration thing with the "preview" view mode on my dev site. However, we don't need to rely on the "preview" mode being properly configured: Media passes a wysiwyg => 1 flag in the settings array when calling media_get_file_without_label(), and so we get an image placeholder back for any media type that approximates the selected file view mode.

Here is an updated patch that addresses the issues from #6, and also addresses backwards-compatibility. I realized that media size adjustments in the WYSIWYG don't get applied by formatters even with the current code, but this patch at least stores those attributes in the filter macro so that they can be used if the formatters are adjusted.

This patch probably needs some more poking; my additions to the javascript made it somewhat messier. I'll spend a little more time with it today before I mark this "needs review" again.

tnightingale’s picture

Awesome! Great to see some progress on this. I can probably make some time over the weekend to review a patch or two if you get to that point. :)

becw’s picture

Assigned: becw » Unassigned
Status: Needs work » Needs review
FileSize
26.02 KB

Here's an updated patch. Changes from #8:

  • rearranged the js so that the WYSIWYG API plugin is at the top of the file
  • implemented the WYSIWYG API isNode method
  • updated the WYSIWYG API invoke method so that if you select media and then click the media button in the toolbar, you can change the view mode of the selected media

This patch is related to the following issues:

This patch is ready for review, and should especially be tested with existing content.

nagy.balint’s picture

I've tried this patch. I've got 3 view modes (Original, Default, Preview). I set the Preview and Default to not do anything just output the placeholder image, and set Original to be the MediaElement Audio player.
Now if i select Original (when inserting the mp3) the system tries to output the audio tag (Original view mode) instead of the placeholder image (in wysiwyg). So i checked and it seems that media_get_file_without_label function in media.filter.inc does not do anything with the 'wysiwyg' attribute of the $settings variable. I thought that that function should return the placeholder always when its in a wysiwyg instance, and return the actual mediaelement player when not.

If i put in this if ($settings['wysiwyg']) $view_mode = 'preview'; line just before the file_view_file function there, then it works for me i see the placeholder in the editor and the player outside.

Am i missing something?

davidseth’s picture

@nagy.balint can you provide a patch for your changes? Thanks.

becw’s picture

I think this might be a place where integration is lacking in the whatever is rendering the audio--we shouldn't force the WYSIWYG placeholder view mode to 'preview' for all WYSIWYG media, because then we don't get the correct display for images and video.

davidseth’s picture

@becw, I was able to apply the patch with some success. The video file type seemed to work as it should and image has always been pretty decent. Where I ran into a lot of problems was with application and audio file types. The audio rendered as a link on node save (as it should) but then was kept as a link and when I went to edit the node it was just a .

nagy.balint’s picture

@becw, I agree, but we need to somehow get the placeholder image from media_get_file_without_label when we are in a wysiwyg (and lets say its an audio or video file). So either we change the view mode on the file_view_file() function, or we call something else.

There is a media_get_thumbnail_preview() function in media.module (2.x dev), and that simply asks for a file_view_file with media_preview view mode.

But even then there are a couple of issues:
- for some reason i cannot select view mode for videos when i insert them they get inserted as media_large
- the placeholder has a fix size (ie 180x180), it would be better if somehow i could apply an image style on it, cause lets say the player is 300x30, then it d look better if the placeholder is 300x30 as well.

becw’s picture

@davidseth & @nagy.balint Do these issues exist with the current WYSIWYG javascript, too? If so, audio and application representation in WYSIWYG deserves its own issue, especially since it reaches into non-js parts of the code.

Johnny vd Laar’s picture

Attached is a patch updated for the latest dev version

Status: Needs review » Needs work

The last submitted patch, 1451316-17-media-wysiwyg_integration_cleanup.patch, failed testing.

Johnny vd Laar’s picture

hmm strange... I created the patch based on the dev version... what went wrong?

ps the fix works for the WYSIWYG module but not for the CKEditor module. In the CKEditor it only shows an icon. no links

becw’s picture

@Johnny vd Larr, the patch has paths like a/modules/media/includes/media.filter.inc instead of a/includes/media.filter.inc. If you have a git checkout of media, you can do "git diff" from the media directory and get the correct paths.

For me, the patch from #10 applies cleanly to the latest -dev.

Johnny vd Laar’s picture

ok lets try again!

Johnny vd Laar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1451316-21-media-wysiwyg_integration_cleanup.patch, failed testing.

Johnny vd Laar’s picture

Status: Needs work » Needs review
FileSize
25.67 KB

ok I suck but I think this should work :(

roma4ke’s picture

ParisLiakos’s picture

Status: Needs review » Needs work

i tried both #10 and #25 and still do not fix #1280758: The WYSIWYG integration in media destroys <script> tags when the WYSIWYG instance is detached

Uncaught TypeError: Cannot read property 'autosubmit' of undefined media.format_form.js:26

when submitting the file through wysiwyg media plugin in the editor.
hope this helps

then when viewing the node, there is nothing for pdf files..for img files just the icon, no link to the file:

<img class="file-icon media-image img__fid__2 img__view_mode__media_link attr__format__media_link" title="image/jpeg" src="/d7/modules/file/icons/image-x-generic.png" alt="">

EDIT:
Actually, above is true for existing content..for new content, image works, but pdf still just gets the icon

agoradesign’s picture

Regarding your PDF problem: check the file display settings at /admin/structure/file-types for Application (multipurpose). You can enable there different displays for each of the display modes (link, preview, small,...). Afaik there's nothing enabled per default for this file type

ParisLiakos’s picture

ax, it is in application! not other...but still i enabled the generic file for link..when inserting a pdf into editor and selecting link display, i only get the icon

agoradesign’s picture

Strange... for me it works. I've also enabled the "generic file" display. Is the field set to visible in the "manage display" tab (admin/structure/file-types/manage/application/display/media_link)?

Have you enabled the "Convert Media tags to markup" filter? (admin/config/content/formats)

agoradesign’s picture

PS: just to be sure: are you using the Wysiwyg Module or the CKEditor module? While CKEditor module can handle images, the integration does not work for PDF's, but for Wysiwyg it works with the patch from this thread.

ParisLiakos’s picture

Strange... for me it works. I've also enabled the "generic file" display. Is the field set to visible in the "manage display" tab (admin/structure/file-types/manage/application/display/media_link)?

yes, it was visible by default

Have you enabled the "Convert Media tags to markup" filter? (admin/config/content/formats)

i have enabled the media filter, since you cant add the media button in the wysiwyg anyways:)

i m using wysiwyg w/ tinymce

agoradesign’s picture

i m using wysiwyg w/ tinymce

-> maybe the Media/Wysiwyg integration doesn't work 100% for TinyMCE??

ParisLiakos’s picture

ok, it works with both ckeditor and tinymce using wysiwyg module..i reverted the patch and reapplied it, cleared the cache and works!
but wont work for existing content:/

Jackinloadup’s picture

Here is a reroll, its my first one so it could be wrong but worked for me.

The results of this patch will affect #1268116-39: WYSIWYG does not record file usage. See issue for more details.

crifi’s picture

Status: Needs work » Needs review
Jackinloadup’s picture

I just realized I should restate what I said above. The results of this Issue in general will affect #1268116: WYSIWYG does not record file usage. Not just the patch I posted.

becw’s picture

@Jackinloadup nice job on your first reroll! That patch applies cleanly and matches the original work.

Regarding inserting file links etc. in WYSIWYG: this does not work with the current WYSIWYG code; wysiwyg editors (ckeditor and tinymce, at least) require an image placeholder for inserts. This is the behavior of the editor js, and is not something that Media has any control over. There's actually an existing issue for this: #832372: Default "Link to file" formatter for all media types in wysiwyg

Maybe we should address that first, because when media doesn't insert properly it makes the reviews for this issue really confusing :)

mrfelton’s picture

Status: Needs review » Needs work
FileSize
32.85 KB

Has some problems. I was looking for a fix that would enable me to embed a PDF into content through the WYSIWYG using the Google Viewer file format. I have the 'Full' view mode of the 'application' file type set up to use the Google Docs file formatter. Without this patch applied I had the problem described in http://drupal.org/node/1016376#comment-6319856. Which I believe is the same as #1510054: JS Error when adding format with no HTML. This patch resolved the js error, and enabled me to insert the file using my desired file formatter. Once inserted, in the WYSIWYG it looks like in the attached screenshot.

However, as soon as the content is saved, or if whilst still editing the node you disable the rich text editor using the provided link, then the embed completely disappears from the source.

mrfelton’s picture

FileSize
11.01 KB
18.05 KB

Here are another couple of examples where this beaks. The two screnshots show before (when editing the node) and after (when viewing the node).

Before:
Before.png

After:
After.png

You can see that I have tried to use two different view modes. The first is set up to use the generic file formatter. The second is set up to use the Image file formatter. The generic file one actually works ok. The image one however gets replaced with the string 'false'. Also, when viewing the source, it looks like raw html is getting inserted, rather than [media] embeds.

becw’s picture

Status: Needs work » Needs review

@mrfelton thanks for the thorough bug report. This is a known bug and is not related to this cleanup issue. Inserting non-image media formats in WYSIWYG falls under #832372: Default "Link to file" formatter for all media types in wysiwyg. It would be helpful if you added your reports to that issue.

The latest patch, from #34 above, still needs review. This patch just needs to match existing functionality, not break editing on existing content, and not break any third-party code (WYSIWYG editors, media browser plugins, etc.).

ParisLiakos’s picture

#35 still does not work for existing content, everything else works as supposed after a few days usage

mrfelton’s picture

Status: Needs review » Needs work

Ok, so the issue with non-image file types is another issue - but this patch still breaks even for images as described in my first comment - unpon disabling the richtext editor, and on saving the node, the markup is removed from the node and the result is that no image at all gets inserted - you just get the string 'false'

ParisLiakos’s picture

reroll against latest dev

ParisLiakos’s picture

Ah, disregard above, contained a small changed i had :/
use this instead

ParisLiakos’s picture

@mrfelton can you retest this now?
i just test 3 or 4 times with images, txt, pdf on different view mode, and cant see any problem

mrfelton’s picture

@rootatwc - everything does look to be working ok with this latest patch. Still can't insert PDFs, but this no longer breaks image inserts, and both image and video inserts look to work very nicely.

EDIT - switching the text field between rich text and plain text using the 'Disable rich-text' link still ends up replacing the image embeds with the string 'fase'.

ParisLiakos’s picture

thanks for testing:)
that is with tinymce right? (if i judge from screenshots)
i cant find any problem with ckeditor, need to check with tinymce later

mrfelton’s picture

Yes - this is with TinyMCE through WYSIWYG module

ParisLiakos’s picture

well, i tested pretty much everything with tinymce as well, cant get the string false, no matter what. maybe another module interferes?
On which file types do you get this and how those filetypes are configured?
just trying to move this forward, noone else seems to be aving problem

mrfelton’s picture

Status: Needs work » Needs review

On a fresh setup, this looks to be working really well :) Must be something conflicting on this other platform.

mrfelton’s picture

I don't know what it is about this other setup. I have gone through and disabled everything. Every WYSIWYG button, every input filter, pretty much every module. But whatever I do, images in the WYSIWYG just get replaced with the string 'false' whenever you switch between rich text and wysiwyg, or save the node :(

Anyone els hitting up against this?

ParisLiakos’s picture

@mrfelton i think somehow create_macro fails with your setup

+function create_macro (element) {
+  var file_info = extract_file_info(element);
+  if (file_info) {
+    return '[[' + JSON.stringify(file_info) + ']]';
+  }
+  return false;
+}

thats were the string false comes from. Any chance i could access the site you have this problem and debug this?

Kazanir’s picture

I have tested this in CKEditor and it appears to work as advertised. It really needs some sort of merge or integration with http://drupal.org/node/1792738 so that you can choose "preview" mode (the 1792738 patch) or choose "as rendered" mode (this patch) somehow. If I'm embedding a document I don't necessarily want a huge iframe taking up my WYSIWYG editor, which is what occurs with this patch. (Again, intended behavior but maybe not optimal.)

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

I just upgraded a site from 7.x-1.0-alpha2 to latest 2.x dev and tried this patch and there no problems with existing content:)
and by existing content i mean images and pdf, as file links..which functionality was broken..Media icon is also highlighted when selecting in wysiwyg existing files.w00t!!
So i am gonna rtbc this and commit it, i think its about time, and mrfelton's problem seems too localized.
Better fix this on follow up issues if happens to someone else

ParisLiakos’s picture

Status: Reviewed & tested by the community » Fixed
ParisLiakos’s picture

jpstrikesback’s picture

From a quick read of the commit am I correct in thinking that any given additional attributes in a hook_media_format_form_prepare_alter(), or media_format_form() would be added to/parsed from the [[media.....]] object? I'm thinking of alt/title support etc. such as added here: #1307054: Accessibility - Media browser image alt and title fields in http://drupal.org/files/1307054-d7-2.patch (minus the changes that would no long apply to wysiwyg-media.js)

Status: Fixed » Closed (fixed)

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

zied.ellouze’s picture

Component: WYSIWYG integration » Code

When I applied the patch #1
I cannot change the width and height of the image after its insertion, should save the node and then edit the image property

zied.ellouze’s picture

Status: Closed (fixed) » Active
Devin Carlson’s picture