Cannot edit embedded files that don't render as images in the WYSIWYG [#2153089]

Note: This does not seem to be a problem when using the Media CKEditor approach. It does affect the WYSIWYG module approach, though.

When adding a document to a WYSIWYG field I am unable to select it and edit the media settings.

The first problem is that the isNode() method in wysiwyg-media.js does not return TRUE for images and documents (although it does for videos). There are two possible fixes here.
1. Alter isNode() to check for all different types of files that can be included by the media module. The ones I tested would need this:

  isNode: function(node) {
    return $(node).parent().is('a.media-element') //for images
      || $(node).is('img.media-element') //for videos
      || $(node).parent().is('span.media-element') //for documents
      // Possibly something else for audio?
    ;
  },

2. In isNode() only check for the class, instead of also for the element like this :

  isNode: function(node) {
    return $(node).is('.media-element') || $(node).parent().is('.media-element');
  },

I tried #1 first, and that seems to work, but might break when new theme functions are used (I tested with colorbox images, youtube and vimeo videos). #2 is better, but might also break when the media element has 2 wrappers like this:

<a href=".." class="media-element"><span class="foo"><img src="media/..." /></span></a>

So to be future proof I propose this:

  isNode: function(node) {
    return $(node).is('.media-element') || ($(node).parents('.media-element').length > 0);
  },

