This is the follow-up issue for #2014895: Image captions & alignment (for inline images). That issue was about having image captions in Drupal core that don't prevent structured content. However, it was still necessary for users to manually modify the HTML with the right syntax; there was no CKEditor (WYSIWYG) support.

This issue is about providing that: a great UX for adding captions to images and aligning images.

It builds on 2 things:

  1. #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms
  2. CKEditor Widgets

It slightly expands the image dialog, and allows the user to directly modify the caption text. Screenshots:

image-caption-dialog.png

image-caption-widget.png

CommentFileSizeAuthor
#83 llamas-dancing.jpg162.64 KBWim Leers
#78 Screen Shot 2013-09-11 at 11.54.41.png97.13 KBWim Leers
#76 Screen Shot 2013-09-10 at 2.58.25 PM.png80.64 KBwebchick
#76 Screen Shot 2013-09-10 at 2.59.32 PM.png66.78 KBwebchick
#74 Create Article | Site-Install 2013-09-10 14-22-48.png137.89 KBwebchick
#74 Create Article | Site-Install 2013-09-10 14-25-31.png231.71 KBwebchick
#74 Screen Shot 2013-09-10 at 2.30.28 PM.png16.99 KBwebchick
#71 Screenshot of an inlined image in a CKEditor. The image is extremely large, causing a scroll bar to appear146.7 KBjessebeach
#69 ckeditor_widget_imagecaption-2027181-69.patch1.31 MBWim Leers
#68 interdiff.txt467 bytesWim Leers
#68 ckeditor_widget_imagecaption-2027181-68.patch1.31 MBWim Leers
#66 image_caption_widget_placeholder.m4v.zip1.68 MBWim Leers
#66 ckeditor_widget_imagecaption-2027181-66.patch1.31 MBWim Leers
#66 interdiff.txt1.58 KBWim Leers
#64 Create Article | Site-Install 2013-09-10 01-23-10.jpg156.68 KBwebchick
#61 Create Article | Site-Install 2013-09-10 01-02-02.jpg133.45 KBwebchick
#61 Create Article | Site-Install 2013-09-10 01-04-04.jpg133.17 KBwebchick
#59 ckeditor_widget_imagecaption-2027181-59.patch1.31 MBWim Leers
#57 ckeditor_widget_imagecaption-2027181-57.patch1.32 MBWim Leers
#57 interdiff.txt664 bytesWim Leers
#55 ckeditor_widget_imagecaption-2027181-55.patch1.31 MBWim Leers
#55 interdiff.txt2.58 KBWim Leers
#54 Screenshot of a node page. The image in the node should have a caption underneath, but that caption is missing613.87 KBjessebeach
#54 Screenshot of a node page after editing the existing node and re-adding the caption. The image caption is now present.610.13 KBjessebeach
#53 ckeditor_widget_imagecaption-2027181-53.patch1.31 MBWim Leers
#53 interdiff.txt15.51 KBWim Leers
#50 ckeditor_widget_imagecaption-2027181-50.patch1.31 MBWim Leers
#44 ckeditor_widget_imagecaption-2027181-44.patch1.31 MBWim Leers
#38 drupalimage plugin js error255.74 KBjessebeach
#37 ckeditor_widget_imagecaption-2027181-37.patch1.31 MBWim Leers
#37 interdiff.txt3.9 KBWim Leers
#33 ckeditor_widget_imagecaption-2027181-33.patch1.31 MBWim Leers
#33 interdiff.txt1.33 KBWim Leers
#31 drupal-error-log.txt11.61 KBseutje
#30 ckeditor_widget_imagecaption-2027181-30.patch1.31 MBWim Leers
#30 interdiff.txt9.05 KBWim Leers
#26 ckeditor_widget_imagecaption-2027181-26.patch1.31 MBWim Leers
#25 imagecaption-widget-piotrek-debugging-do-not-test.patch1.31 MBWim Leers
#16 Screen Shot 2013-07-05 at 6.04.13 PM.png88.23 KBBojhan
#14 ckeditor_widget_image_caption.mov_.zip2.38 MBWim Leers
#13 ckeditor_widget_imagecaption-2027181-13.patch38 KBWim Leers
#13 interdiff.txt2.11 KBWim Leers
#11 ckeditor_widget_imagecaption-11.patch35.95 KBWim Leers
#11 interdiff.txt10.54 KBWim Leers
#11 ckeditor_widget_imagecaption-CKE-UPDATE-2027181-11.patch2.64 MBWim Leers
#8 ckeditor_widget_imagecaption-2027181-8.patch35.42 KBWim Leers
#8 ckeditor_widget_imagecaption-CKE-UPDATE-2027181-8.patch2.64 MBWim Leers
#7 ckeditor_widget_imagecaption-2027181-7.patch30.72 KBWim Leers
#1 ckeditor_widget_imagecaption-2027181-1.patch24.82 KBWim Leers
#1 ckeditor_widget_imagecaption-CKE-UPDATE-2027181-1.patch2.64 MBWim Leers
image-caption-widget.png250.13 KBWim Leers
image-caption-dialog.png21.13 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Here is the patch showing the current state of affairs.

Reviews welcome, especially usability reviews, but no nitpicky code reviews yet please.

Built on top of:

This includes a new build of CKEditor, of their latest d8-imagecaption branch, built with the same build-config.js, with the sole change of having added the "widget" plugin.


