Overlay implementation
effulgentsia - October 21, 2009 - 03:54
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | overlay.module |
| Category: | feature request |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | accessibility, needs accessibility review, overlay |
Description
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.
#1
And 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.
#2
really, really setting to "needs work" this time.
#3
subscribe
#4
We need a thorough summary here.
#5
subscribing
#6
On it.
#7
Still 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.
#8
You'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.
#9
I love how we're cropping the patch up into different issues now. Subscribing.
#10
Awesome 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.
#11
Subscribing. Awesome work, all!
#14
subscribing
#15
I 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?
#16
sub-bass
#17
I'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.
#18
I'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.
#19
Issue D4: #615114: Overlay D4: rename overlay_mode() to overlay_set/get_mode()
#20
P2: #615130: Overlay P2: Very bad performance
I copied David's comments at #15 into the issue. (Thanks David!)
#21
On 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.#22
I 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:
<?phpfunction overlay_enable() {
drupal_goto('<front>', array('fragment' => 'overlay=admin/config/modules'));
}
?>
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.
#23
Thanks 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: Existing URL fragments are broken. 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?
#24
sub
#25
Tested 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.
#26
Found 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.
#27
I can't reproduce 3 anymore at all. I can still reproduce 6.
#28
This 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.
#29
The 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.
#30
Quick 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
#31
Here'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:
<?phpfunction node_admin_paths() {
$paths = array(
'node/*/add' => TRUE,
'node/*/edit' => TRUE,
);
}
?>
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' => FALSEin 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.
+ // If the overlay module is enabled, let it know that the shortcut bar needs+ // to be refreshed on the next page load.
+ if (module_exists('overlay')) {
+ overlay_request_toolbar_refresh('#toolbar', 'page_top/toolbar');
+ }
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:
<?php/**
* Implement hook_exit().
*/
function overlay_exit() {
// If the current user is viewing the overlay, then, for each 'refreshable' region of
// the page, call drupal_render() on it and compare to what we had earlier in the
// page request - which is the last thing the user actually saw. If anything changed,
// that means this region was modified as a result of the current page request, so
// call overlay_request_toolbar_refresh() on it *from inside here*, thereby forcing
// the next page load to refresh it for the user.
}
?>
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.
#32
@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.
#33
Updated 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
#34
There 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: Allow modules to specify per-page custom themes in hook_menu() is required and dead code in the patch marked with @todos need to be fixed to depend on it, see #615138: Overlay D1: $custom_theme is borked
#35
Video of the latest incarnation of the overlay as in the patch above in #28.
Drupal 7 overlay from Gábor Hojtsy on Vimeo.
#36
Hot 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? ;)
#37
@webchick: the latest patch is still on #28.
#38
don't forget the images from Initial D7UX admin overlay comment #190
#39
@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.
#40
Let 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.
#41
Go back to vacation, ksenzee! *whpssh!* ;)
#42
The last submitted patch failed testing.
#43
Test bot running amok.
#44
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 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.
#45
Sweet, happy birthday me!! :-)
#46
Hey, 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.
#47
Handling of "administrative themes" on non-admin paths is already dealt with in #553944: Allow modules to specify per-page custom themes in hook_menu().
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.
#48
If 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: Figure out what to do about shortcut links with query parameters. 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.)
#49
@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).
#50
Chasing HEAD.
#51
Ok, 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.
#52
There 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 P2: Very bad performance.
***
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:
<?phpfunction overlay_init() {
...
// If this is a POST request, or a GET request with a token parameter, we
// have an indication that something in the supplemental regions of the
// overlay might change during the current page request. We therefore
// store the initial rendered content of those regions here, so that we
// can compare it to the same content rendered in hook_exit(), at the end
// of the request. This allows us to check if anything actually did
// change, and, if so, trigger an AJAX refresh of the parent window.
if (!empty($_POST) || isset($_GET['token'])) {
$supplemental_regions = module_invoke_all('overlay_supplemental_regions');
if (!empty($supplemental_regions)) {
$markup = overlay_render_regions($supplemental_regions);
overlay_store_rendered_content($markup);
}
}
?>
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....
#53
@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.
#54
there 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
#55
It 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
<iframe src="/node/2/edit"><p>If you can see this text, your browser does not support iframes.
<a href="/node/2/edit">View the content of this inline frame</a> within your browser.</p>
</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.
#56
Anyone 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 :)
#57
In 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.
#58
@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
#59
@seutje - Yep, 3.5.5. Probably also related to other performance problems people were having. 3.5.5 must have some JS leaks.
#60
Totally 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!