Update CKEditor to latest version (4.21.0) #1081

ajeety4 · 2023-03-31T12:53:33Z

Summary

JIRA Ticket
Updates CK Editor 4 to the at present latest version (4.21.0) and addresses below points. We are using a custom build (with a patch) and hence the update process is manual.
As part of this update, the plugin toolbar was included by default as clipboard is dependent on it. Plugin floatingspace was removed as it is not required and also because it created an empty toolbar on the rich text editor.
Html to text handler was modified to remove additional span elements created by widgets plugin while copying widgets.
Test cases for copying widgets were modified to wait for the copy process to complete as it is an async operation.
About the build: This build was created from source. Patch 1 was addressed in upstream repo however Patch 2 was not. Patch 2 is part of this build. For changes that were made on the build, see commit1 that reverts the patch1 for simplicity and commit2 that merges the 4.21.0 release on this branch.

Feature Flag

Product Description

Safety Assurance

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change
If QA is part of the safety story, the "Awaiting QA" label is used
I have confidence that this PR will not introduce a regression for the reasons below

Automated test coverage

QA Plan

https://dimagi-dev.atlassian.net/browse/QA-4973

Safety story

Local testing was done to ensure patches: Patch 1 and Patch 2 are working correctly.
Automated test cases were run and issues were addressed.

Rollback instructions

This PR can be reverted after deploy with no further considerations

Note for Reviewers

Since this is a manual update to the new version, this commit has bulk of file changes, the CK Editor 4 build itself.

millerdev · 2023-03-31T14:29:48Z

src/richText.js

