WYSIWYG support

It is definitely possible to extend the insert module to handle this, but

• I don't have to time to work on this right now, I've to some paid jobs first
• The proper way would be to insert a custom token instead of the full HTML, instead of inserting the output of theme_picture

If you have a patch feel free to upload it, so we can discuss the best way to handle this. If you want us to work on this, feel free to contact me using my contact form

For anybody wanting to implement this: "The proper way would be to insert a custom token instead of the full HTML", Jelle_S started added support using the media module, but in edit mode you'll see the fallback image. The output uses the 'picture' tag.

The problem is related to how CKEditor handles empty span tags.

If you want to try this, install latest versions of picture, media and insert a picture into the body of a node.

What do you think of this?

I'm approaching from an IMCE, WYSIWYG, CKEditor angle, but this might work for the Media module also:
Configure CKEditor to put a custom data attribute in the img tag with the picture group machine name, such as:
<img src=... data-picture-group="[picture_group_machine_name]">
CKEditor would have a select box for choosing the picture group in the CKEditor image dialog. Then create an input filter searching the post for img tags with the data-picture-group attribute set. This input filter would create an array with keys similar to the Render Array that's passed into picture_file_formatter_picture_view. The input filter would have a function that duplicates most of picture_file_formatter_picture_view, just omitting the parts specifically for file fields. This is where things get murky for me, but I'm hoping that, one thing leads to another, and the needed markup gets returned to the input filter and can be used to replace the original img tag.

I can take care of the CKEditor part of this, but I could use some tips for the rest.

I suppose another issue would be to copy over other attributes of the original img, but maybe that should be another issue.

Media module already has WYSIWYG support, but for stand alone use, your approach looks good. Maybe it would be good to add a data attribute for the fallback image as well.

Using picture_file_formatter_picture_view is indeed the way to go. If you have any troubles with it, let me know.

What do you mean with "I suppose another issue would be to copy over other attributes of the original img, but maybe that should be another issue.", what attributes do you want to copy?

Perhaps picture_field_formatter_view($entity_type, $entity, $field, $instance, $langcode, $items, $display) is a slightly better place to look for inspiration? It doesn't have any file_entity code in there...

Here's a first attempt at creating an inline filter. Hopefully it's easier for you guys to review this than do it yourself!

There are still several TODO's but I'm just checking in to see if things are on the right track.

One issue is that images inserted by IMCE have a root relative path, and I'm not sure how to turn this into a 'URI' with a 'Schema', as expected by theme_picture. The documentation for theme_picture suggest that the 'uri' it expects can be relative to 'base_path()', but this isn't the case, but please correct me if I'm wrong. The image_style_url() function in theme_picture turns '/sites/default/file/filename' into 'http://sites/default/files/styles/[style-name]/public/sites/default/file...', which returns an error. Currently, the filter assumes the inserted image must be public, and removes the 'file_public_path' portion of the root relative path and prefixes it with 'public://'.

Other TODO's include adding an admin section to configure the fallback image per picture group, and also make a nice interface in the CKEditor image dialog to insert the picture_group machine name as the data-picture-group attribute.

Looking forward to your feedback.

Oh, and to answer your question about other attributes: The class attribute of the img tag would need copying, if this is the method a site builder is using to float images, or otherwise the inline style attribute would need copying, I suppose to the root span tag that's returned by the picture module. I don't use inline styles, but definitely copying classes would be important.

Here's today's progress. There are a lot of additions, but it's shaping up pretty well. The only thing left to make is a CKEditor plugin that will modify the CKEditor image dialog by adding a select list for picture groups. There's a settings page added with this patch that configures which picture group to show in CKEditor, with a fallback image style per group, and even a weight to control sort order. Hopefully on Monday I'll have the finished integration! Currently everything works with hand coded image tags. I also added extra data attributes to the top level span tag so that styles can be applied more easily. For example, the following code entered in the WYSIWYG

 <img src="/thefile" data-picture-group="bartik" data-picture-align="right" />

will result in

 <span data-picture="" data-alt data-picture-group="bartik" data-picture-align="right" >
 ...
</span>

and on Monday I'll make CKEditor generate the image tag markup in a user friendly way. Using custom data attributes in this way is much more convenient with CKEditor than using classes.

Shaun Dychko, this is great! I've applied your patch and everything looks good so far.

You may have already come across this but there is a small typo in your patch with a permission name. The access callback for $items['admin/config/media/picture/ckeditor'] should be "administer pictures" not "administer picture".

