Language toolbar item cannot be removed from the toolbar

😬

I have no idea where to start fixing this, but if someone can give me a nudge in the right direction, I'll give it a try.

Meanwhile, here's a workaround if someone wants to disable the language selector:

• Export configuration (drush cex).
• Open the editor.editor.[format].yml file for the input format you want to change.
• Remove the line - textPartLanguage under settings.toolbar.items.
• Remove the line ckeditor5_language: and the line with language_list: below it.
• Save the file and import configuration (drush cim).

I tried to understand why this is happening. It seems that the validation (\Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemDependencyConstraintValidator) kicks off and sets an error before
\Drupal\ckeditor5\Plugin\Editor\CKEditor5::injectPluginSettingsForm has the chance to remove the settings form. I was not (yet) able to figure out why this happens, or why it does not happen when removing the Heading plugin (\Drupal\ckeditor5\Plugin\CKEditor5Plugin\Heading).

Hi again, @marcvangend! Long time no see! 😄

#6 is accurate: that is the valid/viable work-around today.

#7:

1. Great research! 👏
2. Do you think you could add test coverage for this? 🤞 A new test method in \Drupal\Tests\ckeditor5\FunctionalJavascript\AdminUiTest seems appropriate.

3. I was not (yet) able to figure out why this happens, or why it does not happen when removing the Heading plugin (\Drupal\ckeditor5\Plugin\CKEditor5Plugin\Heading).

   → only the language button has the CKEditor5ToolbarItemDependencyConstraint constraint

   That also explains why this does not happen for the Heading or Source Editing plugins (and their respective buttons).

   IIRC textPartLanguage was the first button with required configuration. For Heading, it was added later. Source Editing was added way later.

   Instead of hardcoding that, I think we should update the config schema and/or validators to always trigger this validation error for any plugins that have both toolbar items ($definition->hasToolbarItems() === TRUE) and configuration ($definition->isConfigurable() === TRUE).

4. I've attached a patch created an MR that fixes the problem. We still need to fix the two points 2 and 3 though!
5. Finally, I think we should look into updating the logic I wrote in this patch to only apply it when that config schema validation constraint is actually applicable?

For point 3 in #9: we can check the https://www.drupal.org/project/ckeditor5 commit history and issues to get clarity on this if we need to, but I think #9 is sufficient for now 😊

First manual test result: the fix works as expected.

I will come back to this later.

Quick issue summary update; will need to refine later.

There, da579db4f0b3957fbf933ea338a8122d9950f0f7 should help you be more productive if you choose to tackle this, @marcvangend :D

OK, I've been trying to get a test working, but I'm stuck at the point where I need to add the language button to the active toolbar. I have tried triggering the arrow down key event in different ways:

    $language_button = $page->find('css', '[data-drupal-selector="ckeditor5-toolbar-button"][data-id="textPartLanguage"]');
    $language_button->focus(); // Also tried without focus().
    $language_button->keyPress(40); // Also tried: keyDown(), keyUp().
    $assert_session->assertWaitOnAjaxRequest();

I have also tried dragging the button onto the active toolbar:

    $language_button = $page->find('css', '[data-drupal-selector="ckeditor5-toolbar-button"][data-id="textPartLanguage"]');
    $active_toolbar = $page->find('css', '[data-button-list="ckeditor5-toolbar-active-buttons"]');
    $language_button->dragTo($active_toolbar);
    $assert_session->assertWaitOnAjaxRequest();

Tips are appreciated.

Awesome — thank you so much! 🙏

You can find an example in \Drupal\Tests\ckeditor5\FunctionalJavascript\LanguageTest::test():

    $this->assertNotEmpty($assert_session->waitForElement('css', '.ckeditor5-toolbar-item-blockQuote'));
    $this->triggerKeyUp('.ckeditor5-toolbar-item-blockQuote', 'ArrowDown');
    $assert_session->assertWaitOnAjaxRequest();

