Problem/Motivation

When using a text filter with the filter_html filter (Limit allowed HTML tags and correct faulty HTML in the UI), which is used by the Basic HTML format — the default text format for authenticated users in Drupal 8, the link button does not show up in CKEditor.

That problem has been introduced by #2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats, which itself was a fix for #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored.

Steps to reproduce

  1. Install Drupal 8.
  2. Go to /node/add/article.
  3. Note that the "Link" button is missing.
  4. If you change the format from "Basic HTML" to "Full HTML", then the "Link" button does show up.
  5. The difference between those two formats:
    drupalSettings.editor.formats.basic_html.editorSettings.allowedContent.a === Object {attributes: "href,hreflang", styles: false, classes: false}
    

    vs.

    drupalSettings.editor.formats.full_html.editorSettings.allowedContent === true
    

Proposed resolution

TBD — see #23.

Remaining tasks

TBD

User interface changes

Link button is back in CKEditor + Basic HTML.

API changes

None.

Data model changes

None.

Why this should be an RC target:

A CMS unable to create links by default is plain stupid and hugely disruptive.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jaxxed created an issue. See original summary.

jaxxed’s picture

Here is a patch that I need just to get my sprint branch working before end of sprint. This is not likely a required fix, as it is most likely that I am just missing something in my settings.

marcingy’s picture

Version: 8.0.0-rc1 » 8.0.x-dev
Status: Active » Needs review

I can confirm this issue

marcingy’s picture

Priority: Normal » Major

This also seems like major as it basically prevents ckeditor being used in many contexts

slefevre1’s picture

Issue summary: View changes
slefevre1’s picture

Code sprint for Drupal Camp Ohio 2015.

slefevre1’s picture

Issue tags: +rc target triage
mandclu’s picture

Based on the documentation at http://docs.ckeditor.com/#!/api/CKEDITOR.style it seems that the style constructor only expects two parameters, so it makes sense that the inclusion of the styles broke the button. I would suggest that the patch be updated to remove the line completely (instead of commenting it out).

slefevre1’s picture

Cannot reproduce in drupal-8.0.0-rc1 following the steps initially reported. The link button appears no matter what HTML type I select.

slefevre1’s picture

Status: Needs review » Closed (cannot reproduce)
slefevre1’s picture

Status: Closed (cannot reproduce) » Needs review
slefevre1’s picture

Sorry, tested on wrong version.

slefevre1’s picture

Finished reviewing.

effulgentsia’s picture

Issue tags: -rc target triage +rc target

I don't think the patch is a correct fix, but if this issue was caused by a change introduced after RC (#2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats), then I think fixing it during RC is fine as well. I left a comment in that issue asking for reviews here.

xjm’s picture

(Agreed with this being an rc target.)

webchick’s picture

Confirmed the problem. The 'unlink' button appears, but is greyed out, and there is no 'link' button unless switching to Full HTML.

Since this is one of the most frequently-used pieces of HTML in a typical node body, this is definitely worthy of fixing in RC.

webchick’s picture

...in fact, we may want to rollback the other patch. From my understanding, that one only affects people who have dev tools enabled. This one affects all content authors no matter what their level of experience.

webchick’s picture

Title: CKEditor Link button does not show if HTML filtering is enabled » [regression] CKEditor Link button does not show if HTML filtering is enabled
Issue tags: +regression
Berdir’s picture

No, that other issue breaks JS and therefore ckeditor completely when it happens.

jaxxed’s picture

Issue summary: View changes

fixed some text-formatting issues with my initial post.

DuaelFr’s picture

Title: [regression] CKEditor Link button does not show if HTML filtering is enabled » [regression] CKEditor Link and Image button does not show if HTML filtering is enabled
Issue summary: View changes
FileSize
1.42 KB

After some investigation with @Haza and @WimLeers it turned out to be more critical that it first seemed to be.
All the plugins that uses drupal's Dialogs are now broken. Link, Image and custom/contrib ones are all concerned.

As this error affects all users I think we should rollback #2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats and try to find an other way to fix it.

DuaelFr’s picture

Title: [regression] CKEditor Link and Image button does not show if HTML filtering is enabled » [regression] CKEditor Link button does not show if HTML filtering is enabled
Issue summary: View changes

I was wrong. My cache misleaded me. That bug only affects the drupallink plugin what is still bad ;)

Wim Leers’s picture

I've called in the help of the CKEditor team. I really don't know how to debug this.

This is yet another regression caused by #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored. That caused an error message, which we fixed at[#2589779]. And the latter fix then apparently caused this.

(Did I mention we really need front-end testing?)


