Audit of CKEditor 5 @todo items

Attached patch against 9.5.x

fixed the failed patch #6
Attached inter diff

I verified and tested patch #7 in Drupal 9.5.x version. Patch applied successfully and looks good to me.

Adding parent

Patch no longer applies.

1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
   @@ -663,7 +661,7 @@ public function testAlt() {
        // @todo Uncomment this in https://www.drupal.org/project/ckeditor5/issues/3206522.
        // @codingStandardsIgnoreLine
   -//    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media img[alt=""]'));
   +    // $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media img[alt=""]'));
   

   Actually, #3206522: Add FunctionalJavascript test coverage for media library landed and it forgot to uncomment this test assertion. 🙈 Let's uncomment it and get rid of the @todo! (And hope it passes 😅)

2. +++ b/core/themes/classy/classy.info.yml
   @@ -31,3 +31,5 @@ libraries-extend:
   +ckeditor5-stylesheets:
   +  - css/components/media-embed-error.css
   

   @bnjmnm: Why this addition? We analyzed this in excruciating detail in #3271094: Move Media CKEditor 4 integration into CKEditor in mid-June and AFAICT everything is in the right place now; we do not need to add this (anymore)?

Added reroll of patch #7 on Drupal 9.5.x. and made changes as per comment #11.1.

Keeping the status needs work for point number 2 of comment #11.

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
@@ -722,9 +720,7 @@ public function testAlt() {
-//    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media img[alt=""]'));
+    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media img[alt=""]'));

Looks like this is failing now?! 😬

Conflicts with #3304731: Update remaining tests using Classy to use Starterkit, which landed this morning. Rerolled.

1. +++ b/core/modules/ckeditor5/ckeditor5.module
   @@ -50,10 +50,8 @@ function ckeditor5_help($route_name, RouteMatchInterface $route_match) {
   -      // @todo Uncomment this in https://www.drupal.org/project/ckeditor5/issues/3230230
   -      // $output .= '<li>' . t('HTML tables can be created with table headers and caption/summary elements.') . '</li>';
   -      // @todo Uncomment this in https://www.drupal.org/project/ckeditor5/issues/3222757
   -      // $output .= '<li>' . t('Alt text is required by default on images added through CKEditor (note that this can be overridden).') . '</li>';
   +      $output .= '<li>' . t('HTML tables can be created with table headers and caption/summary elements.') . '</li>';
   +      $output .= '<li>' . t('Alt text is required by default on images added through CKEditor (note that this can be overridden).') . '</li>';
   

   ✅ Both of those issues have been fixed a long time ago!

2. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupallinkmedia/drupallinkmediaediting.js
   @@ -352,13 +352,6 @@ export default class DrupalLinkMediaEditing extends Plugin {
   -    const linkCommand = editor.commands.get('link');
   -    if (linkCommand.automaticDecorators.length > 0) {
   -      throw new Error(
   -        'The Drupal Media plugin is not compatible with automatic link decorators. To use Drupal Media, disable any plugins providing automatic link decorators.',
   -      );
   -    }
   

   🤔 Why this change?

   This is an intentional addition made in #3247683: Disable CKEditor 5's automatic link decorators (in Drupal filters should be used instead), AFAICT we should keep it?

3. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -38,9 +38,6 @@
   - * NOTE: Currently only supports the 'allowed' portion.
   - * @todo Add support for "forbidden" tags in https://www.drupal.org/project/drupal/issues/3231336
   

   ✅ ("forbidden" was removed in core, the issue referenced here was marked as outdated/obsolete because of that!)

   ⚠️ BUT! This actually has already been removed from the 10.0.x branch in 35b8d4f54c5aa0b1ff3a2ecfb85e9d96f246fe64 by #3272516: Deprecate FilterInterface::getHTMLRestrictions()' forbidden_tags functionality — this only continues to exist in the 9.5.x branch. Let's handle this in #3231336-9: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated.

4. +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
   @@ -121,7 +121,9 @@ private function checkNoMarkupFilters(FilterFormatInterface $text_format, Fundam
   -    // @todo Remove in favor of HTMLRestrictions::diff() in https://www.drupal.org/project/drupal/issues/3231336
   +    // @todo Remove in favor of HTMLRestrictions::diff() in https://www.drupal.org/project/drupal/issues/3231336/ @todo
   +    // @todo of a @todo, we need to remove the todo above or create a new issue
   ...
        $html_restrictions = $text_format->getHtmlRestrictions();
        $minimum_tags = array_keys($fundamental->getAllowedElements());
        $forbidden_minimum_tags = isset($html_restrictions['forbidden_tags'])
   @@ -136,6 +138,8 @@ private function checkHtmlRestrictionsAreCompatible(FilterFormatInterface $text_
   
   @@ -136,6 +138,8 @@ private function checkHtmlRestrictionsAreCompatible(FilterFormatInterface $text_
        }
    
        // @todo Remove early return in https://www.drupal.org/project/drupal/issues/3231336
   +    // @todo of a @todo 3231336 is closed.. do we create an issue to remove this\
   +    // or is the early return OK?
        if (!isset($html_restrictions['allowed'])) {
          return;
        }
   

   Ah, yes! We maybe should've done this in #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated instead of marking it outdated/obsolete … so reopened that with a patch to remove it: #3231336-9: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated.

5. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -122,7 +122,6 @@ public function computeSmartDefaultSettings(?EditorInterface $text_editor, Filte
   -      // @todo Remove in https://www.drupal.org/project/ckeditor5/issues/3218985.
   

   🐛 We need to keep this, but the current link is indeed wrong, it should be https://www.drupal.org/project/drupal/issues/3231347.

6. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
   @@ -447,9 +447,6 @@ public function testErrorMessages() {
   -    // @todo Uncomment this in https://www.drupal.org/project/ckeditor5/issues/3194084.
   -    // @codingStandardsIgnoreLine
   -    //$assert_session->responseContains('classy/css/components/media-embed-error.css');
   

   🚢 #3304731: Update remaining tests using Classy to use Starterkit already removed this! — gone in my reroll of #17 👍

7. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
   @@ -628,14 +625,6 @@ public function testEditableCaption() {
   -  /**
   -   * Tests the EditorMediaDialog's form elements' #access logic.
   -   */
   -  public function testDialogAccess() {
   -    // @todo Port in https://www.drupal.org/project/ckeditor5/issues/3245720
   -    $this->markTestSkipped('Blocked on https://www.drupal.org/project/ckeditor5/issues/3245720.');
   -  }
   -
   

   This was pointing to the wrong issue, it should've been updated to #3275120: [drupalMedia] alt_field setting on "Image" media not respected somewhere along the way.

   So I wanted to close that issue by writing a thorough comment. And in doing so, I found a bug: #3275120-3: [drupalMedia] alt_field setting on "Image" media not respected.

8. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
   @@ -722,9 +711,7 @@ public function testAlt() {
   -    // @todo Uncomment this in https://www.drupal.org/project/ckeditor5/issues/3206522.
   -    // @codingStandardsIgnoreLine
   -//    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media img[alt=""]'));
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '[data-media-embed-test-view-mode] img[alt=""]'));
   

   ✅

9. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
   @@ -320,7 +320,7 @@ public function providerAllowingExtraAttributes(): array {
          // Edge case: `style`.
   -      // @todo https://www.drupal.org/project/drupal/issues/3260857
   +      // @todo https://www.drupal.org/project/drupal/issues/3304832
   

   ✅

10. +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
    @@ -65,8 +65,6 @@ class SmartDefaultSettingsTest extends KernelTestBase {
    -    // @todo Remove in https://www.drupal.org/project/drupal/issues/3263384
    -    'ckeditor5_plugin_conditions_test',
    

    ✅

From an ~88K patch to a ~9K patch 👍

+++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
@@ -38,9 +38,6 @@
- * NOTE: Currently only supports the 'allowed' portion.
- * @todo Add support for "forbidden" tags in https://www.drupal.org/project/drupal/issues/3231336
- *
  * @internal
  */
 final class HTMLRestrictions {
@@ -308,7 +305,7 @@ public static function fromTextFormat(FilterFormatInterface $text_format): HTMLR

@@ -308,7 +305,7 @@ public static function fromTextFormat(FilterFormatInterface $text_format): HTMLR
    * @return \Drupal\ckeditor5\HTMLRestrictions
    */
   private static function unrestricted(): self {
-    // @todo Refine in https://www.drupal.org/project/drupal/issues/3231336, including adding support for all operations.
+    // @todo Refine in https://www.drupal.org/project/drupal/issues/3231336
     $restrictions = HTMLRestrictions::emptySet();
     $restrictions->unrestricted = TRUE;
     return $restrictions;
@@ -344,8 +341,7 @@ private static function fromObjectWithHtmlRestrictions(object $object): HTMLRest

@@ -344,8 +341,7 @@ private static function fromObjectWithHtmlRestrictions(object $object): HTMLRest
 
     $restrictions = $object->getHTMLRestrictions();
     if (!isset($restrictions['allowed'])) {
-      // @todo Handle HTML restrictor filters that only set forbidden_tags
-      //   https://www.drupal.org/project/ckeditor5/issues/3231336.
+      // @todo Refine in https://www.drupal.org/project/drupal/issues/3231336

👍 If #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated lands first, this will need to be rerolled. But +1 for this, because it'll ensure that all remaining @todos make sense 😊

1. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -308,7 +305,7 @@ public static function fromTextFormat(FilterFormatInterface $text_format): HTMLR
   +    // @todo Refine in https://www.drupal.org/project/drupal/issues/3231336
   
   +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
   @@ -121,7 +121,7 @@ private function checkNoMarkupFilters(FilterFormatInterface $text_format, Fundam
   +    // @todo Simplify in https://www.drupal.org/project/drupal/issues/3231336
   
   +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
   @@ -720,9 +720,7 @@ public function testAlt() {
        // Verify that the two double quote empty alt indicator ('""') set in
        // the dialog has successfully resulted in a media image field with the
        // alt attribute present but without a value.
   

   Should we update this?

2.     // @todo Nothing in Drupal core uses this ability, and no custom/contrib
       //   module is known to use this. Therefore this is left for the future.

   This @todo exists in \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::providerConstruct. Should we remove it?

3. This needs a Drupal 10 patch also 😇

1. That's being fixed in #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated. If you want to see this truly fixed, please review/RTBC that issue 😇
2. Nice catch! That still needs its own issue indeed.
3. … and that would become simpler if #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated is in 😊😊

Let's postpone this on #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated. Per #22 that'll make things simpler. Ready for review there!

Manually checked, ready to ship! 🚢

Random failures in the CKEditor 4 module. 🤷‍♀️ Unrelated.

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
@@ -720,9 +720,7 @@ public function testAlt() {
     // Verify that the two double quote empty alt indicator ('""') set in
     // the dialog has successfully resulted in a media image field with the
     // alt attribute present but without a value.

We still need to update this. It was mentioned as part of #21 but looks like the comment was pretty confusing 🤦‍♂️

Committed 3f094e0 and pushed to 10.1.x. Also cherry-picked to 10.0.x and 9.5.x. Thanks!

This commit introduced a broken @todo link (https://www.drupal.org/project/ckeditor5/issues/3231347).

I have created an issue - to fix this and also two additional broken links (not related with this). See: #3310760: Broken issue links in @todos

This should be cherry-picked to 9.4.x too. #29 applies. 👍 Test queued.

Committed 00f7077 and pushed to 9.4.x. Thanks!

Thanks!

Restoring prior state.

Since this was backported to 9.4.x, the previous version was correct.