TODO:


2014895

Test here on simplytest.me, using an uploaded image: http://simplytest.me/project/drupal/712fe2a18dce3581d3301b43777ac6749ca6c502?patch[]=https://drupal.org/files/image_captions-2014895-32.patch&patch[]=https://drupal.org/files/ckeditor_dialogs-1879120-91.patch&patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-CKE-UPDATE-2027181-1.patch&patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-2027181-1.patch

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Usability

The last submitted patch, ckeditor_widget_imagecaption-CKE-UPDATE-2027181-1.patch, failed testing.

Bojhan’s picture

I am wondering, don't you set caption position in your html/css? Seems somewhat weird this would be up to the content creator.

Wim Leers’s picture

#3: The caption is always positioned underneath the image. You can change the CSS to put the caption on top of or next to the image.

Note that zero new functionality is being introduced here. It only provides a front-end to #2014895: Image captions & alignment (for inline images)!

Wim Leers’s picture

Wim Leers’s picture

Issue tags: +Needs usability review

Infuriating.

Wim Leers’s picture

Reroll of #1: I forgot one crucial file.

Use the same CKE UPDATE patch.

Wim Leers’s picture

All TODOs mentioned in #1 finished. This is now ready for final reviews, except that #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms needs to get committed *first*.

Inserting new images works well now (it required an API change in CKE Widgets), and using this together with the alignment buttons also works fine.

Built on top of the same patch stack as #1.

Status: Needs review » Needs work

The last submitted patch, ckeditor_widget_imagecaption-2027181-8.patch, failed testing.

Wim Leers’s picture

Category: feature » task

#2014895: Image captions & alignment (for inline images) has just been marked as a task, not a feature request by catch, hence this should also become a task.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.64 MB
10.54 KB
35.95 KB

Reroll now that #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms, #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default) and #1473066: [Meta] Move third-party assets out of core/misc folder have been committed.

Test on simplytest.me: D8 c09d2791a7e75212beb8d51981c3f649983a14fa plus the two attached patches.


Changes:

  • s/imagecaption/drupalimagecaption/, to have every Drupal-specific CKEditor command prefixed with "drupal"
  • Update the version information for the "ckeditor" library, to include the specific commit and branch based on which this CKEditor build was generated
  • File-level doxygen added.
  • One bit of RTL CSS was missing.

Now ready for review!

Bojhan’s picture

How do I get the caption?

Wim Leers’s picture

#12: I'll post a screencast very soon.


Reroll to fix the two minor test failures in #11.

Wim Leers’s picture

Screencast: http://vimeo.com/wimleers/image-caption-ckeditor-widget-drupal-8.

(Also attached as a .zip file in case the above URL ever stops working. I wish d.o allowed uploading + embedding screencasts.)

quicksketch’s picture

So for others that had trouble figuring this out, you have to apply the second patch from #11 (which updates CKEditor) AND #13. Although in hindsight Wim basically said that in #11.

So some experiences I had with this:

  • When I insert an image as the first thing in the editor, it seems to be impossible to get the cursor to go in front of the image. I expect I should be able to use the arrow keys to get the cursor in front of the image, but it just keeps selecting the caption + image widget.
  • Copy and paste of the image widget doesn't work. Seems like cut/copy/paste simply doesn't do anything when attempting to cut/copy an entire image + caption. Drag and drop to move the position of the widget doesn't seem like it's possible either. Should this functionality be working at this point or does CKEditor need to continue refining widgets for these things to work?
