Update CKEditor library to 4.2 [#2036253]

CKEditor promises to bring a number of improvements we probably want to leverage:

handle textarea[required] attributes
Allow list-only indentation
New jQuery adapter
Are http://dev.ckeditor.com/ticket/10042 and http://dev.ckeditor.com/ticket/9923 relevant to us?

At the moment, no more issues are open for 4.2, so it probably won't roll out much later than its due date in mid-July, see: http://dev.ckeditor.com/roadmap
Postponing this until 4.2 release, though.

This blocks:
#2035647: Use CKEditor 4.2's new ability to only allow indentation of lists

As part of this issue:
#2035647: Use CKEditor 4.2's new ability to only allow indentation of lists
#2032341: Remove 'image' and 'link' plugins from bundled CKEditor build
#2035615: Use CKEditor 4.2's new onChange event rather than our temporary work-around
#2035653: Optimize CKEditor's build-config.js

Comment	File	Size	Author
#14	ckeditor_4.2-2036253-14.patch.zip	690.1 KB	Wim Leers
#14	interdiff.txt	824 bytes	Wim Leers
#11	interdiff-build_config.js_.txt	2.2 KB	Wim Leers
#11	interdiff.txt	4.9 KB	Wim Leers
#8	ckeditor_4.2-2036253-8.patch.zip	690.34 KB	Wim Leers

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 6 July 2013 at 17:00

There's also a reference to jQuery 4.2 in #1879120-37: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 7 July 2013 at 07:20

Issue tags:

+sprint, +Spark, +CKEditor in core

Comment #2.0

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 7 July 2013 at 16:09

Issue summary:

View changes

replace workaound issue by "remove workaround" issue

Comment #2.1

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 10 July 2013 at 11:09

Issue summary:

View changes

Reference last CKEditor update issue

Comment #3

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 10 July 2013 at 20:37

Title:

Switch to CKEditor 4.2 upon availability

» Update CKEditor library to 4.2

Reorganized the issue summary a bit. Also added some more links.

Comment #4

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 19 July 2013 at 11:12

Status:

Postponed

» Active

CKEditor 4.2 has been released yesterday: http://ckeditor.com/release/CKEditor-4.2
So we can go and update the library. Who rolls a patch? I won't have time before tonight.

Dropped compatibility support for Internet Explorer 7 is another nice aspect of the release.

If CKEDITOR(inline) crashes when using ol/ul list should be a major problem, we'd have to wait until 4.2.1, but from the comments it seems that it doesn't crash anymore, just has to be further improved.

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 11:16

Assigned:

Unassigned

» Wim Leers

Indeed! And, as planned, working on this now, during this afternoon :)

Comment #6

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 19 July 2013 at 11:19

Awesome! So maybe we can continue with #2035647: Use CKEditor 4.2's new ability to only allow indentation of lists later tonight?

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 11:24

#6: That will be done today, yes :)

Comment #7.0

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 11:24

Issue summary:

View changes

Added some links, reorganized stuff

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 13:18

Title:	Update CKEditor library to 4.2	» Update CKEditor to 4.2
Status:	Active	» Needs review

File	Size
ckeditor_4.2-2036253-8.patch.zip	690.34 KB

So, having worked on this, I came to the following conclusions:

#2035647: Use CKEditor 4.2's new ability to only allow indentation of lists (one line added to build-config.js) must be done as part of this issue, otherwise you won't be able to do indentation of nested lists anymore through CKEditor, so merging that issue with this one.
#2032341: Remove 'image' and 'link' plugins from bundled CKEditor build (two lines deleted from build-config.js) could be done separately, but there is no point in doing it separately, it just results in more overhead, so merging that issue with this one.
#2035615: Use CKEditor 4.2's new onChange event rather than our temporary work-around could be done separately, but it's just removing a work-around in core/modules/ckeditor/js/ckeditor.js in favor of what was introduced in CKE 4.2, so merging that issue with this one.
#2035653: Optimize CKEditor's build-config.js is just removing a bunch of useless lines in build-config.js, it's kind of silly to do that in a separate issue, so merging that issue with this one.

So instead of 5 tiny issues, it is now 1 small issue. Less reviewer, committer and testbot overhead. Better. Simpler. The tiny issues were better for tracking, but due to the nature of the task (committing an updated version of CKEditor), it's just easier to do them all at once.

Patch review guidance

build-config.js: the "adapters" directory is new in CKE 4.2 and we don't need it in Drupal, hence we mark it to be removed; as per #2035647: Use CKEditor 4.2's new ability to only allow indentation of lists, added indentlist; as per #2032341: Remove 'image' and 'link' plugins from bundled CKEditor build, removed link and image; as per #2035653: Optimize CKEditor's build-config.js, removed a bunch of useless lines
interdiff.txt: changes in ckeditor.module: to update version number; changes in core/modules/ckeditor/js/ckeditor.js to leverage CKE 4.2's new onChange event; changes in the drupallink and drupalimage plugins as per #2032341: Remove 'image' and 'link' plugins from bundled CKEditor build