This way we only check for the class (like in #2), but instead of only checking in the current and the parent element, we check all the way up in the DOM-tree.

Comment	File	Size	Author
#7	media.2153089-7-isNode-does-not-detect-all-media-do-not-test.patch	1.33 KB	sime
#7
#5	media.2153089-4-isNode-does-not-detect-all-media-do-not-test.patch	1.31 KB	Yaron Tal
#5
#4	media.2153089-4-isNode-does-not-detect-all-media.patch	1.08 KB	Yaron Tal
#4
#1	media.2153089-0-isNode-does-not-detect-all-media.patch	509 bytes	Yaron Tal
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Yaron Tal CreditAttribution: Yaron Tal commented 10 December 2013 at 09:24

Issue summary:

View changes

File	Size
media.2153089-0-isNode-does-not-detect-all-media.patch	509 bytes

Comment #2

Yaron Tal CreditAttribution: Yaron Tal commented 10 December 2013 at 09:27

Status:

Active

» Needs review

Comment #3

Yaron Tal CreditAttribution: Yaron Tal commented 10 December 2013 at 13:03

Assigned:	Unassigned	» Yaron Tal
Status:	Needs review	» Active

Also needed a small change in the invoke() method and having some trouble with making the button work in links (where isNode doesn't work because no html is selected). Will add a new patch once those are fixed.

Comment #4

Yaron Tal CreditAttribution: Yaron Tal commented 10 December 2013 at 13:47

Assigned:	Yaron Tal	» Unassigned
Status:	Active	» Needs review

File	Size
media.2153089-4-isNode-does-not-detect-all-media.patch	1.08 KB

Updated version of the patch.
Was not able to fix the problem where text links do not give a context to isNode, because no text is selected. I think this is a bug in the WYSIWYG module, so will make a bug report there to see if they can help solve this (see related issues in a couple of min).

The problems this patch fixes:
isNode() now detects most media elements (only problem are texts)
invoke() will also work if a users has selected a child of a media element, it will go up the DOM to find the full media element.

Comment #5

Yaron Tal CreditAttribution: Yaron Tal commented 11 December 2013 at 15:57

File	Size
media.2153089-4-isNode-does-not-detect-all-media-do-not-test.patch	1.31 KB

Created patches in WYSIWYG editor to fix the text thing.
https://drupal.org/comment/8264225#comment-8264225
Let's hope a WYSIWYG module maintainer can chip in here.

In the wysiwyg issue I created 2 patches. One will work nicely with the media.2153089-4-isNode-does-not-detect-all-media.patch patch. The other one will need the slightly different patch media.2153089-4-isNode-does-not-detect-all-media-do-not-test.patch.

Comment #6

ParisLiakos CreditAttribution: ParisLiakos commented 11 December 2013 at 18:25

Comment #7

sime

any/any

Melbourne

CreditAttribution: sime commented 30 October 2015 at 05:17

File	Size
media.2153089-7-isNode-does-not-detect-all-media-do-not-test.patch	1.33 KB

Ok, a slight update, but I couldn't get it working generically just explicitly with CKEDITOR.

Comment #8

sime

any/any

Melbourne

CreditAttribution: sime commented 30 October 2015 at 05:29

Title:	isNode() does not detect images/documents in WYSIWYG as media elements	» Cannot edit embedded documents
Issue summary:	View changes

I've make the title a little more ... urgent. This is a big problem for organisations who manage a lot of document links in content. To edit a document link, the editor must either: remove the existing link and add it again, or edit source and edit the JSON.

Note that once you can edit the document (eg. using the patch in comment #7), the form does not offer a field to modify the description/name/label.

Comment #9

sime

any/any

Melbourne

CreditAttribution: sime commented 2 November 2015 at 01:08

The description/name/label is effectively part of "link_text". This value is not part of the "fields" component of the JSON and hence doesn't get passed back to the format editing form. So, I guess this is out of scope for this issue, but what is happening for my users is that to change the link_text, they are selecting the end of the text in the wysiwyg and using backspace - this is breaking the whole link.

Comment #10

mstrelan CreditAttribution: mstrelan commented 27 April 2016 at 06:46

Status:

Needs review

» Needs work

This doesn't work when using a file view mode with the generic file formatter. You can re-open the media dialog however submitting it inserts duplicate markup at the caret location, unless you have already highlighted the entire link.

Comment #11

brockfanning CreditAttribution: brockfanning commented 22 December 2016 at 13:12

Title:	Cannot edit embedded documents	» Cannot edit embedded files that don't render as images in the WYSIWYG
Issue summary:	View changes

Added a quick note to the description that this issue is not a problem for the Media CKEditor approach. It only pops up if using the WYSIWYG module approach.

Also changed the name to make it more general - this not only affects documents, but also affects video and audio. I also don't believe any of the attached patches will work for audio/video, because the click event on audio/video tags does not trigger the javascript needed to launch the "format" form.

One approach to fixing this might be to build on the attached patches and also make sure that some event listeners get put on audio/video tags.

Another approach might be to allow for a certain view mode to always be used in the WYSIWYG editor, so that that view mode can have it's "Manage File Display" settings configured to show an icon image. I always assumed that was what the media_wysiwyg_view_mode module did, but that module has been removed. If anyone knows of a quick (maybe already existing?) way to do this, let us know.

I may not be much help on this one, since I don't use the WYSIWYG module approach.

Comment #12

brockfanning CreditAttribution: brockfanning commented 22 December 2016 at 16:10

Category:	Bug report	» Support request
Status:	Needs work	» Active

Aha, I hadn't noticed the "WYSIWYG view mode" option when editing file types. So, for those using the WYSIWYG module, I believe the quick fix for this would be to:

Configure the "Manage file display" settings for the document/audio/video file types so that the "WYSIWYG" view mode renders as a "Large filetype icon".
Edit the document/audio/video file types so that the "WYSIWYG view mode" option is set to "WYSIWYG".

By forcing these types of files to display in the editor as images, you should then be able to edit them while embedded.

Changing this to a support request, since I think it can be fixed via configuration. Feel free to re-classify if I'm wrong about that. I'll try to test soon to confirm.

Comment #13

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented 28 January 2019 at 01:59

Status:

Active

» Closed (outdated)

Recent versions of media have resolved most of peoples concerns and is compatible with entity translation, multilingual and various advanced configurations. Due to the high volume of inactive and most often irrelevant issues we are Closing this as (outdated). If for whatever reason this issue is important to you AND you still have issues after checking the media recipe documentation, then let us know and we will review your concerns.

Otherwise, see the recipe documentation for how to configure media and for troubleshooting tips OR refer to the media_dev distribution if you want to see a working media setup.

As mentioned, feel free to make some noise in this issue if you still feel it is important to you or someone else.

Thanks,

Media team