In summary, the situation, and what led to it:

  1. We changed the allowedContent + requiredContent definitions for the drupallink plugin from the string format to new CKEDITOR.style({…}).
  2. We did that, because it allows us to easily merge in additional allowed/required content when Drupal 8 contrib modules have plugins that extend those plugins.
  3. To the best of our knowledge, this triggers a bug in CKEditor itself.

Steps to reproduce:

  1. Install Drupal 8.
  2. Go to /node/add/article.
  3. Note that the "Link" button is missing.
  4. If you change the format from "Basic HTML" to "Full HTML", then the "Link" button does show up.
  5. The difference between those two formats:
    drupalSettings.editor.formats.basic_html.editorSettings.allowedContent.a === Object {attributes: "href,hreflang", styles: false, classes: false}
    

    vs.

    drupalSettings.editor.formats.full_html.editorSettings.allowedContent === true
    
Wim Leers’s picture

Issue summary: View changes

Moving the STR I added to #23 into the IS.

Cleaned up/revised the IS in general.

Wim Leers’s picture

Issue summary: View changes

Confirmed that this only happens when using the filter_html filter.

Curiously, if I override the allowedContent setting for the Basic HTML text format, by:

  1. Evaluating drupalSettings.editor.formats.basic_html.editorSettings.allowedContent.a.attributes = 'href,hreflang,style'; (to allow the style attribute) or even drupalSettings.editor.formats.basic_html.editorSettings.allowedContent.a = true; (to allow ANYTHING on the <a> tag)
  2. And then switching to Full HTML and then back to Basic HTML (to make that hacky override take effect)
  3. … then it still doesn't work
  4. Only doing drupalSettings.editor.formats.basic_html.editorSettings.allowedContent = true makes things work.

This strongly suggests that there is indeed a bug in CKEditor: CKEditor can short-circuit the initializing of ACF when it sees allowedContent === true, which is probably why this problem doesn't appear in that case. But in any other case, this seems to happen.


@CKEditor team: http://docs.ckeditor.com/#!/api/CKEDITOR.filter.allowedContentRules clearly states:

Virtual class which is the Allowed Content Rules formats type.

Possible formats are:

  • the string format,
  • the object format,
  • a CKEDITOR.style instance – used mainly for integrating plugins with Advanced Content Filter, an array of the above formats.

But then if you follow the last link, there is no example at all. I think that would be very helpful.

Wim Leers’s picture

Issue tags: +Needs upstream bugfix

Now officially confirmed to be a CKEditor bug: https://dev.ckeditor.com/ticket/13886.

Wim Leers’s picture

Now awaiting word from the CKEditor team when this will be fixed. If a fix will not be available in the next few days, I think we'll need to commit a work-around until it's fixed upstream; we really can't ship Drupal 8.0.0 like this, and IMO we also cannot ship RC3 like this.

I'm tempted to mark this critical.

Wim Leers’s picture

Looking at CKEditor's test checkelementremovable.js, it looks like if attributes is specified, that you should use style, not styles.

E.g.:

var style = new CKEDITOR.style( { element: 'span', attributes: { lang: 'pt' }, style: { color: '#ffffff' } } );

I can confirm that changing it from styles to style here makes things work.

Wim Leers’s picture

Title: [regression] CKEditor Link button does not show if HTML filtering is enabled » [PP-1] [regression] CKEditor Link button does not show if HTML filtering is enabled
Status: Needs review » Postponed
Related issues: +#2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI

So, there definitely is a bug in CKEditor. We shouldn't be seeing this problem.

But… there through an unfortunate series of events, the problem described in this issue happened. And thanks to #2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI, this actually becomes simpler to resolve. Let me explain.

