Image captions & alignment (for inline images)

Just wondering, will I be able to configure it from the UI?

I am really looking forward to this. :)

In terms of markup output, The HTML5 version in the summary is good, here barely modified:

<figure [classes]>
   <img src="cat.jpg" alt="Alternative text" />
   <figcaption>Caption text</figcaption>
</figure>

- Just checking: if there’s no caption, the output should just be the img, right?
- I would rather not add a class by default (i.e. if no user styles)
- It would be nice to make the alignment classes reusable for all objects embedded in the body (and LTR/RTL agnostic). Something like .embed-align-start, .embed-align-center, .embed-align-end.

CSS review:

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+  border: 1px solid #CCC;
+  padding: 4px;
+  background: #F3F3F3;
+  font-size: 0.857em; /* assuming you have a base font size of 14px, this is 12px */

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+div.caption img,
+div.caption object {
+  margin-bottom: 5px;

These styles should be removed; they are best left to the theme.

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+div.caption-left {

As a rule, don’t qualify selectors (it adds unnecessary specificity), e.g. div.class should be just .class.

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+  margin: 10px 10px 10px 0;

This should be specified in typographic units. I suggest setting only the inside and bottom margins, say 1.4em, which matches the average line-height for body copy. Would need manual testing to tell if this looks good:

.embed-align-start {
  float: left; /* LTR */
  margin-right: 1.4em;
  margin-bottom: 1.4em;
}

@WimLeers the caption. Can I do it inline? Or do I need to go to source to add it?

Oops, sorry for the NW.

Reviewing mostly the PHP. There does not seem to be any tests, but we need those. Also some interesting remarks:

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+/** aligned captions **/

Uppercase? Not sure what is the standard for inline comments in CSS like this.

+++ b/core/modules/filter/filter.moduleundefined
@@ -1425,21 +1433,53 @@ function theme_filter_html_image_secure_image(&$variables) {
+ *   An associative array containing:
+ *   - image: The complete image tag whose image is being captioned.
+ *   - caption: The caption text, or NULL.
+ *   - align: The alignment: 'left', 'center', 'right' or NULL.
+ *   - width: The width of the image.

It would be great to document the type of these. All seem to be string or NULL, but eg. width includes measurement units too for example.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterCaption.phpundefined
@@ -0,0 +1,137 @@
+    // Prevent useless processing if there are no data-caption attributes at all.
+    if (stristr($text, 'data-caption') !== FALSE || stristr($text, 'data-align') !== FALSE) {

Comment not accurate vs. the code. Not sure this comment is needed at all btw.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterCaption.phpundefined
@@ -0,0 +1,137 @@
+          // Retrieve, then remove the data-caption and data-align attributes.
+          if ($image_node->hasAttribute('data-caption')) {
+            $caption = $image_node->getAttribute('data-caption');
+            $image_node->removeAttribute('data-caption');
+          }
+          if ($image_node->hasAttribute('data-align')) {
+            $align = $image_node->getAttribute('data-align');
+            $image_node->removeAttribute('data-align');
+          }
+
+          // Given the updated image node, caption, alignment and width: re-render
+          // the image with a caption.
+          $altered_image_html = theme('filter_caption', array(
+            'image'   => $image_node->C14N(),
+            'caption' => $caption,
+            'align'   => $align,
+            'width'   => $this->getImageWidth($image_node),
+          ));
+

Between getting the caption and the alignment out of the markup towards rendering it, there is no checking of possible / allowed values. I'm not sure I could reproduce an XSS attack through these, but there are at least the possibility to add arbitrary class names via the align value.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterCaption.phpundefined
@@ -0,0 +1,137 @@
+          // Load the new HTML into a new DOMDocument.
+          $dom2 = filter_dom_load($altered_image_html);
+
+          // Locate the snippet of HTML we're interested in.
+          $dom2_image_node = $dom2->getElementsByTagName('body')->item(0)
+                                  ->childNodes->item(0);
+          // Import the new "image" node from the second DOMDocument into the main
+          // one, importing also the child nodes of the new "image" node.
+          $new_image_node = $dom->importNode($dom2_image_node, TRUE);
+          // Finally, replace the original image node with the new image node!
+          $image_node->parentNode->replaceChild($new_image_node, $image_node);

Is this a performant way to do the text replacement? Why not str_replace? :) We don't care because its cached anyway?

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterCaption.phpundefined
@@ -0,0 +1,137 @@
+      if ($image_node->hasAttribute('src')) {
+        list($width) = getimagesize($image_node->getAttribute('src'));

This sounds like could be pretty darn slow for remote images. Also, this may throw warnings level errors ("If accessing the filename image is impossible getimagesize() will generate an error of level E_WARNING.") from http://php.net/getimagesize - should we have a consideration for that.

This also seems to be an interesting opportunity to get the server do HTTP requests to an arbitrary URL (additionally to the client browser doing it). This in itself may not be an issue, but it is intriguing.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterCaption.phpundefined
@@ -0,0 +1,137 @@
+          <li>Algin an image: <code>&lt;img src="" data-align="center" /&gt;

Algin

@Wim asked me in IRC to provide feedback on the 4 points currently in the issue summary as remaining tasks.

1. Implemented as filter?. Based on how heavy the operation seems to be, I don't see what else it would be in our architecture. It needs to be cached, participate in the input format system, etc.

2. Part of filter.module? Where else would it be :) It does not depend on image fields or anything, it merely works with img tags in HTML; so it could be its own filter_caption module, but that sounds very excessive. It is a "basic" HTML transformer from a generic HTML format to a rendered version, so sounds like filter module is the best home for it in core.

3. Target HTML? I don't have any strong opinions on this. :)

4. Filter enabled by default for Basic HTML and Full HTML in Standard profile? If images are otherwise allowed in a default input format, I think captioned images should be too. This is just a small extension to the existing image markup support. Are we upgrade concerned? Is ckeditor even enabled on the upgrade? Should existing format setups be upgraded? Should we print a message to users instead in the upgrade?

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+div.caption-center {
+  display: block;
+  text-align: center;
+}

I'm not sure if this makes sense. The behaviour here is completely different fro m a float. Text will wrap around left/right aligned images, with this text will wont.

Here's an example of behaviour that would be consistent, but a PITA to implement: http://css-tricks.com/float-center/

In this patch, I went through ry5n's comments in #6 and tried to adhere to them as much as possible. Here are some deviations.

- It would be nice to make the alignment classes reusable for all objects embedded in the body (and LTR/RTL agnostic). Something like .embed-align-start, .embed-align-center, .embed-align-end.

Although I agree with the use of start and end vs. left and right, we use left to denote 'start' in LTR languages and left to denote 'end' in RTL languages consistently throughout Drupal right now. Although it's wrong, it's consistent, so I didn't want to introduce an inconsistency in this patch. Therefore, I just left the classes with their left/right naming. I did add an RTL stylesheet.

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+  border: 1px solid #CCC;
+  padding: 4px;
+  background: #F3F3F3;
+  font-size: 0.857em; /* assuming you have a base font size of 14px, this is 12px */
+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+div.caption img,
+div.caption object {
+  margin-bottom: 5px;
These styles should be removed; they are best left to the theme.

I moved these styles to the Bartik theme.

As a rule, don’t qualify selectors (it adds unnecessary specificity), e.g. div.class should be just .class.

I removed the tag qualifications from the classes in the stylesheet.

From LewisNyman's comment in #12

+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,42 @@
+div.caption-center {
+  display: block;
+  text-align: center;
+}
I'm not sure if this makes sense. The behaviour here is completely different fro m a float. Text will wrap around left/right aligned images, with this text will wont.

The reason that the centered caption look different from the left/right align captioned had to do with margin collapse. Floating an element eliminates margin collapse, so it looked like the left/right aligned captions sat farther below the content above them. I've fixed this so that the centered caption also eliminates margin collapse. Now all alignment options position themselves with the same top margin.

So my big question is, does this functionality 100% have to be limited to images? And if not, is it something that could be fixed post July 1st, or have to be fixed prior? My concern is that Media or other contrib modules have to duplicate lots of code to support this same functionality for Videos and Audio files, or all the other types of 'embeddable content'.

This all looks pretty great Wim! Thanks for taking this on and making the filter a stand-alone issue. I think that was a smart way of getting this issue started.

+function theme_filter_caption($variables) {
+  $image   = $variables['image'];
+  $caption = $variables['caption'];
+  $align   = $variables['align'];
+  return '<figure class="caption' . (($align === NULL) ? '' : ' caption-' . $align) . '">' .
+         $image .
+         (($caption === NULL) ? '' : '<figcaption>' . $caption . '</figcaption>') .
+         '</figure>';
+}

I don't think we're adding new theme_* functions to core at this point. This should probably be converted into a Twig template.

After quite a bit of searching, I found what seems a conclusive solution to the combination of the two above problems: no image width parsing and cleaner markup. It involves display: table and table-caption :) See http://stackoverflow.com/a/13363408.

Well I'm impressed. The previous attempts at doing captions that floated left/right of course supported older browsers. Without the need to accomodate older IE browsers this looks like a good approach.

+          // Retrieve, then remove the data-caption and data-align attributes.
+          if ($image_node->hasAttribute('data-caption')) {
+            $caption = String::checkPlain($image_node->getAttribute('data-caption'));
+            $image_node->removeAttribute('data-caption');
+          }

We need to make sure that nested HTML is supported within the data-caption attribute. It will be very common that users will want/need to either insert a link within their caption or simply bold/italicize some text. We need to make sure that we're properly decoding HTML attributes into their HTML equivalents and then (yikes!) making sure that decoded HTML is allowed by other filters. I presume we'd rerun check_markup() *within the filter* to make sure the caption itself is allowed under the same rules as the rest of the content. Although unlikely, we'd want a catch in our filter to prevent recursive handling.

Finally: this indeed then requires filtering to be applied to the contents of a caption as well. Which means… that you might want to apply different restrictions, i.e. a site may want to disallow any tag besides the and tags in the caption. So, if you want this, then we'd need a "Limit allowed HTML tags within the caption" setting. That brings us beyond the KISS use case.

If we believe that is really essential, then I'll do it, but I'm not convinced yet that it is.

I think we can KISS still really easily here. Setting different HTML tags in the caption separate from the rest of the editor would definitely make things messy, but it should just be the same tag list again.

I think we can look to our competitors to see how important this is. I personally was tripped up by the lack of HTML in captions in Wordpress, until 3.4 "Green" was released last year. It was a constant source of trouble for users, leading the feature to be assigned the priority level of "highest omg bbq". So in my opinion, I think we should consider it to be required functionality.

It's against the HTML spec. You could indeed work around it by encoding as <em>.

Yep, that's what we'd need to do. I believe now that we're using underscore, it's escape method will do the job here.

The center alignment can wind up with a mis-alignment between the right edge of the content and the figcaption, like so.

Adding box-sizing: border-box to the child elements of the .caption fixes this.

I tested with image content, pre content, video content. It works really nicely.

I don't understand what's being debated in #25. Everything seems to work well enough to allow me to create semantic captions with little effort. Do we need to make this more complicated?

Wordpress used a non-HTML-based syntax, so it's not 1:1.

That's true. You might take a look at Confluence, which uses a data-caption attribute and allows nested HTML tags in its captions, so it's almost exactly the same thing as what we're wanting to do. Although there's nothing very surprising, they merely entity-encode the HTML that is saved into the caption. We'll need to entity-encode the string already on the JS side just to support quotes in the data attribute, so encoding brackets and other special characters isn't adding any work for us as far as encoding goes.

If we would allow any HTML tag, then CKEditor will also allow any HTML tag, rather than just allowing sane HTML tags. The only way we can sanely implement this IMO is by also allowing the user to restrict which HTML tags may be used inside of the caption. It'd be simpler not too, but it'd also allow for too much craziness. So: not check_markup() (which applies a text format), but filter_xss() (which only allows given tags and prevents XSSes).

I'm not sure if this is worth it. Does CKEditor's widget support allow you to specifically state which tags are allowed within a single widget's editable area? The UI complications of disabling buttons conditionally if you're inside a caption seems like it might get messy unless there is support for that built-into the widget system. Even if we do nothing though, an img tag inside of the caption should be prevented by the UI by the widget system itself, since if you attempt to insert an image into the caption it will open the dialog for editing the caption's image, rather than inserting another image. So at least in the UI you will be prevented from caption-within-a-caption problems.

No matter how you put it, this equates to recursive filtering. Which is kinda crazy.

Yep, but I don't think this is as complicated as it seems. On the filter side we just need to introduce a static variable that prevents the filter from getting into a recursive loop. On the CKEditor side, my hope for the initial round is just that we prevent nested-captions (which should be done for us already). Other buttons which may not make sense, e.g. tables(?) would be unrestricted even though they'd be nonsensical or even non-functional.

I'd just like to pop into this thread and offer some quick feedback:

YES! LIKE THIS! THIS IS HOW JESUS WANTS MARKUP EDITING TO WORK.

Yep, but I don't think this is as complicated as it seems. On the filter side we just need to introduce a static variable that prevents the filter from getting into a recursive loop

This happened on Gardens once. We had someone put a token reference to an image inside that same image's rich text area description field. OOM.

I'm using Xss::filter() (i.e. filter_xss() of before). I'm allowing it to use its default set of allowed tags, which is a sane default, so I haven't made the list of allowed tags configurable. IMHO this is the simplest, sanes possible approach.

My initial reaction to supporting HTML elements inside a figcaption was rejection. It seemed to me to negate the purpose and intent of a limited HTML filter. If I have a limited HTML filter that disallows the blockquote tag, I will be able to render a blockquote on the page regardless by including in the caption's figcaption value.

Now, after a long discussion with Wim. I've changed my opinion. First of all, the caption filter renders a figure element regardless of whether this element is allowed by a limited HTML filter. The expectation is already set that this filter is playing by its own rules. Second, the set of allowed tags that a user can pass through to the caption is limited by filter_xss. This is a truly an innocuous set of tags. Thirdly, if the behavior of this filter isn't what you need on your site, there is nothing preventing the site builder from creating a modified version and using that instead.

I'm fine with the behavior introduced in #32. It fits the expectations of this filter and doesn't require any interdependent logic with any HTML limiting filters.

I've just run this through its paces on a fresh install, and gone over the code changes. It's actually remarkably svelte for the awesomeness of its functionality, and provides a really useful guide for other devs seeking to do similar things in core. Once support for it is integrated into CK it will be extra super-awesome, but for now it's super-awesome. The approach being taken here is one that I think is really important for the future of Drupal's assistive editing technologies.

1. We settle on a simple "storage markup" format that captures the semantic intent, whether it's data-attributes on an existing element, a custom element type, or cross-site itemprop standards.
2. We use the filtering and theming system (or other 'presentation layer' steps) to transform it into appropriate client-facing output.
3. We layer on assistive tools in the WYSIWYG editor that make it easy to manipulate the semantic value, rather than the final client-facing output.

In broad terms, this is the kind of approach that has been settled on in the techcomm world (where custom XML DTDs and technologies like DITA rule the roost). Store semantics, transform for output, and provide assistive tools -- it's a strategy has been shown to work consistently for markup-oriented data.

I was actually going to raise an objection to using figcaption for attribution of a blockquote, but then I dug around and it turns out that's a pretty official recommendation. I'm going to be bold, here, and mark this RTBC.

This doesn't feel like a feature to me, it feels like a required task if we're going to allow tags in a wysiwyg editor in core in any way whatsoever.

That wasn't a deliberate mistake with the <img> tag but see!!!

It still works! :D

+++ b/core/modules/filter/css/filter.caption-rtl.cssundefined
+++ b/core/modules/filter/css/filter.caption-rtl.cssundefined
@@ -0,0 +1,18 @@

@@ -0,0 +1,18 @@
+/**
+ * @file
+ * Caption filter: RTL styling for displaying image captions.
+ */
+
+/**
+ * Caption alignment.
+ */
+.caption-left {
+  float: right;
+  margin-left: auto;
+  margin-right: 0;
+}
+.caption-right {
+  float: left;
+  margin-left: 0;
+  margin-right: auto;
+}
diff --git a/core/modules/filter/css/filter.caption.css b/core/modules/filter/css/filter.caption.css

diff --git a/core/modules/filter/css/filter.caption.css b/core/modules/filter/css/filter.caption.css
new file mode 100644
index 0000000..2c6c059

index 0000000..2c6c059
--- /dev/null

--- /dev/null
+++ b/core/modules/filter/css/filter.caption.cssundefined

+++ b/core/modules/filter/css/filter.caption.cssundefined
+++ b/core/modules/filter/css/filter.caption.cssundefined
@@ -0,0 +1,37 @@

@@ -0,0 +1,37 @@
+/**
+ * @file
+ * Caption filter: default styling for displaying image captions.
+ */
+
+/**
+ * Essentials, based on http://stackoverflow.com/a/13363408.
+ */
+.caption {
+  display: table;
+}
+.caption > * {
+  display: block;
+  max-width: 100%;
+}
+.caption > figcaption {
+  display: table-caption;
+  caption-side: bottom;
+  max-width: none;
+}

RTL needs a reroll because of #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute

EDIT: Removed off topic comment

NW because of #44. About #45 it is an old patch, isn't it? #26 uses Twig.

If #45 was off topic, I suspect so was the category change.

Committed 500c082 and pushed to 8.x. Thanks!

Thanks @alexpott! This is exciting indeed!

I somehow missed this, but don't we need to support picture as well? Or do we do this in contrib?

For what it is worth... I'm late to the game on this, but I just wanted to note that https://drupal.org/project/shortcode and https://drupal.org/project/sc_basic is another alternative to https://drupal.org/project/caption_filter.

Here is a youtube demo http://www.youtube.com/watch?v=IvA3SYPmqVo&feature=youtu.be&hd=1

Here is how one university is using the module http://www.library.uq.edu.au/site-documentation/shortcodes-and-available...

#54 #55 thus I shamelessly put another module in the list https://drupal.org/project/image_autosize. It tries to do the same thing with the same approach: extract the alignment to determine size (so that it works more or less with RWD), and title/alt attribute to use as caption (when there is a caption, you usually don't need the title attribute). The theming layer allows to use just a DIV or figcaption to display the caption.

It is much close to this issue. But I didn't try to put it here because the proposed solution already does exactly what it does. Glad to see it made into D8 with a cleaner code ;)

Note: This issue only is about ckeditor inline images.

Captions for image field formatter seem unresolved.
Adding related issues.