Color button gets disabled when foreStyle configured to use classes

[f1ames]

Type of report

Bug

Provide detailed reproduction steps (if any)

See https://codepen.io/f1ames/pen/jObMrpr for easy reproduction.

1. Configure color button with

colorButton_foreStyle: {
  element: 'span',
  attributes: { 'class': '#(color)' },
  overrides: [ {
    element: 'span',
    attributes: {
      'class': /.text/
    }
  } ]
}

2. Select some text inside editor.

Expected result

Color button is enabled and it's possible to apply colors.

Actual result

Color button is disabled due to ACF not allowing span.class (most probably).

Other details

Adding extraAllowedContent: 'span(*)' solves this issue. My assumption is that color button should take care of this automatically based on colorButton_foreStyle setup.

Looks like it's regression related to #3748 and #3727 (so #3405 PR).

• Browser: All (tested on Chrome)
• OS: All (tested on Linux Mint)
• CKEditor version: 4.14.0
• Installed CKEditor plugins: colorbutton

UPDATE

We agreed that the issue can be easily mitigated by correct ACF rules configuration and only requires docs update to make it more obvious. See the discussion below for more details.

[jacekbogdanski]

I'm not sure if it's a regression. Indeed, button is currently disabled with the config you provided. However, it's disabled due to improvements we did into colorbutton to correctly recognize its state. However, if you verify how it worked previously, ACF still removes added spans with classes that have not been added to the ACF itself.

I'm wondering if using this option should really change ACF filters out of the box 🤔 It makes sense, but on the other side it gives more control (e.g. when talking about security) if you directly tell what rules should be enabled when using such custom style definition like the one in your example (custom classes for span elements). Maybe the issue could be resolved by docs update?

[f1ames]

I'm not sure if it's a regression. Indeed, button is currently disabled with the config you provided. However, it's disabled due to improvements we did into colorbutton to correctly recognize its state. However, if you verify how it worked previously, ACF still removes added spans with classes that have not been added to the ACF itself.

The regression is the fact that the button is disabled in such cases as it was previously enabled. However, OTOH, the fact that it recognizes the state correctly and takes ACF into account means that now it works as it should (so it was the invalid behavior previously). So it is in fact an improvement as you mentioned :thinking: 

The fact that ACF still removes added spans means that probably nobody used it without proper ACF rules (because spans will be removed on e.g. `getData()`) so this change shouldn't break any existing editor integrations. That being said, it seems reasonable to keep this behavior as is and update documentation to mention ACF.

I'm wondering if using this option should really change ACF filters out of the box It makes sense, but on the other side it gives more control (e.g. when talking about security) if you directly tell what rules should be enabled when using such custom style definition like the one in your example (custom classes for span elements).

That's another story I suppose (there is separate issue for that #3996). You can always use extraAllowedContent or disallowedContent to adjust ACF in both ways. The question is which should be the default behavior - the more explicit ACF configuration requirements or a little smarter features working smoothly with ACF.

As it works like that for some time I'm not really pushing to change this behavior, especially that we don't have any issues reported related to this case. The only inconsistency we have is that it works fine with default config but when config is customized you need some additional ACF stuff which may be confusing.

[jacekbogdanski]

Ok, so basically seems like we both agree on that this issue could be simply resolved by updating docs instead.

[f1ames]

Fixed with caa0461.