Browser window opens twice for field image

This sounds like the same issue I posted in #2533312: Media field on Paragraphs item opens two overlapping browser modals. In my case the problem only seemed to appear when using a media field on a Paragraphs field item, but that doesn't seem to be the case here, so maybe this is a more general problem. Unfortunately, telling my users to just close the top browser window isn't a workable solution for me, so I won't be able to update my site until this is fixed. :-(

Yeah, I noticed that I could sometimes get it to work on one field after successfully selecting an image in a different field. It was all very weird.

I just closed #2533312: Media field on Paragraphs item opens two overlapping browser modals as a duplicate of this issue. I got there first, but this deals with the more general case and we don't need two bug reports kicking around for the same thing.

I'm also having this issue but mine is it stacks perfectly on top of each other. I just noticed it when I open then close the pop-up window then it shows my the first pop-up window.

I updated from 7.x-2.x alpha-4 to beta-1.

@leolando.tan, the same with my windows. I assume the top window in the screenshot here was dragged to the side before the screenshot was taken.

@jstoller Oh yeah! I just noticed now that it has the "draggable" title bar on the pop-up window. So far alpha-4 is working alright for the site. Just to add, I haven't tried doing some fixing that you guys did in the previous comments. :)

Just want to add that I am facing the same issue - with two perfectly overlapping windows. maybe it has to do with the enabled administration theme? In my case we use seven as admin theme

Ok... tried several themes, nothing changed... tested 7.x-2.0-alpha4 and the error disappears

Maybe the problem seems to be the jQuery version: I installed the jQuery Update Module and set the version to something larger than 1.4. Then the problem has gone away. If you set it back to 1.4, the window opens twice.

I tried to track down the problem: In media.js the event for media-browser-launch seems to get called twice:

$(selector, context).once('media-browser-launch', function () {
..
});

Configuration: Media 7.x-2.0-beta1, jQuery Update 7.x-3.0-alpha2

Changing to admin Jquery to 1.8 work for me.

My test case was:

- Multiple image field
- Duplicated window only happens on the second image you're uploading.

Changing to admin Jquery to 1.8 work for me.

My test case was:

- Multiple image field
- Duplicated window only happens on the second image you're uploading.

Same for me. Thnx PI_Ron for sharing solution!

Likewise encountered the issue, updating to jQuery v1.8 was also a suitable workaround.

Updating to jQuery 1.8 also worked for me.

I've reverted the commit which broke this for older versions of jQuery.

Hi, I'm reopening this issue as upgrading jQuery is workaround, not a solution :
I never use jQuery versions over 1.5 in admin because it breaks views UI, and I'm probably not alone in that case.

In js/media.js l. 19 :

