Problem/Motivation

Out of the box, pasting content from Microsoft Word or Google Docs leads to unexpected span elements.

In ckeditor 4, these span elements are deeply nested. For example, a single h2, even after allowing ckeditor to detect and modify that the content was pasted from Word, leads to:

<h2><span><span><span><span><span><span><span><span>This is Heading 2</span></span></span></span></span></span></span></span></h2>

In ckeditor 5, it's better, but still adds a language attribute that in most cases isn't needed:

<h2>
    <span lang="EN-US">This is Heading 2</span>
</h2>

Looking back through the git and issue history, it looks like span was added at #1911884-30: Enable CKEditor in the Standard install profile.

The issue doesn't say, but I asked @quicksketch offline and he said:

I think it was primarily because it was needed for underline styling, since <u> tags were frowned upon at the time. Underlines are <span class="underline">.

As well, #1322906: Allow span tag by default for language changes? was marked as fixed with no commits to support span tags for marking language.

However, today in both Drupal 9 and Drupal 10:

  1. The basic text format does not enable the underline button in ckeditor by default.
  2. The language dropdown is not enabled either.

I've seen Lighthouse call this out as a performance issue on large pages due to the number of DOM nodes on the page. It's very confusing to look at a page as a developer debugging this, because visually the page might be relatively simple and it's easy to assume the problem is in the front-end theme or some JavaScript bug, and not that the content itself is saved this way.

Steps to reproduce

  1. Install from the standard profile.
  2. Create a paragraph of text in Google Docs or Word.
  3. Paste it into an article or page body field.

Proposed resolution

If we remove span from the list of allowed tags, it is completely removed with pasting generating much simpler HTML. This wouldn't prevent adding the tag, or solve pasting from word processors when spans are allowed.

  1. Remove span from core/profiles/standard/config/install/filter.format.basic_html.yml.
  2. Ensure that span is added in ckeditor 5 configurations when adding the language selector.
  3. If span is still required for underlines, do the same thing there.

Remaining tasks

User interface changes

API changes

None.

Data model changes

None.

Release notes snippet

The 'Basic HTML' format provided with the Standard profile no longer allows the <span> tag. In particular, CKEditor 4 is prone to generating many unnecessary <span> tags. This should simplify copying and pasting from MS Word, Google Docs and similar programs into either CKEditor 4 or CKEditor 5 on new installs. Existing installs may want to consider removing the tag from text formats and reviewing existing content.

CommentFileSizeAuthor
#4 3316921-4.patch1.49 KBwim leers
#4 interdiff.txt597 byteswim leers
#2 3316921-2.patch1.01 KBwim leers

Comments

deviantintegral created an issue. See original summary.

wim leers’s picture

Component: ckeditor5.module » base system
Status: Active » Needs review
StatusFileSize
new1.01 KB

Oh, wow, some epic historical research you did there! 👏

I personally do not recall <span class="underline"> being the reason or that even being preferable over <u>. But it looks like @quicksketch is the person who indeed introduced it over there, so … it's probably true?

I will say that filter_html has always allowed the lang and dir attributes on all tags — see #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default. But that doesn't mean that<span lang="…"> was also always allowed. So … I think you're on to something!

P.S.: The paste-from-Docs/Word concern is related to #3254346: [upstream] CKEditor 5 PasteFromOffice plugin should not allow MS Office/Apple Pages/Google Docs-specific classes and styles even when HTML is unrestricted. But this issue stands on its own for sure.

Status: Needs review » Needs work

The last submitted patch, 2: 3316921-2.patch, failed testing. View results

wim leers’s picture

Status: Needs work » Needs review
StatusFileSize
new597 bytes
new1.49 KB

Great, the validation logic caught that the Basic HTML text format & editor are now out of sync! 🤓

Let's update the editor too.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

+1 - we don't allow generic <div> so why do we allow generic <span>?

Issue summary is extremely thoroughly researched and all rationale makes sense to me. I've personally run into the multiple nested spans issue when handling paste-from-Word content so to have this fixed out of the box will be nice!

Therefore, assuming tests pass, to me #4 is RTBC.

I also think we can make the argument for backporting to 9.5.x/10.0.x given that this only affects new installs - existing sites are unaffected.

wim leers’s picture

I also think we can make the argument for backporting to 9.5.x/10.0.x given that this only affects new installs - existing sites are unaffected.

+1

mherchel’s picture

OMG, I've fought against nested spans in CKEditor for so long. This would be a big +1 for people's editing experience.

deviantintegral’s picture

Given no one has jumped up to say "this was a feature", could we call this a bug and get it in 9.4.x? Especially since this only affects new installs. I've realized from working on this that so many previous projects I thought spans were intentionally allowed by the site requirements, and not an artifact of installing with standard. Now, those sites all have the spans in content and cleanup means checking to see if editors have added any intentional attributes.

wim leers’s picture

Not even bug fixes are allowed in patch releases if they're disruptive, so … I doubt it 🤓😅

longwave’s picture

Drupal 9.5 and 10.0 are out in 7 weeks, we can wait that long even if it's considered a bug.

  • catch committed f7a7658 on 10.0.x
    Issue #3316921 by Wim Leers, deviantintegral, longwave: Remove span tag...
  • catch committed 7f00930 on 10.1.x
    Issue #3316921 by Wim Leers, deviantintegral, longwave: Remove span tag...
catch’s picture

Version: 10.1.x-dev » 9.5.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: +9.5.0 release notes

This makes lots of sense, thanks for the archaeology! Also agreed with it being fine during beta.

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!

Didn't apply to 9.5.x, but it's such a trivial change I removed the line locally and committed there too.

Added a release notes snippet and tagging, I don't think this needs a CR though really.

  • catch committed 51554df on 9.5.x
    Issue #3316921 by Wim Leers, deviantintegral, longwave: Remove span tag...
wim leers’s picture

#10++

Also, wow, this was an incredibly fast turn-around time!

wim leers’s picture

Issue summary: View changes

Release note tweaks.

longwave’s picture

Issue tags: +10.0.0 release notes

Probably should go in both sets of release notes?

deviantintegral’s picture

Issue summary: View changes

Clarify CKEditor 4 / 5 differences and note about reviewing content in release notes.

Status: Fixed » Closed (fixed)

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