A lot of the formatting and theming code changed in Media: Youtube with the patch in #1555276: Clean up theming function, Use recommended iframe player by default, without js. Although Media: Vimeo didn't have the same big problem with js embed replacement, this patch adds all the other improvements and code changes from the M:YT issue and some follow up issues.

  • Updates iframe query options, and adds them only if they differ from the Vimeo default.
  • Removes unused js and css.
  • Improves theming, removing wrappers and inline style attributes, making the output responsive ready (any css or fitvids-style js can be applied directly to the iframe).
  • Cleans up template variables, putting all formatter options into an $options array.
  • Cleaner js API handling -- only adds an id when the API is enabled.
  • Adds https:// and relative protocol options (see discussion of pros and cons of each in #1368818: HTTPS (SSL) support for YouTube player.).
  • Adds validation for width, height, and color formatter settings (like in this M:YT follow up issue: #1547420: Width and height settings aren't validated against XSS).
  • Brings the formatter settings, theming, and codebase into alignment with M:YT. So we get a unified ux across the projects, better front end development experience (standardized html objects FTW), and easier maintenance.

This patch fixes / addresses the following Media: Vimeo issues:

Theming:
#1698300: Remove width/height from outer-wrapper div
#1636032: Proportional sizes for embedded media
#1051238: Dynamic Height (well, if someone has the same problem on 7.x)

js API:
#1674158: Froogaloop api requires iframe ID to match player_id parameter

https:
#1366706: HTTPS support for secure Vimeo videos

Also addresses this issue, where the Fitvids module is adding !important css declarations to undo M:V inline css. With this patch, no !important css declarations are needed.
#1375102: Videos extend abnormally / padding-top issue, specifically http://drupal.org/node/1375102#comment-6339906 and the comments after.

CommentFileSizeAuthor
#14 media-vimeo-code-alignment-1715438-14.patch26.37 KBRobW
#12 media-vimeo-code-alignment-1715438-12.patch26.41 KBRobW
#3 media-vimeo-code-alignment-1715438-3.patch26.39 KBRobW
#1 vimeo-formatters.png12.92 KBRobW
media-vimeo-code-alignment-0.patch26.34 KBRobW

Comments

RobW’s picture

RobW commented
Issue summary: View changes

Updated list o wins

RobW’s picture

RobW commented
StatusFileSize
new vimeo-formatters.png12.92 KB

Screenshot of the new formatter settings.

formatters

RobW’s picture

RobW commented

More info on the https:// choice:

In M:YT, we decided to use https for the iframe by default. But, Vimeo's FAQ states that mismatched iframe and containing page protocols will break the embed. I did some quick testing of an https video on http pages and everything worked fine. If more people test and confirm that it works (or if Vimeo changes their FAQ) we can make https the default and remove the formatter option, but until then I think we should let developers / site owners make the decision themselves.

RobW’s picture

RobW commented
StatusFileSize
new media-vimeo-code-alignment-1715438-3.patch26.39 KB

Ah, my mistake, it's not caching issues but embedding outside of http context that causes problems with relative urls. Patch updated with correct description.

dddave’s picture

dddave commented

As far as I can check this patch helps a lot. Especially using fitvids.js is now as easy as with media_youtube. Thanks!

I did have some troubles applying this patch though. The parts concerning the removal of flash_detect_min.js and media_vimeo.js wouldn't fly (around lines 483 and 495) for me. Might be my problem using cygwin correctly or might be a problem with the patch.

dddave’s picture

dddave commented
Priority: Normal » Major

Let's reflect the importance of this changes...

RobW’s picture

RobW commented

Dddave, thanks for the test. Patch is applying fine for me -- can you post the error that you're getting?

dddave’s picture

dddave commented

Just tested again and now had no problems. So it was in fact my incompetence.

Just as a heads-up: fitvids.js module just released a new version explicitly targetting wrappers added by this module. *cough* *cough* http://drupal.org/node/1732374
Guess this is the issue: #1375102: Videos extend abnormally / padding-top issue

RobW’s picture

RobW commented

Ah, sweet catch Dddave. I'm glad this is working out for people. Added a link to the fitvids issue in the issue summary.

RobW’s picture

RobW commented
Issue summary: View changes

added caveat

gmclelland’s picture

gmclelland commented

@RobW - I've noticed the changes you have made for media_youtube and media_vimeo. What about the media integration in the ombed module? Just curious, do you think that needs any changes as well?

RobW’s picture

RobW commented

I haven't looked at the Oembed module in a while. Here's my basic ideas: if you want to check Oembed against them, that would be awesome.

The Media: Youtube and Media: Vimeo (and also Field Collection) patches have two main goals: update the formatters to the latest API's, and make the output easily themable and responsive design ready.

The theme improvements can be broken down into:

  1. use easily overridable width and height attributes, never inline styles;
  2. reduce css to the absolute minimum, divided into base, admin, and theme css files;
  3. make sure content outputting modules aren't adding their own css and js that would be better applied across similar output site wide (e.g., every module that outputs an image shouldn't add it's own width:100%; height:auto; -- instead, let the theme/ front end developer/ a utility module apply the rule to all images on the site at once);
  4. make html structure uniform where applicable (OOCSS / HTML object ready);
  5. ensure things are easy to change by making all markup available in the tpl, in the theme layer, or in very basic Drupal functions like l().

Implementing responsive design and modern front end practices like mobile first, OOCSS, and SMACSS requires a lot of custom, per project theming. If we want Drupal to be used by the best front end developers we have to trust them, and give them the ability to change every tiny bit of html, css, and js output.

At some point most of this will make its way into a Media best practices doc that's being worked on right now. Any feedback is definitely appreciated.

gmclelland’s picture

gmclelland commented

Hey @RobW thanks for all your doing. I'm not actively using these modules, but I did remember that oembed has media integration. I'm assuming oembed supports adding youtube, vimeo, and many other offsite videos which probably need to be responsive.

By the way, I haven't tested the patch posted here, but it does apply fine for me.

RobW’s picture

RobW commented
StatusFileSize
new media-vimeo-code-alignment-1715438-12.patch26.41 KB

Updates formatter validation to avoid conflicts with Media: Youtube.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Needs work

Thank you!
Besides those two minor stuff, seems good to go for me:)

