Currently, users can choose from multiple view modes when embedding media in CKEditor; however, this list when embedding is not respecting if a view mode is actually enabled for the media type in question.

For example: I have an Image media type and a Video media type, both have "Embedded" displays but only Image has a "Thumbnail" Display. Both displays are allowed in my CKEditor profile. When adding a video, it allows me to select "Thumbnail" which is not enabled, causing the Video to fallback to the "Default" display.

This is not intuitive and confusing for users.

Proposed Solution

The view mode selection when embedding media should filter its available options according to the view modes that are enabled for the specific media type being embedded.

combined patch (fix with tests)

Remaining tasks

Commit the patch! It's got everything you want.

API changes

None.

UI changes

An existing select list's options will become context-sensitive.

Data model changes

Nada.

Release notes snippet

Previously, when embedding media in a WYSIWYG editor, any view mode that were configured for the filter format could be used to display the embedded media. This could cause the embedded media to render in unexpected ways if using a view mode which was not properly configured for that media type. Now, only the view modes that are configured for the specific media item being embedded will be presented to the user, which means the embedded media will be displayed in a more predictable way.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

b_sharpe created an issue. See original summary.

b_sharpe’s picture

Status: Active » Needs review
FileSize
1020 bytes

Patch changes to restricting to current bundle's available view modes.

phenaproxima’s picture

Category: Bug report » Feature request
Issue tags: +Usability

I'm not sure this is a bug, exactly, because it works as designed currently. Recategorizing, for now, as a feature request.

phenaproxima’s picture

Issue tags: +Needs tests

Patch looks like a nice one-line fix (awesome!) but it will need tests to land in core.

marcoscano’s picture

Status: Needs review » Needs work

The last submitted patch, 2: 3097416-ckeditor-view-modes.patch, failed testing. View results

oknate’s picture

My concern is that Drupal allows you to use view modes for which the Entity View Display isn't enabled. It will use the default, and hooks will affect it, etc. So I think if you have a Thumbnail entity view display and you have a preprocess hook configuring it, rather than an entity view display, you'd want that option to appear even though you haven't configured an entity view display for it. Personally, on some projects, I will create an entity view display, such as "tab' and then a preprocess hook adds a class or something. I don't need to change anything from the default so I don't configure an entity view display. I wouldn't want the list limited to only ones that have an entity view display configured for that bundle.

I can see the benefit of this approach if you wanted view modes to show for certain bundles, but not others. That's why I originally wanted to do this on a per bundle basis in #3074608: Optionally allow choosing a view mode for embedded media in EditorMediaDialog. I think right now, the plan is "leave it to contrib". But determining which show by which have configured Entity View Display plugins is the wrong approach, IMHO, as it doesn't allow for the use of the default when selecting a view mode. They are two separate things in Drupal, although they are easily confused. You can use a view mode without a configured Entity View Display!

I think a better approach would be a contrib module that allows you to tailor the list based on bundle.

b_sharpe’s picture

FileSize
26.96 KB

@oknate I have to disagree, this is just plain old poor UX, for both developer and user.

Example:
Waiter: "We have two side options: Salad or Fries"
Customer: "I'll take the Fries"
Waiter: "Alright, here's your soup"

You are providing a user options they potentially cannot have and might be substituted without notice.

I couldn't find any documentation saying that you are supposed to use view modes in a disabled state, the fact that hooks run for them seems more like a way to ensure things don't blow up if someone does so, but even your example of adding a class would require contrib code, I can't imagine anything in core does this...

If this is "how it was designed", then I think there needs to be some documentation and messaging around it for sure, mainly for a developer as they need to account then for ANY view mode available to content as a user may potentially have that available to them which could be a lengthy list:

view modes

Nathaniel’s picture

I'm exploring how to do something similar with photo albums in the photos module. So far I have a custom field formatter for entity reference. I landed here trying to figure out how to add different display options for "Cover" (cover image that links to the full album view), "Colorbox" (opens a colorbox of the images), "Slideshow"... etc. Without having to create a separate type for each.

Chris Burge’s picture

Re #3074608: Optionally allow choosing a view mode for embedded media in EditorMediaDialog, see comments #12-17. I don't see this getting in. Per phenaproxima:

My preference would be to simply allow the selected view modes for all media types, and leave type-specific fine tuning to contrib.

I'm going to be posting a contrib module in the next day or so that addresses this issue.

phenaproxima’s picture

@Chris Burge, would you be willing to post a patch to the Media Library Extras module? That's a place where we are collecting "grab bag" enhancements like this one, and we'd be very happy to help land this feature in there.

Chris Burge’s picture

@phenaproxima - I'm open to contributing the code to a "grab bag" project; however, the code extends Media, not Media Library. Is there a project for Media extras?

phenaproxima’s picture

@phenaproxima - I'm open to contributing the code to a "grab bag" project; however, the code extends Media, not Media Library. Is there a project for Media extras?

The code already in Media Library Extras, which modifies CKEditor stuff, extends Media anyway :)

