Problem/Motivation

In CKEditor, if...

  1. You're using the styles dropdown
  2. You have a style with with multiple classes, then

Then...

This works: p.a-test.b-test|Test
This doesn't: p.b-test.a-test|Test

Because "a" lexicographically comes before "b". A style won't work if its classes aren't sorted alphabetically. This can be a real puzzler, because, well, yeah.

Steps to reproduce:

  1. Add a new text format
  2. Configure it to use CKEditor
  3. Add "Styles" button to your "Active Toolbar"
  4. Add this "Styles dropdown" entry: p.b-test.a-test|Test
  5. Enable "Limit allowed HTML tags and correct faulty HTML"
  6. Under "Allowed HTML tags", enter <p class="b-test a-test"> (it might get entered automatically)
  7. Go to create or edit content with a Body field or other long formatted text field
  8. Switch field's "Text format" to the format you created
  9. Enter some text (it generates a <p> tag) and see that Styles dropdown is greyed out
  10. Change step 4's "Styles dropdown" entry to p.a-test.b-test|Test (note the alphabetical ordering)
  11. Repeat steps 7-9 - this time the Styles dropdown works!?

Proposed resolution

Upstream bugfix.

Remaining tasks

Upstream bugfix.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#25 stylescombo_plugin_not_listed_alphabetically-2710431-25.patch761 bytesmatthiasm11
#24 2710431-24.patch675 bytesweseze
#23 2710431-23.patch685 bytesweseze
#10 CKE stylescombo reverse alphabetical classes — FAILS.png10.67 KBWim Leers
#10 CKE stylescombo alphabetical classes — works.png10.7 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

charginghawk created an issue. See original summary.

charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Genuine commented
Issue summary: View changes
charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Genuine commented
Title: Styles Dropdown Entry with Multiple Classes Broken when "Limit allowed HTML tags" Enabled » Styles Dropdown Sometimes Broken when "Limit allowed HTML tags" Enabled
Issue summary: View changes
charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Genuine commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2710427: Broken "Allowed Tags" updating: after all values for an attribute are allowed, it should not be overridden to allow only certain attribute values
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs upstream bugfix

WTF, that sounds really bizarre. I'll need to try to reproduce this. This sounds like a bug in CKEditor.

We'll need to narrow it down further to the smallest possible case that still reproduces this.

To clarify: step 11 means this problem is happening randomly?

charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Genuine commented

Not random - it's based on whether the classes are sorted alphabetically.

This works: p.a-test.b-test|Test
This doesn't: p.b-test.a-test|Test

Because "a" lexicographically comes before "b". In my case, I had a.o-button.c-button|Link, and I had to change it to a.c-button.o-button|Link.

I suspect what's going on is there's a sort somewhere. In js, the greater than operator can be used on strings, so maybe that's what's up.

Daltyn’s picture

Daltyn CreditAttribution: Daltyn commented
Version: 8.1.0 » 8.1.7

I can confirm this is an issue. The following settings work

a.button|Button
a.beige.button|Beige Button
a.button.expanded|Expanded Button
a.beige.button.expanded|Beige Expanded Button

These do not.

a.button|Button
a.button.beige|Beige Button
a.button.expanded|Expanded Button
a.button.beige.expanded|Beige Expanded Button

charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Genuine commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Styles Dropdown Sometimes Broken when "Limit allowed HTML tags" Enabled » [upstream] Styles Dropdown fails when using multiple classes, and they're not listed alphabetically
Version: 8.1.7 » 8.2.x-dev
Assigned: Unassigned » mlewand
FileSize
CKE stylescombo alphabetical classes — works.png10.7 KB
CKE stylescombo reverse alphabetical classes — FAILS.png10.67 KB

Reproduced. WTF! So weird!

This is then also confirmed to be an upstream bug. Assigning to the CKEditor team.

  • This works:
  • This does not work (when the cursor is on a paragraph, you cannot select this style):
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes

Simplified the steps to reproduce and the issue summary.

Tade0’s picture

Tade0 CreditAttribution: Tade0 at CKSource commented

I managed to reproduce this bug in a standalone CKEditor instance, so this is definitely a CKEditor issue. Preparing a fix for this.