@@ -731,6 +735,9 @@ define([
                   .replace(/<\/p>/ig, "\n")
                   .replace(/<br \/>/ig, "\n")
                   .replace(/(&nbsp;|\xa0|\u2005)/ig, " ")
+                   // While copying widgets with text, CKEditor adds these html elements
+                   .replace(/<span\b[^>]*?id="?cke_bm_\d+(\w)"?\b[^>]*?>.*?<\/span>/ig, "")


Few questions

Why group on (\w)? And is that intended to match a single word character or multiple?

Is a quotation mark (") considered a word boundary? If not, would it be better to change the second \b to \s?

Should the contents of the span be captured in a group and included in the replacement text?

Few questions
FYI - I adapted both the regex's from the CK code base itself so as to keep the match as close as possible.
Also see sample html for reference here

Why group on (\w)? And is that intended to match a single word character or multiple?
My bad and good catch. Grouping is not needed at all and will be removing it. Intended to match single character as per the id format for bookmark.

Is a quotation mark (") considered a word boundary? If not, would it be better to change the second \b to \s?
Yes quotation mark is part of word boundary.

Should the contents of the span be captured in a group and included in the replacement text?
The bookmark span element has only fixed content:  to support IE, which causes an additonal whitespace character in our plain text. This is not needed in the replacement text.

Looks like the extra group around \w has not been removed. Is that group in the original regex you copied from CK code? If yes, then fine to keep it so it matches as closely as possible.

Sorry got missed and thanks for catching :). Yes the one I copied has it and is used for some processing. Since we don't need it, will get rid of the extra group.

millerdev · 2023-03-31T14:29:50Z

src/richText.js

@@ -731,6 +735,9 @@ define([
                   .replace(/<\/p>/ig, "\n")
                   .replace(/<br \/>/ig, "\n")
                   .replace(/(&nbsp;|\xa0|\u2005)/ig, " ")
+                   // While copying widgets with text, CKEditor adds these html elements
+                   .replace(/<span\b[^>]*?id="?cke_bm_\d+(\w)"?\b[^>]*?>.*?<\/span>/ig, "")
+                   .replace(/<span\b[^>]*?data-cke-copybin-(start|end)[^<]*?<\/span>/ig, "")


Is the intent of this one to remove the entire span, contents included?

Yes, the intention is to remove the entire span element along with their contents for the below reasons:

As per my understanding, these additional elements(including the bookmarks one) are created for copying widgets on top of the actual html content.

This pollutes our plain text with new line character at the end as it wraps the outermost p tag.

millerdev · 2023-03-31T14:32:56Z

tests/richText.js

+                // Wait for CK Editor Async handler (copybin) to complete
+                setTimeout(function(){
+                        callback();
+                },100);


How do we know it will always complete after 100ms? Is there an event we can tie this to rather than an arbitrary amount of time?

Used the same timeout as used by the plugin for supporting chrome on mac. https://github.com/ckeditor/ckeditor4/blob/4.21.0/plugins/widget/plugin.js#L3432

I tested a few times by explicitly setting the CKEditor env as mac to trigger this and they passed.

millerdev · 2023-03-31T14:35:00Z

lib/ckeditor/dimagi-README.md

- clone https://github.com/millerdev/ckeditor-dev and `cd` into ckeditor-dev
- checkout *vellum-build* branch
- download the minimalist skin (v1.0) and unzip/place it in the ./skins
+The repo [dimagi/ckeditor-dev](https://github.com/dimagi/) which is fork of upstream repo is used for creating a build from source using the branch **vellum-build**.


Suggested change

The repo [dimagi/ckeditor-dev](https://github.com/dimagi/) which is fork of upstream repo is used for creating a build from source using the branch **vellum-build**.

The repo [dimagi/ckeditor-dev](https://github.com/dimagi/ckeditor-dev) which is fork of upstream repo is used for creating a build from source using the branch **vellum-build**.

millerdev · 2023-03-31T14:43:38Z

src/richText.js

+                   .replace(/<span\b[^>]*?id="?cke_bm_\d+(\w)"?\b[^>]*?>.*?<\/span>/ig, "")
+                   .replace(/<span\b[^>]*?data-cke-copybin-(start|end)[^<]*?<\/span>/ig, "")


It would be great to have tests for these. The test input should contain HTML as rendered by the editor. I think they will be easiest to add with the "text conversions" tests, which call fromRichText, which calls fromHtml:

Vellum/tests/richText.js

Lines 252 to 292 in 025a0c5

describe("text conversions", function() {

var text = [

["blah\nblah", "blahblah"],

["blah\nblah\n", "blahblah "],

[

"blah\n\nblah\n\n",

"blah blah  "

],

[

"list\n* item\n* item",

"list* item* item"

],

[

"list\n\n* item\n* item",

"list* item* item"

],

[

"list\n\n\n* item\n* item",

"list* item* item"

],

[" ", " "],

[" ", " \xa0 "],

[" ", "   "],

["' ,", "'\u200B,"],

];

_.each(text, function(val){

it("from html to text: " + JSON.stringify(val[1]), function() {

assert.strictEqual(richText.fromRichText(val[1]), val[0]);

});

});

_.each(text, function(val){

it("(text -> html -> text): " + JSON.stringify(val[0]), function() {

assert.strictEqual(

richText.fromRichText(richText.toRichText(val[0])),

val[0]

);

});

});

});

Sounds good! Will be adding them.

These were added

ajeety4 · 2023-04-03T14:38:49Z

Also pasting here the sample of html generated by CK Editor while copying widgets for reference.
The additional elements created are highlighted.

mkangia

I don't have context on most of this so just took a quick pass.

I believe we are going to send this through QA?

mkangia · 2023-04-04T08:01:02Z

lib/ckeditor/dimagi-README.md

@@ -6,7 +6,6 @@ Go to the [CK build editor](http://ckeditor.com/builder).
 Use the following plugins:
 * clipboard (for drag and drop)
 * entities
-* floatingspace


should "toolbar" now be in this list?

Good point.
Not necessary as it is an internal dependency for clipboard which gets automatically included while using the online builder.
However, it would be useful to have a list of sub dependencies (there are others too) when building from source.
Will be including them in a sub section.

mkangia · 2023-04-04T08:09:39Z

@millerdev @ajeety4
Are you able to share what would be the impact if this is released with a bug despite our best efforts?
The form updates on App builder will fail to save or we would end up with bad forms that might also get released to the mobile users?

ajeety4 · 2023-04-04T10:51:22Z

I don't have context on most of this so just took a quick pass.

I believe we are going to send this through QA?

Yes, I will be creating a QA ticket for this.

ajeety4 · 2023-04-04T16:33:45Z

@millerdev @ajeety4 Are you able to share what would be the impact if this is released with a bug despite our best efforts? The form updates on App builder will fail to save or we would end up with bad forms that might also get released to the mobile users?

For more context (ignore if already known), the rich text editor is built on top of CK Editor and it is used in text fields, display conditiions, expression editor etc.
I am not sure on the mobile side. On the form builder side, depending on the bug it can impact things like copy , paste or even the easy references itself.

ajeety4 · 2023-04-05T14:45:08Z

@millerdev @mkangia (FYI)
While doing some additional testing for widgets, I had one observation related to the keyboard navigation.
This is applicable for live HQ version and the updated CK Editor version (this PR) for the below specific scenario.

Add a widget(easy reference) at the start of the editor and add some text after that.
Select the widget(fake): The keys 'End , Shift+End(Right/Down) does not work i.e widget plus right side selection of text.

Note: Rest of keys work fine, things are also good if there is text before the widget, select text first and then widget and it works fine.
From the patch, I understand that this selection thing is handled by the default CK editor handler.
Not a showstopper, however still something wanted to point out!
I think it is good to have a separate ticket for this in the backlog and prioritize accordingly. Any thoughts?

ajeety4 · 2023-04-06T09:50:32Z

FYI: PR for updating ubuntu version in workflow: #1082

millerdev · 2023-04-06T12:14:56Z

tests/richText.js

+                setTimeout(function(){
+                        callback();
+                },100);


nit

Suggested change

setTimeout(function(){

callback();

},100);

setTimeout(callback, 100);

millerdev · 2023-04-06T12:25:58Z

tests/richText.js

+                ['This dob: <output value="#case/dob" /> is of child', prefix_html + 'This dob:&nbsp;' + widget_html + ' is of child' + suffix_html],
+                ['This dob: <output value="#case/dob" />', prefix_html + 'This dob:&nbsp;' + widget_html +  suffix_html],
+                ['<output value="#case/dob" /> is of child', prefix_html + widget_html + ' is of child' + suffix_html],
+                ['<output value="#case/dob" />', prefix_html_1 + widget_html + suffix_html],
+                ['This dob: <output value="#case/dob" /> is of child', 'This dob: &lt;output value="#case/dob" /&gt; is of child']


Could these should be added to the text array above? Seems like this would allow removing the _.each(text_widgets_outputs, ... constructs below, eliminating duplication.

Sure. My initial thought was to keep the widgets html separate, however it is not needed as such and this is better as avoids duplication.

millerdev · 2023-04-06T12:31:46Z

src/richText.js

@@ -731,6 +735,9 @@ define([
                   .replace(/<\/p>/ig, "\n")
                   .replace(/<br \/>/ig, "\n")
                   .replace(/(&nbsp;|\xa0|\u2005)/ig, " ")
+                   // While copying widgets with text, CKEditor adds these html elements
+                   .replace(/<span\b[^>]*?id="?cke_bm_\d+(\w)"?\b[^>]*?>.*?<\/span>/ig, "")


Looks like the extra group around \w has not been removed. Is that group in the original regex you copied from CK code? If yes, then fine to keep it so it matches as closely as possible.

…not required

…itor

millerdev

nit: rebase or force-push in general after reviews have been submitted is disorienting if it modifies any commits that have been reviewed because comments become disassociated with the commits they were reviewing. Better to merge master into the branch, assuming that was the intent of the rebase.

ajeety4 · 2023-04-07T07:02:27Z

nit: rebase or force-push in general after reviews have been submitted is disorienting if it modifies any commits that have been reviewed because comments become disassociated with the commits they were reviewing. Better to merge master into the branch, assuming that was the intent of the rebase.

Noted! Thanks Daniel.

ajeety4 · 2023-04-20T11:17:10Z

Update: QA: https://dimagi-dev.atlassian.net/browse/QA-4973 has passed for this.
Will be proceeding with local sanity testing on HQ master follwed by these deploy steps.

ajeety4 · 2023-04-20T14:05:05Z

@millerdev @mkangia
How should this be merged into HQ master ?
I followed the steps here , however the script failed to push to master with below errors:
remote: error: GH006: Protected branch update failed for refs/heads/master. remote: error: At least 1 approving review is required by reviewers with write access. 5 of 5 required status checks are expected.

mkangia · 2023-04-20T14:51:56Z

@ajeety4
Can you share the process?
Do you need to rebuild vellum and put that in a PR for commcare-hq repo?

millerdev · 2023-04-20T17:07:58Z

I think you will need to omit the --push option when running vellum-to-hq and make a PR with the changes it creates. I updated the docs.

ajeety4 added awaiting QA product/all-users-all-environments product/invisible and removed product/all-users-all-environments labels Mar 31, 2023

ajeety4 changed the title ~~Update CKEditor to current latest version (4.21.0)~~ Update CKEditor to latest version (4.21.0) Mar 31, 2023

millerdev reviewed Mar 31, 2023

View reviewed changes

ajeety4 marked this pull request as ready for review April 3, 2023 08:32

ajeety4 added the review. don't merge label Apr 3, 2023

mkangia reviewed Apr 4, 2023

View reviewed changes

ajeety4 force-pushed the aj/update-cke-editor branch from 025a0c5 to 2e5ec1a Compare April 5, 2023 11:17

ajeety4 requested a review from millerdev April 5, 2023 14:45

ajeety4 force-pushed the aj/update-cke-editor branch from 764a19b to 2e5ec1a Compare April 6, 2023 09:44

millerdev reviewed Apr 6, 2023

View reviewed changes

ajeety4 added 9 commits April 6, 2023 19:54

build config changes for latest ver, plugin floatingspace removed as …

8ca7884

…not required

update cke editor to 4.21.0

3b4edea

nit: set ck editor toolbar to empty since not used

d21d9d4

update richtext html handler for additional elements created by ck ed…

5f99b81

…itor

fix: copy widget test cases failure due to aysnc handler in ck editor

4ac877f

ckeditor readme build steps update

1fdb728

test cases for additional html elements copying widgets

dff83fa

nit: html to text regex update for widgets additional elems

432c5bf

nit: test cases remove duplication

6005cab

ajeety4 force-pushed the aj/update-cke-editor branch from 2e5ec1a to 6005cab Compare April 6, 2023 14:28

millerdev approved these changes Apr 6, 2023

View reviewed changes

ajeety4 added passed QA and removed awaiting QA review. don't merge labels Apr 20, 2023

ajeety4 merged commit 936c8fb into master Apr 20, 2023

ajeety4 deleted the aj/update-cke-editor branch April 20, 2023 11:18

ajeety4 mentioned this pull request Apr 21, 2023

vellum:b8e74d2c3ce5de0961042844f1827a325e5370c3 updates CKEditor to 4.21.0 dimagi/commcare-hq#32861

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update CKEditor to latest version (4.21.0) #1081

Update CKEditor to latest version (4.21.0) #1081

ajeety4 commented Mar 31, 2023 •

edited

Loading

millerdev Mar 31, 2023

ajeety4 Apr 3, 2023 •

edited

Loading

millerdev Apr 6, 2023

ajeety4 Apr 6, 2023

millerdev Mar 31, 2023

ajeety4 Apr 3, 2023 •

edited

Loading

millerdev Mar 31, 2023

ajeety4 Apr 3, 2023 •

edited

Loading

millerdev Mar 31, 2023

millerdev Mar 31, 2023

ajeety4 Apr 3, 2023

ajeety4 Apr 6, 2023

ajeety4 commented Apr 3, 2023

mkangia left a comment

mkangia Apr 4, 2023

ajeety4 Apr 4, 2023

mkangia commented Apr 4, 2023 •

edited

Loading

ajeety4 commented Apr 4, 2023 •

edited

Loading

ajeety4 commented Apr 4, 2023

ajeety4 commented Apr 5, 2023

ajeety4 commented Apr 6, 2023

millerdev Apr 6, 2023 •

edited

Loading

millerdev Apr 6, 2023

ajeety4 Apr 6, 2023

millerdev Apr 6, 2023

millerdev left a comment

ajeety4 commented Apr 7, 2023

ajeety4 commented Apr 20, 2023

ajeety4 commented Apr 20, 2023

mkangia commented Apr 20, 2023

millerdev commented Apr 20, 2023

	The repo [dimagi/ckeditor-dev](https://github.com/dimagi/) which is fork of upstream repo is used for creating a build from source using the branch vellum-build.
	The repo [dimagi/ckeditor-dev](https://github.com/dimagi/ckeditor-dev) which is fork of upstream repo is used for creating a build from source using the branch vellum-build.

		.replace(/<span\b[^>]?id="?cke_bm_\d+(\w)"?\b[^>]?>.*?<\/span>/ig, "")
		.replace(/<span\b[^>]?data-cke-copybin-(start\|end)[^<]?<\/span>/ig, "")

	describe("text conversions", function() {
	var text = [
	["blah\nblah", "<p>blah</p><p>blah</p>"],
	["blah\nblah\n", "<p>blah</p><p>blah</p><p> </p>"],
	[
	"blah\n\nblah\n\n",
	"<p>blah</p><p> </p><p>blah</p><p> </p><p> </p>"
	],
	[
	"list\n* item\n* item",
	"<p>list</p><p>* item</p><p>* item</p>"
	],
	[
	"list\n\n* item\n* item",
	"<p>list</p><p></p><p>* item</p><p>* item</p>"
	],
	[
	"list\n\n\n* item\n* item",
	"<p>list</p><p></p><p></p><p>* item</p><p>* item</p>"
	],
	[" ", " "],
	[" ", " \xa0 "],
	[" ", "   "],
	["' ,", "'\u200B,"],
	];

	_.each(text, function(val){
	it("from html to text: " + JSON.stringify(val[1]), function() {
	assert.strictEqual(richText.fromRichText(val[1]), val[0]);
	});
	});

	_.each(text, function(val){
	it("(text -> html -> text): " + JSON.stringify(val[0]), function() {
	assert.strictEqual(
	richText.fromRichText(richText.toRichText(val[0])),
	val[0]
	);
	});
	});
	});

Update CKEditor to latest version (4.21.0) #1081

Update CKEditor to latest version (4.21.0) #1081

Conversation

ajeety4 commented Mar 31, 2023 • edited Loading

Summary

Feature Flag

Product Description

Safety Assurance

Automated test coverage

QA Plan

Safety story

Rollback instructions

Note for Reviewers

Choose a reason for hiding this comment

ajeety4 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 commented Apr 3, 2023

mkangia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkangia commented Apr 4, 2023 • edited Loading

ajeety4 commented Apr 4, 2023 • edited Loading

ajeety4 commented Apr 4, 2023

ajeety4 commented Apr 5, 2023

ajeety4 commented Apr 6, 2023

millerdev Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millerdev left a comment

Choose a reason for hiding this comment

ajeety4 commented Apr 7, 2023

ajeety4 commented Apr 20, 2023

ajeety4 commented Apr 20, 2023

mkangia commented Apr 20, 2023

millerdev commented Apr 20, 2023

ajeety4 commented Mar 31, 2023 •

edited

Loading

ajeety4 Apr 3, 2023 •

edited

Loading

ajeety4 Apr 3, 2023 •

edited

Loading

ajeety4 Apr 3, 2023 •

edited

Loading

mkangia commented Apr 4, 2023 •

edited

Loading

ajeety4 commented Apr 4, 2023 •

edited

Loading

millerdev Apr 6, 2023 •

edited

Loading