The Youtube iframe doesn't work in browsers with javascript disabled, so we currently load an object embed in the template and then replace it with the iframe in the browser with javascript. But no-js users are a tiny group, and even then many of them are on mobile using Opera Mini or similar browsers which interpret the iframe correctly and provide a link to the device's Youtube application.

To give the majority of users the best experience we should serve the iframe in html. Right now everyone has to download extra js, a little more html, and perform some dom manipulation to get a player. All users will benefit from removing this drag, especially those on mobile devices where asset weight and cpu are serious bottlenecks. And users browsing without javascript are used to larger inconveniences than missing an embedded video -- Youtube.com doesn't work with javascript turned off.

So this is one part bug report, one part feature request. IMO the current setup is solving a problem that doesn't exist at the expense of the majority of users who do.

To recap why we should use the iframe player by default, and remove most of the current module js:

  • Users who need the iframe, many of which are on mobile, are penalized with extra http requests, extra asset weight, and having to perform dom manipulation.
  • Theming is restricted, since the iframe code is defined in js, not through the tpl.
  • The js iframe replacement causes conflicts with 3rd party responsive solutions, both css based and javascript based like fitvids.js.
  • The js flash and HTML5 tests are superfluous, since the iframe player performs the same tests when loaded.
  • The current field formatter has AS2, AS3, and HTML5 player options, many of which are deprecated or only work with an older player. Patch contains options (some new) which are compatible with the currently recommended AS3 and HTML5 player.
  • All in all, less js, less php, simpler everything with better performance on the client side, and better maintenance and theming on the developer side.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobW’s picture

Category: bug » feature
RobW’s picture

Talked to the maintainers on irc and they approve. Working on a patch now.

RobW’s picture

Assigned: Unassigned » RobW
Category: feature » task

OK, 80% done. I have a bunch of questions before I can finish, so without further ado:
Edited with answers

  • Is there any need for media-youtube-outer-wrapper and media-youtube-inner-wrapper besides the partial responsive js in media_youtube.js? If not, I'm going to take it down to one wrapper with media-youtube-[mid] as the id, and an iframe with a media-youtube-player class.

    I don't think so, at least for new users, so I took them out. Best would be users who upgrade get the same structure, will see if I can write that later.

  • I think it's more flexible and front end friendly to build the iframe markup in the .tpl instead of in the module. If a user (ahem me) hates wrapper divs and strips them out obsessively, they should be able to add classes and ids directly to the iframe, which right now can't be done without hacking the module or regex preprocessing. Any reason not to to build the url with query string in the module, and the actual iframe markup in the tpl?

    Did it.

  • Do we need the video_id as a css id if we already have the mid to use?

    I can think of a few uses, but they're extreme edge cases. Removed, but the variable is still available in the tpl if anyone wants to use it.

  • When building the query array in media_youtube_theme.inc (around line 21), is there any reason to not use $variables['options'][$option] right away, if all of the $options have defaults set in media_youtube_variables.inc? Right now it's a big nested ternary operator, which seems bulletproof but maybe overkill.

    As far as I can see, no, so I simplified things.

  • Is function theme_media_youtube_field_formatter_styles($variables) for Styles module integration? if so, I'll toss a comment on top so people know.

    This one I am still unsure of.

  • And as long as I'm in here, what's media_youtube.wysiwyg.css? Seems like it's all to add the YouTube logo in the web sources modal, but that's been changed to a text list in the latest media dev branch. Can we kill it in a separate patch (oh please)?

    Added a comment in the css file, may take out in a separate patch.

Thanks. Looking forward to getting the patch up here for review.

RobW’s picture

Issue summary: View changes

word choice and grammar

RobW’s picture

Category: bug » task
FileSize
27.7 KB

OK, on to the patch. It does a bunch -- I could break it down if people think it's better in a few separate patches.

  • Makes the default output the recommended iframe player. Changes formatter options to match, and updates with new recommended defaults.
  • Removes js that does flash testing and embed replacing, which is no longer needed.
  • Removes all module specific responsive code, so that users can choose their own site-wide responsive techniques. Might be better in a separate patch...
  • Rewrites most of the theme function. Simplifies, cleans up duplicate variables floating in the tpl, moves the embed element markup from the module to the tpl so front end devs/themers can add classes, etc.

