Problem/Motivation

Port \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testDialogAccess(). See #3.

The alt_field setting for image media must be respected.

Steps to reproduce

  1. Install Drupal with the Standard profile.
  2. Create a CKE5 instance with a Media button.
  3. Go to /admin/structure/media/manage/image/fields/media.image.field_media_image and disable the alt field.
  4. Insert an image using the media functionality in CKE5.
  5. 🐛 Observe that the Override media image alternative text button is present when it should not be.

Proposed resolution

  • Tests.
  • Fix.

Remaining tasks

Reviews!

User interface changes

The Override media image alternative text button only appears anymore when it should.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#9 3275120-9.patch5.47 KBWim Leers
#9 interdiff.txt1.91 KBWim Leers
#5 3275120-5.patch5.18 KBWim Leers
#5 interdiff.txt1.16 KBWim Leers
#4 3275120-4.patch4.15 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [PP-1] Port MediaTest::testDialogAccess() » [drupalMedia] Port MediaTest::testDialogAccess()
Status: Postponed » Active
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [drupalMedia] Port MediaTest::testDialogAccess() » [drupalMedia] alt_field setting on "Image" media not respected
Version: 10.0.x-dev » 9.5.x-dev
Category: Task » Bug report

This is irrelevant because:

  1. The CKEditor 5 media integration does not use \Drupal\media\Form\EditorMediaDialog.
  2. The CKEditor 5 media integration achieves the same functionality without having to rely on sever-rendered dialog updates, it does everything client-side, making for a much smoother UX.
  3. It could be argued that the same functional behavior should be tested. Fortunately, it is! There is no more need to explicitly test that a PHP-based dialog respects text format settings because the ckeditor5_drupalMediaCaption CKE5 plugin literally only loads if the filter_caption filter is enabled, same for media_mediaAlign and filter_align.

… the one exception is … that the alt_field setting on "image" media is not respected yet 😬☹️ That's this piece of the CKE4 test coverage:

    // Test that setting the media image field to not display alt field also
    // disables it in the dialog.
    FieldConfig::loadByName('media', 'image', 'field_media_image')
      ->setSetting('alt_field', FALSE)
      ->save();

Even if one disables that (for example, for purely decorative images that do not need descriptions, however farfetched that might be), CKE5 will continue to show the alt override UI in its media toolbar.

So … converting this task into a bug 😬

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » Wim Leers
Issue summary: View changes
Status: Active » Needs review
FileSize
3275120-4.patch4.15 KB

Here's a test.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Wim Leers » Unassigned
Issue summary: View changes
FileSize
interdiff.txt1.16 KB
3275120-5.patch5.18 KB

And here's the fix.

The last submitted patch, 4: 3275120-4.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs review » Needs work

Most of the feedback I thought I was going to provide was addressed by remembering that \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testAlt already does what I wanted done (it's just not in the patch because it already exists).

What remains is just doc nits:

  1. +++ b/core/modules/ckeditor5/src/Controller/CKEditor5MediaController.php
@@ -104,9 +104,13 @@ public function mediaEntityMetadata(Request $request) {
+      // The "alt" field on image media may be disabled.
+      $settings = $media->{$image_field}->getItemDefinition()->getSettings();
+      if (!empty($settings['alt_field'])) {

    May be clearer to move the comment above the conditional and make it cooperate with the if() such as "Check if the alt field is enabled"

  2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
@@ -627,11 +628,34 @@ public function testEditableCaption() {
+   * Tests the CKEditor 5 media plugin respects media with alt_field disabled.

    s/Tests the/Tests that the

    (this bumps it over the char limit, so it'll probably require something additional)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » Wim Leers
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
interdiff.txt1.91 KB
3275120-9.patch5.47 KB

What do you think?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs review » Reviewed & tested by the community

That looks good to me!

  • lauriii committed 76486e0 on 10.1.x 
    Issue #3275120 by Wim Leers, bnjmnm: [drupalMedia] alt_field setting on...

  • lauriii committed accc4ee on 10.0.x 
    Issue #3275120 by Wim Leers, bnjmnm: [drupalMedia] alt_field setting on...

  • lauriii committed 5459c5f on 9.5.x 
    Issue #3275120 by Wim Leers, bnjmnm: [drupalMedia] alt_field setting on...

  • lauriii committed 373979d on 9.4.x 
    Issue #3275120 by Wim Leers, bnjmnm: [drupalMedia] alt_field setting on...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 9.5.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

I like this fix! 🤩

Committed 76486e0 and pushed to 10.1.x. Also cherry-picked to 10.0.x, 9.5.x, and 9.4.x. Thanks!

Status: Fixed » Closed (fixed)

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