Automatically open the file edit modal after uploading a file

Here's a patch. Dave, can we review this in the sprint tomorrow?

Also, this issue is somewhat related to at least these three other issues:

• #835516: Add ability to edit file meta data and fields when embedded in WYSIWYG
• #1404084: Edit media instance in Wysiwyg field
• #1062304: Fill in the available fields when creating media in the WYSIWYG

@becw - your patch isn't applying with the latest media 2.x-dev release and drush make. Can you reroll the patch?

@gmclelland hmm, it applies cleanly with git apply 1426730-1-media-edit_after_upload_in_browser.patch and with patch -p1 < 1426730-1-media-edit_after_upload_in_browser.patch. Is your makefile block like one of these?

; Plain download
projects[media][version] = "2.x-dev"
projects[media][patch][] = "http://drupal.org/files/1426730-1-media-edit_after_upload_in_browser.patch

; Git repository
projects[media][type] = "module"
projects[media][download][type] = "git"
projects[media][download][branch] = "7.x-2.x"
projects[media][download][url] = "http://git.drupal.org/project/media.git"
projects[media][patch][] = "http://drupal.org/files/1426730-1-media-edit_after_upload_in_browser.patch"

Sorry @becw - your patch does apply. It was conflicting with another patch.

After a quick test, I get the following warning after uploading a file.

Warning: filesize(): stat failed for temporary://dsc_0048.jpg in file_entity_image_dimensions() (line 160 of /home/quickstart/websites/updater.dev/profiles/cmf/modules/contrib/file_entity/file_entity.file.inc).

The file dsc_0048.jpg was uploaded

Everything still seems to work fine. After the file is uploaded, I can immediately edit the name and an added caption text field.

I'm running with the latest dev version of File Entity and Media 2.x.

@becw

That error message is related to this commit of the file entity module http://drupalcode.org/project/file_entity.git/commit/d8975331aa752a229e4... from #1447790: image_load makes admin/content/media unusably slow.

If I back up commit earlier, I no longer see the warning and everything works as intended with the patch.

So, I'm not sure if this patch needs to be reworked to accommodate the changes in the latest commit of the file entity module or if we need to open a new issue with the file entity module?

Hope that helps,
-Glenn

