The changes introduced in https://www.drupal.org/node/2194821 completely change and break the rendering of the Media filter. The full template file is always rendered, even if there are no additional fields. Before, only the image tag itself was rendered, now it's surrounded by all the div's and other stuff from the file_entity.tpl.php file. This means, that not only unnecessary markup is rendered by default, it also means, that now block elements are added into the markup, so it could break the layout of existing content dramatically.

Here's a link to a workaround (a hook that you can implement in a custom module): https://www.drupal.org/node/2194821#comment-9515943

However, this problem must be solved within Media WYSIWYG itself.

Note: https://www.drupal.org/node/2401811 is also related to this problem, as contextual links were also rendered due to that change. I want to separate the two issues because there's no general solution by now for this issue and I've already posted a patch for the other problem, which is imho a very huge one! So hopefully at least the quick fix for contextual links exclusion gets committed quickly.

Comments

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

You could create a view mode for embedded media and override the template for it removing any parts of it that aren't necessary for your use case. Templates are a very flexible solution that give you full control over the markup. Generally it is better for html output to be controlled by the theme layer than by custom module code.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

That's true, BUT nevertheless this change is absolutely not backward compatible, so it breaks old behaviour. Plus, there were (and still are) some efforts within the media_wysiwyg code to only render the media tag itself, if there are not additional fields present => the intended behaviour of the module is therefore broken by itself.

Additionally, one must always try to find good and sensible defaults. I don't think, that it's a good idea to let media files rendered with lots of divs, links, headers, etc by default, when 99% of all users just want to insert an image. Integrating CKE and Media is challenging enough at the moment, so we should not build up additional usability barriers. These are exactly the things that let people move on to other CMS like Wordpress, etc.

Plus: consider customers, that want to insert images in a variety of different sizes, styles, optionally with or without Colorbox, etc. I do have to deal with some sites like this. Image again, you are upgrading the site, then you would have to add/edit templates for every single view mode. And probably you wouldn't recognize the new kind of output at first glance. Because when you click through the site, all the page content will be loaded from the filter cache, you'd have to save a node with media in its content first. We are using Media in every single project. That means every updating the existing ones to an upcoming version would be very tedious. And if you oversee a template, I can hear the customers crying already...

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman commented

Without having used the D8 version, I believe this implementation causes it to be more inline with the concept of embeddable entities. If I recall correctly, that's the general idea of Media in D8. So, while it threw a wrench in some of our gears it might be a benefit in the long run.

I personally liked that it emebdded as if part of field_attach_view() versus the entire entity. But now, I believe, it gives WYSIWYG display mode more purpose. I've got a hack in place to revert the changes brought up here - https://gist.github.com/mglaman/f9460f9c1bd6bf230dd5. However I think we should move towards a process that makes this more streamlined in D7.

I'm trying to be optimistic that work on in D8 will be easier to bring into D7 because the concepts are the same. Please, anyone on D8 side correct me if I'm wrong on embedded entities part..

justindodge’s picture

justindodge CreditAttribution: justindodge commented

Personally I support the rendering of the full entity and I think the change was generally a good one in terms of forward thinking, consistency, etc.

There is one significant problem I am having in terms of backwards compatibility though - users are now unable to embed an image via WYSIWYG and create a link to it (using the WYSIWYG editor itself - i.e. CKEditor) because of the block level elements which are now wrapping the image.

I'd like to find a way forward that accommodates this use case since I do think it will be a very common one that people will have issues with:

