Problem/Motivation

We found two bugs related to #3245720: [drupalMedia] Support choosing a view mode for <drupal-media>. The first is a big bug, the second is also a bug but more of an edge case than the first bug and it is preexisting in CKEditor 4 as well.

1. In the embed media filter setting inside the editor configuration, when a user sets the default view mode as something that is NOT checked off as a selectable view mode, the editor fails to initialize because it throws a Javascript error. Although this is due to a prexisting bug in media, a temporary workaround will be added in this issue to prevent the editor from crashing. #3277049: "Embed media" filter allows invalid config of default view mode not being part of view modes selectable was created to fix the preexisting bug.

2. When the user sets a default view mode as a view mode that is not allowed for a certain bundle, it applies that view mode to the element anyway. In my screenshot below, I configured my document media type so that "tiny" view mode is not allowed. However as you can see in the source code, it has "tiny" on the element. (Below screenshot is CKEditor 5 but same thing happens for CKEditor 4). Assuming that this is a preexisting problem with the media module, #3277244: media_embed filter: a default view mode that isn't allowed for a media type gets applied anyway was created for this.

Steps to reproduce

Bug 1:
1. Go to your CKEditor 5 configuration
2. Enable Embed media
3. Select something as the Default view mode
4. Do not check that view mode in the "View modes selectable in the 'Edit media' dialog section
5. Return to editor (editor should fail to load, you should see error in console)

Bug 2:
1. Go to /admin/structure/media/manage/document/display (or whatever media type)
2. In Custom display settings, do not check whatever view mode you want to test with.
3. Go to your editor configuration and under embed media, set the Default view mode to that view mode from #2.
4. Go to your editor and insert a document and check the source code of that element and you should see the view mode that should not be allowed.

Proposed resolution

For Bug #1 come up with a workaround either on the client side or on the PHP side if it is possible. One idea is to force add the Default view mode into the list of drupal element styles on the client side if it is not checked in "view modes selectable in the 'Edit media' dialog.

Another idea is to prevent the editor config from saving if the default view mode is not checked and show an error on the editor page telling the user.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
Screen Shot 2022-04-22 at 11.23.03 AM.png104 KBhooroomoo
Screen Shot 2022-04-22 at 11.12.10 AM.png96.22 KBhooroomoo

Issue fork drupal-3276670

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hooroomoo created an issue. See original summary.

hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
Issue summary: View changes
hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
Issue summary: View changes
hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs work
Issue tags: +Needs followup

1. In the embed media filter setting inside the editor configuration, when a user sets the default view mode as something that is NOT checked off as a selectable view mode, the editor fails to initialize because it throws a Javascript error.

AFAICT \Drupal\media\Form\EditorMediaDialog::buildForm() also only allows selecting the selected view modes, with no fallback behavior to automatically include the default view mode. Which makes no sense. In other words: the validation logic of \Drupal\media\Plugin\Filter\MediaEmbed::settingsForm() is inadequate. Therefore this is a bug in the media module, and deserves a separate issue there.

In other words: I agree with your proposed solution.

Let's move bug 1 into a new issue, so we can keep this issue for bug 2, for which there is less clarity so far.

2. When the user sets a default view mode as a view mode that is not allowed for a certain bundle, it applies that view mode to the element anyway.

What's the behavior of this when using CKEditor 4? I think you're saying it's the same problem?

More importantly: how does this behave in the filter itself, without considering either CKEditor 4 or 5? Looking at \Drupal\media\Plugin\Filter\MediaEmbed::process(), I don't think it's taking into account that the view mode may not exist for the bundle (MediaType). So that'd be another bug in the media module, not in the CKEditor 5 module.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

To clarify:

  • Bug 1 is not a CKEditor 5 bug. Hence the need for a new issue against a component that is not ckeditor5.module.
  • I'm pretty sure that bug 2 is also not a CKEditor 5 bug, but still to be investigated and confirmed. Once confirmed, that will also need a non-the ckeditor5.module component issue.
  • But the editor fails to initialize because it throws a Javascript error. is the weakness we should fix on the CKEditor 5 side, because bug 1 has been around for years at this point and so we need to be able to handle broken configuration 😬 That's why keeping this issue around against the ckeditor5.module component makes sense. Still, it's important that bug/issue 1 gets created first, because then the hardening in the CKEditor 5 module's JS will be able to point to that issue to justify hardening against an edge case that should not exist.

hooroomoo opened merge request !2161

hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
Status: Needs work » Active
hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
Related issues: +#3277049: "Embed media" filter allows invalid config of default view mode not being part of view modes selectable

Created https://www.drupal.org/project/drupal/issues/3277049 for Bug #1

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#9: Thanks — now just needs one more follow-up for bug #2 👍

The test coverage looks great! Zero remarks 🤓

hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
Status: Active » Needs review
hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented

Created https://www.drupal.org/project/drupal/issues/3277244 for bug #2

hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
Issue summary: View changes
Related issues: +#3277244: media_embed filter: a default view mode that isn't allowed for a media type gets applied anyway

Updated issue summary with the two new media issues.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

Followups have been created, and crash is fixed.

  • catch committed 44f5e5c on 10.0.x 
    Issue #3276670 by hooroomoo, Wim Leers: Some configurations of allowed...

  • catch committed 16bdab1 on 9.4.x 
    Issue #3276670 by hooroomoo, Wim Leers: Some configurations of allowed...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x, cherry-picked to 9.4.x and 9.3.x, thanks!

  • catch committed 1ddf650 on 9.3.x 
    Issue #3276670 by hooroomoo, Wim Leers: Some configurations of allowed...

Status: Fixed » Closed (fixed)

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