Following up on http://drupal.org/node/517688?page=1#comment-2172050, this is the remainder of patch 416 without core API changes (the core API changes are in #610204: Overlay API changes). Someone who is familiar with the current status of what still needs to happen on this issue, please provide that summary in a comment.
Comment | File | Size | Author |
---|---|---|---|
#69 | double_shortcut_bars.png | 47.31 KB | karschsp |
#61 | 610234-overlay-61.patch | 92.61 KB | David_Rothstein |
#57 | overlay-links-rendering.jpg | 68.8 KB | te-brian |
#50 | 610234-overlay-50.patch | 79.73 KB | ksenzee |
#48 | 610234-overlay-48.patch | 79.73 KB | ksenzee |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedAnd here's the patch. Regarding questions about what is/isn't an API change, this patch does include adding a toolbar_overlay_child_initialize() function to toolbar.module, but that seemed to belong here rather than the other issue. This also includes dashboard and toolbar js and css changes, but nothing that to me looks like an API change. Finally, this includes what to me looks like perhaps overly extensive changes to seven's style.css, but I suspect that's not subject to the code freeze date, but rather the 11/15 UI freeze, so I guess it's fair game.
Setting this to "needs work", because testbot will choke on it until #610204: Overlay API changes is committed.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedreally, really setting to "needs work" this time.
Comment #3
seutje CreditAttribution: seutje commentedsubscribe
Comment #4
sunWe need a thorough summary here.
Comment #5
markus_petrux CreditAttribution: markus_petrux commentedsubscribing
Comment #6
Gábor HojtsyOn it.
Comment #7
Gábor HojtsyStill on summarizing. Will come back hopefully soon with a comprehensive summary. Meanwhile, attaching the latest image zip file. Uncompress this to modules/overlay to make the close button and loading spinner show up.
Comment #8
Gábor HojtsyYou've asked for it, so here it comes!
I literally took 5(!) hours today to read every single line of feedback on the overlay issue and try to devise whether it was already taken care of and even if it was, whether it morphed into a different problem. Looked through follow up issues in reference for the same problem, looked into the code, tried to reproduce the issues, etc. In case, when I did not manage to reproduce some issue earlier and did not specifically read on the issue that it was solved, I brought that issue over (like sticky table headers being too sticky :).
Deliberately collected every outstanding item without putting any priorities, categorizing and numbering the issues for easier reference. We can take this to a wiki page or use this comment to come back and edit with cross-overs for stuff that is taken care of. Using category numbering might also help others to add new items in follow up comments, and we could still keep referencing the same issues.
Drupal/API issues
<a>
link, a FAPI redirect, or drupal_goto(), Drupal has no way of knowing that it should render the new content without the toolbar. That rendering is controlled via a GET parameter ?render=overlay, so we need to find cases, when this is not added, so the overlay behavior is not triggered.Tying to administration
@sun said multiple times: "What remains in Overlay is the dialog/parent handling as well as the child handling. All strings and descriptions referring to "administration" need to vanish. Overlay is not limited to administration", @markus_petrux fully agreed, @Dries vaguley said API-ification is important.
Workflow/process issues
Styling
Missing features
JavaScipt
Usability
Performance
Browser compatibility
Now I definitely need a break from this issue for today, so have fun ;) There are definitely some low hanging fruits (leftover code, bad code comments, etc), so the items are not at all identical in weight / task size.
Comment #9
RobLoachI love how we're cropping the patch up into different issues now. Subscribing.
Comment #10
drifter CreditAttribution: drifter commentedAwesome list, Gábor. Agreeing that API changes should go into core now, and then in the worst case we could still have the Overlay module in contrib.
Comment #11
mcrittenden CreditAttribution: mcrittenden commentedSubscribing. Awesome work, all!
Comment #12
mcrittenden CreditAttribution: mcrittenden commentedComment #13
mcrittenden CreditAttribution: mcrittenden commentedComment #14
marcvangendsubscribing
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedI spent some time today looking into this one (which by the way does not seem to depend on Firefox version - I have 3.0).
First, the quick result is that it may have some dependence on your graphics card driver. Using Ubuntu Hardy with the "nvidia-glx-new" driver (latest version, 169.12+2.6.24.18-25.2) is where I see the horrendously slow performance. Switching drivers to use the open source alternative, the overlay module is blazing fast. It's an absolutely enormous difference. Note that this driver is known to have some problems when combined with Firefox (https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/223238)...
However, we're not really out of the woods - we can blame some performance issues on the driver, but I browse the Internet all the time with the NVIDIA driver and never see anything remotely resembling the slowdown that the overlay causes (again, we are talking about the browser locking up for 15-20 seconds and 100% of CPU resources being consumed, so this is not a minor problem).
I then spent a little time tracking down where in the overlay code is causing this. It appears to be almost entirely due to Drupal.overlay.resize() in overlay-parent.js. If you put a return statement at the beginning of this function, the overlay works fine in Firefox (there's occasional problems with slow screen refresh while scrolling, but that is probably a separate problem and much easier to legitimately blame on the graphics driver). Note: This function is called repeatedly every
150 seconds[edit: 150 milliseconds] via the autoResize method (see description in P1 of Gábor post), but that doesn't seem to matter much... I found that even one call to the function can be enough to lock up the browser.Interestingly, getting rid of the resize() entirely - which is a rather large chunk of code - does not seem to negatively impact the overlay that much. Basic resizing and fieldset expanding still seem to work fine (?), but there do appear to be some issues on occasion (not winding up at the very top of the overlay when submitting a form, not being able to scroll all the way up after expanding and collapsing some fieldsets repeatedly, etc). So I guess that means we need something there. I haven't looked too deeply into the resize() function to see how it can be improved - there are a fair amount of selectors and such in there that maybe are the culprits. Overall, this is pretty weird, and I'm leaving it for now, figuring that maybe someone who is more familiar with that part of the code can take a further look?
Comment #16
eigentor CreditAttribution: eigentor commentedsub-bass
Comment #17
ksenzeeI'm splitting out the issues from Gábor's extensive list into separate issues, and I'll post the issue numbers here as I create them. Starting with #613730: Overlay A1: overlay needs its own permission.
Comment #18
ksenzeeI've created a github repository for overlay work. (Sorry webchick. I just can't do the patch workflow on this anymore.) I suggest that people who want to work on overlay issues submit patches to those issues against the github repository:
git clone git://github.com/ksenzee/drupal.git ./some/nonexistent/directory
cd ./some/nonexistent/directory #(which git will now have created for you)
git checkout --track -b overlay origin/overlay
Hack away to your heart's content. To create patches:
git diff --no-prefix > myawesome.patch
So the idea is that you're creating a patch that actually reflects what you did on the issue, instead of rerolling the whole huge overlay patch. If anyone wants to work on overlays but is intimidated by this process, *please* ping me on IRC (ksenzee) and I'll quite happily walk you through it.
P.S. If you're an old hand at git, feel free to fork the github repo and submit pull requests. Just post a patch to the issue as well so the discussion stays on d.o.
Comment #19
ksenzeeIssue D4: #615114: Overlay D4: rename overlay_mode() to overlay_set/get_mode()
Comment #20
ksenzeeP2: #615130: Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems
I copied David's comments at #15 into the issue. (Thanks David!)
Comment #21
ksenzeeOn second thought, instead of spamming you all every time I create a new issue, let me instead direct you to this handy list of overlay-related issues. If an issue there isn't taken, feel free to assign yourself, and post a patch against
git://github.com/ksenzee/drupal.git
.Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedI just tried out the latest overlay code from today (containing the URL fragment fixes). Here is some feedback - some of these might eventually want to make their way to separate issues:
There was a previous (but unfortunately more complicated) way of launching the overlay when the module is enabled, which is what this code is trying to do, so we may have to go back to the previous method.
Comment #23
ksenzeeThanks David! Splitting out the above into separate issues: 2 is fixed in github, 4 is at #619352: Overlay: Fix redirects, and 6 is #619362: overlay doesn't scroll to the right position for URL fragments. I'm waiting to file an issue on 5, because I think it'll get fixed along with the redirects.
About 3: I can't replicate this in any browser I've tried on Ubuntu, XP or Vista. Did you grab a screenshot by chance? Is anyone else seeing the same thing?
Comment #24
SeanBannister CreditAttribution: SeanBannister commentedsub
Comment #25
Gábor HojtsyTested the current state of the git repo. Could reproduce 3. The way I did it is that I disabled the overlay, clicked around a bit and then enabled again. From here, I cannot get the overlay to show not under the toolbar. Reloading, going off the overlay, etc does not solve it. Here is how it looks. Seems like the top displace is not adhered to at all.
I can't seem to reproduce #6 interestingly though.
Comment #26
Gábor HojtsyFound out with Katherine that this bug is outside of the overlay. If you clear the cache and reload the page, it starts to work fine again. Basically, the theme preprocess hook is not used when the overlay is disabled and then enabled. A bug outside the overlay.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedI can't reproduce 3 anymore at all. I can still reproduce 6.
Comment #28
ksenzeeThis is ready for review again. I'll be offline for a few days, so I'm moving it back out of github so others can continue work. Please reroll this patch if you have any further fixes. Most of the issues from Gábor's summary are taken care of, and I think the remaining issues are acceptable as followups. The performance issues are the main remaining problem, but I recommend that even though they're a serious issue, we commit this anyway and work on performance as a followup. It's going to be a lot easier if we get more eyes on it, because not everyone can even replicate the performance problems, much less work on them.
The part of this patch that still needs review and perhaps some work is the new hook_admin_paths(). I'm open to name improvements, and we need API documentation, but I'm not sure where to put it. I also have the feeling the node.module implementation of the hook is iffy -- don't we have a setting somewhere that dictates whether node/add etc. are considered administrative? I couldn't remember where it was.
Also please note that I have done nothing to update any other parts of core to make them hook_admin_paths() aware. That can (and should) come later. It won't be an API change, and it probably counts as a bugfix, so I think we could do it in D7.
Comment #29
Gábor HojtsyThe patch includes the BBQ jquery plugin to handle back button issues; this library is licensed under the MIT license, and therefore cannot be included as-is in Drupal. Talking to Katherine, she said the author is considering dual licensing it and would be happy to see it in Drupal 7. This needs to be resolved though, before the patch is committed. In addition, it is probably not a good idea to register this library in overlay module, given it exists in misc. Rather register this library via system module IMHO.
Looking at hook_admin_paths(), you picked the path pattern support as in block visibility, etc. This should indeed be clearly documented. People earlier suggested using hook_menu() like structures, but sticking to a known structure is most important, so block visibility works for me as well. Should probably be documented in system module's APIs. Can be useful elsewhere at many places. On the preexisting node admin path setting, Drupal already has that, and that might be duplicate after this hook lands. I'm not sure it is our job to remove that though in this patch. Since that one has a UI, and this path hook does not, they are not straight replacements.
I'm seeing that you included code to resize and reposition the overlay when the toolbar is resized. That seems to be pretty much toolbar specific. Not sure what we can do there to make it more useful for contrib. Contrib will need to call the overlay resize the same way anyway. Hm. Also, the page refresh code that was implemented for toolbar seems to use a path like notation for addressing array elements, which I've not seen in Drupal elsewhere. Would not say form_set_error()s obscure element hieararchy definitions are better though :|
The hash handling and support for internal page links seems to be cleverly implemented with these hashchange events in JS. Looks good to me. However, there is lots of code involved with maintaining that path from the forms and do the redirections and path munging. Don't think all that can be saved if we intend to have full back button and bookmarkability/permalinkability support (which we do), so this looks like necessary baggage.
Basically, two big chunks causing this patch to be way bigger then it was before are (1) the back button handling (BBQ plugin and all related path munging in JS and PHP) and the toolbar refresh handling which was elaborately implemented for link changes in both the toolbar and the shortcuts. That and smaller other changes almost doubled the patch in size.
Next up, I'll look at issues from above and which ones are still applicable.
Comment #30
Gábor HojtsyQuick testing of the overlay shows forms, toolbar reload, toolbar resize, etc. are working as expected. Noticed two new bugs however:
- when the toolbar is reloaded, none of the links result in any change on the page (except being set to active in the down state); this is true to all links in the toolbar but not links in the overlay
- when editing a shortcut, the change is not refreshed on the toolbar (also, the weighting of the shortcuts change, but that bug is not a problem with the overlay); looking at the code, the toolbar is refreshed when adding new shortcuts or editing/switching the set, but not when individual shortcuts are edited
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedHere's some feedback - this is not based on a full review, but just mostly trying to focus on the parts that touch the rest of core, since those are the most important to get right before any initial commit.
We do, but the two settings are totally different (i.e., I think this aspect of the patch is fine as it is). The other setting is the 'node_admin_theme' variable but that controls the overall theming of the node editing pages. This new setting is for overlay users only and is meant to override the first (i.e., anything that appears in the overlay always tries to use the admin theme if there is one, regardless of what other users see when they are viewing the same page outside the overlay). We very likely want to ship core with 'node_admin_theme' off but node/*/edit "overlay-ized" - Charlie even had that change in some of the earlier overlay patches.
What this all points to is the fact that hook_admin_paths() is going to be a very confusing name :) Why not just call it what it is? - i.e., hook_overlay_paths(), and documentation for it goes in overlay.api.php.
I don't have a very strong opinion but I do think something like 'node/%/edit' is a bit better, since this is developer-facing and that is how they are used to representing menu wildcards.
Also, If I recall correctly, the previous implementation (where this hook was in javascript) had two separate lists, basically "in overlay" and "not in overlay" and one list could override the other. I think I liked that because it gave more flexibility. One way to approximate that here might be to instead have the return value of the hook be like this:
A bit more repetitive at first, but then using FALSE would mean "put in the no-overlay list". This would allow you to do more flexible things such as setting
'node/2/edit' => FALSE
in the alter hook (which would exclude that node, but not any of the others because the more generic 'node/*/edit' would still be TRUE).I hope this makes sense - it probably doesn't :) What I mean is that I liked the style of the previous JavaScript "hook" a bit more than this one.
This three-way coupling between modules is a little scary and not even necessarily correct -- saving a new shortcut does not need to refresh the overlay unless shortcuts are actually being displayed in the toolbar, which they don't have to be.
This also worries me because it seems like a number of contrib modules might have to constantly call the same kind of code in order to work correctly.
Here is an idea (probably a crazy one that is not very well-thought-out) for how to deal with that:
Perhaps "toolbar-like regions of the page" (basically, any module that populates a region of the page which needs to have the 'refreshable' behavior) are labeled somehow by the modules that introduce them. For example, toolbar.module would somehow indicate (either via a hook, or a special key in the renderable array, or whatever) that $page['page_top']['toolbar'] falls into this category.
Then, overlay module does something like this:
Does that make sense? The benefit is that no outside code would ever need to trigger overlay_request_toolbar_refresh() directly; it would all happen automatically from inside here. The obvious flaw is performance (needlessly re-rendering certain stuff twice on every page request for these users) - however, if you are browsing your site in the overlay, you are unlikely to care about that. It would not affect non-overlay-users on the same site at all since the function would simply bail out early, although the fact that hook_exit() is a bootstrap hook means it would be an extra function call even on cached page views, I guess.
Comment #32
Gábor Hojtsy@David: On (1), I can imagine a list of admin paths would be useful for other reasons as well. Such as applying a consistent theme to those paths regardless of the overlay. A contrib module could do that. Or having an overview of ALL those paths at some place. Therefore I think this could be a generic hook not tied to the overlay.
On (4), what would be that earlier time in the page call where that data would be generated initially? Sounds like a neat trick to avoid putting conditional code all over the place for sure and would let us support multiple regions to be refreshed.
Comment #33
Gábor HojtsyUpdated status on the different items based on feedback from Katherine:
Drupal/API issues
D3: class="overlay-exclude" can be used in the parent window to not get an overlay opened for your link (additionally to the JS patterns); there is no such exclude/escape possibility in the overlay itself anymore, only links which are clearly not admin will close the overlay, no way to opt out via markupcovered by the admin link hookD4: @webchick suggested to rename the overlay_mode() function to overlay_set_mode(); which then might be accompanied by an overlay_get_mode() I guessdone in #615114: Overlay D4: rename overlay_mode() to overlay_set/get_mode()D8: When a page (re)loads its content without going through an actualfixed<a>
link, a FAPI redirect, or drupal_goto(), Drupal has no way of knowing that it should render the new content without the toolbar. That rendering is controlled via a GET parameter ?render=overlay, so we need to find cases, when this is not added, so the overlay behavior is not triggered.Tying to administration
@sun said multiple times: "What remains in Overlay is the dialog/parent handling as well as the child handling. All strings and descriptions referring to "administration" need to vanish. Overlay is not limited to administration", @markus_petrux fully agreed, @Dries vaguley said API-ification is important.
A1: @sun had concerns tying access to the overlay with "user_access('access administration pages')"; we probably need our own permission so that people who only have permissions to moderate but not general admin page access can see this page.fixed via #613730: Overlay A1: overlay needs its own permissionA2: @sun had concerns of directly switching to the admin theme at all times, but since then we made progress of making overlay even more admin-centric, so still not sure whether we can decouple the overlay from administration at all; it was suggested that we could have an overlay_theme variable_get(), which then could be overriden by modules but would be the admin theme by defaultnot being fixed, the overlay already has multiple levels of abstraction below itA3: given how isAdminLink() is implemented, if any admin links are aliased, those links might not be matched; @ksenzee says aliasing admin links is not too likely, so we sidestepped this issue; however given that we could generalize this a bit for non-admin use, renaming this to isOverlayLink() could be useful when the aliasing question would be more pressingconverted to hook, PHP level aliasing possible (but could grow array quite a bit)Workflow/process issues
W1: back button works in the overlay, but not when you are thrown out of the overlay with a friendly frontend node or user link; you cannot get back your "Find content" or user filtering page without going through all the filtering hoops you usedfragments implemented via #617372: Overlay W1 / F3: fix the back button and allow permalinksW5: adding a shortcut does not actually make that shortcut appear in the shortcut bar (due to parent page not being reloaded or at least shortcut bar not being reloaded via Ajax/AHAH)fixed via #615160: Overlay W5 / W6: reload (parts of) parent window under certain conditions but has missing functionality with editing shortcuts and code weight disputed by DavidW6: When disabling Toolbar module from within the Overlay, it is still there (again, see W5). This could extend to whatever is displayed outside the overlay (admin_menu, the upper part of the toolbar, any lower bar like l10n_client, etc).See W5W7: Go to your front page. Hit configure block on powered by Drupal block. Edit settings, hit save. Now you get the front page in Garland in the overlay. Duh.Fixed.Styling
All to be post-commit issues except last one.
S8: "Add to shortcuts" button positioning way off compared to mockups (it would be vertically centered to the title ideally)fixed via #615214: Overlay S8: "Add to shortcuts" button is in the wrong placeMissing features
F3: not possible to bookmark or permalink admin pages with the overlay; see also W1fixed via using fragments, see #617372: Overlay W1 / F3: fix the back button and allow permalinksJavaScipt
J1: Drupal.settings is used, but should use the local settings variable passed on in attach() and should pass that on for local methods, see mfer's explanation at http://drupal.org/node/517688#comment-1813954solvedJ2: $('a:not(.overlay-exclude)') does not use the context, should use it!does not exist anymoreJ3: @sun had performance constraints with the $('a:not(.overlay-exclude)') matching of all links (ala Google Analytics), but no better ideas were raiseddoes not apply anymore due to how link matching changed; the current filtering based link matching might have similar issues thoughJ5: @RobLoach says self.isOpen || $('#overlay-container').size() could be re-coded with jQuery.once()too late to redoJ6: @Damien Tournoud and @sun said markup generated by JS should go through Drupal.theme()doneJ7: @RobLoach says self.processed in overlay-child.js can be jQuery once-d with $('body').once('overlay', function() { ... });too late to redoJ8: If shortcut bar was not open in toolbar and overlay was open, if you open the shortcut bar, it goes above the overlaysolved, overlay moves as toolbar is collapsed/expandedJ10: setupDrawer() in the dashboard was not fully updated to attach the exitCustomizeMode click handler to the link; @dmitrig01 has a simplified code suggestion at http://drupal.org/node/517688#comment-2168210 or @seutje has slightly different suggestions at http://drupal.org/node/517688#comment-2171332doneJ11: isAdminLink() using JS regexes was deemed ghetto; @David_Rothstein has a good (@webchick approved) roadmap at http://drupal.org/node/517688#comment-2171822was resolved with a PHP Drupal hookUsability
U3: none of the user menus work in the toolbar (Hello username and Log out) when the overlay is onfixedPerformance
P3: we could add some static caching in isAdminLink() as per @ksenzee to improve performance of link matching (could help if link appears multiple times to same destination)outdated due to newly implemented hookBrowser compatibility
Comment #34
Gábor HojtsyThere are at least two hard outside dependencies for this issue I know of:
- the BBQ library is currently not GPL licensed, we can't commit it as long as it is not
- the custom theme switching support from #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed is required and dead code in the patch marked with @todos need to be fixed to depend on it, see #615138: Some pages display in the overlay in a non-adminstrative theme
Comment #35
Gábor HojtsyVideo of the latest incarnation of the overlay as in the patch above in #28.
Drupal 7 overlay from Gábor Hojtsy on Vimeo.
Comment #36
webchickHot damn, that's lookin' sexy! I can't express how completely ecstatic I am that the back button works!
OMG! The shortcut bar updates too!!
Where's a patch, huh huh, where's a patch huh? ;)
Comment #37
Gábor Hojtsy@webchick: the latest patch is still on #28.
Comment #38
seutje CreditAttribution: seutje commenteddon't forget the images from Initial D7UX admin overlay comment #190
Comment #39
Gábor Hojtsy@David: on your hook_exit() idea, I was trying to map out further implementation details, but the main impediment there seems to be inability to tell which parts of the page array was built by which function. So we'd need to go through the whole page building again from start to allow all alters and other functions to run and get an up-to-date version of the page, right. Eg. how shortcuts add in the page array for toolbar to pick up the drawer contents is not something we can generically model without rebuilding the whole array, do we? Just calling render again would render the same data, and we actually need new data.
Comment #40
ksenzeeLet me pop in from my vacation to quickly point out that I had a conversation with Dries the other day about the tight coupling among overlay, shortcut, and toolbar. My original idea had been to add hooks to toolbar and shortcut that would fire when their menu items changed, and then to implement those hooks in the overlay module. He thought that would work okay, but he preferred to have toolbar, shortcut, and contrib modules know about overlay and implement its API, rather than the other way around. So just FYI on why that coupling is the way it is.
Also, I forgot to list as a serious TODO that the function signature of overlay_request_toolbar_refresh makes no sense. It can't specify a CSS id, because that can be changed at the theme layer. It sounds like you're on the way to solving that though. I just wanted to make sure you had all the information I did.
Comment #41
webchickGo back to vacation, ksenzee! *whpssh!* ;)
Comment #43
webchickTest bot running amok.
Comment #44
ksenzeeWhat this all points to is the fact that hook_admin_paths() is going to be a very confusing name :) Why not just call it what it is? - i.e., hook_overlay_paths(), and documentation for it goes in overlay.api.php.
I can think of lots of uses for this besides the overlay - the most obvious is theme switching on admin pages, but I'm sure contrib will come up with other uses. I did change the structure to be
'node/add' => TRUE
, which I think makes sense for several reasons. With this structure, Advanced Forum (or the forum module itself) can determine whether node/add/forum is administrative or not, which would make for a nice checkbox.This patch includes the newly GPL-licensed jQuery BBQ plugin (yay!!! thanks Ben Alman). I'm switching to the minified version since we appear to be past the debugging stage for that part of the code. I also moved its hook_library() entry to system.module.
Remaining pre-commit TODOs that I'm aware of:
# Look at replacing overlay_request_toolbar_refresh() with David's hook_exit() idea. I believe he's working on that now.
# Get the toolbar links working after the toolbar reloads.
# Get the toolbar refresh to happen when individual menu links exposed by the toolbar are edited.
# Fix a new bug I just discovered, wherein resubmitting the modules page closes the overlay - even if you left overlay module enabled.
I've pushed this version to github, just in case anybody cares, but I'm also including a patch here.
Comment #45
ben_alman CreditAttribution: ben_alman commentedSweet, happy birthday me!! :-)
Comment #46
webchickHey, Ben! Really appreciate your help! Thank you so much. :)
Hopefully we'll get this patch nice and finished up and then all of the developers using Drupal 7+ can use your wonderful library.
Comment #47
sunHandling of "administrative themes" on non-admin paths is already dealt with in #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed.
Also not sure why we need a new hook for this purpose. If you want to flag paths, then you flag them in hook_menu(), collect that info, and cache it.
Comment #48
ksenzeeIf you want to flag paths, then you flag them in hook_menu(), collect that info, and cache it.
Yes, that was my first thought as well, but please see the old issue, #346 et seq., especially #416.
Handling of "administrative themes" on non-admin paths is already dealt with
hook_admin_paths() has nothing to do with setting an administrative theme. It could, which is what I'm pointing out in #44, but it doesn't. The overlay needs to know whether a given path is administrative for other reasons, one of which is so that when a form is submitted, the overlay knows whether to open the destination in the overlay or in the parent window.
All the TODOs from #44 are either taken care of with this patch, or waiting on David's hook_exit() implementation. This also fixes the "add to shortcuts" button, which was broken after the much-needed #609100: Should not be able to re-add same shortcut to same set twice went in. Please note that it still feels pretty broken in a lot of cases, which is because the query-string fix got taken out of the overlay patch and moved to #614498: Add handling for query parameters in shortcut links. You can test the shortcut add/remove button by clicking on, say, Appearance, adding it to the shortcut bar, then deleting it again, all without reloading the page. (If you reload, you'll run into the query-string bug.)
Comment #49
Gábor Hojtsy@sun: adding to what @ksenzee said, there are two other concrete reasons:
- the use of an admin theme might depend on other factors outside the overlay, while the overlay needs to enforce the same admin theme in all cases; as discussed in detail at #546186: Migrate overall node admin theme setting to per-content type, the use of the admin theme and overlay might depend on the permissions of the user; if the user has admin permissions and the overlay is on, all admin pages should show in the overlay; if the user has no overlay permissions (but the overlay is active), the user would probably not see the page in the admin theme
- tying into that per-content type discussion again, some node types might have different purposes on the site, so editing some of them might not open the overlay; this is not implemented in core, but our system allows for that; so you can have a job board or a forum and let users edit those on the website vs. in the admin UI, therefore providing continuity of Drupal's adaptation to the blur borders between administration and contribution on a site; in that case, we need finer control above menu items, which this hook allows for (although it might not be too performant).
Comment #50
ksenzeeChasing HEAD.
Comment #51
mgiffordOk, I've tried the patch in #48. Didn't realize at first I should re-install the Drupal 7 site in order to be able to view the overlay.
Is the overlay an optional thing? I'm assuming that it must be but haven't stumbled over the config yet. In the overlay view it doesn't make sense to have the 'Skip to main content' text there. That is only really relevant if you've got a header with links in it like you do in the normal 7 theme.
It does seem if there are notices on a page that when you hit save they appear in orange for a brief moment (not enough to actually read them), then disappear. But that's more of a usability issue. I'd also hope that if you clicked in the greyed out area behind (and you hadn't changed the page) that it would disable the overlay and take you back. Alternatively having some way to close it. I find I get trapped in it as it is now. Need an X to click on to go back maybe.
I tried to zip down to an anchor within a page in an overlay, that didn't work either (just stayed at the top of the page.):
user/1#overlay=admin%2Fconfig%2Fpeople%2Fpermissions%23module-taxonomy
Tabbing within the overlay seems to work fine. It doesn't seem possible to tab into the top admin toolbars however from the overlay. There needs to be some way to get to the admin menus from the overlay.
I haven't looked at iframe accessibility, but there are some good resources here:
http://www.webaim.org/techniques/frames/#iframe
http://coolwebdeveloper.com/2008/09/making-iframes-content-web-accessibl...
It looks sexy, but seems to load slower in my browser (maybe I've got too many tabs open).
I like the concept, but right now there seem to be too many issues with this.
Comment #52
David_Rothstein CreditAttribution: David_Rothstein commentedThere is a close button. You need to install the images from comment #7 to see it. Hm, it sounds like there is an accessibility problem lurking here?
Hm, I am also one of the people who has seen these performance problems, and I am notorious for browsing with 8 million tabs open. I wonder if that is the common denominator? Anyway, the issue for the performance problems is #615130: Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems.
***
By the way, as advertised above, I am absolutely at work on a general solution for the problem where saving something in the overlay that affects the content of the toolbar needs to be able to (reliably) trigger a refresh of it. I am working on it in my not-so-abundant spare time :) But please yell at me if it's not done by this weekend, because it should be. Here's a rough taste of some of the initial code I have:
The toolbar module would then implement this hook to identify 'page_top' as one of the 'overlay_supplemental_regions', and other similar modules would implement the same. Meanwhile, overlay_render_regions() is a function that returns drupal_render_page(), but only after setting a special flag that causes #access to be set to FALSE on all other page regions. So the end result is that on these particular, rare page requests, we wind up building the page a few times -- once in hook_init(), once for the main page load within the overlay child window, and finally, once in hook_exit() -- but not fully rendering it all three times, so the performance effect even on those pages should hopefully not be too bad.
I think it'll all come together and actually work :) Hopefully it will, because I don't think the alternative is going to fly. Modules have no good way of knowing whether the data they happen to be saving to the database one moment might also be the data that someone else is displaying in the toolbar - they'd basically just be guessing as to whether or not triggering an overlay-refresh is required, and that's on top of all the module_exists('overlay') calls which would eventually be required, well, everywhere in Drupal....
Comment #53
mgifford@David_Rothstein Thanks for the note about the missing images. And man, gotta give credit to this team for pushing this issue ahead.
I'm migrating over some tags from the prior issue (now closed - Thanks Bojhan).
RE performance, I've got to get more ram. That being said it should be something a user should be able to disable if it has a much more complex page. It's essentially loading two themes into one page and not sure how that's not going to cause load issues. Or for that matter issues with mobile technologies if you want to edit a page on your iPhone.
I am looking forward to seeing future versions and will review accessibility of future patches.
Comment #54
seutje CreditAttribution: seutje commentedthere aren't any considerations for mobile devices in core AT ALL at this time and it doesn't seem there are going to be any before D8. So there's really no point in trying ;)
I've never tried to use frames and keep them properly accessible, I know this is a serious pain, which is why I've always avoided frames but I guess I have no choice but to look into it now, thanks for the resources.
I guess the least we can do is put a link behind the iframe to link to a regular non-overlay version of it, but since this is mainly an administrator-feature, I would assume that the overlay feature would be disabled for those admins
Comment #55
mgiffordIt seems to be enabled permanently for user/1. The role based permissions are good, but I'd like to see a user disable it if they chose to do so. Seven isn't the only admin theme. Although. Not sure how well the overlay will work with different themes.
If we get this in core #558928: Form element labeling is inconsistent, inflexible and bad for accessibility I think we'll have a base which is much friendlier for adaption and may work better for mobile environments too.
I've always avoided iframes too. If we're going to use them by default it will probably be fine, but there are enough issues with them that it really needs to be optional. Also agreed on the link behind the iframe
Would be good to be able to have some way to navigate directly to the url without the overlay too.
Impressive how much has been created with jQuery here! Lots of code.
Comment #56
mcrittenden CreditAttribution: mcrittenden commentedAnyone wanting to test the patch in #48 can visit http://d7.mikethecoder.com. Anonymous users have full rights so all I ask is that you don't hack my server :)
Comment #57
te-brian CreditAttribution: te-brian commentedIn mcrittenden's test environment, I am seeing a weird bug for the first time. I haven't tested the last few Overlay patches, but I know back in October this was not happening.
What I'm seeing is that when the overlay has finished fading in, some of the links are still kind of half-rendered. I am using Firefox 3.5 and I have seen something similar in jquery fade effects since upgrading to 3.5. As you can see some of the links are hazy and have a green tinge. This is what I sometimes see during the fade effect but this is the first time the poor rendering has "stuck" after the fade finished.
Probably a bug with jquery + firefox 3.5 and not relating to Overlay code, but I wanted to point it out anyhow.
Comment #58
seutje CreditAttribution: seutje commented@57 yea that's a browser I think, what exact version are you using, coz I haven't seen this occur since Firefox/3.5.5
Comment #59
te-brian CreditAttribution: te-brian commented@seutje - Yep, 3.5.5. Probably also related to other performance problems people were having. 3.5.5 must have some JS leaks.
Comment #60
Manuel Garcia CreditAttribution: Manuel Garcia commentedTotally agree on the "Skip to main content" not being useful on the overlay, in fact, if you click it, the overlay is gone, this will create confusion.
I think I have found another useability problem/bug, in my opinion critical:
Steps to reproduce:
1. Go to create a page/article or whatever.
2. Fill in the title, Full text.
3. Have curiosity about what that "More information about text formats" help link does, and click it.
Result:
You are taken out of the overaly to that help page, resulting in the content you were creating lost, and no way to get it back.
This is something we should really fix. Not sure if I should open an issue for it or what, feedback is welcome! ---I am in the #drupal-usability channel for the sprint, hit me up!
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedThe attached patch completes the previously-described work to properly trigger an AJAX refresh when something in a "toolbar-like" region changes while the overlay is enabled. (I had hoped to finish it a week ago, but the time I had planned to spend on it last weekend was instead spent dealing with the aftermath of a broken-down car, and then after that came holiday craziness, etc...)
The patch seems to work fine for changes to the shortcut bar, toolbar menu, etc. Any remaining refreshes that don't work correctly are probably due to static caching bugs elsewhere in Drupal (with this patch, the overlay module now becomes a great test case for static caching issues!) or are extreme edge cases that can be dealt with later.
Overall, this should hopefully remove the last super-critical issue blocking the overlay patch.
(Note: This patch also contains a followup fix for #60552: Add region.tpl.php for all regions in themes since it needs that to work correctly, but I'll post that part as a separate patch there also.)
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commented@Manuel Garcia (in #60):
See #87994: Quit clobbering people's work when they click the filter tips link and #153313: ckeditor input is lost when using the browser's back button for closely related issues - a lot of this isn't specific to the overlay.
Depending on how those issues (particularly the first one) go, though, it might wind up making sense to have the filter tips help page display in the overlay.
By the way, this is yet another reason to be uneasy about the name of the hook_admin_paths() that this patch introduces. The requirements that the overlay has for what pages should display within it cannot simply be described as "administrative pages only".... Maybe hook_paths_which_are_related_to_editing_stuff_on_a_drupal_site()? :)
Comment #63
mfer CreditAttribution: mfer commentedHad a chance to start reviewing. There are some issues but it could use more eyes. I did not get very far. Leaving as Needs Review so others will poke at it.
Why can't the locations calling drupal_region_class($region) call drupal_html_class("region-$region")?
Why is a * used as the wild card rather than the % which is used in other places. This is inconsistent. Can we change it to %?
Why are mozilla and webkit specific border radius included and not border-radius itself? We should include that to be forward compatible.
There are a couple extra new lines that need to be removed.
This review is powered by Dreditor.
Comment #64
ksenzeeThanks for reviewing, mfer! The only comment I can speak to is
Why is a * used as the wild card rather than the % which is used in other places.
That's the format expected by drupal_match_path(), which then turns it into a regex. If it's that much of a WTF, we could do a str_replace on it, but that seems odd, since it's a Drupal API we're implementing. Might be better to make drupal_match_path() accept a %... but then we're messing with very old, very well tested, very widely implemented block visibility code.
David:
The requirements that the overlay has for what pages should display within it cannot simply be described as "administrative pages only"
I confess that I do not follow this line of reasoning. Why isn't filter tips considered administrative, if content creation is? And if content creation on a given site or for a given node type isn't to be administrative, neither it nor its filter tips page belongs in the overlay -- so kick it out by implementing hook_admin_paths() in contrib. Unless you're suggesting that we use the overlay as a generic popup API, which was discussed ad nauseam in the other issue...
Comment #65
mfer CreditAttribution: mfer commentedCan you explain why the setTimeout of 1ms is here for? This may have been written earlier and there may be a good reason. Just sniffing at what's going on.
Why is this wrapped in anonymous function? Won't setTimeout(self.close();, 1); work?
Again, is there a reason this is wrapped in an anonymous function?
Anonymous function again? Is there a reason?
Oh, again a few lines later.
Same thing.
Gotta give a shout out to Nathan Smith for taking a look at this part of it with me.
I'm on crack. Are you, too?
Comment #66
David_Rothstein CreditAttribution: David_Rothstein commentedThey could, but I don't think we should rely on string magic here.
Content creation is not considered administrative - we are just pretending it is for the purpose of this patch (because the overlay wants to display it using an administrative theme).
Comment #67
mgiffordJust wanted to repeat that for accessibility its important to make Overlay a user configurable feature. It will be great for many users but not all. Having a way to get directly to the actual admin page will be important for some admin users.
There's great work done on this, but it's a huge patch to review.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedWhat do you mean by "user configurable"?
It's already a module - you can turn it on and off at will. It also has a permission, so you can turn it on for some users but not others.
Comment #69
karschsp CreditAttribution: karschsp commentedThe patch in #61 applied successfully and I created an article in the overlay. I then edited the article, hit Save, and my changes weren't reflected. Once I reloaded the page, my changes were there. To confirm that this was an issue with overlay module and not browser or Drupal caching, I disabled the overlay, edited the article and my changes were reflected.
Upon re-enabling overlay, I got 2 shortcut bars (see screenshot). Logging out and logging back in did not remove the 2nd shortcut bar. In addition, once I had the double shortcut bars, none of the links in the toolbar worked (Dashboard, Content, Structure, etc).
I tested on Firefox 3.5.5 on Windows XP SP 3.
Comment #70
marcvangendI think that mgifford means that the module (if enabled, obviously) can be activated/deactivated on a per user basis.
Comment #71
mgiffordLast time I checked this (which wasn't that many patches ago) it was visible to user/1 if it was enabled. You could also assign it to a role.
Good to hear that this is an optional module. If it is enabled though the problems aren't going to generally be per role as much as per-user.
You may want to enable it for all content-editors. However, generally it's on a user level that there are problems. Either the user has a disability, uses a non-standard browser or just is running into resource issues with their computer. They aren't going to say "Dear Admin, can you set up a special role for me so that I am not going to run into problems editing the content" and the admin isn't going to see that either.
However, like with enabling/disabling defaults for the wysiwyg editor on a user basis, they (or an admin on their behalf) can disable the overlay for this individual. As I said up in comment #55, there should either be a way to access the content if iframes aren't loading properly, and likely instructions to disable the Overlay if it isn't displaying within the iframe. I do think this should be presented on a user level.
Comment #72
webchickSo. The last time I gave this issue a good poke was feature freeze, about a month ago. It's come a long way since then! Holy cow!
Improvements since last I looked:
* This code's documentation and coding standards compliance totally exceeds standard webchick-approved level. :) Great job!
* Rather than doing weird path munging stuff in JS, there's now a proper hook for this: hook_admin_paths() (and corresponding alter hook). This information can then be re-used by other modules for their own nefarious purposes.
* OMG THE BACK BUTTON WORKS!
* Shortcuts are added/removed when I click the buttons in the overlay.
* And probably much more... sorry, I'm pretty tapped today.
I'm pretty useless for reviewing the CSS/JS in this patch with any degree of credibility. That's totally awesome that mfer and Nathan Smith reviewed it though. I'd defer to their comments, but they mostly seemed of the clean-up variety, not the "OMG release blocker" variety.
There are three things about this patch that still make me scratch my head and go "Hmm..." though:
1. toolbar_system_info_alter() and shortcut_system_info_alter() are strange. I get that those are exposing custom regions and that's why you put the code in there, but it's doing an explicit check for module_exists('overlay'), which prevents that logic from working in "better_overlay" module in contrib. It feels architecturally cleaner to move the "module_exists" stuff into overlay module instead.
2. I'm still seeing little weird edge-case bugs. For example, if I select "delete" from the contextual links on a node, I'm given the confirm form in the overlay, but in my front-end theme. When I clear cache and hit the back button, I'm taken to a 404 page not found. When I click Add content, then Find content, then hit the back button, my active shortcut link doesn't change back to Add content again. There's also the bug that karschsp found.
3. Last, but certainly not least, we've made huge strides to make D7 the most accessible open source CMS out there, so when mgifford raises accessibility concerns, we definitely need to sit up and take notice. I worried that a blind user would get into the situation of instaling D7 with the default profile, attempt to access an administration page and be unable to navigate it, and then be totally screwed because there would be no way to turn the module off at this point. :( However, ksenzee found Everett's comments in the old issue at http://drupal.org/node/517688#comment-2009406 that the accessibility has been drastically improved, so I'm not currently sure where we stand on this. It sounds like mgifford's recent comments might be around more of the "crappy browser on slow Internet" accessibility variety.
Of those, 1 is an open question that can be dealt with post-commit if necessary, 2 is only going to get worse the longer that this sits here not being committed, where it can then finally be tested and hammered on by 800 core developers and instead the ~10 or so on this issue, but on 3 I really would like us to do whatever we can to help accessibility, so I'm a bit torn on how to handle it...
On the one hand, committing this patch before all accessibility concerns are resolved can send the wrong message; that accessibility work is "clean-up" work after the fact, rather than a fundamental part of our development process. However, a) it appears that Everett's earlier concerns have been addressed, making this remaining accessibility more of a convenience/preference thing? b) this issue is basically at 500+ comments at this point, counting all the various sub-issues, and adding another 100+ (along with the re-rolls of this huge patch) to solve this is not ultimately sustainable, and c) once this patch is committed, it becomes much easier to get many more people working on this important issue since it's more "in your face," if you will.
== SUMMARY, SINCE I TALK TOO MUCH ==
Therefore, weighing all the various pros and cons, and now donning my "release manager" hat, I've decided to commit this patch as-is, so we can use it as a basis for further refinements. We've seen a pattern emerge over and over again in core development with these sorts of "mega" patches that once they're committed, they're a lot less intimidating for folks to pick off little things here and there. We can even take small issues and tag them as "Novice" to get new contributors involved, which makes them win-win. :) And while giving this patch an extra month resulted in tons of really great architectural improvements, it didn't result in nearly as many bug fixes, so leaving it in the queue another 2-3 weeks seems like it will only compound this issue.
So, with a desperate, pleading look in my eyes directed to the patch authors to please work with the accessibility team to help resolve any remaining accessibility issues ASAP, Committed this baby to HEAD, Ooooh yeaaaahhh....! Please open separate issues for further refinement against the overlay.module component.
We need the admin path hooks documented at the module upgrade guide under UNSTABLE-11, and also an issue cross-posted to the Coder module issue queue in the "Upgrade Rules" component.
Also, here are a few comments I picked out as I was reviewing the patch, just so they're noted somewhere.
And revisions?
What's this mean?
aren't you missing a #value here..?
Why are these hunks in their respective modules and not overlay module?
I'm on crack. Are you, too?
Comment #73
Dave ReidSee followup JS bug with #648630: JavaScript failure in overlay-child.js
Comment #74
Linea CreditAttribution: Linea commentedsubscribe
Comment #75
carlos8f CreditAttribution: carlos8f commentedI haven't been following the overlay development 'till now, but two issues I've seen with the committed version so far:
- Overlays don't load at all in the latest Google Chrome or Safari browsers -- the white popop is stuck on the loading gif. The overlay title loads, however.
- In IE8 and Opera 9/10, the overlay links are absent (with the "#" in the path) except on the /admin path. Clicking on "Administer" on the seven breadcrumb causes the overlay to work, but doesn't persist when other things are clicked. Curious.
Not sure if either one of these necessitate follow-up issues, or if it's already in the issue queue, so just documenting this here for now.
Nice work on this, guys! :P
Comment #76
Morbus IffI can confirm #75 - just did a fresh checkout/install five minutes ago and couldn't accomplish anything via admin. Had to manually type in URL for modules and disable the Overlay module.
Comment #77
ksenzee@carlos8f - thanks for the report. Would you create a new issue for the IE8/Opera problem? (We have a report on the Chrome/Safari thing.) I'm going to mark this issue fixed and direct everyone to the overlay.module issue queue. Thanks everyone!
Comment #78
David_Rothstein CreditAttribution: David_Rothstein commentedWe need this open for the documentation - which I'm happy to do, but want to make sure we reconsider the hook_admin_paths() name first...
More later :)
Comment #79
ksenzeeAt this point it's too late to do anything else with the hook for D7, so you can change it to hook_overlay_paths() as far as I'm concerned. I maintain that we should be keeping this information in one central place, but that's a D8 issue.
Comment #80
carlos8f CreditAttribution: carlos8f commented@ksenzee: see my issue and patch -- #650758: Overlay bug: admin links aren't transformed to overlay paths in IE/Opera.
Comment #81
RobLoachThe hook should not be
hook_admin_paths()
. That hook is pretty unique to the Overlay, and should be documented in overlay.api.php. We want to avoid coercion or intruding on the hook_admin* namespace. Hold on a second, hook_admin_paths, am I missing something here? :-)Comment #82
webchickThe idea behind not making it hook_overlay_X is so that other modules that are not overlay could benefit from Drupal's modules explaining what their admin paths are. This information could potentially be re-used elsewhere in contrib.
Please start another issue and cross-link here if you want to discuss this more in-depth.
Comment #83
David_Rothstein CreditAttribution: David_Rothstein commentedPossible reason for keeping the discussion here: So that we don't wind up documenting it first and then re-documenting a new one.....
But actually, maybe hook_admin_paths() isn't so bad as long as we explain it correctly? The confusion is that there are several different ways Drupal categorizes pages as "admin" or not - e.g. deciding what pages the administrative theme applies to under regular conditions is (and must be) different than what the overlay uses. But, if we clarify that this hook is not really supposed to be about putting pages in definitive administrative vs non-administrative bins, but rather about tagging any page that has a little bit of "administrative-ness" to it, maybe it's OK.
If that sounds right, I'll plan to file a separate issue for improving the API docs around this hook and leave it at that.
By the way, thanks for another mega-review, @webchick! Hopefully this overlay thing turns out to be worthwhile... Look for some more exciting followup issues over the next few days.
Comment #84
David_Rothstein CreditAttribution: David_Rothstein commentedThe new hooks are now documented at http://drupal.org/node/224333#hook_admin_paths
I also created an issue for improving the API docs for these functions along the lines of my comment above: #651586: Improve the API documentation for hook_admin_paths()-related functions
Still leaving this at "needs work" until we've had time to go through the above comments and make sure there are corresponding issues in the queue (especially for some of the bigger overarching ones that @webchick raised). But as mentioned earlier, please file any new problems that you find as separate issues against the overlay.module component, rather than here.
Comment #85
David_Rothstein CreditAttribution: David_Rothstein commentedI just finished a big cleanup of the overlay module queue, and also took some notes from the comments above about which new issues still need to be filed (that don't duplicate ones that were filed before).
Ran out of steam to actually file those issues, but I think we should be able to get that done and this issue closed sometime tomorrow.
Comment #86
webchickThanks very much, David! Really appreciate you doing that.
Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedOK, that took longer than expected, but I think this is done now. At the bottom of this post are a list of issues I filed (some with patches, most without) based on comments above in this issue. I created an issue for everything I saw above that still seemed valid and did not duplicate something already filed elsewhere (so search the whole overlay module issue queue if you want to make sure that there is a followup for the issue you reported).
Also, responding to some of @webchick's major questions:
Right, this was tricky - but if it moves to the overlay module then the overlay module would have to "know" that the features provided by those modules exist, which it can't do for contrib modules...
I think we can just remove the module_exists() check but leave the rest where it is. See #655736: Other modules are not made aware of core page elements that are intended to display in an overlay.
Interestingly, there are three modules in core that do this kind of thing (yes, in addition to toolbar and shortcut, the dashboard module also does it, but the overlay-specific code in that module seems to have been committed months ago, way before overlay was committed - huh???). Of those:
I have filed #655526: Make sure that the overlay is accessible as a critical meta-issue to track this, but yeah, my understanding was that the accessibility was pretty good already. People should follow up on that issue if not.
A per-user toggle seems like an interesting contrib feature in its own right, but seems a bit unusual (although not unprecedented) for core - and if we put it in to "address" accessibility concerns then it's not so much addressing them as avoiding them. A blind person should absolutely be able to use the overlay if they choose to; although we tend to think of the overlay as a visual feature only, it's not.
It means that first #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed has to be committed, then followed up by a patch at #615138: Some pages display in the overlay in a non-adminstrative theme.
By the way, I think this is the cause of one of your other comments above, that when you delete a node the confirm form appears in Garland rather than Seven.
**********
In conclusion, here is the complete list of issues I filed as followups based on all the above comments:
#655368: Editing a shortcut causes it to jump to the end of the list
#655376: Node previews are messed up in the overlay
#655388: Many ways to lose data on form input in the overlay
#655416: "Demonstrate block regions" bugs with regions, navigation, overlay
#655448: Horizontal scrollbar shows when overlay is enabled and screen is resized
#655464: Add a "Help on" toggle to the overlay (for D8)
#655470: "Add to shortcuts" button flashes briefly on the page before jumping out to above the overlay
#655490: The back button does not change the active shortcut link (when overlay is being used)
#655492: Clearing cache and hitting the back button in the overlay gives a 404 error
#655508: Certain node-related pages aren't defined in hook_admin_paths() when they should be
#655514: Usability issues with overlay close button
#655518: Tabs do not appear correctly in the overlay in IE6
#655526: Make sure that the overlay is accessible
#655542: Links in overlay on Firefox 3.5 are half-rendered after they fade in
#655562: Use border-radius throughout core in addition to Mozilla and Webkit-specific properties
#655600: Small Code cleanups in overlay module
#655614: Changes made by submitting a form in the overlay are not reflected when the window automatically closes
#655722: Changes made in an overlay session are not reflected when the user closes the overlay
#655736: Other modules are not made aware of core page elements that are intended to display in an overlay
#655740: Fix small JavaScript issues in the overlay module
#655746: Display bugs with the overlay on Safari 4
Comment #88
seutje CreditAttribution: seutje commentedholy ass that huge list just made me cry q.q
Comment #89
Alex UA CreditAttribution: Alex UA commentedDavid_Rothstein created an issue to discuss whether overlay should be removed from core:
#659488: Properly test the overlay to determine if it belongs in core or contrib
Comment #91
sheldon.nelson CreditAttribution: sheldon.nelson commentedIs there a way to prevent the overlay from leaving the "#" when closed which forces the parent window to go to the top?
Comment #92
ksenzeeThe overlay has been in Drupal 7 core for a couple of years now. You can file an issue against it at http://drupal.org/node/add/project-issue/drupal (choose the "overlay.module" component).