#2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored
This added:
var allowedContentDefinition = {
  element: 'img',
  attributes: {
    '!data-entity-type': '',
    '!data-entity-uuid': ''
  }
};
if (widgetDefinition.allowedContent.img.styles) {
  var imgStyles = widgetDefinition.allowedContent.img.styles.split(/\s*,\s*/);
  for (var j = 0; j < imgStyles.length; j++) {
    allowedContentDefinition.styles[imgStyles[j]] = '';
  }
}
Note how allowedContentDefinition does not have a styles key, yet the second code snippet assumes it exists anyway. This was the problem that was then solved in the next patch, which was…
#2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats
This added the missing styles key, so that we ended up with:
var allowedContentDefinition = {
  element: 'img',
  styles: {},
  attributes: {
    '!data-entity-type': '',
    '!data-entity-uuid': ''
  }
};
But this then in turn uncovered a subtle bug in CKEditor: https://dev.ckeditor.com/ticket/13886.
However! Turns out that the original issue (#2579979) caused other regressions too…
#2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI
This fixes those other regressions. And in doing so, it significantly simplifies what the original issue (#2579979) did. In fact, now this bit of code is gone:
if (widgetDefinition.allowedContent.img.styles) {
  var imgStyles = widgetDefinition.allowedContent.img.styles.split(/\s*,\s*/);
  for (var j = 0; j < imgStyles.length; j++) {
    allowedContentDefinition.styles[imgStyles[j]] = '';
  }
}
… and because that code is gone, there is no more need for us to specify the styles key; this snippet of code is why we added it!
Conclusion
As soon as #2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI is committed, we can revert #2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats!

Turns out #21 actually does exactly that, so… I can RTBC this issue!

But, first #2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI must land, so postponing on that.


Note that @reinmar and @oleq were crucial in this research, so please credit them also. It'd be nice to credit me too.

Wim Leers’s picture

Title: [PP-1] [regression] CKEditor Link button does not show if HTML filtering is enabled » [regression] CKEditor Link button does not show if HTML filtering is enabled
Status: Postponed » Reviewed & tested by the community
Issue tags: -Needs upstream bugfix
FileSize
1.41 KB

#2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI landed. This is now unblocked. But #20 doesn't apply anymore. Rerolled. Note that it is still fundamentally the same patch: removing the styles key from all places that #2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats added it to. I also did another round of manual testing. Which is why I'm fine with keeping this at RTBC (it was first RTBC'd in #29, I only did a straight reroll of it).

Also removing Needs upstream bugfix, since that's no longer true per #29.

Wim Leers’s picture

To show that the CKEditor team helped me reach this conclusion, and especially @Reinmar:

[03/11/15 17:40:06] Piotrek Koszuliński: I’m also curious if that’s needed now, after those other changes that were RTBC-ed today.
[03/11/15 17:40:31] Piotrek Koszuliński: There’s a chance that it’ll all solve itself automatically.
[03/11/15 17:40:51] Wim Leers: > I’m also curious if that’s needed now, after those other changes that were RTBC-ed today.
Which changes are you thinking about?
[03/11/15 17:42:06] Piotrek Koszuliński: https://www.drupal.org/node/2585173
[03/11/15 17:42:15] Wim Leers: okay
[03/11/15 17:42:21] Wim Leers: I'll apply that and see if that also fixes it.
[03/11/15 17:43:35] Piotrek Koszuliński: although… none of your plugins use a float style ;| OK, so I’ve got another idea - it might have been thrown from image2. Perhaps it tries to extend the widgetDefinition with some additional things.
[03/11/15 17:44:27] Piotrek Koszuliński: https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/image2/plugin.js#L946
[03/11/15 17:44:44] Piotrek Koszuliński: Yeah… it makes sense.
[03/11/15 17:45:29] Piotrek Koszuliński: So basically you need the styles property to be set to some value which won’t throw when doing `value.float = ‘sth’;`
[03/11/15 17:45:50] Piotrek Koszuliński: empty string, as Olek proposed, definitely makes sense.
[03/11/15 17:47:21] Wim Leers: But empty string violates what CKEDITOR.style actually expects, doesn't it?
[03/11/15 17:49:29] Wim Leers: And yep, that's exactly it
[03/11/15 17:49:43] Wim Leers:           var imgStyles = widgetDefinition.allowedContent.img.styles.split(/\s*,\s*/);
[03/11/15 17:49:51] Wim Leers: that gets the float you mentioned from image2
[03/11/15 17:50:06] Wim Leers: and then it tries to set it on the CKEDITOR.style instance I mentioned above.
[03/11/15 17:50:08] Wim Leers: which then fails
[03/11/15 17:51:43] Piotrek Koszuliński: Aren’t both values your? I mean widgetDefinition.allowedContent is set by your plugin and that CKEDITOR.style instance is create by your plugin. So if I understand correctly, your plugin should check if that style property doesn’t exist yet and if so, then set it.
[03/11/15 17:52:41] Wim Leers: No, this is where we *transform* image2's widgetDefinition.allowedContent to more extensible data structures
[03/11/15 17:52:48] Wim Leers: So this is where we *import* the values from image2
[03/11/15 17:52:58] Wim Leers: ah
[03/11/15 17:53:01] Wim Leers: but that's a good point of course
[03/11/15 17:53:34] Piotrek Koszuliński: So it’s my blind guess, but:

1. drupalimage creates some widgetDefinition.allowedContent object
2. drupalimagecaption reads widgetDefinition.allowedContent. Checks if the styles property exists. If not, creates it. And then does the rest.
[03/11/15 17:53:41] Wim Leers: So, you're saying:
1. we can't set styles: {} because that hits a bug in CKEditor
2. so we must set styles: {SOMETHING} as soon as we have a need
[03/11/15 17:54:00] Wim Leers: hehe my #2 is your #2 basically, but our #1 is different
[03/11/15 17:54:30] Piotrek Koszuliński: yes, as soon as you need the styles property, set it. But not earlier.
[03/11/15 17:54:49] Piotrek Koszuliński: OK, I gtg now, but I’ll be available later.
[03/11/15 17:55:06] Piotrek Koszuliński: I hope that we are on the right track :)
[03/11/15 17:55:57] Wim Leers: Yep, that works:
           for (var j = 0; j < imgStyles.length; j++) {
+            if (!allowedContentDefinition.hasOwnProperty('styles')) {
+              allowedContentDefinition.styles = {};
+            }
             allowedContentDefinition.styles[imgStyles[j]] = '';
           }
[03/11/15 17:56:12] Wim Leers: Now seeing if that other patch actually fixes all of this in the process of cleaning it up :)
…
[03/11/15 18:31:53] Wim Leers: Piotrek: https://www.drupal.org/node/2598070#comment-10523740
[03/11/15 18:31:55] Wim Leers: :)
[03/11/15 18:33:30] Wim Leers: That is quite the clusterfuck
[03/11/15 18:34:11] Wim Leers: Caused by lack of front-end testing in Drupal, confusing API in CKEditor, very subtle bug in CKEditor, and especially a most unfortunate combination of circumstances.
DuaelFr’s picture