+++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.jsundefined
@@ -98,7 +138,7 @@ CKEDITOR.plugins.add('drupalimage', {
-          label: editor.lang.image.menu,
+          label: Drupal.t('Image'),

I don't think we can use Drupal.t() in CKEditor plugins because they don't pass through drupal_js_alter(), which is how Locale.module adds its translated strings.

+++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/theme.jsundefined
@@ -0,0 +1,207 @@
+      widgetDefinition.template =
+        '<figure class="caption caption-img">' +
+          '<img src="" data-caption="" data-align="center" />' +
+          '<figcaption></figcaption>' +

The caption filter runs through the theme layer, but this template seems like it's hard-coded. It seems like we should at least use a CKEditor settings variable for creating our template, making it possible to match any overrides to the theming of caption filter.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.phpundefined
@@ -296,8 +296,8 @@ public function buildContentsCssJSSetting(EditorEntity $editor) {
-    $css = array_merge($css, _ckeditor_theme_css());
     drupal_alter('ckeditor_css', $css, $editor);
+    $css = array_merge($css, _ckeditor_theme_css());

Usually drupal_alter() gets the last shot at modifying something, what's the reason for moving the theme CSS after the alter?

+++ b/core/themes/bartik/bartik.info.ymlundefined
@@ -11,6 +11,8 @@ stylesheets:
+ckeditor_stylesheets:
+  - css/ckeditor-iframe.css

Yay! Nice to see the "CKEditor CSS files from the info file" feature being used.

Bojhan’s picture

@Wim I am getting a white area on simplytest.me?
Screen Shot 2013-07-05 at 6.04.13 PM.png

Wim Leers’s picture

[…] it seems to be impossible to get the cursor to go in front of the image […]

Pinging the CKEditor developers about that.

Copy and paste of the image widget doesn't work.

That will be supported once the underlying CKEditor Widgets system supports it. They're still working on that.


I don't think we can use Drupal.t() in CKEditor plugins because they don't pass through drupal_js_alter(), which is how Locale.module adds its translated strings.

You're right, but what you were doing there earlier (referring to editor.lang.image.menu also doesn't work: those won't exist anymore once we remove the "image" plugin. So we need to figure out a solution to this problem. In any case, that is broken already in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms so I suggest we work on that in another issue. I created one, and I pinged domain experts Gábor Hojtsy & nod_, as well as the CKEditor developers: #2035721: Figure out how to translate Drupal-specific CKEditor plugins.


The caption filter runs through the theme layer, but this template seems like it's hard-coded. It seems like we should at least use a CKEditor settings variable for creating our template, making it possible to match any overrides to the theming of caption filter.

First: note that it's in drupalimagecaption/theme.js. The base logic lives at drupalimagecaption/plugin.js. If any theme wants to override the HTML structure, and they want this to be accurately reflected in CKEditor too, then they must override drupalimagecaption/theme.js as well.

Second: I know that this is less than ideal, and I would have loved to simply have a Drupal.theme.filter_caption function, but the CKEditor developers assured me that this is not technically possible. We've had long discussions about this. If necessary, I'm sure they won't mind explaining that here on the issue.

Third: a factor that should have some weight is the fact that the default HTML structure used by filter-caption.html.twig is using the HTML5 best practices. It should be sufficiently flexible to simply override the CSS to suit >95% of all use cases. Which means only experts would need to override the JS.

So: do you think this approach is acceptable?


Usually drupal_alter() gets the last shot at modifying something, what's the reason for moving the theme CSS after the alter?

This change was necessary to be able to achieve the required CSS order.

First, there is this, to only add the filter_caption's filter when it is actually necessary.

/**
 * Implements hook_ckeditor_css_alter().
 */
function ckeditor_ckeditor_css_alter(array &$css, Editor $editor) {
    $filters = array();
    if (!empty($editor->format)) {
      $filters = filter_format_load($editor->format)
        ->filters()
        ->getAll();
    }

  // Add the filter caption CSS if the text format associated with this text
  // editor uses the filter_caption filter. This is used by the included
  // CKEditor DrupalImageCaption plugin.
  if (isset($filters['filter_caption']) && $filters['filter_caption']->status) {
    $css[] = drupal_get_path('module', 'filter') . '/css/filter.caption.css';
  }
}

Second, there is Bartik's ckeditor-iframe.css, which overrides certain of CKEditor's iframe styling… more specifically, the one mentioned just above: filter_caption's CSS.

So: modules must dynamically add CSS (drupal_alter()) before the theme, because otherwise the theme is no longer capable of overriding the module CSS.

If you can think of cases where the opposite is necessary, then I think we will need two drupal_alter()'s: one before the theme CSS is added, and one after.


Yay! Nice to see the "CKEditor CSS files from the info file" feature being used.

:) Yes!

Wim Leers’s picture

The patch still applies and works fine. Can we please get a usability review to start moving this forward again?

Simplytest.me link to test it: http://simplytest.me/project/drupal/1cb65032844cb12907ed484e3c17ded1528ea44f?patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-CKE-UPDATE-2027181-11.patch&patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-2027181-13.patch


@quicksketch: I also replied to your review — please respond! :)

Bojhan’s picture

Issue tags: -Needs usability review

Looks good, the outline on focus looked a bit wierd here - but perhaps thats because the image is larger than the textarea.

I was a tad bit confused about the interaction to configure it, but Wim told me thats not part of this patch.

Lets get this in :)

webchick’s picture

Priority: Normal » Major

Adding images to posts is really awkward currently; raising priority of this to major to reflect its importance.

Haven't tried this yet, but it looks promising! :D

Bojhan’s picture

@webchick Could you add what is akward? This patch doesn't touch much of that experience.

webchick’s picture

Priority: Major » Normal

Oops. You are right. This just enhances the existing interface some. Sorry!

RE: Awkwardness, I find it very awkward to upload an image *below* the Body field, which where I want to actually *use* the image, and then have to copy its URL, then scroll back up and paste it into the image button dialog. But I suppose I can file a separate issue about that, if I don't find one that already exists. :)

Wim Leers’s picture

Issue tags: +JavaScript

For that part of the UX, please see #1932652: Add image uploading to WYSIWYGs through editor.module :) But I see you've already found it, webchick!

Can we move this forward again now? Usability review happened, this just needs code reviews to get to RTBC?

Wim Leers’s picture

