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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue tags: +overlay
FileSize
58.1 KB

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.

effulgentsia’s picture

Status: Active » Needs work

really, really setting to "needs work" this time.

seutje’s picture

subscribe

sun’s picture

We need a thorough summary here.

markus_petrux’s picture

subscribing

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

On it.

Gábor Hojtsy’s picture

FileSize
3.95 KB

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.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

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

  • D1: $custom_theme is used at many places but not available anymore; some non-admin pages will need theme switching, see #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed which first needs to provide an API for us to use
  • D2: The overlay JS library links to the "old" overlay issue as website URL
  • 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 markup
  • D4: @webchick suggested to rename the overlay_mode() function to overlay_set_mode(); which then might be accompanied by an overlay_get_mode() I guess
  • D5: inline comments in overlay_block_info_alter() about what happens are not up to date
  • D6: overlay-displace-top was implemented but not overlay-displace-bottom, so stuff displayed at the bottom (like the contrib l10n_client module) will roll above or behind the toolbar possibly
  • D7: .to-overlay is not used anymore as a trigger to open the overlay but is kept in toolbar module still; our patch should remove that obsolete code
  • D8: When a page (re)loads its content without going through an actual <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.
  • A2: @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 default
  • A3: 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 pressing

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 used
  • W2: node previews are not working, we do not have a clear model for previews anyway, when people need to break out of the overlay and come back in to continue editing
  • W3: it is very easy to loose a long typed in node by hitting ESC (we've been on and off with a JS alert for this, which was very obtrusive), @leisa mentions in the age of autosave, people will not expect to loose that long-typed content
  • W4: block region demos at admin/structure/block/demo/* show pages themed in the overlay with a non-admin theme; should probably open in the background page; BUT then how could one get back to configure blocks? Uhm.
  • W5: 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)
  • W6: 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).
  • W7: 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.

Styling

  • S1: the tabs appear 1px off from how they were designed in Safari 4, not consistently looking like on mocks anyway
  • S2: Bojhan and JuliaKM could consistently reproduce horizontal scrollbars with the Appearance screen, even on 1280x1024
  • S3: @webchick notes horizontal scrollbars are visible when the background page(!) is wider then your screen width; although overlay is sized down, the parent page enforces a horizontal scrollbar, which could be quite misleading. See first image at http://drupal.org/node/517688#comment-2170538
  • S4: the overlay gradually becomes less wide as different pages are clicked (not consistently but easy to reproduce with the top IA items) in Safari 4
  • S5: we need to fix how the size of the iframe is recomputed when navigating to a new page inside the overlay. Currently, the iframe container collapse itself for a brief moment before the content of the target page is loaded and processed. This even has its own issue at #574164: Fix transitions between pages inside the overlay.
  • S6: at one point, we used to have a rounded close button without a drop shadow; we ended up with the image with drop shadow, although only compatible browsers will only display the drop shadow on the rest of the items, so the close button would stand out; we probably want to back out to the image and technique explained in http://drupal.org/node/517688#comment-2021274
  • S7: for @Bojhan, overlay scrolling pushes off the toolbar on Windows Vista, Firefox 3.5 (on a powerful machine): http://www.screencast.com/users/Bojhan/folders/Jing/media/0decb1c8-87d8-... this was reproduced by @mcrittenden on Ubuntu as well
  • S8: "Add to shortcuts" button positioning way off compared to mockups (it would be vertically centered to the title ideally)

Missing features

  • F1: A "Help: On" toggle was designed but not implemented (yet?)
  • F2: Shortcutting pages with query string arguments (such as sorted lists or lists filtered via GET parameters) is not working at all (due to the generic fix we put in to remove the render=overlay argument by removing all query strings params)
  • F3: not possible to bookmark or permalink admin pages with the overlay; see also W1

JavaScipt

  • 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-1813954
  • J2: $('a:not(.overlay-exclude)') does not use the context, should use it!
  • J3: @sun had performance constraints with the $('a:not(.overlay-exclude)') matching of all links (ala Google Analytics), but no better ideas were raised
  • J4: multiple people including @webchick noted sticky table headers sticking to page top for a little before they jump into their place; cannot reproduce this anymore, but could not reproduce then either
  • J5: @RobLoach says self.isOpen || $('#overlay-container').size() could be re-coded with jQuery.once()
  • J6: @Damien Tournoud and @sun said markup generated by JS should go through Drupal.theme()
  • J7: @RobLoach says self.processed in overlay-child.js can be jQuery once-d with $('body').once('overlay', function() { ... });
  • J8: If shortcut bar was not open in toolbar and overlay was open, if you open the shortcut bar, it goes above the overlay
  • J9: "Add to shortcuts" button flashes briefly before jumping out to above the overlay; should be hidden in overlay via CSS and moved/displayed above when loaded, so it will not show at original place
  • J10: 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-2171332
  • J11: isAdminLink() using JS regexes was deemed ghetto; @David_Rothstein has a good (@webchick approved) roadmap at http://drupal.org/node/517688#comment-2171822

Usability

  • U1: close button is at top, so it might be hard to close the overlay when in a tall admin page (unless you know you can use ESC); usability testing also found the close button is not evident to at least one users (possibly not conclusive), see http://drupal.org/node/517688#comment-1900132
  • U2: hitting "+ Customize" on the dashboard screen scrolls the screen down to align the top of avilable blocks to screen top, so the top of the overlay is not visible; bad!
  • U3: none of the user menus work in the toolbar (Hello username and Log out) when the overlay is on

Performance

  • P1: To follow resizing of the internal contents (fieldsets, etc), Charlie added a 150ms setTimeout, which might be in part causing the bloddy performance in Firefox when the overlay is open; Charlie admits this is not elegant but nobody had better ideas ever since; more info on http://drupal.org/node/517688#comment-1906928
  • P2: @webchick, @sun, @David_Rothstein, myself and others noted that performancy is *very bad* in Firefox 3.5 regardless of OS
  • 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)

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.

RobLoach’s picture

I love how we're cropping the patch up into different issues now. Subscribing.

drifter’s picture

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.

mcrittenden’s picture

Subscribing. Awesome work, all!

mcrittenden’s picture

mcrittenden’s picture

marcvangend’s picture

subscribing

David_Rothstein’s picture

P2: @webchick, @sun, @David_Rothstein, myself and others noted that performance is *very bad* in Firefox 3.5 regardless of OS

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?

eigentor’s picture

sub-bass

ksenzee’s picture

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.

ksenzee’s picture

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.

ksenzee’s picture

ksenzee’s picture

ksenzee’s picture

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.

David_Rothstein’s picture

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:

  1. The URL fragments and back button are working beautifully! I found one small issue (which I couldn't even reproduce) where hitting the back button a few times to back out of the overlay and then hitting the forward button to go back in didn't work, but that's very minor. Overall, this is a huge improvement - the overlay is now actually usable, and as an added bonus, you can even bookmark pages in it, etc.
  2. Unfortunately, you can't install Drupal when the overlay is enabled in your install profile, due to this code:
    function 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.

  3. The positioning of the overlay doesn't seem to be correct anymore - i.e., the tabs are not showing up, and sometimes it appeared underneath or above the toolbar, etc.
  4. The overlay does not work correctly with contextual links and in particular the drupal_get_destination() part of it. From the "main site", clicking on a contextual link (e.g., 'configure block'), doing your thing in the overlay, and then hitting the submit button does not take you back to where you were directly, but rather loads a copy of where you were inside the overlay.
  5. Also with contextual links, I found the following weird bug:
    • Go to the dashboard (inside the overlay).
    • Click on "configure block" for one of the blocks.
    • Make changes and submit - you are correctly taken back to the dashboard.
    • Now click the same "configure block" link again - it won't let you. Probably has something to do with the URL fragments not getting set correctly.
  6. The #id fragments on the permissions page do not work correctly in the overlay. So for example, if you click on "Help" in the toolbar, then "Taxonomy", then "Configure permissions", it takes you to the permissions page and then seems to flash you down to the correct (taxonomy) portion of the page, but then jumps you right back to the top of the page a second later. I assume this happens when the URL fragment parsing code kicks in, but not sure.
ksenzee’s picture

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 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?

SeanBannister’s picture

sub

Gábor Hojtsy’s picture

FileSize
45.04 KB
48.29 KB

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.

Gábor Hojtsy’s picture

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.

David_Rothstein’s picture

I can't reproduce 3 anymore at all. I can still reproduce 6.

ksenzee’s picture

Status: Needs work » Needs review
FileSize
104.68 KB

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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

David_Rothstein’s picture

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.

  1. 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?

    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.

  2. From Gábor:

    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.

    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.

  3. I am noticing that using hook_admin_paths_alter() is going to be a bit of a pain here, since what you'll receive to work with is just one big giant array of values - i.e., removing something from that array requires more complicated PHP functions than one would like to use. Whereas if the values were keys instead, you could just use unset().

    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:

    function 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' => 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.

  4. Overall, I think the only thing that is really worrying me right now is that code like this appears many times throughout the patch:
    +  // 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:

    /**
     * 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.

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Updated status on the different items based on feedback from Katherine:

Drupal/API issues

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 permission
  • A2: @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 default not being fixed, the overlay already has multiple levels of abstraction below it
  • A3: 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 pressing converted 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 used fragments implemented via #617372: Overlay W1 / F3: fix the back button and allow permalinks
  • W2: node previews are not working, we do not have a clear model for previews anyway, when people need to break out of the overlay and come back in to continue editing; to be a separate issue post-commit
  • W3: it is very easy to loose a long typed in node by hitting ESC (we've been on and off with a JS alert for this, which was very obtrusive), @leisa mentions in the age of autosave, people will not expect to loose that long-typed content; to be a separate issue post-commit
  • W4: block region demos at admin/structure/block/demo/* show pages themed in the overlay with a non-admin theme; should probably open in the background page; BUT then how could one get back to configure blocks? Uhm.; to be a separate issue post-commit
  • W5: 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 David
  • W6: 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 W5
  • W7: 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.

Missing features

  • F1: A "Help: On" toggle was designed but not implemented (yet?); to be a separate issue
  • F2: Shortcutting pages with query string arguments (such as sorted lists or lists filtered via GET parameters) is not working at all (due to the generic fix we put in to remove the render=overlay argument by removing all query strings params); to be a separate issue
  • F3: not possible to bookmark or permalink admin pages with the overlay; see also W1 fixed via using fragments, see #617372: Overlay W1 / F3: fix the back button and allow permalinks

JavaScipt

  • 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-1813954 solved
  • J2: $('a:not(.overlay-exclude)') does not use the context, should use it! does not exist anymore
  • J3: @sun had performance constraints with the $('a:not(.overlay-exclude)') matching of all links (ala Google Analytics), but no better ideas were raised does not apply anymore due to how link matching changed; the current filtering based link matching might have similar issues though
  • J4: multiple people including @webchick noted sticky table headers sticking to page top for a little before they jump into their place; cannot reproduce this anymore, but could not reproduce then either; to be a separate issue
  • J5: @RobLoach says self.isOpen || $('#overlay-container').size() could be re-coded with jQuery.once() too late to redo
  • J6: @Damien Tournoud and @sun said markup generated by JS should go through Drupal.theme() done
  • J7: @RobLoach says self.processed in overlay-child.js can be jQuery once-d with $('body').once('overlay', function() { ... }); too late to redo
  • J8: If shortcut bar was not open in toolbar and overlay was open, if you open the shortcut bar, it goes above the overlay solved, overlay moves as toolbar is collapsed/expanded
  • J9: "Add to shortcuts" button flashes briefly before jumping out to above the overlay; should be hidden in overlay via CSS and moved/displayed above when loaded, so it will not show at original place
  • J10: 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-2171332 done
  • J11: isAdminLink() using JS regexes was deemed ghetto; @David_Rothstein has a good (@webchick approved) roadmap at http://drupal.org/node/517688#comment-2171822 was resolved with a PHP Drupal hook

Usability

  • U1: close button is at top, so it might be hard to close the overlay when in a tall admin page (unless you know you can use ESC); usability testing also found the close button is not evident to at least one users (possibly not conclusive), see http://drupal.org/node/517688#comment-1900132 to be handled in followup
  • U2: hitting "+ Customize" on the dashboard screen scrolls the screen down to align the top of avilable blocks to screen top, so the top of the overlay is not visible; bad!
  • U3: none of the user menus work in the toolbar (Hello username and Log out) when the overlay is on fixed

Performance

Browser compatibility

Gábor Hojtsy’s picture

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: 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

Gábor Hojtsy’s picture

Video of the latest incarnation of the overlay as in the patch above in #28.

Drupal 7 overlay from Gábor Hojtsy on Vimeo.

webchick’s picture

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? ;)

Gábor Hojtsy’s picture

@webchick: the latest patch is still on #28.

seutje’s picture

Gábor Hojtsy’s picture

@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.

ksenzee’s picture

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.

webchick’s picture

Go back to vacation, ksenzee! *whpssh!* ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Test bot running amok.

ksenzee’s picture

FileSize
78.47 KB

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.

ben_alman’s picture

Sweet, happy birthday me!! :-)

webchick’s picture

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.

sun’s picture

Handling 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.

ksenzee’s picture

FileSize
79.73 KB

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: 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.)

Gábor Hojtsy’s picture

@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).

ksenzee’s picture

FileSize
79.73 KB

Chasing HEAD.

mgifford’s picture

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.

David_Rothstein’s picture

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.

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?

It looks sexy, but seems to load slower in my browser (maybe I've got too many tabs open).

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:

function 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....

mgifford’s picture

@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.

seutje’s picture

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

mgifford’s picture

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.

mcrittenden’s picture

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 :)

te-brian’s picture

FileSize
68.8 KB

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.

seutje’s picture

@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

te-brian’s picture

@seutje - Yep, 3.5.5. Probably also related to other performance problems people were having. 3.5.5 must have some JS leaks.

Manuel Garcia’s picture

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!

David_Rothstein’s picture

FileSize
92.61 KB

The 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.)

David_Rothstein’s picture

@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()? :)

mfer’s picture

Had 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.

+++ includes/common.inc	29 Nov 2009 02:45:34 -0000
@@ -3591,6 +3591,28 @@ function drupal_html_id($id) {
+function drupal_region_class($region) {
+  return drupal_html_class("region-$region");
+}

Why can't the locations calling drupal_region_class($region) call drupal_html_class("region-$region")?

+++ modules/node/node.module	29 Nov 2009 02:45:34 -0000
@@ -242,6 +242,20 @@ function node_field_build_modes($obj_typ
 /**
+ * Implement hook_admin_paths().
+ */
+function node_admin_paths() {
+  $paths = array(
+    'node/*/add' => TRUE,
+    'node/*/edit' => TRUE,
+    'node/*/delete' => TRUE,
+    'node/add' => TRUE,
+    'node/add/*' => TRUE,
+  );
+  return $paths;
+}

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 %?

+++ modules/overlay/overlay-parent.css	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,130 @@
+  -moz-border-radius-topleft: 0;
+  -webkit-border-top-left-radius: 0;

Why are mozilla and webkit specific border radius included and not border-radius itself? We should include that to be forward compatible.

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+    .add('.overlay-displace-bottom a', context)
+    .click(function () {
+      window.location.href = this.href;
+    });
+
+
+    // Resize the overlay when the toolbar drawer is toggled.
+    $('#toolbar a.toggle', context).once('overlay').click(function () {
+      setTimeout(function () {
+        Drupal.overlay.resize(Drupal.overlay.iframe.documentSize);
+      }, 150);
+
+    });

There are a couple extra new lines that need to be removed.

This review is powered by Dreditor.

ksenzee’s picture

Thanks 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...

mfer’s picture

+++ modules/overlay/overlay-child.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,153 @@
+      setTimeout(function () {
+        // We need to store the parent variable locally because it will
+        // disappear as soon as we close the iframe.
+        var p = parent;
+        p.Drupal.overlay.close(settings.args, settings.statusMessages);
+        if (typeof settings.redirect == 'string') {
+          p.Drupal.overlay.redirect(settings.redirect);
+        }
+      }, 1);

Can 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.

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+      setTimeout(function () { self.close(); }, 1);

Why is this wrapped in anonymous function? Won't setTimeout(self.close();, 1); work?

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+        setTimeout(function () { $target.focus(); }, 10);

Again, is there a reason this is wrapped in an anonymous function?

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+            setTimeout(function () { $closeButton.focus(); }, 10);

Anonymous function again? Is there a reason?

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+            setTimeout(function () { $closeButton.focus(); }, 10);

Oh, again a few lines later.

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+          setTimeout(function () { self.close(); }, 10);

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?

David_Rothstein’s picture

Why can't the locations calling drupal_region_class($region) call drupal_html_class("region-$region")?

They could, but I don't think we should rely on string magic here.

I confess that I do not follow this line of reasoning. Why isn't filter tips considered administrative, if content creation is?

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).