Removing the embed-replacing js and non-working responsive js should fix problems people are having with fitvids.js and the fitvids module. I haven't done testing with WYSIWYG integration, may have broken some things but I'll check that next.

RobW’s picture

Title: Use iframe/html5 ready player by default, without js » Clean up theming function, Use recommended iframe player by default, without js
Category: task » bug
Status: Active » Needs review
RobW’s picture

Issue summary: View changes

Revised wording a bit.

RobW’s picture

Fixed a couple option errors and added kludgey loop support.

dddave’s picture

Category: task » bug

Patch applies nicely and using fitvids.js just became much easier. Thanks! Anything specific to look after when testing this out?

dddave’s picture

Notice: Undefined index: loop in media_youtube_preprocess_media_youtube_video() (Zeile 34 von XXXXXXXXXXXXX/www/drupal/sites/all/modules/media_youtube/includes/themes/media_youtube.theme.inc).

RobW’s picture

Status: Needs review » Needs work
FileSize
27.89 KB

Fixed #8 and switched http/https handling to use the global $is_https Drupal variable (#1368818: HTTPS (SSL) support for YouTube player.).

RobW’s picture

Status: Needs work » Needs review
RobW’s picture

Status: Needs review » Needs work
FileSize
27.89 KB

Added some colons (whoops, those are important) and cleaned up whitespace.

RobW’s picture

dddave’s picture

Status: Needs work » Needs review

Sweet! Fixed #8 indeed. Anything special to look out for? Seems fine atm.

RobW’s picture

I could use some help testing on the WYSIWYG insert side, and just getting more eyes on the patch in general.

My initial tests on WYSIWYG work fine, except for the preview image. Did Media Youtube used to show the Youtube preview image on insert in 7.x-2.x-dev, or did we lose that a while back? As long as I didn't break the insert img, I think this is almost ready to go.

RobW’s picture

Issue summary: View changes

fix up that language

RobW’s picture

Issue summary: View changes

added some more reasons to simplify

RobW’s picture

Also, do people want to include the developer-only parameter html5? It forces the html5 player if html5 is available; further explanation here: http://drupal.org/node/1660980#comment-6184236.

RobW’s picture

Issue summary: View changes

nicer language :)

RobW’s picture

Issue summary: View changes

added more reasons

dddave’s picture

Wysiwyg integration seems to work indeed. As it appears the preview image went away some time ago. At least my installs with unpatched media_youtube don't give a preview image.

It seems this patch doesn't break anything and improves a lot. I am all for including this into the 2x-dev and let people test it by force. 2x is under development anyways and nobody should get hurt if we include it but we would get more eyes on the new functionality.
Concerning #15 -> let's debate this in a dedicated issue. This patch is already huuuge. (Spoiler: I am all for it).
Great work! Thanks!

RobW’s picture

Priority: Normal » Major
FileSize
28.58 KB

Moving this to Major since committing it would fix the following issues:

#1578174: Youtube-nocookie.com support
#1667660: Using Javascript API
#1481752: created option to autohide the controls

and let us close these, with a recommendation to use one of the css methods in the theme, or the fitvids js/module:

#1535954: Responsive Embeds
#1278180: Add a library to handle fluid sizes for embedded media.
#1112462: YouTube videos no longer resize to fit their containers

Last patch until someone finds a bug. This has some minor code and doc fixes, and switches back to default https until the issue mentioned in #9 is closed. Shouldn't try to hit a moving target.

dddave’s picture

The patch whitescreened due to "unexpected" stuff. Unless I screwed up patching I think these are the problems:

At line 430 is definitely something wrong

+  // Reverse the reversed option so that it makes sense in the tpl.
+  if ($variables['options']['controls']) = 1) {
+    $variables['options']['controls']) = 0;
   }

and at line 436

+  else {
+    $variables['options']['controls']) = 1;
   }

it should be

