Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Steps to reproduce:
1. Click on "Structure" in the toolbar to open the overlay, then "Blocks".
2. Configure the Search form to move from the left sidebar to the right sidebar in Garland.
3. Close the overlay.
The Search form is still in the left sidebar until you manually reload the page.
This is similar to (but a bit different than) #655614: Changes made by submitting a form in the overlay are not reflected when the window automatically closes which happens when the overlay closes itself (e.g., if you got to the form via a contextual link). The fix there doesn't really work here, unless we're willing to do a reload of the parent window every time the overlay closes for any reason.
*******
One interesting and fun way to fix this would be to go a step further and try to update the parent window dynamically before the overlay even closes. Currently, we have code to do that with the toolbar as well as anything else in an "overlay supplemental region" (e.g., if you add a shortcut while in the overlay, the javascript is smart enough to know that it needs to do an AJAX refresh of the toolbar to pick up the change). So if we could extend that same code to all regions of the page, then any time you do something within the overlay that affects any part of the parent window (e.g., move a block as in the above example), the relevant portions of the parent window underneath will immediately do a dynamic AJAX refresh.
The attached patch is a start at that, and it almost works, but this might be too complicated to do correctly:
1. The parent window and overlay child window usually have different themes, so we need to do all our rendering checks in the correct theme. So currently, the patch only works at all if you use the same theme in both places (i..e, disable your admin theme). To fix this, we'd probably have to find a way for the parent window to tell the PHP code that runs in the child window its URL (a $_GET parameter?). Then we'd do our checks by rendering the menu callback for that URL.
2. There will be weirdnesses and false positives associated with this (e.g., form IDs change each time a form is re-rendered, code that relies on current_path(), etc). They might be too much to overcome.
On the other hand, all of this stuff only ever gets triggered when a form is submitted in the overlay or a link with a 'token' parameter is clicked (see overlay_overlay_child_initialize()). So we could maybe consider forcing the entire parent window to rerender itself via AJAX every time that happens, without doing any checks first. Most modern browsers do that pretty seamlessly, so you wouldn't even know it was happening. And it would allow a fair amount of complicated code to be removed.
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | overlay-page-refresh.patch | 6.93 KB | bleen |
| #27 | overlay-page-refresh.patch | 6.68 KB | Gábor Hojtsy |
| #24 | overlay-refreshpage.patch | 6.17 KB | casey |
| #21 | 655722-overlay-refresh-on-close.patch | 5.53 KB | ksenzee |
| overlay-parent-window-dynamic-update.patch | 7.28 KB | David_Rothstein | |
Comments
Comment #1
eigentor CreditAttribution: eigentor commented+1 for reloading the parent window to avoid complication.
Does your patch do this? Set to needs review so it gets tested by bot and more participation.
Comment #2
casey CreditAttribution: casey commentedAnother reason to check full (or at least more than we currently do) parent window for changes: #697918: Update theme when default theme changes.
Comment #3
Jacinesubscribe.
Comment #4
casey CreditAttribution: casey commentedAnother one: #718910: Disabling the toolbar module from inside the overlay doesn't remove the toolbar until the next page load
Comment #5
eigentor CreditAttribution: eigentor commentedShouldn't this considered to be critical? For I do not see Drupal can be shipped with this bug... (asking before doing it because might get hit on the head for making the critical issue list longer)
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedMy original patch in this issue would be really cool to make work, but it's got some serious hurdles to overcome.
We'd need to figure out how to deal with the parent and child windows using different themes, as well as how to dynamically replace the main content of the parent page without blowing away the overlay itself - I wonder if that might be tricky? :) Also, if we allow too many regions to be dynamically updated, that could affect performance since all of them need to be checked. This only happens on extremely rare page loads anyway, so it shouldn't actually affect performance much, but people are already starting to grumble about this now when we essentially only check the toolbar...
So, we might consider a simpler solution here. Maybe we could keep track in a $_SESSION variable if the user has submitted any form (or performed any GET update with a token) during the current overlay session, and if they have, we could automatically reload the parent window next time they close the overlay? That way we'd force a reload on overlay close only when we have reason to suspect that something in the parent window might actually have changed.
#697918: Update theme when default theme changes might need to have a different solution - that is kind of a special case.
Comment #7
Kiphaas7 CreditAttribution: Kiphaas7 commentedIn danger of overengineering this, why not perform an ajax request for the original window everytime a form is submitted in the overlay? Would remove the need to special case the theme page.
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedSure, we could do that (like I suggested in the original proposal above). As I wrote there: "Most modern browsers do that pretty seamlessly." However, that means some don't :) And at least in my testing right now, "most" is maybe an overstatement. Reloading the whole page with the overlay displayed in it, Firefox 3 looks good, Internet Explorer 8 looks (mostly) good, but Internet Explorer 7, Safari and Chrome do not look good - there are jitters and flashes and that sort of thing. In particular since this isn't just for form submissions, but also for GET requests (like someone clicking the '+' link to add a shortcut), which people probably expect to have an AJAX-y feel to them, I don't think we would want to do this unless we could make sure it doesn't look herky-jerky on the screen.
If it can be fiddled around with to make it work, that would be great though :)
Comment #9
eigentor CreditAttribution: eigentor commentedAhem I asked before, but isn't this a critical issue? The overlay cannot be shipped withouth this being fixed. Might be water on the mills of those that want to throw it out...
Comment #10
bleen CreditAttribution: bleen commentedWhile I would very much like for the parent page to get updated smartly I'm just going to throw the suggestion that I made at #758732: close & refresh btn for overlay out there... we could have two close buttons on the overlay one that just closes it and one that closes it and refreshes the parent page. This way there will be no confusion on the part of the user.
Again, if we can get this working smartly that would be stupendous but if not then this may be a good option.
Comment #11
eigentor CreditAttribution: eigentor commentedSorry duplicating the close button is no good idea. Feels like having two handles on the door.
This issue needs more of a decision than a lot of work IMHO. Reloading the parent page every time something is saved inside the overlay regardless of the overlay being closed still appears the best solution.
It may appear performance intensive, but it is very simple and saves headaches in determining when to reload. Furthermore it makes sure the closing of the overlay itself is always very quick as it is not delayed by a saving process.
Hm as I read from David's latest comment this does not work smoothly in all browsers. What would be an alternative?
Comment #12
eigentor CreditAttribution: eigentor commentedHit me on the head, but this one is critical for the Overlay, which is part of the default profile.
Comment #13
bleen CreditAttribution: bleen commentedThats where my brain got stuck :)
Comment #14
markabur CreditAttribution: markabur commentedAnother way to see this issue is to change the site name at /#overlay=admin/config/system/site-information -- you can rename the site all you want but the head_title won't update until the page is reloaded.
Comment #15
Bojhan CreditAttribution: Bojhan commentedThis doesn't seem critical, although slightly annoying - its not preventing use.
Comment #16
eigentor CreditAttribution: eigentor commentedAs I rethought this:
Is there any reason we don't just reload the parent window when the overlay (child) closes?
Would not be as smooth as reloading it before, but fully do the job.
This should be pretty easy to do, or is the problem the Child window does not know the parents URL?
Comment #17
ksenzee#16: The child window can communicate with the parent, so that would work. It would just feel dog-slow. :(
I don't have time right now to do the research on this, but I personally consider it a critical bug, and I really don't want D7 to release without our having figured it out. I do think it's solvable.
Comment #18
eigentor CreditAttribution: eigentor commentedVery much appreciating you also consider this as critical ;). Annoys me every time I work with D7 and close the overlay.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedIf it turns out to be too slow, then as per #6 we could only trigger the reload when we think something actually changed, not every time the overlay closed. That's why I made that suggestion. Obviously, triggering the reload every time would be simpler though...
I'm kind of interested in looking into how the Skinr + Dialog API combo does this. That seems to reach the holy grail of having the page update automatically in real time, without waiting for the dialog to close (and it looks pretty smooth although I don't think I've ever used it in anything but Firefox). From a technical perspective they are very different situations so it might not wind up being relevant, but then again, it might :)
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedI looked a bit into how Skinr/Dialog API does this and although it's pretty neat and clever, it's not going to help us here much :(
Basically, Skinr takes advantage of the fact that it's only really adding/removing CSS classes and replacing CSS and JS files (all within the same browser page). So, when you click on something in the dialog, it keeps track of which CSS classes that checkbox/radio/etc was associated with and uses simple jQuery to add/remove that class where appropriate. Then it makes an AJAX request to find out if any CSS or JS files have changed, and also uses jQuery to replace those in the document <head>.
So basically, it's not too different from what we are already doing in the overlay module, just that in overlay we have to be able to detect and deal with any possible change to the underlying HTML, plus deal with two different windows (parent and child) which in our case have two different themes. The patch I originally posted in this issue is therefore probably on the right track if we want to make any region of the page (rather than just the toolbar and similar regions) update in real time like Skinr does, but as already mentioned, it's complicated. If it's too complicated to deal with at this point, then we'll have to go with one of the other solutions proposed here, where we only guarantee to get the entire underlying page correct when the overlay closes.
Comment #21
ksenzeeHere's a simple patch that implements David's idea from #6. It forces a full page refresh when the overlay is closed if the user submitted a form inside the overlay, or submitted a GET request with a token. It's not as dog-slow as I feared it might be.
It would be great to get the parent reloading in the background, but that's probably not going to happen before D7 release unless one of us wins the spare time lottery.
Comment #22
markabur CreditAttribution: markabur commented#21 is good with my quick test -- the site name updates when the overlay closes, which it didn't do before.
I agree that it would be better to refresh the page in the background when the save button is clicked, but if that's not feasible for d7 then refresh-on-close seems like a good compromise.
Comment #23
aspilicious CreditAttribution: aspilicious commentedClicking on the home button, is that closing the overlay to?
Comment #24
casey CreditAttribution: casey commentedksenzee, Nice!
I don't like the logic to be in the overridelink event handler though; we introduced overlay events for that.
Also it shouldn't remove the whole fragment; it could contain more jQuery-BBQ states (see patch in #655416: "Demonstrate block regions" bugs with regions, navigation, overlay).
I took the liberty to roll a new patch.
@aspilicious, clicking on the home button opens already in the parent window so that'll always (re)load the parent window.
Comment #25
Gábor Hojtsy@ksenzee, @casey: yeah, working with this code it turns out if we put it in the overridelink handler, it will only refresh the parent on manual close, it does not provide us with a way to tell the client to refresh the parent page without the user actually clicking that link, so @casey's approach seems to be more suited for a general "refresh parent on close" request from the server.
Comment #26
ksenzeeCasey's approach looks good to me. I am not going to RTBC it since about half the patch is still my code. Gábor?
Comment #27
Gábor HojtsyI think the only remaining issue which was in both patches is that the parent reload information handoff happens *after* the overlay gets closed and is therefore not at all effective if you request the overlay to be closed and the parent to be reloaded in the same page request. If we just move up the information handoff from child to parent to before the overlay close handling, you can do both in the same page request and don't need two page requests to set up the parent reload and the closing of the overlay. Here is a patch rolled with that change.
Comment #28
markabur CreditAttribution: markabur commentedLooks like there's an extra word here, should say "may have decided to tell the parent window..."?
Comment #29
bleen CreditAttribution: bleen commentedthis is the identical patch from #27 ... with the comment fix from #28
Comment #30
casey CreditAttribution: casey commentedDavid_Rothstein, can you review/RTBC this?
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedYes :)
The code looks great, makes sense, and nicely parallels the existing code for immediate AJAX refreshes. And the patch seems to work fine - I've tested it with standard Drupal core, and we've also been testing it for a little while with the current development version of Drupal Gardens we're working on.
I've only tried it in Firefox, but there at least, the page refresh works rather smoothly, so in the case where it turns out to have been unnecessary after all (e.g. a form was submitted in the overlay, but it turns out nothing about the underlying page actually changed) the extra page load is hardly even noticeable.
(Note, by the way, that to get this patch to work you may need to do a hard refresh of your browser - including one inside the overlay iframe itself.)
RTBC as far as I'm concerned.
Comment #32
bleen CreditAttribution: bleen commentedTested in FF(mac) Safari (mac), Chrome (mac), IE 6,7,8 by changing the current theme in overlay, closing it and seeing the new theme ... then by changing the color of the Bartik theme in the overlay, closing and seeing the horrifically ugly colors I chose...
patch = awesomeness
Comment #33
eigentor CreditAttribution: eigentor commentedHeya, great! From a user perspective it really works fine. I had some delays in page reload after closing the overlay but like to believe this is due to the incredibly slow performance of Apache on Windows 7.
Comment #34
Dries CreditAttribution: Dries commentedLooks good. Committed to CVS HEAD.