$(selector, context).once('media-browser-launch', function (){

The var selector is an id string given by the function each(), and should refer to an unique object, but the requested id is used twice in the html dom, which should not be.
The id is used in a <div> for the .media-widget wrapper, which is what media.js is expecting to use, but it's also used in the upload form-text <input> field.

$(selector, context) returns an array instead of a unique object, and that's why the function is run twice.

These 2 identical id's are here because of that litte change in media.module l. 719 :

function media_element_process($element, &$form_state, $form) {
  ...
  // Append the '-upload' to the #id so the field label's 'for' attribute
  // corresponds with the textfield element.
  $original_id = $element['#id'];
  $element['#id'] .= '-upload';
  ...

As it's a right thing to have a label associated with a field, let's change the settings id returned to the js script, and the div wrapper id instead.

in media.module l. 825 :

function media_element_process($element, &$form_state, $form) {
  ...
// Add the media options to the page as JavaScript settings.
  $element['browse_button']['#attached']['js'] = array(
    array(
      'type' => 'setting',
-     'data' => array('media' => array('elements' => array('#' . $element['#id'] => $element['#media_options'])))
+    'data' => array('media' => array('elements' => array('#' . $original_id => $element['#media_options'])))
    )
  );
  ...

in media.fields.inc l. 392 :

function theme_media_widget($variables) {
  $element = $variables['element'];
+ $element_id = substr($element['#id'], 0, -7); // remove -update from the id
  $output = '';

  // The "form-media" class is required for proper Ajax functionality.
-  $output .= '<div id="' . $element['#id'] . '" class="media-widget form-media clearfix">';
+  $output .= '<div id="' . $element_id . '" class="media-widget form-media clearfix">';
  $output .= drupal_render_children($element);
  $output .= '</div>';

  return $output;
}

That's working fine for me.
Hope it helps !

If you use #16 solution and have EPSA Crop module enabled, you'll have to make it get the fid from the right element.

in modules/epsacrop/js/epsacrop-media?js l. 25 :

      // When someone clicks the link to manage EPSA crops
      epsaButton.bind('click', function (e) {
        e.preventDefault();
        var elem = $(this).parents('.media-widget');
-        var fileInfo = epsaDialogSettings[elem.attr('id')];
+        var fileInfo = epsaDialogSettings[elem.attr('id')+'-upload'];
        var fid = fidField.val();
        if(!fileInfo.fid || fileInfo.fid != fid) {

Hi folks,

The code in #16 is working for me. I've made a patch of it that you might include in the next release of the module.

The bug occurs in the latest stable release 7.x-2.0-beta1, but not in the latest -dev. It appears there is a not-so-subtle fix was committed:
http://cgit.drupalcode.org/media/commit/?id=cfb024205ef7682192b8357dc77a...

However, the media browser still produces invalid markup with duplicate ID's, as discovered by seren10pity13 in #16. Attached patch fixed the invalid markup and as a result, the mediabrowser window is only opened once.

I undid the reverted revert commit mentioned by Devin Carlson in #15.

This patch is largely the same as #18, but fixes some coding standards and includes the reverted revert commit mentioned in #15

I updated the failing test to use the new selector.

I've added another related issue, #2551363: Selectors in mediaElement behavior are too vague. That has a patch that solves this issue from the javascript side of things.

I've reviewed the patch. Code looks clean to me and after manual testing I can confirm the problem is solved.

Before applying the patch:

After applying the patch:

Thanks idebr!

Thanks for the patch !

I can confirm that this patch solves the issue.

Can also confirm that path from #24 solves the issue. Thanks a lot!

Some of these improvements have already made their way into our D8 code. I think I'd like to mirror the changes here for simplicity's sake.

Patch in #30 is not working for media 2.x on D7 here.

Patch #24 does work though, so I suggest using that patch ;-)

I had a similar experience as #31 until I cleared caches. At that point the browser began to work correctly! +1 for the patch from #30.

For me the problem is resolved with the patch from #30, i can choose a media in the library.
Now i have a problem with epsacrop.

Thanks.

I can confirm that #30 fixes this for me as well. Only one window pops up now. Running stock jQuery (1.4) from Drupal Core.

Thanks!

Here is a reroll of #24 which is working for me. It looks like the JavaScript change already got in elsewhere.

The patch from #35 is working for me on a basic install with the 2.0-beta1 release.

#35 works for me too. 2.0-beta1. Thanks!

Manually applied patch on 2.0-beta1 - another confirmation that it works.

@nbz There is currently two patches here. #35 and #30 Witch one did you test? The patch is supposed to be applied to dev. so we must test against this, not latest stable.

+1
which patch should we use? I think the patch #35 should be more compatible with other modules ...(epsacrop ....)

From my testing, the patch #30 works on the latest dev release, but won't apply to stable (2.0-beta1).

The patch from #35 is working for me on both the latest dev and stable.

However, it seems like Devin wants to mirror what's going on in D8 in #30, so that should probably be the patch that is tested/RTBC'd.

+1 from me on #30.

I tested number patch in comment #35

@nbz could you also take #30 for a spin?

Patch in #35 applied for me in 2.0-beta1 (make sure you're using patch -p1 ?), and it works (I cleared the caches right after applying).

patch -p1 < ../../patches/media-browser_opens_twice-2534724-35.patch
patching file includes/media.fields.inc
patching file media.module
patching file tests/media.test

; Information added by Drupal.org packaging script on 2015-07-14
version = "7.x-2.0-beta1"
core = "7.x"
project = "media"
datestamp = "1436895542"

As stated before, we should be testing #30 against 7.x-2.x-dev. If that applies and solves the problem, report it here.

Ive just applied #30 to media 7.x-2.x branch. It works.

#30 worked for me as well! Thx

Using #35 with success...RTBC based on all similar reports!

I had the same problem when using a field collection with a media browser inside an other field collection.
I've applied patch #35 to "7.x-2.0-beta1" and it seems to fix it, I still get a JS error after submitting the media browser overlay form:

Uncaught TypeError: Cannot read property 'node_form' of undefined

But it doesn't seem to cause any problems (so far i've tested)

Patch #35 worked for me with 7.x-2.0-beta1. Thanks!

I have had problems with the patch in #35. If you have a custom form with a media field, it will stop working with this patch. The reason is that it uses another theme function in which the id is used. Note that it was this issue that caused all this trouble: #1734716. I have created a new patch that removes -upload from the theme function in media.module.

@dagomar,

could you add an interdiff?

Please use #30, not #35. Apply it to HEAD. That is where the patch should work, not against latest stable.

Hello,

Patch #30 ? Patch #35 ?
if the recommended patch is the #30, do you think include this fix in the dev version? So, the question is closed.
For me the patch #35 is more useful with associated modules to media...(module for crop...)

@kumkum29 apply #30 against latest dev.

#30 +1

#53 +1

Installing the latest dev version of the media module worked for me. Seems like the patch has been applied to that version (7.x-2.x-dev 2015-Oct-01)

I have the "You have not selected anything!" problem. Using just Panels + fieldable_panels_panes. One "Image" field with "Media browser" widget.
Using latest stable without patches of:

Drupal Core 7.41
Panels 7.x-3.5
FPP 7.x-1.7
Views 7.x-3.11
Media 7.x-2.0-beta1
File Entity 7.x-2.0-beta2
Ctools 7.x-1.9

Tried #53, and it did fix the problem.

#53 + 1

Thanks!

Works for me! Thanks!

RTBC+1! We're now using this patch in Panopoly.

@dsnopek, which of the patches is Panopoly using?

@legolasbo: Panopoly is using #53

Is there an active module maintainer willing to apply the successfully tested patch?
This is really a mad bug happening in many of our projects. A final fix would help us a lot. Thank you for your great work!

Thanks, #30 on current 2.x-dev works beautifully!

I confirm that #53 indeed fixes the problem that 'nothing was selected'.

I get the feeling this issue has drifted quite a bit since it's initial inception. Perhaps someone who understands what's going on here could update the issue summary, noting the two working patches (#30 and #53), and describing their different approaches? Then maybe we could get a module maintainer to look at this and make the final call. Just a suggestion.

I agree, we should be using #30 and NOT #35 or #53.

#35 and #53 are work arounds for a bug that resides in media.js.
In theme_media_widget(), It truncates “-upload” from $element[‘#id’] to make it work with code in media.js. In reality though, it’s a flaw with the loop/selection logic in media.js.

#30 Updates the logic of media.js to fix the loop/selection logic. It removes the dependence on #element[‘#id’] and adds appropriate attach and detach functions. This was written as part of the D8 release and we should use this as the fix.

Re-uploaded the patch from #30 to prevent people that have not fully read the issue from using and commenting on the wrong patch. I've also hidden any non relevant files.

tested #72 here and it fixes the double window and https://www.drupal.org/node/2111695 in the context of using media + inline_entity_form

Tested #72 and it seems to work for me.

#35 worked for me. Thank you!

I think it's time to apply the patch to the dev branch soon and if possible create a new stable release together with other important patches?

#53 works great along with beta1 for me. Requires a cache clear though!

This is getting to be a little ridiculous. If everyone keeps testing different patches this thing is never going to be committed. @web506 and @stefan.r, did you also test the patch from #72 and find that didn't work? Or did you just test #35/#53? Can you test #72? It seems like the prevailing wisdom is that #72 is the way forward.

Can we get a maintainer to please decide which approach they want? At this point they've all be tested. :-/

#72 worked for me. Thanks for the great job !

#72 also worked for me.

#53 works with beta1! Thanks

@maximegaul did you also test #72? That is the patch currently being investigated here. Is there a problem with #72 that caused you to use #53?

@jstoller it's just that some of us have to use tagged releases... and #72 doesn't apply to beta1 that's all :)

Applied #72 manual to media 2.0-Beta1 and it works fine.

@stefan.r Why should #72 not apply to beta1 ?

Manually applying #72 to beta1 worked for me.

Here should be a version that applies to 7.x-2.0-beta1. Meanwhile I also committed this to 7.x-2.x.

Hi Dave,

Does it matter that Object.keys and forEach are not supported by IE 8? This patch makes media unusable on IE8; the attach button raises an error "Object doesn't support this property or method". Here's a screenshot

Regards,
Jeff

Regarding Object.keys() issue and forEach on IE8 reported by @loopduplicate

I haven't tested it but this snippet should fix above issue

Find:

elements = settings.media.elements;
Object.keys(elements).forEach(initMediaBrowser);

replace with:

var elements_ids = [];
for(var key in settings.media.elements){
    elements_ids.push(key);
}
for (var i = 0; i < elements_ids.length; i++) {
  var element_id = elements_ids[i];
  initMediaBrowser(element_id);
}

REF: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...

Confirming issue resolved in 7.x-2.x-dev version. Thanks!

Updating media module using the latest epsacrop on a File field breaks the 'Manage image crops' button. See also #2608818.

Ah, I see that this is related to the problem mentioned in #53 (the difference between the two patch approaches)

Note that the backport for beta-1 at #88 breaks the 'Remove' functionality. After browsing and adding an item, clicking the 'Remove' button will leave you with an text field and an 'Attach' button.

The patches in #30 and #72 solve the issue at hand, but create a problem when the form is rebuilt (cfr. #93).
I suspect this is due to the removal of the id. Please solve this before a new release, as this behaviour will break the release.
The patch in #35 does not have this problem.

Here is a patch regarding which fixes the bug mentioned in #94. It's more or less a reroll of #35 but for the current code base.

fixing tests

The changes committed in #87 break the upload widget as the element components are not siblings but children.

Hi,

For the bug with form api which seems introduced by this patch, the problem is caused by something other.

I do an other issue and a patch.

Please, review my work.

https://www.drupal.org/node/2705001

Thank you.

The Patch in #100 worked for me RTBC

Minor change to patch #96 so that the AJAX will convert the button from Attach to Browse.

I'm assuming the change to the tests in the above patch is a mistake, because the old version of the tests works fine with the patched code. Here's a version of 103 without the change to tests.

Using latest dev (patch by Dave Reid) solved the problem for me.

@here patch #24 works for against 7.x-2.x applies and solves the problem

I think there is some confusion here because a fix has already been committed to dev, but it seems like there were related problems found afterwards. I'm seeing patches posted, since the dev commit in #88, that are completely different from each other. Would it make sense to split the newly found problems off into separate issues, so that we know what problem each patch is supposed to be fixing?

I was thinking the same thing, this is getting too confusing. Quickly scanning the issue since the commit suggests that there are three issues being discusses, first (since the commit) reported in #89, #92, #94, respectively.

I'm using the patch from #100 which fixes an apparent issue introduced from the commit to dev. It's working fine. I move that we can RTBC that patch.

@oranges13, could you start a new issue that describes what the problem/fix is, and attach that patch to it?

@brockfanning I'm not sure to what you're referring. The patch in #100 fixes this issue as described for me (the media popup opening twice)

@oranges13, just trying to help get things committed. If it's not clear to the module maintainers what's what, this will probably stagnate. You say #100 works, which is great, but there are other patches before and after #100 for seemingly different problems, including one that was even committed. Just my 2 cents, but it seems like a new issue would be the best way to make things clear for the maintainers. A description of how to recreate the problem would probably be helpful too. (I'm curious why I haven't encountered this problem.)

I'm pretty sure the original issue was fixed long ago. At least in dev. Then there were questions about how it was fixed, and tangential issues came up, and the whole thing got very confusing.

So the next time: followup issue == New issue - with a reference back to the original issue. Thanks! (prevent chaos)

Proposal: people handling these additional issues: open up a new issue, reference this issue, so we can close this one. (also for future issues we will work on, i think it's much better if we isolate these, so when we are searching for the cause of something, we can look up the history a lil bit easier - and quicker!)

Just did a few tests to confirm what's going on. To clarify, I am using Media on a _production_ site, so I needed production level code. I did not use the dev versions on this site because the "beta" version was the current stable release.

When I made my comment on July 13, beta1 still had the double popup bug (see the screenshots on comment #26 for what that looks like).

My normal practice is to use the most up to date patch at the time, which was #100 when I got to this thread. The patch in #100 fixes the issue described in this thread for the version that I was using (beta1). I just confirmed this by using simplytest.me. If you use media-2.x-beta1 this bug is still present. The patch in #100 fixes that.

Comments after #88 (the fix which was committed) seem to indicate that it causes other problems or is incompatible with IE8. To test this, I just spun up a simplytest.me site with beta1 and the patch from #88 applied and tested it with IE8. I confirm that the patch in #88 DOES NOT WORK with IE8.

Media beta2 does not work with IE8 either (which I'm assuming contains the committed fix from #88).

Media beta1 with the patch from #100 DOES WORK with IE8.

Not that I use IE8 that frequently, but it's possible that others might. So the patch in #100 actually fixes this issue. The one that was committed only partially fixes it.

Thank you very very much for your investigations @oranges13. This way we should get things fixed and this issue finally resolved.
I guess it would be best if @DaveReid will have a look at this again (from #86) and should decide how to proceed best. We could create an interdiff from #86 and #100 or perhaps the commit from #86 can be rolled back and the fix from #100 can be applied instead?

I'd suggest to create a new beta3 (EDIT: I wrote beta2 before (typo)) release then with both fixes included to finally fix it. Thank you all very much. Not let's wait for a maintainers decision how to go on...

"I'd suggest to create a new beta2 release ... "

Not possible. The next release will be beta3. (thats just how it works)

"The one that was committed only partially fixes it."

Then it's still a followup. (might have fixed it 100% back then, something might have changed) (it didnt, lots has changed, and the original patch did not fix it fully, but still).

@oranges13 I've also tested a bit yesterday, i can confirm "Media beta2 does not work with IE8 either (which I'm assuming contains the committed fix from #88).", so it's actually still (partially) broken. Will test the rest later this week (if no one beats me to it).

Wonder about other TODOs for this issue.

@oranges13: I'd suggest posting a new issue specifically to address "Media broken in IE8." You can reference this issue as the parent issue, for historical context. Then post a patch against the dev release that undoes the committed patch from #72 and replaces it with the fix in #100.

Continuing to post patches against an older beta release, in a "fixed" issue with an already committed patch, isn't going to move anything forward. This thread outlived its usefulness six months ago.

Yes, this is very confusing.

Surely, any meaningful testing has to be done against the latest 2.x-dev code to allow things to move forward.

From what I can see, after the commit on 2016-Feb-15 the following three issues emerged:

1. Object.keys and forEach are not supported by IE 8
Comment #89 with a proposed fix in #90, that has NOT been rolled into a patch or tested.

2. changes committed in #87 break upload widget as the element components are not siblings but children.
Comment# 100 with patch, two lines.

3. The 'Remove' functionality broken
Comments# 93 reports, After browsing and adding an item, clicking the 'Remove' button will leave you with an text field and an 'Attach' button.
I cannot determine what the status of this is.

These can each be made into separate issues, especially #3.
Unless @DaveReid would suggest a different approach.

I'm finally fed up with this issue (xD)... so here are my manual testing results. Let's get this fixed and create a final patch...
#95 is broken
#97 changes tests which were correct
#100 does NOT work. The attach button is broken completely like in #116
#103 tests are broken

#105 works in tests, in IE8 and ALL other cases I could test manually! So the older patches are no more required.
#105 is against LATEST DEV. So that's the patch which should be tested as RTBC.

After all this discussion I'll now finally hide all patches except #105 and set this issue RTBC for #105 to take things forward.
It would be very very cool if we could get some more feedback on #105, please get it commited, create a new beta3 of it and create a new issue, if further problems appear.

Thank you all very much for this much too long discussion :P ;) :)

PS: I can confirm #105 also works in combination with #951004: Allow selecting of multiple media items for a multi value media field in the same dialog (#197).
All previous issues in combination are gone now. WHAO :)

Patch #105 no longer passes testing.

says #original_id is undefined
this is related to the patch.

I'm not sure if this patch is necessary any longer. I haven't noticed this issue lately and I've been using 'media' a lot lately.

@joseph.olstad I just retested without this patch, and it appears the problem is no longer happening! The old steps to reproduce that I had were to setup a content type with a multi-value file field using the media browser widget, and the 2nd time you clicked "Browse" it would open the dialog twice. But not happening for me with the latest 2.x-dev

Thanks @dsnopek
marking this as 'works as designed'
for anyone still observing this , please upgrade to media 7.x-2.0-rc3 or newer