Problem/Motivation

We finally have a WYSIWYG editor in core: #1890502: WYSIWYG: Add CKEditor module to core.

If we can now also add image captions, align images, and align the caption along with the image, then we'll finally have achieved what quicksketch has been asking for years :)

As usual, we want our content to remain pristine and not contain crappy markup. It should not prevent structured content. Hence it should be a filter.

Furthermore, it should be possible for a site to 1) customize the HTML & CSS used for the captions across the entire site, 2) do 1 after content was created. Using a filter addresses those two requirements as well.

Finally, disabling this filter on a text format should cause the content to not be broken; it should gracefully degrade to just images. Which means custom syntax like this is unacceptable: [caption]<img src="" alt="" />This is an image caption[/caption]. That is: the content stored in the DB must remain clean, it must remain HTML.

Proposed resolution

captioned & aligned images.png

All of the above requirements are met by enriching <img> tags with data- attributes:

<img src="cat.jpg" data-caption="Caption text." data-align="(left|right|center)" />

This is then transformed to something like this by theme_filter_caption() (this is the HTML that the caption_filter module uses, and it's been proven in real-world testing):

<div class="caption caption-left">
  <div class="caption-inner" style="width: 500;">
    <img src="cat.jpg" />
    <div class="caption-text">Caption text.</div>
  </div>
</div>

Note: the generated HTML does not actually matter, we could also change it to something more modern/HTML5y and less infected by divitis:

<figure class="caption-image caption-align-right">
   <img src="cat.jpg" />
   <figcaption>Caption text.</figcaption>
</figure>

(Based on http://alistapart.com/blog/post/more-thoughts-about-blockquotes-than-are....)

Let's figure out what's the best caption markup for Drupal core.

UX to caption & align images

We will make it trivial for CKEditor users to do this, and will even make it a delightful experience. However, this is the necessary first step, which obviously works stand-alone.

Origin of code

The code to do this was partially lifted from the http://drupal.org/project/caption_filter module: the generated HTML and corresponding CSS is unchanged from that in that module. The code that takes care of the filtering has been revamped to be much more robust though: much like the filter_html_image_secure filter, this uses PHP's DOMDocument/DOMNode APIs to do this in a much more robust manner.

(This is exactly what was built for Aloha Editor back in October 2012; in fact, I copied the code I wrote for this from aloha.module. The code was stable/solid then, and it is now.)

Remaining tasks

  1. Build consensus:
    1. Implemented as filter?
    2. Part of filter.module?
    3. Target HTML?
    4. Filter enabled by default for Basic HTML and Full HTML in Standard profile?
  2. Test coverage.
  3. Integrate this with CKEditor: make it a trivial, delightful experience to caption and align images!

User interface changes

  • New filter_caption filter.
  • Image captions in Drupal core!
  • Filter enabled by default for Basic HTML and Full HTML in Standard profile.
  • No UI to caption or align images! This is about the filter only, there will be a separate issue for CKEditor integration.

API changes

None, unless a new theme function counts.

Soon, an issue for: "Integrate this with CKEditor: make it a trivial, delightful experience to caption and align images!".

Files: 
CommentFileSizeAuthor
#48 image_captions-2014895-48.patch21.56 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 56,738 pass(es). View
#48 interdiff.txt1.77 KBWim Leers
#32 html_in_captions.png17.74 KBWim Leers
#32 image_captions-2014895-32.patch21.87 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 56,815 pass(es). View
#32 interdiff.txt5.17 KBWim Leers
#27 Screenshot_6_18_13_12_57_PM.png99.52 KBjessebeach
#27 Screenshot_6_18_13_12_57_PM.png94.37 KBjessebeach
#27 image_captions-2014895-26.patch21.88 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 58,135 pass(es). View
#27 interdiff_26-to-27.txt421 bytesjessebeach
#24 image_captions-2014895-24.patch20.98 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 57,769 pass(es). View
#22 captions_on_anything.png58.72 KBWim Leers
#22 image_captions-2014895-22.patch21.9 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_captions-2014895-22.patch. Unable to apply patch. See the log in the details link for more information. View
#22 interdiff.txt15.83 KBWim Leers
#17 image_captions-2014895-17.patch21.14 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 57,692 pass(es). View
#17 interdiff.txt18.29 KBWim Leers
#17 caption_filter-all_possible_outputs.png151.56 KBWim Leers
#15 image_captions-2014895-15.patch13.19 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 57,444 pass(es). View
#15 interdiff_4-to-15.txt3.26 KBjessebeach
#4 image_captions-2014895-4.patch10.64 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 55,892 pass(es). View
#4 interdiff.txt2.54 KBWim Leers
#1 image_captions-2014895-1.patch10.61 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_captions-2014895-1.patch. Unable to apply patch. See the log in the details link for more information. View
#1 captioned & aligned images.png160.28 KBWim Leers

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
160.28 KB
10.61 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_captions-2014895-1.patch. Unable to apply patch. See the log in the details link for more information. View

I've triple-checked to make sure I haven't forgotten anything, and I still feel I forgot something: this patch is only 11 KiB!
(When I add tests, that will increase of course.)

Issue summary updated to list the attached screenshot of the current state.

Note: CKEditor will currently automatically strip any data- attributes, so please apply #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms first to fix that, or disable CKEditor.

Status: Needs review » Needs work

The last submitted patch, image_captions-2014895-1.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.54 KB
10.64 KB
PASSED: [[SimpleTest]]: [MySQL] 55,892 pass(es). View

A patch just got committed that moves filter.module's CSS files into a css subdirectory apparently.

Bojhan’s picture

Status: Needs review » Needs work

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

ry5n’s picture

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

Status: Needs work » Needs review

Bojhan: Will *what* be configurable from the UI? :)

ry5n: thanks!
- when no caption, only alignment, only image should be aligned: that probably makes more sense. Maybe that should be a setting though?
- no class on figure tag (wrapper) by default: I don't think that will fly: we need to be able to discern between figure tags added by the author vs. those created by this filter…
- work to achieve that is already under way at #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default) and has already received a lot of in-depth discussion, but that is a most excellent point. That means this issue is effectively blocked on that issue.

Back to NR for more reviews, Bojhan set it to NW accidentally.

Bojhan’s picture

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

Oops, sorry for the NW.

Wim Leers’s picture

#8: From the issue summary:

UX to caption & align images

We will make it trivial for CKEditor users to do this, and will even make it a delightful experience. However, this is the necessary first step, which obviously works stand-alone.

So: currently "source", later: "inline".

Wim Leers’s picture

Issue summary: View changes

Add image to match patch in #1.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

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

Gábor Hojtsy’s picture

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

LewisNyman’s picture

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

Wim Leers’s picture

#12: Thanks for the feedback, I'll take that into account! :)

Note: If people don't like core's behavior, the theme can of course override the CSS, or the HTML *and* the CSS.

Wim Leers’s picture

Note to self for reroll: #1985344: Add a dedicated @FieldFormatter annotation will be committed soonish (it's RTBC ATM), when that is committed, this patch will have to be rerolled.

jessebeach’s picture

FileSize
3.26 KB
13.19 KB
PASSED: [[SimpleTest]]: [MySQL] 57,444 pass(es). View

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.

Wim Leers’s picture

Thanks Jesse! :) I'll continue this on Friday: add tests etc.

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
151.56 KB
18.29 KB
21.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,692 pass(es). View

