The WYSIWYG integration in media destroys <script> tags when the WYSIWYG instance is detached [#1280758]

In the detach method of the WYSIWYG integration code, media turns the content string into a jQuery object and returns the result of its html() method. If you have a script tag in your content, it is added to the jQuery object at index 1 with the wrapping div at index 0. html() only returns the html contents of the first matched element, which means that the script tag at index 1 (and any other items that are added to other indexes of the jQuery object) are lost.

This is obviously less than ideal.

Comment	File	Size	Author
#4	media-wysiwyg-detach.patch	1.34 KB	effulgentsia
#4
#3	0001-Issue-1280758-The-WYSIWYG-integration-in-media-destr.patch	1.57 KB	james.elliott
#3
#1	0001-Issue-1280758-The-WYSIWYG-integration-in-media-destr.patch	1.61 KB	james.elliott
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

james.elliott CreditAttribution: james.elliott commented 15 September 2011 at 19:34

Status:

Needs work

» Needs review

File	Size
0001-Issue-1280758-The-WYSIWYG-integration-in-media-destr.patch	1.61 KB

Comment #2

james.elliott CreditAttribution: james.elliott commented 15 September 2011 at 19:48

Something that should immediately stand out is the match length check. I couldn't seem to figure out how to get the regex to not match " as well as the image tags.

Comment #3

james.elliott CreditAttribution: james.elliott commented 15 September 2011 at 19:55

File	Size
0001-Issue-1280758-The-WYSIWYG-integration-in-media-destr.patch	1.57 KB

ah, and I wasn't remembering to group the regex to match more than one image tag.

New patch should fix all.

Comment #4

effulgentsia CreditAttribution: effulgentsia commented 7 October 2011 at 02:07

File	Size
media-wysiwyg-detach.patch	1.34 KB

Minor code style cleanup.

Comment #5

7 October 2011 at 02:09

Status:

Needs review

» Needs work

The last submitted patch, media-wysiwyg-detach.patch, failed testing.

Comment #6

effulgentsia CreditAttribution: effulgentsia commented 7 October 2011 at 02:11

Status:

Needs work

» Fixed

Committed to 1.x and 2.x.

Comment #7

ParisLiakos CreditAttribution: ParisLiakos commented 18 October 2011 at 00:52

Status:

Fixed

» Needs work

Sorry but this patch broke the Generic file display.
It hitted me after i updated to 7.x-1.0-rc2 and went through the changes one by one and thats what breaks it:/

steps to reproduce:

-Go to file types in configuration and then to manage file display.
-Set a display to use Generic file.
-Go to add content and use the wysiwyg button to add a file.
-Choose the display you previously set to display generic file.
-Save the node.
-No filename just a random icon instead of the expected file icon with a link to the filename

Comment #8

becw CreditAttribution: becw commented 24 March 2012 at 04:36

Assigned:

Unassigned

» becw

@rootatwc I can duplicate your bug in the 7.x-2.x branch. The patch from comment #10 of #1451316: Clean up wysiwyg-media.js fixes this issue but needs testing.