Problem/Motivation

FilterCaption was introduced in #2014895: Image captions & alignment (for inline images) and was slightly improved in #2170085: Caption filter may not wrap images that are only aligned, not captioned.

There is still one remaining problem: the fact that you can only enable alignment (data-align) and captioning (data-caption) at the same time. There is no good reason for them to be together in one filter, other than that being slightly simpler to implement originally.

It also helps in promoting the spirit behind filters: combining many very focused Lego blocks together to a greater whole.

Proposed resolution

We should not force this on people; we should give them the choice. So let's split it up into two separate filters. The consequences are:

  1. filter_align, if used together with filter_caption, must run first
  2. filter_caption will no longer pass a align parameter to the Twig template, but instead it will pass the value of the captioned element's class attribute

Remaining tasks

  1. We will also need to make sure that the corresponding CKEditor plugins (drupalimage/drupalimagecaption) can cope with this change, but that should wait on #2039163: Update CKEditor library to 4.4, which is changing them drastically.
  2. Update FilterUnitTest

User interface changes

Two visible filters instead of one.

API changes

None.

CommentFileSizeAuthor
#15 interdiff.txt3 KBwim leers
#15 filter_caption_filter_align-2239413-15.patch31.98 KBwim leers
#11 interdiff.txt8.96 KBwim leers
#11 filter_caption_filter_align-2239413-11.patch31.58 KBwim leers
#9 filter_caption_filter_align-2239413-9.patch22.44 KBblueminds
#6 filter_caption_filter_align-2239413-6.patch23.05 KBwim leers
#4 filter_caption_filter_align-2239413-4.patch20.34 KBwim leers
#1 filter_caption_filter_align-2239413-1-do-not-test.patch10.86 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
StatusFileSize
new filter_caption_filter_align-2239413-1-do-not-test.patch10.86 KB

This patch already works just fine, we just need to wait for #2039163: Update CKEditor library to 4.4 to push it to the finish line :)

Thanks to Piotrek Koszuliński from the CKEditor team for pointing out that this doesn't make sense!

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Issue tags: +Media Initiative

Yes! This makes way more sense, especially for allowing alignment to be used on things that captions don't make as much sense.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Postponed » Needs review

I think this also may require a change to EditorImageDialog to ensure that neither, one of, or both of the 'Alignment' or 'Caption' elements are visible if we're splitting the filters?

Un-postponing since #2039163: Update CKEditor library to 4.4 has been resolved.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new filter_caption_filter_align-2239413-4.patch20.34 KB

I was waiting for #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags to land as well. That has now landed, so rerolling.

Updated test coverage, and also implemented #3 — which I failed to mention in the issue summary, but of course agree with :)

Status: Needs review » Needs work

The last submitted patch, 4: filter_caption_filter_align-2239413-4.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new filter_caption_filter_align-2239413-6.patch23.05 KB

FilterAlign.php was missing from #4 :/ Stupid!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#6 still applies. Please review!

cs_shadow’s picture

cs_shadow commented

Changes look good. Just a minor nitpick:

+++ b/core/modules/filter/filter.module
@@ -80,7 +80,7 @@ function filter_theme() {
         'caption' => NULL,
-        'align' => NULL,
+        'classes' => NULL,

Is assigning NULL to classes necessary here?

Otherwise works like a charm, tested on my dev site.

My only concern is that can we also split the ckeditor/js/plugins/drupalimagecaption/plugin.js similar to the filters as a follow-up?

blueminds’s picture

blueminds commented
StatusFileSize
new filter_caption_filter_align-2239413-9.patch22.44 KB

Reroll.

Re assigning NULL to the classes - not sure if you can do anything else here? You need to have the classes key around, however do not have any value to assign to it at that stage.

If the reroll is okay, for me RTBC.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

My only concern is that can we also split the ckeditor/js/plugins/drupalimagecaption/plugin.js similar to the filters as a follow-up?

Good point! This is also listed in the IS as a remaining task (the last one in fact!).

The existing patch already updates DrupalImageCaption and EditorImageDialog, but the JS is indeed not yet updated accordingly. Fixing that now.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new filter_caption_filter_align-2239413-11.patch31.58 KB
new interdiff.txt8.96 KB

RE: assigning NULL to classes: this is the convention, so there's nothing to do there.

I've now updated ckeditor/js/plugins/drupalimagecaption/plugin.js. The server side tells it which filters are enabled (the alignment or captioning) and it disables corresponding logic accordingly. (I talked to the CKEditor developers what the best approach would be, and they recommended this.)
When testing with an image that has both a caption and alignment, and then either disabling the caption or alignment filter and revisiting the node form, you can see that everything is rendered as expected. And when you open the "source" mode, you can see that the data- attribute for the disabled filter is kept. So I can't find any bugs anymore. I can't RTBC my own patch of course.

So if either of you can do another round of manual testing, and you find no problems, you can RTBC this :)

While working on this, I found one bug (which already was present in the above patches): some CSS was not being loaded when the caption filter was disabled, but the alignment filter was enabled.

cs_shadow’s picture

cs_shadow commented
Status: Needs review » Reviewed & tested by the community

Happy to mark this as RTBC. Applies cleanly and works perfectly.

slashrsm’s picture

slashrsm commented

+1 for RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/filter/src/Plugin/Filter/FilterAlign.php
@@ -0,0 +1,73 @@
+          <li>… and you can apply this to other elements as well: <code>&lt;blockquote&gt;Llamas are awesome&lt;blockquote;&gt;

Missing the data-align attribute - also given this is user facing text perhaps we shouldn't use llamas here?

filter_align, if used together with filter_caption, must run first

I can not see any code that enforces this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new filter_caption_filter_align-2239413-15.patch31.98 KB
new interdiff.txt3 KB

Missing the data-align attribute […]

You're right, thanks! In fixing that, I also noticed a (pre-existing) typo and incomplete information for the caption filter help text.

[…] also given this is user facing text perhaps we shouldn't use llamas here?

That's fair :D

I can not see any code that enforces this.

The same "filter X must run before filter Y" exists for almost all filters: filter_html must run before filter_htmlcorrector, filter_html_escape must run before filter_autop, and so on. This is an unsolved problem that we've had for as long the filter system has existed.

cs_shadow’s picture

cs_shadow commented
Status: Needs review » Reviewed & tested by the community

LGTM.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +TCDrupal 2014

Committed 008fb11 and pushed to 8.0.x. Thanks!

  • alexpott committed 008fb11 on 8.0.x 
    Issue #2239413 by Wim Leers, blueminds: Split FilterCaption into...

Status: Fixed » Closed (fixed)

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

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -Media Initiative +D8Media