Linked image use case
1. User inserts image into WYSIWYG
2. User double-clicks image and adds a link URL using CKEditor (or other editor's) interface
3. Fail. Divs surround the image in the rendered output which also contain hidden nested links - image can't be linked to.
4. Backwards compatibility fail: Images that were linked previously no longer work. We have many sites with many of these.

I don't think users should have to implement a custom module hook in order to achieve this simple feature which the UI implies should be an available option.

Using file_view() in the rendering of the entity makes a lot of sense to me, it's hard to imagine backtracking - most other hassles with markup changing can be handled with CSS or whatever (I think) - but I'm having trouble thinking of a solution for this one.

Thoughts?

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

Hmmm my idea is not the best/most beautiful one, but would be a compromise: in 7.x-2.x make a config option whether to have full entity rendering or existing behaviour. in all future versions (D8 or 3.x) only use the new approach.

+ backwards compatible
+ D8 similarity
- higher maintenance costs / complexity in this part of the module

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman commented

I had considered the option suggested in #5. "How do you wish to render Media WYSIWYG? Entity or Field Attach View"

Since 2.x is technically still in beta, I think it's a valid change. I also think it's one we should adapt. The option to change rendering makes it more complicated... and I'm afraid it'll become an artifact just looming in code. It's a change that jostled some, but I don't know if that is enough to add such an arbitrary feature, in my opinion.

Plus, if we provide this feature we're maintaining two methods instead of solidifying a single method.

FWIW - I haven't experienced issues in WYSIWYG edit, just front end rendering. Once complication was using Bootstrap based themes and the .media class. Had odd padding and overflow issues. But that's theme related.

justindodge’s picture

justindodge CreditAttribution: justindodge commented

@agoradesign - I think this could be a useful option to have, but when it comes to customizing the rendering of the entity on a whole I feel like custom hooks, templates, etc do seem more appropriate.

When it comes to correcting my "link to images" use case, I'm starting to brainstorm towards a javascript solution - a smart little JS script could look for media block elements that are wrapped in links (in other words, find the <a href="test.com"><div class="media media-element-container media-full"><div id="file-11195" class="file file-image file-image-jpeg contextual-links-region">... and intelligently wrap the <img>'s within that div with the link that the user is trying to apply with WYSIWYG.

This is a definitely a little bit hacky, but may solve the problem for most people. I can't really think of much else - the next line of solutions is maybe to get some communication going between the WYSIWYG editor itself and the media token generated - which sounds a little daunting.

I'm going to give my proposed workaround a shot so I can at least correct my own problems caused by these markup changes.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

It's true, that media 2.x officially isn't stable. but it is existing already for a while. the module just has a quite conservative release policy, but I bet there are lots of live sites put there using it.

Maybe a switch would still be appropriate. but at least thus must be marked as bold and big as possible in the release notes and we should find some default setting for rendering without surrounding elements, eg. a default template for wysiwyg view mode that simply prints $content.

Regarding the link problem: the empty template would solve this also, but a general solution would be nice. unfortunately I also don't have a clean and good idea. maybe it's possible to attach the href to the media pseudo element as data attribute in the editor and inject it later into the field rendering in php

bneil’s picture

bneil CreditAttribution: bneil commented

It would be great if we could get maintainer feedback on this issue. For us, I consider this a regression, as the markup results in a major changes to how embedding documents work from being inline to block level now due to the container.

izmeez’s picture

izmeez CreditAttribution: izmeez commented

I keep finding myself referred back here from other issues such as #2401811-30: With Media WYSIWYG enabled - "Contextual links" are shown for anonymous users.

Is the solution in #3 just reverting the change or is it helping move forward to embrace the change?

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

The proposed solution in #3 is currently just a workaround, but it may build the base for a permanent solution. Imho we have two options:

1. Embrace the change like discussed above, but provide a backwards-compatible solution. This could be done e.g. with a self-implementation of this hook by Media WYSIWYG.
2. Revert the breaking changes

chiebert’s picture

chiebert CreditAttribution: chiebert commented
Related issues: +#2194821: Embedded media objects should honor display suite settings

Chiming in here, as I'm trying to understand all four of the related/intertwined issues:

@mglaman, for me, your workaround in #3 only strips out the container <div>, leaving the individual fields separate from the rendered file itself. So, it only 'reverts' the changes from the commit at #2194821-8: Embedded media objects should honor display suite settings for a limited set of circumstances.

As for the ongoing discussion of whether to embrace the change, or revert: +1 for embracing the change. It makes logical sense: if you want to embed an entity - and you've been given a tool to select a view mode for it (via the included Media WYSIWYG View Mode module) - then you should be able to expect the entity to be embedded as per how you've configured the view mode (whether you're using Display Suite or a custom view mode and the standard D7 field UI. While I haven't tested this yet, if you don't want the default markup for your entity, you have tools like Fences and Semantic [Insert Module Name Here], right?

But the pressure is on for me to find a solution now - I'd prefer to be moving forward, rather than build out a feature, only to have to jump through hoops later because of more conflicting patches. I'll see if I can think of options that address the backwards-compatibility issue over the course of the day.

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented
Status: Active » Closed (outdated)

Closing this issue as outdated. However, if you think this issue is still important, please let us know and we will gladly re-open it for review.
sincerely,
- the Drupal Media Team