Problem/Motivation

Per https://ckeditor.com/blog/ckeditor-5-v26.0.0-with-extensible-builds-inli..., CKEditor 5 v26 brings back the "language of parts" plugin from CKEditor 4 — see https://github.com/ckeditor/ckeditor5/issues/8989.

Drupal 8 core added this in the form of \Drupal\ckeditor\Plugin\CKEditorPlugin\Language in #1993928: Language of parts: Introduce a language toolbar button.

This is important to not regress in functionality compared to CKEditor 4.

We already updated to CKEditor 5 v26 in #3202334: Update to CKEditor5 26.0.0, so this should be relatively straightforward.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ckeditor5-3205565

Command icon Show commands

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

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

Comments

Wim Leers created an issue. See original summary.

zrpnr’s picture

Issue tags: +d10readiness, +NorthAmerica2021

Tagging for Contribution event

gábor hojtsy’s picture

Issue tags: -d10readiness +Drupal 10

Fixing tags.

anand.toshniwal93’s picture

Assigned: Unassigned » anand.toshniwal93

working on this so assigning to myself

anand.toshniwal93’s picture

Assigned: anand.toshniwal93 » Unassigned
Status: Active » Needs review
StatusFileSize
new123.67 KB
new249.21 KB

language of parts

language of parts

I have converted PNG to SVG for the language icon as CKEditor doesn't provide an icon for it. Please check if we need to update it.

anand.toshniwal93’s picture

Status: Needs review » Needs work

Need to provide TextPartLanguageOption.

anand.toshniwal93’s picture

Assigned: Unassigned » anand.toshniwal93
anand.toshniwal93’s picture

Assigned: anand.toshniwal93 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new169.74 KB

I was trying to follow the same pattern as "imageUpload" for id but facing the below error so I set the plugin id to "language".

Drupal\Core\Config\ConfigValueException: ckeditor5.language key contains a dot which is not supported. in Drupal\Core\Config\ConfigBase->validateKeys() (line 211 of core/lib/Drupal/Core/Config/ConfigBase.php).

wim leers’s picture

Status: Needs review » Needs work

I wanted to RTBC this, and while this looks great already … the configuration parts of this do not yet work 😅 Any chance you could fix that? 😊🙏

anand.toshniwal93’s picture

@Wim Leers Forgot to upload the screenshot showing config works, please check the attached screenshots. The screenshot previously attached were before creating the configuration form.

In CKEditor 5, AFAICT the default plugin configuration really should be set in ckeditor5.ckeditor5.yml, under plugin_config.

will work on this.

anand.toshniwal93’s picture

wim leers’s picture

Oh, wow, that does appear to work — which is super surprising because it means that the configuration value that CKEditor 5 accepts is identical to that for CKEditor 4! 🤯

Any chance you could add a test similar to \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5ContribTest::testContribPluginAdded(), where you add this CKEditor 5 plugin, configure it, and check that the expected languages appear in the dropdown when interacting with CKEditor 5 as a content author? 🙏🤓

anand.toshniwal93’s picture

I will work on the test case part.

wim leers’s picture

Thanks, you rock @anand.toshniwal93! 🥳

anand.toshniwal93’s picture

Assigned: Unassigned » anand.toshniwal93

working on this so assigning to myself

wim leers’s picture

🥳

anand.toshniwal93’s picture

After adding this plugin kernel test case is failing due to the order of plugins in case 2

Case 2: The CKEditor 5 layercake plugin is available and the library should be loaded without having to enable plugins.


1) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testEnabledPlugins
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     14 => 'ckeditor5.superscript'
     15 => 'ckeditor5.table'
     16 => 'ckeditor5.underline'
-    17 => 'language'
-    18 => 'ckeditor5_test.layercake'
+    17 => 'ckeditor5_test.layercake'
+    18 => 'language'
 )

This issue might come up with each new plugin added, is it ok to sort an array and assert them or the order of the plugin is important here?

   $default_plugins_with_layercake = array_merge($default_plugins, ['ckeditor5_test.layercake']);
    asort($default_plugins_with_layercake);
    $this->assertSame(array_values($default_plugins_with_layercake), $plugin_ids);
anand.toshniwal93’s picture

In test case we are using $plugin_ids = array_keys($this->manager->getEnabledDefinitions($editor));

getEnabledDefinitions function sorting plugin ids so I think we can use the code from comment 12 .

 public function getEnabledDefinitions(Editor $editor) {
    $definitions = $this->getDefinitions();
    ksort($definitions);
    foreach ($definitions as $plugin_id => $definition) {
      $plugin = $this->createInstance($plugin_id);
anand.toshniwal93’s picture

Assigned: anand.toshniwal93 » Unassigned
Status: Needs work » Needs review

In CKEditor 5, AFAICT the default plugin configuration really should be set in ckeditor5.ckeditor5.yml, under plugin_config.

This is fixed.

Added tests for the language of parts plugin.
Fixed failing kernel test after adding a plugin.

zrpnr’s picture

Status: Needs review » Needs work

This works well, the settings form lets a user pick which set of languages to use, which matches the lists CKEditor 4 loads.
I'm also excited to see the `settingsForm` used here, I think it's the first new plugin that has configuration (besides ImageUpload)
I left some feedback in the issue fork for a few changes, but this is really close, nice work @anand.toshniwal93 !

anand.toshniwal93’s picture

Assigned: Unassigned » anand.toshniwal93

working on this so assigning to myself

wim leers’s picture

As @zrpnr wrote:

For this MR we can leave it as language and if we need to rename the plugins that will be in a follow-up.

Plugin system maintainer @tim.plunkett opened an issue exactly for the problem you encountered — that'd have helped prevent this design choice early on: #3215662: Putting "." in a plugin ID will cause problems if you attempt to store configured plugins via the Configuration API.

We'll open an issue + merge request shortly to change this.

zrpnr’s picture

When I test testLanguageOfPartsPlugin locally it fails, but it seems to be because the dropdown is opening up and it's cut off.
Curious if the testbot will pass still, I can't see where anything changed.

zrpnr’s picture

There's also an incomplete schema exception for editor.editor.ckeditor5:settings.plugins.language "missing schema"

anand.toshniwal93’s picture

Assigned: anand.toshniwal93 » Unassigned
zrpnr’s picture

Status: Needs work » Needs review

This should be back to needs review, I added the schema and rebased off the latest 1.0.x.

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

zrpnr’s picture

Status: Reviewed & tested by the community » Fixed

Updated the postponed issues, thanks!

  • zrpnr committed 2e586e8 on 1.0.x authored by lauriii
    Issue #3215603 by Wim Leers, Reinmar, lauriii: Follow-up for #3205565:...

Status: Fixed » Closed (fixed)

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