Invalid markup generated, breaking a.media-colorbox link [#1477662]

See comments (up through #5 at least). This problem was due to a misconfiguration of the module -- using a file view mode on a file with a YouTube video (through media_youtube) that didn't use "YouTube Preview" or "YouTube Video" styles.

When using a media-colorbox display with a file style on a file field (with a video selected through media_youtube), I'm seeing that a.media-colorbox is wrapped around a broken link to /media/18 (should be /file/18 but I'm guessing that has nothing to do with this module).

The browser won't accept an <a> within an <a>, so what happens is the browser re-interprets the markup to pull the inner <a> outside of the outer one, thus completely breaking the a.media-colorbox link.

Here's the markup being generated:

<div class="field field-name-field-video field-type-file field-label-hidden">
  <a href="/media_colorbox/18/default/und" title="Video" class="media-colorbox " style="" rel="gallery_all" data-mediaColorboxInitialWidth="590" data-mediaColorboxInitialHeight="369">
    <span id="styles-0-0" class="styles file-styles sidebar">
      <a href="/media/18">
        <img typeof="foaf:Image" src="http://zoo.dev/sites/default/files/styles/sidebar/public/media-youtube/d9NF2edxy-M.jpg" />
      </a>
    </span>
  </a>
</div>

The browser (Chrome 17 and Firefox 10 tested) interprets this as

<div class="field field-name-field-video field-type-file field-label-hidden">
  <a href="/media_colorbox/18/default/und" title="Video" class="media-colorbox " style="" rel="gallery_all" data-mediaColorboxInitialWidth="590" data-mediaColorboxInitialHeight="369">
    <span id="styles-0-0" class="styles file-styles sidebar">
    </span>
  </a>
  <a href="/media/18">
    <img typeof="foaf:Image" src="http://zoo.dev/sites/default/files/styles/sidebar/public/media-youtube/d9NF2edxy-M.jpg" />
  </a>
</div>

Versions

Drupal 7.12
colorbox 1.2
file_entity 2.0-unstable3
media_youtube 1.0-beta2
media 2.0-unstable3 with this patch:
; http://drupal.org/node/1278180#comment-5085370
projects[media][patch][] = "http://drupal.org/files/media-embed-library-resize.patch"

I'll give a try on media and media_colorbox HEAD and see if that changes anything...

Comment	File	Size	Author
#4	media_colorbox-video-field-display-settings_1477662.jpg	31.98 KB	adamdicarlo

Comments

Comment #1

adamdicarlo commented 11 March 2012 at 18:37

Version:

7.x-1.0-rc1

» 7.x-1.x-dev

Same result pulling from git 7.x-1.x branch on media_colorbox (and pulling latest also from media, file_entity, and media_youtube).

To clarify, due to the browser interpreting the markup a bit differently, the GOOD link, /media_colorbox/18/default/und, is not clickable when hovering the image; the bad link is.

A nasty work-around for now is

/*
 * Media Colorbox file display doesn't render properly -- the thumbnail isn't
 * inside of the a.media-colorbox tag, it's in another (broken-link) a tag,
 * so we'll make the a.media-colorbox cover up the bad link.
 */
.field-type-file a.media-colorbox {
  position: absolute;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
}

Comment #2

kmadel commented 12 March 2012 at 22:12

This is the markup that I get when embedding a Media YouTube video in a file field:

<div class="field field-name-field-youtube-video field-type-file field-label-above"><div class="field-label">YouTube Video: </div><div class="field-items"><div class="field-item even"><a href="/media_colorbox/44/media_original/und" title="YouTube Video" class="media-colorbox init-media-colorbox-processed cboxElement" style="" rel="gallery-55" data-mediacolorboxinitialwidth="650" data-mediacolorboxinitialheight="400"><img typeof="foaf:Image" src="http://d7dev.local:8888/sites/default/files/styles/square_thumbnail/public/media-youtube/gxTuEC8RMzE.jpg" width="180" height="180" alt=""></a></div></div></div>

Using:
file_entity 2.0-unstable3
media_youtube 1.0-beta2
media 2.0-unstable3

Also, on the Manage display admin page for the Video file type, in this case, the Preview view mode (admin/config/media/file-types/manage/video/file-display/media_preview), make sure that you don't have 'YouTube Video' checked as an 'Enabled display', rather only 'YouTube Preview Image' and then that is the View mode that you will want to use as the Media Colorbox 'File view mode'.

Comment #3

kmadel commented 12 March 2012 at 22:18

Status:

Active

» Closed (works as designed)

Actually, I see now that I was able to reproduce by selecting 'Generic file' as the 'Enabled display'. Try going with 'YouTube Preview Image'. The 'Generic file' is a bit wonky and actually creates an icon that will open the colorbox, but the second anchor tag is to the file url and will not open a colorbox.

So, this is more an issue with the 'Generic file' formatter and how it is handled by the 'File entity' module, rather than Media colorbox.

Comment #4

adamdicarlo commented 12 March 2012 at 22:24

Status:

Closed (works as designed)

» Active

Status	File	Size
new	media_colorbox-video-field-display-settings_1477662.jpg	31.98 KB

I didn't even think -- media colorbox works just fine for videos on my Teaser node view mode using Rendered file / Colorbox.

The one that doesn't work is on a different node view mode for the same content type -- the reason I have two is because I need to have two different preview image sizes for the video, both of them opening the video itself in a colorbox.

Here's a screenshot of the configuration for the other node view mode. I've tried creating a file style and using that instead but I get the same problem.

I hope this allows you to reproduce the issue! I still *think* this is a bug, though it is something of an edge case.

Comment #5

adamdicarlo commented 12 March 2012 at 22:29

Status:

Active

» Closed (works as designed)

Ah ha -- I wasn't using "Generic file" I was using "Sidebar", a file view mode that was set with file style: sidebar as the only enabled display. I changed that to YouTube Preview and that solves it.

Thank you!

So not a bug exactly but I do wonder if there's a way to detect/prevent misusing the module this way.

Comment #6

kmadel commented 13 March 2012 at 12:18

Yes, I suppose you are right. The only thing is, is that there are a number of different file formatters out there that can be used with Media/File entity based entities, and for that matter there will be a lot of custom local file formatters. Obviously it won't be easy to test against them all. Perhaps as certain formatter recipes are discovered to not work well, they can be pointed out in the README file. Thanks.

Comment #7

BarisW commented 20 July 2012 at 09:56

I'm having the same issue. I try to get the label of the file to link to the colorbox, instead of the thumbnail. Generic file indeed doesn't work, but I don't see another formatter that only shows the label. Any thoughts?

Comment #8

BarisW commented 20 July 2012 at 09:56

Priority:	Critical	» Major
Status:	Closed (works as designed)	» Active

Re-opening the issue.

Comment #9

BarisW commented 20 July 2012 at 10:02

If it helps, this is the markup generated:

<a href="/media_colorbox/132/media_large/nl" title="" class="media-colorbox " style="" rel="gallery-132-" data-mediaColorboxInitialWidth="900" data-mediaColorboxInitialHeight="740"><a href="http://youtube.com/watch?v=u4zai_OO_o0" type="video/youtube; length=0">ZomerExpo 2012</a></a>

Comment #10

BarisW commented 20 July 2012 at 10:08

I managed to fix this by copying the theme_media_colorbox() function to my theme and by placing a strip_tags around the text.

OLD:

<?php
  $output = theme('link', array(
    'text' =>drupal_render($variables['item']),
    'path' => $variables['path'],
    'options' => array(
      'html' => TRUE,
      'attributes' => array(
        'title' => $caption,
        'class' => 'media-colorbox ' . $variables['item_class'],
        'style' => $variables['item_style'],
        'rel' => $gallery_id,
        'data-mediaColorboxInitialWidth' => $initialWidth,
        'data-mediaColorboxInitialHeight' => $initialHeight,
      ),
    ),
  ));
?>

NEW:

<?php
  $output = theme('link', array(
    'text' => strip_tags(drupal_render($variables['item'])),
    'path' => $variables['path'],
    'options' => array(
      'html' => TRUE,
      'attributes' => array(
        'title' => $caption,
        'class' => 'media-colorbox ' . $variables['item_class'],
        'style' => $variables['item_style'],
        'rel' => $gallery_id,
        'data-mediaColorboxInitialWidth' => $initialWidth,
        'data-mediaColorboxInitialHeight' => $initialHeight,
      ),
    ),
  ));
?>

However, it would be good if the module checks for links and strips these. Only text and images should be possible.

Comment #11

kmadel commented 18 August 2012 at 22:09

Status:

Active

» Closed (fixed)

Fixed on latest dev release. However, it will only strip anchor tags as there will be many other types of tags that will be desired by most to keep intact.