All other changes are just for the updated CKEditor itself.

Drupal.org limits uploads to 3 MB, but the patch is 3.4 MiB/3.2 MB… So I had to zip it, unfortunately. Can somebody maybe give me permission to upload bigger files, or just increase the upload size?

Comment #8.0

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 13:18

Issue summary:

View changes

Remove incorrect blocker mentions.

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 13:20

Title:

Update CKEditor to 4.2

» Update CKEditor library to 4.2

Didn't mean to change title, sorry.

Comment #10

nod_

French

Lille

CreditAttribution: nod_ commented 19 July 2013 at 14:24

Status:

Needs review

» Reviewed & tested by the community

Changes in ckeditor module are all good

I guess you forgot to upload a couple of files but zip is good to go.

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 18:12

File	Size
interdiff.txt	4.9 KB
interdiff-build_config.js_.txt	2.2 KB

Here are the interdiffs that should've been attached to #8 and to which I refer in #8.

(They're also what nod_ meant by the missing files in #10.)

Comment #12

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 20 July 2013 at 00:44

Status:

Reviewed & tested by the community

» Needs work

Regarding the translations, Wim wrote in #2:

we also shouldn't list a specific set of languages to include in build-config.js, because then newly added translations won't be picked up.

Well I'm not sure we want to pick up whatever is offered to us, because we don't want to ship CKEditor translations for languages we're currently not supporting with a Drupal translation, such as currently sr-latn, fr-ca or en-** and probably more to come. So optimally we would be using LanguageManager::getStandardLanguageList.
Either way we need to find ways to match language codes like pt-pt => pt, no => nn, zh-cn => zh-hant, and zh => zh-hans (Let this be a followup, though).

Secondly, the docs in build-config.js should be updated as well:

 * At the time of writing, this build is identical to the "standard" build of
 * CKEditor, includes all languages, excludes the "placeholder" and includes the
 * "sourcedialog" plugin.

// @todo D8: CKEditor Widgets is not available in 4.1 RC, and we're not yet

Also it might be helpful to list all currently existing plugins, setting the excluded ones to 0, so it remains clear which omissions are on purpose and which plugins might be missing because they were (re-)added in future CKEditor releases. Optimally we would have a short reason per omitted plugin.
Same holds for languages.

Otherwise looks good. You probably already did a successful test run, did you?

Comment #13

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 20 July 2013 at 00:55

Also, I'm not sure if it's possible or makes sense to have contrib add plugins that were excluded here.
Regarding the excluded 'indentblock' plugin, you said in #2035647-4: Use CKEditor 4.2's new ability to only allow indentation of lists that this would only be possible in contrib. Is it then? I've no clue but we should make sure it is possible.

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 25 July 2013 at 10:49

Status:

Needs work

» Reviewed & tested by the community

File	Size
interdiff.txt	824 bytes
ckeditor_4.2-2036253-14.patch.zip	690.1 KB

#12:

1. mapping Drupal languages to CKEditor languages: you're right, and apparently I did not yet have an issue for this. Now I do: #2050097: Map CKEditor languages to Drupal languages. Thanks!
2. Fixed. I removed that entirely because now it is much more Drupal-specific. Plus, it's trivial to compare Drupal's build-config.js to CKEditor's defaults (basic/standard/full) by just diffing the files. The CKEditor Widgets todo must remain, because that is still not yet in there, only once #2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images lands, I'll be able to remove that todo.
3.

Also it might be helpful to list all currently existing plugins, setting the excluded ones to 0, so it remains clear which omissions are on purpose and which plugins might be missing because they were (re-)added in future CKEditor releases. Optimally we would have a short reason per omitted plugin.
Same holds for languages.

No. Go to http://ckeditor.com/builder. There are many dozens plugins. New ones get added every week or so. New languages get added every month or so. Neither of this makes sense. It'd be like listing every Drupal module on drupal.org in a Drush .make file and then marking them as "don't download/install this one". I veto this.

#13:

Also, I'm not sure if it's possible or makes sense to have contrib add plugins that were excluded here.

Yes, this is of course possible. :) Many months ago, we made sure that that would be possible.

Having fixed the docs in build-config.js, and having created a new issue for the relevant remaining problem, this is now back to RTBC.

Comment #15

nod_

French

Lille

CreditAttribution: nod_ commented 25 July 2013 at 12:55

rtbc +1

Comment #16

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 27 July 2013 at 22:12

Status:

Reviewed & tested by the community

» Fixed

I've confirmed that all the included issues have been included, I've reviewed changes to non ckeditor code, and I've manually tested - all looks good.

Committed 7aae5d3 and pushed to 8.x. Thanks!