I'm really excited to see what you come up with for the rest of it! Let me know what I can do to help.
Colin

Amazing job, kudos!

General remarks:

• the 80 column limit is only for comments, not for code and strings, so put them on one line.
• the use of quotes inside t() strings is not a common habit, better leave them out

+++ b/picture.admin.incundefined
@@ -216,3 +216,132 @@ function picture_admin_settings($form, &$form_state) {
+      '#title' => t('Choose which picture groups will be available in the
+      CKEditor image dialog.'),

this can stay on one line.

+++ b/picture.admin.incundefined
@@ -216,3 +216,132 @@ function picture_admin_settings($form, &$form_state) {
+        '#title' => t('"@name" picture group', array('@name' => $machine_name)),

the quotes aren't necessary

+++ b/picture.admin.incundefined
@@ -216,3 +216,132 @@ function picture_admin_settings($form, &$form_state) {
+        '#options' => array(
+          '1' => t('1'),
+          '2' => t('2'),
+          '3' => t('3'),
+          '4' => t('4'),
+          '5' => t('5'),
+          '6' => t('6'),
+          '7' => t('7'),
+          '8' => t('8'),
+          '9' => t('9'),
+          '10' => t('10'),
+        ),

no need to translate numbers, can be written as drupal_map_assoc(array(1, 2, 3, ...)

+++ b/picture.moduleundefined
@@ -935,3 +952,152 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+  // create DOM object. A consequence of using DOM is that HTML will be
+  // corrected, which normally is a selectable filter. In other words, using
+  // this filter is the same as using this filter and the Drupal
+  // "Correct faulty and chopped off HTML" together.

is there a possibility that there will be side effects?

+++ b/picture.moduleundefined
@@ -935,3 +952,152 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+function picture_page_alter(&$page) {

probably better to use http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

But is it really necessary to do this?

+++ b/picture.moduleundefined
@@ -935,3 +952,152 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+function _picture_filter_process($text, $filter) {

Maybe it is easier/better to use regular expressions to extract the pictures, see attached patch

PS: Pull first, I changed the line endings in picture.admin.inc

+++ b/picture.moduleundefined
@@ -935,3 +952,152 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+  // figure out the schema for the file since theme_picture expects
+  // a file uri, not the root relative path
+  // @TODO Please review this:
+  // I'm not sure how to get a 'uri' with a 'schema' (public or private...)
+  // knowing just the drupal root relative file path.
+  // This currently will work only with public files.
+  $file_root_relative = $image->getAttribute('src');
+  $public_path = variable_get('file_public_path', conf_path() . '/files');
+  $uri = 'public://' . str_replace($public_path, '', $file_root_relative);
+  $uri = file_stream_wrapper_uri_normalize($uri);

AFAIK there's no drupal function for this, so two options:
Insert the uri as a data- attribute when the file is inserted (will only work for managed files)
Loop all stream wrappers till we find a match, see file_get_stream_wrappers(STREAM_WRAPPERS_WRITE_VISIBLE) to get all wrappers and getDirectoryPath() to get the real path

Hey Shaun Dychko,
Any luck on the interface aspect of this? I may need to work something like this for a project this week and I'd love to be building towards the same thing.

Colin

Hey Colin, the big reveal's lined up for tomorrow! Spent today wrestling with the WYSIWYG module to figure out the best way to add a CKEditor plugin that I already built for resp_img, and learning how to send PHP variables to JS. More details tomorrow. Thanks for the interest! I'll be looking forward to your feedback and code review. I'll feel much better using this on client sites after it's passed scrutiny from other developers (who have more experience than myself!).

OK ladies and gentlemen, here's the first draft of the whole package.

@Carl, thanks for noticing the access callback typo, and it's fixed.

@attiks, thanks for all the suggestions, and I've implemented them all. In #11 you mention "Pull first, I changed the line endings in picture.admin.inc", but I don't see anything new, so this patch is against http://drupalcode.org/project/picture.git/commit/10d1ffb

With finding the Drupal URI from the root relative image src, there was a nice side effect that imce_wysiwyg now works with private images. It didn't before since it inserted src paths like /sites/default/files/private/image.jpg which are forbidden by .htaccess. Bonus! (I guess... but who uses private images anyway? They're not even really private unless managed by an access module...)

Getting the CKEditor plugin loaded by the WYSIWYG module was a real puzzle. There seems to be an issue with loading plugins that don't have buttons, and the only other Drupal module I could find that has a no-button WYSIWYG plugin is imce_wysiwyg, so the technique here borrows from that module. The CKEditor plugin will be loaded anytime a picture group has been enabled at /admin/config/media/picture/ckeditor. Loading the plugin is not controlled by a checkbox on the WYSIWYG button/plugin config page. More details are in the code comments.

When testing this, you'll probably want the picture_wysiwyg.css file in your admin theme. Please have a look at the comments in the file for details.

@attiks re: picture_page_build. I changed it from picuture_page_alter. Yes, the JS has to be loaded somehow, and the technique used with the fields doesn't work for some reason. I couldn't find good documentation for the #attached property, and the field hooks aren't even triggered for the node body field, so I guess this is treating differently? Maybe drupal_render is needed? I have no idea. But in any case, if the picture module is being used, it's probably needed on most pages, in which case there's a performance advantage to having it aggregated, which is possible only if it's included on every page. The lines 1056 to 1060 in _picture_filter_prepare_image()

    '#attached' => array('library' => array(
      array('picture', 'matchmedia'),
      array('picture', 'picturefill'),
      array('picture', 'picture.ajax'),
    )),

should probably be removed since I don't think they do anything.

I've tried to include as many code comments as I could, so I let those tell the rest of the story. I'm looking forward to your feedback!

Known issues:

• No colorbox support. Should we support this?
• I'll bet there's an unknown issue ;)

Nice job, it works. For anybody wanting to test you'll need the following

• drush dl imce_wysiwyg --dev
• drush dl wysiwyg --dev
• drush dl imce
• drush dl ckeditor
• wget http://download.cksource.com/CKEditor/CKEditor/CKEditor%204.0.1/ckeditor...

You need to patch wysiwyg, see #1853550-97: Ckeditor 4.0 - The version of CKEditor could not be detected., in short replace \' twice with [\'"] on the line starting with if (preg_match('@version:

You need to enable your picture groups for integration at admin/config/media/picture/ckeditor

+++ b/picture.admin.incundefined
@@ -216,3 +216,111 @@ function picture_admin_settings($form, &$form_state) {
+      $form[$machine_name][$machine_name . '_enabled'] = array(
...
+      $form[$machine_name][$machine_name . '_weight'] = array(
...
+      $form[$machine_name][$machine_name . '_fallback'] = array(
+        '#type' => 'select',

this can just use 'enabled' instead of $machine_name . '_enabled', same for weight and fallback

+++ b/picture.admin.incundefined
@@ -216,3 +216,111 @@ function picture_admin_settings($form, &$form_state) {
+  // Generate the custom CKEditor plugin file

is this still needed?

+++ b/picture.moduleundefined
@@ -935,3 +952,277 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+  $needles = array(); ¶

trailing space

+++ b/picture.moduleundefined
@@ -935,3 +952,277 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+    // Can't figure out the Drupal uri

dot at the end

+++ b/picture.moduleundefined
@@ -935,3 +952,277 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+        $groups[] = array('Not Set', 'not_set');

can we change the labels on the insert screen as well?

Thanks attiks for the feedback. Here's the latest.

I ran this through the Coder module on the "minor" settings to find any other issues in this patch.

I'm not exactly sure what you mean by "can we change the labels on the insert screen as well?" Do you mean the CKEditor image dialog? On the CKEditor image dialog the displayed labels are the human readable breakpoint groups. We can add a variable from the settings page to set the label for the 'not_set' value on the CKEditor image dialog, if that's what you mean. Anyhow, your comment inspired me to update the settings page at /admin/config/media/picture/ckeditor with Breakpoint Group human readable names, rather than machine names. I've also added a CSS selector suggestion since site builders wouldn't otherwise know the machine name to target. I also updated picture_wysiwyg.css which has example CSS for styling these images, and added an @file block to picture_ckeditor.js.

Thanks for mentioning testing instructions for the patch. I should have done that. I would write it a little differently, although I'm sure your instructions work:

• drush dl imce_wysiwyg (no need for --dev)
• drush dl wysiwyg (no --dev)
• drush dl imce
• wget http://download.cksource.com/CKEditor/CKEditor/CKEditor%204.0.1.1/ckedit...

Apply the patch #73 to wysiwyg: http://drupal.org/node/1853550#comment-6968584 if you're using CKEditor 4.x. This patch for Picture was developed with CKEditor 4.0.1, but it should probably work with 3.x since all it does is modify a dialog (nothing API heavy). There are some comments in http://drupal.org/node/1853550 that the CKEditor standard download is missing buttons, so the link I've put above is to the "Full" version of CKEditor (a distinction that I think is new with v. 4)

There's no need to

• drush dl ckeditor

since this patch doesn't support that module. Doing so would not be too hard, probably, since one would search the ckeditor module for a hook that fires when the CKEditor is being prepared for the page, and then load the JS as shown in picture_wysiwyg_plugin(). I don't use the ckeditor module, which is why I went with the wysiwyg module.

Include picture_wysiwyg.css in your theme CSS in order for this to be included in the CKEditor edit area, assuming you have set the WYSIWYG profile to use theme CSS. If you've set "Define CSS" at admin/config/content/wysiwyg then manually include the picture_wysiwyg.css file. This is needed for having images align properly in the edit area, and you might also consider styling the image size, perhaps using the example commented out in picture_wysiwyg.css for inspiration and substituting your own data-picture-group values.

Then, yep, enable picture groups for CKEditor integration at admin/config/media/picture/ckeditor as you said.

Let me know what's next ;)

I mean this label (in the dialog)

Done.

Some minor things, but I'm probably going to commit this during the weekend, thanks!

+++ b/picture.moduleundefined
@@ -935,3 +952,282 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+    //@TODO remove following cache line when development is finished.
+    'cache' => FALSE,

I assume this can go?

+++ b/picture.moduleundefined
@@ -935,3 +952,282 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+      // create the render array expected by theme_picture_formatter
...
+      // get the responsive markup for this image
...
+      //replace the original img tag with the responsive markup

dot needed at the end

+++ b/picture.moduleundefined
@@ -935,3 +952,282 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+ * @param $image
+ *   An img tag

add a @see picture_field_formatter_picture_view()

+++ b/picture.moduleundefined
@@ -935,3 +952,282 @@ function picture_file_formatter_picture_settings($form, &$form_state, $settings)
+  $src = $attributes['src'];

add a check to make sure we have a src

+++ b/picture_ckeditor.jsundefined
@@ -0,0 +1,182 @@
+  // this disables the browser resize handles since the width will now be set

this -> This

+++ b/picture_ckeditor.jsundefined
@@ -0,0 +1,182 @@
+        // this = CKEDITOR.ui.dialog.select

not needed anymore?

+++ b/picture_ckeditor.jsundefined
@@ -0,0 +1,182 @@
+        element.setAttribute( 'data-picture-group', this.getValue() );
...
+        if ( type == IMAGE ) {
+          var value = element.getAttribute( 'data-picture-group' );
...
+            element.setAttribute( 'data-picture-group', this.getValue() );
...
+          element.setAttribute( 'data-picture-group', this.getValue() );
...
+          element.setAttribute( 'data-picture-group', '' );
...
+        if ( this.getValue() == 'not_set' ) {

no space after ( or before )

+++ b/picture_ckeditor.jsundefined
@@ -0,0 +1,182 @@
+      //'imageSize'
...
+        // this = CKEDITOR.ui.dialog.select

debug leftovers?

With a commit on the horizon, this is getting exciting! Yes, definitely, let's remove the filter cache => FALSE.

I've also looked carefully at all the comments and made sure they're formatted as // [Capital] ... period.

Added the src check, reformatted the bracket spacing in the JS file, and removed the comments regarding "this", which were just to indicate what the JS object "this" referred to in that context.

I also added:

          var message = 'Please make a selection from ' + Drupal.settings.picture.label;
          alert(message);

to make the alert message refer to the proper label on the imageSize box.
And I added:

  variable_del('picture_ckeditor_label');

in hook_uninstall to clean things up.

I also removed:

    '#attached' => array('library' => array(
      array('picture', 'matchmedia'),
      array('picture', 'picturefill'),
      array('picture', 'picture.ajax'),
    )),

from line 1056 of picture.module since that isn't actually doing anything, and the '#attached' property isn't used in either theme_picture_formatter() nor theme_picture().

Thanks a lot for your feedback!

Committed, thanks Shaun

Does the project description need to be updated to reflect this change? Under the sub heading "Difference with Responsive images and styles" it currently reads "no support for inline images (feel free to open an issue)".

#24 project info updated

Thanks a lot attiks for committing this, and for the code reviews. I'm looking forward to using this module!

Subscribe.

BTW is this also including the WYSIWYG Media module?

It should work with Media if the Picture input filter is enabled after the media input filter in your text format configuration. You'd have to double click on images to insert the data-picture-group attribute after inserting it with Media, and you'd also want to insert a high resolution picture (preferrably the original file).

Hello,

Really awesome and promising module.

I test this feature with media module 2.x (latest dev version) and wysiwyg module

I create 2 display mode of file in media (file display)
1- the first, called, "large" use a display of core's image (with a style fixed)
2- the second, called "responsive" use a display of picture (with a picture group)

I inserted an image with media module in a body field and used the both display mode created

1- In the first case, the same style (large) is always used. I double click on image to insert the data-picture-group attribute after inserting it with Media, and save. But the configuration of the data-picture-group attribute is not saved

2- In the second case, It's the same thing, the data-picture-group attribute is not saved when I double click on image to insert them. But the image is responsive, with the display mode i called "responsive" wich is defined on a picture group (in /file-types/manage/image/file-display). You can see in the html this span with data-attribute empty.

<span data-alt="" data-picture="">
<span data-height="75" data-width="120" ......</span>
...
<img .....>
</span>

In both case, the attribute image-alignment is not saved too.

Otherwise, are you plan to include CKEditor module too ?

Thanks

EDIT: it's done : media module support

To avoid overload on this issue, it sounds like there are two separate issues here: 1) CKEditor module. Definitely doable, but not there yet. 2) Supporting the Media module in the WYSIWYG. Please create these issues with your notes, and I'll try to help there when I can if no one else gets there first.

The current implementation works as per #18 with IMCE.

I want to understand what I need to do to use picture with wysiwyg.

Do I need to use the latest dev version of Picture?
Do I need to also install one the patches above?
How did I install the patch?

Thanks!

#31 you need the latest dev version of picture, the patch is already included.

So, upon looking in the new dev module I can see the patch has indeed been applied (although with a few tweaks). yet sadly it doesn't appear to be working for me. So my question is, does it work for everyone else, and if so, was there any other dev/patched versions required of other modules, is the end of this ticket implies this is no longer necessary, but wonder if anything has changed since.

Thanks,
Dan

Ignore that, schoolboy mistake i'm afraid. If anyone else gets to this point, to enable this it isn't automatic, you need to go into your WYSIWYG editor and check the responsive checkbox as you would things like bold/italic/imce.

If anybody feels like writing documentation (on d.o. or a blog post) that would be great, I don't have much time for the moment

Documentation available http://drupal.org/node/1983312

Feel free to review it. English is not my native language.

#36 Thank you!

Hi,
there is a problem if drupal is not installed at root ( / ).
I've a site which is reachable at http://www.example.com/drupal7/ and the module doesn't translate img into span in CKeditor (but it works fine with image in a node).
The problems seems to come from function picture_image_uri($src) (picture.module file).
If I put
$needles[$scheme] = "drupal7/" . $needles[$scheme];
after the line
$needles[$scheme] = trim($stream_wrapper->getDirectoryPath(), '/');
everything is working fine.
This is an ugly hack but I'm a newbie to Drupal and don't know how to fix this in an elegant manner.

Can you try it with $base_url . '/'? make sure to add global $base_url; on the line above.

No, it doesn't work.
I did this:

    global $base_url;
    $needles[$scheme] = $base_url . '/' . trim($stream_wrapper->getDirectoryPath(), '/');

And the result of $needles[$scheme] is:
http://www.example.com/drupal7/sites/default/files

With previous changes, it was:
drupal7/sites/default/files

my bad, try with $base_path

    global $base_path;
    $needles[$scheme] = $base_path . trim($stream_wrapper->getDirectoryPath(), '/');

Almost...
Works with:

    global $base_path;
    $needles[$scheme] = trim($base_path . $stream_wrapper->getDirectoryPath(), '/');

Because of the leading '/'

Thank you for your help.

hey guys,

I have the latest dev version and have configured Picture and WYSIWYG CKeditor to use the responsive image settings from picture.

When I select my photo in CKeditor on my content type, I can add the picture style to it, and the data tag gets added to the img tag. However, the responsive images aren't being injected onto the page...

Is there anything I'm missing here?

Thanks!

Craig

#45 This issue is closed, so can you open a new one in the future, otherwise we might miss it.

To answer your question: did you enable the picture option in your text format?