View mode classes accumulate when changing image view mode [#2422153]

TL;DR - when using Media WYSIWYG, if you change the View Mode on the image, it adds classes rather than replacing them which creates problems if there's a conflict in the rules, as is the case with some Panopoly CSS.

Something that I thought for a long time was a config problem on my end. I just tested it on a clean install of Panopoly 1.15 simplytest.me and originally posted in the Panopoly queue, but moved here since it seems like it's coming from here somewhere, but this may have to be moved again.

The Problem:

If you change view modes, you end up with conflicting classes on your images. Rather than replacing the old classes based on view mode and image style class, it retains the original ones and adds the new ones.

This is a particular problem if the image is first inserted using Quarter Size or Half Size because the Panopoly CSS gives them widths and max-widths. So if I start at Quarter Size and change to Thumbnail, I get a highly pixelated quarter-size image.

That first image style class will never change, so if you are using the Panopoly CSS straight up, it leads to problems.

Obviously it would be great to have this fixed upstream so it updates properly, but for now I'm just overriding the Panopoly CSS, which is a bit of a challenge in itself because those widths are defined with !important.

Steps to Recreate

Spin up a clean install of Panopoly
Edit a page and add an image. Select the Quarter Size view mode in the Display As select dropdown. Save. You'll now have the image class twice and the view mode class once (see code samples below)
Edit the page again, click on the image, click the image editing icon, change the view mode to Thumbnail and save. Note that image is pixelated and that classes are getting added, not replaced (again see code below)
Do it again and change it to Original
The image now has the original image class and the last one, as well as all three view mode classes (see code samples below)

I have also had situations where the image style doesn't change, which is how I first noticed this. If you have an image style that's small (say a thumbnail) and you change the view mode to something larger, I end up with the actual thumbnail image style and then CSS classes that display it larger (and obviously pixelated). This latter problem I have NOT seen on a clean install with panopoly, so it's probably something in my setup.

Code Samples

After the steps listed above, here is the image tag

<img class="panopoly-image-quarter media-element file-teaser file-preview file-default panopoly-image-original" src="http://s5bbe65dd8c0c740.s3.simplytest.me/sites/default/files/styles/panopoly_image_original/public/j0400264.jpg?itok=wZdFSpLg" alt="">

Note that all three view mode classes are there (file-preview, file-default, file-teaser) and the last image-style classe (panopoly-image-original) and the first one (panopoly-image-quarter).

Comment	File	Size	Author
#10	view_mode_classes-2422153-10.patch	660 bytes	brockfanning
#10	7.x-2.x: PHP 5.3 & MySQL 5.5, D7 53 pass
#9	view_mode_classes-2422153-9.patch	647 bytes	brockfanning
#9	7.x-2.x: PHP 5.3 & MySQL 5.5, D7 53 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

ergophobe CreditAttribution: ergophobe commented 7 February 2015 at 23:31

Title:	Image and View Mode classes doubled and accumulate on inserted inline image.	» Image and View Mode classes accumulate when inserting inline image.
Project:	Panopoly	» D7 Media
Version:	7.x-1.15	» 7.x-2.x-dev
Component:	WYSIWYG	» Media WYSIWYG

Comment #2

ergophobe CreditAttribution: ergophobe commented 7 February 2015 at 23:34

Issue summary:

View changes

Comment #3

ergophobe CreditAttribution: ergophobe commented 7 February 2015 at 23:34

Issue tags:

+panopoly

Comment #4

dsnopek

he/him

English

USA

CreditAttribution: dsnopek commented 10 February 2015 at 17:38

Title:

Image and View Mode classes accumulate when inserting inline image.

» View mode classes accumulate when changing image view mode

At least part of what you are describing comes from Panopoly: it's panopoly_images_preprocess_image_style() that's adding the image style class.

However, even if you comment out that bit of code, you'll still accumulate classes for the file entity view modes, for example, I got this in my testing: <img class="media-element file-teaser file-preview file-default" ...>

I'm not sure what can be done here generically in Media: it needs to preserve any classes added in the WYSIWYG using WYSIWYG tools. I haven't dug into the code yet, but don't know if it'd be possible to identify which ones came from Drupal originally and remove only those when switching view modes. :-/

But I do think we could for sure fix this in Panopoly! We know which classes should and shouldn't be there for a particular view mode and maybe we could filter them out in preprocess function somewhere?

So, if we can't find a generic solution in Media, I'd be OK with moving this back to Panopoly and hard-coding a solution that works for us...

Comment #5

ergophobe CreditAttribution: ergophobe commented 18 February 2015 at 05:10

The issue with the view modes is coming, I think, from media/modules/media_wysiwyg/js/media_wysiwyg.filter.js lines 264ff of 7.x-2.0-alpha4+25-dev

      if (info.view_mode) {
        classes.push('file-' + info.view_mode.replace(/_/g, '-'));
      }
      element.addClass(classes.join(' '));

This is adding classes, but not clearing the original class.

Ideally it would grab a list of view modes, remove any classes based on view modes, and then add in the one class for the current view mode.

Does anyone know how to grab a list of view modes in Javascript? I can do it in PHP, but have no clue how to do it in Javascript.

Comment #6

dsnopek

he/him

English

USA

CreditAttribution: dsnopek commented 18 February 2015 at 12:44

You'd probably have to grab the list in PHP, then pass it to Javascript via drupal_add_js('setting', array(...)). The Media WYSIWYG code is already passing a number of things from PHP to Javascript, so there's probably somewhere to just add the view modes, but I haven't actually dug into the code to find where that is.

Comment #7

chrisgross CreditAttribution: chrisgross commented 10 July 2015 at 20:13

From what I can tell, this problem appears to be caused by a media patch included with panopoly: See this issue. Without it, alpha4 does not have this problem.

Comment #8

rwohleb

he/him

English

CreditAttribution: rwohleb commented 12 February 2016 at 21:05

Patch #66 on #2126697: Wysiwyg -- Alt and Title fields require some special handling. appears to correct the duplicate view-mode class issue.

Comment #9

brockfanning CreditAttribution: brockfanning commented 27 April 2016 at 16:41

Status:

Active

» Needs review

File	Size
view_mode_classes-2422153-9.patch	647 bytes
7.x-2.x: PHP 5.3 & MySQL 5.5, D7 53 pass

I think that this fix, which is minor, belongs here its own patch. Here is the relevant part from the issue rwohleb found.

Comment #10

brockfanning CreditAttribution: brockfanning commented 4 July 2016 at 19:32

File	Size
view_mode_classes-2422153-10.patch	660 bytes
7.x-2.x: PHP 5.3 & MySQL 5.5, D7 53 pass

1 file was hidden/shown/deleted

File	Size
view_mode_classes-2422153-9.patch	647 bytes
7.x-2.x: PHP 5.3 & MySQL 5.5, D7 53 pass

Slight change to catch any cases of empty strings.

Comment #11

ultimike

English

Florida, USA

CreditAttribution: ultimike at DrupalEasy commented 6 July 2016 at 20:47

I've just tested the patch in comment 10 on a local environment of a "real" site and it appears to work.

-mike

Comment #12

ultimike

English

Florida, USA

CreditAttribution: ultimike at DrupalEasy commented 11 July 2016 at 18:39

I spoke too soon - it does not remove the "panopoly-" style classes...

-mike

Comment #13

ultimike

English

Florida, USA

CreditAttribution: ultimike at DrupalEasy commented 11 July 2016 at 19:42

I spent about an hour digging into this, and the patch in comment 10 works for clearing out classes added from View Modes, but it doesn't address the need to also clear out classes added from Image Styles.

As dsnopek pointed out in comment 4, panopoly_images_preprocess_image_style() adds a single class to each image based on the image style machine name. The Media module (specifically, the media_wysiwyg.filter.js file) doesn't know (or care) about these classes, and therefore leaves them alone. So, when a view mode is changed, the class originally added (based on the image style) isn't removed. I _think_ that this only happens when the image is first added, in my testing, I don't think classes based on image styles are added on any subsequent view mode changes (I'd love for someone to double-check me on this).

I'm surprised more folks haven't found this to be an issue. I was just able to reproduce this issue on a fresh install of Panopoly, so it appears to be a valid issue still.

I think solving this in Panopoly Images is a good idea, but I'm unsure as to which preprocess function we'd be able to leverage to fix this...

thanks,
-mike

Comment #14

brockfanning CreditAttribution: brockfanning commented 11 July 2016 at 20:00

@ultimike, this patch specifically looks for file-* classes, with the assumption that they will be the view mode classes. Maybe a better solution would to add an alter hook, like maybe "hook_media_wysiwyg_class_prefixes_alter", that allowed other modules to indicate additional prefixes to act on, such as panopoly-*.

Comment #15

dsnopek

he/him

English

USA

CreditAttribution: dsnopek commented 12 July 2016 at 12:15

@ultimike: Can you open a new Panopoly issue for this? Your problem seems to be on the Panopoly side of the fence, where this issue is attempting to address the Media part. That said, I thought we fixed having the extra image style classes in #2422163: Panopoly CSS causes issues with WYSIWYG image because of image style class names which was merged over a year ago. :-/ This added some code to panopoly_images_preprocess_image_style() which attempted to remove the excess classes, but apparently it's not working in your case...