[drupalImage] Optionally apply image style to images uploaded in CKEditor 5

Yeah, what we talked about is that from an end-user point of view, both an image upload field that's attached to a Field API "image" field and an image upload field found inside CKEditor are conceptually the same: an upload field, where you stick an image. It's therefore very perplexing when one has responsive goodness without having to think about it and the other doesn't, and it makes D8 look kinda bad, considering mobile was such a strong focus for us.

Thanks, Wim! :)

Issue for the picture module at #2000178: Add support for inline pictures in short this is possible using an image tag with a data-picture attribute

+++ b/core/modules/image/image.module
@@ -592,7 +593,7 @@ function image_form_editor_image_dialog_alter(&$form, $form_state) {
+      '#default_value' => isset($image_element['data-image-style']) ? $image_element['data-image-style'] : $image_options_keys[0],

Can't that just be key($image_options)?

I spend a long time discussing this with WimLeers (I was a little confused, IRC to the rescue!) . Overall its a great idea, Drupal is quite unique if we pull this off.
However it requires a lot of fine tuning of the details.

I was quite confused by "Standard responsive image" as a option. Because that immediately made me think about image styles, instead of picture maps. I expect that users will also face this confusion, and will go to Configuration > Media > Image styles and end-up terribly confused that they don't find "Standard responsive image” in there.

Just an idea. What if we turn the label around and indicate in some way that its different from ordinary image styles?

Responsive image style: “Standard"

Although I am worried that “responsive” might be a odd label for many content authors and possibly outdated in a few years. It is the label we use elsewhere. I think we can safely assume that if you use picture maps that they will be responsive.

If anyone has a better label than “picture mapping” that could work too.

(FYI, in D7 discussions with Nate I offered the idea of using “image styles” instead of “image cache”. It looks like this situation is just the same, we gotta find a good label.)

pls proof thats the pictures are possible to add in a picture sitemap.
if not we need to open a new issue. thanks

If you mean the following it isn't supported by any responsive solution, but it's possible to do. I think it's better to open a new issue and link it to this one. You'll probably need to define multiple maps depending on the image to be used.

<img src="..." usemap="#mapname" />
<map name="mapname">
  <area shape="rect" coords="9,372,66,397" href="..." />
</map>

https://drupal.org/node/2126287 done ..maybe bad writeup

+++ b/core/modules/editor/lib/Drupal/editor/Form/EditorImageDialog.php
@@ -130,6 +130,8 @@ public function buildForm(array $form, array &$form_state, FilterFormat $filter_
+    $filters = $filter_format->filters()->getAll();

is this used? doesn't seem to be.

Half way through review

I'm curious, why not support all the formatters available to image fields rather than just only show image styles? Something like <div data-image-fid="5" data-image-formatter="image" data-image-formatter-options="{json encoded string of optional formatter options" />

Note that the breakpoints property of a breakpointGroup is now protected. You need to use the getBreakpoints method instead. This effects two lines in the new FilterPictureMapping.php file that this patch installs.

Original code

  if (isset($picture_mapping->breakpointGroup->breakpoints[$breakpoint_name])) {
              $breakpoint = $picture_mapping->breakpointGroup->breakpoints[$breakpoint_name];

Suggested changes.

  if (isset($picture_mapping->breakpointGroup->getBreakpoints[$breakpoint_name])) {
              $breakpoint = $picture_mapping->breakpointGroup->getBreakpoints[$breakpoint_name];

#12: To be sure, you can call reset($image_options) first (see http://www.php.net/reset)

#20 makes me think about arbitrary embeds of fields and picking a view mode / field formatter :)

@Dave Reid / @Wim Leers:

that'd result in a <div class="field"><div class="field-item"><img src="http://drupal.org/foobar.jpg" /></div></div> inside a text body. That's… excruciatingly bad.

Not necessarily. The above is if you call field_view_field() : "html for a fully themed field, with wrappers, rdf, label & multiple values".

But you can still let a single formatter run with field_view_value() : single value, no decoration, pure formatter output

(side note: field_view_field() / field_view_values() are still old-style functions, and their signatures are still largely modeled after the D7 entity model. This is being taken care of in #2134887: move field_view_field() / field_view_value() to methods)

@Wim Leers : I guess you mean
"but then you're no longer rendering a *file* [what the patch does currently], you're rendering a field item" ?

If so, then yes, true, would be a big change. You can't easily turn an arbitrary file into a "file field item" - on which entity, with which field definition ?
Which I guess is the reason why this patch only goes with "rendering direclty through an image style" rather than rendering through a field formatter - there's no field :-)

Hmm - I think we're saying the same thing ;-)

At any rate, I agree with #29. When I wrote #25, I overlooked the fact that the patch works on files, not on "values in an image field".

Just wanted to say thank you so much for working on this! I've been a Media user for a while now (and have been reading the media and asset management discussions with regards to Drupal 8 - https://groups.drupal.org/node/384813), although the main use case we have for smaller sites is exactly the one you've addressed here.

Apologies in advance for my unfamiliarity with the patch submission process in Drupal core - but can you tell me what the status is of this patch? Above it says 'Needs Review' and so I'm assuming it's not yet been accepted to core - and won't yet appear in the Drupal 8 Dev Alpha download correct?

If I'd like to to experiment with this patch, will I need to clone the Drupal 8 git repository, and then download an apply the patch separately?

Thanks again....

No worries, we all started somewhere :) Need review means that it is not yet in core, you're correct. It needs someone to test that the functionality works as expected and that the code is up to core standards. Until it is in the "Fixed" state, it won't be in what you download from the alpha version.

The process is the one you described, you need a git clone of the drupal repository and you need to apply the patch on top of it: https://drupal.org/patch/apply

Thanks for this nod_ - and thanks too for the URL on patching. :-)

The current patch no longer applies, and there are a number of suggestions for fixes in comments above since the last patch.

Rerolled the patch.

I revised the latest patch "2061377.36.patch".
There's remaining two issues:
- After image upload there's an error in the file core/modules/file/file.js.
- On saving the content with an image it won't save the parameter of the used image-style.

Added missing files to patch.

hello, just to inform you about the latest news and whats going on. http://www.matthew-steele.com/responsive-images-picture-srcset/

Will be looking at this.

Still needs re-roll...

If nobody has started yet, I'll reroll this.

Jelle_S - are you at the sprint in Amsterdam? I've just started looking at this issue too, happy to help.

Yes I am, I'm at the table next to the aegir guys. Red DrupalCon Amsterdam T-shirt and glasses ;-).

Straight reroll, tests will probably still fail since they failed in #42 as well, but let's first get this reroll out of the way.

Sorry Jelle, having trouble locating you, as aegir isn't marked on the table plan. Whereabouts are you sitting?

I'm on the media table (ground floor, by the window on the right hand side as you come in the room). And I'm wearing the red amsterdam t-shirt too :)

New patch should fix the failing tests.

Sorry I don't have dreditor on this computer. However, quick review:

1. $form_state in the hook_XXX_alter() and the hook_XXX_validate() needs to be updated to current FormStateInterface API
2. Maybe add some tests for 1) since the patch shouldn't have passed.

More in-depth reviews soon...

CNW for #57

Re #57:
@Erik is working on it, I told him about the test as well since it shouldn't have passed indeed. I just rerolled an fixed the failing tests.
Since the last patch was pretty old, it seems to require a considerable amount of work since a lot has changed (FormStateInterface, Breakpoints are now plugins, no longer entities, and probably many more)

Updated #56 to fix the following issues:

• use FormStateInterface instead of array in form submit/validation functions
• use SafeMarkup::set() for form #prefix/suffix
• filters now return a FilterProcessResult object instead of a string - see https://www.drupal.org/node/2278483
• reflect change that Breakpoint is a plugin instead of an entity

Still needs work, but hopefully this helps move things on.

+++ b/core/profiles/standard/standard.info.yml
@@ -25,6 +25,7 @@ dependencies:
+  - responsive_image

This turns on the responsive_image module for the standard profile - should that really be in here?

I think we will need it, but maybe this is not the right issue to do it (if so create a new issue so we do not forget). The idea is to ship Drupal 8 with responsive image enabled so site builders will follow best practices.

Already an issue for that: #1855412: Enable responsive_image.module by default for standard install profile

1. +++ b/core/profiles/standard/config/install/filter.format.basic_html.yml
   @@ -18,6 +18,12 @@ filters:
   +    weight: 6
   +    settings: {  }
   +  filter_picturemapping:
   +    id: filter_picturemapping
   +    provider: filter
   +    status: true
   

2. +++ b/core/profiles/standard/config/install/filter.format.full_html.yml
   @@ -9,6 +9,12 @@ filters:
   +    weight: 7
   +    settings: {  }
   +  filter_picturemapping:
   +    id: filter_picturemapping
   +    provider: filter
   +    status: true
   

3. +++ b/core/profiles/standard/config/picture.mappings.standard_responsive_image.yml
   @@ -0,0 +1,13 @@
   +id: standard_responsive_image
   +uuid: 789bae80-c66c-4e38-bf00-bb450f4bac33
   +label: 'Standard responsive image'
   +mappings:
   +  module.toolbar.narrow:
   +    1x: thumbnail
   +  module.toolbar.standard:
   +    1x: medium
   +  module.toolbar.wide:
   +    1x: large
   +breakpointGroup: module.toolbar.toolbar
   +status: true
   +langcode: en
   

This should be removed again as well then, because it won't work unless the responsive_image module is enabled.

Stumbled upon #2350061: Javascript error when uploading image through the editor image dialog while I was working on this patch.

This needs a reroll, I unassigned it so somebody else can work on it.

Rerolled.

Hope it works!

Tweaking the title name to reflect the change of the picture module to the responsive image module.

Last patch won't apply anymore.

Rerolled and fixed several API changes I spotted. (sorry, no interdiff yet)

#2513604: Create default responsive image styles provides some sensible default responsive image styles. It would be good to work on getting that committed, rather than default responsive image style in the Toolbar module added in the patch above. Once that is in, I think it's worth looking at #1855412: Enable responsive_image.module by default for standard install profile so the Responsive Image module is turned on by default, which would allow those default responsive image styles to work out of the box with the Edit widget.

Meanwhile, Entity Embed will be there in contrib and work for all output and field formatters for images. I think that's acceptable for now.

Here's a reroll of #73 to get us up to date.

This removes the default image style added in this patch. Not necessary anymore since we already have default responsive image styles.

Glitch due to testbot. Here's the patch.

Ah, I was really confused by that. Thanks Wim!

This patch needs a bunch of renames: picture mappings no longer exist, they are now responsive image styles. There's todos in there with a link to a fixed issue (#2123251: Improve DX of responsive images; convert theme functions to new #type element) . I'll have a stab at it.

This patch used a lot of old code that is no longer there. Thankfully the refactoring was quite smooth with #2123251: Improve DX of responsive images; convert theme functions to new #type element fixed. I couldn't get the data-attributes to stick somehow. They are never in the output... The rest of the patch seems to work ok. Dialog has correct contents etc. Maybe some theming issues in the previews, but I'm gonna leave those up to the people that know about how things should look in order to look good ;-). Unassigning for now, maybe I'll revisit this later today.

Oh, and I don't know how testable this code is, with all the JS involved, but it feels like this should have tests. Right now there is no response yet for the patch in #81, but if that comes back green, this definitely needs test because that code should just throw fatal errors I think.

#87

+++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
@@ -0,0 +1,119 @@
+        $image_style_id = $node->getAttribute('data-image-style');

+++ b/core/modules/responsive_image/responsive_image.module
@@ -506,3 +508,171 @@ function responsive_image_library_info_alter(array &$libraries, $module) {
+    $responsive_image_style = entity_load('responsive_image_style', $attributes['data-responsive-image-style']);

Don't we have to whitelist the new data- attributes?

Also don't we need javascript to add them, similar to core/modules/ckeditor/js/plugins/drupalimage/plugin.js

@Wim Leers?

Yeah that's what I thought as well but I'm not sure about the approach, that's why I posted my progress so far so someone with more knowledge about this could chime in :-D

#90 That would be @Wim Leers I guess, I had a quick look, the other data attributes are added during install, but I guess there's a dynamic way as well, so we only have to add them when needed.

#92 What else is remaining, because now it is a bit hard to test. Or do you have pointers where to look? I had a quick look at the javascript, but both allowedContent and requiredContent are hard-coded, I don't see a way to override these?

Layout of the preview is of, but not sure we even need it?

Talked to Wim on IRC, the short version:

- Dialog save is working
- Newly added PHP is working
- AJAX post back is working
- but

WimLeers attiks: We need to update those plugins to take into account *any* data- attributes basically
WimLeers attiks: that will allow any filter to add additional data- attributes in EditorImageDialog, and then have them be reflected

Decided it has to be part of this issue so we have a use case as well, but not for me today

Quick update: we have a working version, if anybody has an idea how this can be tested, please let us know

Here's the working patch. Tested on Full HTML format, see #92.

I must say I enjoyed writing this patch, DX has really improved in D8 compared to D7!

@Wim Leers I guess we need to add the data- attributes to the allowed filter as well during install?

#98

- needs d8 to d8 system update
- needs d8 to d8 responsive image update
- needs to add data-image-style to standard profile config
- needs to add data-responsive-image-style to hook enable

Update: Removed the hook enable, since it will get added once the filter gets enabled

#101

1. Is from CKEditor, see http://docs.ckeditor.com/#!/api/CKEDITOR.style
2. Thanks
3. Indeed for consistency, is a bit small to put in a separate issue
16. Because the img is indented with 2 spaces in the twig file, if we don't do the trim, the first node is #text

After that is all fixed, the only remaining thing to be done is some additional JS for on the text format/text editor configuration page, so that if you enable either of these filters, that the filter_html filter is also updated accordingly automatically, so that the required attributes are also whitelisted automatically.

Is this how the image caption / image align is done?

#103 Just discovered that after selecting image style and hitting save, they get added

Adding 'rc deadline' tag, will assign to @webchick once the code is ready

Change record added: https://www.drupal.org/node/2579075

Patch with #1001, 2 and 3 fixed. 4 has no use because of #104.

RE #101:

Thanks!!

:) :) :)

