Problem/Motivation

<i class="fa fa-drupal">Drupal</i>

Steps to reproduce

Entering

<i class="fa fa-twitter">Twitter</i>

(as suggested here for icons) using the CKEditor 5 "Sourcecode" button results in

<p>
    <em><i class="fa fa-twitter">Twitter</i></em>
</p>

when leaving the source code edit area and back.

Expected result

<p>
  <i class="fa fa-twitter">Twitter</i>
</p>

without the wrapping <em>.

I have no idea why and where this is done, but I wouldn't rate this as expected?
Also, I don't think it's totally untypical that such kinds of iconfonts are used in Drupal WYSIWYG by site builders?

This can be easily reproduced in a fresh Drupal 10 installation using https://simplytest.me/ for example.

Testing the same on the CKEditor 5 Demo page leads to different (expected) results:
https://ckeditor.com/docs/ckeditor5/latest/features/source-editing.html

<p>
    <i class="fa fa-twitter">Twitter</i>
</p>

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Release notes snippet

TBD

Issue fork drupal-3341280

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Anybody created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Title: <i> (iconfont icon) turned into blank <em> in CKEditor5 » <i> (iconfont icon) wrapped into <em> in CKEditor5
Issue summary: View changes

Sorry, I had to correct the details as I wrongly used "Basic HTML" in my reproduction. Should have been "Full HTML".
Basic HTML works as expected, as <i> was not allowed there!

For Full HTML I'll now try, if entering in "Source Editing" fixes this.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Title: <i> (iconfont icon) wrapped into <em> in CKEditor5 » <i> (iconfont icon) wrapped into <em> in CKEditor5 with "Full HTML"
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

For Full HTML I'll now try, if entering in "Source Editing" fixes this.

No, that doesn't change anything. The <i> is always wrapped into <em> when leaving the "Source" editing.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes
cilefen’s picture

cilefen commented

#2722451: <i> tags without content are stripped by CKEditor even with "Full HTML" (i.e. without ACF) reappeared as #3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements.

Is this intrinsic to Drupal or is it CKEditor behavior?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Is this intrinsic to Drupal or is it CKEditor behavior?

Seems to be Drupal-specific!

https://ckeditor.com/docs/ckeditor5/latest/features/source-editing.html
Trying that here leads to expected results:

<p>
    <i class="fa fa-twitter">Twitter</i>
</p>
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: <i> (iconfont icon) wrapped into <em> in CKEditor5 with "Full HTML" » <i> (iconfont icon) wrapped into <em> with "Full HTML"
Priority: Normal » Major
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs tests
  1. Is this reproducible in Basic HTML if you explicitly allow <i>?
  2. Any chance you could write a test for this? 🙏 😊

This is related to #3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements but is definitely a different problem.

I'm 99% certain this is caused by DrupalEmphasisEditing — see core/modules/ckeditor5/js/ckeditor5_plugins/drupalEmphasis/src/drupalemphasisediting.js.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Any chance you could write a test for this? 🙏 😊

I'd very much like to, but currently missing spare time. I'll add an entry for our team, hopefully someone has some time to do it. Do you know a similar example to copy as a quick start? Especially for the CKEditor setup and interaction? That would speed things up a lot, I think.

I'll also ask to test 2. but if ANYONE coming here can do it, please do it and don't wait!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed (maintainer needs more info) » Needs work

Thank you!

Actually, 99% of what you need is already provided by \Drupal\Tests\ckeditor5\FunctionalJavascript\EmphasisTest — you just want to explicitly test with Full HTML.

Basically, you want to copy \Drupal\Tests\ckeditor5\FunctionalJavascript\EmphasisTest::testEmphasis(), rename it testEmphasisWithoutFilterHtml() and modify the text format config entity's filter_html to be disabled. That should reproduce it 🤓

I think that'll help you get started! 🤞

karamveersingh’s picture

karamveersingh commented

we also facing a similar issue on Ckeditor5 and would like to know a fixed solution. Do we've to downgrade it to 1 version means Ckeditor4?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@Wim Leers: FYI: Thanks for #12! We'll do it, but we didn't have the time yet. If anyone can do this earlier, I'd of course appreciate that. Sorry.

jcandan’s picture

jcandan commented

This is bothersome. Any workarounds until a patch can be made?

Note: Additionally, when no element content is supplied, the <i> tag get filtered out completely.

<p>
    <i icon-name="menu"></i>
</p>

Becomes

<p></p>
jcandan’s picture

jcandan commented

In an attempt to work around, I tried to use an <svg>. But, it gets pushed outside the <a> it's in.

This is unrelated to the issue described above, but also annoying.

<a href="#!">
    Open an account&nbsp;
    <svg class="bi bi-arrow-right" ... viewBox="0 0 16 16">
        <path fill-rule="evenodd" d="..."></path>
    </svg>
</a>

Becomes

<a href="#!">
    Open an account&nbsp;
</a>
<svg class="bi bi-arrow-right" ... viewBox="0 0 16 16">
    <path fill-rule="evenodd" d="..."></path>
</svg>

ravi.shankar opened merge request !3644

ravi.shankar made their first commit to this issue’s fork.

ravi.shankar’s picture

ravi.shankar commented

Working on it.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Grevil made their first commit to this issue’s fork.

grevil’s picture

grevil commented

Tried adding a failing test, to show the problem, but it succeeds...

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@Grevil: testEmphasisWithoutFilterHtml is failing as expected, but we should be sure the result is what we expect. Could you please check the test result again? Could we perhaps explicitly compare the expected result so the test shows XX expected, actual result YY?

Thanks for picking this up for me!

grevil’s picture

grevil commented

Unfortunately, this is NOT the "expected" failure...

I'll have a look later.

grevil’s picture

grevil commented

Found the problem, I forgot to copy over all changes, as I am still struggling to set up a proper dev environment for core issues...

grevil’s picture

grevil commented

Now, comment #22 should apply, and the test will succeed (but it is expected to fail).

grevil’s picture

grevil commented

I adjusted the test accordingly, it should fail now.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.