→ interpretation: first wait for it, then trigger the right keyboard event, then wait for the AJAX request and response to get processed.

Once that's done, you can move on to the next operation in your test!

Nice, good to see that someone has already solved this problem for me :-)

Seeing this, I noticed that we already have \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::testLanguageConfigForm. Wouldn't it make sense to expand that test, instead of adding a new one in AdminUiTest? (Maybe we need to have a good look at the naming and organisation of all test classes in core/modules/ckeditor5/tests/src/FunctionalJavascript. It seems a bit messy and inconsistent to me, but maybe I'm just blind to the logic.)

Ahhhh — yes! I didn't spot that test method, because it's in an odd location 😬 I think we should move it into AdminUiTest because it not testing anything related to allowed tags, despite the name of its current class!

(Maybe we need to have a good look at the naming and organisation of all test classes in core/modules/ckeditor5/tests/src/FunctionalJavascript. It seems a bit messy and inconsistent to me, but maybe I'm just blind to the logic.)

+1! There is some logic to it though:

1. (Image|Media|MediaLibrary)Test each test the functionality of those CKE5 plugins
2. CKEditor5OffCanvasTest tests CKE5's compatibility with Drupal core's off-canvas functionality
3. CKEditor5CKEditor4Compatibility tests that CKE5 and CKE4 can execute in the same document
4. AdminUiTest tests general admin UI functionality
5. CKEditor5AllowedTagsTest tests the impact of admin UI interactions on the list of allowed HTML tags — I think it should be renamed to AdminUiFilterHtmlTest or something like that — would you like to do that? 🤞
6. LanguageTest really tests CKE5's translation support — I think it should be renamed — would you like to do that? 🤞
7. CKEditor5Test is kind of a catch all which should also be scrutinized. At a minimum I think ::testLanguageOfPartsPlugin() should be moved into its own test, similar to the first bullet in this list

Sounds like a plan. I created a follow-up issue for the shuffling and renaming: #3259555: Organize and rename CKEditor 5 tests and test classes

Adding the test-only patch for review.

Expected test failure.

Test looks good, so lets remove the filtering on test groups and run the fix with the full suite.

3766 failures — huh! That's not possible.

All of them are like this:

Remaining direct deprecation notices (1)

  1x: The "Drupal\Tests\Listeners\DrupalListener" class implements "PHPUnit\Framework\TestListener" that is deprecated.
    1x in DrupalListener::endTest from Drupal\Tests\Listeners

I don't see why/how this can happen. I wonder if it's because it changes drupalci.yml? Re-testing…

Ah, apparently this was caused by an upstream change and it got fixed 1 minute ago: #3259744: PHPUnit 9.5.12 (released 2022-01-21) throws unhandled deprecation notice on "Drupal\Tests\Listeners\DrupalListener". 👍

The new test should fix this.

Thanks Wim. Test result is green now.

Thank you so much, @marcvangend!

I finally did update the issue summary. I incorporated #9 in it. I made the next steps more detailed. Would you be interested in tackling remaining task #3 too? It's a very interesting part of Drupal IMHO 🤓🤞

Aaaaaaaactually … I think that that remaining task #3 would make for a great follow-up. Did that: #3259795: Generalize textPartLanguage's validation: no configuration allowed unless the plugin's toolbar item is active.

So if I'm not mistaken, there is no more work to be done here.

Sorry, YES! I just can't RTBC this because I wrote the fix. Definitely needs review!

In that case, I'll review :)

Fix looks good, we have a failing => passing test that looks great.

NB. The test was moved the a more logical spot, the new test fully covers the deleting of the test in the old spot, great!

Discussed with @lauriii. We agreed that his suggestion makes sense — it was only a matter of subjective clarity and objective robustness. We're on the same page now 👍

This is indeed more robust, so +1 on that change, nice one.

Leaving RTBC since it never got moved away from that.

Follow-up #3259555: Organize and rename CKEditor 5 tests and test classes is ready to be reviewed.