Updated: Comment #212

Problem/Motivation

Inserting an image in the text editor dialog today allows the user to fiddle with image dimensions. It doesn't even have aspect ratio locking.

It's not great for the authoring experience nor for structured content reasons that users are defining the specific dimensions of every single image they insert. It'd be much better to allow them to choose from image styles — just like we do for image fields.

Proposed resolution

Allow an image style to be selected in the image dialog, which gets stored in a data-imagestyle attribute, and is handled by a yet-to-be-added imagestyle filter.

Remaining tasks

  1. Initial patch: select image style in dialog, inserting that sets a data- attribute, and a filter transforms the end result.
  2. Get #1589176-4: Follow-up: Use data-* to store #states api informations — a blocker to this patch — committed.

User interface changes

  • The new ability to select an image style.
  • A new filter.

API changes

None.

Files: 

Comments

Wim Leers’s picture

Title: Allow image style to be selected in Text Editor's image dialog (image refs in text fields, necessary for structured content) » Allow image style/picture mapping to be selected in Text Editor's image dialog (necessary for structured content)

webchick labeled Drupal 8's inability to have responsive images (i.e. the <picture> element) in the body field as a bug over at #2099909-13: Images in body field are not responsive. Looks like we might indeed want to have the ability to select picture mappings in the image dialog then!

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue tags: +sprint

Alright, confirmed, I'll be working on this in the next two weeks :)

Wim Leers’s picture

Issue tags: +mobile

Adopting the tags of #2099909: Images in body field are not responsive, which was marked as a duplicate in favor of this one.

webchick’s picture

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! :)

attiks’s picture

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

Wim Leers’s picture

#5: Thanks!

Wim Leers’s picture

While working on this, I encountered an important regression in the #states API that blocks this patch, see #1589176-14: Follow-up: Use data-* to store #states api informations.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Testing the patch

No setup necessary. Everything enabled out of the box, including a default picture mapping. Just install on simplytest.me, go to node/add/article, click the "image" button in CKEditor and it's there.

Note that if you want the in-dialog preview to work correctly, you need to apply #1589176-14: Follow-up: Use data-* to store #states api informations as well.

(For future note: this patch was rolled on top of 75debc110d4b8bbc2f5afdd60ddfd2c03bbd1c48.)

Patch concept

  1. Set data-picture-mapping attribute on the <img>, with the assistance of the image dialog in the text editor. It includes a toggleable preview that explains what can be expected to happen.
    (Nothing about this is CKEditor-specific; the only changes made to CKEditor-related code is to prevent the DrupalImageCaption plugin from ignoring and hence stripping attributes it doesn't particularly care about.)
  2. Have filter_picturemapping filter detect this attribute and, replace the <img tag with whatever HTML picture.module generates.
  3. Tada, responsive images!

(For those who don't want responsive images, the exact same stuff is implemented for "just" image styles: data-image-style + filter_imagestyle.)

Screenshots! Screencasts!

(Please ignore the extremely crappy video quality when resizing the browser window to show that the image is responsive. Either Chrome or QuickTime Player suck, or both.)

Picture
HQ demo of filter_picturemapping: https://dl.dropboxusercontent.com/u/80682862/spark/image%20dialog/pictur... (LQ demo attached for posterity, HQ demo is too big for d.o)
editor_image_dialog-picturemapping.png

editor_image_dialog-picturemapping-preview.png

editor_image_dialog-picturemapping-editor.png

Image style
HQ demo of filter_imagestyle: https://dl.dropboxusercontent.com/u/80682862/spark/image%20dialog/images... (LQ demo attached for posterity, HQ demo is too big for d.o)
editor_image_dialog-imagestyle.png

editor_image_dialog-imagestyle-preview.png

editor_image_dialog-imagestyle-editor.png


Things of note

  • "Picture mapping" is called "Image style" in the UI because to 99% of people, "Picture mapping" sounds like gibberish :) Choosing "an image style" is something that makes conceptual sense, choosing "a responsive image style" works too but is very long and choosing "a picture mapping" would cause a lot of eyes to glaze over :)
  • When using filter_imagestyle: a "live preview" in the text editor is trivial: just set the src, width and height attributes to the corresponding values for the selected image style.
  • When using filter_picturemapping: a "live preview" in the text editor is non-trivial: having an actual <picture> element in CKEditor is theoretically possible, but very, very complex. Not something we want to tackle here. So instead, we just want to show *one* (non-responsive) image, but then the question is: which breakpoint do we choose (which would then be the image style we'd use). For now, I've gone with picking the smallest breakpoint, because that'll work across devices. So, effectively, it works analogously to filter_imagestyle.
  • Some picture.module-related code will become cleaner once #2123251: Improve DX of responsive images; convert theme functions to new #type element is fixed :)
  • As said in #2120875: Remove breakpoint and picture module from core, bullet 3, Bartik's breakpoints are not being picked up by the breakpoint module, so the default picture mapping in the patch is currently against Toolbar's breakpoints, but that's trivial to change once that's been fixed :)
  • Many of the changes in this patch can be split off further yet into separate issues/patches, because many changes are essentially fixes to other code. This exercises quite a few APIs and has uncovered a lot of problems :)

Further thoughts

  • Configuring a single image style or picture mapping in the filter configuration — hence not allowing content authors to choose — could be a contrib module.
  • … much more for sure, but it's getting late and can't think of anything else right now :(

Status: Needs review » Needs work

The last submitted patch, responsive_inline_images-2061377-8.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
36.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,829 pass(es). View

Works fine here. Different PHP versions. Easy fix.

Testing the patch

No setup necessary. Everything enabled out of the box, including a default picture mapping. Just install on simplytest.me (use the link below), go to node/add/article, click the "image" button in CKEditor and it's there.

simplytest.me link that includes all dependencies

tim.plunkett’s picture

+++ 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)?

Wim Leers’s picture

#11: hah — I did not even know that function existed :D We can trust that PHP will always have the array pointer pointing to the first item? I've never dared to rely on the internal array pointer before.

Bojhan’s picture

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.)

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

I'm totally fine with labeling it "Responsive image style" instead of "Image style" in the image dialog UI. But as you already say: this might be outdated in a few years, and then just look ridiculous. That's why I called it just "Image style" in the image dialog UI. However, that's precisely what confused you.
So, AFAICT, "Responsive image style" is still the best choice.

If all discovered problems are just of labeling things, then I'd be very surprised/happy :P :D

I'll hold off rerolling until more things need to change than just UI strings. Unassigning myself too, because I'd love somebody else who's passionate about this to take the initial patch I provided and continue from there. I'll be working on performance issues for the majority of my time the next two weeks.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

eule’s picture

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

attiks’s picture

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>
eule’s picture

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

larowlan’s picture

+++ 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

Wim Leers’s picture

#18: You're right. That should be removed. Looking forward to the second half :)

Dave Reid’s picture

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" />

David4514’s picture

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];
Wim Leers’s picture

#20: 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.

#21: thanks! :)

Jelle_S’s picture

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

dasjo’s picture

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

yched’s picture

@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

yched’s picture

(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’s picture

#25: but then you're no longer rendering a field, you're rendering a field item. That complicates things.

yched’s picture

@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 :-)

Wim Leers’s picture

No, that's not what I meant. What I meant was that if you want to use field_view_value(), then you must pass it an $item, not a $field. Therefor, the user must select a field item, not a field. That complicates the UX.

This patch only deals with files, just like the existing "insert image" functionality in Text Editor/CKEditor does, which is totally fine, because each file is a file entity and we embed the file entity's UUID in the markup, so we can map it to a file entity. There's simply no need for a field using that approach.

yched’s picture

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".

Wim Leers’s picture

Issue summary: View changes
Issue tags: -sprint

Great to see that you agree :)

Removing "sprint" tag, because we're not actively working on this. The Spark team is focused on solving beta blockers.

blue_waters’s picture

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....

nod_’s picture

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

blue_waters’s picture

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

mdrummond’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

miraj9093’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
40.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2061377.36.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled the patch.

Status: Needs review » Needs work

The last submitted patch, 36: 2061377.36.patch, failed testing.

attiks’s picture

Issue tags: +Needs reroll
mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
mark.labrecque’s picture

Assigned: mark.labrecque » Unassigned
tonnosf’s picture

Status: Needs work » Needs review
FileSize
17.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,201 pass(es), 9 fail(s), and 0 exception(s). View

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.

tonnosf’s picture

FileSize
27.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,020 pass(es), 13 fail(s), and 0 exception(s). View

Added missing files to patch.

Status: Needs review » Needs work

The last submitted patch, 42: 2061377.42.patch, failed testing.

The last submitted patch, 41: 2061377.41.patch, failed testing.

eule’s picture

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

ja_ca’s picture

Assigned: Unassigned » ja_ca

Will be looking at this.

ja_ca’s picture

Assigned: ja_ca » Unassigned
Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue summary: View changes

#1589176: Follow-up: Use data-* to store #states api informations (a blocker) got committed — updated the IS.


Rerolling this now.

almaudoh’s picture

Still needs re-roll...

Dave Reid’s picture

Issue tags: +Media Initiative
Jelle_S’s picture

Assigned: Wim Leers » Jelle_S

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

erik.erskine’s picture

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

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
27.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,828 pass(es), 3 fail(s), and 0 exception(s). View

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.

erik.erskine’s picture

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 :)

Status: Needs review » Needs work

The last submitted patch, 53: 2061377.53.patch, failed testing.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
27.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,866 pass(es). View
1.05 KB

New patch should fix the failing tests.

almaudoh’s picture

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...

almaudoh’s picture

Status: Needs review » Needs work

CNW for #57

Jelle_S’s picture

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)

erik.erskine’s picture

FileSize
27.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
10.43 KB

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.

erik.erskine’s picture

+++ 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?

attiks’s picture

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.

mdrummond’s picture

Jelle_S’s picture

  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.

Jelle_S’s picture

Wim Leers’s picture

Status: Needs work » Needs review

Great progress here, thanks guys!

Status: Needs review » Needs work

The last submitted patch, 60: 2061377.60.patch, failed testing.

attiks’s picture

Assigned: Jelle_S » Unassigned

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

dimaro’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.41 KB
27.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,614 pass(es). View

Rerolled.

Hope it works!

mdrummond’s picture

Title: Allow image style/picture mapping to be selected in Text Editor's image dialog (necessary for structured content) » Allow image style/responsive image mapping to be selected in Text Editor's image dialog (necessary for structured content)

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

Dave Reid’s picture

Issue tags: +D8Media
garphy’s picture

Issue tags: +Needs reroll

Last patch won't apply anymore.

garphy’s picture

Issue tags: -Needs reroll
FileSize
27.13 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,288 pass(es). View

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

mdrummond’s picture

Status: Needs review » Needs work

#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.

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev

I talked to @webchick and @mdrummond about this.

Drupal 8 claims to do All The Right Responsive Things, but without this issue, no inline images are ever responsive. Which is kinda sad.

OTOH, #1855412: Enable responsive_image.module by default for standard install profile is also not done yet, so actually, out of the box, Drupal 8 doesn't do any responsive images things at all.

Given that RC1 is in exactly one week, I don't think it's realistic to get all this done by then. Especially because this would benefit from real-world feedback. I think it'd be better to put this patch in a D8 contrib module during 8.0.0, gather real-world feedback, and move it into core in 8.1.0. Then 8.1.0 can say "responsive images support has now matured, it is now enabled by default, and includes support for inline images also".

It's very unfortunate, but from a release manager POV, I think that's the most responsible thing to do.

Dave Reid’s picture

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.

Wim Leers’s picture

#76: thanks, that's an excellent additional point!

mdrummond’s picture

Status: Needs work » Needs review
FileSize
27.27 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch allow_image-2061377-73-reroll.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 78: allow_image-2061377-73-reroll.patch, failed testing.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
650 bytes

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

mdrummond’s picture

FileSize
26.64 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,254 pass(es), 6 fail(s), and 0 exception(s). View

Glitch due to testbot. Here's the patch.

Status: Needs review » Needs work

The last submitted patch, 81: 2061377-79-wysiwyg-image-styles.patch, failed testing.

Wim Leers’s picture

Version: 8.1.x-dev » 8.0.x-dev

8.1 is outdated; for the patch to apply, you need to specify 8.0.

mdrummond’s picture

Status: Needs work » Needs review

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

Jelle_S’s picture

Assigned: Unassigned » Jelle_S

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.

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
FileSize
23.9 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,249 pass(es). View

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.

Wim Leers’s picture

Awesome, thanks for the reroll, Jelle!