We chose the name "Media Library Extras" mostly because the media library is how you, as a user, interact with the media system -- we figure people don't generally care which subsystem it actually lives in. ;-) So, in the interest of simplicity, I would say that Media Library Extras is very much the correct place to put this feature.

Chris Burge’s picture

b_sharpe’s picture

Aside from additional functionality listed above, I do believe the original request still needs to be addressed.

Tests updated to test that only enabled view modes are rendered for the bundle.

The last submitted patch, 15: 3097416-ckeditor-view-modes-TEST-ONLY.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

Replying to #7:

I think that view modes and view displays, and the relationship between the two (including the whole fallback to the "default" view display), are a pair of really complex concepts that are difficult even for site builders to grasp. To authors/normal users, I would imagine they are impenetrable, and contribute to the understandable perception that Drupal is hard to use.

I think my preference is to do what will make the most sense for users, and that is to give them the options that they actually have, without exposing the fallback mechanism in any way. So:

  • In core, the filter should be unchanged -- it lets you choose which view modes to expose to users, regardless of which media types actually have displays for them. Tweaking the configuration UI feels more like a contrib thing, and @Chris Burge has made great strides towards implementing it in Media Library Extras.
  • But, when presenting the choice of view mode to the user, filter the allowed view modes by which ones have view displays for the type of media being embedded. In other words, give the user meaningful options.

So basically, I agree with @bsharpe. The current situation is fine for site builders/configurator experience, but poor for author experience. If you select "Thumbnail" when embedding a video, you expect it to look like a thumbnail, not to do a silent and utterly baffling fallback to something that does not look like a thumbnail. As a user, I shouldn't be given options that will not produce the results I'm looking for.

webchick’s picture

The last paragraph of #17 makes sense to me, as does the problem described in the issue summary (and #8 almost cost me a laptop :D). I guess I would be helped by some before/after (or current/proposed) screenshots to be sure, but generally +1 to the approach.

phenaproxima’s picture

For the record, I understand @oknate's reasoning in #7. For small, preprocess-level modifications, it is certainly overkill to have an entire view display that, essentially, just mirrors the default one for a particular media type. That's not very convenient.