mgifford’s picture

Just 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.

David_Rothstein’s picture

Just wanted to repeat that for accessibility its important to make Overlay a user configurable feature.

What 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.

karschsp’s picture

FileSize
47.31 KB

The 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.

marcvangend’s picture

I think that mgifford means that the module (if enabled, obviously) can be activated/deactivated on a per user basis.

mgifford’s picture

Last 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.

webchick’s picture

Status: Needs review » Needs work
Issue tags: +Needs documentation

So. 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.

+++ modules/node/node.module	29 Nov 2009 02:45:34 -0000
@@ -242,6 +242,20 @@ function node_field_build_modes($obj_typ
+  $paths = array(
+    'node/*/add' => TRUE,
+    'node/*/edit' => TRUE,
+    'node/*/delete' => TRUE,
+    'node/add' => TRUE,
+    'node/add/*' => TRUE,
+  );
+  return $paths;

And revisions?

+++ modules/overlay/overlay.module	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,777 @@
+  // @todo: custom_theme does not exist anymore.
+  global $custom_theme;

What's this mean?

+++ modules/overlay/overlay.module	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,777 @@
+  if (overlay_get_mode() == 'child') {
+    $form['overlay_parent_url'] = array(
+      '#type' => 'hidden',
+    );
+  }

aren't you missing a #value here..?

+++ modules/shortcut/shortcut.module	29 Nov 2009 02:45:34 -0000
@@ -603,3 +604,16 @@ function shortcut_preprocess_page(&$vari
+/**
+ * Implements hook_system_info_alter().
+ *
+ * If the overlay module is enabled, indicate that the link for adding or
+ * removing shortcuts is one of the page "regions" that should display in the
+ * overlay.
+ */
+function shortcut_system_info_alter(&$info, $file, $type) {
+  if (module_exists('overlay') && $type == 'theme') {
+    $info['overlay_regions'][] = 'add_or_remove_shortcut';
+  }
+}

+++ modules/toolbar/toolbar.module	29 Nov 2009 02:45:34 -0000
@@ -135,6 +135,19 @@ function toolbar_preprocess_html(&$vars)
 /**
+ * Implements hook_system_info_alter().
+ *
+ * If the overlay module is enabled, indicate that the 'page_top' region (in
+ * which the toolbar will be displayed) is one of the overlay supplemental
+ * regions that should be refreshed whenever its content is updated.
+ */
+function toolbar_system_info_alter(&$info, $file, $type) {
+  if (module_exists('overlay') && $type == 'theme') {
+    $info['overlay_supplemental_regions'][] = 'page_top';
+  }
+}

Why are these hunks in their respective modules and not overlay module?

I'm on crack. Are you, too?

Dave Reid’s picture

Linea’s picture

subscribe

carlos8f’s picture

I 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

Morbus Iff’s picture

I 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.

ksenzee’s picture

Status: Needs work » Fixed

@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!

David_Rothstein’s picture

Status: Fixed » Needs work

We 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 :)

ksenzee’s picture

At 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.

carlos8f’s picture

RobLoach’s picture

The 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? :-)