Which specific aspect(s) are you referring to though?

Everything is so easy! Want something new for CKEditor? Write a CKEditorPlugin! Does it need a filter? Great! Write a filter (also a plugin). Everything is in one place. No more info hooks where you define callbacks (watch the typos!), then write that callback, etc., etc... Now the interfaces you inherit from tell you exactly what to do.

Then there's the JavaScript. My God! The JavaScript! I can actually read and understand it first time around now... CKEditor helps too of course (see further down this comment).

1. Not sure if it's new, or how new it is, but I just opened the docs to see if there was a way to define them that would make them easier to alter/extend and then I found this (http://docs.ckeditor.com/#!/api/CKEDITOR.filter.contentRule) about the requiredContent property:

This is a simplified version of the CKEDITOR.filter.allowedContentRules type. It may contain only one element and its styles, classes, and attributes. Only the string format and a CKEDITOR.style instances are accepted. Required properties are not allowed in this format.

and this http://docs.ckeditor.com/#!/api/CKEDITOR.style

2. I thought so too, hence the DX comment ;-)

3. Yes consistency, I'll take it out again if you want to put this in a separate issue.

4. Yes it does :-)

5. Gotcha.

6. <3<3

7. <3<3

8. I was following the examples I found in core. You can use CKEDITOR.style for allowedContent as well, so if you want I can change it too in this patch (but I'll have to change it in drupalimage as well).

9. Yup.

10. Uhu.

11. Yeah it crossed my mind but I didn't do it eventually, will fix.

12. Then love is what it will get.

13. More love coming in!

14. Same comment as above ;-)

