SafeMarkup in CKEditor toolbar configuration UI — fix by not generating markup in PHP but using Twig Macros

For now, you have to wrap these inline HTMLs with a new SafeMarkup() object

Bumping the priority since this is broken in latest HEAD.

So #1825952: Turn on twig autoescape by default broke this (and many other things), and fixing all the brokenness is being tracked at #2297711: Fix HTML escaping due to Twig autoescape.

Here is the patch. Not quite sure if it's a good solution but it works. See attached screeenshot.

It's perfect, thank you :)

Committed baacf9d and pushed to 8.x. Thanks!

This needs to be redone and SafeMarkup::set removed.

@chx,
Is it a good approach to remove SafeMarkup::set and introduce inline_template? I've made that change in ckeditor.admin.inc file and it seems to be working as expected.

Can you please have a look at the attached patch and then lets decide that whether it's a good approach to mitigate and resolve this issue.

Btw, there was a </ul> missing in the ckeditor-settings-toolbar.html.twig template.

Just removed the unused use Drupal\Component\Utility\SafeMarkup from file. Attached fixed patch.

Here's another approach that I believe we should really take.

It's not complete and there are a few templates strings that need to be moved to the template and the image uris need to be passed to the button array. Though I hope you agree this looks much more approachable?

Also this uses the translated label for the title/alt instead of the image_alternative string, which could be another fix, imo.

Using a macro to reduce the template duplication: http://twig.sensiolabs.org/doc/tags/macro.html

@joelpittet, this may be a more convincing way, can you please complete the file and re-queue for review?

Ran out of time this evening, you're welcome to post improvements. If not I'll finish this up likely over the weekend.

Okay, I'll try to work on it :-)

WOW :O
WOW :O
WOW :O

I had no idea this was possible! So this uses http://twig.sensiolabs.org/doc/tags/macro.html… This is indeed an excellent fit for this problem. So much cleaner!

+1 for this alternative solution.

I updated the issue title accordingly.

@joelpittet: Thank you so much for this nudge in a much better direction! I don't know about everybody else, but I had no idea this existed :) AFAICT this would be the first use of Twig Macros in Drupal core, and I'd be happy/proud to have CKEditor be the first :)

@joelpittet,
I had a quick look and one major issue I found is with Drupal\Core\Render\Element::children. Plugin elements are not properly being rendered there. While a bried debugging I found, ckeditor-multiple-button, separator & ckeditor-button-separator elements are not rendered as an array but string.

Also, to convert the Image URI to URL do you think $base_url can be used? Please have a look at the attached interdiff file. I also think we don't require directional settings in time of image rendering.
button.image ~ direction == 'rtl' ? '_rtl' was not working and even if this works then the resulting image will have .png_rtl extension.

I'll try to fix these issues but needs a bit of time, let me know if I can help you in this.

#19: never ever ever ever use $base_url or base_path(). It prevents altering file URLs to serve them from a CDN. For files, always use file_create_url().

@Wim, thanks. I almost forgot about this :-(
Will keep in mind from next time though.

No problem :) I have to keep reminding everyone of that, including core committers :)

Yeah that caught me too with the url() vs file_create_url() before, trying to get that in Twig for assets: #2168231: Twig Functions needed in templates and I think there is another issue trying to do this as well.

1. +++ b/core/modules/ckeditor/ckeditor.admin.inc
   @@ -82,6 +82,7 @@
   +      'base_url' => $base_url,
   
   +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -23,7 +23,7 @@
   +        <img src="{{ button.base_url ~ '/' ~ button.image }}" title="{{ button.label }}">
   

   I think this needs to be built in the preprocess for now and passed in on the button associative array for now. So you have the right idea just add an if statement above the $button_item to check for and generate the url need for the src.

2. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -48,7 +48,7 @@
   -             <li{{ disabled_button.attributes }}>{{ ckeditor.button(disabled_button) }} }}</li>
   +             <li{{ disabled_button.attributes }}>{{ ckeditor.button(disabled_button) }}</li>
   

   Thanks for spotting that!

Hope that helps put you on a track, thank you for pitching in @aneek

Thanks @joelpittet.
Let's solve the big issue with the render() function in Drupal\Core\Render\Element::children. Elements in that function expects array but some cases due to the modification of