Awesome!
+1 RTBC :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, that's quite a tiny fix for such a big problem. I can totally see how folks got confused.

I'm quite satisfied that people who know more about JS in a tiny hair than I will in my entire life are good with this, and it'd be great to get widespread end-user testing on RC3, so let's get this in.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 61583a6 on 8.0.x
    Issue #2598070 by Wim Leers, jaxxed, DuaelFr, Haza, Reinmar, oleq: [...

Status: Fixed » Closed (fixed)

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

iainH’s picture

Status: Closed (fixed) » Active

This is still an issue in Drupal 8.0.5.

By default, moving the Underline button into the toolbar will not enable the Underline button in the Basic CKEditor config. It just doesn't show up in the user's toolbar.

Un-checking "Limit allowed HTML tags and correct faulty HTML" in the Basic CKEditor config allows users of the Basic config to see the Underline button.

Wim Leers’s picture

Status: Active » Closed (fixed)

Please open a new issue rather than reopening one from half a year ago. Thanks :)

iainH’s picture

Wim Leers’s picture

Note that the regression was now fixed: http://dev.ckeditor.com/ticket/13886. Will be in CKEditor 4.5.9. Should we bring back styles: {}, which was removed in this issue? (See committed patch in #30.)

DuaelFr’s picture

#39: I think we should get that styles back so this is leaner for contrib modules to extend the allowed styles (because they don't have to check if the object already exist before adding something inside).

mlewand’s picture

#39 @DuaelFr made a valid point here, it will be easier to extend, while being slightly more explicit.

Wim Leers’s picture

Status: Closed (fixed) » Active

Alright. @DuaelFr, you want to open an issue for that?

(Temporarily making this issue active again so we don't lose track.)

DuaelFr’s picture

@Wim Leers: I can't before May 1st so do not hesitate to leave me behind this time ;)

xjm’s picture

Status: Active » Fixed

When we mark issues back to open, contributors are no longer credited for them, and it gets really confusing.

Marking fixed for now to let it re-auto-close in two weeks. Please just create any needed followups though, even if it's just a stub.

DuaelFr’s picture

Wim Leers’s picture

Thanks! And sorry, @xjm.

Wim Leers’s picture

Status: Fixed » Closed (fixed)

Accelerating what d.o would do in a few weeks for us.

ec-adam’s picture

This issue is back for me in 8.1.9

There are no link buttons in the basic html editor unless I uncheck Limit Html. Having custom Styles doesn't matter either way. I've tried removing the link buttons and re-adding them in other groups and everything else I can think of.

ec-adam’s picture

Version: 8.0.x-dev » 8.1.x-dev
kala4ek’s picture

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

Yep, the same on 8.2.5

rwam’s picture

With Vs. 8.2.7 I have working link buttons, but I cannot use the alignment buttons. Only with deactivated filter they are visible and usable. See for a similar issue and a description at #2766105

EDIT: I've found the solution. See my comment. With the correct filter settings it works.