15. Only the label or also the plugin name?

16. See #102.16

17. Will do!

After that is all fixed, the only remaining thing to be done is some additional JS for on the text format/text editor configuration page, so that if you enable either of these filters, that the filter_html filter is also updated accordingly automatically, so that the required attributes are also whitelisted automatically.

See #104

NR for testbot, afterward back to NW for todo's listed above.

Ok so about #101.8 & #101.14:

We could make CKEDITOR.style objects out of them but CKEditor themselves defined it as a plain js object (see http://docs.ckeditor.com/#!/api/CKEDITOR.filter.allowedContentRules, 'the object format', and see the CKEditor source code). That means we have to either overwrite it (=> create an entirely new CKEDITOR.style object with the correct defaults) or find a way to parse it in to a CKEDITOR.style object. Either way, I would argue that's out of the scope of this issue? It would still be nice to get it in though. Same as the link plugin refactoring. It's still in this patch for now, but I'm open to creating separate issues for them both (link plugin & allowedContent)

EDIT: I didn't have the time to look through the entire source code of CKEditor, but maybe a CKEditor developer could help us in the right direction. There might be code somewhere in CKEditor to parse such a string in to a CKEDITOR.style object.

• Filter was not yet added to the full html format.
• Restricted images filter must run before image style filter for reasons yet unknown to me.

Unassigning for reviewers

Not sure if the preview is needed, it will make the dialog messy and complex, so I'm in favor of removing it.

Some nitpicks and we need some user documentation on the filters.

1. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
   @@ -0,0 +1,79 @@
   +      // Override the image2 widget definition to handle the additional
   

   image2 = drupalimage in this context

2. +++ b/core/modules/responsive_image/js/plugins/drupalresponsiveimagestyle/plugin.js
   @@ -0,0 +1,79 @@
   +      // Override the image2 widget definition to handle the additional
   

   drupalimage

3. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -506,3 +508,171 @@ function responsive_image_library_info_alter(array &$libraries, $module) {
   +  // When responsive image functionality is available, disallow the user from
   +  // specifying the dimensions manually, and from selecting an image style, only
   +  // allowing a responsive image style to be selected.
   

   This needs to be clarified on the filter as well, basically image style and responsive image style are mutually exclusive

4. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -506,3 +508,171 @@ function responsive_image_library_info_alter(array &$libraries, $module) {
   +      // @todo: what if the image is not valid?
   

   We should bail out

Agreed with Wim here, the guidance from @webchick in the other topic is clear and allows for more breathing room to perfect this approach.

Preview is from my point of view required to make sense of this very complex interface already.

Just so everybody is clear, this isn't only about responsive images. Right now you can't select normal image styles when placing images via WYSIWYG. You have to manually specify the height and width of the image. It's pretty awful.

I totally understand the concerns, and I'm on board with releasing this in 8.1. Just want to make sure the full picture is clear on what this issue is trying to do.

Can we at least change the existing ckeditor javascript so this can be fixed in contrib?

#117 Done, created #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored

Is there a way to do this without *requiring* responsive image module? Bonus points if it still works if responsive image module is enabled.

#119 responsive image is not required, the image style part work independent. Since there's no change this will get added before 8.0.1, we will move it to the picture module, so at least it will be usable.

It would be nice if #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored gets committed so it is way easier to do it in contrib, but even if it doesn't get committed we still can override it.

I think this is worth one last consideration.

An earlier version of this patch turned on responsive image module in the standard profile. But the latest patch... doesn't. Since turning on the responsive image module was the primary reason to push this off, and that reason looks to be no longer an issue, let's take one last look here. This is an improvement for WYSIWYG images even when responsive image module is turned off, so I think this is worth consideration.

I'll do a manual test.

The good news is that I got this to work when manually testing! The bad news is that it took some doing to do so.

1) With this patch applied, I can confirm that responsive image module is NOT enabled by default. Good!

2) By default, I can go to an article, I can use wysiwyg to place an image, and I can select an image style when doing so, and it appears to work. Good!

3) If I enable responsive image module, and I go to an article to place an image, I can only select an image style, not a responsive image style. :(

4) If I go to text filters, and turn on the responsive image filter in Basic HTML, and don't change its position, when I go to an article, I can't place any images with wysiwyg. Strange.