Jesse did a great job moving us forward wrt target HTML and cleaner CSS in #15. However, the HTML still included a <div> tag that ideally wouldn't be there.
Furthermore, #10 rightfully questions whether we should really retrieve the image width, because that can easily cause big performance problems. But that means we must somehow fix this with just CSS.

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.

This slightly changes the way it looks, but changing the looks is something that can trivially be done at any point in the future; the styling is least important right now, the feature is what matters.


#10: all fixed!

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.

Wow, great point! :) Added test coverage proves that the updated code in this reroll guarantees that.

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

Indeed, it's cached anyway, but more importantly: it handles tricky edge cases in bizarrely formatted HTML for us. DOM manipulation rather than string manipulation guarantees this.


There is now *less* code. The patch is only bigger because it adds test coverage. It also fixes weirdness in FilterUnitTest. This is what it currently looks like, for all possible cases:

  1. caption, aligned right
  2. caption, aligned left
  3. caption, aligned center
  4. caption, not aligned
  5. no caption, aligned

caption_filter-all_possible_outputs.png

Remaining tasks

From the issue summary:

  1. Build consensus:
    1. Implemented as filter? as per #11
    2. Part of filter.module? as per #11
    3. Target HTML? as per #6
    4. Filter enabled by default for Basic HTML and Full HTML in Standard profile? as per #11
  2. Test coverage. provided here!
  3. Stand-alone follow-up: Integrate this with CKEditor: make it a trivial, delightful experience to caption and align images!

So, unless I forget something, this issue has completed all remaining tasks. It just needs to get to the RTBC state now. Please review! :)

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

The last submitted patch, image_captions-2014895-17.patch, failed testing.

Wim Leers’s picture

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

#17: image_captions-2014895-17.patch queued for re-testing.

Dave Reid’s picture

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

quicksketch’s picture

Status: Needs review » Needs work

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.83 KB
21.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_captions-2014895-22.patch. Unable to apply patch. See the log in the details link for more information. View
58.72 KB

#20: It does not, and to leave the door open, I named it filter_caption, not filter_image_caption :) In fact, the HTML structure used here is also the recommended one to mention authors in quotes, for text with code examples, and so on. So I just went ahead and implemented it — it even makes the code simpler :)

<video autobuffer="" controls="" data-caption="yarhar" height="515" preload="none" src="http://wimleers.com/sites/wimleers.com/files/unidirectional_text_editor_configuration_to_text_format_filter_settings_syncing.mp4" width="420">&nbsp;</video>

<blockquote data-caption="Oscar Wilde">If one cannot enjoy reading a book over and over again, there is no use in reading it at all.</blockquote>

