[drupalMedia] Ability to mark image media as "decorative"

The merge request above, modeled on the same UX as the drupalImage plugin, provides the ability to toggle decorative alt text for the drupalMedia, while still preserving the ability to use or override the default alt text from the Media entity.

Even though it's a task seems like the kind of feature that could/should have test coverage.

Moving to NW for failures in MR though.

Even though it's a task seems like the kind of feature that could/should have test coverage.

Yes, based on the comment thread, I'm not sure that "Task" is the right categorization. I'm reading arguments in favor of classifying this as a 'major regression' (bug), and on the other hand, the original issue description reads more like a proposed enhancement.

Regardless, yes, this should have test coverag! I've added that in the recent commits.

Moving to NW for failures in MR though.

I've used Prettier to update the syntax in the JS, but I'd like input from others about the spell checking failure: cspell is complaining about the unknown words "switchbutton" and "switchbuttonview". But those words already exist elsewhere in Drupal core code (see /modules/ckeditor5/js/build/drupalImage.js). So I don't want to inappropriately increase the scope of this change by modifying core/misc/cspell/dictionary.txt, unless others feel that should be done now.

Setting back to "Needs Review" for input on that.

It appears from the demonstration animated gif of the decorative image toggle → that the default is decorative. The default would best be non-decorative, requiring that the editor assertively choose that the image is decorative.

Please clarify as to whether this animation shows the default or reflects an image that was previously set to decorative.

Please clarify as to whether this animation shows the default or reflects an image that was previously set to decorative.

The GIF is on a loop that makes it a little confusing! The default is non-decorative. It's an opt-in thing, for sure!

I've Tested this MR- MR !6391 mergeable on Drupal version- 11.0-dev. The patch was applied successfully and it looks good to me.

Testing Steps:

• Set up Drupal 11.
• Enable the Drupal media for CK editor.
• Goto Content> New node- Add an image using the "Insert Drupal Media" option.
• Select an existing Media library image or upload a new one.
• When the image is inserted, click the "Eye" icon on the contextual toolbar to open the Alt text options. See before results- There is no toggle or way to mark the image as decorative.
• Download the shared MR & apply.
• Goto Content> New node- follow the above steps and re-verify this.

Testing Results:
After applying the patch, the option to mark the image as decorative is available now. Sharing after result.

Tested this out with my accessibility hat on, according to https://www.w3.org/WAI/tutorials/images/decorative/

a null (empty) alt text should be provided (alt="") so that they can be ignored by assistive technologies, such as screen readers.

Confirmed using the ANDI toolbar tester that using this does get flagged.

Thanks for reviewing, smustgrave! Can you provide clarification on what needs work? To my understanding this is designed to render alt="" on the page when the "Decorative" option has been selected, and based on my testing, that is what happens. That seems to match the WAI guideline referenced above. Is that not happening for you?

I can retest but I wasn’t getting any alt text.

Here are the steps I was using to test, for comparison:

1. Installing from the "Standard" profile, enable media_library
2. Under admin/config/content/formats/manage/basic_html, add the "Drupal Media" plugin to the CKEditor toolbar
3. Enable the "Embed media" text format filter and save the text format.
4. Create a node of type "Basic page."
5. Using the Basic HTML toolbar, add an image using the "Media library" plugin that is now present in the toolbar.
6. After the image is inserted into the rich text area, click on it.
7. Click the "Override media image alternative text" icon in the balloon ribbon.
8. Set the "This is a decorative image" toggle to "On"
9. Click the "Save" checkmark in the CKEditor balloon modal.
10. Save the page.
11. View the page. The image should render with alt text in the page source reading alt=""

Ah so I tested with the just the regular image button, which needs a follow up as the decorative feature doesn't add an empty alt text.

The media feature works fine only concern was that without this ticket not sure I would of known to go under the "Override alt" button to turn decorative. Could be just me though

Put in review for other thoughts

Put in review for other thoughts

Thanks! It would be great to hear from others. Specifically....

only concern is that without this ticket not sure I would of known to go under the "Override alt" button to turn decorative. Could be just me though