5) If I turn on responsive image filter for Full HTML, I think that worked right away. Good!

6) When I went back to Basic HTML and moved the responsive image filter after the image style filter, then I could select a responsive image style. That was confusing and not intuitive.

7) With responsive image filter on, the label for the selector was still image style. It would make more sense if that was labelled responsive image style.

8) When I view source on the output image, the responsive images have a src element, which will result in a double download, as described in #2481637: Use src in fallback image. I know the spec says there should be a src element, but when I checked with some RICG folks the advice I got was that at this time, probably best to leave off src when using Picturefill. Browser support is growing but not enough to risk double downloads.

So things I'd like to see changed:
- If I enable the responsive image module, the responsive image filter should be turned on and placed in the correct order in all of the default text formats. I shouldn't have to figure out how to turn that on: I've already decided to turn on the responsive image module.
- With responsive image filter, label for style select in dialog should be "Responsive image style:"
- Drop the src attribute in the img element output for responsive images.

#122

1. ok

2. ok

3. is by design, filters are never enabled automatically, extreme example: what if a module implements a PHP filter

4. strange indeed

5. good

6. see 4

7. not good, needs to be fixed

8. this is out of scope for this issue and was introduced by #2348255: Provide option to use srcset and/or sizes attributes on img tag instead of the picture element which was approved by all of us. So if you/we want to change this, best to do it in a separate issue.

