YouTube videos previously would resize to fit the container that they are placed in. This broke when another wrapper media-youtube-outer-wrapper was added.

This patch updates the JS to handle this better so that videos will no longer overflow their containers.

Comments

effulgentsia’s picture

How can I test this patch? Under what circumstance does the current code result in a size mismatch?

james.elliott’s picture

You can test this by putting a youtube video in a narrow column. If you put a video in say, a sidebar, the video shouldn't overflow it's container. This is especially noticably if the container has overflow: hidden; set.

effulgentsia’s picture

Status: Needs review » Needs work
+  wrapper
+    .height(context.width() * embed.hw + 15)
...
   video
-    .height(context.width() * hw)
+    .height(context.width() * embed.hw + 15)

I'm not sure this addition of 15px (to accommodate the youtube control bar) is desirable. Even when adding a video into wysiwyg for an article node (where it's displayed in the main content region that's plenty wide enough), it results in the default settings of 480x360 being rendered as 480x375. We can check with Aaron, but I think the width and height (whether hard-coded or eventually made available to UI settings) are intended to represent the total size, including the control bar.

Beyond the above, we're also currently running into a conflict between the goals of this patch, and Aaron's commit in http://drupalcode.org/project/media_youtube.git/commitdiff/09979be?hp=5c.... Also, when using the Styles module, the parent of this outer wrapper (i.e., the "context" this patch tries to resize to) is a Styles module provided div, rather than the container we really want to resize to (e.g., something constrained to the block contents width). I think we need to straighten out our use-cases a little more to make sure we're approaching all of our various wrappers correctly.

Finally, unrelated to this patch, but something I noticed while trying to test this patch, is that by default the media filter is cacheable, and when there's a filter cache hit but a page cache miss, media_youtube.js isn't even added to the page. As a separate issue, we'll need to deal with that by moving those drupal_add_*() calls to somewhere else. I'm not sure where yet. I'll try to do some research to find out.

effulgentsia’s picture

Just a reroll of original patch for git. Still need work as per #3.

james.elliott’s picture

New patch based on #1278180: Add a library to handle fluid sizes for embedded media. being added to the core media module.

dddave’s picture

The patch in #5 works brilliantly. Thanks!

Johnny vd Laar’s picture

This problem:
Finally, unrelated to this patch, but something I noticed while trying to test this patch, is that by default the media filter is cacheable, and when there's a filter cache hit but a page cache miss, media_youtube.js isn't even added to the page. As a separate issue, we'll need to deal with that by moving those drupal_add_*() calls to somewhere else. I'm not sure where yet. I'll try to do some research to find out.

A work arround for this is:
/**
* Implements hook_filter_info_alter().
*/
function custommodule_filter_info_alter(&$info) {
$info['media_filter']['cache'] = FALSE;
}

And then resave the input filter such that it isn't cachable anymore. But it would be better if you wouldn't have to do this.

RobW’s picture

Is this code is even necessary? I think at the least it should be pulled out into its own module, maybe be used to refine Media: Responsive or Fitvids, or just be abandoned altogether in favor of those projects. There's discussion between RobFeature and I about some alternative/existing video resizing methods in #1535954: Responsive Embeds -- the information in these issues should probably be consolidated and one marked as a duplicate.

VanD’s picture

Applying patch from #5 to 7.x-1.x-dev fails for me with a fresh module install.

wolverine:/var/web/dev5/sites/default/modules/media_youtube # patch -p1 --dry-run < 0001-Issue-1112462-YouTube-videos-no-longer-resize-to-fit.patch
patching file css/media_youtube.css
patching file includes/themes/media-youtube-video.tpl.php
patching file includes/themes/media_youtube.theme.inc
Hunk #1 succeeded at 33 (offset -7 lines).
Hunk #2 FAILED at 55.
1 out of 2 hunks FAILED -- saving rejects to file includes/themes/media_youtube.theme.inc.rej
patching file js/media_youtube.js
wolverine:/var/web/dev5/sites/default/modules/media_youtube #
mrfelton’s picture

Status: Needs work » Needs review
StatusFileSize
new7.87 KB

Updated patch for latest dev, although it doesn't seem to work for me. I have the media patch applied too.

RobW’s picture

Status: Needs review » Closed (duplicate)

Based on the discussion in #1535954: Responsive Embeds, #1112462: YouTube videos no longer resize to fit their containers, http://groups.drupal.org/node/233238, and #1555276: Clean up theming function, Use recommended iframe player by default, without js, I think M:YT is abandoning serving its own responsive js in favor of an html object based approach. With the patch in the last issue, you can apply third party css and js solutions that work on all iframes in a site.

The iframe without js patch needs to be backported to 7.x-1.x, so I'm going to mark this as a duplicate to drive effort over there.