Problem/Motivation
The current media implementation is heavily flawed sincde the HTML-code and not the necessary media tokens are stored.
This heavily reduces the usability of the integration e.g. changes in file display formatters won't be propagated and so on.
Proposed resolution
The library.js already provides the necessary functions (attach / detach) to replace the HTML-Code with the appropriate media tokens.
Implement the attach / detach functions as neccesary.
By the way: With this the attached patch you'll be able to insert videos and other stuff as well since in the wysiwyg editor you'll see the placeholder but the output will be the in media configured embedding code.
Remaining tasks
Reviews needded.
User interface changes
None
API changes
None
Original report by hctom
Hi @all,
I am desperately trying to get the media module integration to work, but the editor content always contains HTML tags insted of media tokens.
As far as I investigated, the "attach" and "detach" methods in plugins/media/library.js are never called... but those are the ones that take care of the token rendering.
So am I doing something wrong or is this currently not really working?! I am using CKEditor 7.x-1.8 and Media 7.x-1.0.
I'd appreciate any help.
Thanx in advance & best regards
hctom
Comment | File | Size | Author |
---|---|---|---|
#120 | media-add-ckeditor-support-1504696-120.patch | 22.25 KB | Devin Carlson |
#120 | ckeditor-accomodate-latest-media-changes-1504696-120.patch | 17.81 KB | Devin Carlson |
Comments
Comment #1
das-peter CreditAttribution: das-peter commentedI just installed all the stuff to give the latest spark version a try and totally agree with hctom, the media integration doesn't work as intended.
Attached patch integrates the missing detach / attach functions and fixes some of the code in library.js - parts of the code were copied from the Media Aloha module I worked on.
Comment #2
das-peter CreditAttribution: das-peter commentedUpdated issue summary.
@hctom I hope it's oky that I've hijacked your ticket ;)
Comment #2.0
das-peter CreditAttribution: das-peter commentedApplied issue summary standards.
Comment #3
nod_Looks RTBC to me, haven't tested IRL so I'll leave it to ckeditor guys to commit it :)
Comment #4
mdixoncm CreditAttribution: mdixoncm commentedHey @das peter - I'm getting a 'patch unexpectedly ends in middle of line' error when applying the patch in #1 - would you mind re-rolling the patch to see if it fixes the problem?
Many thanks!
Mike.
Comment #5
kevinchampion CreditAttribution: kevinchampion commentedPatch re-rolled.
Comment #6
jcisio CreditAttribution: jcisio commentedDid anyone really test it? ;)
Comment #7
das-peter CreditAttribution: das-peter commentedWell, I tested it while developing. But that doesn't count for RTBC-ing ;)
@kevinchampion Thanks for the re-roll.
Comment #8
chiebert CreditAttribution: chiebert commentedA couple of notes - all attempts:
1. I wasn't able to get this patch to apply, either with git apply or with patch -p1 (against -dev from Feb 9). With the latter, I kept getting errors that the patch had already been applied, etc. So I manually applied the changes.
2. On a clean D7.20 install, using Media 7.x-1.3, the patch almost works. If I insert an image, for example, and choose a view mode, the element is being tokenized. If I later edit the image style that that particular view mode uses, then the node in question reflects the change. However, if I select at different image style for that view mode (Configure->Media->File types and then 'manage file display' for the 'image' file type, etc.), then those changes are not reflected in the node unless I first edit and re-save the node (no other edits required).
Here's an example tokenized element (note: I'm using DS to create a custom view mode):
[[{"type":"media","view_mode":"test_mode","fid":"2","attributes":{"alt":"","class":"media-image","height":"180","src":"http://d7civi/sites/default/files/styles/square_thumbnail/public/cross2.jpg?itok=MNIGamsv","typeof":"foaf:Image","width":"180"}}]]
Note that, while the view mode is there in the JSON, so is the src, which is explicitly pointing to the image style generated file (in this case, the 'square_thumbnail'). So that's why changes to the image style work (because that file is just being regenerated), but unless I edit the node (thus re-triggering the attach and detatch methods which handle the tokenizing), changing to a different image style entirely don't take effect.
3. As a further point, when I try this patch on an existing site using Media 7.x-2.x-unstable7, there's no tokenizing happening at all.
Comment #9
das-peter CreditAttribution: das-peter commentedI can't reproduce this, whether with git on the command line nor with the tortoise git UI. Patch #5 applies just fine to the latest code in the 7.x-1.x branch.
Did you clear the caches? The token filter is a cacheable on, which means the parsed raw-data are cached as long as there was no re-save. If manually flushing the cache is an issue (after editing a view mode) this is something to report in the media issue queue.
src
is ignored, seemedia_token_to_markup()
. As far as I remember the path in the src points to the placeholder of the media e.g. for Youtube-videos you'll see a thumbnail instead the video element while editing.Btw. we didn't re-invent the tokenizing code, this is the one from media itself - just slightly adjusted (comments, error handling).
I'm not sure what you mean by this? Are you pointing out that the existing embedded images aren't switched to tokes? Or that newly inserted images don't store the token?
We could introduce an update hook to initialize the token replacements. But I'm not sure if that wouldn't lead to further issues. Intentionally added image tags could be replaced as well, resulting in an worse situation than having non-tokenized tags.
Comment #10
treksler CreditAttribution: treksler commenteddoes this work with ckeditor 4.1?
over at http://drupal.org/node/1951964 there is an issue where 4.1 strips the tokens
Comment #11
das-peter CreditAttribution: das-peter commentedI don't know, I think I developed / tested with a 4.0.x version.
@treksler As it seems like you've installed 4.1, how about giving it a try and posting the result here?
Comment #12
treksler CreditAttribution: treksler commentedAlright,
First, as far as I can tell, the patch in #5 does not work (I double checked that it was not a caching issue)
a) if there is already a valid token in the code, it renders it as text (I verified the token to be valid with tinymce or ckeditor and wysiwyg module)
b) when using the media module button to insert an image, it inserts it as straight HTML (no token)
Second, using 4.1 or 4.0 makes no difference. (i.e. the token does not disappear on render like it does in 1951964)
This leads me to believe that if the patch worked for me, it would also work with 4.1
PS. I am using Media 7.x-1.3
Comment #13
kevinchampion CreditAttribution: kevinchampion commentedIn my case #5 works using the latest 7.x-2.0-dev of Media. I believe it also works with Media 7.x-2.0-unstable7.
Comment #14
das-peter CreditAttribution: das-peter commented@treksler Thanks for testing! "PS. I am using Media 7.x-1.3" I guess that's essential. I never tested it with the Media 1.x branch.
@kevinchampion That seems to proof my experience that the patch works with the Media 2.x branch.
The question is now how to continue. Frankly speaking in my eyes the only clean solution would be to refactor the code in media so that the token handling (replacement) is handled there and ckeditor (or whatever uses media tokens) just has to invoke the handling.
Comment #15
das-peter CreditAttribution: das-peter commentedI've tried to unify the token / filter functionality across the Media 1.x / 2.x branch.
That way the ckeditor should be able to work with both versions.
To test it apply the Media patch matching you version and the ckeditor patch.
I've tested only with Media 2.x yet, tests with 1.x would be appreciated.
@treksler Would be awesome if you could give this new approach a try :)
Comment #16
dmsmidtThe patches for ckeditor and media 2.x apply flawlessly.
(Can't test functionality because after hitting submit while embedding media I get: "TypeError: c is undefined", I guess unrelated to this issue)
Comment #17
chiebert CreditAttribution: chiebert commentedBoth patches applied cleanly against today's media-7.x-2.x-dev and ckeditor-7.x-1.x-dev with the ckeditor 4.0.2-full library. Cleared all caches, Drupal and browsers. But now I'm not getting any JSON-type token creation at all (tested on two sites - one of them a clean install). Instead, I get something like the following:
<img alt="A cross" class="image-style-medium media-element file-default attr__typeof__foaf:Image img__fid__12 img__view_mode__default attr__format__default attr__title__Cross" data-file_info="%7B%22fid%22:%2212%22,%22view_mode%22:%22default%22,%22type%22:%22media%22%7D" height="220" src="http://d7civi/sites/default/files/styles/medium/public/cross2_0.jpg?itok=7EVR2Sah" title="Cross" typeof="foaf:Image" width="147" />
... both in the editor and in the generated output. (In the above example, I inserted an image from the library using the 'default' file view mode, which itself is configured to use the 'medium' imagestyle preset.
If I later change the 'medium' imagestyle preset to, say, scale the image smaller (and clear the caches, just in case), the same output comes though the browser - a smaller, regenerated image is served up (as expected), but since the height and width attributes are hard-coded into that tag, it appears the same size as before, but pixelated.
I'm not seeing any errors in the js console, and no PHP notices or warnings. I'm willing to believe I've got my text filter/ckeditor profile settings out of kilter or in the wrong order, but I've moved things around to no effect.
Comment #18
dmsmidtThe patches work for me now as well.
The hardcoded height and width could be a problem indeed, but still, good work!
Comment #19
das-peter CreditAttribution: das-peter commentedHmm, I've to check where the fixed height, width properties come from.
But I think the media token filter should take care of omitting those attributes if present.
I think it's more important to be able to properly use the files display styles than manually setting those attributes for media elements.
Comment #20
dmsmidtYes, file display styles should be first priority and standard behaviour.
Could manually setting the width/height attribute overwrite this behaviour when wanted? Some non-tech editors might find it strange if nothing happens.
Another problem of the height/width added automatically arises when designing a responsive theme and using a width: 100%; height: auto; without !important.
Comment #21
das-peter CreditAttribution: das-peter commentedAs far as I understand the filter keeps all properties set in the variable
wysiwyg_allowed_attributes
. The attributes height and width are part of the default value. Unfortunately the value is not exposed in a settings form an you've to manually adjust it if needed.The handling of manual set attributes works as described below:
The file formatters provided by the file_entity module e.g.
file_entity_file_formatter_file_image_view()
check for the propertyoverride
in the given file object to set the attributes.And currently the media module uses always the
override
to pass the attributes provided by the media token - seemedia_get_file_without_label()
.I think you could argue it's sensible of the media module to use the override functionality to set the customized attributes and it makes sense that the image formatter allows it.
However, the use case described above (changing the image style) is nonetheless a valid one too.
So my purposed solution would be to add a display setting to the image file formatter to define whether the override of the height / width attributes is allowed or not.
Comment #22
dmsmidtOk, new problem.
Testing on latest CKEditor 7.x-1.x-dev (version 4.1 full library) and Media 7.x-2.x-dev with both patches.
Adding an image via the media module results in an image in the CKEditor body field, but switching to plain text or saving prints: "false" instead of any image html or tokens.
Only when picking a display mode with no display is enabled (using file entity) a generic file icon is shown, with the appropriate html (no media tokens).
I found out that removing the "Stylesheet Classes" manually (via the CKEditor image properties popup, tab "Advanced") results in the image staying in place.
So after switching view, or showing the source or saving, the image is preserved.
Trying the following code in library.js, also works. But breaks all good intentions.
EDIT: Ok this is a CKEditor 4.1 problem.
See related issues: Media Button in WYSIWYG CKEditor 4.1+ broken and CKeditor 4 converts Media tags to string "false"
This is rather important since Edit module requires version 4.1+ as of recently. So we can hack around it by setting the "allowedContent" CKEditor setting to "TRUE" somewhere.
Additionally: can we block the default CKEditor image properties popup for files added with the media plugin? Since the changes aren't reflected. Or even better bring up the media plugin embedding settings again?
Comment #23
das-peter CreditAttribution: das-peter commentedI've just created a issue in the media issue queue #1970370: Split media token handling from WYSIWYG integration javascript
Would be good if you could post the test-results as well.
I think we should streamline the media code first so that we can continue on a proper base.
Comment #24
jcisio CreditAttribution: jcisio commentedI'm postponing this one then.
We haven't had plan to put specified Media 2.x support code in CKEditor any time before it have a stable release. About Media 1.x, I guess people want to follow #1950498: JavaScript and other tags are being removed after updating to CKEditor 4.1 to fix the "new feature" in CKEditor 4.1.
I change the issue title as I don't think Media 1.x will accept such a change. Feel free to revert it if that issue get into the next Media 1.x stable release.
Comment #25
Martijn Houtman CreditAttribution: Martijn Houtman commentedFor now, you can just add a setting to 'Advanced options' / 'Custom JavaScript configuration' at admin/config/content/ckeditor/edit/Advanced:
config.allowedContent = true;
Comment #26
rooby CreditAttribution: rooby commentedThis sounds worrying to me.
At the rate the media module has been creating new releases it could be years before there is a full stable 2.0 release. It has been almost 2 years and they are only almost at alpha. Then I assume there will also be betas and release candidates.
They had a few large changes in the unstable release series but I would expect large changes to be almost non-existent in alpha and probably non-existent in beta.
Already 2.x users are at around 12.5% (13074) of the total D7 media module users (106499).
That will climb a lot before media hits 2.0.
Waiting until a stable release to start work seems crazy as then it will be longer past that point until integration is working.
I would think starting during beta or even alpha would mean that it is ready to go when 2.0 is released (most likely before as I would expect its features & apis will be pretty stable long before 2.0).
Comment #27
jcisio CreditAttribution: jcisio commented#26 I completely agree with you, but no CKEditor maintainer has time to add (broken?) support for Media 2.x, a unstable branch that Media maintainers do not recommend to use.
Media 2.x wysiwyg integration is changing a lot. Will it be easier to put the CKEditor support into Media?
Comment #28
rooby CreditAttribution: rooby commentedPossibly. I haven't looked into it yet but WYSIWYG module support is in media.
I think it should be because generally it is up to the other module to integrate with the editor, for example footnotes or image captions.
Really, the text editor module should provide the API and then the other modules handle integration, IMO.
I would be in favour of moving this issue to the media module.
Comment #29
das-peter CreditAttribution: das-peter commented@rooby: That was the idea of #1970370: Split media token handling from WYSIWYG integration javascript - having a stable token filter / WYSIWYG integration over all media branches. More attention to this issue would definitely help to push it.
Comment #30
Barry Tielkes CreditAttribution: Barry Tielkes commented#25 worked for me!
Thx, Martijn. )
Comment #31
dubbydubby CreditAttribution: dubbydubby commented@#15 - For those of you submitting patches, can you please not use console.log, its reference is a JavaScript bug in old versions of IE which some companies still force their developers to support.
Comment #32
tobby CreditAttribution: tobby commentedI had a tough time getting the media.filter-js-2.x.patch patch from #15 to apply for me. Perhaps it was my version of Media 2.x-dev, but it failed on 2/4 hunks. I've rerolled that patch after manually applying the changes and attached it here, in case it's useful to anyone else. The CKEditor patch in #15 worked fine for me.
Comment #33
catchHere's another re-roll, also fixes a fatal js error when inserting videos (filter js was still looking specifically for
<img>
tags).Comment #34
catchErr less misleading patch name!
Comment #35
catchmedia.filter.js was hardcoded to
<img>
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.
Comment #36
catchSorry got confused which issue I was on, moved the media patch over there.
Updated ckeditor patch (also fixes hardcoding to image, changes insertHtml to insertElement) attached.
Comment #37
douggreen CreditAttribution: douggreen commentedAttached is the latest version of the patch that @catch and I have been working on. This version does a better job of markup replacement. Instead of doing exact string matches, which fail when the editor adds newlines or other minor manipulation to the HTML, it adds a marker before and after the HTML, and just replaces everything between the markers.
Comment #38
douggreen CreditAttribution: douggreen commentedugh, I made the same mistake, here is the ckeditor patch. The above media patch is actually for #1970370: Split media token handling from WYSIWYG integration javascript
Comment #39
Martijn Houtman CreditAttribution: Martijn Houtman commented@douggreen: your patches work fine for me, except for the fact that the root directory can not be found (/docroot/sites/all/modules/contrib/ckeditor/) in my install.
One thing I notice is that once the files have been placed, the user can edit an image using the ckeditor image popup. Changes to, for example, the image dimensions, however, are not saved. Is there a way to turn the edit ability of an image off? It's a tad bit confusing for the user.
Also, when I display the title field in the manage display tab of an image, the field is displayed nicely when viewing the content after saving, but not in the ckeditor window (it seems to be stripped out).
Nearly there, I think!
Comment #40
das-peter CreditAttribution: das-peter commentedFixed paths in patch. Patch was created in the docroot instead in the module folder.
Comment #41
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedpatch does not apply cleanly against latest dev version (6/22/13)
aegir@devmac:~/p/7/m/all/ckeditor$ patch -p1 < ckeditor_1504696_37_0.patch
patching file includes/ckeditor.lib.inc
patching file plugins/media/library.js
Hunk #2 FAILED at 20.
Hunk #3 succeeded at 349 (offset 63 lines).
1 out of 3 hunks FAILED -- saving rejects to file plugins/media/library.js.rej
patching file plugins/media/plugin.js
Comment #42
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedScratch the last comment.
this patch applies cleanly.
the effor was caused because of a conflict with another ckeditor media integration patch which I applied found here: https://drupal.org/node/1898958#comment-7190352
Comment #43
saltednutI tried the patch from #40 out along with #1970370: Split media token handling from WYSIWYG integration javascript which has been committed to Media's latest dev - it looks like we are close but CKeditor is still commenting out the media code after the embed is inserted.
Comment #44
saltednutScreenshot of the HTML that gets output after I go through the modal, select/upload an image, and set up embed settings:
Comment #45
das-peter CreditAttribution: das-peter commented@brantwynn Unfortunately I can't reproduce this issue (however, I had a hard time to convince my browser cache to drop the ckeditor media plugin and reload it). Could you please provide more information about your setup?
Here some data from my test:
- Browser: FF 23, Win8 64bit
- Configured all text formats to convert media tags, enabled CKEditor Media plugin for all profiles.
- Media: latest dev
- FileEntity: latest dev
- CKEditor Module: latest dev + #40
- CKEditor library: 4.2 standard.
Source view: shows
<p>[[{"fid":"1","view_mode":"preview","type":"media","attributes":{}}]]</p>
WYSIWYG view: shows the image.
However I came across an issue too, it looks like I can't delete an inserted image / media in the wysiwyg. I guess that's because if I delete the image/media the wrapper (
<!--MEDIA-WRAPPER-START-N-->
still exists and is replaced later with the media token from the tagmap. If I delete the media token in the source view the media is gone. I guess we could add a condition to the converter to check if a wrapper is empty and if so ignore it. However, the downside of this could be that broken media output (empty output) is considered as deleted media.Comment #46
das-peter CreditAttribution: das-peter commentedOh, wrong status - I'm working on a patch for the issue I mentioned.
Comment #47
jcisio CreditAttribution: jcisio commentedI guess we won't drop support for Media 1.x when we plan to support Media 2.x.
Comment #48
das-peter CreditAttribution: das-peter commentedBtw. here's the follow-up for the issue I've found in #45: #2066859: Removed media are restored on save in WYSIWYG
I thought it's best to handle this scenario in the media part as this is likely an issue other WYSIWYG editors will have too.
Comment #49
saltednutOkay - I did a rebuild and using Chrome 28.0.1500.95 with latest file_entity (dev HEAD), media (dev HEAD), ckeditor (dev HEAD w/ patch from #40) and CKEditor library 4.2 standard.
Things are better, but still not as expected.
After inserting an image, I am not seeing the rendered media inside the WYSIWYG.
However, when I switch to the source view, I do see the proper media embed code.
FWIW, the actual rendered node also shows the inserted media correctly.
Comment #50
saltednutOk I did a rebuild again. This time using standard profile. (I was using DF distro in #49)
Using
Drupal (7.x HEAD)
CKEditor module (1.x-dev) + patch from #40
CKEditor library (4.2 standard)
File Entity (1.x-dev)
Media (2.x-dev)
Everything is working as expected now. I see the rendered entity in the WYSIWYG. I see the Media embed code in the source view. I see the rendered entity on all view modes.
Lets get a few more eyes on this but I think it can be RTBC.
Comment #51
das-peter CreditAttribution: das-peter commentedHere's an updated patch that depends on an updated Media patch: #1970370-14: Split media token handling from WYSIWYG integration javascript
Comment #52
saltednutI can confirm #51 is working with the patch from #1970370: Split media token handling from WYSIWYG integration javascript
I think this is ready now.
Comment #53
saltednut#1970370: Split media token handling from WYSIWYG integration javascript landed so that makes things a bit easier.
From my tests, this patch is working with multiple CKEditor libraries:
ckeditor_4.1.3_standard - Works
ckeditor_4.2_standard - Works
ckeditor_3.6.6.1 - Works initially, but I see the embed code instead of the media when going back in to edit the node after having created it.
Comment #54
saltednutScreenshot showing 3.6.6.1 bug - unsure if that makes this as Needs Work. Is the intent to fully support 3.6.6.1?
Also went in to add another piece of Media here and the source shows it is not putting in the code correctly once editing the existing content.
Comment #55
das-peter CreditAttribution: das-peter commentedHere's an updated patch. This one should fix the 3.x issue.
The code for initializing the html (setting the placeholders) seems to work totally different.
While working on this I came across some strange issues.
It looks like deleting the media placeholders in the wysiwyg mode can lead to serious trouble.
Next step will be to check how we can ensure the whole placeholder including the comments (
<!--MEDIA-WRAPPER-START-N-->
) is always selected when working in wysiwyg mode.I'm afraid that this (potential) bug doesn't just affect this module but the wysiwyg module integration as well.
Comment #56
saltednutTested patch from #55 with the following versions of CKEditor library.
ckeditor_4.1.3_standard - works!
ckeditor_4.2_standard - works!
ckeditor_3.6.6.1 - ....works!
I think this is ready to go now for real.
Comment #57
willyk CreditAttribution: willyk commentedFWIW, I tested this too and got it working with all 3 versions of CKE as well.
Comment #58
das-peter CreditAttribution: das-peter commentedI continued work on the patch. Here the exact scenarios I used to test (with Win8x64, Chrome28):
Unfortunately all those cases seemed to fail with the patch from #54.
The new patch uses a custom tag (
<mediawrapper>
instead comments (<!--MEDIA-WRAPPER-START-N-->
) to wrap the placeholder. That way I've at least a chance to handle selections.The new code does it's best to ensure the whole media wrapper element is selected if e.g. the placeholders image is selected.
Further it tries to move any selection within the media wrapper to its outside to ensure text data aren't inserted into the media wrapper but it's parent element.
Unfortunately this doesn't work as expected in CKEditor 4.
I've hacked a bit around that: if the cursor is set right next to the placeholder and there's already text after the placeholder you'll see that the cursor is moved always to the second char in the text (ugly).
That's because CKEditor 4 resisted all my attempts to avoid inserting text into the media wrapper while adding text right next to it. Unfortunately this hack works only if there's already text after the media wrapper, otherwise CKEditor 4 still happily inserts the text into the wrapper.
As I'm definitely no CKEditor expert every feedback would be appreciated and if someone is bored, here are the API documentations of CKEditor ;)
Comment #59
willyk CreditAttribution: willyk commented@das-peter FWIW, I've done some similar custom WYSIWYG and related plug-in implementations for clients relating to multimedia functionality similar to what you've been working on for CKEditor (although the work I did was on TinyMCE). The approach/conclusion I came to when doing so about 2 years ago was similar to what you've been doing here, especially with the most recent approach. In particular, I ended up implementing a custom wrapper for each plug-in that I could consistently rely on the identify and also increment the media elements, with the later being helpful for purposes of removing and cleaning up tags prior based on user input/upon certain triggers. There are likely better/more elegant options, but that approach did get the job done for immediate purposes.
Comment #60
micbar CreditAttribution: micbar commentedThere is another issue in media-filter.js.
#2067063: Wysiwyg integration is broken
Seems that the new token handling, introduced in #1970370: Split media token handling from WYSIWYG integration javascript doesn't work quite well
it might affect this issue.
Comment #61
willyk CreditAttribution: willyk commented@das-peter I tested the above patch with the latest dev versions of File Entity, Media and CKEditor and it worked well for me with both CKE 3.6 and 4.1.
Comment #62
rooby CreditAttribution: rooby commented@das-peter:
Updating old comments makes it almost impossible for people to know what you changed and it is confusing when you come to see a new one and have to trawl through the timestamps/new flags to find the latest comments.
Best to use new comments where possible and making note of what you edit in old posts if you have to do so.
Comment #63
MiroslavBanov CreditAttribution: MiroslavBanov commentedI did some research on how this integration works with media_internet, media_youtube, media_vimeo and I am posting what I found out.
The integration (patch #58) works with pre-September releases of ckeditor, file_entity, media, media_youtube, media_vimeo, and with ckeditor library 3.6.6.1. The new version of media_youtube in particular breaks the inserting of youtube videos. I'm not sure about the rest of the modules - it is possible they can be updated.
I couldn't make the media_internet work for youtube/vimeo videos with ckeditor library 4.2.1, with any setup. If someone has more success with this, please post your findings. Marking this as "Needs work".
Comment #64
Danny_Joris CreditAttribution: Danny_Joris commentedI would like to include media_crop to this for testing as well. I've been trying to get this work for with media 2.x and ckeditor with patch #58, but I'm not having any luck. Without the patch, None of the images are tokenized, but with the patch things don't work either.
One problem is that when the image is passed through ckeditor's JS, the media crop image isn't created yet, and thus the token generated doesn't know of the media crop ID yet. I can kind of fix that in media_crop.image_replace.js by replacing the values in Drupal.settings.tagmap, that contain the {MDIID} placeholder.
Another issue I notice is that I can't get to call media's media_token_to_markup() not anymore somehow.Edit: got this working again.On top of that, there's 2 places in the media's JS where it cuts of attributes from the info object, so it can't create longer media tokens.
Comment #65
adamdicarlo CreditAttribution: adamdicarlo commentedMy findings:
Comment #66
dafederAs mentioned in #60, there is another issue going on in media module that particularly affects image resizing, discussed at #2067063: Wysiwyg integration is broken. Using patch #57 on that issue, which resolves the image resize issue, still breaks some JS introduced in patch #58 here. Here is a small patch that corrects that. Again, apply #58 above before this patch. I suppose until that patch gets committed to media module this is sort of a fringe case.
Comment #67
das-peter CreditAttribution: das-peter commentedIt took me way longer than I wanted, but here's a patch that introduces media as a widget in CKEditor >=4.3.
It provides the same functionality for all earlier versions of CKEditor as before - with the same issues - but from version 4.3 on it uses the new widget API to ensure the Media specific markup is handled as one block.
For now the integration is very basic but from here we could add inline editing of portions of the media markup.
@dafeder The new widget behaviour will essentially effect that because resizing has to be implemented specifically for it.
Comment #68
ndobromirov CreditAttribution: ndobromirov commentedThanks for the patch, but unfortunately it does not apply against the current 7.x-1.x HEAD version, so I was not able to test it.
I see the following output after "git apply ckeditor_1504696_67.patch":
error: cannot apply binary patch to 'plugins/media/icons/hidpi/media.png' without full index line
error: plugins/media/icons/hidpi/media.png: patch does not apply
error: cannot apply binary patch to 'plugins/media/icons/media.png' without full index line
error: plugins/media/icons/media.png: patch does not apply
error: cannot apply binary patch to 'plugins/media/images/icon.gif' without full index line
error: plugins/media/images/icon.gif: patch does not apply
Comment #69
ndobromirov CreditAttribution: ndobromirov commentedI've removed the sections in the patch that were causing the issues (some image files) and the patch applied.
Posting it here and I'll start experimenting with it.
I'll probably be slow in testing, because this patch conflicts with some other patches that I am using currently for this module.
List of all the patches I am using now:
projects[ckeditor][patch][] = "http://drupal.org/files/ckeditor-media-plugin-files-1898958-4.patch"
projects[ckeditor][patch][] = "http://drupal.org/files/ckeditor-media-alt-title-1475774-2_0.patch"
projects[ckeditor][patch][] = "http://drupal.org/files/2024149-ckeditor-media-broken-image-ie8-2.patch"
projects[ckeditor][patch][] = "http://drupal.org/files/ckeditor-translation-1203846-17.patch"
projects[ckeditor][patch][] = "http://drupal.org/files/ckeditor-media_ie_fix-1914904-4.patch"
Comment #70
saltednutPatch in #67 is working fine for me - using it during a drush make build. Applies to current HEAD. #68 May be an issue with needing to apply the patch differently. Maybe try
git apply [patchname] --index
Tested functionality manually and I am able to use the media button in ckeditor to insert an image onto the WYSIWYG.
Example markup:
<p>[[{"fid":"1","view_mode":"default","type":"media","attributes":{"height":"283","width":"481","class":"media-element file-default attr__typeof__foaf:Image img__fid__1 img__view_mode__default attr__format__default attr__field_file_image_alt_text[und][0][value]__ attr__field_file_image_title_text[und][0][value]__"}}]]</p>
No interdiff provided but I am thinking the patch posted in #69 is probably unnessecary.
I'm RTBC'ing #67 here.
Comment #71
das-peter CreditAttribution: das-peter commented@brantwynn Could you also test with CKEditor 4.3? That version handles the whole thing a bit different because it uses the widget API instead plain markup.
Comment #72
DamienMcKenna@brantwynn: Could you please clarify the versions of the ckeditor library you used. Thanks.
Comment #73
das-peter CreditAttribution: das-peter commented@DamienMcKenna: I think we should re-test all the cases listed #2067325: [META] Tracking WYSIWYG compatibility as I've made quite some changes with the last patch. I've tested 4.3 primarily.
Comment #74
saltednut@DamienMcKenna - using 4.2 - http://download.cksource.com/CKEditor/CKEditor/CKEditor%204.2/ckeditor_4... -- tested and works.
I tried switching out the library to 4.3 beta but that makes the editor look crazy and the media button is nowhere to be found.
After that test, I also switched out the library 4.3 for 4.2.2 - in that case the editor looked fine except the media button was missing. I was able to click the empty space where the media button would be and the functionality was there for adding an image.
Is the goal to support a beta version (4.3) or just the latest (4.2.2)?
If anyone wants to re-test this for #2067325: [META] Tracking WYSIWYG compatibility in a much more thorough manner, thats likely what we'll need done to get this back to rtbc.
Comment #75
das-peter CreditAttribution: das-peter commentedI think the goal is to support everything >= 3.6
Unfortunately the approach with the wrapper tags/comments has massive flaws as I pointed out several times. Thus I'd prefer to go for 4.3 where the widget API/plugin was introduced which solves all those trouble.
Comment #76
das-peter CreditAttribution: das-peter commented@brantwynn Could you please re-test? I've tested again (Win8, FF24) and only found an issue with 4.0.x versions.
The attached patch should solve that.
Fyi to test the different versions I switch the version using the git tags (https://github.com/ckeditor/ckeditor-releases).
But to test 4.3 beta I've to use the code provided here: http://ckeditor.com/download - if I use the related git branch (https://github.com/ckeditor/ckeditor-dev) the editor doesn't show up at all but also doesn't throw any error.
Comment #76.0
das-peter CreditAttribution: das-peter commentedUpdated issue summary.
Comment #77
Devin Carlson CreditAttribution: Devin Carlson commentedWith the patch from #76 I ran into the issues outlined in #74.
There are some changes to the buttons in the latest version of CKEditor which cause the editor to look weird if you've got some of the previous images or CSS cached. I found that the buttons appeared properly after clearing my browser cache.
The patch in #76 attempts to create two new .png image files, as required by the CKEditor 4.x plugin API, (
media/icons/hidpi/media.png
andmedia/icons/media.png
) but it doesn't include the image files themselves. Attached is a patch that is identical to #76, except it includes a binary of the current Media icon converted from .gif to .png.@das-peter if you have a better icon and/or a HiDPI version can to post an updated version of #76 that includes the image binaries? (
git add
the files and thengit diff --cached --binary > example.patch
).Functionally the patch seems to work great! :)
Comment #78
saltednutI'm on the fence about marking this as "needs work" - I have run into one issue with #77. If one is attempting to use the Edit module with CKEditor, the modal does not appear and I am seeing the following in my console:
The functionality is there on a node edit page. It is only in the Inline Edit scenario that we see this issue.
Comment #79
FunkMonkey CreditAttribution: FunkMonkey commentedI had things working fairly well after applying the patch in #77 (using library 4.2.2). However, I just switched the library to 4.3 (looks like it is out of beta) and now I get no CKEditor showing up at all. Just a gray box. If I disable the Media plugin CKEditor works correctly.
I've tried clearing caches in Drupal and in my browser.
EDIT: Changing to CKEditor 4.2.3 fixed things right up.
Comment #80
jcisio CreditAttribution: jcisio commentedAs CKEditor co-maintainer, I suggest move this issue into Media module so that code between WYSIWYG and CKEditor module integration can be shared.
Comment #81
jcisio CreditAttribution: jcisio commentedWas moved to wrong module.
Comment #82
kevinchampion CreditAttribution: kevinchampion commentedGiven that ckeditor module has moved this to media and media has stated frequently that it is not supporting ckeditor module, what should we do with this from here?
Comment #83
DamienMcKennaDefine "support"? CKEditor 4.2 works with WYSIWYG and Media when the patch from #1956778: Ckeditor 4.1 ACF is applied.
Comment #84
kevinchampion CreditAttribution: kevinchampion commentedI'm referring to support for the standalone ckeditor module, not using wysiwyg module. I'm basing the conclusion that media is not interested in supporting ckeditor standalone module from the conclusion of this issue:
https://drupal.org/node/1840632
I believe other issues have been closed as well based on the same premise.
Given that this issue was a ckeditor standalone module issue seeking to resolve integration with media module in the ckeditor standalone module media plugin, but has now been moved to media module queue by a ckeditor standalone module maintainer, and in context of the points above, it would seem this issue is a bit lost.
The competing namespaces of all this is truly confusing...
Comment #85
kevinchampion CreditAttribution: kevinchampion commentedThis patch adds on to #77 to provide support for applying the alt and title attributes to images.
Comment #86
DamienMcKennaThanks for the reminder, Kevin, I've got a touch of flu brain today and I apologize if I missed the obvious.
Comment #87
PI_Ron CreditAttribution: PI_Ron commentedI've applied patch from #85 to latest dev with 4.3 library, I still cannot see the media module embed icon available on my profile edit page.
I have ticked:
- Plugin for inserting images from Drupal media module
- Plugin for inserting Drupal embeded media
Comment #88
axe312 CreditAttribution: axe312 commentedHello folks,
thank you for your work. You did a good job!
Here is a updated version of the patch, it fixes the following things:
My setup:
Important: I did NOT check the patch with other module and CKEditor versions, for example the current Media "stable" release.
What worked:
What does not work:
What I am missing:
Please test and improve the patch :)
Greetings,
axe312
Comment #89
FunkMonkey CreditAttribution: FunkMonkey commentedI have tested the patch in #88 with the same setup as axe312.
I added an image and the image stayed after saving and re-editing. Did not test drag and drop (I may not have that enabled I guess) or video.
To add to axe312's 'Text align' comment: If I only add an image and no text, the text align works correctly. It did not appear to allow me to enter any text if I added the image first, however. If I add text first, insert an image, and then try to edit the image properties it does not appear to pick up the image URL correctly so I can't apply a text alignment at all. If I click Source, and the back out, I can then select the image and choose 'image properties' and it then picks up the URL and the text align works correctly. This seems to be different than axe312's results. The text align seems to stay in place after a save and re-edit as well.
This seems to be very close now!
Comment #90
saltednutThanks for working on this everyone. Are patches for CKEditor to continue now that this issue is moved to Media? I thought the plan was to move forward with removing the Media stuff from CKEditor and working on a patch for Media module?
See: #2140155: Remove media plugin
Comment #91
das-peter CreditAttribution: das-peter commentedAwesome to see other's jump in here :)
Well, again some history. I originally started working on the Media WYSIWYG stuff because I wanted to support the spark initiative and thus the, then favoured, aloha editor.
To do so I wanted to clean up the token handling and the integration into the WYSIWYG module of the Media module.
The code was unnecessarily mixed up and so I started working in this: #1970370: Split media token handling from WYSIWYG integration javascript
Unfortunately somewhere along the way other changes were introduced into that patch as well, what lead to huge confusion.
In the mean-time spark switched from aloha to CKEditor and so I started to work in this issue too.
And because of the "other changes" in #1970370: Split media token handling from WYSIWYG integration javascript e.g. the unlucky wrapper tag the things became messed up quite a bit.
Now to the question itself - as far as my understanding goes the Media module maintainers have, understandably, no interest in supporting other WYSIWYG integrations. Thus it was an important step to clean up the JS(-API's).
And I personally think Media and it's maintainers should focus on Media and File Entity and not (other) WYSIWYG integrations.
Thus from my perspective this issue belongs to CKEditor.
My idea is that the integration of the Media into the WYSIWYG module should provide just the basic functionality and we can do our own in-depth integration for CKEditor here and leverage all the features of CKEditor where the WYSIWYG module is limited due it's requirement for interoperability between different WYSIWYG libraries.
Are there other ideas / opinions around? Does anybody see disadvantages of the outlined idea?
Feedback very welcome!
Comment #92
FunkMonkey CreditAttribution: FunkMonkey commentedCKEditor is going to be the default WYSIWYG editor built into Spark and Drupal 8, correct? Wouldn't the Media team very much want to support CKEditor then? Even in preference over supporting the WYSIWYG module? There's probably a lot of history and maybe some politics I don't understand, but on the surface of it, CKEditor seems very important to support.
On another note, and I may be displaying my development ignorance here, but would it make sense to move this into a 'CKEditor to Media Bridge' module in the vein of the 'WYSIWYG to IMCE bridge' module? Seems that would make both the Media and CKEditor maintainers happy :) But again, from a development stand point that might be a terrible idea. I just don't know what is involved.
Comment #93
WorldFallz CreditAttribution: WorldFallz commentedthe implementation of ckeditor that went into core for d8 is, for the most part, the wysiwyg version iirc.
much as the ckeditor module was my preference for a long time, once the extensive visibility settings were removed, the main advantage it had was no longer applicable. I've been using the wysiwyg version for d7 exclusively, and it works great.
My guess is there won't even be a ckeditor module for d8.
Comment #94
WorldFallz CreditAttribution: WorldFallz commentedalso, there's basic image uploading/insertion built into core's ckeditor implementation as well.
Comment #95
das-peter CreditAttribution: das-peter commentedI continued working on the patch primarily to be up to date again.
Changes:
data-file_info
to ensure no media infos are stripped.I really hope this time I managed it to properly include the binary files in the patch ;)
Some more refactoring would really be required but for now I would be happy if we could manage it to get this properly working.
Comment #98
kevinchampion CreditAttribution: kevinchampion commentedFrom #91
I completely agree with this.
From #92
I've had the same thought. If ckeditor module no longer wants to support this and if it doesn't really belong in media module, perhaps there could be a way of splitting it out as a separate contrib. Not ideal, but it could help things to move forward.
@das-peter Is this a viable option? Do you have any interest in heading in that direction?
Comment #99
axe312 CreditAttribution: axe312 commentedIts very nice to see that much activity in this issue!
My colleagues are trying to start a BoF about this the next Days at DrupalCamp Vienna. You can contact them via our wunderkraut munich booth! I won't be there, I am sick and can not attend :/
I'll test your patch tomorrow, what you've done sounds very good. I just need some sleep now :)
I'd also agree with this.
Seems to be a good idea, if no maintainer of the both modules will support the "Media to WYSIWYG" implementation.
Comment #100
deggertsen CreditAttribution: deggertsen commentedCouldn't get the patch in #95 to apply.Edit: My bad, I applied the patch incorrectly. Ignore this.
Comment #101
kevinchampion CreditAttribution: kevinchampion commented#95 applies fine for me to 7.x-1.x.
With ckeditor 4.3 custom including widgets plugin, I didn't get any tokenization happening.
With ckeditor 4.2.3 full, file_links are broken and not tokenizing for me and image alt and title attributes aren't applied on embed from the media browser.
Comment #102
kevinchampion CreditAttribution: kevinchampion commentedBuilds off of #95 to provide support for applying the alt and title attributes to images.
Comment #104
das-peter CreditAttribution: das-peter commentedThe interesting thing is that there is already something like a Media WYSIWYG module. Of course, currently it's dedicated to handle file display modes - but why? We could convert that to a module that takes care of the whole WYSIWYG integration. Including different WYSIWYG libraries.
It would make sense to have this module enabled always if you use WYSIWYG editors - I think the more powerful Media becomes (embedding videos n stuff) the worse the issue with the WYSIWYG placeholders (multiple tags, no wrappers and so on) will become.
So yes, as far as I'm concerned creating a dedicated module for the purpose of handling all the WYSIWYG related stuff is a good idea.
However, I'm not interested at all to become maintainer of a contrib module within the media module repository. I think the whole thing is volatile enough already and I won't help :P
The disadvantage of having an own repository would be the compatibility handling between the Media versions, especially the backwards compatibility to versions that brought their own WYSIWYG integration.
Next step: Let's check what all the maintainers of Media think about this idea.
@kevinchampion Thank you very much for restoring the attribute stuff. I thought that was a legacy piece :|
@deggertsen Did you use the
--binary
flag when applying? The binary stuff in the patch makes trouble sometimes.@axe312 Unfortunately I'm not in Vienna - couldn't make it. But I'll try to stay online as consistently as possible so I should be reachable (luckily it's in the same timezone ;) ).
Comment #105
randallknutson CreditAttribution: randallknutson commentedHey all,
I was in need of something like this today and wrote a quick module to replace all file upload/browsing buttons within ckeditor with the media browser popup. While it is a little different than the discussion here, it solves a very similar issue. See: https://drupal.org/project/ckeditor_media
The reason I did it this way was that I'm creating various CKEditor widgets and many of them have images. I don't want to assume that we're using the media popup on them so I coded them for the generic file browser that comes with ckeditor. Then I created this module to override the functionality of the file buttons and use media instead. That way I can use the widgets with media, ckfinder, the default file functionality or any other file browser.
Comment #106
Devin Carlson CreditAttribution: Devin Carlson commentedI'd be in favor of moving the WYSIWYG integration into a submodule. I'd also like to split out any other units of functionality, such as multiple file support (plupload/multiform integration, etc), into separate sub modules as was suggested in some of the talks/articles about media that came out of DrupalCon Prague.
I'm also for CKEditor module integration and employing the new CKEditor widgets API to help address some longstanding issues with clientside editors messing with markup on attach/detach, etc but we'll definitely need to maintain backwards compatibility with the current WYSIWYG implementation. I'd support maintaining the status quo for WYSIWYG integration and having new development occur in a CKEditor-specific plugin. I can see Media for Drupal 8 only shipping with support for the default CKEditor module with support for other editors left up to other projects.
As the media integration included with CKEditor is being removed, the current patch will need to be pieced out and moved into Media as appropriate and rerolled to have the CKEditor plugin included with and registered by Media. I'd feel even better if someone who worked on this issue having some form of a commitment/maintainership to the WYSIWYG sub module, (even if they try to avoid it *cough* @das-peter *cough* ;)).
Comment #107
axe312 CreditAttribution: axe312 commented@Devin Carlson:
Yesterday I was talking with das-peter about the sub-module. He'll create a proposal about that stuff this weekend and will post it on sunday evening.
Our current plan is to create the submodule based on the code in this patch. Me and my colleagues are going to help with the implementation and may get the first running version next week! Stay tuned :)
Comment #108
das-peter CreditAttribution: das-peter commentedI've just created following issue #2142571: Spin-off WYSIWYG support in dedicated module / project
Please help me to polish the proposal and once it's worth to review / discuss to gain some attention from Media maintainers :)
@Devin Carlson *cough* I don't know what you mean *cough* :P
@randallknutson Sound's interesting, I'll happily take a look into the module if I find some time :)
Comment #109
PI_Ron CreditAttribution: PI_Ron commentedI've applied #102 against latest ckeditor dev and latest media dev. The button works great for images, however for files (I'm attempting with PDF's), when you switch to plain text or save the node it strips the media tag.
I have disabled 'Advanced Content Filter' and added
config.allowedContent = true;
to custom javascript configuration field.Comment #110
kevinchampion CreditAttribution: kevinchampion commentedI can confirm #109 and have traced it to this chunk in library.js:
Returning this chunk to what it was before #88 allows the media token to be preserved:
I'm not totally clear on the purpose behind this code, so haven't been able to determine a fix that preserves the addition but doesn't break document tokens.
Comment #111
mikgreen CreditAttribution: mikgreen commentedSome notes:
Media dev from Nov 13th.
Ckeditor dev.
Ckedtior library 4.3 with widget support.
Patch from #102.
Tokens are incorrent until I changed in plugin.js:
to
Also editing (#95) don't work for me.
This is what my html code looks like in editor (breaks added):
This is how token looks like in source mode (breaks added):
Comment #112
PI_Ron CreditAttribution: PI_Ron commentedUpdate:
Ckeditor not-patched
- Latest Ckeditor Dev (with no patch applied)
- Latest media dev with media_wysiwyg enabled.
Ckeditor patched from #102
- Latest Ckeditor Dev (patch from #102 applied)
- Latest media dev with media_wysiwyg enabled.
Comment #113
Devin Carlson CreditAttribution: Devin Carlson commentedTwo patches: one for CKEditor and one for Media. The patches integrate the CKEditor plugin from #95 into Media, removes the dependency on the WYSIWYG module from Media: WYSIWYG, allows Media to alter the bundled CKEditor Media plugin and changes CKEditor to include the proper JS when using Media WYSIWYG instead of the bundled CKEditor Media plugin.
Everything is functioning fine, except I'm getting a "Uncaught TypeError: Cannot read property 'length' of undefined" error when pressing submit on the media format form.
Comment #114
jcisio CreditAttribution: jcisio commented@Devin: the patch for CKEditor looks very correct and is a nice clean up. Do you mind filing it against CKEditor module?
Comment #115
micbar CreditAttribution: micbar commented@Devin
The patch for ckeditor changes the way attributes are saved. The wysiwyg module stores the attributes in the data-file-info attribute of the img tag.
Do you see any reason to put them into the class attribute?
if we use the create_element and create_macro methods in media_wysiwyg.filter.js they would take care of the attribute handling.
if #2126697: Wysiwyg -- Alt and Title fields require some special handling. gets committed, we can change
this
to
to take care of the special handling for the alt and title fields.
Comment #116
deggertsen CreditAttribution: deggertsen commentedHere's a little bit more about that error @Devin mentioned in #113. It's making it so that it wont submit. I haven't figure out how to get it fixed yet.
Comment #117
micbar CreditAttribution: micbar commentedMaybe i discovered something. Drupal.settings.media.wysiwyg_allowed_attributes in media_wysiwyg.filter.js is undefined so the create_element() method throws an error.
it somehow depends on the wysiwyg_module. With the wysiwyg module enabled, it is properly set. It is added in media_wysiwyg/wysiwyg_plugins/media.inc.
media_wysiwyg.filter.js in line 73
Comment #118
saltednutUsing patches from #113 I do not see a JS error "Uncaught TypeError: Cannot read property 'length' of undefined" but instead the modal goes to a white screen that says "The requested page "/media/1/format-form?render=media-popup&fields=undefined" could not be found."
Tested this with CKEditor library 4.3_beta_standard and with 4.3.1_standard and got the same result.
Edit: Needed to enable the media_wysiwyg module.
I am now seeing 'Uncaught TypeError: Cannot read property 'length' of undefined' when attempting to submit.
Comment #119
saltednut#117 hit the nail on the head. Looking at
media/modules/media_wysiwyg/wysiwyg_plugins/media.inc
I noticed thatmedia_wysiwyg_include_browser_js()
needs to be called in order to get Drupal.media.wysiwyg_allowed_attributes to be set.media_wysiwyg_media_plugin()
was the only function that was calling it. This explains why things 'just worked' when WYSIWYG was installed - becausemedia_wysiwyg_media_plugin()
is a hook provided by WYSIWYG.The media_wysiwyg module needs to call
media_wysiwyg_include_browser_js()
if ckeditor module is present. I also noticed that the file media_wysiwyg.ckeditor.inc was not being included anywhere. So the hook it was provided would not be working, probably. I'm not sure if this is the best possible way to be callingmedia_wysiwyg_media_plugin()
but its something, and working in my tests.Comment #120
Devin Carlson CreditAttribution: Devin Carlson commentedNice job tracking this down everyone and thanks for the patch @brantwynn!
Tested with version 4.2 and 4.3 of the CKEditor library and everything seems to be functioning. I think that the file inclusion, given the current architecture, should be taken care of by CKEditor. So I've attached rerolls of #113 with modified versions of the fixes from #119.
Comment #121
gmclelland CreditAttribution: gmclelland commentedWill this still work if you apply ckeditor styles(from the custom classes dropdown) to the embedded media?
When I used the CkEditor library + WYSIWYG module + Media 2.x in the past, if I applied a style using the CKEditor dropdown it would cause the media tags to get messed up.
See #2028255: ckeditor styles break media module markup
I didn't notice the media tags were broken until after I reedited the node and examined the media tag markup.
Comment #122
micbar CreditAttribution: micbar commentedthank you all for the great work!
i managed to apply both patches cleanly to the latest dev versions of ckeditor and media module.
I successfully inserted images into ckeditor 4.3.1.
If i switch to source-code or save the node, the media tag gets replaced by an image tag.
i get an error on the coeditor config page:
Notice: Array to string conversion in ckeditor_load_plugins() (Zeile 342 von ../sites/all/modules/ckeditor/includes/ckeditor.lib.inc).
My setup: ckeditor dev, media dev, ckeditor 4.3.1., tested with safari and firefox.
Comment #124
deggertsen CreditAttribution: deggertsen commented#120. It works! Wonderful! Thank you everyone!
Hid all old files so that this issue only displays the most updated patches.
Comment #125
micbar CreditAttribution: micbar commentedi found the cause of the error mentioned in #122.
It is caused by the link it module. The link it module and the ckeditor module both implement the hook_load_plugins(). They both implement the same plugin and the plugin config strings get converted into an array with duplicated entries.
Comment #126
aaron CreditAttribution: aaron commentedCommitted to http://drupalcode.org/project/media.git/commit/c3cda2b. Thanks for the great work!
Comment #127
saltednutHas a followup been posted in the CKEditor issue queue for the other patch?
Comment #128
deggertsen CreditAttribution: deggertsen commentedI don't believe so. Who wants to take care of that?
Comment #129
saltednutDone. #2159403: Make CKEditor plugin system modular and clean
Comment #130
das-peter CreditAttribution: das-peter commentedYou guy's are awesome, thank you so much!!
I really hope I can spend some time in contrib & D8 during the upcoming holidays :)
Comment #131
webatelier CreditAttribution: webatelier commentedI'm bit confused about versioning and where to apply which patch to get things working ...
could anyone tell me what versions to use
media 7.x-2.x-dev ...
still need to apply patch ?
https://drupal.org/files/issues/media-add-ckeditor-support-1504696-120.p...
ckeditor module 7.x-1.x-dev
still need to apply the patch there
https://drupal.org/files/issues/ckeditor-accomodate-latest-media-changes...
and which ckeditor library ? 4.3.1 full ?
http://download.cksource.com/CKEditor/CKEditor/CKEditor%204.3.1/ckeditor...
Comment #132
saltednut@webatelier - here is a gist containing makefile snippets of what you'll currently need: https://gist.github.com/brantwynn/8015543
(The extra modules, Linkit and Media YouTube are optional.)
For those too lazy to click on a link.
Comment #133
ergophobe CreditAttribution: ergophobe commentedSlight update to brantwynn's comment - it looks like the patch for #1504696: Integration with CKEditor module was committed on 18 Dec.
The other patch RE #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working) is currently marked as Needs Review, so you still need to apply that patch... until you see a strike-through line in this sentence.
Comment #134
saltednutRe #133 - #2159403: Make CKEditor plugin system modular and clean has not been committed. The issue you linked to is this issue - its the Media side of the changes and it was committed on Dec 18th. As a result, this issue is closed (fixed).
I would recommend the following patches to CKEditor to get it working with the latest Media dev
#2159403: Make CKEditor plugin system modular and clean
#2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working)
As of this posting, nothing new has been committed to CKEditor since Nov 4, 2013.
Comment #136
PI_Ron CreditAttribution: PI_Ron commentedI've installed the modules / library combination suggested in #132 with patches suggested in #135 but I'm not seeing a media browser button available when I am editing my full html ckeditor profile.
I've ticked:
I've enabled 'Convert media tags to markup' on my full html input format.
I'm testing on a clean install of 7.25
Comment #137
reallyordinary CreditAttribution: reallyordinary commentedDid everything in #132. When I click on the "add media" button in ckeditor I just get a blank modal. Empty. Nothing in it. No error messages in the log. Just... blank.
Comment #138
saltednutRe #136: "Plugin for inserting images from Drupal media module" should be checked. "Plugin for inserting Drupal embeded media" is deprecated and should not be checked.
#137 and #136 - you should be following up here, #2159403: Make CKEditor plugin system modular and clean; this thread is closed.
Comment #139
jmuzz CreditAttribution: jmuzz commented#137 (Blank popup): It is probably caused by Media: YouTube. #2178023: Media Browser Pop up Blank upon latest upgrade
Comment #140
Martijn Houtman CreditAttribution: Martijn Houtman commentedBlank popup can also be caused by not having the Media WYSIWYG module enabled, if I recall correctly.