Thanks for the extensive review

This fixes the label in the dialog.

If the src in the source code is a separate issue, okay, we'll handle it elsewhere.

I retested this manually, and after enabling the filter, the image icon didn't immediately show up in the wysiwyg toolbar, but it did after I cleared cache. And then it didn't matter what position I had for the responsive image filter. Lots of things require clear cache, so that resolves my concerns there.

If we don't automatically activate filters for other modules, that resolves my concerns there as well. I'd rather this exist with some instructions than to not exist at all. We can make this easier once we finish all cleanup for responsive image module and can make it active by default.

In my opinion, this is good to go.

Since we basically split this issue, I'm going to postpone it on #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored, once that is committed we can add it in contrib first to test it, and move it back to 8.1.x

Filed #2580695: Prevent responsive image without picture element from causing double downloads of images as a follow-up.

It would be really great to get this in for Drupal 8.1. I just got really excited about the image handling in ckeditor in Drupal 8 then realised its fundamentally flawed without image style control and picture based responsive images.

This is maybe my top priority for 8.1. We were nearly there before RC, I would think this would be doable.

It looks like #125 should be our starting point for picking this up again. We need a fair amount of test coverage to move forward. Tests are not my strong suit. Would welcome any and all assistance with that.

Any tips how i can pick this issue back up? last patch is from 5 months ago

