You can see this, for example, with the node creation form. Submitting it closes the overlay and redirects you to the node you created, but before it does that, the form is quickly redrawn inside the overlay for a brief second, which is annoying.
Currently it needs to do that because the trigger for closing the overlay comes from JavaScript code loaded into the overlay itself, so that JavaScript needs to get added somehow before the overlay can close.
A clean solution to this problem might involve changing a lot of the way the overlay works. Here is a stab at a simpler solution where we just render an empty page in the overlay before closing it, rather than rendering the whole form. The basic idea is from @ksenzee, and then I thought we might be able to do it somewhat simply with this new delivery callback feature in Drupal 7 (see the patch). This works functionally, but (a) it's kind of a hack, and (b) even the rendering of the blank page is still a bit distracting, because there's a bit of whitespace plus the page title and close button that flash on the screen when this happens. So, "needs work" for now.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | recent log entries.png | 113.73 KB | Georg |
| #7 | overlay-close-without-full-refresh-662856-7.patch | 3.21 KB | David_Rothstein |
| #3 | overlay-close-without-full-refresh-662856-3.patch | 3.18 KB | David_Rothstein |
| overlay-close-without-full-refresh.patch | 3.18 KB | David_Rothstein |
Comments
Comment #1
David_Rothstein commented@ksenzee says this looks a lot smoother on her computer than mine, plus it actually does look smooth on my computer when I combine it with some of the recent performance patches at #615130: Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems.
So, promoting this to "needs review" :)
Comment #2
ksenzeeI didn't even realize this hook existed until David pointed it out. (I wonder what other hooks I've missed.) This is perhaps an unintended use of the delivery callback feature, but that doesn't necessarily make it a hack. It lets us do precisely what we want to do: transmit the Javascript settings in as minimal a page as possible to the iframe, when certain conditions are met. In fact, using this for forms submitted with a ?render=overlay parameter isn't really all that different from, say, using an XML delivery callback for ?render=xml.
It's hard to benchmark this sort of interaction, but subjectively this is a sizable performance improvement. I think it's worth doing. RTBC after the typo in overlay_form_submit is fixed (s/Requess/Request).
Comment #3
David_Rothstein commentedOops.
Comment #7
David_Rothstein commentedThat failing test is almost guaranteed to be bogus. Let's try again.
Comment #8
kiphaas7 commentedYou probably meant "needs review"?
Comment #9
David_Rothstein commentedI think RTBC because @ksenzee already declared it as such in #2.
Would be nice to see the testbot finally confirm it at some point, though :)
Comment #11
webchickIs it just me, or does anyone else not notice a difference with/without this patch? Either way there's an awkward pause while it's closing the overlay and redirecting.
I think this could probably use at least one more review from someone who didn't work on the solution.
Comment #12
moshe weitzman commentedcode looks good to me. so, rtbc.
i can't actually test the fix since the friggin overlay is showing the toolbar again on the page and thus and obscuring the node title textfield.
i don't think the overlay should be used for node add/edit, but thats a different issue.
Comment #13
ksenzeeThe friggin overlay can't do much to nix the toolbar if its preprocess functions aren't getting called, as mentioned in #651086: Cache clearing is an ineffective mess in module_enable() and system_modules_submit(). Clear cache and the hugely annoying double toolbar will be a thing of the past.
Comment #14
aspilicious commentedCan we review the comment of webchick please (#11)
Comment #15
Georg commentedI tried the patch and I'm sorry to report that I did not see any difference.
Tested in Browsers:
Firefox 3.6.3
IE8
Opera 10.51
Safari 4.0.5
Chrome 4.1.249.1045
On my VirtualPC tested with IE6
What I did notice (without the patch) was, that sometimes there seemed to have been displayed a notice. But I wasn't able to read it, because the overlay got closed and the node was displayed (which does not work in Opera [I'll search for a different issue or open a new one for this]). In the recent log entries I did see a notice but don't know enaugh to know, whether this was displayed or silent. I attached the log as screenshot, maybe this helps.
Considering this, the question coming to my mind is: Do we want to close the overlay without giving the user a chance to see the notice? (If the node saving failes, we return to the overlay, that's allright)
edit: new #773396: After creating a node in Opera the new node isn't displayed and overlay breaks.
Comment #16
cwgordon7 commentedOk, but regardless of whether or not this changes the visual experience, the changes in this patch are also necessarily to avoid a variety of problems that arise with actually loading the page. For example, loading the page causes status messages to wipe so they don't appear when the overlay closes. Basically, we want the page callback to fire without the theming system doing anything, which is why this solution works nicely. Committing this patch will make it possible to fix several other core issues, including the critical bug at #744976: Overlay suppresses drupal_set_message() after node update. Resetting to rtbc.
Comment #17
EvanDonovan commentedSubscribing - I believe this is the patch that David_Rothstein told me last night would fix #744976: Overlay suppresses drupal_set_message() after node update, as cwgordon7 states.
Comment #18
webchickI have no idea what this does, since I still can't get it to do anything, and it gives me ooky feelings :P, but it has buy-in from the Overlay module maintainers, and apparently is a precursor for getting other stuff in, so....
Committed to HEAD. ;)
Comment #19
David_Rothstein commentedThis patch definitely did used to work, but in the intervening months it appears to have stopped working - now that it's actually committed it doesn't do anything for me either :)
The overall cleanup patch at #658720: Clean up overlay_close_dialog() and related code fixes it for me (as well as fixing a bunch of other issues) and with that it does lead to a noticeable difference.