-    $button_item = array(
-      'value' => $value,
+    $button_item = $button + array(
+      'rtl' => $rtl,
       'attributes' => new Attribute($attributes),
     );

breakes what is expected. Any thoughts on this regards?

Thanks!

I think the big render issue is because left some HTML strings in the image_alternate values over in getButtons method.

The rtl image src should be just built as it was before but instead of using #theme => image we just need the url.

+++ b/core/modules/ckeditor/ckeditor.admin.inc
@@ -64,25 +63,6 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
-        '#uri' => $button['image' . $rtl],

Take this uri, use file_create_url() to produce the URL for the image source, and pass it to the $button_item.

Yes, you are right.
This bit of code in /core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php

'-' => array(
        'label' => t('Separator'),
        'image_alternative' => '<a href="#" role="button" aria-label="' . t('Button separator') . '" class="ckeditor-separator"></a>',
        'attributes' => array(
          'class' => array('ckeditor-button-separator'),
          'data-drupal-ckeditor-type' => 'separator',
        ),
        'multiple' => TRUE,
      ),

is one of many that is causing this issue.
Just finished fixing another issue (#2309929) and its already very late at night so I'll look into it tomorrow along with the image URI fix. :-)
Thanks again.

@aneek thanks give it a shot tomorrow, I'll be on in the afternoon and likely give this another shot after your go at it.

@joelpittet,
I had done some changes today and as of the UI point of view its fine. But needs a thorough logical review. Below are some main points I tweaked.

1. Since we don't actually require image URI, I converted it to image URL. Like, $button['image'] = file_create_url($button['image' . $rtl]);
2. Based on the HTML placed in getButtons() method, I removed them and added extra condition in template to display them.
3. Modified the template file for some minor class name fix. Like, {{ button.image_alternative|replace({" " : ""}) }

Uploading a patch and an interdiff for your review. Let me know your thoughts.

Thanks! :-)

Remember to set the status as 'Needs review' every time you submit a patch so that the testbot can pick it up and run tests.

@cs_shadow, I do know that, but thanks for reminding me. Actually it's as of now only for logical review. Once the logical review is done by others and we finalize the right approach then I thought to make it ready for "Review" for the bots.

Thanks!

@aneek, ok, alright. I didn't knew that.

Thanks @aneek, i'll see if I can't finish it.

Going to remove the "Clone $button and remove unwanted array keys for HTML rendering." The extra context data won't hurt the template.

@aneek I wish we didn't have to introduce 'button_text' but I see why you did that, so thank you for pointing that out. If it could be just button.label it could almost be less conditionals, but I think I got it down to the bare minimum at the moment in the macro.

I changed the variable to image_url, so we still have access to the image variable(for whatever reason) and it's clear what the variable is.

@Wim Leers This may very well be, but it has to beat out #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template, which I love as well because it's also a recursive macro from @dawehner!

This works well from a quick test, but needs a bit of another set of eyes on the logic like @aneek mentioned and a manual testing.

Tesbot engage! (Dreditor should add a little paperclip to the bottom right corner.)

I think I offended it somehow... re-uploading.

@joelpittet,
Applied latest patch and working as expected. Thanks, for making me a part of this issue. It was a good learning experience with twig macros and CkEditor settings.

Btw, can we do good logical & manual review and a performance review if needed?

@aneek you sure can. I added Needs manual testing. In this case, it would be good to render that page before and after the patch is applied (remember to clear cache) You should see in the markup the Labels being used instead of the alternative_image text after the patch as a hint to know you are seeing the different page.

There are many ways to do markup comparison, I usually pump the results into a diff tool and take a screenshot:) See here: https://www.drupal.org/node/1987424#comment-8242319

Once something along those lines have been done you can remove that tag from here and post your findings. (crosses fingers the changes hold up)

And! thank you very much for helping out on this as well! We likely don't need a performance review on this as it's admin facing and we're opting out on things we don't expect to have a significant change in admin facing twig changes.

You can see markup comparisons on nearly every twig conversion issue in #1757550: [Meta] Convert core theme functions to Twig templates

Would like to get someone who wasn't working on the patch to do a logic look over but you're welcome of course to review the logic and let me know any concerns you see. I think I may have had hand even on that missing <ul>tag in the first place :S

@joelpittet, I'll surely have a look at the way you mentioned, and report if something is missed or needs correction. I totally agree some one as third person should have a review on this.

Thanks!

ROFL @ #36 & #37 :D

This looks great! Thank you so much for working on this — this looks much better than what I could've come up with! Just minor stuff:

1. +++ b/core/modules/ckeditor/ckeditor.admin.inc
   @@ -61,54 +60,25 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
   +    $rtl = $language_direction === 'rtl' ? '_rtl' : '';
   +    if (isset($button['image' . $rtl])) {
   

   This $rtl variable is confusing. I'd call it $suffix.

2. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -13,6 +13,33 @@
   +  {# Value of the button item. #}
   

   This seems to be the only documentation here. Seems kind of lost?

3. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -13,6 +13,33 @@
   +    {% if button.image_alternative in ['format', 'styles']  %}
   +      <a href="#" role="button" title="{{ button.button_text }}" aria-label="{{ button.button_text }}">
   +        <span class="ckeditor-button-dropdown">{{ button.button_text }}<span class="ckeditor-button-arrow"></span></span>
   +      </a>
   

   It's not okay to special case these, because contrib will add more buttons. We'll need a flag in the ckeditor.admin.inc PHP (e.g. isDropdown) that the Twig code can use instead.

4. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -13,6 +13,33 @@
   +    {% elseif button.image_alternative == 'separator' %}
   +      <a href="#" role="button" title="{{ button.button_text }}" aria-label="{{ button.button_text }}" class="ckeditor-separator"></a>
   

   This one *is* okay, because the separator is provided by CKE, and other CKE plugins won't add more of these.

5. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -13,6 +13,33 @@
   +  {% else %}
   +    ?
   +  {% endif %}
   

   …? :)

6. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -13,6 +13,33 @@
   +{% import _self as ckeditor %}
   

   What does this do? Perhaps could use a comment?

@Wim Leers,
Just a thought, can't we move the macro code in a twig file and inport it? Say "ckeditor-buttons-macro.twig" and import it to the main template file? And that file can have the required documentation.
Like,

{% import "@ckeditor/ckeditor-buttons-macro.twig" as ckeditor %}

So the code will be more clearer I guess.
About your point # 5, this is added by default by CKE, if no buttons are found rather no image_alternative or button.image_url are found then all the button texts are shown as "?".

On point #3, don't you think the buttons should supply "isDropdown" flag from the getter methods (getButtons)? So if we make just a small change in the template by removing button.image_alternative in ['format', 'styles'] and making it just button.isDropdown == true or button.isDropdown == false then we can reduce all the static logical calls.

What are your thoughts on these?

just a quick interdiff for more references. It only consists of the new template creation for the macro code.

@Wim Leers all great points in #42 I agree with all of them.

@aneek want to take a stab at them? Regarding #43 you can and if it's useful to other templates that makes a bunch of sense but for now let's just keep it inline and add a comment to what it does. Maybe referencing the twig docs on the matter. Keeping it in the same file gives it the context. Splitting the template seems to disassociate them and only see that being a good way to go if it's re-usable.

@joelpittet: thanks, I'll surely work on it, this week. One point though, isn't that be good if we use isDropdown as attributes (array keys) to the elements that need the dropdown type button? Just like,

'Format' => array(
  'label' => t('HTML block format'),
  'button_text' => t('Format'),
  'image_alternative' => 'format',
  'isDropdown' => TRUE,
)

So later on if any contrib modules pass the same then we can have a dynamic way to generate <span class="ckeditor-button-dropdown">.
In macro code we will only change the conditions accordingly.
May be you can have a better idea to do this. What are your thoughts in this regards?

@aneek yes that is good I think but be consistent with naming convention there, we use _ separators not camelCase. Though dot syntax makes that camelCase look nicer we should be consistent still.

Indeed, let's keep the macro in the same file for context (though I really liked the import statement in #43 — makes it much clearer what that import is for), and +1 to #46 and #47 (is_dropdown).

Thanks @joelpittet & @Wim Leers for sharing your valuable suggestions. I'll start implementing a new patch and post it soon. :-)

Thanks!

@joelpittet & @Wim Leers,

While working on the patch I've made a few changes.

1. Added is_dropdown to the elements where needed. And also added 'is_separator' => TRUE,. It's unlikely that other modules will add separators but its to make the conditions in twig more specific.
   

   '-' => array(
           'label' => t('Separator'),
           'image_alternative' => 'separator',
           'button_text' => t('Button separator'),
           'is_separator' => TRUE,
           'attributes' => array(
             'class' => array('ckeditor-button-separator'),
             'data-drupal-ckeditor-type' => 'separator',
           ),
           'multiple' => TRUE,
         ),
   

2. In the twig template I've used same as test. http://twig.sensiolabs.org/doc/tests/sameas.html
   So the code becomes,

   {% if button.is_dropdown is same as(true) %}
         <a href="#" role="button" title="{{ button.button_text }}" aria-label="{{ button.button_text }}">
           <span class="ckeditor-button-dropdown">{{ button.button_text }}<span class="ckeditor-button-arrow"></span></span>
         </a>
       {% elseif button.is_separator is same as(true) %}
         <a href="#" role="button" title="{{ button.button_text }}" aria-label="{{ button.button_text }}" class="ckeditor-separator"></a>
       {% else %}
         <a href="#" role="button" title="{{ button.label }}" aria-label="{{ button.label }}" class="cke-icon-only cke_{{ button.direction|default('ltr') }}" >
           <span class="cke_button_icon cke_button__{{ button.image_alternative }}_icon">{{ button.label }}</span>
         </a>
       {% endif %}
   

Let me know if you have any feedbacks.
Thanks!

#50: you forgot to attach the patch? :) Both points look fine to me :) (Joël will be better able to answer your second point.)

@Wim Leers, no it's not complete yet. Once done I'll upload it. Also awating Joel's suggestions.
Thanks!

@aneek You can post a patch, we'll read it just the same, but feedback on #50

1. That makes sense to me so far.
2. It's much simpler, we aren't using strict variables in twig for this very reason.

{% if button.is_dropdown is same as(true) %}
becomes
{% if button.is_dropdown %}

@joelpittet, thanks for the feedback. I'll post a patch soon enough for review.

Few observations, if we change the language direction from LTR to RTL.

1. The images are then searching for the array key image_rtl instead of image in respective getButtons() methods. Resulting "?" to appear in the UI. This needs to be fixed in the getButtons() methods.
   
2. The buttons however, are adapting to the language direction change properly as we've added an array key $button['language_direction']. So usages of 'image_alternative_rtl' => TRUE is not required in the respective getButtons() methods.
   
3. A minor change in twig template.
   

   +      <a href="#" role="button" title="{{ button.label }}" aria-label="{{ button.label }}" class="cke-icon-only cke_{{ button.direction|default('ltr') }}" >
   +        <span class="cke_button_icon cke_button__{{ button.image_alternative }}_icon">{{ button.label }}</span>
   +      </a>
   

   This "cke-icon-only cke_{{ button.direction|default('ltr') }}" needs to be "cke-icon-only cke_{{ button.language_direction|default('ltr') }}" to show the language changes properly.

I'll upload a patch by tomorrow for you guys to review.
Thanks!

@joelpittet & @Wim Leers,

Please find the attached patch & interdiff for logical review.

Let me know if any further fixes are needed. :-)

Thanks!

I think everything you did except the two things mentioned below are spot on. Just need a bit of clarification as I couldn't put two and two together in these cases:

1. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
   @@ -53,10 +53,12 @@ public function getConfig(Editor $editor) {
   +    $language_direction = \Drupal::languageManager()->getCurrentLanguage()->direction;
   +    $suffix = $language_direction === 'rtl' ? '_rtl' : '';
   ...
   -        'image' => drupal_get_path('module', 'ckeditor') . '/js/plugins/drupalimage/image.png',
   +        'image' . $suffix => drupal_get_path('module', 'ckeditor') . '/js/plugins/drupalimage/image.png',
   
   +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalLink.php
   @@ -52,14 +52,16 @@ public function getConfig(Editor $editor) {
   +    $language_direction = \Drupal::languageManager()->getCurrentLanguage()->direction;
   +    $suffix = $language_direction === 'rtl' ? '_rtl' : '';
   ...
   -        'image' => $path . '/link.png',
   +        'image' . $suffix => $path . '/link.png',
   ...
   -        'image' => $path . '/unlink.png',
   +        'image' . $suffix => $path . '/unlink.png',
   

   How does it know that the rtl exists? Maybe the scope of this went a bit far or maybe this is an improvement? ... hard to tell

2. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
   @@ -178,34 +178,28 @@ public function getButtons() {
   -        'image_alternative_rtl' => TRUE,
   ...
   -        'image_alternative_rtl' => TRUE,
   ...
   -        'image_alternative_rtl' => TRUE,
   

   Some of these had image alternatives that are RTL (image alternatives is usually a CSS sprite or something it seems) But I don't think all of them have this, I couldn't notice how you accounted for these removals.

@joelpittet,
Please find the explanations below.

About #1: If you check the CKEditorPluginButtonsInterface (/core/modules/ckeditor/src/CKEditorPluginButtonsInterface.php) image_rtl is mentioned as

image_rtl: If the image needs to have a right-to-left version, specify

What I've done is, I just took the default language direction from the configuration and appended to the image URI. My thought was we may not need RTL specific images for these buttons (link, unlink & image).

$language_direction = \Drupal::languageManager()->getCurrentLanguage()->direction;
$suffix = $language_direction === 'rtl' ? '_rtl' : '';
// This suffix added to image URI
'image' . $suffix => $path . '/link.png',

So if the language direction is RTL then image URI becomes 'image_rtl' => $path . '/link.png'.
Other modules may explicitly specify "image_rtl" if they need different images for the buttons depending on language change. Yes, you may call this is as an improvement. But if you don't have this and change the language direction in your site, then you will not get the image buttons working and will see "?" instead of images. (Refer to attached screenshot on #55)

About # 2: Again if you follow the interface CKEditorPluginButtonsInterface, you will see that its written,

image_alternative_rtl: Similar to image_alternative, but a right-to-left version.

So yes you are right about they using sprite images or something. As an example, in the CkEditor Toolbar, "BulletedList" button has different images for image_alternative and image_alternative_rtl. Since in your code from #34 patch, you already taken care of this on this line,

// Add language direction property.
$button['language_direction'] = $language_direction;

And I just changed in twig from,

<a href="#" role="button" title="{{ button.label }}" aria-label="{{ button.label }}" class="cke-icon-only cke_{{ button.direction|default('ltr') }}" >

to,

<a href="#" role="button" title="{{ button.label }}" aria-label="{{ button.label }}" class="cke-icon-only cke_{{ button.language_direction|default('ltr') }}" >

In this case "button.direction" was an empty variable and I thought it should be "button.language_direction". The main difference it makes is toggling the HREF class "cke-icon-only cke_ltr" for LTR or "cke-icon-only cke_rtl" for RTL direction. So do we actually require to add the key in getButtons() method?

I hope these explanations above clears your doubts? Let me know your thoughts.
Thanks!

#58.1/#59.1: These changes are also factually wrong because no RTL variant used to exist for them, it still doesn't exist, it doesn't need to exist, but the code changes make it seem like they do or may exist. They also make a call into global state (current language direction), which should be avoided.
Why not use the previous behavior? Default to image, not image_ltr, which can then be used for both LTR and RTL — because not all buttons need a RTL version.
If the concern is that Drupal favors LTR in its code, well, that's something that happens everywhere (e.g. in CSS), and we shouldn't start changing that here.

#58.2/#59.2: Indeed, these were using a sprite. The changes described in #59.1 indicate that a image_ltr version must be present, which is an API change that we can and should avoid, for the reasons mentioned above.

I hope that all made sense and that you agree with it — ping me on IRC if something isn't clear!

@Wim Leers, had a discussion with @joelpittet about the implementation and about your comments. Here is the summery.

• We will keep image_alternative_rtl as it is. And add changes to the code and twig template to accommodate that.
• If the language is changed from LTR to RTL image buttons are searching for "image_rtl". This is causing to show "?" in the UI. This we think is a bug that was already there and not introduced via the patch. So we will try to make a test case that will change the language direction and check if the page has any existence of "?"
• We'll also change image buttons to add image_rtl key in getButton() methods and add changes to template to adapt the key addition.

@joelpittet, please add your thoughts if I've missed something.

@joelpittet, created a patch to check the image button load if the language is changed from En to Ar. So the language directions will also change.

This patch will product 3 Undefined index: image_rtl in the test result. Please have a look.

Soon, I will post the fix patch and the testonly patch for further reviews.

Uploading patches for the fix.

Added a new element in $button array as "language_direction_class" which will handle the LTR to RTL language direction conversion and 'image_alternative_rtl' => TRUE key as well.

@joelpittet & @WimLeers, please review and let me know your thoughts.

Thanks!

Wow, your test is awesome!

It goes to the page and picks up 3 rtl images broken:) I like how it doesn't assert anything, well truthfully that makes me uneasy a bit but it still does a great job of showing the error!

Maybe add one assert to what you expect to be on the page for the rtl button to be working?

And one thing I think was mentioned before but may need @Wim Leers to confirm, the buttons shouldn't need to define an rtl version (especially the same ltr version) we should fallback to the ltr image if there is no rtl defined.

So pseudo simplified logic:

IF image_alternative THEN
   Language Direction is RTL AND image_alternative_rtl is true THEN
      ADD 'cke_rtl' class.
   ELSE 
     ADD 'cke_ltr' class.
ELSE IF image
   Language Direction is RTL AND image_rtl is not empty THEN
     SHOW rtl image
   ELSE 
     SHOW ltr <img>
ELSE 
   SHOW '?'

Just had a go on the test case again. This time I tried to add xpath() check to get the faulty image. Suggested by @joelpittet.

What I was trying to do is to get the IMG SRC from one of the images that comes in CKEditor toolbar. So I did the following,

public function testLanguageDirectionChange() {
    // Login with admin user.
    $this->drupalLogin($this->admin_user);

    // Install the Arabic language (which is RTL) and configure as the default.
    $edit = array();
    $edit['predefined_langcode'] = 'ar';
    $this->drupalPostForm('admin/config/regional/language/add', $edit, t('Add language'));

    $edit = array('site_default_language' => 'ar');
    $this->drupalPostForm('admin/config/regional/settings', $edit, t('Save configuration'));

    // Once the language is changed then goto CKEditor Full HTML configuration.
    $this->drupalGet('admin/config/content/formats/manage/full_html');

    // Check the RTL images are loaded in the page.
    $a = $this->xpath('//span[@class="cke_button_icon"]/img');
    $b = $this->xpath('//span/img');
    debug($a);
    debug($b);
  }
}

But I was not able to get the SRC attribute. @chx and @berdir helped me in this. While debugging, @berdir found out that the toolbar was not actually generated via HTML but with JavaScript. A code snippet looks like

a href=\u0022#\u0022 role=\u0022button\u0022 title=\u0022Image\u0022 aria-label=\u0022Image\u0022\u003E\u003Cspan class=\u0022cke_button_icon\u0022\u003E\u003Cimg src=\u0022http:\/\/drupal8.local\/\u0022 alt=\u0022\u0022 title=\u0022Image\u0022 \/\u003E\u003C\/span\u003E\u003C\/a\u003E\u003C\

So I removed the xpath checks and added assertPattern() instead.
This patch checks for if any *_rtl.png is present in the RAW HTML. But, if we follow the algorithm mentioned in #68 then this test case also needs alterations.
@joelpittet & @Wim Leers any thoughts on this one?

Created a patch considering @joelpittet's pseudo code from #68. The test doesn't assert anything though but gives warnings as before.

Logical code review needed.

Thanks!

For now, only a review of the test coverage — full review will come though!

1. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   + * Definition of \Drupal\ckeditor\Tests\CKEditorLanguageDirectionTest.
   

   s/Definition of/Contains/

2. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   +  protected function setUp() {
   

   Missing {@inheritdoc}

3. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   +    $full_html_format = entity_create('filter_format', array(
   ...
   +    $editor = entity_create('editor', array(
   

   Don't use entity_create(). Use FilterFormat::create(), Editor::create(), etc.

4. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   +    $full_html_format->save();
   ...
   +    $editor->save();
   

   You don't need to assign these newly created entities to a variable, since you're not using them anywhere anyway. And you can invoke ->save() upon them immediately.

5. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   +    ));
   +
   +  }
   

   Unnecessary newline.

6. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   +    // Login with admin user.
   

   This comment has a typo and can be removed.

7. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   +    // Once the language is changed then goto CKEditor Full HTML configuration.
   +    $this->drupalGet('admin/config/content/formats/manage/full_html');
   

   I don't see any actual tests here yet? :)

8. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,75 @@
   +  }
   +}
   

   There must be a newline after the last method's closing brace and the class's closing brace.

Thanks @Wim Leers, I'll work on the test again to fix these issues. Btw, please have a look at comment #70 for the test and why its not working with xpath() and images.

Let me know your thoughts on this.

Rerolled, fixed the language direction classes and added assertion for the test.

@lauriii, thanks.

Can you please provide us an interdiff? Also, have you addressed review comments by Wim Leers? Lets have a quick look into this. Such as not not using entity_create and using FilterFormat::create() methods.

I haven't had the time, but I will work on this weekend. :-)

I fixed the points pointed by Wim. It is not possible to create interdiff for reroll because the original patch doesnt apply

@lauriii, I think #78 is just a re-roll and you are working on the changes from #76?

I don't know what has happened there since im pretty sure I fixed those other points also.. Now they are there.

Here's some improvements, removed some extra bits that I expect aren't needed.

I still need to review the inside the macro with a bit more rigor but we'll see what testbot says here.
I tried to make the code about the language direction more clear and just refactor it a bit @aneek.

Also, due to how that language direction check for image works, we don't need to provide the exact same image to the 'image_rtl' key if it's the same as the 'image' key. Both with the way you had it and my refactor. So I removed those additions.

@joelpittet, the re-factor is good. I will also have a good look later on.

Thanks @aneek, this feels really close and a relief it is green:)

There is a few minor bits here that I missed on that go, you're welcome to roll them in on your review if you have a moment?

1. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,77 @@
   +   Editor::create(array(
   

   One space indent needed here that I missed.

2. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -3,7 +3,22 @@
   + * - button: An array containing the CKEditor buttons.
   

   Should probably read "A list" as we are purposefully avoiding data types in twig.

   https://www.drupal.org/node/1823416#datatypes

3. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -3,7 +3,22 @@
   + *   - language_direction_class: A toggling class for button spans.
   

   Don't think this exists anymore.

Thanks @lauriii for addressing the reviews. I had a quick look and it seems all are quite okay.
@joelpittet, should we wait for more reviews by others or can we make this RTBC?

Very sorry for the delay in reviewing this!

1. +++ b/core/modules/ckeditor/ckeditor.admin.inc
   @@ -60,54 +59,35 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
   +    // Check for the image button to be present and convert URI to URL.
   

   s/URI/file URI/

2. +++ b/core/modules/ckeditor/ckeditor.admin.inc
   @@ -60,54 +59,35 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
   -    return $button_item;
   +    return $button;
   

   The function name says it's for building a button item. This says it's a button.

   You probably want to update the function name to be $build_button() rather than $build_button_item().

3. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
   @@ -126,163 +126,164 @@ public function getConfig(Editor $editor) {
   -        'image_alternative' => $button('bold'),
   +        'image_alternative' => 'bold',
   
   +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -3,7 +3,21 @@
   + * @see http://twig.sensiolabs.org/doc/tags/macro.html
   

   From CKEditorPluginButtonsInterface::getButtons()'s documentation:

      *   - image_alternative: If this button does not render as an image, specify
      *     an HTML string representing the contents of this button.
   

   This is not an HTML string.

   You basically moved all of this into the Twig file. I didn't realize it before, but this is very problematic, because what you moved into the Twig template is not generically usable; it's only usable for buttons that are part of the CKEditor build!
   It's entirely possible that an additional CKEditor plugin is installed that also needs the ability to specify a HTML string to simulate its button in the admin UI, but because it's not built in to the CKEditor build that Drupal 8 ships with, it can't use this same mechanism, because it needs different HTML.

   However, I don't want to block the awesome work you've done here.

   So I think we should keep image_alternative</ code> as it works in HEAD, and then add a new <code>internal_button.

   So, from:

   'image_alternative' => 'bold',
   

   to:

   'internal_button' => 'bold',
   

   I hope that made sense.

4. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
   @@ -126,163 +126,164 @@ public function getConfig(Editor $editor) {
   +        'image_alternative_rtl' => TRUE,
   

   For the same reason, this his highly problematic. Would need to become internal_button_has_rtl.

5. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
   @@ -126,163 +126,164 @@ public function getConfig(Editor $editor) {
   -        'image_alternative' => '<a href="#" role="button" aria-label="' . t('Button separator') . '" class="ckeditor-separator"></a>',
   +        'image_alternative' => 'separator',
   +        'button_text' => t('Button separator'),
   +        'is_separator' => TRUE,
   
   +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/StylesCombo.php
   @@ -59,7 +59,9 @@ public function getButtons() {
   -        'image_alternative' => '<a href="#" role="button" aria-label="' . t('Styles') . '"><span class="ckeditor-button-dropdown">' . t('Styles') . '<span class="ckeditor-button-arrow"></span></span></a>',
   +        'button_text' => t('Styles'),
   +        'image_alternative' => 'styles',
   +        'is_dropdown' => TRUE,
   

   These are very much approximations of what the real deal looks like. Why can't these use image_alternative and pass in HTML, just like in HEAD?

6. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,77 @@
   + * Tests the button's display text or display image on CKEditor toolbar.
   ...
   +   * Tests loading of CKEditor CSS, JS and JS settings.
   

   These descriptions are wrong.

7. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,77 @@
   +    // Create a Full HTML filter format and associate this with CKEditor.
   

   Create text format, associate CKEditor.

8. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,77 @@
   +    ))->save();
   +
   +    Editor::create(array(
   

   Let's remove this empty line. That makes it more clear the comment above applies to both of these statements.

9. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -0,0 +1,77 @@
   +    // Once the language is changed then goto CKEditor Full HTML configuration.
   

   Once the default language is changed, go to the tested text format configuration page.

10. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
    @@ -3,7 +3,21 @@
    + * This template uses twig macro to create the CKEditor buttons.
    

    s/twig/Twig/
    s/to create the CKEditor buttons/to generate the HTML for internal buttons (those included with CKEditor) — we can't use regular image URLs here because these images are part of a sprite. By generating the correct HTML, we can use the images in that sprite, just like actual CKEditor instances./

Marking this issue as fixed. There is no raw HTML in text format setting pages, and it is working without applying any patch. May be some other commit have resolved this issue (I guess this one #2324371: Fix common HTML escaped render #key values due to Twig autoescape). Image attached for reference.

@subhojit777 thanks for testing, though this patch does a bit more than just fix the auto-escape issue so I think it still needs to be considered as we've been working hard to make this easier to use.

Needs an issue summary update though and may need to be split into a couple patches if the approach is not viable because there is a bug fix in there as well.

Thanks lauriii, much better already :) Almost there:

1. +++ b/core/modules/ckeditor/src/Tests/CKEditorLanguageDirectionTest.php
   @@ -69,7 +68,7 @@
   +    // Once the default language is changed, go to the tested text format configuration page. ¶
   

   Trailing space and 80 col rule.

2. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -3,12 +3,12 @@
   + * This template uses Twig macro to create the CKEditor buttons.
   

   s/uses Twig macro/uses a Twig macro/

3. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
   @@ -3,12 +3,12 @@
   + *   - internal_button: Indentifier of internal button.
   

   s/Indentifier/Identifier/ :)

   Should also include the text I proposed for the second substitution for point 10 in #91.

4. +++ b/core/modules/ckeditor/src/CKEditorPluginButtonsInterface.php
   @@ -45,10 +45,9 @@
   -   *   - image_alternative: If this button does not render as an image, specify
   -   *     an HTML string representing the contents of this button.
   -   *   - image_alternative_rtl: Similar to image_alternative, but a
   -   *     right-to-left version.
   +   *   - internal_button: Identifier of internal button.
   +   *   - image_internal_has_rtl: Flag indicating whether there's RTL version
   +   *     available.
   

   See #91.5. I think we may need to keep image_alternative around for that reason.

   (It is possible for a non-internal CKE button to not have an image but require an alternative HTML-based presentation, but with this change, you're saying that's only allowed for buttons included in our CKEditor build).

@Wim Leers Uploading patch with corrections you suggested in #99
@lauriii correcting your spelling mistakes :p

@subhojit777, thanks for the patch. I am not sure I am getting the following,

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
@@ -100,6 +100,7 @@ public function getConfig(Editor $editor) {
       'resize_dir' => 'vertical',
       'justifyClasses' => array('text-align-left', 'text-align-center', 'text-align-right', 'text-align-justify'),
       'entities' => FALSE,
+      'image_alternative' => '<a href="#" role="button" aria-label="' . t('Button separator') . '" class="ckeditor-separator"></a>',
     );

I don't see any implementation of this image_alternative in theme layer. I am a bit confused here. @Wim Leers, can you help me with understanding this implementation of image_alternative thing?

Thanks!

#103: From #91:

From CKEditorPluginButtonsInterface::getButtons()'s documentation:

   *   - image_alternative: If this button does not render as an image, specify
   *     an HTML string representing the contents of this button.

This is not an HTML string.

You basically moved all of this into the Twig file. I didn't realize it before, but this is very problematic, because what you moved into the Twig template is not generically usable; it's only usable for buttons that are part of the CKEditor build!
It's entirely possible that an additional CKEditor plugin is installed that also needs the ability to specify a HTML string to simulate its button in the admin UI, but because it's not built in to the CKEditor build that Drupal 8 ships with, it can't use this same mechanism, because it needs different HTML.

Thanks @Wim Leers, for the explanations. About the patch #101, just by adding RAW HTML to image_alternative won't serve the purpose. Just had a checked at the failed test results and in /core/modules/ckeditor/src/Tests/CKEditorTest.php @ line 98 the values which are being compared are not same in the variables $expected_config & $this->ckeditor->getJSSettings($editor). Please have a look at the pastes for more reference.
$expected_config
and
$this->ckeditor->getJSSettings($editor)

So based on the two debug dumps, we have a difference,

'image_alternative' => '<a href="#" role="button" aria-label="Button separator" class="ckeditor-separator"></a>'

We may need to address addition of "image_alternative" in a different way. I hope this may help to create a new patch. If I get some good time today or tomorrow I may make one. But other are welcome too !! :-)

EDIT #1
I think even we pass raw HTML, it should pass via SafeMarkup::checkAdminXss().

Thanks.

@Wim Leers @aneek is there any way I can run these tests locally?

Definitely, just enable the testing module in your local D8 installation. And then run the tests from configuration menu.

So something changed in this patch that allows image_alternative to make it into JS settings.

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
@@ -100,6 +100,7 @@ public function getConfig(Editor $editor) {
       'resize_dir' => 'vertical',
       'justifyClasses' => array('text-align-left', 'text-align-center', 'text-align-right', 'text-align-justify'),
       'entities' => FALSE,
+      'image_alternative' => '<a href="#" role="button" aria-label="' . t('Button separator') . '" class="ckeditor-separator"></a>',
     );
 
     // Add the allowedContent setting, which ensures CKEditor only allows tags

This is the culprit. It's simply a wrong hunk, and it causes the problem described in #105.

@Wim Leers I dont get this :S You have asked to add image_alternative in #99. So finally do we need image_alternative or not. Sorry if I am not being able to understand something straightforward.

While having a look at the code and the total thread, these following came to my mind.

1. In patch #101, the place where "image_alternative" is added is not quite right. Instead of adding it to getConfig() method it should be placed in getButtons() method. All these deals with CKEditorPluginButtonsInterface::getButtons() interface.

2. @Wim Leers, on your review #91, you suggested to change 'image_alternative' => 'bold', to 'internal_button' => 'bold', and to keep image_alternative to send HTML as is the HEAD version. The main idea was, if any external plugin supplies raw HTML image_alternative then this button needs to be printed. So I am guessing that internal_button only supplied via the internal buttons from Drupal core. If this is the idea then external plugins should not supply internal_button in their getButtons() methods.

So the template "ckeditor-settings-toolbar.html.twig" code may have,

{% if button.image_alternative %}
    {{ button.image_alternative }}
{# Buttons with text as labels#}
{% elseif button.internal_button %}
  ........

Regarding the language directions, if image_alternative has to be present then why we want to delete image_alternative_rtl. This is same as the image_rtl. External plugins may supply different class based HTML or different markups based on different RTL versions.

3. image_internal_has_rtl this key doesn't have any implementation in the twig template or in the preprocess function to generate the buttons. If we want to introduce new key then there should be one implementation of the key also. Isn't it?

4. While checking, I found out that the system deals with only "internal" & "stylescombo" as the internal plugin. The others that are shown in the toolbar is treated as external plugin like "drupalimage" or "drupallink" etc. It may also happen that other external plugins can supply "internal_button" key in their getButtons() method. So the name is quite confusing. Can it be changed to other like "text_button" or simply "default_button"? See, currently we have 3 types of buttons,

• image: This provides and image. RTL version available as image_rtl
• internal_button: This is currently used by internal CKEditor code. But not all plugins are considered as internal (Based on isInternal() method). So I suggest to change the name to another.
• image_alternative: This is again assigned for external plugin usage. But why image_alternative? Can't it be html_button? and html_button_rtl? Again just because of the name.

@Wim Leers, please suggest your thoughts. Also, love to hear from other contributes as well.

I'm not a professional on this one. This is the only issue I've been working on CKEditor. I'm sure that @Wim Leers is better one to answer most of the questions.

In my patch #92 I replaced image_alternative_rtl with image_alternative_has_rtl so they should have the same functionality. Anything haven't been removed at least intentionally.

I also think that button_internal is a bit confusing and we need to find something better for that.

It would be nice if someone could summarize the discussion here to the issue summary.

The patch removes multiple calls to SafeMarkup::set()method from template_preprocess_ckeditor_settings_toolbar() function.

Patch #101 doesn't apply now. Something has changed in ckeditor.admin.inc

actual tag has no dash.

Rerolled.

Rerolled

Created a followup #2409817: CKEditor toolbar configuration UI missing ending UL for the UL that was being fixed here but is really not part of this issue. It was in #101 and it was lost in #117. So we can just leave it out of this issue and fix it in the other.

+++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
@@ -3,16 +3,65 @@
- * - active_buttons: A list of active button rows.
+ * - active_buttons: A list of active button rows

This still needs a period at the end, not sure where that went from the reroll.

Fixed the corruption in #119 patch file.
Updated in according to comment #121.

New patch with some changes.

1. Changed image & image_rtl with image_button & image_button_rtl.
2. Added a new key text_button to denote the button will only supply text in the plugin file. (Internal.php). This is mainly renaming of internal_button key.
3. image_alternative is changed to html_button and also image_alternative_rtl to html_button_rtl. So html_button keys can be used by the external plugins which supply HTML to generate the buttons.

Sorry for the interdiff though. Can't generate it with patch #122.

Review!!

#2329781: Move CKEditor toolbar classes from preprocess to templates got in a few weeks ago, so the templates will at least need some rerolling.

Re-rolled. Uploading the patch here.

Patch rerolled.

Sorry. Added wrong related issue by mistake

there is overlap between this issue and #2501711: Remove or document SafeMarkup::set in template_preprocess_ckeditor_settings_toolbar

Ok that safe markup was committed today! We should see where this is at now. The template we were going to use a macro for was turned into an inline template, which is cool too. Maybe we can just re-roll and remove the macro bit and ensure we didn't miss anything that we've done in here that could be of use?

#137 sounds excellent!

@joelpittet, what are your thoughts with this patch now? Can you give me some details on this please. :)

Rerolling and following comments from #137. I hope this is what you meant.

I've compared with #2501711: Remove or document SafeMarkup::set in template_preprocess_ckeditor_settings_toolbar and all seems to be working except the the language direction change effects the image buttons. If the language is changed to RTL then image buttons disappear. Also, all the other buttons show inline texts inside them. Refer to the attached image.

To solve this I am opening a new issue, and we will try to close the above described there. If we can find we are missing something in here that was not committed with #2501711: Remove or document SafeMarkup::set in template_preprocess_ckeditor_settings_toolbar we can keep this open else I don't think we need this issue anymore. Please correct me if I am wrong.

Thanks.

@aneek++
@aneek++
@aneek++
@aneek++
@aneek++

@Wim Leers & @joelpittet do we need this issue any more? Can we close it?

I think we can mark it as a duplicate of #2563543: CKEditor Toolbar - various button issues when the language is RTL?

As discussed with @WimLeers, lets try to find anything from the existing patch (code and test cases) that we might have missed. Like the issue #2563543: CKEditor Toolbar - various button issues when the language is RTL. If we don't find any issues then lets make this issue a duplicate of #2563543: CKEditor Toolbar - various button issues when the language is RTL.

I'll have a go this weekend or may be soon. Others are also welcome!!

Thanks :)

@aneek Is there anything left to actually do here? AFAICT the bulk of the work was actually done in #2563543: CKEditor Toolbar - various button issues when the language is RTL, right? Hence marking this as minor.

And consequently, I think this can be deferred to 8.1.

@WimLeers, yup, I think we can better close this issue. A lot of new things were done in this issue but those are covered in more simpler ways in #2563543: CKEditor Toolbar - various button issues when the language is RTL & #2563543: CKEditor Toolbar - various button issues when the language is RTL. I also checked the test files in this issue. All seems to me is covered.

So, closing this issue as this is now duplicate of #2563543: CKEditor Toolbar - various button issues when the language is RTL & #2563543: CKEditor Toolbar - various button issues when the language is RTL

Thanks to all of the contributors in this issue. :-)

Hurray! :)