Closing Media Browser dialog with Escape causes orphaned HTML to hang around

Reviewed and tested in IE7 and solves the problem. Also solves the problem reported in #1826718: Bug report for media file browser upload form in IE browser (7,8)

Thanks. I've committed this. Good work. We need to port this back now.

A backport of #1.

Committed #5 to 7.x-1.x

This may not be the best thread to post on, but I believe it's related and should be addressed within the context of the discussion above.

We have been having trouble selecting more than one image in the media browser on a single page. Here's the case:

-- Add Image
-- Browse to 'My Files'
-- Choose a file
-- Click 'submit'
-- Dialog closes, image is added to the field as expected.

All works well - except if you then run jQuery('#mediaBrowser') in the console, it finds a match (which, I would expect to be empty).

Staying on the same page with no other updates:
-- Add Another Image
-- Browse to 'My Files'
-- Choose a file (same or different, doesn't matter)
-- Click 'submit'
-- Dialog does not close, but instead loads the homepage within the iframe. Inspecting the console shows:

Uncaught TypeError: Cannot call method 'click' of undefined
media.browser.js:81

The issue is that there are now two elements on the page with an id of 'mediaBrowser', but jQuery will only select the first one it finds for efficiency, as it is improper markup to have more than one element with the same id attribute.

It appears that the 'destroy' method implemented above is now leaving behind the entire #mediaBrowser iframe, so when the submit handler goes to retrieve the buttons, jQuery is selecting the hidden iframe and not the active iframe as it should.

Reading the destroy jQuery UI documentation carefully (http://api.jqueryui.com/dialog/#method-destroy) I'm reading 'This will return the element back to its pre-init state' as meaning that the dialog is removed completely, but the contents of the dialog are returned to the DOM, which then causes our problem.

Using the 'close' method instead appears to completely eliminate the dialog AND its contents, so the iframe is completely removed.

For our case, the included patch is what worked. We are using Media 7.x-2.x, jQuery 1.8 and jQuery UI 1.8.24 (via jQuery Update and this patch via this issue), and I'm wondering if we're experiencing the issue because of these later versions than the Drupal default. I'm not sure when jQuery implemented the singular id selector feature or if it has always been there, but it's possible that if jQuery is happy to select both '#mediaBrowser' elements, then the user will never notice the issue.

Patch media-browser-not-removing-iframe-1947094-8.patch works for me.

When testing this with a stock 7.22 instance, with the default version of jQuery, I do not see the original issue with the site loading in the iFrame.

However, when inspecting the HTML in the console, it is not removing the HTML properly. So perhaps it is a jQuery issue, as to why it works with stock drupal jQuery and not with jQuery 1.8.x

I tested the patch, and the patch does NOT remove the extra iFrame html from the bottom of the page when you are using a stock 7.22 install and the stock version of jQuery.

Just tested on a clean 7.22 install, added only required modules: ctools (1.3), views (3.7), media (2.0-unstable7+60-dev), file_entity (2.0-unstable7+93-dev). No jquery update. Just straight up Drupal 7.22 standard install.

Before patching: All appeared to be working, but iframes were stacking up in the DOM. It does appear that the stock jQuery 1.4 doesn't care about multiple ids on a page (which may be why the issue is largely unnoticed) -- there were several iframes and everything was working fine from the user's perspective.

After patching with #8: All appeared to still be working, and iframes were no longer stacking up in the DOM. Patch appears to work as designed.

@crazybutable, can you provide more information about your install? I'm curious why the patch didn't clear things up for you. A css/js cache flush is in order after applying the patch, otherwise your javascript may not be properly updated when re-attempting after patching.

Sure. I just set up the standard media_dev development profile. (https://drupal.org/project/media_dev)

I just tried it again, I verified that I have the patch properly applied, I restarted apache and mysql and flushed all caches and I am still seeing extra leftover iframes.

I guess I didn't mention it before but I am using Safari 6.0.4. Perhaps it is a browser issue?

I checked the versions of everything and I guess I was mistaken about it being a "stock 7.22 install". (Don't blame me, I was sitting next to Dave Reid and he approved my original comment as written. ;)

Core: 7.23-dev
Media 7.x-2.0-unstable7+56-dev
File Entity 7.x-2.0-unstable7+92-dev
CTools 7.x-1.3+4-dev
Views 7.x-3.7+10-dev

I tested it in Chrome and the patch DOES work. So there appears to be a problem with Safari.

I don't know the best approach is to take with this issue except perhaps to try to follow it up with another. Otherwise this looks good to me.

Using Safari 6.0.5 on OSX with jQuery 1.8/ui 1.8.24, this patch does seem to resolve the issue. I will throw together a clean build tomorrow and test-drive Safari with this patch to see if there's a workaround.

+1 for starting a new thread to follow-up with a browser-specific issue.

Committed to http://drupalcode.org/project/media.git/commit/6169934

Now we need to port this to 7.x-1.x

Taking up the cause

Patch from #8 will work when applied 'as is' to 1.x, but here is an explicit 1.x patch. only difference is line numbers due to 2.x additions from https://drupal.org/node/1881152

Committed to http://drupalcode.org/project/media.git/commit/54c0f28