Closed (duplicate)
Project:
Media: YouTube
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
31 Mar 2011 at 20:50 UTC
Updated:
24 Jul 2012 at 17:30 UTC
Jump to comment: Most recent file
Comments
Comment #1
effulgentsia commentedHow can I test this patch? Under what circumstance does the current code result in a size mismatch?
Comment #2
james.elliott commentedYou 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.
Comment #3
effulgentsia commentedI'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.
Comment #4
effulgentsia commentedJust a reroll of original patch for git. Still need work as per #3.
Comment #5
james.elliott commentedNew patch based on #1278180: Add a library to handle fluid sizes for embedded media. being added to the core media module.
Comment #6
dddave commentedThe patch in #5 works brilliantly. Thanks!
Comment #7
Johnny vd Laar commentedThis 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.
Comment #8
RobW commentedIs 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.
Comment #9
VanD commentedApplying patch from #5 to 7.x-1.x-dev fails for me with a fresh module install.
Comment #10
mrfelton commentedUpdated patch for latest dev, although it doesn't seem to work for me. I have the media patch applied too.
Comment #11
RobW commentedBased 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.