However, the reason I came down on @bsharpe's side with this one is because I think, for the Media Library module especially, usability needs to trump developer convenience. (That sounds a lot meaner in writing than it is meant to.) The media library was designed, first and foremost, to be usable for authors. It was not meant to be super extensible and flexible -- site builder/developer needs were deliberately put in the backseat, since that makes it easier to deliver a consistent and sense-making (dare I say delightful?) experience to users. I agree with that design decision; media in Drupal should be easy to use above all.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
+++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -1181,13 +1206,14 @@ public function testViewMode() {
     // Test MediaEmbed's allowed_view_modes option setting enables a view mode
-    // selection field.
+    // selection field if enabled on the bundle.

This change doesn't really add anything, AFAICT; can we remove it? Otherwise, I think this is a great, straightforward patch with fine test coverage, so I'm removing the "needs tests" tag for now.

However, there is a small wrinkle to consider -- namely, this will change the behavior of existing sites. The options available to authors will suddenly change. What should we do about that? A few possible options, in order of my personal preference:

  • Do nothing; create a release note and only commit this to 8.9.x and 9.0.x, then let site builders deal with the implications on their own. I like this approach because it creates the smallest possible "mess" and will not put sites in the position of having to undo anything we did.
  • Make this globally configurable (i.e., a setting of the Media module itself, not the filter), with an update path that sets it to preserve the existing behavior. It would be obscure and hard to describe, sure...but since showing all view modes to users (regardless of which ones have view displays associated with them) is probably an edge case, that's not necessarily the worst outcome in the world.
  • Add an update path that creates all the entity view displays that are needed in order to preserve the existing behavior. (This would be complex and increase the size of the patch by an order of magnitude.)

Other ideas?

b_sharpe’s picture

OK:

  1. Changed to 8.9.x
  2. Remove erroneous comment and extra line ending (interdiff provided)
  3. Comment below on upgrade path

I think the "Do nothing" approach makes the most sense. It is likely pretty rare that someone will be using a view mode in it's disabled state, and in that case there will be no immediate impact on a site, and to the author just that the selection will be missing, in which the fix is to just enable that view mode for the media type in which they were using. Since a new view mode is based off of "Default" anyways, this will be a pretty simple and painless "fix".

phenaproxima’s picture

Title: CKEditor Media: Only allow enabled view modes » When embedding media, don't let authors choose view modes that are not enabled for that media type

Re-titling for clarity.

seanB’s picture

+++ b/core/modules/media/src/Form/EditorMediaDialog.php
@@ -172,7 +172,7 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
+    $view_mode_options = array_intersect_key($this->entityDisplayRepository->getViewModeOptionsByBundle('media', $media->bundle()), $media_embed_filter->settings['allowed_view_modes']);

Sitebuilders are able to limit the list of view modes. This could technically mean the list of displays a user can select will be empty. Not sure if it would be clear to users why they sometimes can choose a display, and other times they can not.

In general, I agree we should be careful showing content authors options that would produce unexpected results. I also see lots of cases where view modes only make sense for some media types, but do not for others (for example, links are common for documents, and less for remote video's).

If we add a description or something for the Displays field to make it clear the available displays are specific for the chosen media item that would already help. We might also need a text for when there is only 1 option available, or none.

Regarding the update path, since this only affects users creating new content, I think a release note should be sufficient. Site builder can easily fix it by enabling displays and more complex needs can be fixed by a form alter if I'm not mistaken.

webchick’s picture

Ok, since my comment in #18 was a bit wishy-washy, here's a more firm +1 on the proposal, which as I understand it is:

1) Leave the embed configuration form on the backend simple, and expose all possible view modes that could be displayed in the library in a simple checkbox list.

2) Filter the selection at "embed time" to only those view modes that make sense for a particular media type. (i.e. select box shows both "default" and "thumbnail" for images, but no selection is visible on remote video, because it would fall back to default, so "thumbnail" is irrelevant and don't bother the user with a single choice).

I don't personally feel we need text to explain what is happening here; it's a common UX pattern to limit exposure to irrelevant settings, and "image" and "video" are different enough concepts to trigger the notion that they might expose different options. If you have some field data that contradicts this feeling though, let me know.

The patch seems to have a bug though, because in @phenaproxima's demo, it was hiding the selector for both image and remote video, which it should not.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks, @webchick! Nice to know that this is a good idea which will help clarify the interface.

Indeed, I think we need more test coverage here, because after applying the patch, I did not see the view mode selector exposed when editing an embedded image, even though two view modes should have been available for it.

phenaproxima’s picture

Nope, never mind -- the bug was PEBKAC on my part. The problem was that I had enabled "Full content" and "Thumbnail" view modes when configuring the embed filter -- but had only set up custom display settings for the "Thumbnail" view mode. When I corrected this and manually tested, it worked as intended.

So: in #24, @webchick addressed @seanB's points from #23. I manually demoed this to @webchick, so it's passed WebchickTestCase. I've reviewed the code, and there is test coverage. We have discussed, and decided, what to do about an update path (nothing).

All that remains, I think, is to update and flesh out the issue summary (including a release note). Then I think this is RTBC.

seanB’s picture

Sitebuilders are able to limit the list of view modes. This could technically mean the list of displays a user can select will be empty.

I think this could still be a thing when we only allow a "thumbnail" view mode via the backend config for ckeditor, but do not configure a thumbnail view mode for a media type. If the list of options is empty, we should probably hide the select or something and let it fall back to default. It would be good to have a test for this as well.

b_sharpe’s picture

@seanB, What you are describing is how it already works.

If only one mode available = No Select List.
If no modes available = No Select + use default view mode

b_sharpe’s picture

Status: Needs work » Needs review
bkosborne’s picture

Just want to chime in to say +1 to this. This would be incredibly useful in situations where an "Image Gallery" media bundle has 3 different entity view displays defined that don't apply at all to "Image" media bundle, but right now they're all selectable for both.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

phenaproxima’s picture

Priority: Normal » Major
Issue tags: +Media Initiative, +Triaged Media Initiative issue, +ckeditor

Escalating priority, since this adds sad glaze of confusion to the authoring experience of embedding media.

joseph.olstad’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Patch also applies cleanly also to 9.2.x, 9.1.x, 9.0.x

Has tests. I will trigger test for those branches

We very much need this usability improvement.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

I think we still need the issue summary to be updated, and a release note, as per #26. Then I agree, we're good to go!

MrPaulDriver’s picture

This is a most welcome improvement and I find it works very well when tested against 9.1

It would be good to see this RTBC and committed.

jds1’s picture

Status: Needs review » Reviewed & tested by the community

Works great for me on 8.9.16!

phenaproxima’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updating the issue summary and adding a release note snippet.

phenaproxima’s picture

Issue summary: View changes
alexpott’s picture

Version: 8.9.x-dev » 9.2.x-dev
Category: Feature request » Bug report
Status: Reviewed & tested by the community » Patch (to be ported)

This feels very much like a UX bug fix - re categorizing.

Committed acbf515 and pushed to 9.3.x. Thanks!

Will cherry-pick to 9.2.x once I confirm it is not frozen for a release. 8.9.x and 9.1.x only receive security releases now that 9.2.x-rc is out.

  • alexpott committed acbf515 on 9.3.x
    Issue #3097416 by b_sharpe, phenaproxima, Chris Burge, seanB, webchick,...

  • alexpott committed cf7be70 on 9.2.x
    Issue #3097416 by b_sharpe, phenaproxima, Chris Burge, seanB, webchick,...
alexpott’s picture

Status: Patch (to be ported) » Fixed

Backported to 9.2.x so this will be in the next bug fix release.

Status: Fixed » Closed (fixed)

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

phernand42’s picture

Patch re-roll for the latest core update (8.9).

mstrelan’s picture

This change caught me out and resulted in a regression. See #3308069: Provide per-bundle configuration for embeddable view modes for details and proposed follow up.