Overhaul the media browser code to not use an iframe, and be more understandable, maintainable, and extendable

I pushed these changes to http://drupalcode.org/project/media.git/shortlog/refs/heads/1139514_medi...

It does not merge cleanly into the file_entity branch which will soon become the main branch, I'll create another branch to tackle that.

After initial testing, here are the bugs I've found. All are noted in attached skitches:

A couple quick code things

function media_media_display_types() {
  $path = drupal_get_path('module', 'media');
  $display_types = array();

  $display_types['list'] = array(
    'title' => t('List'),
    'description' => t('Display as a list.'),
    'icon' => $path . '/images/display-list.png',
    'icon_active' => $path . '/images/display-list-active.png',
    'callback' => 'media_browser_table',
    'file' => drupal_get_path('module', 'media') . '/includes/media.admin.inc',
  );

  $display_types['thumbnails'] = array(
    'title' => t('Thumbnails'),
    'description' => t('Display as thumbnails.'),
    'icon' => $path . '/images/display-thumbnails.png',
    'icon_active' => $path . '/images/display-thumbnails-active.png',
    'callback' => 'media_browser_thumbnails',
    'file' => drupal_get_path('module', 'media') . '/includes/media.admin.inc',
  );

  return $display_types;
}

The media_browser_thumbnails function is not in media.admin.inc, you've put it in media.browser.inc. I suppose it belongs in pages.inc tho...

It would be good to add docblocks on the new functions. There are about a dozen in media.browser.inc w/o any docs on them.

In general, I like not using an iframe, but my concerns are were totally valid. Just add this to your theme's CSS file:

td { padding: 5px; border:2px solid black;}

This will pretty much wreck havok on the list view.

Or if we do:
form {padding:margin:10px} or input {} or various other base HTML tags. jquery will reset the dialog itself, but the contents are up for grabs. I'm not at all confident that this will work in most themes. I think this is why similar javascript widgets often use iframes.

I'm not saying we *can't* fix it, but it's not as simple as "just works".

Here are a few examples. The first is one of the most widely used themes for D7 on d.o
The others are gardens sites I found after picking through a few and testing.

I don't think we can say "blame the theme" Especially in Gardens where you have total amateurs using a GUI tool to change the styles. For instance, take a stock product theme, love the black background.

I'm not sure if I'm looking at the right code. Just to check, what I did was push into the 1139514_media_browser_fat a merge of 7.x-1.x commits that happened since you merged the file entity work. And I also changed 2 files from Windows line endings to Unix line endings.

In case it helps, I'm adding a patch here of 1139514_media_browser_fat changes relative to current 7.x-1.x. Please inspect either this patch or the git branch to see if it has the correct code, or if there's some artifacts of a merge gone bad.

On a fresh D7 install, if I just install the Media module, here's what I get when I try to add media from admin/content/media: https://skitch.com/effulgentsia/r9dhp/content-localhost.

If I then also enable WYSIWYG, here's what I get when using its toolbar: https://skitch.com/effulgentsia/r9dh5/create-article-localhost.

I decided to stop my review at that point, as there's probably something obviously going wrong that I'm hoping you can shed some light on.

Better title? Also, upping to "major", since this is pretty huge and awesome, once the bugs are worked out.

even more catchy title in the hopes that people who might care about this have a chance to see it.

Subscribing.

Subscribing

subscribe

sub

I was about to create a new issue, but the resolution of this problem will make the browser more useful/understandable.

The media selection interface (the browser) displays thumbnails, with a severely shortened file name. This causes a real problem if you have any number of files that begin with the first 15 characters. For example, a PDF that is broken out into sections, so that a visitor can download the whole PDF, or just a section of it if they want. I've attached a screenshot of our current situation that is causing this problem. I can think of two possible solutions, I'll look into both of them when I have a chance.

1. Provide a "list view" and "thumbnail view" options like the admin/content/media page
2. Give each media item listed a "title" attribute with the full file name, so that when hovering over the item, we get the full name in a tool tip

I'm guessing that 2 is easier, but 1 would be way nicer.

About the CSS issues - I am experiencing the same with the toolbar all to often: some fancy CSS style in my theme messes with the fontsize of the toolbar, and everytime I think: someone has not thought enough of this.
Would it be a solution to bruteforce lock the design by having a cascade like
#media-wrapper #browser-window #inner-wrapper .yourclass and then adding CSS for the Browser and other elements that style the interface?

