Overlay breaks the Update manager workflow

Confirmed...

This will be a quick fix for chasey if he is back.

Some related problems with overlay + Update manager were reported in the duplicate #727720: Incomplete information display with overlay (on 'Authorize file system changes').

It'd be really nice to completely disable the overlay during the update manager. I don't know what the state of the overlay is these days, and how easy it is to control this sort of thing, but all sorts of problems have surfaced trying to get the overlay to play nicely with various parts of the update manager workflow. Instead of spending lots of developer time fixing each one (anyone who claims overlay hasn't cost a lot of developer hours is lying) I'd rather just leave the overlay once you start the update manager workflow. It's already a multi-page wizard-like workflow, and it'll be impossible to stay in the overlay the whole time, so we might as well just start the update manager as a fresh page load, with no illusions that we're keeping your context. Updating your site isn't something you casually do in the middle of other stuff. It's a stand-alone administrative operation, so it really has no place inside an overlay. The page you were on when you started might change or even disappear as a result of updating your site...

.

As much as I agree that it sucks up way to much developer time, it confuses users if we decide some modules do and others don't use the overlay

@dww:

It's already a multi-page wizard-like workflow, and it'll be impossible to stay in the overlay the whole time

Can you explain why that's an impossibility?

No, I have no interest in spending another 2 minutes working around overlay for the Update manager. I have real bugs to fix. If Bojhan thinks it's too confusing to disable the overlay, then y'all can find a developer willing to try to make it work. That's not me. ;) Good luck -- I genuinely hope you succeed.

p.s. Regardless of everything else, we simply CANNOT be in the overlay once we hit authorize.php. At that point, you've left "Drupal" -- it's a stand-alone script. It'd be like trying to run update.php inside the overlay. No thanks. Given the resources we have on hand, my "professional" advice is what I wrote above. If you want to ignore that, feel free, but don't expect me to spend my time helping out here. ;)

@dww I am not ignoring any of your comments, to the contrary - I am trying to make very concise decisions. I agree very strongly, that it would be a bad idea to run Update Manager in overlay when it could break during install (authorize.php being one of them). But your description in #4 did not make it clear to me that is happening here. What to me, at least the issue states does not affect authorize.php nor does it create a impossibility (as is stated, its a quick css/js). If you say, it is a impossibility - I am obviously inclined to go with that.

My comment about impossibility is that the update manager workflow always goes through authorize.php. Therefore, it's always going to hit a page that can't be in the overlay. So, we can't maintain an overlay through the whole thing. So, if we're going to leave overlay anyway, why not leave it from the start so that we don't waste yet more precious time working around CSS/JS problems in overlay on the preparatory steps of the update manager workflow?

Alright then, #11 makes sense to me. Also bumping to critical since we obviously can't ship like this. Not sure if this still belongs in overlay.module or not.

So where is everywhere? I do not really see it needed to disable it on pages such as this initial add url or zip page? I hope you understand I want to limit the amount of outer overlay experiences to a minimum

#687594: Overlay and authorize.php don't play nicely together is duplicate but has a more complete description of the bug. Currently, in some workflows authorize.php gets shown in the overlay as described above, but in others, it already does take you out of it (then creating the usability issue of how you get back in...)

Are we sure there is no way to keep someone in the overlay throughout? As @dww says, authorize.php is essentially "not Drupal", but then again, who says it's not possible to actually display a completely external webpage in the overlay in the first place? I don't see why it's definitely impossible. Almost all the JavaScript that the overlay needs to work lives in the parent window anyway. There are very few lines of JavaScript that are loaded in the child window at all, and most of that isn't relevant for something like authorize.php (or update.php) anyway, so could be skipped. I think the main thing is really just the code that attaches a click handler to the links. Maybe there is some way to have a substitute for that in the parent window, in cases where it's not present in the child window?

Marked #749140: Ready to update bug in the ajax overlay as a duplicate of this issue.

As @dww says, authorize.php is essentially "not Drupal", but then again, who says it's not possible to actually display a completely external webpage in the overlay in the first place? I don't see why it's definitely impossible.

Now I'm not 100% on this and will be glad to be corrected, but I think the reason for this being impossible is that the authorize.php as well as update.php are designed to be 'unbreakable'. That is, if you install a completely broken module that kills Drupal you can still run authorize.php and roll that install back or somehow get out of the mess (I'm not sure if Update manager can do rollbacks currently, but I'm pretty sure that conceptionally that's the idea, that you aren't left with a broken Drupal and that's it). Therefore, if you load authorize.php in the Overlay, you will still have to bootstrap Drupal, which in turn could be broken.
I don't know if it is possible or worth it, but maybe authorize.php could be made to look like the overlay without actually bootstrapping Drupal. That way you would land at example.com/authorize.php#overlay or something and authorize.php would include overlay's .js files and all so that you think you are in Drupal. But again, could be complete overkill in terms of benefit / developer time. Also I don't know how you would get that 'background' that shows whatever site you were on before entering the overlay, that you get with the 'real' overlay.

Also a short note concerning:

Are we sure there is no way to keep someone in the overlay throughout?

Actually there is. And that is (I think) if your webserver is configured in a way that the apache user is the same as the ftp user. Or something like that. Anyway, that's how it is on my shared hosting. I just type in the URL of the tarball on d.o and voila, it's on my server.

@tstoeckler, my point is that if we treat it like an external webpage, then we obviously aren't relying on the Drupal bootstrap at all, since external webpages might not even be running Drupal :) And as shown by your second point (see also #14) authorize.php already does display in the overlay under some conditions - it just doesn't work 100% correctly.