+++ b/css/media_vimeo.wysiwyg.cssundefined
@@ -1,7 +1,11 @@
- *  Overlay for Vimeo thumbnails in the media library browser.
+ *  Overlay for YouTube thumbnails in the media library browser.

I guess this was an accident:)

+++ b/includes/media_vimeo.formatters.incundefined
@@ -63,61 +56,105 @@ function media_vimeo_file_formatter_video_view($file, $display, $langcode) {
+    '#description' => 'A required id in the format <code>media-vimeo-{file_id}< /code> will be added to each iframe.',

You forgot t()

RobW’s picture

RobW commented
Status: Needs work » Needs review
StatusFileSize
new media-vimeo-code-alignment-1715438-14.patch26.37 KB

Thanks for the review. Fixed, and also wrapped the js API id in drupal_html_id(), which we decided was a good idea in #1730746: Adjustments needed to utilize Youtube iframe API.

I don't want this patch to keep ballooning, so I'm not going to include anything from the newer issues we've just committed. M:V might want to look at #1538542: YouTube Preview Image missing alt text attribute in a separate issue though; it's a quick and easy fix.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

i am rtbcing but i guess it ll take a while till this get commited.
Rob, why dont you apply for co-maintainership?

RobW’s picture

RobW commented

I might. The code base of Media(s): Youtube and Vimeo are really close to each other, so having a few overlapping maintainers makes sense.

As an aside, I wonder with the theme output change if this patch should eventually be put into 1-beta or a new 2.x-dev.

ParisLiakos’s picture

ParisLiakos commented

thats true. people usually dont like when their layout change..but more than that i think a new dev branch should be opened that contains a new tab for media browser, like youtube 2.x does. just to keep it consistent

RobW’s picture

RobW commented

After reviewing some of the other modules in alpha/beta that I and many others use in production, I think it's safe to commit this to the 1.x dev branch. Before rolling a new beta or stable release we'll have to add an update hook that gives those updating a template that apes the old markup containers, and those doing a fresh install the new tpl. We'll be doing that in Media: YouTube anyways, so it won't be that much extra work.

As for a 2.x branch and the media browser, let's discuss that in a new issue. We should look at Media as a whole and the direction of the Media browser -- I know the normal library is being removed in favor of the Views library -- before doing any work on new browser options.

RobW’s picture

RobW commented
Status: Reviewed & tested by the community » Fixed

Committed to dev: http://drupalcode.org/project/media_vimeo.git/commit/51c8e3d200a45092c5c....

Thanks for testing, everyone.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

added a link to the fitvids issue