I see that is ugly, but a theme would need three nested IDs in selectors to break this (or is my CSS knowledge mistaken).
We often want clean CSS and not too much nesting in Html. But Themes breaking modules and core CSS is a serious and visible problem and maybe such a pragmatic approach could help the iframe - no iframe issue.

subscribe

I pushed up a few minor fixes to the issue branch, as well as some merges from 7.x-1.x that were dropped for some reason from #18.

Here's an up to date patch file representing this issue's work relative to 7.x-1.x. Still needs review. Will start on that shortly.

+++ b/js/wysiwyg-media.js
@@ -35,17 +38,20 @@ Drupal.wysiwyg.plugins.media = {
+    Drupal.ajax[instanceId+'-ff'].element_settings.url = Drupal.basePath + 'media/' + mediaFile.fid + '/format-form';
+    Drupal.ajax[instanceId+'-ff'].options.url = Drupal.basePath + 'media/' + mediaFile.fid + '/format-form';

s/Drupal.basePath/Drupal.settings.basePath/

That was preventing ability to test insertion to wysiwyg, so pushed fix to feature branch, and am uploading modified patch.

sub

This is very incomplete (the patch is large), but here's a partial review. Will post more when I have a chance.

+++ b/includes/media.admin.inc
@@ -81,165 +48,38 @@ function media_admin($form, $form_state) {
-function media_admin_validate($form, &$form_state) {
+function media_admin_delete_validate($form, &$form_state) {

Why change this function name? Even if "delete" is the only current bulk operation, couldn't more operations be added later?

+++ b/includes/media.filter.inc
@@ -269,6 +279,30 @@ function _media_generate_tagMap($text) {
+function media_format($media) {

This function (and all new functions) needs a docblock. Also, s/$media/$file/.

+++ b/includes/media.filter.inc
@@ -278,6 +312,10 @@ function _media_generate_tagMap($text) {
+  drupal_add_css(drupal_get_path('module', 'media') . '/css/media.thumbnails.css');

Later in this function, CSS is attached with #attached. Can this be as well? Also, can all attached files be attached in the same place, rather than some at the beginning of the function, and some at the end?

+++ b/includes/media.filter.inc
@@ -320,7 +358,7 @@ function media_format_form($form, $form_state, $file) {
-  $form['#attached']['js'][] = $path . '/js/media.format_form.js';
+  #$form['#attached']['js'][] = $path . '/js/media.format_form.js';

If this JS file is no longer relevant, can this line be deleted rather than commented out?

+++ b/includes/media.filter.inc
@@ -361,6 +399,33 @@ function media_format_form($form, $form_state, $file) {
+      '#value' => isset($form_state['values']['trigger']) ? $form_state['values']['trigger'] : $_POST['trigger'],

Why is this needed? Doesn't FAPI do this automatically (and with greater security)?

+++ b/media.module
@@ -252,10 +259,15 @@ function media_menu() {
+function media_dialog_theme() {
+  $ajax_theme = isset($_POST['ajax_page_state']['theme']) ? $_POST['ajax_page_state']['theme'] : variable_get('theme_default', 'bartik');
+  return $ajax_theme;
+}
+

Why use this function at all instead of ajax_base_page_theme(), which implements better security?

+++ b/media.module
@@ -423,36 +433,6 @@ function media_styles_style_flush($style) {
-function media_page_alter(&$page) {
-  // Show a nagging message when the media installation needs to be completed.
-  if (user_access('administer media') && media_variable_get('show_file_type_rebuild_nag')
-    // Prevent form submissions from creating duplicate messages.
-    && ($_SERVER['REQUEST_METHOD'] == 'GET')
-    // Show on all the admin pages, except the batch and the rebuild form.
-    && path_is_admin(current_path()) && (arg(0) != 'batch') && (current_path() != 'admin/config/media/rebuild_types')) {
-    drupal_set_message(t('Media module install is not complete. <a href="@type_rebuild_link">Finish the install</a>.', array('@type_rebuild_link' => url('admin/config/media/rebuild_types'))), 'warning', FALSE);
-  }

This part is still needed.

+++ b/media.module
@@ -962,13 +952,13 @@ function media_element_process(&$element, &$form_state, $form) {
+//  $setting['media']['elements'][$element['#id']] = $element['#media_options'];

If we're commenting this out, then should at least #media_options still be used somewhere above?

Powered by Dreditor.

+++ b/media.api.php
  * - #weight (optional): Weight of the plugin in relation to other plugins
  *  when being displayed, e.g. tabs in the browser.
+ * - #title  (required): The title of the tab to be displayed in the media browser
+ * - #form_id (required): The name of the form that will be used to select media
+ * ¶
+ * Within this function the implementor is responsible to call any required
+ * module_load_include calls so that the central browser processing callback
+ * can process the form correctly.
  *
  * @example
  * <code>
+ * ¶
+ * module_load_include('inc', 'my_module', 'my_include');
+ * ¶
  * array(
  *  'unique_plugin_name' => array(
+ *     '#title' => 'unique_plugin_tab_name',
+ *     '#form_id' => 'unique_plugin_form_id',
  *     '#weight' => 42,
  *   ),
  * );

So, this patch removes hook_media_browser_plugin_view() and modifies what's expected from hook_media_browser_plugin_info(). I can't tell yet if this BC break is necessary or not. If it's not, then let's not do it. But if some BC break around media browser plugins is needed in order to accommodate the switch to an AJAX-based rather than iframe-based browser, then let's use the opportunity to do it right, since once we release RC1, we won't have an opportunity for another BC break until we start on Media version 2, and that might not be for a while.

What do I mean by "do it right"? We should use CTools plugins, of course. The dev version of Media now depends on CTools anyway. If you haven't worked with CTools plugins before, read up on "help/plugins-creating.html" and "help/plugins-implementing.html" within the CTools module. In addition to the many other benefits that come with following what is now the defacto Drupal standard for how to work with plugins, the ctools_plugin_get_function() function has built-in support for lazy loading. This lets us avoid requiring plugins to call module_load_include() within their info definition, which is good, since we need to get the info for all plugins when we launch the browser, but we don't really need the form callback included until we go to that plugin's tab.

subscribe

subscribe

Subscribe. I would like to alter the tabs (Web / Upload / Library) depending on the role, that doesn't seem easy to do now, but making the whole popup follow standard ajax/tab standards would be helpful.

We do need the BC break for plugins to switch away from an iframe. So I made a commit to the feature branch yesterday that implements ctools plugin support. I merged in the 7.x-1.x branch as well, so the branch is up to date. I don't have it documented at all though - I'm not even sure how to document a ctools plugin requirement. :/

subscribe

It's nice to see this issue getting some more love (Dave). Let's consider the CSS bleedthrough issue w/o the iframe a bit too. I demonstrated how fragile it is earlier, it would be nice to take some measures to protect it from themes going bananas.

Re #34: In Drupal Gardens, we have a fairly robust (probably not 100% robust) CSS reset file that we use for the Theme Builder. Next chance I have, I'll clean that up, and attach it to this issue for testing and review.

Additionally, some consensus is emerging on #1175830-27: Update to jQuery UI 1.10.2. We may want to factor that in to the code being proposed in this issue.

We may also want to consider adding the CSS reset and whatever is needed for accessibility as per above into the Dialog API project, and make the Media browser use that module.

Also, moving this issue to 2.x. All new code needs to go into there first. Some issues could get backported if there's motivation to do so, but this one probably won't, as it involves major BC breaks.

Subscribe

I merged the 2.x branch into the 1139514_media_browser_fat branch (where did the 'fat' come from btw?). Here's the diff between that branch and 7.x-2.x, for those following along at home.

Just a quick FYI of one problem I experience with the iframe popup - often I get my entire website popping up in the iframe and not just the file upload dialog. Sometimes it works appropriately. I haven't noticed any pattern that causes it.

desierto, you mean that happens without this patch applied, right?

Sorry to confuse... yes my issues are without the patch. I know this thread has worked up a patch... I was just mentioning one of the issues with using iframes. I have not tried the patch as my site is on a shared host and I'd have to apply it manually... it takes too much time with the numerous patches for various modules, some pretty complicated.

That's a nice improvement, that iframe popup was a strange thing.

I'd like to implement the browser as form fields directly in the node form, without popups, in order to be able to have fields for "Upload", "Web" and "Library" displayed as normal fields, populating the media field itself, but it is more difficult than I thought - these changes to the popup make it a little easier though.

That would let the user type in a URL in a textfield without having to open the popup beforehand, similar to what facebook does.

Are there any plans for this ? Interest ? Ideas ?...

BTW, I'm confused about what version should these patches be applied against ? It seems that the "fat" branch has the last patch applied though.

@desierto in reply to #38 I have found this happens to me too. I get the admin menu in the iframe popup which pushes the submit and cancel buttons out of the frame. This seems to happen only after clicking "Add another item" on the node edit form. I created a bug for this too http://drupal.org/node/1264982

How do I test this? I've checked out the 1139514_media_browser_fat git branch and copied it into my drupal install.

After drush en media I get:

The following extensions will be enabled: file_entity, media
Do you really want to continue? (y/n): y
PHP Fatal error:  Call to undefined function media_type_invalid_files_count() in ...sites/all/modules/contrib/media/media.install on line 83

Then on uninstall I get:

PHP Fatal error:  Call to undefined function media_variable_default() in /home/adamb/projects/mohbigbird/sites/all/modules/contrib/media/media.install on line 104

subscribing.

subscribing

I wanted to work a little on that issue and found a little problem. The media browser popup loads a wrong path if the drupal is installed in a subdirectory (only found this issue on the 'fat' branch).

Here is a minimalist patch just for that.

edit: I was thinking about using ctools to make media reinvent less stuff (modal and maybe some ajax stuff). The more I think about it the more I doubt. What do you think?

Should have checked the code arround for other instances of the same issue as above. One more in this patch.

Sub,

So where are we with all this? I have a site that's very close to launch and the library issues discussed here are very relevant to the project.

I am a little confused with the wealth of patches in here, what versions am I needing to get the latest version? I can git clone the 1139514_media_browser_fat branch but what patches from here are required on top?

subscribe

I just tested the code in the media browser fat branch and the final insert of an image does not work. The error is "Uncaught ReferenceError: CKEDITOR is not defined" on line 72 of wysiwyg-media.js. Seeing as how I'm using TinyMCE there I'd expect that the problem is somehow tied to the new media library code coupled to CK Editor.

I discussed this with james.elliott, bangpound, and a bunch of other people, and consensus is that removing the iframe is not viable. As initially pointed out in #5, we need to isolate the media browser contents from receiving unexpected base page theme CSS. By default, the media browser contents should be rendered in the admin theme (a site should be able to override this, of course, but that's the expected default behavior). Per #35, I thought we could implement CSS isolation in the media browser with an appropriately constructed reset file, but the challenge with that is that we want to replace the media browser's library tab with a View (#1224766: Remove default media browser and replace with a default view), and that can lead to various 3rd party libraries providing CSS that we won't be able to reset to. So, unless someone comes up with a brilliant idea for how to resolve this, I'm afraid we're stuck with an iframe as the only technology that provides the needed CSS isolation.

So, I'm closing this issue. Apologies to everyone who invested lots of time in it. However, it wasn't a complete loss. In the course of this issue, James and several other people (re)familiarized themselves with some of the legacy crufty JavaScript code, and so in separate issues, as these people have time, they'll be posting some smaller, digestible issues to incrementally make the JavaScript easier to maintain.

I am a little confused with the wealth of patches in here, what versions am I needing to get the latest version? I can git clone the 1139514_media_browser_fat branch but what patches from here are required on top?

All patches posted in the Media issue queue should be rolled against the 7.x-2.x branch. We should delete the 1139514_media_browser_fat branch at this point, but I don't want to do that quite yet, in case people who experimented in that branch want to look back on that branch's history and repurpose some of that work into new patches.

I'm reopening this with the hope that there's stomach to reconsider this decision. It sounds like this came down to the inability to do complete CSS isolation without an iframe, but imho that's a misguided conclusion. It just seems wrong from a purists' standpoint to be forced in to kludgy, x-iframe javascript communication and cruft-filled markup just to satisfy a very high level presentation (CSS!) requirement.

I've been putting effort into cleaning up a number of the modules we use in nearly all projects to use standard JQuery UI-based markup (and widgets) wherever relevant, in particular replacing custom widgets with stock jquery ui equivalents. I've also created a jQ UI theme for the admin theme we use (Rubik) with the hope of creating a very slick and consistent administrative user experience.

I took a look at media for the first time in a while and was thrilled to see how much more stable it was, and since media's using a jquery ui dialog I thought it would fall right in line with the rest of my efforts, but the wierd title-bar hiding, extra elements in the markup, and finally the iframe and control buttons stopped that effort in it's tracks.

Can you point out any specific examples of what third party tools would add in CSS -- particularly when using a view -- that's so egregious as to make media browser unusable? Could the maintainers provide any feedback on the approach of stripping out a great deal of the style css that's current in the browser code and relying more on jquery to produce a themable consistent look and feel that works with whatever admin theme is in place (assuming it's got a reasonable jquery UI implementation, or works ok with the built in jquery UI theme)?

I am closing this again as it seems to me that there is no progress in sight and the issue was closed already by one of the maintainers.

If there is still interest in tackling this I highly recommend participating in the Media Office hours which happen bi-weekly. Dates can be found on the Media Group page.