@mitchalbert were you able to pick this back up?

First step is probably a reroll of the most recent patch. Entirely possible we'll need to do some updates due to D8 changes since last patch was rolled.

@Jon@s im working on rerolling the patch against 8.2.x-dev

hopefully done today

Rerolled the patch against 8.2.x-dev

some minor fixes were needed to get things working

For testbot...

damnit, forgot to add the new files to the patch.
Not a git wizard here :( i try again

And for the testbot again …

Here is a updated patch. Fixed some issue's compared to previous patch

• Fixed css/attach to form
• Fixed preview in editor(was not working correctly

I was wondering, is the image_form_editor_image_dialog_validate the right place to alter the attributes?

@mitchalbert I'm currently porting your patch to a contrib module so that I can use it on 8.0.x and I figure that way it can get plenty of community testing before it gets commited to core. Currently I got most of the code ported, and managed to fix an issue with upcast/downcast in ckeditor (mentioned at start of #125), still need to work on the install/update functions especially addressing issues mentioned in #125. Also trying to determine if the template/css changes are actually needed and add some test coverage. I'll create a sandbox shortly.

Any chance your good at writing tests? It's really not my strong suit.

@Jon@s Great! can't wait to see what you did so far. You also update my patch or just sandboxing?

about writing tests; I didnt make any tests for D8 yet. Perhapse this issue is a good motivation to start with writing tests :)

@mitchalbert i tried to test your patch but got this js error:

Error: cannot call methods on dialog prior to initialization; attempted to call method 'option'
...""),isReady:!0,error:function(a){throw new Error(a)},noop:function(){},isFunctio...