I was getting also troubles when trying to apply the patch (but maybe because of some other commits that occured since march 22, haven't checked).

Did a reroll. I think the edit entity form could use some padding still.

A related question maybe, but somehow the view library denies clicking on a link - I was just trying to add an edit link underneath a file. With this patch, people would be allowed to browse the library and edit from there as well if they wanted.

Thanks @swentel - I tested the file uploading and immediate editing portion of your patch and everything seems to work as you said. I didn't test the second part of the patch for the edit links.

The errors I was getting in #7 were just fixed in the file_entity module with this patch http://drupal.org/node/1533576#comment-5882834.

Hope that helps

Thanks @swentel this patch works for me. A couple thing I noticed
* the modal that renders automatically after you upload a file is different than when you click on the edit media button. Not a big deal. Seems like when you click edit it uses ctools.
* the modal that renders after uploading a file is kinda small and causes a scrollbar. Its using the same size as the upload modal was. I looked at the code and it looks like it resizes the iframe. I'm not sure why it isn't working - if i add #media-browser-page-wrapper div.messages.status{display:none;} to my styles the scroll bar goes away.
* Once the modal switches to the edit screen console throws a js error but still works. Uncaught TypeError: Cannot read property 'browser' of undefined in media.popups.js:103

related discussions #1553114: Adding files should be a multi-step process over in the file entity issue queue.

Doesn't looks like this currently checks if the user has permission to edit files? In which we shouldn't open the file edit modal?

Should work with #1227706: Add a file entity access API also.

#9: 1426730-9.patch queued for re-testing.

The patch in #9 no longer applies against the latest dev versions of media and file_entity modules.

Let's try this one.

In the media_form_alter, the if-statement that control whether or not set the new submit-handler on the file_entity_edit-form looks like this:

  elseif ($form_id == 'file_entity_edit' && !empty($params['render']) && $params['render'] == 'media-popup') {

However, $params isn't defined anywhere in the functions scope, so I don't see how this could ever be triggered. I changed all $params into $_GET and it worked as expected.

Otherwise, very nice patch.

This one fixes the issue found in #19 and provides user access support as requested in #13.

Another one using media_access() instead of user_access() per davereid

The patch in #18 didn't work, but the patch in #21 DID work. I didn't test the access calls. I just tested that it would automatically open the file edit modal when you upload a file.

Thank you @redndahead

So after reviewing the access control really doesn't make sense quite yet. I think we can leave it as is for now, but once the access api goes in we may have to revisit it. Since @gmclelland tested the patch and it works and access isn't an issue I'm marking this as rtbc

+++ b/media.moduleundefined
@@ -562,19 +562,48 @@ function media_form_alter(&$form, &$form_state, $form_id) {
+  // Expose file edit form after selecting files from the media browser.
+  // @see media_browser_form_submit(), media_browser_edit_form_submit()
+  $params = drupal_get_query_parameters();
   if (!empty($form_state['#media_browser'])) {
     $form['#submit'][] = 'media_browser_form_submit';
   }
+  elseif ($form_id == 'file_entity_edit' && !empty($params['render']) && $params['render'] == 'media-popup') {
+    $form['#attached']['js'][] = drupal_get_path('module', 'media') . '/js/media.browser.edit.js';
+    $form['actions']['cancel']['#access'] = FALSE;
+    $form['actions']['delete']['#access'] = FALSE;
+    $form['actions']['submit']['#submit'][] = 'media_browser_edit_form_submit';
+    $form['#submit'][] = 'media_browser_edit_form_submit';

I think we should move this logic to media_form_file_entity_edit_alter() instead so we're not calling drupal_get_query_parameters() on every single form ever?

+++ b/media.moduleundefined
@@ -562,19 +562,48 @@ function media_form_alter(&$form, &$form_state, $form_id) {
+    if (media_access('edit')) {
+      $form_state['redirect'] = array("file/{$file->fid}/edit", array('query' => array('render' => 'media-popup')));
+    }
+    else {

What happens if we add 'destination' => 'media/browser?render=media-popup&fid=' . $file->fid to 'query' in this redirect? Do we even need the second redirect form logic then?

+++ b/media.moduleundefined
@@ -562,19 +562,48 @@ function media_form_alter(&$form, &$form_state, $form_id) {
-  if (!empty($form_state['files'])) {
-    $files = $form_state['files'];
-    $form_state['redirect'] = array('media/browser', array('query' => array('render' => 'media-popup', 'fid' => array_keys($files))));

Why is this being removed? Can we leave it in for now or does it cause problems?

When I have the patch applied, after I submit the upload form, I see the modal window changes pages, but then it shrinks down to a 20px high dialog and I cannot see or do anything with the modal.

This seems to work just fine when used with the WYSIWYG button, but not when I am selecting a media file for a normal file/image field. The JS errors I get are:

Uncaught Error: can't load XRegExp twice in the same frame
Uncaught TypeError: Cannot read property 'browser' of undefined in media.popups.js:103

Revised patch with the changes, but still encountering the JS error.

I'll take a stab at trying to get the above mentioned error fixed, and review the patch today.

If it doesn't already do so, I'll try to get multi-select/upload working too, as I need that for an upcoming project... But that might belong to a separate issue :)

This error Uncaught TypeError: Cannot read property 'browser' of undefined in media.popups.js:103 is easily solvable.

It's because Drupal.media.popups.mediaBrowser.mediaBrowserOnLoad checks to see if this.contentWindow.Drupal.media.browser.selectedMedia contains anything. If it does, it should auto-"submit" the dialog, by "clicking" the OK, button.
However, on the edit-page, Drupal.media isn't set, because it's only set on 'media/browser' by the MediaBrowser plugin.

So the onload-function just needs to check if Drupal.media exists before trying to read properties from it. The patch is trivial, so I'll continue with a few other clean-ups/todos before updating this issue with a new patch.

Haven't been able to recreate the Uncaught Error: can't load XRegExp twice in the same frame error in Safari - which browser is that in?

Okay, here's the patch that solves the above mentioned JS bug.

I've also added support for multiselect upload-edits (that is, uses plupload to upload multiple files, then ask you to edit all of then, then returns). Note that this requires plupload and multiform.
I've tested this in the browser-testbed, and appears to work fairly solidly.

Tried with patch in #30, and I'm still experiencing the problem with the window shrinking down to 20px or so in height after seleting and confirming the file. This makes it impossible to finalise the added media. Looks like the some fields show for a split second before the modal being reduced in height and breaking.

I'm trying to attach a pdf file to a file field with the media file selector widget.

I have a js error in the console:

Uncaught TypeError: Cannot read property 'browser' of undefined media.popups.js:103

mrfelton: I'll try to take a look at it tomorrow. What browser are you using when you experience this?

@fangel Chrome, OSX. Although I tried with Firefox too and had the same problem.

mrfelton: Could you verify that you have the latest edition of the patch, and that you have refreshed the JS in your browsers cache? I have my line 103 as

if (this.contentWindow.Drupal.media == undefined) return;

which is specifically there to prevent the error you're mentioning above. (Note, I had a lot of trouble getting my browser to properly refresh the JS files, it kept reading the old from it's cache, even after reloading. I had to manually visit the JS file directly in the browser and hit refresh there)

--

I believe the problem with not resizing to the correct size, is caused by the JS error, halting JS execution, so you never get to the JS that resizes the popup.

May have been a caching issues after all. Patch in #30 is now eorking. Nice one fangel!

I think this is ready to go. @davereid is your issue fixed with this?

Tested and working great for me, thank you!

Okay, I was wrong about my assumption about what could force the dialog to resize to a incorrect size.

It was a incorrect way of figuring out how much "air" was above the dialog, that didn't take page-scroll into account, so if you were scrolled down on the page when you launched the Media browser, it would resize incorrectly if it didn't fit in the size dictated by Media.

Here's an updated patch. Only changes made is the re-ordering of the setting of padding, and the resize routine (to help get the size closer to the real size) and then the crucial addition of $(top).scrollTop()

Marking as 'needs review' again, so please test out the new changes and mark it as RTBC if it works for you.

error: patch failed: js/media.popups.js:100
error: js/media.popups.js: patch does not apply

Rerolled

I think the behavior is a bit strange after applying the patch (see attached screenshot). Why does that page need to show (and it is strange that I already get a chance to replace the file just uploaded).

Hi Lars: The page need to change into the edit-page - it just shows you file/:fid/edit. And the standard behaviour isn't to have a replace file-field, that must be another contrib module you have installed that enables that functionality.

I've attached a screenshot of a vanilla Drupal 7.14 with latest git checkout of Media 7.x-2.x and the patch applied (I've also rerolled my patch against latest git, just in case)

@lsolesen Haven't tested yet, but the recent commit on the File Entity module #1123570: Replace file while retaining field metadata might be showing you the file replace stuff.

Env

PHP 5.3.6-13ubuntu3.8 with Suhosin-Patch
Drupal 7.14

Applying against latest 2.x-dev:

Checking patch js/media.browser.edit.js...
Checking patch js/media.format_form.js...
Checking patch js/media.popups.js...
Checking patch js/plugins/media.views.js...
Checking patch media.module...
Hunk #1 succeeded at 440 (offset 9 lines).
Hunk #2 succeeded at 472 (offset 9 lines).
Hunk #3 succeeded at 537 (offset 9 lines).
Hunk #4 succeeded at 548 (offset 9 lines).
Hunk #5 succeeded at 577 (offset 9 lines).
Applied patch js/media.browser.edit.js cleanly.
Applied patch js/media.format_form.js cleanly.
Applied patch js/media.popups.js cleanly.
Applied patch js/plugins/media.views.js cleanly.
Applied patch media.module cleanly.

Attached screen show of modal after I submit upload image and it this work's OK. What not is working just right is if you now click 'Replace file' and upload another file. This will close the modal and you are not able to change the media title without reopening the media file.

steinmb: So what you're saying is that you want the 'edit'-dialog to keep popping up, if you keep replacing the file? Like Upload -> Edit -> Replace -> Edit -> Replace -> Edit -> [repeat] -> OK?

Why would you want to replace a file you just uploaded? Wouldn't it make more sense to not have the option to replace files that was just created?

I'm just pointing out inconsistency in the user experience. If we want to keep the replace function, I do think we have to loop back and repeat, though a much better idea, as you pointed out, is simply to remove the replace option as it does not make any sense to me either.

I agree, but that is an issue to bring up with the File Entity module, as it's them that introduced it (in #1123570: Replace file while retaining field metadata).

I think it would be ok to hide the replace file field in this context. It doesn't make sense to show it in this context and it's causing problems (although it does if you're using the 'Edit media' button).

This one removes the replace field in the modal.

The patch remove the field though I have not reviewed the code but to me is it RTBC.

I also played with 'Edit media' button and found another problem. It might be unrelated to this issue but I should still mention it:

If you click 'Edit Media' and choose to replace media file or/and media name, press save. The modal is then closed but the form is not updated to show the new image/name.

If I preview the node or save it I then get the drupal_set_message() 'Image my_image_name has been updated.' and the thumb and name is updated.

Yes, the edit-button is unrelated to this issue, as it only deals with being able to edit just uploaded files – but you have a very valid point. It is a weird behaviour.

Pushing this to RTBC then

Lets open another issue for the problem stated in #54. It would be nice to have that solved eventually but probably shouldn't hold back this from reaching out end users.

#50 works as expected. Nice enhancement!

@davereid re: #49, I don't think it makes sense to remove the "replace file" field as a part of this patch.

If we want to hide the "replace file" field, we should discuss it in a separate issue.

The patch from #43 is RTBC; here's a re-rolled version so it applies without fuzz.

Committed #60 to Git (with the missing js file from the patch as well): http://drupalcode.org/project/media.git/commit/1a0304d

Let's file followup issues as new issues.

Have different issues been filed for removing the "replace file" and the issue mentioned in #54? Are we following up on these?

Out of curiosity, if I prefer not having the modal popup after an image is uploaded. What is the best way to disable it?

Good point. I'd like to see the modal on by default, with some smart logic for popping up (e.g., don't pop up if there are no fields), and a configuration checkbox per file field to enable/disable.

@@ -100,6 +100,8 @@ Drupal.media.popups.mediaBrowser = function (onSelect, globalOptions, pluginOpti
diff --git a/js/plugins/media.views.js b/js/plugins/media.views.js
index aec2323..95b673b 100644
--- a/js/plugins/media.views.js
+++ b/js/plugins/media.views.jsundefined
@@ -32,6 +32,12 @@ Drupal.behaviors.mediaViews = {
       for (index in Drupal.settings.media.files) {
         if (Drupal.settings.media.files[index].fid == fid) {
           selectedFiles.push(Drupal.settings.media.files[index]);
+
+          // If multiple tabs contains the same file, it will be present in the
+          // files-array multiple times, so we break out early so we don't have
+          // it in the selectedFiles array multiple times.
+          // This would interfer with multi-selection, so...
+          break;
         }
       }

Can someone tell me how this works? I don't understand how this break is accomplishing the task the comment states.

I ran into this while trying to re-roll #1658452: Regression: Views Library doesn't support mutliselect, 1.x library did

Jackinloadup: The reason it's there is because Drupal.settings.media.files sometimes contain the file-entity multiple times (if the file is present in more than one tab), so the array can look like this

[
  {fid: 1, filename: ....},
  {fid: 2, filename: ....},
  {fid: 1, filename: ....}
]

So therefore, we break out of the loop going through the array as soon as we've found the first instance of the file. Otherwise we would add all instances of the file in the Drupal.settings.media.files to selectedFiles, which - if you have multi-selection enabled - ends up selecting the same file multiple times.

Was that a good enough explanation?

Yeah, it took a new day and that great explanation to realize where my thought process was wrong.

Goodbye and thanks for all the fish ^_^

fish = hard work

Out of curiosity, if I prefer not having the modal popup after an image is uploaded. What is the best way to disable it?

Not clear there's a way yet, but looks like an issue for that exists at #1823622: Provide a way to suppress the edit screen after file upload.

Just upgraded from 7.x-2.x-unstable7+6 to unstable7+38 (latest dev) and it appears this has been reverted/removed since then. When I upload a file it does not present a modal to enter additional information. It does work when I click the 'edit media' button though.

Issue is closed. No re-opening :-p

I have the same has Elijah Lynn, i can't make it working, either on unstable 7 or last dev...

I tested this on the latest dev version of media and file_entity and it seems to be working.