(I wanted to reroll, but this is blocked on https://github.com/cksource/ckeditor-dev/tree/d8-imagecaption being rebased on the CKEditor 4.2 release, since Drupal 8 now ships with CKEditor 4.2. Hence this is briefly blocked on the CKEditor developers.)

Wim Leers’s picture

Status: Needs review » Needs work
FileSize
1.31 MB

While working on the update, I ran into a problem. Uploading a temporary patch so I can give the CKEditor developers access to a patched D8 on simplytest.me instance :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.31 MB

This is a straight reroll of #13, with an updated build of CKEditor (a specific branch that will become part of CKEditor 4.3, but is based off of CKEditor 4.2 that's already in Drupal core, so there won't be any regressions).

Because the number of changes to CKEditor is now much smaller (e.g. none of the language files are being changed), it now all fits in one patch.

Please review, so this patch can move forward!

(Based on Drupal 8 commit 6400bf8b21cb44338e2af94e25107003cdfeeeb7.)

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -sprint, -Spark, -CKEditor in core

The last submitted patch, ckeditor_widget_imagecaption-2027181-26.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Usability, +sprint, +Spark, +CKEditor in core
seutje’s picture

Status: Needs review » Needs work

I'm concerned about the upcasting system.
Try the following:

  1. type 2 letters
  2. turn them into a list-item
  3. insert an image in between the 2 letters, no caption or alignment, just a url
  4. switch to source and back
  5. edit image
  6. no more captioning or alignment options because it wasn't upcasted due to not being alone in its block-level wrapper

Also, I found the interface a bit confusing, I thought the alignment would have effect on the caption.

[edit: masking my poor grammar skills.]

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.05 KB
1.31 MB

#29:

Also, I found the interface a bit confusing, I thought the alignment would have effect on the caption.

I've swapped them: now the alignment settings come first. A small change that should subtly help. Also: this is a one-time learning experience, so I'm not too worried about it in general — otherwise Bojhan would have noticed it too :)

RE: bug: great, great catch! Worked around this by disallowing alignment and captions on inline images that are surrounded by text. That makes hardly any sense anyway: the main use case for inline images are smileys, and those should not be captioned. Aligning inline images completely does not make sense: why would you want the image to be in a specific location in the text, if you're still going to change the alignment anyway?

I recognize that this is suboptimal UX-wise, but overall this is still a very big step forward. Only when CKEditor's Widget system is finalized in version 4.3, we'll be able to refine this further, because they're still working on allowing widgets to work both for inline and block elements: http://dev.ckeditor.com/ticket/9764#comment:57.

So, please don't let perfect be the enemy of good!

seutje’s picture

FileSize
11.61 KB

Applying patch from #30 resulted in a partially built page with following markup:
http://pastebin.com/gmXL5WcE
error log in attachment.

seutje’s picture

Status: Needs review » Needs work

status

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
1.31 MB

Also because of #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity… Easy fix :)

This should do the trick.

Also: this is why I hate how PHP handles namespaces. If you do

use Haha/This/Does/Not/Exist/So/PHP/Will/Surely/Fail.php