<code data-caption="Simple Drupal announcement example.">Drupal.announce(&#39;Hello, world!&#39;);</ code>

<pre data-caption="Simple HTML example.">
&lt;p&gt;Hello, world!&lt;/p&gt;
&lt;p&gt;How are you?&lt;/p&gt;
</pre>

Results in

captions_on_anything.png


#21:

  • Great :)
  • Converted to Twig template.
  • RE: new CSS technique: yay :)
  • So… this is tricky territory. Is this truly essential?
    1. It's against the HTML spec. You could indeed work around it by encoding <em> as &lt;em&gt;.
    2. Captions are intended to be brief, not to contain whole paragraphs. I do agree that <a> and <cite> for example are valid use cases.
    3. 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 <em> and <a> 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.

Status: Needs review » Needs work

The last submitted patch, image_captions-2014895-22.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
20.98 KB
PASSED: [[SimpleTest]]: [MySQL] 57,769 pass(es). View

Rebased.

quicksketch’s picture

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.

Wim Leers’s picture

#25: Okay. The fact that it was considered so important in Wordpress is convincing!

Caveats:

  • Wordpress used a non-HTML-based syntax, so it's not 1:1.
  • 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).
  • No matter how you put it, this equates to recursive filtering. Which is kinda crazy.
jessebeach’s picture

FileSize
421 bytes
21.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,135 pass(es). View
94.37 KB
99.52 KB

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

Screenshot_6_18_13_12_57_PM.png

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

Screenshot_6_18_13_12_57_PM.png

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?

quicksketch’s picture

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.

Wim Leers’s picture

Alright. I'm going to work on this first thing tomorrow morning :)

eaton’s picture

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.

jessebeach’s picture

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.

Wim Leers’s picture

FileSize
5.17 KB
21.87 KB
PASSED: [[SimpleTest]]: [MySQL] 56,815 pass(es). View
17.74 KB

And voila, HTML support in captions.

<video autobuffer="" controls="" data-caption="yarhar" height="515" preload="none" src="http://wimleers.com/sites/wimleers.com/files/unidirectional_text_editor_configuration_to_text_format_filter_settings_syncing.mp4" width="420">&nbsp;</video>

<blockquote data-caption="Oscar Wilde, &lt;cite&gt;Some magical book&lt;/cite&gt;">If one cannot enjoy reading a book over and over again, there is no use in reading it at all.</blockquote>

<code data-caption="Simple &lt;strong&gt;Drupal&lt;/strong&gt; announcement example.">Drupal.announce(&#39;Hello, world!&#39;);</ code>

<pre data-caption="Simple &lt;a href=&quot;/glossary/html&quot;&gt;HTML&lt;/a&gt; example.">
&lt;p&gt;Hello, world!&lt;/p&gt;
&lt;p&gt;How are you?&lt;/p&gt;
</pre>

Results in
html_in_captions.png

(Compare to #22.)


As you can see in the interdiff, the amount of changes is absolutely minimal.

I chose to *not* apply the same text format again (using check_markup()), but for something simpler, that also automatically prevents recursion problems: 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.

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

The last submitted patch, image_captions-2014895-32.patch, failed testing.

Wim Leers’s picture

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

#32: image_captions-2014895-32.patch queued for re-testing.

jessebeach’s picture

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.

eaton’s picture

Status: Needs review » Reviewed & tested by the community

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.

Wim Leers’s picture

Follow-up issue to integrate with CKEditor posted and almost complete: #2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images.

catch’s picture

Category: feature » task

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

catch’s picture

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

Wim Leers’s picture

#39: haha :D

Wim Leers’s picture

#32: image_captions-2014895-32.patch queued for re-testing.

Wim Leers’s picture

This probably no longer applies/passes tests after 9 days, retesting.

mparker17’s picture

It still works! :D

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

EDIT: Removed off topic comment

jcisio’s picture

Category: task » feature

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

effulgentsia’s picture

Category: feature » task
Status: Reviewed & tested by the community » Needs work

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

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.77 KB
21.56 KB
PASSED: [[SimpleTest]]: [MySQL] 56,738 pass(es). View

Addressed #44.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Wim Leers’s picture

Issue tags: -sprint
quicksketch’s picture

Thanks @alexpott! This is exciting indeed!

attiks’s picture

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

Wim Leers’s picture

#52: it supports any HTML tag currently. I'm not sure if that is sufficient.

gmclelland’s picture

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

Wim Leers’s picture

#54: the reasons why those shortcodes are inferior are explained in the issue summary and have been discussed at length at many occasions. Don't get me wrong: they work just fine, but it results in a mix of HTML and custom mark-up, as well as a poor authoring experience. The filter added in this issue, combined with #2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images allows for clean, structured content and a stellar authoring experience.

I'll create a screencast for #2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images tomorrow, because by pointing you to it now, I realized that the screenshots there don't say enough. Follow that issue and stay tuned :)

jcisio’s picture

#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 ;)

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

Anonymous’s picture

Issue summary: View changes

Explicitly state in the list of UI changes that this issue does not provide a UI.

axel.rutz’s picture

Note: This issue only is about ckeditor inline images.

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