fisharebest / webtrees

Online genealogy
https://webtrees.net
GNU General Public License v3.0
463 stars 299 forks source link

Fixes: #4948 - Update ckeditor-js.phtml extraAllowedContent #4949

Closed photon-flip closed 7 months ago

photon-flip commented 8 months ago

Allow CKEditor to accept all element classes. The current webtrees ckeditor-js.phtml CKEDITOR.config.extraAllowedContent ='*[class,style];'appears intended to do this but is not working consistently. See issue #4948 CKEditor strips some bootstrap and other classes. This also facilitates using classes added in the CSS and JS module.

fisharebest commented 8 months ago

I am hesistant to allow wildcard attributes.

e.g. <img src="no-such-url" onerror="do_evil()"

photon-flip commented 8 months ago

The wildcards in *(*)are for any element, allow any class name. This doesn't change attributes - any attribute would be in square brackets[*]. Your example <img src="no-such-url" onerror="do_evil()" the attribute gets striped out as it should.

CKEditor documentation:

elements [attributes]{styles}(classes) Where:

elements – a list of space-separated element names or an asterisk (*) character,
attributes – a comma-separated list of attribute names or an asterisk (*) character,
styles – a comma-separated list of style names or an asterisk (*) character,
classes – a comma-separated list of classes or an asterisk (*) character.
photon-flip commented 8 months ago

Another quick note. According to the CKEditor documentation, the element wildcard in config.extraAllowedContent is for already enabled elements. It's not allowing all and any element so still quite restrictive.

photon-flip commented 8 months ago

Sorry to pester but any chance of moving this forward? I have a project I've been working on for a month that is rendered pretty useless without this. And for everyone else it takes the unpredictability and frustration out of editing in CKEditor source mode. I believe I've researched the documentation and tested this pretty well but if you're unsure it would be quite simple for you test yourself. And it looks to me as though this was the intention of the original *[class,style]; but that is not working s it should.

MurrayJ

photon-flip commented 7 months ago

Tested that *(*)only allows classes for already allowed elements. dl dt dd elements (which are not allowed elements) when added in source mode these are striped out even with *(*)in the config.extraAllowedContent

photon-flip commented 7 months ago

Are there any other tests you would like me to do to check that attributes are not affected by *(*)it only allows classes.

I can't seem to find what the current *[class,style]is doing in practice. Removing it seems to have no effect. It looks like it was supposed to allow classes and styles on all allowed elements but that is not working. *(*)is the way to do that. without it, styling anything other than div elements involved torturous work arounds.

fisharebest commented 7 months ago

I can't seem to find what the current *[class,style] is doing in practice.

I'm guessing that it enables class and style attributes on any element. This is old code, so perhaps it was needed on an older version of ckeditor?

But as you say, these attributes seem to be allowed by default, so no longer a need to specify them.

You have added ;*(*) to allow classes. Presumably you also want to add styles? ;*{*}(*). The server-side filtering allows classes and styles.

photon-flip commented 7 months ago

Great, classes as well as inline styles working on all allowed elements. Thanks.