attiks’s picture

#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?

Jelle_S’s picture

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

attiks’s picture

#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.

Wim Leers’s picture

Yep, the corresponding filter could add them upon being enabled. I can take that part on once it's the last part remaining.

attiks’s picture

#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?

attiks’s picture

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

attiks’s picture

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

The last submitted patch, 81: 2061377-79-wysiwyg-image-styles.patch, failed testing.

attiks’s picture

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

Jelle_S’s picture

FileSize
40.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,623 pass(es). View
19.14 KB

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!

attiks’s picture

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

attiks’s picture

#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

Wim Leers’s picture

Status: Needs review » Needs work



I'm speechless. This is beautiful. This is so much more elegant. It makes so much more sense. There are still some rough edges, but I'm certain we can overcome those. This is already a massive leap forward compared to #3 :)

Awesome work, @attiks & @Jelle_S!

attiks++
Jelle_S++


#98:

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

:) :) :)

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


#99: Correct. We need to allow these additional attributes by default in the Standard install profile's text formats.


  1. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
    @@ -30,7 +30,17 @@
    +        widgetDefinition.requiredContent = new CKEDITOR.style({
    

    I didn't know about this! Is this new?

  2. +++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
    @@ -51,7 +51,10 @@
    +        var requiredContent = widgetDefinition.requiredContent.getDefinition();
    +        requiredContent.attributes['data-align'] = '';
    +        requiredContent.attributes['data-caption'] = '';
    +        widgetDefinition.requiredContent = new CKEDITOR.style(requiredContent);
    

    Nice!

  3. +++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
    @@ -13,8 +13,17 @@
    -        allowedContent: 'a[!href,target]',
    -        requiredContent: 'a[href]',
    +        allowedContent: {
    +          a: {
    +            attributes: '!href,target'
    +          }
    +        },
    +        requiredContent: new CKEDITOR.style({
    +          element: 'a',
    +          attributes: {
    +            href: ''
    +          }
    +        }),
    

    Why also update the link plugin? For consistency? Shouldn't we just do that in a separate issue?

  4. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,80 @@
    +/**
    + * @file
    + * Drupal Image Caption plugin.
    + *
    + * This alters the existing CKEditor image2 widget plugin, which is already
    + * altered by the Drupal Image plugin, to:
    + * - allow for the data-caption and data-align attributes to be set
    + * - mimic the upcasting behavior of the caption_filter filter.
    + *
    + * @ignore
    + */
    

    Docblock needs some love :)

  5. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,80 @@
    +      // data-responsive-image-style attributes.
    ...
    +          // Parse the data-responsive-image-style attribute.
    

    data-image-style

  6. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,80 @@
    +        // Override default features definitions for drupalimagecaption.
    +        CKEDITOR.tools.extend(widgetDefinition.features, {
    +          responsiveimage: {
    +            requiredContent: 'img[data-image-style]'
    +          }
    +        }, true);
    

    <3

  7. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,80 @@
    +        var requiredContent = widgetDefinition.requiredContent.getDefinition();
    +        requiredContent.attributes['data-image-style'] = '';
    +        widgetDefinition.requiredContent = new CKEDITOR.style(requiredContent);
    

    <3

  8. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,80 @@
    +        widgetDefinition.allowedContent.img.attributes += ',!data-image-style';
    

    Shouldn't this use code similar to that for requiredContent?

  9. +++ b/core/modules/image/src/Plugin/CKEditorPlugin/DrupalImageStyle.php
    @@ -0,0 +1,91 @@
    + * Defines the "drupalresponsiveimage" plugin.
    

    drupalimagestyle

  10. +++ b/core/modules/image/src/Plugin/CKEditorPlugin/DrupalImageStyle.php
    @@ -0,0 +1,91 @@
    +    return array();
    ...
    +    return array();
    ...
    +    return array();
    

    Let's use short array notation.

  11. +++ b/core/modules/image/src/Plugin/CKEditorPlugin/DrupalImageStyle.php
    @@ -0,0 +1,91 @@
    +    // text editor uses the filter_responsive_image_style filter and the
    

    filter_image_style

  12. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -41,9 +41,9 @@ public function process($text, $langcode) {
    -      foreach ($xpath->query('//*[@data-editor-file-uuid and @data-image-style]') as $node) {
    ...
    +      foreach ($xpath->query('//*[@data-entity-uuid and @data-image-style]') as $node) {
    
    +++ b/core/modules/responsive_image/src/Plugin/Filter/FilterResponsiveImageStyle.php
    @@ -37,9 +37,9 @@ public function process($text, $langcode) {
    -      foreach ($xpath->query('//*[@data-editor-file-uuid and @data-responsive-image-style]') as $node) {
    ...
    +      foreach ($xpath->query('//*[@data-entity-uuid and @data-responsive-image-style]') as $node) {
    

    Right, the original patch predates the data-editor-file-uuid -> data-entity-uuid conversion :)

    However, you'll also want to add the condition @data-entity-type="file".

    Just like _editor_parse_file_uuids() already does.

  13. +++ b/core/modules/responsive_image/js/plugins/drupalresponsiveimage/plugin.js
    @@ -0,0 +1,80 @@
    +/**
    + * @file
    + * Drupal Image Caption plugin.
    + *
    + * This alters the existing CKEditor image2 widget plugin, which is already
    + * altered by the Drupal Image plugin, to:
    + * - allow for the data-caption and data-align attributes to be set
    + * - mimic the upcasting behavior of the caption_filter filter.
    + *
    + * @ignore
    + */
    

    Also needs love :)

  14. +++ b/core/modules/responsive_image/js/plugins/drupalresponsiveimage/plugin.js
    @@ -0,0 +1,80 @@
    +        widgetDefinition.allowedContent.img.attributes += ',!data-responsive-image-style';
    

    Same remark as above.

  15. +++ b/core/modules/responsive_image/src/Plugin/CKEditorPlugin/DrupalResponsiveImage.php
    @@ -0,0 +1,91 @@
    + * Defines the "drupalresponsiveimage" plugin.
    ...
    + *   id = "drupalresponsiveimage",
    + *   label = @Translation("Drupal responsive image"),
    ...
    +class DrupalResponsiveImage extends PluginBase implements CKEditorPluginInterface, CKEditorPluginContextualInterface {
    

    Shouldn't this be "responsive image style", for consistency with "image style" above?

  16. +++ b/core/modules/responsive_image/src/Plugin/Filter/FilterResponsiveImageStyle.php
    @@ -88,11 +88,10 @@ public function process($text, $langcode) {
    -        $updated_node = Html::load($altered_html)->getElementsByTagName('body')
    +        $updated_node = Html::load(trim($altered_html))->getElementsByTagName('body')
    

    Interesting, why/how did you run into a problem that caused this change to be necessary?

    If it's unnecessary, let's revert this change.

    If it's necessary, this will need test coverage proving that it is necessary.

  17. +++ b/core/modules/responsive_image/src/Plugin/Filter/FilterResponsiveImageStyle.php
    @@ -88,11 +88,10 @@ public function process($text, $langcode) {
               ->item(0);
    -
             // Import the updated node from the new DOMDocument into the original
    

    Let's revert this unnecessary change.

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.

attiks’s picture

#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?

Wim Leers’s picture

#102.16: actually, no, the image caption/align filters also don't have this yet! Great point. This was an oversight in #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default. So I think it's fine to also not do that here for these filters, and do all of them at once in a single patch (align + caption + image style + responsive image style, for automatically whitelisting data-align, data-caption, data-image-style and data-responsive-image-style, respectively).

attiks’s picture

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

attiks’s picture

Assigned: Unassigned » jensimmons
Issue tags: +rc deadline

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

Jelle_S’s picture

Assigned: jensimmons » Jelle_S
attiks’s picture

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
43.69 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,583 pass(es). View
3.65 KB

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.

Jelle_S’s picture

FileSize
43.59 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,644 pass(es). View
16.74 KB

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.

Jelle_S’s picture

FileSize
44.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,634 pass(es). View
1.71 KB
  • 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.
Jelle_S’s picture

Assigned: Jelle_S » Unassigned

Unassigning for reviewers

attiks’s picture

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

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: -rc deadline

Given #1855412-43: Enable responsive_image.module by default for standard install profile, I think it's clear we can't get this in before RC either.

Let's make Responsive images in Drupal 8.1 blow people's minds. :) No need to rush this in now; we'll be able to do better work then.

This work is not wasted; this brings us to a great place to land this early in the 8.1 cycle :)


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

I think the preview is helpful to make the content creator understand what is going to happen: It includes a toggleable preview that explains what can be expected to happen.

Bojhan’s picture

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.

mdrummond’s picture

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.

attiks’s picture

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

Wim Leers’s picture

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

+1, but separate issue. Since it's a contrib blocker, I'm fairly confident this can also happen after RC1.

attiks’s picture

mdrummond’s picture

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

attiks’s picture

#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.

mdrummond’s picture

Version: 8.1.x-dev » 8.0.x-dev

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.

mdrummond’s picture

Status: Needs review » Needs work

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.

attiks’s picture

#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

mdrummond’s picture

Status: Needs work » Needs review
FileSize
44.27 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2061377-124-wysiwyg-image-styles.patch. Unable to apply patch. See the log in the details link for more information. View
765 bytes

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.

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev

I think this is a super important issue, and I think you all did fantastic work pushing this forward.

But, it's just too much, too rushed in. Test coverage is completely absent. And in my testing, even fairly basic things are broken: when you edit the image style of an existing image, the dialog always shows "Large", because the client fails to send the data-image-style attribute to the server.

I think this will be a splendid addition for 8.1, but to rush it in now and have it be a buggy, frustrating feature would be worse than not having it in 8.0. So let's do this in 8.1 :)


  1. +++ b/core/modules/image/image.module
    @@ -473,3 +475,105 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +function image_form_editor_image_dialog_alter(&$form, FormStateInterface $form_state) {
    ...
    +function image_form_editor_image_dialog_validate(array &$form, FormStateInterface &$form_state) {
    

    This needs test coverage.

  2. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,119 @@
    +class FilterImageStyle extends FilterBase {
    

    Needs test coverage.

  3. +++ b/core/modules/responsive_image/responsive_image.install
    @@ -0,0 +1,27 @@
    +function responsive_image_update_8001() {
    

    I don't think it's good to assume every site will want to use this. Some sites forbid images in the WYSIWYG editor altogether — it depends on how you model your data.

    Plus, this would need test coverage.

  4. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -512,3 +514,171 @@ function responsive_image_library_info_alter(array &$libraries, $module) {
    +function responsive_image_form_editor_image_dialog_alter(&$form, FormStateInterface $form_state) {
    ...
    +function responsive_image_form_editor_image_dialog_validate(array &$form, FormStateInterface $form_state) {
    

    Needs test coverage.

  5. +++ b/core/modules/responsive_image/src/Plugin/Filter/FilterResponsiveImageStyle.php
    @@ -0,0 +1,123 @@
    +class FilterResponsiveImageStyle extends FilterBase {
    

    Needs test coverage.

  6. +++ b/core/modules/system/css/components/container-inline.module.css
    @@ -11,3 +11,7 @@
    +/* Allow items inside inline items to render themselves as blocks. */
    +.container-inline .container-block {
    +  display: block;
    +}
    

    Why this change?

  7. +++ b/core/modules/system/system.install
    @@ -1808,5 +1808,27 @@ function system_update_8011() {
    +function system_update_8012() {
    

    This would belong in image module. And again, making too many assumptions. And again, missing test coverage.

attiks’s picture

Status: Needs review » Postponed

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

Status: Postponed » Needs work

The last submitted patch, 124: 2061377-124-wysiwyg-image-styles.patch, failed testing.

Wim Leers’s picture

Issue tags: +minor version target

Sadly, WordPress will ship with this before us: https://make.wordpress.org/core/2015/09/26/responsive-images-feature-plu... + https://make.wordpress.org/core/2015/11/10/responsive-images-in-wordpres....

We should definitely ship this with Drupal 8.1.

tanc’s picture

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.

Wim Leers’s picture

Agreed.

The 8.1 branch opens in the beginning of January 2016 IIRC. Hopefully @mdrummond, @attiks and @Jelle_S will continue their awesome work then :) I'll be providing reviews for sure!

mdrummond’s picture

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

mdrummond’s picture

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.

mitchalbert’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jon@s’s picture

@mitchalbert were you able to pick this back up?

mdrummond’s picture

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.

mitchalbert’s picture

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

hopefully done today

mitchalbert’s picture

Rerolled the patch against 8.2.x-dev

some minor fixes were needed to get things working

almaudoh’s picture

Status: Needs work » Needs review

For testbot...

Status: Needs review » Needs work

The last submitted patch, 139: wysiwyg-image-styles-2061377-139.patch, failed testing.

mitchalbert’s picture

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

cbeier’s picture

Status: Needs work » Needs review

And for the testbot again …

mitchalbert’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 144: wysiwyg-image-styles-2061377-144.patch, failed testing.

Jon@s’s picture

@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.

mitchalbert’s picture

@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 :)

prics’s picture

@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

fkelly’s picture

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.

mitchalbert’s picture

@prics

What version of drupal did you run the patch against?

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

prics’s picture

@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

mdrummond’s picture

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.

mdrummond’s picture

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?

mdrummond’s picture

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.

mdrummond’s picture

Missed adding in the files.

mdrummond’s picture

Good news! I did some more manual testing, and this is indeed working both for image styles and for responsive image styles.

I needed to move the the image style filter towards the end of the list for this to work with the basic text editor, but for all I know a clear cache would have been necessary too. It's annoying this isn't turned on by default, though. Feels like I shouldn't have to know to turn on this filter for this to work. Not sure how possible it would be to change this though. I know this was discussed above in regards to responsive image styles, and it's not possible to have that filter automatically enabled because responsive images is an optional module. But the image styles filter seems like it should be on by default at the very least.

This still needs tests.

mdrummond’s picture

To get ready for putting in tests, I wanted to make a list of things this patch is doing, which should help us come with a testing strategy:

EditorImageDialog

Check if the form state has a src attribute, if not add the src attribute with the value of the file URI.

image.admin.css

Tweak the margins of the preview image, and set it to display: block.

image.module

Here is a big one! New function: image_form_editor_image_dialog_alter

There is a lot going on in here, but the short version is this adds the ability to select an image style within the wysiwyg image selector dialog. Here is how this goes down:

  • Get the filter format from the form state, in getBuildInfo['args'][0].
  • Get the list of filters from that filter format.
  • Get the image element from the form state.
  • If the filter image style is enabled, then do all these other things:
    • Hide the width and height dimension selector.
    • Get the list of image style options and their keys.
    • Create an image style form item, wrapped with a container div.
    • Add an image style selector that has a container-inline class, and add container-inline to the wrapper as well. Also add a data-image-style to the parents.
    • Add a preview toggle checkbox.
    • Add all the image style previews: visibility is toggled on and off depending on the image style selected. The image style preview theme function is drupal_render pre-rendered and then that is placed in #markup.
    • The image/admin library is attached to this form.
    • Attach the form validation on save modal.

So, validation!

  • Get the attributes out of the form state.
  • If there isn't an empty value for ['fid'][0] in the form state:
    • Load the image style based on the data-image-style attribute.
    • Load the file based from ['fid'][0].
    • Set the src attribute to a URI generated from the image style.
    • Get an image attribute from the image factory. As long as that is valid, get the width and height of the image and set that on the attributes.
    • In between getting the width and height and setting that on the attributes, run transformDimeonsions on the image style. I am not sure what effect that transformation actually has.

Phew!

drupalimagestyle/plugin.js

This adds a drupalimagestyle plugin. Not sure it makes sense to walk through every single thing this does. That would get very long.

CKEditorPlugin/DrupalImageStyle.php

This takes care of connecting the drupalimagestyle plugin for CKEditor with Drupal. I think.

FilterImageStyle

This takes care of filtering text, looking for data-image-style and then transforming that into an img element that uses that image style.

image-style-preview.html.twig

Add a line break after the original image preview and the text that describes it: do the same for the image style preview. Feels weird to hardcode a br element in here.

drupalresponsiveimagestyle/plugin.js

This adds a CKEdtior plugin for responsive image styles.

responsive_image.install

Add data-responsive-image-style as an allowed attribute.

responsive_image.module

Alter the editor image dialog form to add a selector for responsive image styles. If the image styles filter is turned on too, disable that in addition to the width/height selectors. In the validation, build a preview image from the smallest image.

CKEditorPlugin/DrupalResponsiveImageStyle.php

This takes care of connecting the drupalresponsiveimagestyle plugin for CKEditor with Drupal.

FilterResponsiveImageStyle

This takes care of filtering text, looking for data-responsive-image-style and then transforming that into an img element that uses that image style.

container-inline.module.css

Allow container blocks inside of container inline to have display: block.

system.install

Add data-image-style as an allowed attribute.

Config for filter.format.basic_html.yml

Add data-image-style as a default attribute. Also add the image style filter config.

Config for filter.format.full_html.yml

Ditto.

CSS for Classy container-inline.css

This actually probably can't be changed anymore as this is frozen. But this does some things with inline container checkboxes, making some other styles for labels also apply to checkboxes too.

That is everything going on in this patch for better or worse. A lot of things to be tested.

mdrummond’s picture

Questions:

  • Why are drupalimagestyle/plugin.js and DrupalImageStyle.php in the image module? DrupalImageCaption, which is somewhat similar, has these files in CKEditor module.
  • Similarly, FilterImageStyle is in the image module, while FilterCaption is in the Filter module.
  • I can sort of see why the equivalent files for responsive images are in the Responsive Image module, because that isn't installed by default. But our goal is once this in, to make Responsive Image module a default one. I don't know, I suppose it could still be disabled.
  • If we are adding config files for image styles, we should probably do the same for responsive image styles, right?
mdrummond’s picture

I am making an executive decision that the items in image module that should probably not be in image module should be moved to the locations where other similar things are located. I am also getting rid of file statements no longer needed in these files, as well as some use statements that are not used anymore.

mdrummond’s picture

FileSize
1.95 KB

That interdiff was wildly inaccurate. Here is one that is not.

Wim Leers’s picture

#152: I can confirm that those changes landed in other issues, so are no longer necessary. :) Although you'll still need to modify those JS files to allow the new data- attributes that this issue adds.


#158:

Why are drupalimagestyle/plugin.js and DrupalImageStyle.php in the image module? DrupalImageCaption, which is somewhat similar, has these files in CKEditor module.

Similarly, FilterImageStyle is in the image module, while FilterCaption is in the Filter module.

Because they use functionality provided by image module: (responsive) image styles. OTOH, DrupalImageCaption doesn't do anything image style-related, and can therefore live in the CKEditor module.

I can sort of see why the equivalent files for responsive images are in the Responsive Image module, because that isn't installed by default. But our goal is once this in, to make Responsive Image module a default one. I don't know, I suppose it could still be disabled

Yes, it can still be disabled. But more importantly: functionality that needs a certain module to be installed should live in that module. We do this always, not just in this case. Only old/crappy code doesn't do this.

If we are adding config files for image styles, we should probably do the same for responsive image styles, right?

Well, I think there was at one point of this patch's life, but it got removed at another point, because we tried to still get it ready for Drupal 8.0, and since Responsive Images aren't enabled by default, shipping such config also wouldn't make sense. But… we totally could include such config in the Responsive Image module so that you get it as soon as you install that module, which would make sense IMO.


#159: Feel free to talk to core committers about this, but I'm 99% certain this does not belong where you just moved it :) Easy to change again of course.

mdrummond’s picture

FYI, I will move the files back to the image module. I am currently working on a patch with dawehner that converts the filter plugin to unit testable code, and then adds a unit test for it. Will post once ready as a model for other tests we will need.

mdrummond’s picture

This moves the code back to whence it came. Bad Marc.

This also converts FilterImageStyle to unit-testable code.

I'm feeling stuck, so also posting my in-progress code so it doesn't get lost. I have my old version of the test in this patch purely to save the work I did on mocking up expected output. The unit test is in progress and not working yet. There are a bunch of services that need to be mocked up, and I'm feeling ind of overwhelmed by that. Hoping to find some help to move this forward.

mdrummond’s picture

FileSize
17.3 KB

Wrong interdiff. This is the right one.

mdrummond’s picture

Moving this forward a little more with the help of dsnopek. Moving some of the FilterImageStyle->process logic into helper functions to make this easier to test (and to make it more clear what is going on in process itself).

Wim Leers’s picture

Moving some of the FilterImageStyle->process logic into helper functions to make this easier to test (and to make it more clear what is going on in process itself).

+1!

I'm feeling stuck […] There are a bunch of services that need to be mocked up, and I'm feeling ind of overwhelmed by that.

Are you feeling stuck on other things, or only the mocking? Mocking can totally feel very overwhelming. Happy to help.

mdrummond’s picture

I had gotten up at 5:30 a.m. to write this patch, and after an hour of work, I swore I posted a patch with my progress, but apparently something went wrong and the comment didn't post.

Anyhow, I had finished refactoring the process method to break it down into more easily tested functions. One of the tests had a bug, but progress.

mdrummond’s picture

Made some progress with fixing the unit test, but still getting a weird error, could not figure out why with some help in sprint room. Could not get xdebug working so hard to track this down.

Bojhan’s picture

This is pretty relevant for the media work.

mdrummond’s picture

Thanks to the help of alexpott and tstoeckler, we now have a working unit test for FilterImageStyle!

Will be refactoring FilterResponsiveImageStyle to create a similar test next.

mdrummond’s picture

This refactors FilterResponsiveImageStyle along the same lines as FilterImageStyle to make this more unit-testable. Also fixes a few minor things in FilterImageStyle.

mdrummond’s picture

Added a unit test for FilterResponsiveImageStyle, adapted from FilterImageStyleTest. Works!

mdrummond’s picture

This adds a test for image_form_editor_image_dialog_alter() to check that if we are using the imagestyle filter, that you cannot select the image width and height on the editor dialog.

I'll need to do the same for responsive images.

Not sure what else we want to test with the editor image dialog alteration.

We will still need to test and double-check how we add the data-image-style attribute as being valid, and the same for data-responsive-image-style.

Thanks so much for your help tstoeckler and dawehner!

mdrummond’s picture

One more patch to add a test for the responsive image dialog form alteration.

Wim Leers’s picture

This is looking great already! Thanks so much for pushing this forward! Here is a first comprehensive review :)


Did manual testing.

Image style
  1. Upon enabling the filter, the corresponding attribute is not whitelisted automatically. At least not immediately: upon reopening the text format/editor page, it is added, but that means you have to save it a second time. But this is not the fault of this patch, it's a pre-existing bug.
  2. The preview looks broken: https://www.drupal.org/files/issues/Screen%20Shot%202016-05-20%20at%2014...
  3. The stored HTML is correct, it includes the data-image-style attribute:
    <img alt="asdfasdfsadf" data-entity-type="file" data-entity-uuid="db5f4e97-e88e-41be-803d-8f6aac000a0c" data-image-style="medium" src="http://tres/sites/default/files/styles/medium/public/inline-images/search_2.jpg?itok=2z1jqnXA" />
    
  4. However, if you either choose to align the image to the center, or choose to specify a caption, then upon editing the image, the dialog seems to have "forgotten" the chosen image style. This is because the JS doesn't send this attribute to the server. In the POST request, these are the attributes it does send:
    editor_object[src]:http://tres/sites/default/files/styles/medium/public/inline-images/search_2.jpg?itok=2z1jqnXA
    editor_object[alt]:asdfasdfsadf
    editor_object[width]:
    editor_object[height]:
    editor_object[data-entity-type]:file
    editor_object[data-entity-uuid]:db5f4e97-e88e-41be-803d-8f6aac000a0c
    editor_object[data-align]:center
    editor_object[hasCaption]:true
    

    See the detailed analysis below, including a solution :)

Responsive image style
  1. Same installation problem as above.
  2. The preview looks broken too, but for a very different reason: the image won't work: https://www.drupal.org/files/issues/Screen%20Shot%202016-05-20%20at%2015...
  3. The stored HTML is correct again:
    <p><img alt="asdfdsf" data-align="right" data-entity-type="file" data-entity-uuid="86b18ecf-745a-4f69-ac52-9948a2521296" data-responsive-image-style="wide" src="/sites/default/files/inline-images/search_3.jpg" /></p>
    
  4. And again when centering or captioning an image, things break, with the same root cause :)

  1. wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
    $ eslint core/modules
    
    /Users/wim.leers/Work/drupal-tres/core/modules/image/js/plugins/drupalimagestyle/plugin.js
      14:3   error  Strings must use singlequote        quotes
      45:19  error  'findElementByName' is not defined  no-undef
    
    /Users/wim.leers/Work/drupal-tres/core/modules/responsive_image/js/plugins/drupalresponsiveimagestyle/plugin.js
      14:3   error  Strings must use singlequote        quotes
      45:19  error  'findElementByName' is not defined  no-undef
      73:71  error  Unexpected trailing comma           comma-dangle
    
  2. +++ b/core/modules/image/image.module
    @@ -243,6 +250,10 @@ function image_style_options($include_empty = TRUE) {
    +  /**
    +   * @var string $name
    +   * @var \Drupal\image\ImageStyleInterface $style
    +   */
    
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    /**
    +     * The image style.
    +     *
    +     * @var \Drupal\image\ImageStyleInterface $image_style
    +     */
    ...
    +    /**
    +     * The image file.
    +     *
    +     * @var \Drupal\file\FileInterface $file
    +     */
    ...
    +    /**
    +     * The image object.
    +     *
    +     * @var \Drupal\Core\Image\ImageInterface $image
    +     */
    

    These typehints that we add for IDEs are fine. But always use

    /** @var \F\Q\C\N $foo */
    

    , never use

    /**
     * Some description
     * 
     * @var \F\Q\C\N $foo
     */
    
  3. +++ b/core/modules/image/image.module
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +function image_form_editor_image_dialog_alter(&$form, FormStateInterface $form_state) {
    +
    +  /** @var \Drupal\filter\Entity\FilterFormat $filter_format */
    

    Unnecessary newline.

  4. +++ b/core/modules/image/image.module
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +  if ((isset($filters['filter_imagestyle']) && $filters['filter_imagestyle']->status) &&
    +      (!isset($filters['filter_responsive_image_style']) ||
    +        isset($filters['filter_responsive_image_style']) && !$filters['filter_responsive_image_style']->status)) {
    

    This has strange indentation and, more importantly, is exceptionally difficult to read. Let's simplify this by storing some of these in variables, and then using the variables in this if-test.

  5. +++ b/core/modules/image/image.module
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    // Get the image from the form.
    ...
    +    // Disallow the user from specifying image dimensions manualy.
    ...
    +    // Get an array of image styles to present as options for selection.
    ...
    +    // Create a form item for image style selection.
    ...
    +    // Add a select element to choose an image style.
    ...
    +    // Add a checkbox to toggle preview of the image style.
    ...
    +    // Create a set of image style previews that display if toggled on.
    ...
    +    // Attach the image admin library.
    ...
    +    // Validate that the image shown in the text editor matches the image style.
    

    This is literally adding an inline comment for every single bit of code. That's fine in custom/contrib, but not in core.

    We only add inline comments for tricky things.

  6. +++ b/core/modules/image/image.module
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    $image_styles = ImageStyle::loadMultiple();
    +    $image_style_options = image_style_options(FALSE);
    

    This suggests the first line is necessary for the second, but it is not.

  7. +++ b/core/modules/image/image.module
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    $image_style_keys = array_keys($image_style_options);
    ...
    +      '#default_value' => isset($image_element['data-image-style']) ? $image_element['data-image-style'] : $image_style_keys[0],
    

    This variable is kinda pointless, it'd be easier to read if this would just do array_keys($image_style_options)[0]. Because it's only used once.

  8. +++ b/core/modules/image/image.module
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    $form['image_style'] = array(
    ...
    +      '#attributes' => array('class' => array('container-inline')),
    +      '#parents' => array('attributes', 'data-image-style'),
    

    s/array()/[]/, here and elsewhere.

  9. +++ b/core/modules/image/image.module
    @@ -483,3 +494,147 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    /** @var \Drupal\image\ImageStyleInterface $image_style */
    

    This typehint is correct!

  10. +++ b/core/modules/image/image.module
    --- /dev/null
    +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    
    +++ b/core/modules/image/src/Plugin/CKEditorPlugin/DrupalImageStyle.php
    @@ -0,0 +1,86 @@
    + * Defines the "drupalimagestyle" plugin.
    ...
    + *   label = @Translation("Drupal image style"),
    ...
    +class DrupalImageStyle extends PluginBase implements CKEditorPluginInterface, CKEditorPluginContextualInterface {
    
    --- /dev/null
    +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    
    +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,255 @@
    + *   id = "filter_imagestyle",
    
    +++ b/core/modules/image/tests/src/Unit/FilterImageStyleTest.php
    --- /dev/null
    +++ b/core/modules/responsive_image/js/plugins/drupalresponsiveimagestyle/plugin.js
    
    +++ b/core/modules/responsive_image/src/Plugin/Filter/FilterResponsiveImageStyle.php
    @@ -0,0 +1,265 @@
    + *   id = "filter_responsive_image_style",
    ...
    + *   title = @Translation("Display responsive images"),
    ...
    +class FilterResponsiveImageStyle extends FilterBase implements ContainerFactoryPluginInterface {
    

    drupalimage/plugin.js, DrupalImage, "Image"

    drupalimagestyle/plugins.js, DrupalImageStyle, "Drupal image style",FilterImageStyle, filter_imagestyle, "Display image styles"

    vs

    drupalresponsiveimagestyle/plugin.js, DrupalResponsiveImageStyle, "Drupal responsive image style", FilterResponsiveImageStyle, filter_responsive_image_style,
    "Display responsive images"

    This is inconsistent in multiple ways.

    Let's change the CKEditor plugin labels from "Drupal image style" to "Image style" and from "Drupal responsive image style" to "Responsive image style".

    Let's change the filter plugin title from "Display responsive images" to "Display responsive image styles".

    Let's change the plugin IDs from filter_imagestyle to filter_image_style. Or, better, change them to image_style and responsive_image_style.

  11. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,79 @@
    +          element = originalUpcast.call(this, element, data);
    +
    +          // Parse the data-image-style attribute.
    +          data['data-image-style'] = element.attributes['data-image-style'];
    

    These two statements need to be flipped. Because originalUpcast will return an upcasted element, which means <img src="…" data-caption="foo"> will be transformed to <figure><img src="…"><figcaption>foo</figcaption></figure>.

    That means element.attributes will be checking the attributes on the <figure> tag in case of a captioned image.

    In other words: in case of a captioned image, this will be looking at the wrong element's attributes, and hence fail.

    Similarly, if an image has data-align="center", it will be wrapped in a <p> tag, which causes similar problems.

  12. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,255 @@
    +   * Constructs a Drupal\Component\Plugin\PluginBase object.
    

    Nit: outdated comment.

  13. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,255 @@
    +      // Process each img element DOM element found with the necessary
    +      // attributes.
    ...
    +        // Get the UUID and image style for the file.
    ...
    +        // If the image style is not a valid one, then don't transform the HTML.
    ...
    +        // Transform the HTML for the img element by applying an image style.
    ...
    +        // Finally, replace the original image node with the new image node.
    ...
    +      // Process the filter with the newly updated DOM.
    ...
    +    // Process the filter if no image style img elements are found.
    

    This code is also adding an inline comment for every single thing. Please only keep the essential comments.

  14. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,255 @@
    +  /**
    +   * Loads the image styles.
    +   *
    +   * @return string[]
    +   */
    +  protected function loadImageStyles() {
    +    return array_keys($this->entityTypeManager->getStorage('image_style')->loadMultiple());
    +  }
    

    We load the image styles, but then only look at their IDs. This function needs a better name.

  15. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,255 @@
    +   * Removes attributes that will be generated from image style theme function.
    ...
    +    $dom_element->removeAttribute('data-entity-type');
    +    $dom_element->removeAttribute('data-entity-uuid');
    +    $dom_element->removeAttribute('data-image-style');
    

    These won't be generated.

  16. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,255 @@
    +    // Make sure all non-regenerated attributes are retained.
    

    "retained" vs "removes" -> that doesn't make sense.

  17. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,255 @@
    +    $image_uri = $image_info['uri'];
    +    $image_width = $image_info['width'];
    +    $image_height = $image_info['height'];
    ...
    +      '#uri' => $image_uri,
    +      '#width' => $image_width,
    +      '#height' => $image_height,
    

    Why assign these to temporary variables if we only use them in one place?

  18. +++ b/core/modules/image/templates/image-style-preview.html.twig
    @@ -33,7 +33,7 @@
    -      {{ 'original'|t }} (<a href="{{ original.url }}">{{ 'view actual size'|t }}</a>)
    +      {{ 'original'|t }} <br >(<a href="{{ original.url }}">{{ 'view actual size'|t }}</a>)
    
    @@ -45,7 +45,7 @@
    -    {{ style_name }} (<a href="{{ derivative.url }}?{{ cache_bypass }}">{{ 'view actual size'|t }}</a>)
    +    {{ style_name }} <br >(<a href="{{ derivative.url }}?{{ cache_bypass }}">{{ 'view actual size'|t }}</a>)
    

    Why are these changes necessary?

  19. +++ b/core/modules/image/tests/src/Kernel/EditorImageStyleDialogTest.php
    @@ -0,0 +1,141 @@
    +    $this->installSchema('system', ['key_value_expire']);
    +    $this->installSchema('node', array('node_access'));
    +    $this->installSchema('file', array('file_usage'));
    

    Inconsistent array syntax. Let's use the newer syntax everywhere.

  20. +++ b/core/modules/image/tests/src/Kernel/EditorImageStyleDialogTest.php
    @@ -0,0 +1,141 @@
    +    // If image style is selected, image dimensions should not be available.
    +    $this->assertFalse($form['dimensions']['#access']);
    

    This is the only actual assertion.

    I think we can assert several more things:

    1. assert the presence of a <select> allowing an image style to be selected, and the options it provides
    2. assert that neither this existing assertion nor the one I suggested above appear for a text format that doesn't have this filter enabled
  21. +++ b/core/modules/image/tests/src/Unit/FilterImageStyleTest.php
    @@ -0,0 +1,110 @@
    +class FilterImageStyleTest extends UnitTestCase {
    

    Great test!

  22. +++ b/core/modules/image/tests/src/Unit/FilterImageStyleTest.php
    @@ -0,0 +1,110 @@
    +  protected $entityTypeManager;
    +  protected $entityRepository;
    +  protected $imageFactory;
    +  protected $renderer;
    ...
    +    $this->entityTypeManager = $this->prophesize(EntityTypeManager::class);
    +    $this->entityRepository = $this->prophesize(EntityRepository::class);
    +    $this->imageFactory = $this->prophesize(ImageFactory::class);
    +    $this->renderer = $this->prophesize(Renderer::class);
    

    Nit: missing docs… or just don't do $this->foo, and just do $foo. If you don't need to access them elsewhere in the test, there's no need to make them properties on this class! :)

  23. +++ b/core/modules/image/tests/src/Unit/FilterImageStyleTest.php
    @@ -0,0 +1,110 @@
    +    $generated_src = 'styles/medium/public/inline-images/image.png';
    +    $generated_width = '200';
    +    $generated_height = '150';
    +
    +    $generated_img = '<img src="' . $generated_src .'" width="' . $generated_width .'" height="' . $generated_height .'" alt="' . $original_alt .'" />';
    +    $generated_text = '<p>' . $generated_img . '</p>';
    

    s/generated/expected/

  24. +++ b/core/modules/responsive_image/responsive_image.install
    @@ -0,0 +1,27 @@
    +/**
    + * Add allowed attributes to existing html filters.
    + */
    +function responsive_image_update_8001() {
    +  $config_factory = \Drupal::configFactory();
    +  foreach ($config_factory->listAll('filter.format') as $name) {
    +    $config = $config_factory->getEditable($name);
    +    if (!$config->get('filters.filter_responsive_image_style.status')) {
    +      continue;
    +    }
    +    $allowed_html = $config->get('filters.filter_html.settings.allowed_html');
    +    $matches = [];
    +    if (!empty($allowed_html) && preg_match('/<img([^>]*)>/', $allowed_html, $matches)) {
    +      $new_attributes = array_filter(explode(' ', $matches[1]));
    +      $new_attributes[] = 'data-responsive-image-style';
    +      $allowed_html = preg_replace('/<img([^>]*)>/', '<img ' . implode(' ', array_unique($new_attributes)) . '>', $allowed_html);
    +      $config->set('filters.filter_html.settings.allowed_html', $allowed_html);
    +      $config->save();
    +    }
    +  }
    +}
    

    Interesting! :) I would personally love this. But I'm not sure whether it'll be acceptable.

    We'd also need a valid rationale to do this for responsive_image but not for image/standard.

  25. +++ b/core/modules/responsive_image/src/Plugin/CKEditorPlugin/DrupalResponsiveImageStyle.php
    @@ -0,0 +1,91 @@
    +/**
    + * @file
    + * Contains \Drupal\responsive_image\Plugin\CKEditorPlugin\DrupalResponsiveImageStyle.
    + */
    

    These @file headers can be removed in all files. We don't need them anymore since 8.2.x. (YAY!)

  26. +++ b/core/modules/responsive_image/src/Plugin/Filter/FilterResponsiveImageStyle.php
    @@ -0,0 +1,265 @@
    +  protected function getImageInfo($file_uuid) {
    ...
    +  protected function prepareResponsiveImageAttributes(\DOMElement $dom_element) {
    

    This is 90% duplicating what already exists in the corresponding code in image.module. Let's put that in a treat in image.module, then this can reuse that, because responsive_image depends on image.

  27. +++ b/core/modules/responsive_image/src/Plugin/Filter/FilterResponsiveImageStyle.php
    @@ -0,0 +1,265 @@
    +    // Responsive image styles should override image styles.
    +    if ($dom_element->hasAttribute('data-image-style')) {
    +      $dom_element->removeAttribute('data-image-style');
    +    }
    

    These should be data-responsive-image-style.

  28. +++ b/core/modules/responsive_image/tests/src/Kernel/EditorResponsiveImageStyleDialogTest.php
    @@ -0,0 +1,151 @@
    +    // If responsive image style is selected, image dimensions should not be
    +    // available.
    +    $this->assertFalse($form['dimensions']['#access']);
    +
    +    // If responsive image style is selected, image style should not be
    +    // available.
    +    $attributes = $form_state->getValue('attributes');
    +    $this->assertFalse(isset($form['image_style']));
    +    $this->assertFalse(isset($attributes['data-image-style']));
    

    There's some more assertions here, but still not a whole lot.

  29. +++ b/core/modules/responsive_image/tests/src/Unit/FilterResponsiveImageStyleTest.php
    @@ -0,0 +1,110 @@
    +class FilterResponsiveImageStyleTest extends UnitTestCase {
    

    Again same remarks here.

    And this too could probably benefit from a trait to share some of the test code.

  30. +++ b/core/modules/system/system.install
    @@ -1650,5 +1650,27 @@ function system_update_8014() {
    +function system_update_8015() {
    +  $config_factory = \Drupal::configFactory();
    +  foreach ($config_factory->listAll('filter.format') as $name) {
    +    $config = $config_factory->getEditable($name);
    +    if (!$config->get('filters.filter_imagestyle.status')) {
    +      continue;
    +    }
    +    $allowed_html = $config->get('filters.filter_html.settings.allowed_html');
    +    $matches = [];
    +    if (!empty($allowed_html) && preg_match('/<img([^>]*)>/', $allowed_html, $matches)) {
    +      $new_attributes = array_filter(explode(' ', $matches[1]));
    +      $new_attributes[] = 'data-image-style';
    +      $allowed_html = preg_replace('/<img([^>]*)>/', '<img ' . implode(' ', array_unique($new_attributes)) . '>', $allowed_html);
    +      $config->set('filters.filter_html.settings.allowed_html', $allowed_html);
    +      $config->save();
    +    }
    +  }
    +}
    +
    

    Oh, so you did do this for image too, but you put it in the system module. That's definitely wrong.

  31. +++ b/core/modules/system/system.install
    index 92224c2..8281a53 100644
    --- a/core/profiles/standard/config/install/filter.format.basic_html.yml
    
    +++ b/core/profiles/standard/config/install/filter.format.basic_html.yml
    index e5febb2..115891d 100644
    --- a/core/profiles/standard/config/install/filter.format.full_html.yml
    

    OMG YAY!

sachbearbeiter’s picture

It's so calm here since two months - is the progress going on in other issues?
Will the functionality be released in 8.2?
Is some kind of testing needed (i can't support this because of poor programming skills)?
For my clients this is a major important thing ...

Thanks
SB

blue_waters’s picture

Hi All - have been lurking/watching this issue since it started. Have also tried once or twice to set aside the time to test, or event take a stab at making some of the changes/fixups recommend by @Wim Leers, after the great work from @mdrummond.

With the Drupal 8.2.0-Beta1 announcement - I guess I have a couple of questions.

Firstly, at a glance, this feature https://www.drupal.org/node/2421427 - also headed for core, seems to have picked up a little more momentum, and yet appears to offer significantly less functionality. To me the ability to be able to apply image styles/responsive images styles, to inline images is a killer feature. It means that several editorial scenarios can be catered for out of the box.

Has this issues stalled because of lack of development/test resource? Or because it's no longer relevant? Entity embed is awesome, but the approach here still seems like the ideal solution. Or am I missing something? Can this feature AND https://www.drupal.org/node/2421427 coexist in core?

I have a ton of respect for all the hours contributed here, and elsewhere in Drupal - and so always feel a little guilty when all I can offer timewise, is a comment, or the offer to test, but this is one feature/issue that I've been hoping would land for a while now, and so if nothing else would be interested to know what the current thinking is here.

fkelly’s picture

Like Blue_Waters I have been lurking (only since Drupal 8.0 was released though) in this three year old thread. And seeing the 8.2.0 beta announcement I too was concerned that the features highlighted in this thread will not make it into 8.2 ... which seems to mean that the great unwashed among us won't see them until 2017 or later.

Right now I make do by using:
https://www.drupal.org/project/inline_responsive_images

and also be adding the IMCE module besides ckeditor. The combination of these two let's me apply responsive styles to images and also BOTH upload new images and edit existing ones AT THE SAME TIME. (Also IMCE let's me reuse images I've uploaded by other means (FTP) in my content types ... a feature that is essential to me).

I would also echo Blue_Waters' "ton of respect" and guilt at not being able to do more, but just ask if we can get a quick status on what the current thinking is and what we can expect to see in 8.2.

mdrummond’s picture

I'm sorry, this is on me. I got discouraged by the amount of work still necessary after I worked on this for almost all of DrupalCon's extended sprints. I would still like to get this in, but it seems challenging to do so by next Wednesday when I have a lot of other things on my plate as well. I would guess that if/when #2560457: Support drag-and-drop image uploads in CKEditor goes in, that will lead to a big merge conflict with this patch. Maybe not.

It is frustrating, because for the most part the functionality for this has been working since right before Drupal 8.0 RC1, but we've been lacking tests. I'm not the most knowledgeable on writing tests: I've tried to learn more, but it's still a pretty high bar. Particularly when it's hard to figure out what exactly needs to get tested.

I'll see if I can take another look and try to bite off at least a few chunks. I very much appreciated the feedback. I just found 31 points of things that needed to be fixed very intimidating. Maybe we can get this done one bite at a time.

Wim Leers’s picture

#177 #2421427: Improve the UX of Quick Editing single-valued image fields is only related to this issue in that it is something that relates to images. It's not in any technical way remotely related to this issue. And yes, this feature can co-exist with that one. #2421427: Improve the UX of Quick Editing single-valued image fields is for in-place editing image fields. This issue is about images embedded in a body field via Text Editors, and does not touch image fields in any way.

#179: AFAICT there will be zero conflict between this and #2560457: Support drag-and-drop image uploads in CKEditor. And of course, this can totally be done one point at a time!

#177+#178: mdrummond is right, the only thing we really lack here, is test coverage. Test coverage ensures it's working correctly today, prevents it from breaking tomorrow, and overall makes this maintainable. You're more than welcome to help with writing the necessary tests!

mdrummond’s picture

FileSize
108.48 KB
58.46 KB

Thanks for the update Wim that this should still be clear even with the other issues. That's good to know.

Making my way through the feedback items, starting with 1-9.

I have a new local setup for my core work. Getting this set up again held me back from digging into this for a while, but seems to be working again now. Hopefully the patches are still okay given that my setup is slightly different now.

  1. Fixed the eslint errors. Needed to copy the findElementByName function into the respective JS files. This was copied from the drupal image caption js
  2. Went through and fixed all the typehints in the files this patch touches. I think this mostly only affects the code that comes from this patch, but it's possible it touches up a few other bits.
  3. Trimmed the newline.
  4. Added some variable assignments. The logic in the assignment is slightly different for responsive images, but I believe the end result is the same.
  5. I get what you're saying here, but I'm not going to make taking out comments a priority right now. Will return to this as time allows.
  6. Moved the first line closer to where it is used, further down in the code.
  7. Removed this temp variable.
  8. I admittedly may have gone overboard here. Trying to look line by line through the patch to find errant array() assignments seemed like a tall order. So instead i went through the files this patch touches, and tracked down all the array()s that needed to be updated to []s. This increases the size of the patch, and if I'm going to be slapped down for this, that's understandable. In theory we eventually want all of these to be updated anyhow, so maybe this is helpful?
  9. Cool.

More later.

Outstanding item that still needs to be addressed: excessive commenting mentioned in point 5.

mdrummond’s picture

FileSize
108.44 KB
16.81 KB

Additional fixes based on feedback in #175:

10. Label, title and ID changed. Used the new ID in the update hooks as well, which is I believe the only place they're referenced.
11. That explanation makes sense about upcast. Flipped the order.
12. Updated this comment both here and in the corresponding responsive image class.
13. NOT ADDRESSED: As in my previous update, I haven't dug into removing comments yet.
14. The image styles are also used in the tips function at the bottom of this class, so not changing the name of this function.
15. Clarified the comment that these are being removed because they are no longer needed in the filtered output.
16. Moved around the comments to help clarify this.
17. Fair enough. Removed the temporary variable assignment.
18. NOT ADDRESSED: I don't remember. I'd have to look back through the issue history to better understand that.
19. Already tackled the array syntax conversion.
20. NOT ADDRESSED: Will tackle this later.
21. Thanks.
22. NOT ADDRESSED: I'll take a look at that.
23. Replaced generated with expected.
24. Leaving this as is for now.
25. Believe these file headers are all removed where possible now.
26. NOT ADDRESSED: To me it feels like a lot of overhead to add a trait for this, particularly when we'll still need some overrides (data-image-style vs data-responsive-image-style). Can look at that later if it seems essential.
27. Fixed.
28. NOT ADDRESSED: Fair enough. Will look later.
29. NOT ADDRESSED: Also will look later. For both 28 and 29, a general statement of "more assertions" is challenging to tackle. If there are any obvious things that you think should be tested, that would be helpful.
30. Moved this from system.install to image.install
31. I'm not entirely sure what this is referencing, but if you're happy, I'm happy.

So, here's the revised punch list:
- 5: Prune comments
- 13: Prune comments
- 18: Figure out what is going on here.
- 20: Add assertions
- 22: Look at missing docs/whether these are needed.
- 26: Determine if trait is needed.
- 28: Add assertions (guidance would be appreciated)
- 29: Add assertions (guidance would be appreciated

mdrummond’s picture

FileSize
108.12 KB
3.68 KB

In #157, I went through what is going on with each file in the patch.

I noted this, with answers the question raised in point 18:

image-style-preview.html.twig
Add a line break after the original image preview and the text that describes it: do the same for the image style preview. Feels weird to hardcode a br element in here.

It's fair to note that's odd, not sure how much time we want to spend finding another way to make sure the text appears on a new line.

Point 22:
Yes, looks like we can handle this with just local values rather than fully documented class properties. Patch attached that does that.

Point 26:
The following methods have nearly identical code in FilterImageStyle and FilterResponsiveImageStyle
- loadImageStyles vs loadResponsiveImageStyles
- getImageInfo
- prepareImageAttributes vs prepareResponsiveImageAttributes
- getImageStyleHtml vs getResponsiveImageStyleHtml

It's possible this could be written in a generic way. Maybe we have a boolean parameter for whether we're dealing with images vs responsive images that could then handle the slight differences between the two functions without the method that calls this from the trait needing to know the implementation details.

mdrummond’s picture

Just want to note that I've been thinking about point 26 some more, and I'm fairly dubious that it would be worthwhile to write a trait to share the image processing code with the responsive image module.

I know that 90% of the code is the same, and that's duplicative, but dealing with that 10% difference is going to be a pain. We're going to need to pass in some sort of parameter to trigger the responsive image version of the code output for these similar functions. However, the responsive image module is not enabled by default. So we'd have to account for that in determining whether to make use of that parameter or not. Between that and the extra boilerplate of writing a trait, I think the number of lines of code we'd write to avoid duplication would dwarf the couple dozen lines of code that are repeated. It doesn't seem like a good tradeoff to me.

If the code was exactly the same for images and responsive images, I'd feel differently.

If the responsive image module is enabled at a future point, perhaps adding that trait could be a future refactor.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Issue tags: -Media Initiative
igorik’s picture

sorry for this my post, but I feel this as exactly one of the reason why there never will be Drupal 9.
So useful feature, it is implemeted in wordpress, but Drupal users probably never will be have it.
Drupal 8 is here for 8 months, and it is already losing it's chance to be interesting for others.
Now here will be 2 years of discussion about this issue, 700 comments, 3 big changes of the structure and after this time already nobody will be use Drupal.
#RIP_Drupal

mdrummond’s picture

Settle down, everything will be okay. There is some more testing work that needs to get done to get this in. I had hoped to get that done for 8.2, but did not manage to do so. I'm planning to continue to work on this in the weeks ahead, though, so that hopefully we can this in for 8.3. This isn't something controversial, we just need to make sure this is solid before we merge it.

ron_s’s picture

@igorik, if you did a bit of research, you would know this functionality has existed in Drupal 7 for at least three years as part of the Picture module (https://www.drupal.org/project/picture). We use it on all our client projects, and it works really well.

The answer is really simple and straightforward. If this is a critical feature for you, don't upgrade to Drupal 8 yet. Drupal 7 is still fully supported and works.

sachbearbeiter’s picture

@mdrummond - thanks for all the work you spend here ...

effulgentsia’s picture

Priority: Normal » Major

Great work on this issue so far! Promoting it to Major. I think it should have been prioritized that way all along. Image styles and responsive image mappings are a foundational Drupal concept, applied to image fields, and it's a glaringly missing feature to not have them applied to images within text fields.

erik’s picture

Out of curiosity, I tried out patch #183 against my 8.1.8 installation, which results in the onscreen error
'The website encountered an unexpected error. Please try again later.'
This error appears both on:
node/add/page and
admin/config/content/formats/manage/basic_html.

The syslog displays the following error:
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "filter_imagestyle" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of /var/www/mydomain.com/dev/htdocs/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

Should the patch be ran against 8.3.x-dev and is the error there because of core 8.1.8 or is it for another reason?

rootwork’s picture

@erik I'm sure it's because of the core difference. Try testing against 8.3.x.

@mdrummond Thanks for all your continuing hard work on this. It's really exciting to see it moving forward!

fkelly’s picture

Version: 8.3.x-dev » 8.2.x-dev
Priority: Major » Normal
fkelly’s picture

Version: 8.2.x-dev » 8.3.x-dev
Priority: Normal » Major

cripes re. #194. I'm just an onlooker hoping this issue gets resolved. I would never change the version or priority (I don't even know how). Firefox hung on my computer and when I looked at the screen it appears I changed the version and and priority. This is not something I would do but I don't see any way to change it back or to delete my comment #194. If a moderator or someone in the know can fix it back for me I'd appreciate it. (edit to edit I just found issue metadata and think I've fixed it)

In fact I was contemplating whether to thank our core committer from #191 for boosting the priority on this. It is a feature that is badly needed out in the trenches.

Maybe in the interim between NOW and 8.3 a little love and attention could be given to:
https://www.drupal.org/project/inline_responsive_images
which, as a contributed module provides something of a work around.

And, maybe I'm missing something, but it does not seem possible to BOTH have ckeditor set up to upload and edit images at the same time. I use IMCE to work around this but it would be desirable to do both. Should I post that as a separate issue (say Feature Request). I realize you don't want multiple issues in one queue.

erik’s picture

@fkelly

Would indeed be a nice alternative until this one is ready for core. :)
But the thing was that it didn't work well, it doesn't like the output Twig debugging generates: https://www.drupal.org/node/2770965#comment-11509195

mdrummond’s picture

Tried to get started on a reroll for this, but I have a composer-based setup for my core local, and composer install does not appear to be working for 8.3.x yet. So I'm kind of jammed on moving forward because of that. I can work against 8.2.x for now, but once this is rerolled for 8.2.x, I effectively won't be able to test this unless I set up a new local development environment just to work on this patch. I'm not keen to do that.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
108.17 KB
7.3 KB

Got some time to work on this today. Here's a new patch that addresses some bugs since I made some of the feedback fixes.

I've renamed the filters to filter_image_style and filter_responsive_image_style. Using filter_ as a prefix seems to make more sense when lined up against other similar filter plugins. Particularly because that ends up being used as the config file, and it's really weird to see a file named image_style that isn't the definition of an image style but instead is the definition of a filter. filter_ just seems a lot more clear to me.

I fixed a few other little glitches. Something is kind of off with the responsive_image side of things, but I'm not quite sure what yet. Something with the validation function is off. I'll try to track that down.

This is rolled against 8.2.x because the subtree split for 8.3.x isn't ready yet. Glad to update this once that's ready.

Status: Needs review » Needs work

The last submitted patch, 198: 2061377-198-wysiwyg-image-styles.patch, failed testing.

JKerschner’s picture

I've applied the patch from #198 to my site, enabled the Pictures module, and setup the text format to use the new filter. Now CKEditor allows me to choose a responsive image style, but it seems to never be rendering the image. I've tried several filter processing orders, but none of them have fixed it. Is there some additional step necessary to get this working?

Also, just wanted to say thank you for all the work that's been done so far on this... I was really surprised when I found it wasn't part of Drupal Core, given how much of an emphasis was given to Drupal 8's responsiveness when it was being promoted, so it's nice to know that progress is being made toward making it responsive in the place where it seems--to me--it is most likely to come up. I look forward to it being in core, even if that isn't until 8.3.x or later.

afoster’s picture

@JKerschner - I can't confirm is the same bug but contrib port of this work in https://www.drupal.org/project/inline_responsive_images
reports an identical bug where if you enable TWIG debug. https://www.drupal.org/node/2770965.

The Twig comments break the output and nothing renders, disabling twig allows the image to render fine. It's worth trying to see if disabling twig debug fixed the issue (I did the trick for me on the contrib module)

Here's a patch that reports to fix the bug which may be a possible basis for a fix here. https://www.drupal.org/node/2802013 This patch is currently untested.

guschilds’s picture

Hey all,

I also found myself in the "I need this right now but I don't have the time or knowledge to bring it to the finish line" camp and it's apparent that I'm not alone, so I created a Simple Inline Responsive Images sandbox. As mentioned in the sandbox description, it's built primarily from the FilterResponsiveImageStyle class within the patch from #198, with the key difference of rendering all inline images through a single responsive image style rather than configuring the image style for each image. It currently works with Drupal core 8.2.0.

As also mentioned in the sandbox description, thanks to everyone who has worked so hard on this! I feel a bit guilty creating the sandbox from it without contributing to the patch itself, but I hope it can at least provide temporary relief to others while you continue to focus on Doing It Right.

Gus

juancasantito’s picture

The array short syntax changes somewhere between ~140 & 180+ in EditorImageDialog is causing some issues with applying this to 8.2.x. There's already a separate issue for that code style change, can we remove it from this patch?

juancasantito’s picture

Status: Needs work » Needs review
FileSize
102.63 KB
+++ b/core/modules/editor/src/Form/EditorImageDialog.php
@@ -96,26 +96,26 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor
-        'file_validate_size' => array($max_filesize),
-        'file_validate_image_resolution' => array($max_dimensions),

@@ -204,22 +204,24 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      if (!$form_state->getValue(['attributes', 'src'])) {
+        $form_state->setValue(['attributes', 'src'], $file_url);
+      }

Is this the only change in EditorImageDialog? If it is, then this should clean things up a little. That's the only piece added to that class. This also needed a re-roll, so no interdiff. Just re-roll and limiting what changed in EditorImageDialog.

Status: Needs review » Needs work

The last submitted patch, 204: allow_image-2061377-204.patch, failed testing.

juancasantito’s picture

Status: Needs work » Needs review
FileSize
102.61 KB
1.85 KB

Status: Needs review » Needs work

The last submitted patch, 206: allow_image-2061377-206.patch, failed testing.

juancasantito’s picture

So, this is a very large patch (>100k). Can we break this into two parts to make it easier to review? Maybe one for image module, one for responsive image?

For me, I just need the functionality from image module. And that is currently functional. The responsive part isn't functional, at all.

heddn’s picture

Title: Allow image style/responsive image mapping to be selected in Text Editor's image dialog (necessary for structured content) » Allow image style mapping to be selected in Text Editor's image dialog (necessary for structured content)
Status: Needs work » Needs review
FileSize
35.83 KB
69.13 KB

This is a much slimmer patch that only addresses the functionality in image module. It should make reviews and testing easier. We'll have to open a follow-up for responsive_image.

The interdiff is large, because it has a lot of code removed. I also removed a ton of out of scope short array fixes that were introduced in an earlier version. Which also helped a ton in getting this down to something more manageable.

Status: Needs review » Needs work

The last submitted patch, 209: allow_image-2061377-209.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
3.61 KB
36.3 KB

Status: Needs review » Needs work

The last submitted patch, 211: allow_image-2061377-211.patch, failed testing.

heddn’s picture

+++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
@@ -0,0 +1,253 @@
+      return new FilterProcessResult(HTML::serialize($dom));

Self review. Does this need to add any cache context for image styles. Like if the image style is rebuilt, flushed or changed?

heddn’s picture

heddn’s picture

Status: Needs work » Needs review
FileSize
985 bytes
36.53 KB

Status: Needs review » Needs work

The last submitted patch, 216: allow_image_style-2061377-216.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
36.42 KB
1.55 KB

Status: Needs review » Needs work

The last submitted patch, 218: allow_image_style-2061377-218.patch, failed testing.

heddn’s picture

OK, I've opened a follow-up to address #2822589: Followup: Allow image style to be selected in Text Editor - add preview. This means the next patch will have even less in it. I'm moving the image style preview stuff into a follow-up. The reason is posted in the other issue. TLDR; there's enough style problems with image_style_preview that a follow-up to add it back in is a good idea.

heddn’s picture

Status: Needs work » Needs review
FileSize
33.46 KB
2.39 KB
Wim Leers’s picture

Status: Needs review » Needs work

Thanks for the big push forward, @heddn! Here's a comprehensive review in return.

  1. +++ b/core/modules/editor/src/Form/EditorImageDialog.php
    @@ -209,7 +209,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -      $form_state->setValue(array('attributes', 'src'), $file_url);
    +      if (!$form_state->getValue(array('attributes', 'src'))) {
    +        $form_state->setValue(array('attributes', 'src'), $file_url);
    +      }
    

    Why is this change necessary?

  2. +++ b/core/modules/image/image.install
    @@ -61,3 +61,25 @@ function image_requirements($phase) {
    +function image_update_8001() {
    

    s/8001/8300/

    Must also be wrapped in comments like this:

    /**
     * @addtogroup updates-8.3.0
     * @{
     */
    
    …
    
    /**
     * @} End of "addtogroup updates-8.3.0".
     */
    
  3. +++ b/core/modules/image/image.install
    @@ -61,3 +61,25 @@ function image_requirements($phase) {
    +    $config = $config_factory->getEditable($name);
    +    if (!$config->get('filters.filter_image_style.status')) {
    +      continue;
    +    }
    +    $allowed_html = $config->get('filters.filter_html.settings.allowed_html');
    +    $matches = array();
    +    if (!empty($allowed_html) && preg_match('/<img([^>]*)>/', $allowed_html, $matches)) {
    +      $new_attributes = array_filter(explode(' ', $matches[1]));
    +      $new_attributes[] = 'data-image-style';
    

    Hm…

    1. When this patch was originally started, an approach like this made sense. But since then, we added the concept of "post-update functions". Those can actually use the proper config entity API. This should be moved to core/modules/image/image.post_update.php, where you can also find a pre-existing example.
    2. More importantly, this update will not actually update any existing text format. It's bailing if the file_image_style text filter is not enabled for a text format. Well, because that's a new text filter added by this patch, not a single text format will have it.
      We should choose: either we upgrade basic_html automatically for all sites, or we should let existing sites continue function like they do. Arguments can be made either way. But the update that this is doing is not really helpful right now.
      (And I'm probably the one who wrote this originally! My bad!)
  4. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +  /** @var \Drupal\editor\Entity\Editor $editor */
    

    Should typehint to the interface.

  5. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +  $filters = $editor->getFilterFormat()->filters()->getAll();
    ...
    +  if ((isset($filters['filter_image_style']) && $filters['filter_image_style']->status)) {
    

    This can be simplified to:

    if ($editor->getFilterformat()->filters('filter_image_style')->status) { … }
    

    An example of exactly that pattern can be found at \Drupal\editor\Form\EditorImageDialog::buildForm.

  6. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +  if ((isset($filters['filter_responsive_image_style']) && $filters['filter_responsive_image_style']->status)) {
    +    $filter_responsive_image_style_status = TRUE;
    +  }
    +  else {
    +    $filter_responsive_image_style_status = FALSE;
    +  }
    

    Should be removed.

  7. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +  if ($filter_image_style_status && !$filter_responsive_image_style_status) {
    

    Second condition shoudl be removed.

  8. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    // Get the image from the form.
    

    Not clear enough. What about:
    // Get the image (<img>) that is being edited on the client.

  9. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    // Disallow the user from specifying image dimensions manualy.
    +    $form['dimensions']['#access'] = FALSE;
    

    There's no form element for 'dimensions', so this can simply be deleted. (I think that did exist at some point a very very very long time ago.)

  10. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    // Create a form item for image style selection.
    +    $form['image_style'] = array(
    +      '#type' => 'item',
    +      '#field_prefix' => '<div class="container-inline clearfix">',
    +      '#field_suffix' => '</div>',
    +    );
    

    This container is no longer necessary if we're not doing the preview thing. If necessary, #2822589: Followup: Allow image style to be selected in Text Editor - add preview can add it back.

  11. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +      '#title' => t('Image style'),
    

    $this->t()

  12. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +      '#default_value' => isset($image_element['data-image-style']) ? $image_element['data-image-style'] : array_keys($image_style_options)[0],
    

    Is picking the first one by default sane/correct?

  13. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +      '#wrapper_attributes' => array('class' => array('container-inline')),
    +      '#attributes' => array('class' => array('container-inline')),
    +      '#parents' =>array('attributes', 'data-image-style'),
    

    We should triple-check whether we still need all this now that the preview is gone.

  14. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    // Validate that the image shown in the text editor matches the image style.
    

    We don't "validate" that. We ensure that. This comment can be removed; the validation callback's comments already explain this.

  15. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    // Get the image style attributes.
    

    Pointless comment. (Plus, it's wrong!)

  16. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    $image_style = \Drupal::service('entity_type.manager')->getStorage('image_style')->load($attributes['data-image-style']);
    

    ImageStyle::load($attributes['data-image-style'])

  17. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    $attributes['src'] = $image_style->buildUrl($uri);
    

    This must use a relative file URL.

    See \Drupal\editor\Form\EditorImageDialog::submitForm() + #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send.

  18. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    $image = \Drupal::service('image.factory')->get($uri);
    

    Must be injected.

  19. +++ b/core/modules/image/image.module
    @@ -483,3 +484,114 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +      $dimensions = array(
    

    s/array()/[]/

  20. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,112 @@
    +          responsiveimage: {
    

    Why not call this drupalimagestyle instead?

  21. +++ b/core/modules/image/js/plugins/drupalimagestyle/plugin.js
    @@ -0,0 +1,112 @@
    +        // Override downcast().
    ...
    +        // Override upcast().
    

    I think "override" describes better what's going on than "override".

  22. +++ b/core/modules/image/src/Plugin/CKEditorPlugin/DrupalImageStyle.php
    @@ -0,0 +1,86 @@
    +      $enabled = FALSE;
    +      $settings = $editor->getSettings();
    +      foreach ($settings['toolbar']['rows'] as $row) {
    +        foreach ($row as $group) {
    +          foreach ($group['items'] as $button) {
    +            if ($button === 'DrupalImage') {
    +              $enabled = TRUE;
    +            }
    +          }
    +        }
    +      }
    +      return $enabled;
    

    This can be simplified by doing:

        $toolbar_buttons = CKEditorPluginManager::getEnabledButtons($editor);
         $neabled = in_array('DrupalImage', $toolbar_buttons);
    

    This is a new API that was introduced in 8.1.0, see https://www.drupal.org/node/2648056. For another conversion issue, see #2696771: Minor clean-up of \Drupal\ckeditor\Plugin\CKEditorPlugin\Internal::getConfig().

  23. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,253 @@
    +  /**
    +   * @var \Drupal\Core\Entity\EntityRepositoryInterface
    +   */
    +  protected $entityRepository;
    +
    +  /**
    +   * @var \Drupal\Core\Image\ImageFactory
    +   */
    +  protected $imageFactory;
    +
    +  /**
    +   * @var \Drupal\Core\Render\RendererInterface
    +   */
    +  protected $renderer;
    

    Nit: incomplete docblocks.

  24. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,253 @@
    +   * Constructs a Drupal\image\Plugin\Filter\FilterImageStyle object.
    

    Nit: no FQCN here.

  25. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,253 @@
    +      // Load all the image styles so each img found in the text can be checked
    ...
    +    // Process the filter if no image style img elements are found.
    

    s/img/Only local images are allowed./
    or
    s/img/image/

  26. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,253 @@
    +      $image_styles = $this->loadImageStyles();
    ...
    +        // If the image style is not a valid one, then don't transform the HTML.
    +        if (empty($file_uuid) || !in_array($image_style_id, $image_styles)) {
    

    If we're going to be validating image styles, we're also going to need to validate file UUIds. Which this code does not. Nor does the existing \Drupal\editor\Plugin\Filter\EditorFileReference filter. Of course, this sounds like a good idea. But we should apply it to everything consistently then, including the existing filter.

    So let's defer this to a follow-up.

  27. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,253 @@
    +    // Remove attributes that are no longer needed.
    +    $dom_element->removeAttribute('data-entity-type');
    +    $dom_element->removeAttribute('data-entity-uuid');
    +    $dom_element->removeAttribute('data-image-style');
    

    This is also something \Drupal\editor\Plugin\Filter\EditorFileReference does not yet do. Again should be consistent. Let's move this to another follow-up.

  28. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,253 @@
    +    return $this->renderer->render($image);
    

    This is wrong. It could cause bubbling. It needs to be rendered in isolation. And it's then up to the calling code to import the bubbled cacheability metadata + attachments.

    This must be wrapped in an executeInRenderContext() call. Like so:

        $output = $this->executeInRenderContext(new RenderContext(), function () use (&$image) {
          return $this->render($image);
        });
    

    Specifically, we need to do the same as what EntityEmbedFilter did:

    +            // We need to render the embedded entity:
    +            // - without replacing placeholders, so that the placeholders are
    +            //   only replaced at the last possible moment. Hence we cannot use
    +            //   either renderPlain() or renderRoot(), so we must use render().
    +            // - without bubbling beyond this filter, because filters must
    +            //   ensure that the bubbleable metadata for the changes they make
    +            //   when filtering text makes it onto the FilterProcessResult
    +            //   object that they return ($result). To prevent that bubbling, we
    +            //   must wrap the call to render() in a render context.
    +            $entity_output = $this->renderer->executeInRenderContext(new RenderContext(), function () use (&$build) {
    +              return $this->renderer->render($build);
    +            });
    +            $result = $result->merge(BubbleableMetadata::createFromRenderArray($build));
    

    (See http://cgit.drupalcode.org/entity_embed/commit/?id=c248cda0da575d6a9f5f9...)

  29. +++ b/core/modules/image/tests/src/Kernel/EditorImageStyleDialogTest.php
    @@ -0,0 +1,144 @@
    +      'format' => 'filtered_html',
    

    Let's give this a $this->randomMachinename()

  30. +++ b/core/modules/image/tests/src/Kernel/EditorImageStyleDialogTest.php
    @@ -0,0 +1,144 @@
    +      'name' => 'Filtered HTML',
    

    Let's use $this->randomString()

  31. +++ b/core/modules/image/tests/src/Kernel/EditorImageStyleDialogTest.php
    @@ -0,0 +1,144 @@
    +      'format' => 'filtered_html',
    

    $format->id()

  32. +++ b/core/modules/image/tests/src/Kernel/EditorImageStyleDialogTest.php
    @@ -0,0 +1,144 @@
    +    // If image style is selected, image dimensions should not be available.
    +    $this->assertFalse($form['dimensions']['#access']);
    +    // Image style should be medium.
    +    $this->assertEquals('medium', $form['image_style']['selection']['#default_value']);
    

    This is not testing very much.

  33. +++ b/core/modules/image/tests/src/Unit/FilterImageStyleTest.php
    @@ -0,0 +1,106 @@
    +  public function testProcessWithImage() {
    

    This is only testing one case. This is not testing any edge cases.

    Using a data provider would make sense here.

  34. +++ b/core/modules/system/css/components/container-inline.module.css
    @@ -11,3 +11,7 @@
    +/* Allow items inside inline items to render themselves as blocks. */
    +.container-inline .container-block {
    +  display: block;
    +}
    

    Is this change really necessary?

  35. +++ b/core/profiles/standard/config/install/filter.format.basic_html.yml
    @@ -36,6 +37,12 @@ filters:
    +  filter_image_style:
    +    id: filter_image_style
    +    provider: image
    +    status: true
    +    weight: 10
    +    settings: {  }
       editor_file_reference:
         id: editor_file_reference
         provider: editor
    
    +++ b/core/profiles/standard/config/install/filter.format.full_html.yml
    @@ -3,12 +3,19 @@ status: true
    +  filter_image_style:
    +    id: filter_image_style
    +    provider: image
    +    status: true
    +    weight: 7
    +    settings: {  }
       filter_align:
         id: filter_align
         provider: filter
    

    In both of these text formats, the new filter is configured to run before editor_file_reference. I'm pretty certain that will break, because editor_file_reference will overwrite the src attribute. (Since #2666382: EditorFileReference filter: generate URLs for inline images.) This points to one big gap in testing: integration tests. I'd like to see a FunctionalJavaScriptTestBase test where we have integration testing with actual verification that the inserted image gets the dimensions of the image style etc, and that after saving, the image still has those dimensions. I'm 99% certain that after saving it won't have those dimensions, due to the problem I explained here.

  36. +++ b/core/themes/classy/css/components/container-inline.css
    @@ -7,10 +7,12 @@
    -.form-type-radios .container-inline label:after {
    +.form-type-radios .container-inline label:after,
    +.container-inline .form-type-checkbox label:after {
       content: '';
     }
    -.form-type-radios .container-inline .form-type-radio {
    +.form-type-radios .container-inline .form-type-radio,
    +.container-inline .form-type-checkbox {
    

    Let's revert these changes?

Wim Leers’s picture

Title: Allow image style mapping to be selected in Text Editor's image dialog (necessary for structured content) » Allow image style to be selected in Text Editor's image dialog (necessary for structured content)

Title was a bit inaccurate. ("Responsive image style" used to be called "picture mapping", that's where the "mapping" word originated from.)

heddn’s picture

Status: Needs work » Needs review
FileSize
30.62 KB
15.07 KB

1) That came in at #2061377-155: Allow image style to be selected in Text Editor's image dialog (necessary for structured content). Removing it as it seems like a stray addition. Or maybe Marc can remember and we'll add it back.

2-10) done
11) This isn't a class, it is a module file. I don't $this is possible
12) removed a default so as to force the user to select something
13) removed
14-17 done
18) I do not think this is possible in a .module file.
19) I'm intentionally not introducing short array syntax. The rest of the .module file uses the older style, so I've stuck with that in this file. I think that is how we are supposed to do this, yes? If there are brand new files, I've adopted short array.
20) done
21) Don't understand the comment. Leaving things as is for now until I get clarification.
22) You mean drupalimagestyle, no?
23-25) done
26-27) a stub todo added, need to still open the follow-ups.
28) Still noodling on this comment. No changes just yet while I try to digest it.
30-31) done
32-33) agreed, no changes yet though.
34) reverted
35) weights altered.
36) 110% agreed.

Plus, we need a test of the post_update. But moving to needs review for the test bot. I'm going to put this to the side for a few days, in case someone else wants to jump on this and keep the momentum going.

Side note: we are down to 30kb from an original 102kb. I think this is shaping up nicely.

Wim Leers’s picture

  • 11 + 18: d'oh, of course!
  • 19: of course, makes sense :)
  • 21: That's because my comment made no sense at all :) I wanted to say decorate instead of override.
  • 22: no, that comment of mine was correct AFAICT
  • 28: yeah this is the most complex part of my review for sure. You may want to read the associated entity_embed issue. Or I can reroll this patch with just that part changed.
heddn’s picture

Issue tags: +Novice

Flagging novice for 225's #21 & #22. Less novice is #28.

Adita’s picture

Assigned: Unassigned » Adita
Adita’s picture

Assigned: Adita » Unassigned
FileSize
30.62 KB
2.33 KB

#22 it was already done in the last patch 224, #21 done, #28 done.

tkoleary’s picture

Issue tags: +Usability, +sprint
tkoleary’s picture

Patch not working in simplytest.me

Uploading image no combination of settings submits the form and closes the modal, but no error appears.

tkoleary’s picture

Status: Needs review » Needs work
ifreeman’s picture

This fixes several bugs to the core functionality of the recent patches:

1. Restore data-image-style attribute when inserting inline images (access ckeditor allowedContent as the array that it is, allowing the js plugin to do its job and apply data-image-style).

2. Transform image url during editor dialog validation to relative only after grabbing it from image style.

3. Revert a name change for the DrupalImage ckeditor toolbar button, allowing the drupalimagestyle plugin to load.

4. Access ckeditor allowedContent as the array that it is, allowing the js plugin to do its job and apply data-image-style. same as #1.

Other points:
5. Add more testing for image style filter and related javascript. Changed FilterImageStyleTest from unit to functional. The efficacy of the unit test seemed poor with the meat of the filter, getImageStyleHtml(), being mocked.

6. Fixed all CS violations.

7. I think the proposed functionality to automatically apply an image style rather than allow selection should be in core. Without that functionality, this issue only partially solves the intent of placing style-formatted inline images. I'm sure many site owners will not want to give style selection control to all users.

I propose adding a default image style selection to the filter settings, modifiable by those with administer image styles permission. Additionally, the image style selection in EditorImageDialog should similarly be available only to those with the same permission. Thoughts?

8. image.post_update.php could still use coverage.

Status: Needs review » Needs work

The last submitted patch, 232: 2061377-232.patch, failed testing.

mdrummond’s picture

I think in an ideal world it would be possible to select which image styles can be applied via WYSIWYG. I could certainly see a possibility for allowing different types of inline images, rather than just one. For example, images that take up the full width of a content block vs images that float left or right. I could also see that sometimes varying for different wysiwyg fields. Varying allowed image styles—whether that's per text format, per field, and/or per role—seems like a pretty significant scope increase. I'd rather see a simple version of this get in first, then add additional features like that in follow-up issues. This issue has already taken ages without a big scope increase.

Thanks so much for the work on this, ifreeman.

ifreeman’s picture

Status: Needs work » Needs review

I totally agree that it should be a follow-up. I was commenting in reply to Wim Leers' #8:

Configuring a single image style or picture mapping in the filter configuration — hence not allowing content authors to choose — could be a contrib module.

To that, I think it's important enough to come in core. However I did not think through the possibilities as you have. Yes, the follow-up could be quite a complex issue.

Marking for review since the retest passed.

claudiu.cristea’s picture

Status: Needs review » Needs work
Issue tags: -Novice, -sprint +Needs upgrade path tests
  1. +++ b/core/modules/image/image.module
    @@ -483,3 +484,84 @@ function image_field_config_delete(FieldConfigInterface $field) {
    + * Alters the image dialog form for text editor, to allow the user to select an
    + *   image style.
    

    Second line should be left-aligned with the first.

  2. +++ b/core/modules/image/image.module
    @@ -483,3 +484,84 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +function image_form_editor_image_dialog_alter(&$form, FormStateInterface $form_state) {
    

    It's obvious why the hook implementation lives here: When image.module is disabled, the feature is disabled too. However, even this solution is simple and ingenious, from an architectural point of view looks wrong. This is an editor functionality that hooks in another editor functionality. At least for me looks weird to see functionality belonging to editor module inside image module. For this reason I would move this hook implementation (+the validation callback) in Editor module even this mean to add an

    if (!\Drupal::moduleHandler()->moduleExists('image')) {
      return;
    }
    

    I think it would make more sense. Let's discuss this.

  3. +++ b/core/modules/image/image.module
    @@ -483,3 +484,84 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +  if ($editor->getFilterformat()->filters('filter_image_style')->status) {
    

    s/getFilterformat/getFilterFormat

    Off-topic: I wonder why FilterBase::$status is a public property instead of being protected with a setter and a getter.

  4. +++ b/core/modules/image/image.module
    @@ -483,3 +484,84 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    // Get an array of image styles to present as options for selection.
    +    $image_style_options = image_style_options(FALSE);
    
    +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,252 @@
    + * @Filter(
    + *   id = "filter_image_style",
    ...
    + * )
    ...
    +class FilterImageStyle extends FilterBase implements ContainerFactoryPluginInterface {
    

    The image dialog can be exposed also to regular users. The admin might not want to show all the image styles to the user. Imagine that it's possible to have styles, used in other places on the site, that might break here the layout if are used. For this reason, I suggest that filter_image_style should implement settingsForm() method and allow site builders to restrict the list of image styles being used in that specific text format.

  5. +++ b/core/modules/image/image.module
    @@ -483,3 +484,84 @@ function image_field_config_delete(FieldConfigInterface $field) {
    +    $form['image_style']['selection'] = array(
    ...
    +      '#parents' => array('attributes', 'data-image-style'),
    ...
    +      $dimensions = array(
    

    Let's switch to modern square brackets array syntax.

  6. +++ b/core/modules/image/image.post_update.php
    @@ -20,3 +21,26 @@ function image_post_update_image_style_dependencies() {
    +function image_enable_filter_image_style() {
    

    This needs upgrade path test.

  7. +++ b/core/modules/image/image.post_update.php
    @@ -20,3 +21,26 @@ function image_post_update_image_style_dependencies() {
    +  $filters = ['basic_html' => 11, 'full_html' => 12];
    +  foreach ($filters as $filter => $weight) {
    ...
    +      $filter->setFilterConfig('filter_image_style', ['status' => TRUE, 'weight' => $weight]);
    

    I don't understand the reason for those weight values. We cannot predict the weight of existing text formats, do we?

  8. +++ b/core/modules/image/image.post_update.php
    @@ -20,3 +21,26 @@ function image_post_update_image_style_dependencies() {
    +  foreach ($filters as $filter => $weight) {
    

    Let's use $format_id for the text format ID for more clarity. And $filter (the config entity) should be $format, right?

  9. +++ b/core/modules/image/src/Plugin/CKEditorPlugin/DrupalImageStyle.php
    @@ -0,0 +1,77 @@
    + * Defines the "drupalimagestyle" plugin.
    ...
    + *   id = "drupalimagestyle",
    ...
    +class DrupalImageStyle extends PluginBase implements CKEditorPluginInterface, CKEditorPluginContextualInterface {
    

    I don't understand why prefixing with "Drupal".

  10. +++ b/core/modules/image/src/Plugin/CKEditorPlugin/DrupalImageStyle.php
    @@ -0,0 +1,77 @@
    +    // button is enabled.
    

    The word 'button' can go to the previous line.

  11. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,252 @@
    +use Drupal\Component\Utility\Html;
    

    Unused statement.

  12. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,252 @@
    +        // @TODO: File a follow-up. See #222-26.
    ...
    +    // @TODO: File a follow-up. See #222-27.
    

    The follow-ups should be opened.

  13. +++ b/core/modules/image/src/Plugin/Filter/FilterImageStyle.php
    @@ -0,0 +1,252 @@
    +   * Get the the width and height of an image based on the file UUID.
    ...
    +   * Get the HTML for the img element after image style is applied.
    

    s/Get/Gets

  14. +++ b/core/modules/image/tests/src/Functional/FilterImageStyleTest.php
    @@ -0,0 +1,121 @@
    +/**
    + * @coversDefaultClass \Drupal\image\Plugin\Filter\FilterImageStyle
    + * @group image
    + */
    

    Still needs a description line.

  15. +++ b/core/modules/image/tests/src/Functional/FilterImageStyleTest.php
    @@ -0,0 +1,121 @@
    +   * Text filter allowing images and has FilterImageStyle applied.
    

    "Text filter"? I think "Text format"

  16. +++ b/core/modules/image/tests/src/Functional/FilterImageStyleTest.php
    @@ -0,0 +1,121 @@
    +   * @var \Drupal\Core\Entity\EntityInterface
    

    Let's cast to FilterFormatInterface

  17. +++ b/core/modules/image/tests/src/FunctionalJavascript/AddImageTest.php
    @@ -0,0 +1,106 @@
    +    $this->assertJsCondition("jQuery('$image_button_selector').length > 0", 20000);
    ...
    +
    

    20 seconds is a huge interval. 10 or 5?

  18. +++ b/core/modules/image/tests/src/FunctionalJavascript/AddImageTest.php
    @@ -0,0 +1,106 @@
    +    $script = "jQuery('input[id^=\"edit-actions-save-modal\"]').click()";
    +    $this->getSession()->executeScript($script);
    

    Why do we run JS instead of directly clicking on the element?

ifreeman’s picture

Thanks for the quick review, claudiu.cristea. To your points:

  1. Looks like I got CS-fix hungry and applied indentation to the wrong continuation. Fixed.
  2. I agree in part and other code placement in this patch feels odd to me as well. However this has been discussed in this issue already (most notably #161). The conclusion is:
    functionality that depends on a module needs to live in that module

    Therefore, I won't move this.

  3. Bah. Php's insensitivity needs to go away. Fixed.
    Off-topic: Probably because status can be set by property during instantiation, although I haven't read into the underlying code.
    FilterFormat::create([
      'filters' => [
        'filter_image_style' => ['status' => TRUE],

    While finding my code to paste above, I went ahead and replaced 'status' => 1 with 'status' => TRUE in the patch.

  4. I agree, and it has been discussed earlier. I've opened up #2840462: Restrict image styles in Text Editor's image dialog for follow-up.
  5. Initially I didn't convert them because of heddn's #224-19. But after reading that #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 was just approved by the coding standards committee, I think we can do our part to change this patch's code. Done.
  6. Indeed. This is still outstanding.
  7. I have to say I don't understand this either. Perhaps the author was confusing the importance of filter weights with format weights? It should be investigated.
  8. Should be rewritten along with the previous item.
  9. There is precedent for this. See the existing ckeditor plugins:
    core/modules/ckeditor/js/plugins/drupalimage/plugin.js
    core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php

    core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
    core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImageCaption.php

    core/modules/ckeditor/js/plugins/drupallink/plugin.js
    core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalLink.php

    I imagine the other three plugins are not named such because they are from upstream CKEditor plugins. I imagine that is the motivation to prefix with Drupal, to differentiate from upstream plugin code.

  10. Done.
  11. Actually it was in use by HTML::load($text) and HTML::serialize($dom), but again, case sensitivity. Fixed class references.
  12. Opened #2840511: Validate file instances in Text Editor fliters and #2840513: Remove attributes from filtered inline images.
    I left the empty uuid check in this patch, since it prevents an exception later on.
  13. Done.
  14. Done.
  15. Absolutely. Done.
  16. $this->format instanceof \Drupal\Filter\FilterFormatInterface is already true, so I'll just update the FQCN.
  17. I was running into random test fails with various parts of the test (js not done loading elements). The extra duration can only help, and should be cut short as soon as the condition is true. QuickEditImageTest uses the same duration for one of its load tests, so is not unprecedented. See next issue.
  18. I tried to document that in the comment. More detail:
    Clicking on the button directly resulted in the same internal phantomjs exception as described in #2830485-12: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly, even with a wait of 25 seconds in place. Trying instead to submit the form was the same. This implementation was my third attempt and thankfully worked. I'm not sure why this succeeds where ->click() does not.

Leaving as needs work due to #6 above.

naveenvalecha’s picture

Assigned: Unassigned » naveenvalecha
Status: Needs work » Needs review

Setting to N/R for testing #237
Assigning to myself for #236.6
//Naveen

naveenvalecha’s picture

Initial attached patch which changes the name of the post-update hook and rename of few variables b/c the update hook was failing on local. however the above patch passes b/c the update hook was not properly named.

Error: Call to undefined method Drupal\filter\Plugin\Filter\FilterHtml::setFilterConfig() in /Applications/MAMP/htdocs/d83/core/modules/image/image.post_update.php,
line 41

//Naveen

Status: Needs review » Needs work

The last submitted patch, 239: 2061377-239.patch, failed testing.

naveenvalecha’s picture

Assigned: naveenvalecha » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs upgrade path tests
FileSize
37.23 KB
4.17 KB
3.44 KB

Addressed #236.6(Added upgrade path test.
// Naveen

Status: Needs review » Needs work

The last submitted patch, 241: 2061377-241.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Tytest’s picture

So I understand correctly, this feature is still under development and slated for the 8.4 release, or potentially can we see it sooner?

Additionally, it would be great if the Inline Image could also Link Image To (original file or other image style) just as we do with managing content types.

fkelly’s picture

Since this issue does not seem amenable to a programming approach anytime soon, was wondering if it would be possible to produce documentation about how we could use the source view in ckeditor to type in the proper codes to have responsive image styles in our content?