Placing this toggle within the "Override alt" button (i.e., the eyeball with a strike through it) is simply following precedent established for the drupalImage plugin ( #3222757: [drupalImage] Make image alt text required or strongly encouraged → ). I've updated this issue's description to reflect that this proposes to replicate that UX, not introduce a new UX.

Don't want my comment to be the blocker.

Opened a follow up for #19 🐛 Decorative image feature doesn't add empty alt Active

Did test the MR and verified using media library button it works as expected.

So after the follow up probably good to self RTBC.

My advice, as someone who teaches people what "decorative" means, is to pick a field label that does not assume understanding what "decorative" means -- something that inherently discourages clicking the button unless you know what you are doing.

Something like "This image contains no meaningful information and should be ignored by screen readers"

Another suggestion -- I suggest flipping the order of the fields, so that the dynamic textfield comes after the toggle.

Two reasons --

1. For keyboard users, this is the simplest pattern for making dynamic content discoverable, since tabbing through the page in order means you "hit" the new content automatically.
2. Things inserted after the toggle come between the toggle and the save button. They can't be missed if they appear above the viewport. Having dynamic content above the toggle appearing outside the viewport is most likely for mobile users or users with their browser set to high zoom levels.

One more thing about showing/hiding content:
Since the decorative image does not have an alt text it might be a good idea to hide the default alt-text as well as the override field. In my opinion it would be better express the effect of marking an image decorative.

• @mark_fullmer Thanks so much for pushing this forward! 🤩🙏🙏🙏
• @smustgrave in #19: There's explicit test coverage in HEAD to ensure this works correctly for decorative images. Responded at #3421421-2: [drupalImage] Decorative image toggle doesn't add empty alt → .
• @itmaybejj in #24 + #25: this should have exactly the same UI strings as the drupalImage plugin, to avoid confusion/inconsistency. It's totally possible that it can/should be improved. But that'd require a separate issue then. Can you please open that? 🙏

Review

1. To demonstrate that the UX is consistent between drupalImage and drupalMedia, please add GIFs for both to the issue summary.
2. The UI string used here should match that of drupalImage, it currently does not.
3. I'd like to understand why there's a need to introduce additional classes + CSS, instead of only slightly expanding the existing CSS rules.

I'd like to understand why there's a need to introduce additional classes + CSS, instead of only slightly expanding the existing CSS rules.

Thanks for the question!

The new CSS staged in the MR (i.e., targets for .ck.ck-responsive-form.ck-media-alternative-text-form--with-decorative-toggle) are added to core/modules/ckeditor5/css/drupalmedia.css to replicate the behavior of differently namespaced CSS from core/modules/ckeditor5/css/image.css (i.e., .ck.ck-responsive-form .ck.ck-text-alternative-form__decorative-toggle).

These implementations for drupalImage and drupalMedia could potentially be made D.R.Y. That would require consolidating the alt text switchbutton CSS into a single file and then adding that to both libraries in modules/ckeditor5/ckeditor5.libraries.yml

These implementations for drupalImage and drupalMedia could potentially be made D.R.Y. That would require consolidating

That sounds great! 👍 Let's do that?

FYI: 🐛 [drupalImage] Rename "Text alternative" field label to "Alternative text" Fixed just landed, so we should match that text now.

One thought, based on the GIF in comment #9, I wonder if the toggle should also hide the default alt text, because with that visible but the override option hidden, it kind of implies that the default alt text will be used. I think it would be better to hide the default alt text as well, because once the image is set as decorative, the default alt text is no longer relevant.

[# https://www.drupal.org/project/drupal/issues/3419894] → just landed, so we should match that text now.

To demonstrate that the UX is consistent between drupalImage and drupalMedia...

The UI string used here should match that of drupalImage, it currently does not.

Before we proceed with changes, I think we need to revisit the assumption that the UX, and therefore the text and other elements, is in fact the same/consistent between drupalImage and drupalMedia. Even prior to this issue to add the decorative toggle, drupalImage and drupalMedia were designed differently.

The main functional difference is that, in drupalImage, there is no default alt text to start with. Someone must enter alt text in this UI for the image to have alt text, or they must indicate the image is decorative.

In drupalMedia, alt text from the media entity is used if no override is supplied. Per the current logic, leaving the "Alternative text" field empty will result in the media entity's alt text being used.

So, in drupalImage, this interface is the method for *entering* alt text. In drupalMedia, this interface is the method for *overriding* default alt text.

Given this, I think we must do one of the following:

1. Conclude that these serve different purposes and allow divergence in field labels: drupalImage remains "Alternative text," while drupalMedia is "Override default alternative text" or something to that effect. Screenshot:

2. Rework the existing logic of the drupalMedia implementation: the media entity's alt text would *prepopulate* the "Alternative text" field. Screenshot:

Given this, I think we must do one of the following:
1. ...drupalMedia functions as "Override default alternative text"...
2. ...media entity's alt text *prepopulates* the "Alternative text" field...

My own answer to this question, upon reflection, is that #1 is the less confusing UX, even if it will be different from drupalImage. If we were to do #2, a user who wants to restore the default media entity alt text after overriding it would be hard-pressed to figure out how to do that.

I wonder if the toggle should also hide the default alt text, because with that visible but the override option hidden, it kind of implies that the default alt text will be used.

This is addressed, per the latest commits!

The UI string used here should match that of drupalImage, it currently does not.

The label for the decorative toggle now reads "Decorative toggle," matching that of drupalImage.

To demonstrate that the UX is consistent between drupalImage and drupalMedia, please add GIFs for both to the issue summary.

This is done! See summary.

Trying to keep up with this.

Believe there are 2 open threads on the MR if they could be closed or answered. Left 2 comments/questions myself.

Thanks for the further input, smustgrave & Wim! I've addressed all of the comments and marked as resolved all but one, which I've left an explanation for, and will defer for someone else to confirm the rationale for that business logic.

Back to "Needs review," with updated screenshot in the original issue description!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Moving this back to "Needs review," as I believe the Needs Review Queue Bot's analysis of this no longer applying is incorrect?

Believe all feedback has been addressed in the MR.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I've updated the merge request to incorporate the latest changes from 11.x. Tests are passing again, so I'm setting this back to the "RTBC" status previous to the Needs Review Queue Bot.

So, in drupalImage, this interface is the method for *entering* alt text. In drupalMedia, this interface is the method for *overriding* default alt text.

#32++

The second proposal in #32 could be problematic depending on how it is implemented: if this would result in the alt attribute actually getting populated even when the alternative text is unchanged, it'd cause updates on the Media entity to not propagate down. That'd be a regression compared to today.

But … given #33, that won't be a problem 😄👍

Review

This looks great! And 98% ready. The last 2% are a few small concerns I have wrt the UI strings and documenting the meaning of "" plus the difference compared to how this works in drupalImage 🙏 Once those are addressed, I'll happily re-RTBC :)

The last 2% are a few small concerns I have wrt the UI strings and documenting the meaning of "" plus the difference compared to how this works in drupalImage

Awesome! These should all be addressed via the latest commits. Setting back to "Needs Review"!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core.

Merge conflicts resolved. Setting back to "Needs Review" :)

Feedback appears to be addressed but would like @Wim Leers thumbs up too.

@Wim Leers any objection to marking RTBC?

Believe this one is still good to go.

I think this change needs both a change record and a release note. A frontend framework manager would be better placed to review the code... so once those exist

Also I used the functionality and looked at the source it was width="1600" height="900" alt loading="lazy" ... do we care that it is not alt="" which is what the https://www.w3.org/WAI/tutorials/images/decorative/ documents? I checked this is not what my browser is doing... it looked the same if I use curl. Reading https://stackoverflow.com/questions/75256822/accessibility-alt-or-alt-fo... makes me this this is just fine. phew.

Change record drafted at https://www.drupal.org/node/3442726 →

Setting back to "Needs review"

Change record looks good so removing that tag.

Added release note to the summary

But MR appears to need a manual rebase.

This has been rebased of the last commit on 11.x. Setting back to "Needs Review."

Believe all points have been addressed. Left open for additional reviews for about a month so going to mark.

Did retest and still behaving as advertised.

nod_ → closed merge request !6391

Committed and pushed 8e56f38400 to 11.x and 9c9c730ab6 to 11.0.x and a770ef7b65 to 10.4.x. Thanks!