+  else {
+    $variables['options']['controls'] = 1;
   }

I guess.

dddave’s picture

Status: Needs review » Needs work

Couple of notices I am getting, too.

Notice: Undefined variable: width in include() (Zeile 21 XXXXXXXXXXX/www/drupal/sites/all/modules/media_youtube/includes/themes/media-youtube-video.tpl.php).

Notice: Undefined index: url in include() (Zeile 21 XXXXXXXXXX/www/drupal/sites/all/modules/media_youtube/includes/themes/media-youtube-video.tpl.php).

Notice: Undefined variable: height in include() (Zeile 21 XXXXXXXXXX/www/drupal/sites/all/modules/media_youtube/includes/themes/media-youtube-video.tpl.php).

RobW’s picture

Thanks, some copy pasting gone wrong. Will fix tomorrow.

RobW’s picture

Status: Needs work » Needs review
FileSize
28.58 KB

Thanks for the error reports. Give this one a shot.

dddave’s picture

imho this patch is ready for Dave's eyes. No notices and it seems to work well under production circumstances. Improves soooo much all around and lifts this module way up. Great work RobW.

RobW’s picture

Two updates after testing.

  1. Adds wmode=opaque to fix flash player z-index and overlay problems.
  2. Removes mid as id in the template file and adds it as a class. A more sensible default, avoids duplicate id errors if a video is shown more than once on the same page.

That's all I could find after using this for a week.

dddave’s picture

Works still great.

esbite’s picture

I really think this is the way to do it, but I just discovered that the standard iframe embed from youtube cannot play videos on iOS. Apparently it works with the old embed code because iOS detects it and replaces it with its own native youtube player.

See the issue described more in detail:
http://stackoverflow.com/questions/10969334/making-youtube-com-embed-url...

Have you tested this patch on iOS? Maybe you have another solution to the problem "Your browser does not currently recognize any of the video formats available."?

RobW’s picture

Thanks for the report and link. I've tested in iOS 3.x and 5.x on iPhone with no trouble. The html5 play button appears and the video loads in the iOS YouTube player.

Can you reproduce the Stack Overflow bug while using this patch? My instinct is that if this was a problem Google would have fixed it in the iframe asap, and we wouldn't need to change anything here.

esbite’s picture

I have not actually tested the patch, but thanks for your response that it should work. I started comparing my iframe tags to the official youtube tag bit by bit and found that I was using "http://youtube.com/embed/123" and just by adding "www" to the src, it started working. I guess it properly triggered the iOS native player.

So, whenever you embed youtube, be sure to include the full domain :)

dddave’s picture

Status: Needs review » Reviewed & tested by the community

@RobW Could you ping Dave on IRC or is there some other only mildly intrusive way to get him take a look at this? Let's move this forward.

aleksijohansson’s picture

This is just awesome, thanks a lot for the patch!

dddave’s picture

aaron’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

thanks for the awesome work, RobW and dddave! I have committed this to the 7.x-2.x branch. However, the patch does not apply to the 1.x branch, so it needs a bit more work for that.

aaron’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev
RobW’s picture

YEESSSSSSSS. OCD standardistas and front enders everywhere thank you!

Be aware this won't upgrade perfectly on existing installs. A bunch of the settings are different, as is the theme implementation (classes and structure).

I'm working on some other contrib for the next month or two so I won't be able to backport any time soon, but I should have time to do a quick Media: Vimeo patch to bring the theme implementations back in line with each other. If someone else wants to jump in and get their hands dirty here, I'm happy to answer any questions.

RobW’s picture

RobW’s picture

Status: Patch (to be ported) » Fixed

Instead of backporting this, I think we're going to work towards a 2.x stable and an upgrade path from 1.x.

Status: Fixed » Closed (fixed)

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

rerooting’s picture

In IE9 with 7.x-1.0-beta3 I was getting this in the console:

SCRIPT16385: Not implemented
 
media_youtube.js?moe8ub, line 18 character 7

Upgrading to the latest media 2.x solved this issue for me.

rerooting’s picture

Issue summary: View changes

revise language