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
- Install Drupal 8.
- Go to
/node/add/article
. - Note that the "Link" button is missing.
- If you change the format from "Basic HTML" to "Full HTML", then the "Link" button does show up.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#30 | rollback-2598070-30.patch | 1.41 KB | Wim Leers |
Comments
Comment #2
jaxxed CreditAttribution: jaxxed at Wunder commentedHere 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.
Comment #3
marcingy CreditAttribution: marcingy at Examiner.com commentedI can confirm this issue
Comment #4
marcingy CreditAttribution: marcingy at Examiner.com commentedThis also seems like major as it basically prevents ckeditor being used in many contexts
Comment #5
slefevre1 CreditAttribution: slefevre1 commentedComment #6
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedCode sprint for Drupal Camp Ohio 2015.
Comment #7
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedComment #8
mandclu CreditAttribution: mandclu at Northern Commerce commentedBased 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).
Comment #9
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedCannot reproduce in drupal-8.0.0-rc1 following the steps initially reported. The link button appears no matter what HTML type I select.
Comment #10
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedComment #11
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedComment #12
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedSorry, tested on wrong version.
Comment #13
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedFinished reviewing.
Comment #14
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #15
xjm(Agreed with this being an rc target.)
Comment #16
webchickConfirmed 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.
Comment #17
webchick...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.
Comment #18
webchickComment #19
BerdirNo, that other issue breaks JS and therefore ckeditor completely when it happens.
Comment #20
jaxxed CreditAttribution: jaxxed at Wunder commentedfixed some text-formatting issues with my initial post.
Comment #21
DuaelFrAfter 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.
Comment #22
DuaelFrI was wrong. My cache misleaded me. That bug only affects the drupallink plugin what is still bad ;)
Comment #23
Wim LeersI'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:
allowedContent
+requiredContent
definitions for thedrupallink
plugin from the string format tonew CKEDITOR.style({…})
.Steps to reproduce:
/node/add/article
.vs.
Comment #24
Wim LeersMoving the STR I added to #23 into the IS.
Cleaned up/revised the IS in general.
Comment #25
Wim LeersConfirmed that this only happens when using the
filter_html
filter.Curiously, if I override the
allowedContent
setting for the text format, by:drupalSettings.editor.formats.basic_html.editorSettings.allowedContent.a.attributes = 'href,hreflang,style';
(to allow thestyle
attribute) or evendrupalSettings.editor.formats.basic_html.editorSettings.allowedContent.a = true;
(to allow ANYTHING on the<a>
tag)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:
But then if you follow the last link, there is no example at all. I think that would be very helpful.
Comment #26
Wim LeersNow officially confirmed to be a CKEditor bug: https://dev.ckeditor.com/ticket/13886.
Comment #27
Wim LeersNow 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.
Comment #28
Wim LeersLooking at CKEditor's test
checkelementremovable.js
, it looks like ifattributes
is specified, that you should usestyle
, notstyles
.E.g.:
I can confirm that changing it from
styles
tostyle
here makes things work.Comment #29
Wim LeersSo, 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.
allowedContentDefinition
does not have astyles
key, yet the second code snippet assumes it exists anyway. This was the problem that was then solved in the next patch, which was…styles
key, so that we ended up with:styles
key; this snippet of code is why we added it!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.
Comment #30
Wim Leers#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 #29.
, since that's no longer true perComment #31
Wim LeersTo show that the CKEditor team helped me reach this conclusion, and especially @Reinmar:
Comment #32
DuaelFrAwesome!
+1 RTBC :)
Comment #33
webchickWow, 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!
Comment #36
iainH CreditAttribution: iainH as a volunteer commentedThis 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.
Comment #37
Wim LeersPlease open a new issue rather than reopening one from half a year ago. Thanks :)
Comment #38
iainH CreditAttribution: iainH as a volunteer commentedCreated new issue #2684861: CKEditor "underline" button conflicts with html filtering
Comment #39
Wim LeersNote 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.)Comment #40
DuaelFr#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).
Comment #41
mlewand CreditAttribution: mlewand commented#39 @DuaelFr made a valid point here, it will be easier to extend, while being slightly more explicit.
Comment #42
Wim LeersAlright. @DuaelFr, you want to open an issue for that?
(Temporarily making this issue active again so we don't lose track.)
Comment #43
DuaelFr@Wim Leers: I can't before May 1st so do not hesitate to leave me behind this time ;)
Comment #44
xjmWhen 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.
Comment #45
DuaelFrI finally opened that follow-up: #2728893: Bring back 'styles' to widget definitions in Drupal core's CKEditor plugins
Comment #46
Wim LeersThanks! And sorry, @xjm.
Comment #47
Wim LeersAccelerating what d.o would do in a few weeks for us.
Comment #48
ec-adam CreditAttribution: ec-adam commentedThis 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.
Comment #49
ec-adam CreditAttribution: ec-adam commentedComment #50
kala4ekYep, the same on 8.2.5
Comment #51
rwam CreditAttribution: rwam commentedWith 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.
Comment #52
phuang07 CreditAttribution: phuang07 commentedMy issue is probably related to this. If I have the filter - "Display any HTML as plain text" enabled, many of the buttons disappear.