Again, most of the work is done by the parent window already, and that is a totally separate webpage that is already loaded in the browser - just running JavaScript. It doesn't matter that it originally resulted from a Drupal bootstrap. We only care about avoiding Drupal bootstraps in the child window, since that is the one that is being reloaded during this process.

Also note that at the moment, Drupal Gardens has something working where a Drupal 6 site can be displayed inside the Drupal 7 overlay... it's pretty cool that it works :) Gábor Hojtsy implemented this - I'll ping him and see if he has any insight here.

If a local page (ie. same domain) is displayed in the overlay, then just including the overlay-child.js file in the overlay should allow it to interact properly with the parent window AFAIS. The remote Drupal 6 site displaying overlay content to Drupal 7 site needed some cross-domain iframe communication, since it could not use the overlay-child.js code due to cross-domain restrictions in browsers.

This is still happening as of Apr. 2 CVS HEAD: http://skitch.com/evandonovan/n7d3i/authorize-changes.

Is there a patch yet?

Possibly this would be another reason not to show the administer modules/install modules section of the site in the Overlay, as I suggested in #760944: Don't show admin/modules & admin/build/block in overlay (for Performance & Usability). Bojhan thinks that would cause usability problems, though, because of context switching.

janusman: Maybe try rolling the patch off a CVS checkout of Drupal HEAD & then run cvs diff -up? That seems to be the safest route.

#21: authorize-overlay-679190-21.patch queued for re-testing.

this is a strange comment:

"// Add the overlay support JS"

o_O

--- authorize.php	Mon Apr 05 14:43:52 2010
+++ authorize.php	Mon Apr 05 14:39:41 2010
@@ -121,6 +121,9 @@
 
   // See if we've run the operation and need to display a report.
   if (isset($_SESSION['authorize_results']) && $results = $_SESSION['authorize_results']) {
+
+    // Add the overlay support JS
+    drupal_add_js("modules/overlay/overlay-child.js");    
     
     // Clear the session out.
     unset($_SESSION['authorize_results']);

This use drupal_add_library('overlay', 'child') instead. It seems like it's something that the Overlay itself should be doing instead of authorize.php.....

drupal_add_library() doesn't work in authorize.php, so we have to use drupal_add_js(). We also have to add jQuery UI core, so we get the :tabbable selector, which is required in Drupal.overlay.bindChild. I'm attaching a new version of the patch that should work.

Unfortunately I don't think this approach -- adding a drupal_add_js() call to authorize.php -- is acceptable to dww. I'm going to work on calling Drupal.overlay.bindChild from the parent window instead of the child window, which will let us use the overlay not only for authorize.php but also for update.php, without adding any JS.

Oops, patch attached this time.

Should the patch be reviewed?

I think 'S'he left this needs work cause it's not a nice solution as he pointed out in #29

Ok, I just wanted to set to "needs review" so the testbot could test her patch. :) Changing back.

Just a note that #668640: Overlay shouldn't be based on jQuery UI Dialog (or the specific patch #655492: Clearing cache and hitting the back button in the overlay gives a 404 error will break authorize.php out of the overlay, which is essentially correct behavior as authorize.php doesn't contain the overlay-child.js.

So if we want authorize.php to stay inside the overlay it needs overlay-child.js to be added; Something like #30.

Screenshot 2 shows the problem, it shouldn't display the gray ish bar with Authorize file system changes, its misaligned ect.

Personally, I don't think it's "safe" to close this issue until the overlay is not used in the update manager workflow. We don't try to run update.php in the overlay (nor should we). I fail to see why the update manager should be otherwise. But, I will certainly be overruled and ignored on this point, so I'm not going to keep fighting that. However, no, I don't think it's safe to close this issue until the other issue is committed and confirmed to at least work-around the problem here. Thanks.

In preparation for the new "major" issue priority, I'm tagging this (not actually critical, but still important) issue as priority-major.
See: http://drupal.org/node/669048#comment-3019824 (#669048: We should have four priorities not three (introduce a new 'major' priority)) for more information about the coming major issue priority.

Sorry, no. This is in fact critical. We can't ship 7.0 if overlay and update manager are both enabled by default and the update manager fails inside the overlay.

@dww Is right, lets just move on here the solution seemed 99% working.

I just committed #668640: Overlay shouldn't be based on jQuery UI Dialog, and I believe that it addresses this issue. Or at least Overlay and Update Manager is working basically fine for me now, and the issue reported in the OP (which I've also hit when giving D7 demos ... oops! ;)) is no longer happening. Yay!

The only weirdness is if I'm in the overlay, click "Install modules", it plops me out of Overlay to complete the task, and then when I click "Enable modules in the Views package" or whatever, it pops me back to the admin panel sans-overlay, which is not how I left it.

However, I'm pretty sure this is either a separate issue, or in any case a non-critical one.

I think it's worth just leaving this open but lowering the priority: The issue title is still somewhat accurate, all the history is here, and the latest patch posted here seems to (mostly?) solve the remaining problem...

However, I'm not really sure that adding overlay-specific code to authorize.php is really going to fly as a solution :)

David, this issued is fixed due to: #668640: Overlay shouldn't be based on jQuery UI Dialog
There is NO overlay code in authorize.php :)

So it IS fixed :D

As per #42 this is obviously not fixed. Fairly different issue though.

"However, I'm pretty sure this is either a separate issue, or in any case a non-critical one."

==> #821248: Installing a module or running updates from the overlay makes you leave the overlay but doesn't bring you back.