Enabling 'use overlay' setting on profile page does not immediately use overlay for admin pages

This seems to work for me.

We had code for it already (for the case where you turn on the overlay module itself on the Modules page, and consequently jump right into the overlay), just weren't using it here.

The patch in #1 appears to result in the desired behaviour, but there also appears to be one small bug. When you are on the profile page, when you first enable "Use the overlay for administrative pages" it correctly displays the overlay, but if you then dismiss the overlay with the X at the top right of the overlay, it redirects back to the front page of the site, rather than back to the profile page.

This bug is quite specific, as it only does this if you have just enabled the overlay, and not if you simply view an already-enabled overlay by clicking on the edit tab of a profile where the overlay is already active and then dismissing the overlay... in that circumstance, it does correctly return to the profile page.

Confirmed this behaviour only appears after this patch is applied, and not in the current HEAD.

Attached patch modifies patch in #1, so that overlay module checks when it has just been activated, and if the current page was the user profile page, it places the overlay over the user profile, rather than the front page. This specifically checks for just this one profile page location, but it would not appear a more generic solution is required since there are not an undefined number of locations in the interface where the overlay can be enabled and disabled anyway.

I guess this needs a test, to ensure that the page does indeed redirect to this location when the overlay is activated from the user profile page, and not if it is activated from elsewhere...

Huh... there are no tests *whatsoever* for anything to do with the Overlay module?

Hmm, looks like Contextual, Toolbar and Overlay are the only core modules that don't have tests, or at least, not tests that reside in a modulename.test file inside the module folder... So if a test as per #4 is created, guess it won't be uploaded as a standard patch file then... :)

+	  if ($current_path =  'user/' . $user->uid) {
+	 	drupal_goto('user/' . $user->uid, array('fragment' => 'overlay=' . $current_path . '/edit'));

Right now this code is running on other pages also, because of the single equal sign - obviously needs to be a double equal sign :) And in that case I don't see how it would run at all... we are never on the "user/$uid" page when the overlay has just been enabled, are we?

Because of that, I don't fully understand why you think the previous behavior of using the front page (just like we do for admin/modules or anywhere else) is a bug. I guess you are saying that because people are very likely to have gotten to "user/$uid/edit" from "user/$uid" in the first place, it is likely that's what they'd expect to find underneath the overlay when they close it? I guess that might be true. If going ahead with this, I think we'd probably rather use $_SESSION['overlay_enable_redirect'] itself to pass along the desired 'underneath the overlay' page though, rather than searching for paths elsewhere.

You can't test JavaScript, which is most of what the overlay is, but there are probably some things that could be tested. This could be one of them, assuming we can assert the presence of a certain fragment in the URL after a page has been submitted (not sure if that's possible with the testing framework). Note that it's possible to add new files to a patch using "cvsdo" or other methods: http://drupal.org/patch/create

Well, it's obvious that late night why-don't-I-write-a-patch coding is not to be trusted with me! Ah well. The attached corrects both of the fairly obvious errors above... with a == where it should be, and changing both paths so that it reads:

+	  if ($current_path == 'user/' . $user->uid . '/edit') {
+	 drupal_goto('user/' . $user->uid, array('fragment' => 'overlay=' . $current_path));

...which gives us what we were actually looking for.

I would argue strongly that using the front page is a bug. Every other time you activate and then close the overlay on the profile edit page, it returns to the "view" of the profile page. On this one occasion, however, when you have first activated the overlay on the profile page, closing it returns you instead to the front page for no apparent reason. I can't see why we'd want it to do something different this first time. When you are on an edit tab, and click save, the expected behaviour is to return to view. This patch does that, in this special instance.

I'm trying to figure out where the other contexts are where the overlay would have been activated for the first time, since I've not been tracking the overlay development v. closely. Where else in the interface might it be reactivated?

Hmm, will see if I can get some advice on those testing issues.

That's odd... status ignored on that patch. Trying again.

Renaming...

Or could just tag needs review. D'oh.

I still don't think we should hardcode a list of paths. What about other modules that might want to control the overlay as well and make use of this functionality?

Something like the attached is what I was thinking...

Subscribe. Found this via #896364-184: Screen reader users and some keyboard only users need a clear, quick way to disable the overlay, so want to keep up with its status.

I'm not able to test the patch in #11 at present as I'm away from my development machine but the logic appears sound. However, while implementing a generalized solution is more elegant, wouldn't #11 be considered an API change, whereas the patch in #9 is simply a bug-fix for the one location in core other than the modules page where the overlay can be enabled, to fix what is otherwise unexpected behaviour. While hard-coding for that location may not be all that pretty, it is arguably a more pure "bug fix" and it would seem to me it would be a (minor) crime to have this end up deferred to D8 because it was an API change, when a simple bug fix could at least be committed?

I don't see it as an API change because the old usage of the API should still work fine - I was careful to preserve it for that reason. It just adds something small on top of that.

I don't think it's a good idea for one module to make assumptions about other modules' URLs, at least when we can help it... It makes the code harder to maintain, and less reusable in general.

#11 looks great to me, and IMO is not an API change, and is a very reasonable, well-contained but extensible solution to the problem. My only concern is:

+++ modules/overlay/overlay.module	11 Oct 2010 06:02:42 -0000
@@ -91,6 +92,21 @@ function overlay_user_presave(&$edit, $a
+  if ($user->uid == $form_state['user']->uid && !empty($form_state['values']['overlay']) && empty($form_state['user']->data['overlay'])) {

The last part seems very fragile. We're relying on $form_state['user'] NOT having been already updated by the earlier submit handler, user_profile_form_submit(), to include the new setting. At the very least we need a test for this (can simpletest handle session data?) in case user_profile_form_submit() is changed in the future to update the form's user object. At best, this could be solved without relying on this WTF. Perhaps by ensuring that overlay_control_submit() runs before user_profile_form_submit()?

Powered by Dreditor.

Some options related to #15:
1) Changing $form['#submit'][] = 'overlay_control_submit'; to array_unshift($form['#submit'], 'overlay_control_submit');.
2) Check $form['overlay_control']['overlay']['#default_value'] instead of $form_state['user']->data['overlay'].
3) Have overlay_form_user_profile_form_alter() add the "pre-submit" state of the setting to $form_state. E.g., $form_state['overlay_enabled_for_user'].

I'm not sure which of the above makes the most sense. I'd be fine with any of them.

Unrelated to #15, can we also change the name of overlay_control_submit() to something like overlay_user_profile_form_submit()? At first, I thought this was meant to be a reusable submit handler for other forms, but I realize it's pretty tightly coupled to the user profile form, so I think the name should convey that.

This patch addresses the comments in #16. I renamed the submit handler to overlay_user_profile_form_submit() and went with option 3 (storing the current overlay status in $form_state) to address the other feedback.

#17: overlay-931730-17.patch queued for re-testing.

Determining if an old patch is still relevant, drupal office hours http://core.drupalofficehours.org/task/955
Re-roll of the previous patch

#20: overlay-931730-17.patch queued for re-testing.

#20: overlay-931730-17.patch queued for re-testing.

Re-roll.

#27: overlay-931730-27.patch queued for re-testing.

#27: overlay-931730-27.patch queued for re-testing.

Overlay is dead to D8 #2088121: Remove Overlay.