Allow dashes in Styles dropdown's element names

I'm working on this at DrupalCon.

I mentored @mttsmmrssprks working on this issue at DC Vienna. Thanks Matt for working on this!

I need to leave DrupalCon now but will pick this up and test the patch from home this weekend.

tested the patch successfully against 8.5.x (see screenshot)

Thanks for creating this issue. Makes sense! 👍

Before this can be committed, this needs test coverage. You can expand the existing test coverage at \Drupal\Tests\ckeditor\Kernel\CKEditorTest::testStylesComboGetConfig().

added test

👍

1. +++ b/core/modules/ckeditor/tests/modules/src/Kernel/CKEditorTest.php
   @@ -346,6 +346,11 @@ public function testStylesComboGetConfig() {
   +    $settings['plugins']['stylescombo']['styles'] = "  p .test-class   |  classwithDash ";
   

   Nit: Let's add a comment above this, to be consistent with the existing tests. Something like:

   // Configuration that includes a dash in the class name.
   

2. Can you also upload a test-only patch? The test-only patch should fail. That will then prove that there is test coverage proving that it didn't work before, and does work after.

Added comment and uploaded the test-only patch

Perfect!

The test-only patch doesn't fail, so it looks like the testcoverage is not sufficient.

#15++ — I RTBC'd before the test results were available, I shouldn't have done that!

Wim--

improved on the test coverage. Please find the latest patch

The test-only patch is still passing, which means the test coverage isn't strict enough yet.

mistakely upload the wrong patch

hope this test fails

test only patch uploaded in #25 and this is the final patch with test

This patch still only needs to have a test-only patch that fails, and a patch that passes. It's so close! :)

test failed as expected

Thanks, @b_sharpe!

The test-only patch is updating both \Drupal\Tests\ckeditor\Kernel\CKEditorTest and \Drupal\Tests\ckeditor\Functional\CKEditorStylesComboAdminTest. But only the latter is failing. I thoroughly reviewed the latter, the changes to that one are correct.

So, back to NW for CKEditorTest, sorry!

Have the same problem as well.

Only patch re-roll to 8.8.x

This patch should fail.
This means that the patch in #43 works.

Patch #2911527-43: Allow dashes in Styles dropdown's element names works

Also tested the patch in #43. Looks good and @vacho solved the issue.

@Waldoswndrwrld It looks like you used underscores instead of hyphens in your patch's name. "911527_44_test_only.patch" instead of "test-only-911527-44.patch". I believe that was triggering the automatic setback to Needs Work.

I'm just re-uploading @vacho's patch in #43 and setting to RTBC.

Thanks, #44 indeed proves that #36 was addressed: both updated tests are failing without the fix!

Setting back to Needs Review because above test fails are unrelated.

I could be wrong, but I don't think there is an issue here. My classes have hyphens and it is passing validation.

My configuration under "Styles dropdown" (Full HTML):

div.flex-video|Flex Video
div.content-alert-box|Box
ul.inline-list|Inline list
div.small-block-grid-1.medium-block-grid-2|Block grid 2
div.small-block-grid-2.medium-block-grid-3|Block grid 3
div.small-block-grid-2.medium-block-grid-4|Block grid 4

The configuration saves successfully and the styles appear in the dropdown.

I think test-class|test-class fails because it is missing an element. It passes with div.test-class|test-class. The second- test class|test class- fails because spaces aren't allowed. test.class|test class and div.test-class|test class both pass.

This seems to conform to the required syntax:

Enter one or more classes on each line in the format: element.classA.classB|Label. Example: h1.title|Title. Advanced example: h1.fancy.title|Fancy title.

Drupal version 8.7.4

One more thing: 'testclass|testclass' passes validation but 'testclass' is assumed to be an element, not a class. The resulting HTML with this configuration is:

<p><testclass>Suspendisse potenti</testclass></p>

So I think this is all functionally correct but perhaps the description and (certainly) the validation messages could be clearer.

Attached patch modifies patch from #43 with the following modifications:

• Escapes hypen in regex
• Updates regex in ckeditor.stylescombo.admin.es6.js and ckeditor.stylescombo.admin.js
• Addresses a code standard issue in CKEditorTest.php

@othermachines - Classes with hyphens are already allowed. This issue seeks to allow elements with hyphens, such as <drupal-entity>.

Correcting title. Removing old tags.

It looks like this seems to partially solve the problem?

It fixes the issue where I cannot submit the form where styles have hyphens in them, however they dont seem to apply to media entities... Not sure if this is an issue with the hyphens patch or that I'm trying to use it with media entities?

I'm using this style.
drupal-media.float-right|Float Right

It appears in the styles dropdown (see attached) when I select the Media Entity... however it does not seem to apply.

@nicholasThompson, the issue you're experiencing is #3117172: Allow CKEditor styles for <drupal-media>. I'm adding it as a related issue.

I'm ignoring #57 here. Rerolled #48. What else is left here?

I'm ignoring #57 here.

@jibran - An explanation would be helpful.

I've reviewed #62 and #57. Even though hyphens are special characters, they don't need escaped in the given syntax, so those changes from #57 can be omitted.

The coding standards issue remains:

--- a/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php
+++ b/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php
@@ -369,6 +369,14 @@ public function testStylesComboGetConfig() {
     $editor->save();
     $expected['stylesSet'] = FALSE;
     $this->assertIdentical($expected, $stylescombo_plugin->getConfig($editor), '"StylesCombo" plugin configuration built correctly for customized toolbar.');
+
+    // Configuration that includes a dash in either the element or class name.
+    $settings['plugins']['stylescombo']['styles'] = "drupal-entity.has-dashes|Allowing Dashes";
+    $editor->setSettings($settings);
+    $editor->save();
+    $expected['stylesSet'] = [['name' => 'Allowing Dashes', 'element' => 'drupal-entity', 'attributes' => ['class' => 'has-dashes']]];
+    $this->assertIdentical($expected, $stylescombo_plugin->getConfig($editor), '"StylesCombo" plugin configuration built correctly for customized toolbar.');
+
   }

FILE: ...-styles-hyphen/web/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php

...
 377 | ERROR   | [ ] If the line declaring an array spans longer than 80
     |         |     characters, each element should be broken into its own
     |         |     line
...

It's specifically this line:

 $expected['stylesSet'] = [['name' => 'Allowing Dashes', 'element' => 'drupal-entity', 'attributes' => ['class' => 'has-dashes']]];

Other than that, I think this patch is good for RTBC.

New changes has been applied as suggested by @Chris Burge in #63

@jibran - An explanation would be helpful.

Sorry, I didn't realize #48 was reuploaded version on #43. I should have started from #57 :)

The coding standards issue remains:

There is nothing reported in https://www.drupal.org/pift-ci-job/1751295. And now #65 has a lot of unwanted changes. :(

It looks like a plethora of coding standards were fixed in #65, which, while desirable, is out of scope for this issue.

Attached is an updated patch that builds on #62. Only coding standards within the patch itself are addressed.

(Re coding standards, I'm using Coder, which provides Drupal coding standards for PHP_CodeSniffer.

@Chris: I have applied patch #69 and its working as expected.
I am able to add more style in editor settings and can use it in the content editor without any error.

Drupal ver: 9.1.x
Maradb ver: 10.3.x
Php ver: 7.3

RTBC

@Manav - Thanks for testing and reviewing

#73 was the result of an unrelated branch test failure. Tests passed on a subsequent run. Setting back to RTBC.

Saving issue credit.

Committed 0c9c57f and pushed to 9.1.x. Thanks!

I don't think we need a CR here - I think people would expect this to work.

This patch provides a class and element but doesn't work for selected drupal-media or drupal-entity elements in my case. Perhaps I need to test with a vanilla 9.1 fresh install

see screencast:

Is there another patch somewhere to make this work for drupal-media elements or drupal-entity elements selected as per screencast?

Note: I also have tried with a drupal-entity element
no dice except to make an empty element, selecting a real one and applying the style does not do anything

 <drupal-entity class="seven-hundred">
  </drupal-entity>

  <drupal-entity alt="loft window" data-embed-button="media_browser" data-entity-embed-display="media_image" data-entity-embed-display-settings="{&quot;image_style&quot;:&quot;&quot;,&quot;image_link&quot;:&quot;&quot;}" data-entity-type="media" data-entity-uuid="83949e37-eb0b-455a-a9d0-3b76a9d2f19a" data-langcode="en" title="house construction 2009">
  </drupal-entity>

I wanted to get class="seven-hundred" on the actual embedded drupal-entity element but it take it.

similar result with drupal-media , class is not applied to the selected item

**EDIT**

I have found something promising perhaps:

https://www.drupal.org/forum/support/post-installation/2018-08-31/how-to...

however I haven't tested it fully yet.
**EDIT**