[upstream] <script> tag support in GHS [#3245950]

Problem/Motivation

<script> tags are stripped even when using the Full HTML text format, or more generally: whenever there are zero HTML restrictions and hence even <script> should be allowed.

Steps to reproduce

Copy paste

<p>Hello</p>
<blockquote class="twitter-tweet"><p lang="en" dir="ltr">Feeling frustrated with myself for messing up what should&#39;ve been a simple thing. Who&#39;s got furry friend pics? :)</p>&mdash; webchick (@webchick) <a href="https://twitter.com/webchick/status/1451664162291998720?ref_src=twsrc%5Etfw">October 22, 2021</a></blockquote> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<p>Bye!</p>

… and this <script> tag should not be lost upon save.

Proposed resolution

❌ We evaluated adding https://ckeditor.com/docs/ckeditor5/latest/features/html-embed.html to our build of CKEditor 5. It could have been a better way to deal with "random HTML snippets" such as social media embeds.
We tried, and concluded (see #12) that A) it generates additional wrapping mark-up (which is an unacceptable regression compared to both CKEditor 4 in D8|9 and CKEditor 5 in HEAD), B) it still does not prevent losing <script> markup (which is the regression in HEAD, when upgrading from CKEditor 4 to 5).
✅ Fix GHS upstream. This does not yield the beautiful UX that the HTML Embed CKEditor 5 plugin would have given us (or rather, that we were hoping for), but it still fixes the problem at hand: the critical data loss bug.

Remaining tasks

~~Working PoC.~~
~~Dig into html-embed docs to see if we can tweak its configuration to make it do what we need~~
~~Work to get upstream fix in https://github.com/ckeditor/ckeditor5/issues/10891~~
~~Confirmed that the upstream fix works: #27~~
Wait for the January 26, 2022 release of CKEditor 5: #26 + #27

User interface changes

None.

API changes

None.

Data model changes

None.

Comment	File	Size	Author
#24	3245950-test-upstream-24.patch	1.5 MB	lauriii
#12	Screenshot 2021-11-16 at 16.52.16.png	24.07 KB	wim leers
#12	Screenshot 2021-11-16 at 16.52.08.png	20.42 KB	wim leers

Issue fork drupal-3245950

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

1 hidden branch

3245950-html-embed-cke5-plugin

changes, plain diff MR !1419

Check out this branch for the first time

Check out existing branch, if you already have it locally

Issue fork ckeditor5-3245950

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

1 hidden branch

3245950-evaluate-adding-the

changes, plain diff MR !166

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

26 October 2021 at 12:21

Wim Leers created an issue. See original summary.

Comment #2

wim leers

Ghent 🇧🇪🇪🇺

commented 26 October 2021 at 12:22

Title:	Evaluate adding the "HTML Embed" CKE5 plugin	» [PP-1] Evaluate adding the "HTML Embed" CKE5 plugin
Status:	Active	» Postponed
Related issues:		+#3245942: Update CKEditor 5 to 31.0.0

Let's do this after we update to the latest CKE5 release (shipped today): #3245942: Update CKEditor 5 to 31.0.0.

Comment #4

10 November 2021 at 07:46

yash.rode made their first commit to this issue’s fork.

Comment #5

yash.rode commented 10 November 2021 at 11:28

Assigned:

Unassigned

» yash.rode

Comment #6

11 November 2021 at 09:54

yash.rode opened merge request !166

Comment #7

yash.rode commented 11 November 2021 at 15:56

Status:

Active

» Needs review

Comment #8

damienmckenna

TN, USA

commented 12 November 2021 at 18:24

Are there any security concerns about allowing people to use this to add script tags?

Comment #9

wim leers

Ghent 🇧🇪🇪🇺

commented 16 November 2021 at 15:18

#8: Yes — just like we have those concerns for the Full HTML text format + CKEditor 4.

Full HTML = "you're on your own".

That won't change.

(Unless #3097468: Deprecate the "Full HTML" text format in Standard and Umami in favor of a "content editor HTML" for content editor roles lands!)

Comment #10

wim leers

Ghent 🇧🇪🇪🇺

commented 16 November 2021 at 15:30

Project:	CKEditor 5	» Drupal core
Version:	1.0.x-dev	» 9.3.x-dev
Component:	Code	» ckeditor5.module
Assigned:	yash.rode	» wim leers
Status:	Needs review	» Needs work
Issue tags:		+Needs screenshots

Porting this to a Drupal core MR now that CKE5 has been committed…

Unfortunately @yash.rode only posted code, no commentary. So… no idea if this actually works, what his thoughts on it are. In the future, please include screenshots or at least post a comment explaining how to test it, how well it works, concerns you have, et cetera 🙏😊 This is always important, but doubly so when the issue title says to "evaluate"!

Comment #11

16 November 2021 at 15:45

Wim Leers closed merge request !166

Comment #12

wim leers

Ghent 🇧🇪🇪🇺

commented 16 November 2021 at 15:53

Issue tags:

-Needs screenshots

+Needs issue summary update

Status	File	Size
new	Screenshot 2021-11-16 at 16.52.08.png	20.42 KB
new	Screenshot 2021-11-16 at 16.52.16.png	24.07 KB

Got it to work locally. Next up: MR.

It does not quite do what I was expecting/hoping:

generates

… but maybe we can tweak its configuration… 🤔

Comment #13

16 November 2021 at 15:54

Wim Leers opened merge request !1419

Comment #14

wim leers

Ghent 🇧🇪🇪🇺

commented 16 November 2021 at 15:59

Assigned:

wim leers

» Unassigned

Core MR has been created. Still more work to be done on this issue though; just like before the move to core.

Comment #15

wim leers

Ghent 🇧🇪🇪🇺

commented 16 November 2021 at 16:02

Issue summary:	View changes
Issue tags:	-Needs issue summary update

Figured I might as well update the issue summary while I have it all in my head!

Comment #16

wim leers

Ghent 🇧🇪🇪🇺

commented 16 November 2021 at 16:03

Issue summary:

View changes

Comment #17

wim leers

Ghent 🇧🇪🇪🇺

commented 17 November 2021 at 12:35

Title:

Evaluate adding the "HTML Embed" CKE5 plugin

» Evaluate adding the "HTML Embed" CKE5 plugin, to represent <script> tags?

Clarifying issue title.

Comment #18

wim leers

Ghent 🇧🇪🇪🇺

commented 18 November 2021 at 14:33

Questions I will ask the CKEditor team after checking https://ckeditor.com/docs/ckeditor5/latest/features/html-embed.html:

Can we automatically wrap only <script> tags in this?
But would that break the preview?
But <script> tags are apparently never executed anyway? So e.g. embedded tweets would not work anyway?

Comment #19

wim leers

Ghent 🇧🇪🇪🇺

commented 18 November 2021 at 16:34

Title:	Evaluate adding the "HTML Embed" CKE5 plugin, to represent <script> tags?	» [upstream] [assuming CKE5 Jan 2022 release adds <script> to GHS] Add the "HTML Embed" CKE5 plugin when it supports visualizing <script> tags
Category:	Task	» Feature request
Status:	Needs work	» Postponed
Issue tags:		+Usability, +Needs issue summary update

@Reinmar said in the CKEditor 5 meeting just now:

no technical reason why <script> tags need to be stripped in GHS, it was just not a high enough priority.
but we also considered using the HTML Embed plugin to make <script> tags discoverable/visible.
GHS approach is definitely simpler, but we think we may want to add something like HTML Embed-ify-
-tags eventually anyway.
will prioritize <script> tag support in GHS for the end-of-January release.

@lauriii and I +1'd point 3 — we agree this would massively improve usability. That means this issue is postponed until GHS approach is definitely simpler, but we think we may want to add something like HTML Embed-ify--tags eventually anyway. is done. Then this will be a usability improvement. Hence recategorizing as Feature request and tagging Usability. This is assuming the late January release will add <script> tag support to GHS. Clarified that in the title.

Comment #20

18 November 2021 at 16:35

Version:

9.3.x-dev

» 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #21

wim leers

Ghent 🇧🇪🇪🇺

commented 17 December 2021 at 10:35

Assigned:	Unassigned	» wim leers
Priority:	Normal	» Major
Issue tags:		+Needs upstream feature, +stable blocker

Definitely needs issue summary update. Will do that.

Upstream implementation plan at https://github.com/ckeditor/ckeditor5/issues/10891

Comment #22

wim leers

Ghent 🇧🇪🇪🇺

commented 7 January 2022 at 14:15

Reported again in #3256566: [upstream] <style> tag support in GHS.

Comment #23

wim leers

Ghent 🇧🇪🇪🇺

commented 13 January 2022 at 16:25

Title:	[upstream] [assuming CKE5 Jan 2022 release adds <script> to GHS] Add the "HTML Embed" CKE5 plugin when it supports visualizing <script> tags	» [upstream] <script> tag support in GHS
Assigned:	wim leers	» lauriii
Status:	Postponed	» Active
Issue tags:	-Needs upstream feature

Discussed in CKE5 meeting: https://docs.google.com/document/d/1R0WI9eKt7vug79HrlEwI5BHfmdTgM1KJrjB6...

Fix has been merged two days ago: https://github.com/ckeditor/ckeditor5/commit/277a5919870e69c330337f13a7d....

@lauriii will create a CKE5 build of that specific commit, to allow us to test this prior to the January 26 release that they've planned.

(I will definitely still update the issue summary, unless @lauriii beats me to it.)

Comment #24

lauriii

he/him

Finnish

Finland

commented 17 January 2022 at 12:55

Status	File	Size
new	3245950-test-upstream-24.patch	1.5 MB

Updated documentation to include instructions for how to test upstream features before they are released: https://www.drupal.org/docs/core-modules-and-themes/core-modules/ckedito... .

Using those instructions, generated this patch. ~~I noticed bug with the upstream change and reported it on the PR: https://github.com/ckeditor/ckeditor5/pull/11075~~. Before posting this comment, @Reinmar mentioned that it was fixed earlier today 🎉.

Comment #25

lauriii

he/him

Finnish

Finland

commented 17 January 2022 at 12:56

Assigned:

lauriii

» Unassigned

Comment #26

wim leers

Ghent 🇧🇪🇪🇺

commented 17 January 2022 at 13:54

@lauriii So … that means that we can confirm that the upstream fix is effective, right?

If so, I think we can mark this Postponed and wait for the January 26 release of CKEditor 5; we don't need to commit a custom build (i.e. not an actual official CKEditor 5 release) to Drupal core IMHO. Do you agree?

(In the mean time, we should still update the issue summary though.)

Comment #27

lauriii

he/him

Finnish

Finland

commented 17 January 2022 at 14:03

Status:

Active

» Postponed

If so, I think we can mark this Postponed and wait for the January 26 release of CKEditor 5; we don't need to commit a custom build (i.e. not an actual official CKEditor 5 release) to Drupal core IMHO. Do you agree?

Yes, I think we should mark this as postponed and wait for the release. Just wanted to keep this open for a while more in case anyone else had any additional feedback.