EDIT: you can track the progress of this fix here: https://dev.ckeditor.com/ticket/14909.

charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Genuine commented

Tade0's linked ticket was closed as a duplicate of: https://dev.ckeditor.com/ticket/13206

When the classes are not sorted alphabetically (in the string), it will not be made available because it is deemed not "applicable", because it does not pass the test in objectCompare(element.attributes,clone.attributes).

Example: a style in stylesset with the following definition: {name:'Multi Class', element:'p', attributes:{class:'c b a'}}

Will not be selectable from the combo because when a clone is created from the element and then compared, the compared class-string is ordered alphabetically 'a b c';

1652 core/filter.js/processElement/updateElement loops through all possible classnames and then sorts them on line 1658 classesArr.sort().join( ' ' );

140 stylescombo/plugin.js style.checkApplicable() 366 core/style.js checkApplicable/filter.check() 744 core/filter.js CKEDITOR.toosl.objectCompare()

thus the object comparison fails...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kkri’s picture

kkri CreditAttribution: kkri at Liip for FREITAG lab. AG commented

This is still an issue as of today, and I would like to share my encounter with this bug.

Here's an other example with special characters that won't work:
a.btn.btn--link.b-color| CTA Extern
Dashes are for some reason considered to be in front of the letters in the alphabet, so I had to move the class "b-color" to the front.
a.b-color.btn.btn--link| CTA Extern

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [upstream] Styles Dropdown fails when using multiple classes, and they're not listed alphabetically » [upstream] StylesCombo plugin fails when using multiple classes, and they're not listed alphabetically
Status: Active » Postponed

Now on GitHub: https://github.com/ckeditor/ckeditor-dev/issues/727.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: mlewand » Unassigned
Status: Postponed » Active
Issue tags: -Needs upstream bugfix
Related issues: +#2999691: Update CKEditor library to 4.10.1

See #2999691-4: Update CKEditor library to 4.10.1, this has been fixed in CKEditor 4.10.1! Please test that patch to verify this has been fixed :)

weseze’s picture

weseze CreditAttribution: weseze as a volunteer commented

This bug is still present in ckeditor 4.10.1. Jus tested on D8.6.4 with ckeditor 4.10.1 included.

weseze’s picture

weseze CreditAttribution: weseze as a volunteer commented

It also seems that (for us at leaset) this problem only started after we update to a Drupal version which had ckeditor 4.10.1. Before it did work properly.

weseze’s picture

weseze CreditAttribution: weseze as a volunteer commented
FileSize
2710431-23.patch685 bytes

Attached is a patch that works around the issue by sorting the classes before passing them to drupalSettings.

weseze’s picture

weseze CreditAttribution: weseze as a volunteer commented
FileSize
2710431-24.patch675 bytes

Small corretion (still had my local paths in the patch)

matthiasm11’s picture

matthiasm11 CreditAttribution: matthiasm11 at Randstad Digital commented
FileSize
stylescombo_plugin_not_listed_alphabetically-2710431-25.patch761 bytes

Although I think this should be fixed in CKeditor (https://github.com/ckeditor/ckeditor-dev/issues/2578), I'm attaching an improved patch because this will speed up fixing the issue in Drupal.

  • Fixed the notice Only variables should be passed by reference caused by referencing a function instead of an array in the sort function.
  • Fixed the sorting return value. The sort function works by reference, it does not return an array but a boolean.
matthiasm11’s picture

matthiasm11 CreditAttribution: matthiasm11 at Randstad Digital commented
Status: Active » Needs review

The last submitted patch, 23: 2710431-23.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 25: stylescombo_plugin_not_listed_alphabetically-2710431-25.patch, failed testing. View results

weseze’s picture

weseze CreditAttribution: weseze as a volunteer commented

I don't think we should include the patch core actually. It changes the order in the list of classes and that might lead to unexpected results. The actual fix should be in ckeditor, comparing classes alphabetically but applying them as given.

The patch here should only be used to quickly work around the issue in cases that allow it.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Project: Drupal core » CKEditor 4 - WYSIWYG HTML editor
Version: 9.4.x-dev » 1.0.x-dev
Component: ckeditor.module » Code

CKEditor has been removed from core, CKEditor 4 is removed from Drupal Core in 10.0.0