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.
Comment | File | Size | Author |
---|---|---|---|
#23 | media_youtube-use_iframe-1555276-23.patch | 28.68 KB | RobW |
#21 | media_youtube-use_iframe-1555276-21.patch | 28.58 KB | RobW |
#17 | media_youtube-use_iframe-1555276-17.patch | 28.58 KB | RobW |
#11 | media_youtube-use_iframe-1555276-11.patch | 27.89 KB | RobW |
#9 | media_youtube-use_iframe-1555276-9.patch | 27.89 KB | RobW |
Comments
Comment #1
RobW CreditAttribution: RobW commentedComment #2
RobW CreditAttribution: RobW commentedTalked to the maintainers on irc and they approve. Working on a patch now.
Comment #3
RobW CreditAttribution: RobW commentedOK, 80% done. I have a bunch of questions before I can finish, so without further ado:
Edited with answers
media-youtube-outer-wrapper
andmedia-youtube-inner-wrapper
besides the partial responsive js in media_youtube.js? If not, I'm going to take it down to one wrapper withmedia-youtube-[mid]
as the id, and an iframe with amedia-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.
Did it.
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.
$variables['options'][$option]
right away, if all of the$option
s 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.
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.
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.
Comment #3.0
RobW CreditAttribution: RobW commentedword choice and grammar
Comment #4
RobW CreditAttribution: RobW commentedOK, on to the patch. It does a bunch -- I could break it down if people think it's better in a few separate patches.
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.
Comment #5
RobW CreditAttribution: RobW commentedComment #5.0
RobW CreditAttribution: RobW commentedRevised wording a bit.
Comment #6
RobW CreditAttribution: RobW commentedFixed a couple option errors and added kludgey loop support.
Comment #7
dddave CreditAttribution: dddave commentedPatch applies nicely and using fitvids.js just became much easier. Thanks! Anything specific to look after when testing this out?
Comment #8
dddave CreditAttribution: dddave commentedNotice: 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).
Comment #9
RobW CreditAttribution: RobW commentedFixed #8 and switched http/https handling to use the global $is_https Drupal variable (#1368818: HTTPS (SSL) support for YouTube player.).
Comment #10
RobW CreditAttribution: RobW commentedComment #11
RobW CreditAttribution: RobW commentedAdded some colons (whoops, those are important) and cleaned up whitespace.
Comment #12
RobW CreditAttribution: RobW commentedComment #13
dddave CreditAttribution: dddave commentedSweet! Fixed #8 indeed. Anything special to look out for? Seems fine atm.
Comment #14
RobW CreditAttribution: RobW commentedI 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.
Comment #14.0
RobW CreditAttribution: RobW commentedfix up that language
Comment #14.1
RobW CreditAttribution: RobW commentedadded some more reasons to simplify
Comment #15
RobW CreditAttribution: RobW commentedAlso, 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.Comment #15.0
RobW CreditAttribution: RobW commentednicer language :)
Comment #15.1
RobW CreditAttribution: RobW commentedadded more reasons
Comment #16
dddave CreditAttribution: dddave commentedWysiwyg 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!
Comment #17
RobW CreditAttribution: RobW commentedMoving 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.
Comment #18
dddave CreditAttribution: dddave commentedThe patch whitescreened due to "unexpected" stuff. Unless I screwed up patching I think these are the problems:
At line 430 is definitely something wrong
and at line 436
it should be
I guess.
Comment #19
dddave CreditAttribution: dddave commentedCouple 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).
Comment #20
RobW CreditAttribution: RobW commentedThanks, some copy pasting gone wrong. Will fix tomorrow.
Comment #21
RobW CreditAttribution: RobW commentedThanks for the error reports. Give this one a shot.
Comment #22
dddave CreditAttribution: dddave commentedimho 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.
Comment #23
RobW CreditAttribution: RobW commentedTwo updates after testing.
wmode=opaque
to fix flash player z-index and overlay problems.That's all I could find after using this for a week.
Comment #24
dddave CreditAttribution: dddave commentedWorks still great.
Comment #25
esbite CreditAttribution: esbite commentedI 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."?
Comment #26
RobW CreditAttribution: RobW commentedThanks 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.
Comment #27
esbite CreditAttribution: esbite commentedI 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 :)
Comment #28
dddave CreditAttribution: dddave commented@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.
Comment #29
aleksijohansson CreditAttribution: aleksijohansson commentedThis is just awesome, thanks a lot for the patch!
Comment #30
dddave CreditAttribution: dddave commented#1693648: Create stable release of Media: YouTube for 7.x
Comment #31
aaron CreditAttribution: aaron commentedthanks 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.
Comment #32
aaron CreditAttribution: aaron commentedComment #33
RobW CreditAttribution: RobW commentedYEESSSSSSSS. 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.
Comment #34
RobW CreditAttribution: RobW commentedFollow up issue for Media: Vimeo, with patch: #1715438: Update code and bring it in line with the latest Media Youtube release. Fixes numerous issues.
Comment #35
RobW CreditAttribution: RobW commentedInstead of backporting this, I think we're going to work towards a 2.x stable and an upgrade path from 1.x.
Comment #37
rerooting CreditAttribution: rerooting commentedIn IE9 with 7.x-1.0-beta3 I was getting this in the console:
Upgrading to the latest media 2.x solved this issue for me.
Comment #37.0
rerooting CreditAttribution: rerooting commentedrevise language