webchick’s picture

The 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.

David_Rothstein’s picture

Possible 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.

David_Rothstein’s picture

Issue tags: -Needs documentation

The 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.

David_Rothstein’s picture

I 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.

webchick’s picture

Thanks very much, David! Really appreciate you doing that.

David_Rothstein’s picture

Status: Needs work » Fixed

OK, 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:

  • 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.

    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:

    1. The shortcut code will hopefully be removed entirely via #646874: Modules like Contextual links and Shortcut cannot add to "template regions".
    2. The toolbar code might be removed entirely via #655722: Changes made in an overlay session are not reflected when the user closes the overlay.
    3. So with the exception of dashboard (which is a very odd edge-case) the whole problem might go away anyway :)
  • 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.

    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.

  • +++ modules/overlay/overlay.module	29 Nov 2009 02:45:34 -0000
    @@ -0,0 +1,777 @@
    +  // @todo: custom_theme does not exist anymore.
    +  global $custom_theme;
    

    What's this mean?

    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

seutje’s picture

holy ass that huge list just made me cry q.q

Alex UA’s picture

David_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

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sheldon.nelson’s picture

Status: Closed (fixed) » Needs work

Is there a way to prevent the overlay from leaving the "#" when closed which forces the parent window to go to the top?

ksenzee’s picture

Status: Needs work » Closed (fixed)

The 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).