- Issue created by @leymannx
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Very interesting! Thanks for doing this research! 🙏👏
CKEditor 5 should never strip explicitly allowed markup, not even more obscure markup like this. Bumping to and tagging .
Second critical GHS bug in a week: 🐛 [upstream] [GHS] CKEditor 5 does not retain custom HTML tags that are not defined by CKEditor 5 plugins whenever /.*/ is not allowed (e.g. when filter_html is enabled) Postponed .
But … frankly, the https://www.drupal.org/project/fontawesome → module should provide a filter instead. Because markup like
<fontawesome>drupal</fontawesome>
which the corresponding filter plugin then transforms to
<i class="fab fa-drupal"></i>
would be far clearer, and it'd make it much easier to do transformations on existing content, e.g. to update icons 😅
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Discussed this with @Reinmar from CKEditor 5!
Observations:
- This also doesn't work in CKEditor 4 without a custom plugin — I tried: Full HTML + CKEditor 4 +
<p><i class="fab fa-drupal"></i> asdflksjdflaskdfj</p>
pasted ⇒<i class="fab fa-drupal"></i>
gets stripped! - The only reason it works for you in CKEditor 4 is because you have a custom plugin, and one that sets
// Allow empty tags in the CKEditor since Font Awesome requires them. if ('editor' in drupalSettings && 'fontawesome' in drupalSettings.editor) { $.each(drupalSettings.editor.fontawesome.allowedEmptyTags, (_, tag) => { CKEDITOR.dtd.$removeEmpty[tag] = 0; }); }
- In CKEditor 5 you also need a custom plugin to do this.
That being said, this should work. They confirmed this, and they are planning to work on this in the next few months! 👍 Just know that in the mean time, you could work around it by adding explicit support for this custom tag in a CKEditor 5 plugin in 📌 CKEditor 5 compatibility Needs review 😊 If that weren't the case, I'd have tagged this .
- This also doesn't work in CKEditor 4 without a custom plugin — I tried: Full HTML + CKEditor 4 +
- leymannx Berlin
Wow, thanks for the update, Wim!! 🤗
But I just double-checked that this wasn't an issue in Drupal 9 CKE 4. So I've quickly set up a D 9.4, logged in, and created an article, putting
<span></span> My span
in the editor, after clicking "Source", and saved. Visiting the article, inspecting the markup and saw that the empty tag persisted.BUT as soon as I went to edit the article again, clicking "Source" again, the empty
<span></span>
was gone. 🤯mkdir my-drupal9-site && \ cd my-drupal9-site && \ ddev config --project-type=drupal9 --docroot=web --create-docroot && \ ddev start && \ yes | ddev composer create "drupal/recommended-project:9.4" && \ ddev composer require drush/drush && \ ddev drush -y site:install --account-name=admin --account-pass=admin && \ ddev drush uli | xargs open
- 🇺🇸United States daniel.moberly
To address the "filter" idea in #5 - this markup is not custom to the Fontawesome module, but rather the actual implementation of Fontawesome itself (see https://fontawesome.com/docs/web/setup/get-started). Changing the way the tags are added prevents folks from using any of the Font Awesome documentation or writing the code themselves site-agnostic.
- 🇫🇷France gaboo
Having the same problem here for something other than FontAwesome.
Please allow whatever we want to put in the HTML source, including unkwnow or empty tag, when we use full HTML. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#8: So … that does mean that plain CKEditor 4 exhibited the same behavior, right? That's why it also only works in CKEditor 4 with that custom plugin installed. The problem is that no equivalent API exists in CKEditor 5, so even writing that plugin is impossible today.
#9 fair point!
#10: that's definitely always been the goal.
- 🇺🇸United States glynster
@Wim Leers we are having the same issue with an a tag wrapped around a custom picture block:
CKeditor source view:
<div class="adblock"> <a href="/link"> <picture> <source media> </picture> </a> </div>
CKeditor default view:
<div class="adblock"> <picture> <source media=""> </picture> </div>
And indeed dataloss. We migrated to CKeditor 5 and just notcied all links were stripped from our adblocks.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#12: wow, that's a whole new kind of data loss, separate from this one! Created new issue at 🐛 [upstream] [GHS] CKEditor 5 removes s that wrap HTML elements not natively supported by CKEditor 5 Fixed and credited you 👍 Discussing with the CKE5 team later today.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Closed 💬 CKEditor 5 not supporting fa fa icon Closed: duplicate as a duplicate.
- 🇺🇸United States glynster
@Wim Leers thanks so much! I responded in the other issue as well along with our workaround/solution.
- 🇩🇪Germany Anybody Porta Westfalica
Confirming this issue still exists in Drupal 10.1.0
We tried:
<a href="#" class="button"><i class="fa fa-drupal"></i></a>
which is being erased, while
<a href="#" class="button"><i class="fa fa-drupal">XXXXX</i></a>
is kept. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
https://github.com/ckeditor/ckeditor5/issues/9888 has not yet been fixed upstream, so it's also not in 📌 Update CKEditor 5 to 38.1.0 Fixed , so
10.1.1
won't fix it either. - 🇩🇪Germany ed-media
I was preparing one of our sites to migrate to Drupal 10 but it looks like the issue is also there with Drupal 9.5.10 and CKEditor 5 enabled see attached screenshots.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
https://github.com/ckeditor/ckeditor5/issues/9888 landed upstream 1 hour ago!
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Better yet: so far, that upstream issue is looking likely to ship in a
38.x.0
minor release — meaning that we could update Drupal10.1.x
to a version that includes the fix! 😊No guarantees though.
- Assigned to wim leers
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
We need test coverage to prove this is broken today and will be fixed in 📌 Update CKEditor 5 to 39.0.0 Fixed .
- Issue was unassigned.
- Status changed to Needs review
over 1 year ago 11:22am 3 August 2023 - last update
over 1 year ago 574 pass, 1 fail - last update
over 1 year ago 575 pass, 1 fail The last submitted patch, 23: 3337298-23.patch, failed testing. View results →
- Status changed to Needs work
over 1 year ago 1:40pm 3 August 2023 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I discussed #23 in detail with @Reinmar and @witeksocha.
Conclusion:
- For unrestricted text formats, #23 MUST NOT assume that all tags can have empty inline elements.
- Instead, even for unrestricted text formats, which elements ought to support empty elements should be explicitly configured.
- This matches the behavior in CKEditor 4, where it could not even be configured through the UI, it required custom code — see #7.
So, adjusting the test coverage slightly for that: even for unrestricted text formats, we should expect
<p>Before <i class="fab fa-drupal"></i>
to be transformed to
<p>Before and after.</p>
- last update
over 1 year ago 574 pass, 1 fail - last update
over 1 year ago 575 pass, 1 fail - Status changed to Needs review
over 1 year ago 2:21pm 3 August 2023 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Green test result for https://git.drupalcode.org/project/drupal/-/merge_requests/4530/diffs?co... 🚀
- Status changed to Fixed
over 1 year ago 2:33pm 4 August 2023 - 🇺🇸United States smustgrave
Since https://www.drupal.org/project/drupal/issues/3377562 📌 Update CKEditor 5 to 39.0.0 Fixed was merged into 11.x and hopefully soon 10.1.x
Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Fixed
11 months ago 3:02pm 20 December 2023