This is the follow-up issue for #2014895: Image captions & alignment. 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.pngimage-caption-widget.png

Files: 
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
PASSED: [[SimpleTest]]: [MySQL] 58,667 pass(es).
[ View ]
#68 interdiff.txt467 bytesWim Leers
#68 ckeditor_widget_imagecaption-2027181-68.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,050 pass(es).
[ View ]
#66 image_caption_widget_placeholder.m4v.zip1.68 MBWim Leers
#66 ckeditor_widget_imagecaption-2027181-66.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,206 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 58,812 pass(es).
[ View ]
#57 ckeditor_widget_imagecaption-2027181-57.patch1.32 MBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 57,505 pass(es), 594 fail(s), and 255 exception(s).
[ View ]
#57 interdiff.txt664 bytesWim Leers
#55 ckeditor_widget_imagecaption-2027181-55.patch1.31 MBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,551 pass(es), 1 fail(s), and 90 exception(s).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 58,626 pass(es).
[ View ]
#53 interdiff.txt15.51 KBWim Leers
#50 ckeditor_widget_imagecaption-2027181-50.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,730 pass(es).
[ View ]
#44 ckeditor_widget_imagecaption-2027181-44.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,216 pass(es).
[ View ]
#38 drupalimage plugin js error255.74 KBjessebeach
#37 ckeditor_widget_imagecaption-2027181-37.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 57,922 pass(es).
[ View ]
#37 interdiff.txt3.9 KBWim Leers
#33 ckeditor_widget_imagecaption-2027181-33.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,114 pass(es).
[ View ]
#33 interdiff.txt1.33 KBWim Leers
#31 drupal-error-log.txt11.61 KBseutje
#30 ckeditor_widget_imagecaption-2027181-30.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 57,940 pass(es).
[ View ]
#30 interdiff.txt9.05 KBWim Leers
#26 ckeditor_widget_imagecaption-2027181-26.patch1.31 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,216 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 56,927 pass(es).
[ View ]
#13 interdiff.txt2.11 KBWim Leers
#11 ckeditor_widget_imagecaption-11.patch35.95 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 56,449 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#11 interdiff.txt10.54 KBWim Leers
#11 ckeditor_widget_imagecaption-CKE-UPDATE-2027181-11.patch2.64 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 56,497 pass(es).
[ View ]
#8 ckeditor_widget_imagecaption-2027181-8.patch35.42 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ckeditor_widget_imagecaption-2027181-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 ckeditor_widget_imagecaption-CKE-UPDATE-2027181-8.patch2.64 MBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 57,983 pass(es).
[ View ]
#7 ckeditor_widget_imagecaption-2027181-7.patch30.72 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ckeditor_widget_imagecaption-2027181-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 ckeditor_widget_imagecaption-2027181-1.patch24.82 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ckeditor_widget_imagecaption-2027181-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 ckeditor_widget_imagecaption-CKE-UPDATE-2027181-1.patch2.64 MBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,393 pass(es), 2 fail(s), and 5 exception(s).
[ View ]
image-caption-widget.png250.13 KBWim Leers
image-caption-dialog.png21.13 KBWim Leers

Comments

Status:Active» Needs review
Issue tags:+Needs usability review, +Usability
StatusFileSize
new2.64 MB
FAILED: [[SimpleTest]]: [MySQL] 58,393 pass(es), 2 fail(s), and 5 exception(s).
[ View ]
new24.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ckeditor_widget_imagecaption-2027181-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

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

Infuriating.

StatusFileSize
new30.72 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ckeditor_widget_imagecaption-2027181-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll of #1: I forgot one crucial file.

Use the same CKE UPDATE patch.

Status:Needs work» Needs review
StatusFileSize
new2.64 MB
PASSED: [[SimpleTest]]: [MySQL] 57,983 pass(es).
[ View ]
new35.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ckeditor_widget_imagecaption-2027181-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Category:feature» task

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

Status:Needs work» Needs review
StatusFileSize
new2.64 MB
PASSED: [[SimpleTest]]: [MySQL] 56,497 pass(es).
[ View ]
new10.54 KB
new35.95 KB
FAILED: [[SimpleTest]]: [MySQL] 56,449 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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!

How do I get the caption?

StatusFileSize
new2.11 KB
new38 KB
PASSED: [[SimpleTest]]: [MySQL] 56,927 pass(es).
[ View ]

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


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

StatusFileSize
new2.38 MB

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

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.

StatusFileSize
new88.23 KB

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

[…] 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!

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

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

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

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

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

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?

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

Status:Needs review» Needs work
StatusFileSize
new1.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 :)

Status:Needs work» Needs review
StatusFileSize
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 58,216 pass(es).
[ View ]

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.

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

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

Status:Needs work» Needs review
StatusFileSize
new9.05 KB
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 57,940 pass(es).
[ View ]

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

StatusFileSize
new11.61 KB

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

Status:Needs review» Needs work

status

Status:Needs work» Needs review
StatusFileSize
new1.33 KB
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 58,114 pass(es).
[ View ]

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… :(

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)

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?

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

Status:Needs work» Needs review
StatusFileSize
new3.9 KB
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 57,922 pass(es).
[ View ]

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: Bartik should provide a stylesheet 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 :)

Status:Needs review» Needs work
StatusFileSize
new255.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.

Status:Needs work» Needs review

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

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.

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

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

Patch no longer applies

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 58,216 pass(es).
[ View ]

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.

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

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

Status:Needs review» Reviewed & tested by the community

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

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

Yet another reroll is required :(

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 58,730 pass(es).
[ View ]

Run tests, run!

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

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?

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.

Status:Needs work» Needs review
StatusFileSize
new15.51 KB
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 58,626 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.58 KB
new1.31 MB
FAILED: [[SimpleTest]]: [MySQL] 58,551 pass(es), 1 fail(s), and 90 exception(s).
[ View ]

Great find, Jesse! Fixed now.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new664 bytes
new1.32 MB
FAILED: [[SimpleTest]]: [MySQL] 57,505 pass(es), 594 fail(s), and 255 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 58,812 pass(es).
[ View ]

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

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!

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new133.17 KB
new133.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.

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

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 59,206 pass(es).
[ View ]
new1.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.

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.

StatusFileSize
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 59,050 pass(es).
[ View ]
new467 bytes

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.

StatusFileSize
new1.31 MB
PASSED: [[SimpleTest]]: [MySQL] 58,667 pass(es).
[ View ]

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

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.

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.

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.

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

Priority:Normal» Major
Status:Reviewed & tested by the community» Needs work
StatusFileSize
new16.99 KB
new231.71 KB
new137.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.

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.

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.

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new97.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!

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

Issue tags:+Needs followup

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

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.

And the Ewoks danced.

Issue tags:-sprint
StatusFileSize
new162.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 and this issue, no?

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