@Jon@s what the status of the module ? is there a sandbox yet ?
Thanks

I've been testing the inline responsive images module at:

https://www.drupal.org/node/2702981

works good for me. I posted some testing results there. Some evolved version of it would be great to have in core to assist with resolving the two-year old deficiencies noted in this thread. However, even as a contributed module it allows you to wrap an image in a style.

@prics

What version of drupal did you run the patch against?

For the module:
https://www.drupal.org/project/inline_responsive_images

@mitchalbert i'm using drupal 8.2.x

What i actually need is to have the width and height in the dialog for the inline image and tried to use this plugin:
http://ckeditor.com/addon/image

But it's not compatible with the core's image2 plugin, for which i can't find a way to override ... so i need to find another solution.
The patch with image styles looked like a good idea and i'm further exploring it.

Do you maybe have a suggestion for my issue ?
Thanks

It's been awhile since has work has been done on this, but I'm going to try my best to move this forward at DrupalCon NOLA.

The reroll in #142 was missing new files, and then #144 added those files back in but also made some additional changes. I think that's going to be hard to untangle. So I wanted to start over with a clean reroll. I'm glad to look at #144 to see if I can find what changes were made there to see if those should be incorporated.

This reroll is 39k, while the patch I based this off (#124) was 44k. I wanted to double-check what the discrepancies were, so I did a diff of the patches. This is messy, but if anyone wants to double-check this, here's essentially the conflicts I resolved in this patch. From what I could tell when resolving conflicts, these bits were things not really needed because some of the changes made in the #124 patch were resolved elsewhere in core.

> index 2cd6254..3e45eff 100644
> --- a/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
> +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
> @@ -30,7 +30,17 @@
>          }
>  
>          // Override requiredContent & allowedContent.
> -        widgetDefinition.requiredContent = 'img[alt,src,width,height,data-entity-type,data-entity-uuid]';
> +        widgetDefinition.requiredContent = new CKEDITOR.style({
> +          element: 'img',
> +          attributes: {
> +            'alt' : '',
> +            'src': '',
> +            'width': '',
> +            'height': '',
> +            'data-entity-type': '',
> +            'data-entity-uuid': ''
> +          }
> +        });
>          widgetDefinition.allowedContent.img.attributes += ',!data-entity-type,!data-entity-uuid';
>          // We don't allow <figure>, <figcaption>, <div> or <p>  in our downcast.
>          delete widgetDefinition.allowedContent.figure;
> diff --git a/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js b/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
> index e68c809..94c5353 100644
> --- a/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
> +++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
> @@ -51,7 +51,10 @@
>          }, true);
>  
>          // Override requiredContent & allowedContent.
> -        widgetDefinition.requiredContent = 'img[alt,src,width,height,data-entity-type,data-entity-uuid,data-align,data-caption]';
> +        var requiredContent = widgetDefinition.requiredContent.getDefinition();
> +        requiredContent.attributes['data-align'] = '';
> +        requiredContent.attributes['data-caption'] = '';
> +        widgetDefinition.requiredContent = new CKEDITOR.style(requiredContent);
>          widgetDefinition.allowedContent.img.attributes += ',!data-align,!data-caption';
>  
>          // Override allowedContent setting for the 'caption' nested editable.
> @@ -63,9 +66,12 @@
>          // Override downcast(): ensure we *only* output <img>, but also ensure
>          // we include the data-entity-type, data-entity-uuid, data-align and
>          // data-caption attributes.
> +        var originalDowncast = widgetDefinition.downcast;
>          widgetDefinition.downcast = function (element) {
> -          // Find an image element in the one being downcasted (can be itself).
> -          var img = findElementByName(element, 'img');
> +          var img = originalDowncast.call(this, element);
> +          if (!img) {
> +            img = findElementByName(element, 'img');
> +          }
>            var caption = this.editables.caption;
>            var captionHtml = caption && caption.getData();
>            var attrs = img.attributes;
> @@ -94,6 +100,7 @@
>          //   - <img> tag in a paragraph (non-captioned, centered image),
>          //   - <figure> tag (captioned image).
>          // We take the same attributes into account as downcast() does.
> +        var originalUpcast = widgetDefinition.upcast;
>          widgetDefinition.upcast = function (element, data) {
>            if (element.name !== 'img' || !element.attributes['data-entity-type'] || !element.attributes['data-entity-uuid']) {
>              return;
> @@ -103,6 +110,7 @@
>              return;
>            }
>  
> +          element = originalUpcast.call(this, element, data);
>            var attrs = element.attributes;
>            var retElement = element;
>            var caption;
> diff --git a/core/modules/ckeditor/js/plugins/drupallink/plugin.js b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
> index 4d0832a..f4db09e 100644
> --- a/core/modules/ckeditor/js/plugins/drupallink/plugin.js
> +++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
> @@ -13,8 +13,17 @@
>      init: function (editor) {
>        // Add the commands for link and unlink.
>        editor.addCommand('drupallink', {
> -        allowedContent: 'a[!href,target]',
> -        requiredContent: 'a[href]',
> +        allowedContent: {
> +          a: {
> +            attributes: '!href,target'
> +          }
> +        },
> +        requiredContent: new CKEDITOR.style({
> +          element: 'a',
> +          attributes: {
> +            href: ''
> +          }
> +        }),
>          modes: {wysiwyg: 1},
>          canUndo: true,
>          exec: function (editor) {
> @@ -111,8 +120,17 @@
>        editor.addCommand('drupalunlink', {
>          contextSensitive: 1,
>          startDisabled: 1,
> -        allowedContent: 'a[!href]',
> -        requiredContent: 'a[href]',
> +        allowedContent: {
> +          a: {
> +            attributes: '!href,target'
> +          }
> +        },
> +        requiredContent: new CKEDITOR.style({
> +          element: 'a',
> +          attributes: {
> +            href: ''
> +          }
> +        }),
>          exec: function (editor) {
>            var style = new CKEDITOR.style({element: 'a', type: CKEDITOR.STYLE_INLINE, alwaysRemoveElement: 1});
>            editor.removeStyle(style);
14c128
< index 7f8314a..3b6eca4 100644
---
> index 2c222f8..7c3796e 100644
27c141
< @@ -483,3 +485,105 @@ function image_field_config_delete(FieldConfigInterface $field) {
---
> @@ -473,3 +475,105 @@ function image_field_config_delete(FieldConfigInterface $field) {
559c673
< index 89ac37f..9040b1e 100644
---
> index 8060498..c0f8c98 100644
982c1096
< index c716891..43665ab 100644
---
> index 47f6dca..94f27cd 100644
985c1099
< @@ -1650,5 +1650,27 @@ function system_update_8014() {
---
> @@ -1808,5 +1808,27 @@ function system_update_8011() {
991c1105
< +function system_update_8015() {
---
> +function system_update_8012() {
1011c1125
<   * @} End of "addtogroup updates-8.0.0-rc".
---
>   * @} End of "addtogroup updates-8.0.0-beta".

This makes me think we'll want to go through this patch with a fine-tuned comb to make sure that everything in here is really necessary and is the minimum change necessary to move this forward. It's entirely possible some of the things this patch was trying to address before have been resolved elsewhere, even if they were resolved in slightly different ways.

Hopefully this is a good starting point to move forward.

Hmm. Well, manually tested #152, and this isn't actually working. No image style field is showing up in WYSIWYG. So that's sort of an issue. But hey, the patch applies cleanly?

I took a look at the differences between #144 and #142, to get back the fixes made in #144. I'm still not seeing the image style selector in the dialog. I realized to test this I probably needed to run the update hook. However even with that, I'm still not seeing the image style selector.

The end result is that this shows why this patch so very much needs tests, as that would help to track down why exactly the patch isn't working.

Hoping to find some help writing tests during #DrupalConNOLA sprints.

Missed adding in the files.