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.
Comment | File | Size | Author |
---|---|---|---|
#44 | 3097416-ckeditor-view-modes-8.9-combined.patch | 3.64 KB | phernand42 |
#21 | interdiff.txt | 723 bytes | b_sharpe |
#21 | 3097416-ckeditor-view-modes-8.9-combined.patch | 3.63 KB | b_sharpe |
#15 | 3097416-ckeditor-view-modes-combined.patch | 4.09 KB | b_sharpe |
#15 | 3097416-ckeditor-view-modes-TEST-ONLY.patch | 3.09 KB | b_sharpe |
Comments
Comment #2
b_sharpe CreditAttribution: b_sharpe at ImageX commentedPatch changes to restricting to current bundle's available view modes.
Comment #3
phenaproximaI'm not sure this is a bug, exactly, because it works as designed currently. Recategorizing, for now, as a feature request.
Comment #4
phenaproximaPatch looks like a nice one-line fix (awesome!) but it will need tests to land in core.
Comment #5
marcoscanoThere is some background with more context about why we arrived to the current status in #3074608: Optionally allow choosing a view mode for embedded media in EditorMediaDialog
Comment #7
oknateMy 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.
Comment #8
b_sharpe CreditAttribution: b_sharpe at ImageX commented@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:
Comment #9
Nathaniel CreditAttribution: Nathaniel commentedI'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.
Comment #10
Chris Burge CreditAttribution: Chris Burge at University of Nebraska commentedRe #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:
I'm going to be posting a contrib module in the next day or so that addresses this issue.
Comment #11
phenaproxima@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.
Comment #12
Chris Burge CreditAttribution: Chris Burge at University of Nebraska commented@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?
Comment #13
phenaproximaThe 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.
Comment #14
Chris Burge CreditAttribution: Chris Burge at University of Nebraska commentedPatch posted: #3108619: Allow View Mode Options to be Restricted on a Per-bundle Basis for Media Embed
Comment #15
b_sharpe CreditAttribution: b_sharpe at ImageX commentedAside 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.
Comment #17
phenaproximaReplying 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:
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.
Comment #18
webchickThe 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.
Comment #19
phenaproximaFor 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.
Comment #20
phenaproximaThis 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:
Other ideas?
Comment #21
b_sharpe CreditAttribution: b_sharpe at ImageX commentedOK:
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".
Comment #22
phenaproximaRe-titling for clarity.
Comment #23
seanBSitebuilders 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.
Comment #24
webchickOk, 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.
Comment #25
phenaproximaThanks, @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.
Comment #26
phenaproximaNope, 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.
Comment #27
seanBI 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.
Comment #28
b_sharpe CreditAttribution: b_sharpe at ImageX commented@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
Comment #29
b_sharpe CreditAttribution: b_sharpe at ImageX commentedComment #30
bkosborneJust 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.
Comment #32
phenaproximaEscalating priority, since this adds sad glaze of confusion to the authoring experience of embedding media.
Comment #33
joseph.olstadPatch 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.
Comment #34
phenaproximaI 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!
Comment #35
MrPaulDriver CreditAttribution: MrPaulDriver commentedThis 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.
Comment #36
jds1Works great for me on 8.9.16!
Comment #37
phenaproximaUpdating the issue summary and adding a release note snippet.
Comment #38
phenaproximaComment #39
alexpottThis 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.
Comment #42
alexpottBackported to 9.2.x so this will be in the next bug fix release.
Comment #44
phernand42 CreditAttribution: phernand42 commentedPatch re-roll for the latest core update (8.9).
Comment #45
mstrelan CreditAttribution: mstrelan at PreviousNext commentedThis 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.