Then PHP will continue just as if nothing is wrong… :(

seutje’s picture

Kinda figured it was cause I took too long to test it, mi scsi!

Gave up on PHP a while back...

Will try to give this a run later tonight (have to go have dinner with the folks, so no guarantees)

seutje’s picture

Status: Needs review » Needs work

Quick review, not much time. will try to review in further detail later tonight.

kinda annoying to have the vendor stuff in there :/

issue reported in #31 is indeed fixed

it's kind of weird though that until introducing either alignment or a caption, there is no margin applied, but there is after applying it. Due to it gaining a <figure>-wrapper, to which default margins are applied (by Chrome). I guess it's a limitation to the fact that we have to turn it into a block-level element, but perhaps we should remove those margins from default themes and the editor?

tiny nitpick:

+++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.jsundefined
@@ -62,13 +51,67 @@ CKEDITOR.plugins.add('drupalimage', {
+            existingValues.height = imageDOMElement ? imageDOMElement.height : '';
+            // Populate all other attributes by their specified attribute values.
+            var attribute = null;
+            for (var key = 0; key < imageDOMElement.attributes.length; key++) {

Can we cache the length? like for (var key = 0, len = foo.length; key < len; key++) so we don't have to re-query for it?

seutje’s picture

some more nitpicking:

+++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.jsundefined
@@ -0,0 +1,234 @@
+      if (command) {
+        if (value in { right:1,left:1,center:1 }) {

can we space out the object literal with a space after each comma and colon?

+++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.jsundefined
@@ -0,0 +1,234 @@
+
+            this.setState(
+              (align === value) ? CKEDITOR.TRISTATE_ON : (value in allowed) ? CKEDITOR.TRISTATE_OFF : CKEDITOR.TRISTATE_DISABLED);

this is a bit much, no?

+++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/theme.jsundefined
@@ -0,0 +1,211 @@
+          // The image is not wrapped in <figure>.
+          else if (this.element.is('img')) {
+            // The image is not wrapped in <figure>, but it should be.
+            if (this.data.hasCaption || this.data.data_align !== null) {
+              // Destroy this widget, so we can wrap the <img>.
+              editor.widgets.destroy(this);
+              // Replace the widget's element (the <img>) with the template (a
+              // <figure> wrapping an <img>) and then replace the the template's
+              // default <img> by our <img> so we won't lose attributes. We must
+              // do this manually because upcast() won't run.
+              var figure = CKEDITOR.dom.element.createFromHtml(this.template.output(), editor.document);
+              figure.replace(this.element);
+              this.element.replace(figure.findOne('img'));
+              // Reinitialize this widget with the current data.
+              editor.widgets.initOn(figure, 'drupalimagecaption', this.data);
+            }
+            else {
+              // Nothing remains to be done, this is already taken care of in
+              // originalDataFn().
+            }
+          }

since the else isn't really necessary, maybe it's useful to combine the else if and the if, reducing indentation?

also, as a side note, I'd like to point out that <img/> is interpreted by every browser as <img> (unless served as application/xhtml+xml), it's a void element and an end tag is actually forbidden according to html 4.01 and 5 spec. It is generally done for XHTML compliance, which is pretty silly in the whole 80/20 picture. But that is not a can of worms I want to open at this point, so feel free to ignore. :)

other than these small notes, I have no more concerns on the JS-side of things

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
1.31 MB

it's kind of weird though that until introducing either alignment or a caption, there is no margin applied, but there is after applying it. Due to it gaining a -wrapper, to which default margins are applied (by Chrome). I guess it's a limitation to the fact that we have to turn it into a block-level element, but perhaps we should remove those margins from default themes and the editor?

Oh! Excellent point! :) Thanks! Now fixed.

Can we cache the length?

Done.

kinda annoying to have the vendor stuff in there :/

Two separate patches also causes problems; core committers don't like that.

this is a bit much, no?

I agree, but this is copied verbatim from existing CKEditor plugins. It's not *that* bad. It'd be worse to split it over multiple lines.

since the else isn't really necessary

Indeed — removed!


With the JS review out of the way (thanks, seutje!), I think the most important remaining thing to be done for this patch is how to deal with the need to duplicate Bartik's CSS for captions in a ckeditor-iframe.css file for Bartik. I think the easiest way forward is to just duplicate it, because using @import is bad for performance, and a style.css file importing a ckeditor-iframe.css file also seems pretty bad.
This is essential for image captions to look decent on the node add/edit form out of the box. But the rest of Bartik's styling should also be approximated. #2064379: Remove ckeditor-iframe.css and load relevant Bartik CSS files for CKEditor's iframe mode. was created for that a few days ago.

With that, the last @todo is out of the way. Somebody can now RTBC this patch :)

jessebeach’s picture

Status: Needs review » Needs work
FileSize
255.74 KB

wow, CKEditor plugins/widgets are complex :) I reviewed the JS/PHP/CSS. It all looks great. I did some manual testing and ran into one error with in-place editing:

drupalimage plugin js error

STR

  1. Quick edit a node with a captioned/aligned image.
  2. Select the image
  3. Press the image button in the toolbar
  4. An error should be raised on line 119 of drupalimage/plugin.js

That's the only issue I found. I don't have time at the moment to look into it, but I'll have a peek later tonight.

Wim Leers’s picture

Status: Needs work » Needs review
jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

Ok, the bug I raised in #38 was the only blocker for me RTBCing this issue. Since we have an RTBC'ed fix for the bug in #2067427, then my blocking issue is addressed.

Setting to RTBC. I'm excited to see Drupal forms driving CKEditor configuration. Amazing work Wim Leers!!!

eaton’s picture

Seconding the excitement here. This patch also provides an extremely useful demonstration of the technique for other developers interested in building similar editing enhancements for Drupal 8.

Wim Leers’s picture

#41: Woot! Very happy to have you excited here, eaton :)

CKEditor Widgets will only get better, so I am very happy to see all this excitement!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
1.31 MB

The commit of #2029737: Create hook_help() for ckeditor module before this one caused this patch to no longer apply in a single hunk.

Status: Reviewed & tested by the community » Needs work
Issue tags: -JavaScript, -Usability, -sprint, -Spark, -CKEditor in core

The last submitted patch, ckeditor_widget_imagecaption-2027181-44.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Usability, +sprint, +Needs reroll, +Spark, +CKEditor in core
Wim Leers’s picture

Issue tags: -Needs reroll

d.o tag monster again — as well as testbot failing to retrieve the patch from d.o and hence marking this issue as NW instead of retrying. :(

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Patch is green — RTBC again as per #40 & #41.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Yet another reroll is required :(

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
1.31 MB

Run tests, run!

(Changing array('system', 'drupal.debounce'), to array('system', 'drupalSettings'), did the trick! I wish patches were smarter :))

webchick’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies. :( Here are a few Qs in the meantime.

+++ b/core/assets/vendor/ckeditor/CHANGES.md
@@ -1,6 +1,10 @@
+## CKEditor 4.3
 ...

+++ b/core/modules/ckeditor/ckeditor.module
@@ -107,9 +109,19 @@ function ckeditor_library_info() {
+    'version' => '4.3-dev — d8-imagecaption branch commit 887d81ac1824008b690e439a1b29eb4f13b51212',

Most of the patch is this. Normally we would do this in a separate issue, but we're actually updating CKEditor to a "dev" release equivalent, not a full release. I checked with nod_ and he said he was ok with this approach.

+++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
@@ -0,0 +1,234 @@
+        // Build the HTML for the widget.

+++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/theme.js
@@ -0,0 +1,207 @@
+      widgetDefinition.template =
+        '<figure class="caption caption-img">' +
+          '<img src="" data-caption="" data-align="center" />' +
+          '<figcaption></figcaption>' +

Does this need to run through the Drupal theme system somehow?

+++ b/core/modules/editor/lib/Drupal/editor/Form/EditorImageDialog.php
@@ -127,6 +127,48 @@ public function buildForm(array $form, array &$form_state, FilterFormat $filter_
+          'none' => t('None'),
+          'left' => t('Left'),
+          'center' => t('Center'),
+          'right' => t('Right'),

AFAIK these should now be $this->t().

+++ b/core/themes/bartik/bartik.info.yml
--- /dev/null
+++ b/core/themes/bartik/css/ckeditor-iframe.css

Hm. Interesting. How does Aloha Editor in contrib get Bartik to provide support for it?

eaton’s picture

Does this [widget code that turns things into a figcaption] need to run through the Drupal theme system somehow?

I'd say it's probably overkill. It only applies to the representation of the captioned item inside the CKEditor edit window, not and that's functionality specific to the editing plugin. Overriding that would change the behavior of the plugin, and would have to be accompanied by CSS and JS refactoring equivalent to writing a different plugin... which people can do. :D

Hm. Interesting. How does Aloha Editor in contrib get Bartik to provide support for it?

Couldn't it provide its own Bartik-specific stylesheet? The idea that contrib modules should be "first class citizens" is an important principle, but having CSS in a core theme to cover new core modules and capabilities sees perfectly fine. As long as the file is genuinely Bartik-specific, it's equivalent to adding a set of CSS tweaks to cover a new Views plugin style that ships with core.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.51 KB
1.31 MB

#51:

Does this need to run through the Drupal theme system somehow?
It's a valid question. It's been asked before. It's been answered before (in early July). eaton already answered it, but here is the full version:
The caption filter runs through the theme layer, but this template seems like it's hard-coded. It seems like we should at least use a CKEditor settings variable for creating our template, making it possible to match any overrides to the theming of caption filter.

First: note that it's in drupalimagecaption/theme.js. The base logic lives at drupalimagecaption/plugin.js. If any theme wants to override the HTML structure, and they want this to be accurately reflected in CKEditor too, then they must override drupalimagecaption/theme.js as well.

Second: I know that this is less than ideal, and I would have loved to simply have a Drupal.theme.filter_caption function, but the CKEditor developers assured me that this is not technically possible. We've had long discussions about this. If necessary, I'm sure they won't mind explaining that here on the issue.

Third: a factor that should have some weight is the fact that the default HTML structure used by filter-caption.html.twig is using the HTML5 best practices. It should be sufficiently flexible to simply override the CSS to suit >95% of all use cases. Which means only experts would need to override the JS.

AFAIK these should now be $this->t().
Yes, that's the latest new thing. Fixed. Migrated EditorImageDialog to using FormBase. This also allowed me to remove the empty validateForm() method, which FormBase already does for us.
Hm. Interesting. How does Aloha Editor in contrib get Bartik to provide support for it?
eaton answered it nicely :)

Besides webchick's apt remarks, quite a bit else has changed in the mean time in Drupal 8 HEAD, forcing further changes upon this patch:

  1. filter_format_load() is gone, I converted it to use entity_load() instead.
  2. #1932652: Add image uploading to WYSIWYGs through editor.module landed; this required the data-editor-file-uuid attribute to also be handled in the code being added here.
  3. That same issue has also introduced an AJAXy form element: the managed_file element to upload images; the AJAX submit that occurs when a user presses the "Upload" button that comes with that form element caused havoc. Fixed now. (Rather than using $form_state['input']['editor_object'] directly, which gets messed up by AJAXy form elements, we now store that information explicitly in $form_state['image_element'], just like elsewhere in Drupal core.)
  4. The use of data-cke-saved-*, which was introduced in #2071957: Cannot change existing images or links through Text Editor's image/link dialogs, should also be applied here.

#52: Thanks for the feedback! :)

jessebeach’s picture

The diff in #53 looks good. I manually tested the feature and found that a caption and alignment are correctly applied for a Full HTML format. While testing the Basic HTML format, I found that while I'm able to add an image with a caption, that caption is not preserved when the node is created.

Screenshot of a node page. The image in the node should have a caption underneath, but that caption is missing

If I edit the new node, re-add a caption and save it (without changing the format value from Basic HTML), the caption is preserved and presented on the node view page.

Screenshot of a node page after editing the existing node and re-adding the caption. The image caption is now present.

This was the only issue I found while manually testing. Let's get this resolved and I'll retest.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
1.31 MB

Great find, Jesse! Fixed now.

Status: Needs review » Needs work

The last submitted patch, ckeditor_widget_imagecaption-2027181-55.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
664 bytes
1.32 MB

Status: Needs review » Needs work

The last submitted patch, ckeditor_widget_imagecaption-2027181-57.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.31 MB

Ugh. The many many rerolls that I'm doing caused me to contaminate this patch with hunks from another :( Removed the CommentNew* hunks.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

The issue I found in #54 has been resolved. This is an amazing feature that folks have been clamoring for for years. I've pounded on with manual testing and it's held up. The code has been through numerous rounds of review and refinement. Let's get it in!

webchick’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
133.17 KB
133.45 KB

I'm really sorry to "needs work" this patch after all the extensive work and reviews, but in trying this out...

I really think that the "Caption" checkbox needs to invoke the States API and show a textfield for the caption to use as a default:

Show textfield below checkbox if it's checked

Because otherwise the UI for entering your caption is not remotely obvious at all, particularly on a large image:

Caption field missing..?

But even in a best-case scenario, it doesn't make sense to use an in-place edit UI as a data entry UI.

Secondly, when I erased my old image and inserted a new one, now I get "Inline images cannot be aligned." and "Inline images cannot be captioned." What gives? Why can't I do that to inline images? And if that's true, why didn't I get that the first time?

I'm in Firefox atm, will test it in Chrome monentarily to see if that works any better.

webchick’s picture

K, Chrome does indeed work much better. Guess this is "needs work" for Firefox, too. :\

Bojhan’s picture

@webchick This is a little confusing, the whole point of in-place edit is that you can edit everything. I don't think users will understand the data entry UI vs. in-place edit UI concept at all. If we start spitting it out, and some text can only be edited in a config modal like you are proposing, then you are only introducing more inconsistencies in the experience.

There probably should be a placeholder area, with a "placeholder" text to enter a caption there - that sounds more natural to me, than having it in a modal dialog. Even without this placeholder text, I didn't find it particularly confusing - but perhaps thats also because I test on chrome - where you get an empty bar below it.

webchick’s picture

Oooh. Placeholder in the in-place editing UI could work, yeah. It's just that until you get used to what you're looking at and how it works, it's not really obvious that...

Blank rectangle under image is non-obvious

If there was text there saying "Edit me" or whatever, then that concern would go away.

Bojhan’s picture

@webchick Yhea, you are right. You need to know that you can type there, I was already in a frame of mind when seeing that box. Lets add the placeholder :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
1.31 MB
1.68 MB

To summarize #61#65:

  1. There appear to be CSS/JS issues in Firefox.
  2. There should be placeholder text where the user can enter caption text, because that might otherwise not be clear.
  3. In-place editing vs. Dialog for caption entry.
  4. "Inline images cannot be aligned/captioned."

Answers:

  1. I cannot reproduce this. It works just fine. Tested with Firefox 21 and Firefox 23.
    The second screenshot in #61 does not show the styling that would be applied if you had checked the "caption" checkbox in the dialog. So I think you just forgot to check it? :) Without checking that, you indeed won't be able to enter a caption.
  2. Added! I agree with you and Bojhan that this removes the "WTF now?" moment. Good call.
  3. Besides what Bojhan already said in #63: the caption allows formatting. I.e. HTML tags. So then you'd get a input[type=text] with HTML tags in there. Plus, two ways for editing the caption. Plus, more cognitive load in the dialog. Plus, then we might almost as well just not use CKEditor Widgets' nice UX at all.
  4. See #29/#30. The problem is that images can be both inline and block elements. When using e.g. a smiley image, you want that to be inline, i.e. flowing with the text. When using e.g. a photo, you want that to be a block, i.e. in between two paragraphs (which are block elements too). It does not make sense to caption inline images (smileys). And transforming an image from being inline to block level or vice versa is a nontrivial task. Hence we just say that we don't support that — at least for now. It's the edge case. If you press enter first (i.e. start a new empty paragraph) and then insert an image, you'll see it works as expected again.
    Google Docs does this similarly: when the cursor is within a paragraph (anywhere, including at the end), then the image will be inserted inline. Start a new paragraph first, then insert an image, and the image will be a block-level image.

In the attached reroll, I added a placeholder caption. No new JS logic, just one additional CSS rule! Thanks to Piotrek Koszuliński from CKEditor for the tip.

Attached is also a screencast of this placeholder feature in both Chrome and Firefox. When you delete your caption in Firefox, the placeholder reappears, as it should. In Chrome, it does not, because Chrome's contentEditable implementation adds a <br> for an unknown reason, hence the CSS selector does not apply. But the most important thing is of course that the placeholder is displayed initially, without adding any more complex JS logic.

Bojhan’s picture

The only outstanding issue I'd see is that it isn't visually clear it is to be replaced. Perhaps making it italic, and adding ellipses helps with this. We can't make it more gray, because that would probably contrast to low.

Wim Leers’s picture

I discussed this with Bojhan; we're not adding an ellipsis because that would 1) look weird IMHO, 2) be inconsistent with the placeholder at e.g. admin/modules.

Attached patch makes the placeholder text italic.

Wim Leers’s picture

D'oh, the patch in #68 *does* include the ellipsis, even though the interdiff indicates it does not. I forgot to update the patch after removing it, but I did update the interdiff. Sorry for the noise :(

webchick’s picture

The bug in #61 was from using an exceptionally LARGE image + Firefox. I'll try it again with the latest patch sometime today, if no one beats me to it.

jessebeach’s picture

Tested in Firefox with an exceptionally large issue.

Screenshot of an inlined image in a CKEditor. The image is extremely large, causing a scroll bar to appear

The issues in #66 have been addressed.

I can't find any more issues with this patch.

My vote is RTBC. I'd like Bojhan to have a chance to review, so I'm leaving this as Needs Review.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed it, earlier (me and WimLeers tend to chat on IRC about these kind of issues) I was waiting for Angie to chime in. But this looks good marking it back to RTBC.

jessebeach’s picture

Angie will bang it with her war hammer Bug Shine, the revealer of truth and instability.

webchick’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
FileSize
16.99 KB
231.71 KB
137.89 KB

So! Close!

Here's what happened this time. Again, testing in Firefox since I know you guys use Chrome. ;)

#1: The cursor is weird:

Cursor aligned to the left, even though placeholder text is in the center.

OTOH, on a wide image, the cursor being on the left is the only way I know that clicking into the region did anything:

Cursor on left, placeholder text off-screen.

However, I actually think either way, we want to center the cursor so it appears over the placeholder text. Chances are that's what I would click on when I'm ready to edit, even if I have to scroll to the right to do so.

Then, because #2076847: FilterHtmlImageSecure incorrectly flags images coming from site URLs with ports in them as insecure is still an issue, I remembered I needed to switch my text format from "Basic HTML" to "Full HTML." I was being a jerk and entering HTML like <script>alert('foo');</script> into the caption to see what would happen (nothing bad) but when changing the text format, it turned into:

Angle brackets get double-escaped in caption output.

This is also the case even if I just post the node and then go back to edit it. So something is not quite preserving the previous contents; seems to be double-check_plain()ing. Not sure if that's introduced in this issue or not, but raising it nonetheless.

Also, escalating this issue to "major" to indicate its importance. This actually exercises the CKEditor Widget API to ensure it can meet our needs, and provides a hook-in point for contrib to do more stuff cool UX like this.

jessebeach’s picture

I don't believe there's anything we can do to center the cursor short of inserting a non-breaking space on focus, then removing that nbsp on the first key down. This is a browser content editable node. The behavior is the same in Firefox and Chrome.

webchick’s picture

Oooh, one other thing I found while testing #1741498: Add a responsive preview bar to Drupal core. Captions seem to break responsive image handling.

Large image w/ no caption:

Image is correctly shrunk down to fit screen size.

Large image w/ caption:

Image causes scrollbar.

eaton’s picture

Oooh, one other thing I found while testing #1741498: Add a responsive preview bar to Drupal core. Captions seem to break responsive image handling.

That would be an excellent followup issue, IMO. The basic approach taken by the module -- figure, figcaption, and image -- should work just as well as the existing setup from a markup perspective. It LOOKS like this doesn't break "responsive image handling" so much as "the current responsive image handling does not properly handle images inside of figure tags."

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
97.13 KB

To summarize #74#77:

  1. Cursor is weird.
  2. HTML tags used in the caption are messed up when switching text formats.
  3. Image with caption is "not responsive anymore".

Answers:

  1. As Jesse pointed out: that's a browser bug, nothing we can do about it, except for reporting it.
  2. Great find! I tested this in detail, two conclusions:
    1. HTML formatting (e.g. applying <em>) is not lost.
    2. Using brackets (< and >) and ampersand does indeed result in them being HTML encoded.

    So it's not as bad as I thought it was after reading the comments :), but it's still definitely not good. I talked to the CKEditor developers, and they said this will be fixed in the final version of the Widget API, which will be part of CKEditor 4.3. First we will get a CKEditor 4.3 beta, which will help finalize the API. This beta is coming in the next few weeks.
    I think the best conclusion is to commit this as-is, we will then get the improved/fixed/finalized Widgets API in the near future. That will fix this problem automatically: the custom plugin being added here does not do anything related to HTML encoding, it's a bug in the CKEditor Widgets API.
    We already have an issue to update to the CKEditor 4.3 beta: #2039163: Update CKEditor library to 4.4.

  3. As eaton pointed out: that's very much out of scope for this issue. Nor does it break anything that was intentionally crafted/created/customized/styled, it just appears to be breaking default browser behavior. However, if I look at it in Chrome's simulation, it looks just fine (I didn't want to apply both patches and run into potential conflicts). See the attached screenshot. So I'm actually not sure it even "breaks" anything at all.

Overall conclusion: 1 is a non-issue, 2 is an upstream bug that will be fixed for us, 3 is at least out of scope, and possibly not broken. Hence back to RTBC.

P.S.: I did test in Firefox, as you can see in the screencast included in #66. In fact, I also tested that before, and it's always worked just fine!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, thanks for looking into my feedback.

Too bad about 1. :(

2. Sounds good.

3 is definitely broken. Just try resizing your browser window and you'll see the problem (at least in Firefox). However, I can concede this is probably an issue w/ Picture module, and not one with this patch.

Just banged this around again and showed it to Dries as well, and he's also +1. So!

COMMITTED AND PUSHED TO 8.X. YEAAAAHHHH!! :D

Thanks all so much for your work on this!!

webchick’s picture

Issue tags: +Needs followup

Oh, but let's get a follow-up for that Picture module stuff.

webchick’s picture

Oh btw. This patch is so insanely massive because it updates CKEditor to a "dev" release. I checked w/ nod_ and he was cool with this approach. Updating to a proper release of CKEditor will be done in #2039163: Update CKEditor library to 4.4.

eaton’s picture

And the Ewoks danced.

Wim Leers’s picture

Issue tags: -sprint
FileSize
162.64 KB

#79: Picture module is not at all in the picture (heh…) here, it only works for image fields that use the Picture field formatter. This is just plain standard browser behavior. Also note that desktop browsers do *not* always handle images in the same way as mobile browsers!

I can reproduce the problem you're seeing on Firefox, but not on Chrome. I also can't reproduce it in the iPhone Simulator (i.e. Mobile Safari). Nor in desktop Safari. So I'm pretty sure this behavior is limited to Firefox on desktops/laptops. Which makes sense, really: on a desktop you usually don't want content to shrink, you just scroll horizontally. Chrome/Safari/Mobile Safari seem to apply the same logic they apply on mobile devices, and continue to show the image.

Not in any way is anything responsive involved: not picture.module, not CSS with media queries, nothing!


#82: the llamas dancing on my desk join your Ewoks!
llamas-dancing.jpg


I think it might be worthwhile to create a combined change notice for #2014895: Image captions & alignment (for inline images) and this issue, no?

Automatically closed -- issue fixed for 2 weeks with no activity.