Our site runs media, media_wysiwyg, media_internet, media_vimeo and wysiwyg (latest stable with ckeditor).
Nodes with Vimeo videos cannot be edited. When I open them, the WYSIWYG renders a broken video tag instead of the image preview it used to. And if I view source in ckeditor, it shows HTML video tags instead of the media tags it used to.
Saving the node at this point (which some of my users have) effectively ruins data, saving the broken HTML video tags instead of the media tags. :(
I have done a commit-by-commit test and this happens after the commit from #1792738: Allow custom file view modes for WYSIWYG display. And downgrading does *not* help.
I have also tested the patch from #2831810: media wysiwyg tokens with html are corrupted on save on media-2.0-beta10 and that did not make any difference.
There are no errors in javascript console or watchdog.
Comment | File | Size | Author |
---|
Comments
Comment #2
fuzzy76 CreditAttribution: fuzzy76 commentedComment #3
fuzzy76 CreditAttribution: fuzzy76 commentedComment #4
fuzzy76 CreditAttribution: fuzzy76 commentedInserting new videos defaults to a viewmode named "Default" which IS broken. But any other viewmode works.
The problem is that all existing content using the broken default viewmode can't be edited.
Comment #5
joseph.olstadComment #6
fuzzy76 CreditAttribution: fuzzy76 commentedMy emergency fix turned out to be downgrading to beta7 and run the following SQL:
Now I just have to figure out some smart search-and-replace to fix the nodes my editors have tried to edit the last week. Will post it here as well, but that will be over the weekend.
Comment #7
joseph.olstadyikes
Comment #8
joseph.olstadDid you try 7.x-2.x dev before downgrading? Recent fixes for token issues
Comment #9
LittleRedHen CreditAttribution: LittleRedHen commentedI was seeing something similar when using media_ckeditor and media_oembed to insert YouTube videos. Even with the rich-text being encoded and decoded properly, the placeholder HTML was generated like:
<video controls="controls" preload="auto" alt="alt text for my video" class="media-element file-large cke_widget_element" data-delta="1" data-fid="130" data-media-element="1" data-cke-widget-data="%7B%22classes%22%3A%7B%22file-large%22%3A1%2C%22media-element%22%3A1%7D%7D" data-cke-widget-upcasted="1" data-cke-widget-keep-attr="0" data-widget="mediabox" width="480"><source src="https://youtu.be/some_video_id" type="video/oembed"></video>
When swapped to source view (or saved), this was NOT properly converted to a JSON macro, which meant that the page and associated media could never be edited without working only in source mode. Worse, and as I suspect you found with media_vimeo, it's not even valid video HTML, because the OEmbed call to retrieve the actual HTML hasn't happened yet. This means that the content never works, and never can.
I had similar problems when trying to embed PDF documents that use PDFPreview in their view mode: *those* use div containers with nested divs, which were also failing to be converted to macros, and failed even more spectacularly when swapped between source and wysiwyg mode.
So: there *is* a workaround for this if you're using media_wysiwyg, which is to specify a special view mode for use in Wysiwyg editors which just renders a plain image as the placeholder for each file type. That image can be successfully swapped to and from the file macro, which means the page works OK when viewed, and the file can be edited with the media browser. It does make checking the layout tricky in edit mode, though.
The actual problem seems to be the
replacePlaceholderWithToken
function inmedia_wysiwyg.filter.js
. It is trying to use regular expressions to parse media placeholders out of the wysiwyg HTML, and this is set up to only find placeholders that are img or span tags! Div, video, audio and any other container tags are not recognised at all, causing macro substitution to be skipped. If you have something like nested spans, then the regex will not identify the complete container: only up to the end of the first , which is not going to be the last one: it then substitutes the macro for only *part* of the placeholder, which makes the generated HTML not parse at all (this is arguably worse). A comment in the script notes that this is not an ideal approach.I've attached a patch that uses JQuery to parse the wysiwyg content and identify the media placeholders, instead of the regex. It recognises *any* placeholder element which has
class="media-element"
Comment #10
LittleRedHen CreditAttribution: LittleRedHen commentedNew version of the patch with proper whitespace handling (the last one had some rogue tabs which misaligned a couple of statements).
Comment #11
fuzzy76 CreditAttribution: fuzzy76 commentedSorry for posting obfuscated SQL, that might seem scary for some. I don't know why Sequel Pro (my MySQL client of choice) did that on the export. You can decode the strings with http://www.rapidtables.com/convert/number/hex-to-ascii.htm if you want to see what those insert statements actually insert.
Tested the head of the 7.x-2.x branch and the bug was still present. Tested with the patch from #10 and everything seemed to work. Yay! It sort of works better than it did on beta2. The earlier behaviour was to show all videos as broken images in the WYSIWYG editor (we learned to live with that). Now they show as a broken video player instead. :-P
Comment #12
fuzzy76 CreditAttribution: fuzzy76 commentedNope, YouTube is broken.
Comment #13
joseph.olstadit'd be nice to get this fix into beta11 .
@fuzzy76 you reported in #11 that this worked, but then in #12 state that YouTube is broken, can you please confirm what exactly was fixed as described in #11? (vimeo fixed? anything else fixed that was broken?)
Comment #14
joseph.olstad@fuzzy76 do your youtube placeholder elements have the class 'media-element' ? (see comment #10) also @LittleRedHen says there is a workaround for this.
Comment #15
brockfanning CreditAttribution: brockfanning commentedI guess we still need some feedback on this one, but in the meantime, @LittleRedHen: Do you think there is any way to do this without requiring jQuery 1.8? Even if the code is clunkier, I think it would be worth it to avoid having to require a minimum version of jQuery.
Comment #16
joseph.olstad@brockfanning , the patch that @LittleRedHen wrote is compatible with lower jQuery versions , have another look at the patch, the line in question is
//commented out
, I think he left that line there intentionally so that others would not try to use the >= 1.8 function.Comment #17
brockfanning CreditAttribution: brockfanning commentedAh, my bad, cool.
Comment #18
LittleRedHen CreditAttribution: LittleRedHen commented@brockfanning, @joseph.olstad: It definitely works on JQuery 1.7, but I haven't tested lower versions than that.
The $.parseHTML line is in there because I did test its use: it's preferable in many ways because it doesn't execute any embedded scripts by default, but for patching purposes, I figured it wasn't appropriate to increase the installation complexity. There's therefore a possibility in that someone could potentially use source mode to add scripts that break the submission, since the source is parsed here before the server-side text filters get applied (assuming those have been properly set to strip out embedded script tags).
It's a client-side vulnerability rather than a server-side one, though: as far as I can tell, it's possible to break the page, but the filters *will* still get applied before anything is saved, and so there's no additional risk to the system. In addition to that, if the CKEditor Advanced Content Filter is on (media_ckeditor install guidelines current recommend that it be turned off), then that could filter out script tags before reaching this function.
I can re-roll the patch without the comment if that's misleading?
Comment #19
fuzzy76 CreditAttribution: fuzzy76 commentedMy comment in #11 was about editing of media_vimeo embeds in a wysiwyg editor. I later found out that media_youtube embeds still was not editable in a wysiwyg editor, which was what I was reporting in #12.
The workaround mentioned in #9 is fine for new sites, but that does not fix my existing content.
LittleRedHen, could you take a look at media_youtube and see if there is a way to have those embeds handled as well?
Comment #20
brockfanning CreditAttribution: brockfanning commented@LittleRedHen: I believe out-of-the-box Drupal is at jQuery 1.4.4, so if possible let's try to get this to work there. (If not possible, we may have to add jquery_update as a dependency for media_wysiwyg). It's not liking the prop() method. I manually changed "prop" to "attr" and it worked OK. I'm not sure if that would cause an issue in more recent jQuery versions.
With that change, though, this is working for me in all three of the "streaming" use-cases I know of: media_oembed, media_youtube, and media_vimeo. @fuzzy76 we may have to investigate what's going on with your media_youtube testing. Can you try clearing your browser's "Cached images and files" and see if that helps? If not, any idea if there are javascript errors going on?
Comment #21
LittleRedHen CreditAttribution: LittleRedHen commented@brockfanning: there is an existing open issue about jQuery versions and use of 'attr' which should probably be taken into account as well: https://www.drupal.org/node/2416839
My site requires jQuery 1.7 as a minimum due to use of an unrelated module, so I'm not set up for testing anything less than that, but I can confirm that $(this).attr('outerHTML') does NOT work with 1.7: which makes sense, since both of those are really accessing the element.outerHTML DOM property, rather than an actual 'outerHTML' attribute on the markup (jQuery < 1.6 was hazy about the difference, but 1.6+ are not).
Here's a re-rolled version of the patch that fetches the outerHTML the olde-school way: via the DOM element property if possible (for performance reasons) and by cloning and wrapping the object, then fetching the content of the wrapper if that property doesn't exist (mostly very, very old browsers but better safe than sorry).
If you *do* end up choosing a dependency on jquery_update, then this chould be changed back to use the simpler .prop() syntax. And maybe consider going to 1.8+, so that parseHTML is available as well!
Comment #22
LittleRedHen CreditAttribution: LittleRedHen commentedComment #23
fuzzy76 CreditAttribution: fuzzy76 commentedTested the patch from #20 (which complained about whitespaces).
Inserting a vimeo video gave me
[[{"fid":"3","view_mode":"default","fields":{"format":"default"},"type":"media","field_deltas":{"1":{"format":"default"}},"link_text":false,"attributes":{"class":"media-element file-default","data-delta":"1"}}]]
and switching back and forth between source view managed to convert the tag properly, but left a<p><source src="http://vimeo.com/192534375" type="video/vimeo"> </p>
for each time I flipped back and forth, building up crap.Inserting YouTube did the same:
No Javascript errors.
Comment #24
fuzzy76 CreditAttribution: fuzzy76 commentedOur jquery is 1.7.2 and I tested in both Safari and Firefox. Will see if I can recreate in a simpler install.
Comment #25
fuzzy76 CreditAttribution: fuzzy76 commentedOn the other hand, doing a completely basic install using:
Made everything work. So there is something about our install profile. I will keep adjusting to see if I can reproduce.
Comment #26
joseph.olstadtest results by @fuzzy76 on a vanilla installation show that this patch is backwards compatible with out of the box drupal 7.x jQuery, meaning it IS compatible with >= jQuery 1.4
Excellent work @LittleRedHen
Any objections?
Comment #28
joseph.olstadin 7.x-2.x dev
Comment #29
fuzzy76 CreditAttribution: fuzzy76 commentedI believe there is some configuration combination that still breaks it, but I can post a follow-up issue once I isolate the cause.
Comment #30
fuzzy76 CreditAttribution: fuzzy76 commentedOh, no, this is still severly broken, but I did manage to improve the steps to reproduce. If you have any html tags *after* the media tag (reproducable with empty p-tags), it leaves behind video-tags every time you switch in and out of source view. And this is on a completely basic install like the one explained above (only addition is actually downloading ckeditor). This happens for media_vimeo, media_youtube and media_oembed. So the entire media_internet family of modules are probably affected.
Comment #31
joseph.olstad@fuzzy76
I am having another closer look at: replacePlaceholderWithToken
it is called "when a rich text editor detaches"
thanks for your description, should help locate the annoyance
Comment #32
joseph.olstadHi @fuzzy76 ,
In this patch I've backed off/reverted all changes in the related function replacePlaceholderWithToken since beta2
Not sure if the older regex is still compatible but it is worth testing.
Comment #33
joseph.olstadComment #34
joseph.olstad@LittleRedHen any ideas?
Comment #35
brockfanning CreditAttribution: brockfanning commentedBefore we revert anything let's make sure the issue can be recreated. I'm not seeing the same thing, @fuzzy76, when I try putting content after the embed. For me it's working fine. Here is a drush make recipe for the setup I'm using to test:
Does any of that look different from what you have? This is what I'm doing to set up the testing site: (NOTE THAT THIS WIPES THE SITE, SO DON'T DO THIS ON AN ACTUAL SITE)
And then these manual steps:
Comment #36
joseph.olstad@fuzzy76 , the ball is in your court for now. I'll follow up later.
Comment #37
fuzzy76 CreditAttribution: fuzzy76 commentedI didn't have time to test this today, I'll have to do it over the weekend. One thing I notice is that you tested with the dev-branch of wysiwyg, while I used the last stable which in turn requires ckeditor 3.6 instead of 4.6...
Comment #38
joseph.olstadok @fuzzy76, someone needs to honk some big loud horns and get wysiwyg maintainers to publish a new version asap, meanwhile if we wanted we could write some code in the .install file for media_vimeo or 'media.install' to check that IF you are using wysiwyg module that it MUST be the dev version and NOT the 4 year old "stable" (sic) release.
Comment #39
joseph.olstadComment #40
joseph.olstadhiding the 'revert' patch as I don't think it is necessary. I am unable to reproduce the tokenhandling issue as reported by @fuzzy76
Comment #41
fuzzy76 CreditAttribution: fuzzy76 commentedTested an identical side-by-side setup with wysiwyg-dev and one site using ckeditor 3.6.6.2 (last in 3.x series) and the other using 4.6.0. The first had the bug, the latter didn't. So I guess the bug is isolated.
But that means that media_wysiwyg now require a dev-release of wysiwyg, which does not sound good. And considering it was your code-change that broke the integration, I don't think honking anything in the wysiwyg queue would be appropriate. They are working towards a new release, but since it has been 4 years since the last, I would not hold my breath. I have made them aware of this issue, though.
Comment #42
joseph.olstadA wysiwyg release is coming very soon from email I got from the maintainer. Hope he is true to his word.
Comment #43
joseph.olstadPlease open a new issue if there's any new concerns.