-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update CKEditor to latest version (4.21.0) #1081
Conversation
src/richText.js
Outdated
@@ -731,6 +735,9 @@ define([ | |||
.replace(/<\/p>/ig, "\n") | |||
.replace(/<br \/>/ig, "\n") | |||
.replace(/( |\xa0|\u2005)/ig, " ") | |||
// While copying widgets with text, CKEditor adds these html elements | |||
.replace(/<span\b[^>]*?id="?cke_bm_\d+(\w)"?\b[^>]*?>.*?<\/span>/ig, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -731,6 +735,9 @@ define([ | |||
.replace(/<\/p>/ig, "\n") | |||
.replace(/<br \/>/ig, "\n") | |||
.replace(/( |\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, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intent of this one to remove the entire span, contents included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tests/richText.js
Outdated
// Wait for CK Editor Async handler (copybin) to complete | ||
setTimeout(function(){ | ||
callback(); | ||
},100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/ckeditor/dimagi-README.md
Outdated
- 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**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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**. |
src/richText.js
Outdated
.replace(/<span\b[^>]*?id="?cke_bm_\d+(\w)"?\b[^>]*?>.*?<\/span>/ig, "") | ||
.replace(/<span\b[^>]*?data-cke-copybin-(start|end)[^<]*?<\/span>/ig, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:
Lines 252 to 292 in 025a0c5
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] | |
); | |
}); | |
}); | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Will be adding them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should "toolbar" now be in this list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@millerdev @ajeety4 |
Yes, I will be creating a QA ticket for this. |
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. |
025a0c5
to
2e5ec1a
Compare
@millerdev @mkangia (FYI)
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. |
764a19b
to
2e5ec1a
Compare
FYI: PR for updating ubuntu version in workflow: #1082 |
tests/richText.js
Outdated
setTimeout(function(){ | ||
callback(); | ||
},100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
setTimeout(function(){ | |
callback(); | |
},100); | |
setTimeout(callback, 100); |
tests/richText.js
Outdated
['This dob: <output value="#case/dob" /> is of child', prefix_html + 'This dob: ' + widget_html + ' is of child' + suffix_html], | ||
['This dob: <output value="#case/dob" />', prefix_html + 'This dob: ' + 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: <output value="#case/dob" /> is of child'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/richText.js
Outdated
@@ -731,6 +735,9 @@ define([ | |||
.replace(/<\/p>/ig, "\n") | |||
.replace(/<br \/>/ig, "\n") | |||
.replace(/( |\xa0|\u2005)/ig, " ") | |||
// While copying widgets with text, CKEditor adds these html elements | |||
.replace(/<span\b[^>]*?id="?cke_bm_\d+(\w)"?\b[^>]*?>.*?<\/span>/ig, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
2e5ec1a
to
6005cab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Update: QA: https://dimagi-dev.atlassian.net/browse/QA-4973 has passed for this. |
@millerdev @mkangia |
@ajeety4 |
I think you will need to omit the |
Summary
toolbar
was included by default as clipboard is dependent on it. Pluginfloatingspace
was removed as it is not required and also because it created an empty toolbar on the rich text editor.Feature Flag
Product Description
Safety Assurance
Automated test coverage
QA Plan
https://dimagi-dev.atlassian.net/browse/QA-4973
Safety story
Rollback instructions
Note for Reviewers