Attached is a first version of the D7UX overlay we've been working hard for some time. Specs for the overlay was that it would provide specially themed admin pages on top of the real website, so one can navigate the admin area of a site on top of the actual site via the Drupal 7 admin toolbar. Goods example of the admin overlay in action are at http://www.d7ux.org/content/

Initially Paul Lovvik and I took the Popups module and ported that to Drupal 7 (#466732: Drupal 7 port) to build a layer on top of that to implement the overlay. It turned out that Popups has quite big of a custom JavaScript library on its own to manage rendering and display of the pages. It uses an Ajax based model where the popups shown are merged to the same DOM document displayed on the page. This results in interesting problems when you have elements with the same IDs on the page, or you want to theme the displayed popup window radically differently (such as with a different theme). Classes and rules in the two themes can easily clash.

So on advice following on my blog post (http://hojtsy.hu/blog/2009-jun-10/starting-make-some-drupal-7-ux-concept...) we also started to look at the Modal Frame API, which reuses jQuery UI (in core in Drupal 7) and a much smaller JavaScript library to handle the popups. It also uses iframes, which have several advantages. We can use iframes to theme the overlay content with a different theme to the original page without any problems of clashing styles or rules. It also supports the notion of browser history naturally, the back button is not lost. To open pages in the overlay, the toolbar module in Drupal core already adds a "to-overlay" class, on which we can bind and change link behavior to open in the overlay and not in a new window.

So we took the Modal Frame API code, updated that to Drupal 7 (#491224: Port modalframe API to Drupal 7) and started tinkering with a mapper module for our style/layout and linking style. Because Modal Frame API was still a bit beyond what we need for the admin overlay, we refactored some of the code we built on top of it and built all into one module we called the overlay module. I believe the jQuery UI dialogs this module reuses are already there to provide a level of abstraction, so we looked at providing a thin layer on top of it to provide the overlay functionality for Drupal 7.

Interesting challenges posed by the task:

- most links opened from the admin toolbar should open in the popup; the top menu bar was specified to not retain an active state when clicked, so that users will not assume that the lower bar is context sensitive; the lower bar should retain state to show which button's action is activated
- when pages open in the overlay, the page should use the admin theme; we used the "render=overlay" GET param to achieve this, so when this is in the URL, the page is themed with the admin theme in a style suitable for the overlay (only the content and help regions are perserved); otherwise if on an admin page, Drupal itself displays the admin page in the admin theme
- with pages displayed in the overlay, links clicked should still open in the overlay (we get this with iframes by default), but to keep the overlay theming, we need the "render=overlay" param passed on with the links
- with forms displayed in the overlay (such as with most admin forms), the results of the form submission should still be displayed in the overlay; when should we actually close the overlay on form submission and when should we keep it is up for discussion; it could highly depend on the button clicked in the form and could require Form API level information

TODO:

- links with "class=to-overlay" get a click handler; this amusingly disallows right-clicking; ideally if you right-click and open in a new tab, that would not use the overlay theming, but only left-clicking would do the overlay theming and overlay display
- we need to identify links which should not keep being displayed in the overlay; from "Find content" for example, one finds posts; when admin links on the posts are clicked, that should open in the overlay; when the post link itself is clicked, that should break out of the overlay
- same goes for form submissions as detailed above; multistep forms, "save and edit" type of buttons, etc. should keep the overlay, but when a node is submitted for example, the form should close, and the new node should be redirected to in the site's theme in the main window
- more tricky are node previews (and any other type of preview which unlike Views would like to display the output in-place in the real site theme); the D7UX effort's answer to node preview's is to break out of the overlay for a preview and display a "minibar" at the top (like in http://www.d7ux.org/edit-on-page/) to "Edit again" or "Publish" based on the previewed look
- some tricky forms and admin pages still redirect to non-overlay-themed versions of the admin pages; such as when you manually run cron or check for clean URLs; we either need a systemic way to keep the "rendering mode" or need to identify these places and fix individually

Not all of the TODO items should be solved in the initial committed version of the overlay I'd say, since one patch can only attract so many collaborators. However, if anyone is interested in collaborating on fixing issues with the overlay patch as it is so far, feel free to ask for contributor access to our open Subversion repository at http://code.google.com/p/d7ux/ (Check out testing versions of the overlay coupled with the D7UX admin theme from there too).

The overlay works with Garland and should work with arbitrary admin themes, but ATM works best with the initial d7ux admin theme from #484860: Initial D7UX admin theme. This is how it looks:

This patch has contributions from at least the original Modal Frame API maintainer (Markus Petrux) as well as Charlie Gordon and Philip Vergunst.

CommentFileSizeAuthor
#416 overlay-517688-416.patch68.54 KBGábor Hojtsy
#409 overlayinsideoverlay.png216.9 KBRobLoach
#401 overlay-517688-401.patch63.91 KBksenzee
#400 overlay-517688-400.patch65.43 KBDavid_Rothstein
#400 interdiff-391-400.txt524 bytesDavid_Rothstein
#391 Shortcutsbefore.png29.29 KBGábor Hojtsy
#391 Shortcutsafter.png28.85 KBGábor Hojtsy
#391 overlay-517688-391.patch65.53 KBGábor Hojtsy
#389 374-reroll-nobatchapi.patch62.32 KBdrifter
#387 374-reroll.patch64.78 KBdrifter
#386 366plus371-reroll.patch61.36 KBdrifter
#384 366-reroll.patch59.16 KBdrifter
#382 517688-374-overlay2.patch65.08 KBGyt
#381 drupal-overlay.patch12.45 KBmfer
#377 addlink.png21.49 KBGyt
#377 517688-374-overlays.patch64.14 KBGyt
#374 517688-374-overlay.patch61.45 KBksenzee
#372 517688-372-overlay.patch61.11 KBksenzee
#371 overlay_load_after_enable.patch.txt2.47 KBmarcvangend
#368 image_416x207.png7.3 KBmcrittenden
#368 image_1159x121.png14.12 KBmcrittenden
#366 517688-366-overlays.patch59.01 KBGábor Hojtsy
#361 517688-361-overlays.patch56.86 KBAnonymous (not verified)
#357 517688-357-overlay.patch56.11 KBksenzee
#349 517688-349-overlay.patch55.46 KBksenzee
#342 517688-341-overlay.patch53.87 KBksenzee
#342 changes_since_340.patch.txt6.23 KBksenzee
#340 517688-340-overlay.patch54.98 KBGábor Hojtsy
#338 517688-337-d7ux-overlay_0.patch51.68 KBksenzee
#338 changes_since_312.patch.txt4.08 KBksenzee
#312 517688-312-d7ux-overlay.patch50.58 KBAnonymous (not verified)
#310 Alignmentstructure.png4.3 KBBojhan
#310 tabsspacing.png2.68 KBBojhan
#298 pagenotfound.png41.17 KBAnonymous (not verified)
#297 517688-297-d7ux-overlay.patch50.34 KBJacobSingh
#296 517688-296-d7ux-overlay.patch50.36 KBksenzee
#287 517688-287-d7ux-overlay.patch58.14 KBAnonymous (not verified)
#274 letsusejqueryuicorrectly.patch52.28 KBAnonymous (not verified)
#268 LetsUsejQueryUICorrectly.patch52.24 KBRobLoach
#268 Screenshot128.61 KBRobLoach
#267 OverlayFirefox.png57.55 KBGábor Hojtsy
#264 drupal-overlay.264.patch53 KBsun
#257 Screenshot-1.png167.86 KBRobLoach
#256 Screenshot-2.png177.98 KBRobLoach
#256 overlay.patch49.89 KBRobLoach
#252 drupal-overlay.252.patch52.77 KBsun
#249 overlay-517688-249.patch49.97 KBmarkus_petrux
#248 overlay-517688-248.patch49.98 KBmarkus_petrux
#245 overlay-517688-245.patch51.85 KBGábor Hojtsy
#242 overlay-517688-242.patch52.09 KBGábor Hojtsy
#242 TabsForMe.png13.76 KBGábor Hojtsy
#241 overlay-517688-241.patch50.25 KBGábor Hojtsy
#240 overlay-517688-240.patch57.08 KBmarkus_petrux
#229 tabs.png9.35 KBBojhan
#229 marks.png10.58 KBBojhan
#225 overlay.patch50.81 KBGábor Hojtsy
#212 overlay_n.patch49.5 KBGábor Hojtsy
#212 BeforePatchSafari.png66.35 KBGábor Hojtsy
#212 OverlayLibrary.png101.39 KBGábor Hojtsy
#209 spacing.png15.59 KBheather
#209 overlayspacing.jpg24.01 KBheather
#206 overlay.patch46.08 KBseutje
#200 d7ux_overlay_39.patch47.62 KBmarcvangend
#197 d7ux_overlay_38.patch45.97 KBmarcvangend
#192 d7ux_overlay_37.patch45.97 KBmarcvangend
#190 d7ux_overlay_36.patch45.92 KBmarcvangend
#190 images.zip3.95 KBmarcvangend
#187 d7ux_overlay_35.patch45.88 KBcwgordon7
#184 images.zip5.1 KBcwgordon7
#183 d7ux_overlay_34.patch46.42 KBcwgordon7
#179 overlay_css2.png32.61 KBmarcvangend
#179 overlay_css3.png33.89 KBmarcvangend
#170 d7ux_overlay_32.patch49.23 KBmarcvangend
#168 Weird overlay.png140.02 KBDamien Tournoud
#162 d7ux_overlay_31.patch49.11 KBcwgordon7
#160 d7ux_overlay_30.patch48.98 KBcwgordon7
#157 d7ux_overlay_29.patch49.22 KBcwgordon7
#153 d7ux_overlay_28.patch49.06 KBcwgordon7
#150 d7ux_overlay_26.patch48.89 KBcwgordon7
#148 d7ux_overlay_26.patch49.8 KBmarcvangend
#146 d7ux_overlay_25.patch49.47 KBcwgordon7
#144 d7ux_overlay_24.patch49.5 KBcwgordon7
#141 d7ux_overlay_23.patch49.13 KBcwgordon7
#123 d7ux_overlay_22.patch46.81 KBcwgordon7
#122 overlay.png20.01 KBrobertgarrigos
#121 d7ux_overlay_21.patch46.72 KBcwgordon7
#120 d7ux-overlay-images-2.zip6.47 KBGábor Hojtsy
#115 d7ux_overlay_20.patch46.56 KBcwgordon7
#113 d7ux_overlay_19.patch46.38 KBcwgordon7
#111 d7ux_overlay_18.patch46.42 KBcwgordon7
#110 Screenshot-178.jpg35.22 KBeigentor
#110 Screenshot-179.jpg47.47 KBeigentor
#107 OverlayBreadcrumbs.png48.58 KBGábor Hojtsy
#102 d7ux_overlay_16.patch45.29 KBcwgordon7
#97 ResizingFunny.png44.11 KBGábor Hojtsy
#80 d7ux_overlay_13.patch43.75 KBcwgordon7
#79 d7ux_overlay_12.patch43.76 KBcwgordon7
#77 d7ux_overlay_11.patch42.95 KBcwgordon7
#75 d7ux_overlay_10.patch42.91 KBcwgordon7
#74 d7ux_overlay_09.patch40.38 KBcwgordon7
#66 scrollbar33.02 KBJuliaKM
#66 missing_close74.36 KBJuliaKM
#63 d7ux_overlay_08.patch40.45 KBcwgordon7
#56 d7ux_overlay_07.patch38.51 KBcwgordon7
#53 d7ux_overlay_06.patch38.41 KBcwgordon7
#50 d7overlaycamino.png89.15 KBleisareichelt
#39 tabs.png9.96 KBBojhan
#37 d7ux_overlay_06.patch38.41 KBcwgordon7
#35 d7ux_overlay_05.patch38.24 KBcwgordon7
#21 overlay_517688.patch34.13 KBdrewish
#10 overlay.png63.28 KBcatch
#3 d7ux_overlay_03.patch35.29 KBcwgordon7
#2 d7ux_overlay_02.patch35.28 KBcwgordon7
Overlay.jpg84.05 KBGábor Hojtsy
d7ux-overlay-images.zip49.63 KBGábor Hojtsy
d7ux-overlay.patch35.46 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review

Our overlay also lacks the "Help: On" indicator as designed on the overlay. That should be another TODO item, which I believe is again not about to be part of the initial patch and needs very similar treatment for the admin theme in its non-overlay mode, so will be done in a follow up patch.

cwgordon7’s picture

FileSize
35.28 KB

Here is an updated patch that features cleaner code (a bit of dead code removed too), including the introduction of a new reusable forms API property, #after_build_recursive, which applies an array of functions not just to the element it is attached to, but all of its children as well. This saves the module the effort of iterating through all the form elements itself, and can leave that to form_builder(). It is quite possible that other modules would want to use this ability as well, so instead of hard-coding it, I made it into a form property. This patch also adds the module to the default profile (though this is probably up for discussion). The images and screenshot given by Gabor in #0 still apply.

cwgordon7’s picture

FileSize
35.29 KB

Whoops, sorry, patch was missing a recent bug fix.

skilip’s picture

subscribe

moshe weitzman’s picture

subscribe. wasn't the overlay discussed at length in another issue? seems a bit shady to just walk away from the feedback there.

Gábor Hojtsy’s picture

Moshe: I don't know of another issue. If you can find one, we can move the feedback over here or we can move the action over there.

cbrookins’s picture

subscribe

Dries’s picture

Issue tags: +Favorite-of-Dries

Looks great based on the write-up, but haven't reviewed it yet.

Bojhan’s picture

Not sure when anybody is going to say it. But, ugh iframes? I get there are quite some advantages, but I know that our designers will not like us for this.

catch’s picture

FileSize
63.28 KB

@Bojhan, there's a difference between iframes in html markup, and iframes generated by javascript, afaik the latter is OK.

Just applied the patch - clicked on admin/content - then content, then scrolled to the last item in the list, and clicked edit - the iframe worked, but now the top of the overlay started half way down my viewport. This is on firefox 3.0/ubuntu. Attached screenshot of it happening on block edit, this is recursive the more links you click - I had to scroll down about 45 cm to even find the top of the overlay on this page.

Gábor Hojtsy’s picture

@Bojhan: since we need to present essentially two pages in one browser window (a regular site page and an admin page on top of it) with two different Drupal themes, doing this by merging in the two themes on the page would get us a lot more hatred from those designers IMHO :) Building a theme so it would not clash with an arbitrary other theme sounds a bit too adventurous.

@catch: this positioning question is interesting indeed. We can always position the overlay on top of the page, but then if you scrolled into two thirds of the document, then you might not realize there was an overlay opened. If the toolbar moves with the page, the overlay should open on the viewport top where you are. However, when you scroll up, it does look odd, that the overlay was not opened at the top of the page. Should it move up with your up-scrolling but not move down with your down-scrolling? Not sure about the right answer here.

karschsp’s picture

subscribe

mfer’s picture

Status: Needs review » Needs work

I wasn't able to give it a good once over but on a brief review 2 things jumped out at me.

First, In a few places like:

Drupal.behaviors.keepOverlay = {
  attach: function (context) {

the settings is not passed locally. The new function signature should have it read

Drupal.behaviors.keepOverlay = {
  attach: function (context, settings) {

And we should refer to the settings variable rather than the global Drupal.settings. This local version of the settings is the one we should pass around as well. For more detail see http://drupal.org/update/modules/6/7#local_settings_behaviors.

Second, Is the overlay module dependent on the toolbar module? The .info file doesn't say so but the following line makes me think it is:

+  // Only act if the admin toolbar "is enabled" (user has access).
+  if (user_access('access toolbar')) {
Dries’s picture

Status: Needs work » Needs review

@Bojhan, euhm, not quite ... there is proper iframe usage and ugly iframe usage. This would classify as proper iframe usage.

catch’s picture

@Gabor - if I scroll down first, then open the overlay, the positioning is off and I get a big blank space at the top of my screen anyway. Pretty sure this is because the actual size of the screen changes - so when I open the overlay, my relative position is adjusted by the browser. So it's currently not working for either case. I hadn't thought of opening the overlay when you're half way down a page, that does make this tricky.

My preference would be for the overlay to open at the top, and to scroll the main page up with javascript at the same time (we either do or did have a similar thing when opening collapsed fieldsets to take you to the top of the fieldset). That way it's always in a consistent place and you won't end up with 5 metre long browser windows.

One other niggle - there's currently no [x] button that I can find to close the overlay, this doesn't seem to be mentioned in the TODOs. Note I'm being deliberately annoying and reviewing this in Garland for extra breakage.

Gábor Hojtsy’s picture

@mfer: Note taken on the settings passing.

@mfer: On toolbar module dependency, some parts of the JS are not clearly written to make it optional but should. Needs to be fixed. There is only one place where the permission is referenced in the module, and looking at that, it does not seem to actually require the toolbar in any way. So looks like we can eliminate any hard dependency on the toolbar. For positioning and management of the active state of toolbar links, we still need to reference the toolbar, but can make those references optional.

Gábor Hojtsy’s picture

@catch: are you not seeing the close button which is shows on the opening screenshot at the top? (I can see it on Safari and Firefox on my Mac). Maybe you did not uncompress the images to the images directory below the module?

catch’s picture

Arggh, applied the patch from #3 and forgot the images in #1, sorry. Maybe we can persuade Dries/webchick to commit the images early so reviewers don't need to remember that step.

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

subscribing.

drewish’s picture

Status: Needs work » Needs review
FileSize
34.13 KB

re-rolling around the changes to way profile dependencies are listed. setting to needs review to get test bot feedback.

xmacinfo’s picture

Looks nice! My concern is for long admin pages and the close button position, which I would duplicate at the bottom of the screen. I've not tested this yet, though.

As for the images required for the overlay, is there any consensus to commit them before RTBC?

Dries’s picture

I can commit the images early but they need to be renamed not to use abbreviations, and all underscores need to be changed to dashes. Eg. ovrly-shw-bt-lt.png needs to become overlay-shadow-xxx-xxx.png.

Shouldn't some of these be added to the sprite instead?

joshmiller’s picture

An enthusiastic... SUBSCRIBE!

Bojhan’s picture

Gábor: Can we please get a demo running? It's kind of getting annoying to review any D7UX stuff - due to unable to test anything.

sun’s picture

Status: Needs review » Needs work
+  if ((isset($element['#after_build']) || isset($form_state['complete form']['#after_build_recursive'])) && !isset($element['#after_build_done'])) {

#after_rebuild_recursive needs to be removed. That is against the nature of all other FAPI properties.

+Drupal.overlayChild = Drupal.overlayChild || {
+  processed: false,
+  behaviors: {}
+};

Please write this in one line.

+/**
+ * Use a Drupal behavior to attach the child dialog behavior so that it will be
+ * automatically attached to new content.
+ */

JSDoc summaries should be on one line; additional description can follow in the JSDoc block afterwards, though.

+  attach: function(context) {

Missing settings.

+/**
+ * Check if the given variable is an object.
+ */
+Drupal.overlayChild.isObject = function(something) {
+  return (something !== null && typeof something === 'object');
+};
+
...
+/**
+ * Check if the given variable is an object.
+ */
+Drupal.overlay.isObject = function(something) {
+  return (something !== null && typeof something === 'object');
+};

Duplicate definition of the same method.

+  $('a:not(.overlay-processed)', context).addClass('overlay-processed').each(function() {

Given the recent discussions around JavaScipt code in Google Analytics module, I'm not sure whether this will lead to major performance/memory issues.

...

Given that I see so much code to properly handle IFRAMEs, I am questioning whether it would not make more sense to follow the AJAX (Popups API) approach.

Also, it seems like we're trying to move 2-3 contributed modules into core with this patch. The maintainers of those modules have to sign-off this patch at least. (Can we please stop to repeat mistakes, please?)

Stefan Nagtegaal’s picture

Gabor, if you want to you can use my domain and hosting environment for setting up a demo website for this UX7-stuff.
Unfortunatly I can not review/test the patch because Drupal 7 HEAD is uninstallable for me at my localhost due to problems with installation profiles.

But I'm happy to give you access to my server and let you use it for demo this stuff, so we get more people to review it..

eigentor’s picture

Dries’s picture

- I was also frowning upon #after_build_recursive -- weird concept. Maybe it is the right thing though.

- @sun, other maintainers are welcome to participate, but they do not need to sign off on this. Given that there are various 'competing modules' already, it is unlikely that we'll get everyone on the same page. Plus, Gabor has been very open about this from day one -- he started with blog posts 2 months ago so maintainers have had plenty of time to get involved, and can still get involved.

eigentor’s picture

A medium experienced Drupal user who had never seen Drupal 7 before and was not acquainted with D7UX did a 20-minute test of the overlay. She tried content creation and gives also some opinions on sections like the permissions page, that are actually not relevant to this issue, nor even directly to Drupal 7. So just filter out the stuff that directly affects the theader. I think it removes the "create content" link and makes it visible only in the header? Some troubles ahead when not logging in as admin...

http://vimeo.com/groups/drupalux/videos/5657241

Everett Zufelt’s picture

Issue tags: +Accessibility

tagging

Everett Zufelt’s picture

I'd love to audit the accessibility of this patch. Is there a testing environment setup somewhere that I can gain access to? Or, can someone give me the exact steps to get my fresh install of head patched with this patch and a path where the functionality can be accessed?

kika’s picture

Perhaps a dumb question: will ui.dialog() and similar popups work when overlay in on?

Bojhan’s picture

Should the overlay not close, when you click on the dark background?

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
38.24 KB

Ok, updated patch, I think this addresses all the feedback. Changes include:

- Various coding style changes, both pointed out in this issue and ones I just noticed myself,
- Removed #after_build_recursive, changed the patch to use hook_element_info_alter() instead,
- A click on the dark background will now cause the overlay to close,
- Some weird alignment issues occurred previously, they are gone now; the overlay always opens at the top of the page, and you are scrolled there automatically,
- Added a way for non-administration links on admin pages (such as the node listing page), to escape from the overlay by adding an 'overlay-escape' class; this is not yet implemented for all administration links in an effort to keep this a reasonably sized, reviewable patch, but it is implemented for the link to content on the administer content page as a demonstration,
- Added a way for forms to request for the overlay to be closed; this is currently implemented as a demonstration for the node add/edit form, which, if used from the overlay, will close the overlay upon submission,

Response to some questions:
- I have no idea whether the images should be added to the sprite or not, it will probably lead to faster loading times, but this pattern is not implemented for other core themes or modules, I'm not entirely convinced it should be used here. I'd welcome input from some people who are into the design side of Drupal.
- To audit the accessibility of this patch, you need to apply it to the latest Drupal 7 code (see http://drupal.org/patch/apply). An administration toolbar will appear at the top of the page; clicking on any of the links in it will bring up an "overlay" iframe. Alternatively, there seems to be some talk of setting up a live demo site, you could wait for that.
- I just tried it, ui.dialog() still works, so I'd assume other popups will work as well.

Bojhan’s picture

Edit: It seems some are addressed by the last patch, let me retry.

Overlay

1. All tabs (meta for example) seem miss aligned.
2. I am having a horizontal scroll bar.
3. Could the height be flexible to the last item? Now you see a small difference between content mangement and user management.

cwgordon7’s picture

FileSize
38.41 KB

My apologies, the last patch had references to the wrong filenames because I forgot to properly roll the patch with the CVS hack to add a directory. The attached patch should be properly formed.

cwgordon7’s picture

@Bojhan - I think you're confusing some of the administration theme patch with this overlay patch. Feedback for the administration theme (such as the capitalization or alignment or secondary tabs theming) can go to #484860: Initial D7UX admin theme - I see you posted there as well(?) I think most (all?) of your points should have been addressed in the latest patch, I'd be interested to know if any of them remain applicable.

Bojhan’s picture

FileSize
9.96 KB

I tried the last patch, updated my original issue and also attached the tabs alignment issue.

Gábor Hojtsy’s picture

@Bojhan: the mockups from Mark specify the tabs to not be aligned to the end of the overlay. Look at the closup image at http://drupal.org/node/472126#comment-1719664 for example. The tabs are specified to align with the actual content of the overlay, so they are padded with the same padding from the right side like the content of the overlay. This is clearly visible on your screenshot of our implementation and at the actual mockup shot at http://drupal.org/node/472126#comment-1719664

Dries’s picture

1. - Added a way for forms to request for the overlay to be closed; this is currently implemented as a demonstration for the node add/edit form, which, if used from the overlay, will close the overlay upon submission.
I haven't looked at the code yet, but the form API doesn't need to know anything about overlays. Let's keep proper separation of APIs. As said, I have not looked at the code yet so maybe this is a non-issue. It just sounded worrisome from your comment.

2. I'm perfectly happy to have the tabs align as suggested by Mark. Looks nice to me.

Bojhan’s picture

@Dries 2. It doesn't look right. But the complications of changing it seems rather large, after talking to Gabor.

cwgordon7’s picture

@Bojhan (#36/39):

1. Gabor addresses this in #40, and Dries addresses this in #41 #2. I'm happy having them either way; if I recall correctly, I originally aligned them as far to the right as possible so it lined up, but some extra padding was added on the right to make it fit the mockup.

2. On which page(s) and with what size of a screen? At some point a horizontal scroll bar becomes necessary, but it shouldn't be on a reasonably sized screen and a page of reasonable width; I can try to debug this if you give me the page and screen size.

3. I don't understand what you mean by "could the height be flexible to the last item" - do you mean you want a minimum dialog height such that it's almost (but not quite) causing the page to scroll vertically?

@Dries (#41):

1. It's just a function call to overlay_close_dialog(TRUE) in form submission handler - I don't think there's anything wrong with that.

Status: Needs review » Needs work

The last submitted patch failed testing.

leisareichelt’s picture

I see that we've implemented clicking on the dark background = close the overlay.

I think we should remove this functionality as it is too easy for this to be done in error and there is no easy way for a user to recover from this error, there is far more potential to create a poor user experience than a good one, and closing the overlay is not a difficult task.

Clicking on the dark area around the overlay window should have no effect. (we did toy with doing something fun if you double clicked tho!)

There may be an argument for having the close button follow you down the screen, but I suspect that might be over complicating things as well.

qbnflaco’s picture

Status: Needs work » Needs review

I've taken this for a spin on the d7uxdemo. It seems on the find content overlay, if you have one item on the list and click edit, the whole form has to be scrolled through the tiny gap at the bottom of the overlay. Conversely, when you have many items, you aren't able to scroll to the very bottom of the edit form. I think clicking this edit link should affect the height of the overlay and add the new edit div's height to the overall overlay.

sun’s picture

Status: Needs review » Needs work
+function overlay_element_info_alter(&$types) {
+  foreach (array('submit', 'button', 'image_button', 'form') as $type) {
+    $types[$type]['#after_build'][] = 'overlay_form_after_build';
+  }
+}

You don't need hook_element_info_alter() here; hook_elements() is sufficient.

+    // When rendering a page for the overlay, skip all regions generated by
+    // blocks, except the content and help regions.
+    foreach ($variables['page'] as $key => $value) {
+      if (strpos($key, '#') !== 0 && !in_array($key, array('content', 'help'))) {
+        unset($variables['page'][$key]);
+      }
+    }

What about element_children() ?

+    case OVERLAY_PARENT:
...
+   case OVERLAY_CHILD:
+      // Disable admin toolbar, which is something child windows don't need.
+      module_invoke('toolbar', 'suppress', TRUE);

Other modules need a chance to get triggered in those cases as well. (Also note the wrong indentation for the second case)

+++ modules/node/node.admin.inc	28 Jul 2009 11:15:25 -0000
@@ -451,6 +451,9 @@ function node_admin_nodes() {
   foreach ($result as $node) {
     $nodes[$node->nid] = '';
     $options = empty($node->language) ? array() : array('language' => $languages[$node->language]);
+    // Set a class to flag to the overlay, if present, not to open the link in
+    // the overlay.
+    $options['attributes']['class'] = 'overlay-escape';
     $form['title'][$node->nid] = array('#markup' => l($node->title, 'node/' . $node->nid, $options) . ' ' . theme('mark', node_mark($node->nid, $node->changed)));

This is a form. And forms can be altered by overlay.module.

+++ modules/node/node.pages.inc	28 Jul 2009 12:47:21 -0000
 function node_form_submit($form, &$form_state) {

Likewise changes in here. If overlay module needs to do something somewhere in between, then we need to refactor the other module's code. This overlay patch/module, however, must not alter anything in other core modules.

Gábor Hojtsy’s picture

@qbnflaco: the d7ux google repo has the "accordions for flooded state screens" patch applied. Sounds like your feedback is related to how that one behaves, see #492834: Hover operations for flooded state screens

xmacinfo’s picture

@cwgordon7 A click on the dark background will now cause the overlay to close

Is the overlay supposed to act as a regular modal dialog box? I think we should keep overlays non-clickable. The only ways we should close an overlay (and its content), is by clicking the close button or pressing the escape key.

Here we already have the close button. As for the 'esc' key, I have no clue if it is planned to support it. :-)

As for the close button, we need to think about large content pages. When at the end of a long overlay document, do we need to scroll all the way up or press the home key to reach the close button?

leisareichelt’s picture

FileSize
89.15 KB

I'm not sure what browsers we're supporting but the overlay has suddenly gone a little skewiff in Camino

Bojhan’s picture

I have been, using the overlay for the past day - to actually really use it. And the click and close is quite bad, as for example in vista where you click between screens you constantly close it, heh. Lets remove that again, sorry!

Gábor Hojtsy’s picture

While testing this patch without the toolbar, it looks like it has some bugs where it adds ?render=overlay to form result pages even if the form was not initiated from the overlay. Looks like this can be consistently reproduced.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
38.41 KB

This is still at needs work because not all stuff has been addressed yet, but this patch should fix the bug in #52 as well as the one that caused Drupal not to install for the test bot.

A few quick comments though:

@#47 - Mostly valid criticism, I intend to roll those changes into my next patch (this one is just a quick critical bug fix reroll). I actually disagree about the use of hook_form_alter() by the overlay module to alter the node administration page, however. This is not a case of the overlay module using a node/system API; this is a case of the node module using the overlay module's API. A similar case is the search module - as the node module implements the search module's API and not the other way around, the node module is responsible for calling search_reindex() in node_delete_multiple() even though the search module could implement hook_node_delete().

@#49 - The consensus seems to be to remove the background-click behavior, so I will do that in the next patch. The escape key should currently be working though - are you having a problem with it?

@#50 - I have no idea where to even start debugging that. I'm on windows, I don't think I can test with that browser(?) - can anyone provide more information about the specific CSS that's causing those bizarre red/green/blue horizontal bars?

Status: Needs review » Needs work

The last submitted patch failed testing.

Stefan Nagtegaal’s picture

@ comment#50: It's caused by the opacity.
I'm not sure, but I guess you are changing it somewhere to '1'. Which is causing problems in most Gecko browsers. Try to give it 0.9999 instead of one..
In the past I had some good experience when doing an opacity: 0.5999 instead of 0.6 to fix these kind of things, although it also depends on the rest of your XHTML-code and CSS.

Good luck...

cwgordon7’s picture

FileSize
38.51 KB

Um, I attached the wrong patch, sorry.

Bojhan’s picture

The last checkout of the Google repository seems to give me all kinds of errors.

http://screencast.com/t/VsZX8y2tTHD
Horizontal scrollbar?

http://drupalbin.com/10745
When creating a node.

http://drupalbin.com/10746
When creating a menu link - and it trows me in the normal admin theme, without the overlay.

qbnflaco’s picture

the close link for the overlay doesn't seem to work in IE6 or 7.

eigentor’s picture

Do we have a plan how to control the tab madness? The currrent state does not really take care of it - I saw some of them stay on the "regular" node form and some go on the overlay. Might be a way to clear up the clutter, as some setups go wild in tabifying the node form (and other places like the user account / profile).

Ath the moment the layouts look nice and clean, but with a wealth of five or six tabs ... ( translation, revision, even media gets one now =8-0 http://www.flickr.com/photos/mverbaar/3758839274/sizes/o/in/set-72157621...)

Everett Zufelt’s picture

I did a coarse accessibility evaluation of the d7ux admin theme and overlay. Primarily looking at the "Content" menu item.

FF 3.5.1; IE 6; SAfari 4.0.1; JAWS 10.0.1154; JAWS6 6.00.410; NVDA 0.6p3.2; VoiceOver OS X 10.5.7.

5. After selecting the "Content" link from the top navigational links (top in DOM order, but perhaps not in display order):

  • JAWS on FF speaks "Content Dialog", but I am left with focus on the "Content" link, I must refresh the JAWS virtual buffer and find the "Content" dialog / iframe (it is a known issue that later updates of JAWS 10 do not correctly unhide elements with style="display: none;").
  • JAWS6 on IE normally lands near the beginning of the iframe and speaks the element that is landed on, there is no real predicting where JAWS6 will land.
  • NVDA on FF speaks "Add Content" and I am taken to the add content link, which is disorienting as it is where the bottom of the page used to be.
  • VO on SA takes me to the “Add content” link and actually treats this as a dialog by “trapping” the user in the iframe until they stop "interacting" with the iframe, by pressing Shift + Control + Option + Up arrow. Unfortunately users will likely not know to do this as VO does not alert the user that they are within a frame.

5.1 Found later in the evaluation, there is content relevant to the “Content” iframe but which is outside of the iframe “Content” “Close button” and links
for “Content” and “Comments”.

  • JAWS and NVDA on FF and JAWS6 on IE do not identify that “Content” or “Comments” are tabs, or which is selected (I imagine that even if these are not visually styled as tabs that they serve the same functional purpose.
  • VO on SA users do not know that the above options exist because VO traps the user in the iframe, see VO and SA in 5 above.

6. Navigating by headings.

  • With JAWS and NVDA on FF I find the last heading on the page to be "Welcome to Example Site", and not "Content Administration" or some other heading that would indicate to me that I am now on the section of the page that will allow me to make changes to content.
  • With JAWS6 on IE there is a heading level 1 “Content” which appears directly before the “Add content” link. This heading would be better placed above the other links mentioned in 5.1 above.

Recommendations:
1. It would seem to me that for best accessibility the tabs, close button, etc. should always be placed inside of the iframe.

2. Links that makeup tabs should be identified as such non-visually. Some non-visual indication of which "tab" is selected should be provided. See #521852: Local tasks lack semantic markup to indicate an active task.

3. Proper headings should be made apparent to all assistive technology (all browsers) to allow users to quickly skip to the beginning of the overlay content.

The full d7ux admin theme evaluation can be found at http://openconcept.ca/blog/everett/first_glance_accessibility_evaluation...

eigentor’s picture

A new User Test of the Ovelay is online:
http://vimeo.com/5994325

Quite some of the issues identified do not relate directly to the overlay. But one is interesting: the user just did not find the close button (might have to do that far right is quite out of focus physically). Eyetracking would be of some use in this case :)

The main other issue (not so specific for the header, but to seperation of admin/live site in general): where am I? How do I get back to my live site? how do I go to the front page? Just have a look yourselves.

xmacinfo’s picture

@eigentor: Agreed. I have not looked at the video, but I expect that the close button for the overlay will be hard to find.

I would suggest to give to overlay a real titlebar, complete with a more standard close button. To complete this, at the bottom of the overlay we should also display a regular form Close button.

The overall overlay should look like a real window so as not to confuse newbies.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
40.45 KB

Here's a new patch that addresses all of the above feedback except the accessibility issues outlined in #60 which I'm not quite sure how to address yet. Changes include:

- Fix for the IE bug in which the overlay would not close.
- Fix for the errors in #57
- Opacity fix explained in #55 (using 0.9999 now instead of 1)
- Another bug not mentioned here was fixed - if the iframe is resized dynamically through javascript, such as with collapsible fieldsets, the overlay does not expand to keep up with the expanded iframe. To solve this, I have the javascript checking every 150 milliseconds for a resized iframe. Although this is admittedly a somewhat ugly solution, I thoroughly explored other possible solutions, such as some javascript event (no fitting events exist) and requiring Drupal javascript to trigger the overlay's javascript when the page is resized (even uglier than this solution as well as bad developer experience). I'm open to better suggestions, but I think this is the best we can do.
- Rethemed parts of the overlay to match changes to the Seven administration theme.

Status: Needs review » Needs work

The last submitted patch failed testing.

markboulton’s picture

I think, watching the video more closely, that the issue isn't perhaps the close button, but rather one of orientation. That aside, we should provide more feedback and more ways in which the overlay can be closed. We should perhaps consider a 'tooltip' like hover when hovering over the space outside of the overlay to provide more feedback. I also think the transparency could be a shade lighter so it's obvious that the overlay is overlaying your site, and to get back, you just close the overlay.

The issue of Where Am I?

We've got a few issues I think. Firstly, the notion of content types being 'in site' eg job posts, comments, forum posts. And content types being 'in admin'. The current prototype deals with this by showing an overlay for the latter, and the non-javascript admin theme for the former. This, in my opinion, is wrong and could be adding to this user's confusion. Going right back to one of our core UX principles: 'I should know where I am'. As a user, I should do 'in site' content things in the site, and not in the admin. And as an admin, I should do admin type stuff in the admin and not on the site. This distinction is important for the 80% of content creators and this video clearly shows.

Lastly, it's worth noting that this is just *one* user test. I don't think we should be making changes based on feedback from one person's experiences.

JuliaKM’s picture

FileSize
74.36 KB
33.02 KB

I'm not sure whether you are looking for specific bug reports at this point. I noticed a few problems with the modal box after installing the patch in #63 and testing using Safari 4.02 on a Mac.

1. When I go to Content > Find content and am on the Comments tab and then click the Content tab, the box juts out to the site. Basically, what happens is that the text box below the Content tab moves to the right and then back in place to the left.

2. Similarly, when I switch to any menu item with tabs, the box will move the right and then back again in place to the left. When I go from a navigation item with tabs to one without, the right side seems to go out to the right and then come back in.

3. I'm also seeing a scroll bar on the Appearance, Configuration and modules, Help, and Content pages. (screenshot attached)

4. I could not seem to find the close button. I eventually realized out that you could close the box with escape, but I spent a few minutes searching around puzzled. The little box with a circle on the right isn't showing up for me. (screenshot attached)

Gábor Hojtsy’s picture

@markboulton: I've opened an issue to migrate our "all admin node editing"/"all non-admin node editing" to a notion of admin and user content types (per type): #546186: Migrate overall node admin theme setting to per-content type

@JuliaKM: Looks like Charlie did not post updated images. Just use the images from the start of the issue. Some of them are not needed anymore, but it should be fine for testing for now. The close button will show once you extract the images under the overlay module into an images directory.

eigentor’s picture

Yeah, we definitely need more user testing. Am a bit out of the loop at the moment due to other work.
Maybe some of the people Leisa triggered with her call for testing for the Header prototype can be re-enthused?
There were quite some people doing tests.

mthart’s picture

subscribing

Gábor Hojtsy’s picture

I've made some changes in the google repository (not rolling new patch, since we have other outstanding issues):

- removed border from the tabs, they are not supposed to be on the overlay tabs, only on the standalone theme tabs; the overlay tabs stand out in themselves from the background
- removed some extra margin from inbetween the tabs
- tried to make inactive tabs again show up 1px above the overlay; this is not consistently doable with our current CSS/markup in Safari and Firefox for some reason
- pushed the tabs back 20px (same as the padding on #page inside the overlay) as on the mockups and as highlighted by feedback from Mark and Dries
- moved from 0.8 opacity to 0.7 as per Mark's suggestion to make the overlay look more above your site and not obscure your site as much

There are some other issues I found but did not look into fixing:

- if you resize the parent window, the overlay does not go with it; I'd suggest looking into binding to the parent window's resize event: http://snipplr.com/view/6284/jquery--window-on-resize-event/ the background resizes already but the overlay itself does not
- the tabs are still not spot on with 1px margins; there is still too much space inbetween them
- even on a relatively speedy machine like mine (core 2 due), I see the scrollable table headers jump to hidden when I open an overlay with a table in it (this may be an issue with the table headers, but others like JuliaKM on #66 experienced similar jumping around due to slowness)
- related: when a new overlay is opened or a new tab is clicked, the overlay becomes small and the drop shadow shows on the white background while the page loads; not nice
- you forgot to include the Seven style.css changes which make the messages look better with added padding, so they don't go to the edge of the overlay
- paths like /admin/* and node/add/* (if configured to be admin themed) should open in the overlay; ie. we need some kind of path rewriting to include the render=overlay GET param; maybe not the exact scope of this patch but a workflow issue to keep in mind nonetheless; note that the above user testing showed that there can be considerable issues due to not implementing this

qbnflaco’s picture

- paths like /admin/* and node/add/*

How about node/*/edit too? It's a little inconsistent that when editing a node, the same overlay doesn't come up as when you first created it.

Gábor Hojtsy’s picture

@qbnflaco: of course it would be best if the Edit tab would not show on the node or would at least lead you to the overlay for editing just like when you submitted the node.

webchick’s picture

Could we please get a new patch here? I'd really like to test this, but don't fancy testing an old, crusty patch, and the with the d7ux repo I can't tell what's part of this issue and what isn't.

cwgordon7’s picture

FileSize
40.38 KB

Here's a rerolled patch as requested, it still needs the issues pointed out in #70 to be fixed, hopefully a patch containing those fixes will be up later today, but here's a reroll that just incorporates everything currently in the d7ux SVN repository.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
42.91 KB

Ok, I beileve this patch should resolve all the current issues:

- The overlay now resizes with page resize (only a resizing of the width is necessary, as a design decision was made earlier when we decided that an overflow in overlay height would cause the page to scroll, not the overlay, to avoid a scroll bar within a scroll bar scenario).
- The overlay height problems should be fixed now, it's a lot smoother opening/closing the overlay.
- Tableheaders.js had a slight problem, I am not quite sure why it was not showing up on normal pages too, but this patch now fixes tableheaders.js to avoid the flash of the sticky header.
- Changed seven.css so messages set in the overlay don't force the messages beyond the boundaries of the overlay.
- Caught a minor spelling error and removed a dead comment.

As you said, the workflow issues here are somewhat outside the scope of this issue; I happen to think it's a bad idea to always open certain links in the overlay (if the node/add page is opened outside the overlay, is the right thing to do to open individual node/add/* pages in the overlay each time?), but that discussion can go in another issue.

I think that should address everything, and this patch is ready for reviews again. :)

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
42.95 KB

Sorry about that, patch didn't apply to default.info, rerolled version attached.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
43.76 KB

No idea why this was affected by this patch, but locale.test seems to be failing because the regular expression !admin/config/international/translate/edit/(\d)+! is matching the path admin/config/international/translate/edit/10 as 0 rather than 10, so a minor change in the test case's regular expression fixed the failures for me. (I don't understand why this wasn't failing before).

cwgordon7’s picture

FileSize
43.75 KB

Apparently HEAD changed the paths on me so the locale.test stuff no longer applies, this one should work for real now though!

Everett Zufelt’s picture

Have the accessibility concerns (particularly 5.1 and 6) in comment 60 above been addressed in this most recent patch?

Status: Needs review » Needs work

The last submitted patch failed testing.

sun.core’s picture

Status: Needs work » Needs review
sun’s picture

Assigned: Unassigned » sun
sun’s picture

Assigned: sun » Unassigned
Status: Needs review » Needs work

I considered this patch as one suitable target for the code sprint today and thereby assigned this issue to me.

Please note that I'm not really good at properly expressing all of my thoughts in reviews like this. I've been told in IRC that I can post this review - I don't mean to insult you, because you put a lot of effort into this, but I also put a lot of effort into my review, so please don't take it personally. I'm trying to stick to the technical issues.

- The first and foremost major issue I have to question is: Why is UI fanciness tackled before actual functionality? I see that this overlay properly handles a fancy shadow below and next to the overlay area, a close button that is nicely aligned to the right of the overlay, and a title along with tabs that are nicely placed at the top-right above (outside) of the overlay. The actual overlay functionality, however, is taking a wrong approach and is not at all commit-worthy.

- The stylesheet depends on Seven theme, because certain CSS selectors are used that simply hide the page header and footer in the output. Disabling Seven theme or changing the admin theme to something else shows the entire page output in the overlay - so the entire JS/CSS functionality is tied to Seven theme and it does not work in any other theme. This approach will not work out, because themes can apply arbitrary CSS IDs and classes, and it is not the job of an overlay feature to determine them correctly. The new action links that were committed today already break this assumption.

- The overlay functionality simply outputs page content in an IFRAME as if the user would request the same page without an overlay. This means that we load and render a lot of unnecessary data in the header and footer and do not take advantage of faster page request times that could be possible if only the requested page fragments were delivered. Themes are able to build a lot of things around the main content, and it is absolutely not clear how we want to enforce a CSS ID of "#content" for the main content in all core, contributed, and custom themes to make this work consistently and properly.

- When clicking links in the overlay, it is easily possible to display the entire, regular site content in the overlay instead of the parent window. Some links in the overlay try to open a new window on behalf of the user instead of using the parent window. All of this is most probably caused by the previous issue I mentioned, i.e. the overlay simply does not know which link belongs to the overlay and which does not, because the overlay does not only contain the content we actually want to display - it contains anything that is possible to do in a theme.

Those issues should be solvable, but in the current approach taken, they require a major re-thinking on how the entire overlay functionality is supposed to work. The current implementation, which just outputs the content that a(ny) menu callback returns, is not going to work out - instead, that should have been the primary focus of the implementation - leaving UI fanciness aside.

Yes, this is an overlay functionality. But aside from jQuery UI's built-in features, I have a hard time to understand why this needs so much code to open an IFRAME - because that is all what it (properly) does currently (for me during testing).

I am sorry - but this is a won't fix to me - at least for D7. It's very unlikely that we find a completely revamped solution, from scratch, for D7, in the next 7 days.

Everett Zufelt’s picture

I hope that any reworking / redesign of this component takes accessibility into consideration as well as usability. As always, I am available to assist with accessibility considerations at any stage of the development process for core components.

markus_petrux’s picture

Really glad to see this is happening. :)

Subscribing.

catch’s picture

Sun makes a lot of good points in his review. When I tried the overlay out, a few weeks ago, my main complaint was watching the spinner for some time before the page opened - this on localhost with a reasonably fast machine.

When something loads in an overlay, you notice the time loading it a lot more than when it opens in a browser tab, because overlays/AJAX are supposed to be fast - and progress is displayed more prominently, so this needs to be as optimized as possible, which absolutely means only generating what we need to see.

+/**
+ * Preprocess template variables for page.tpl.php.
+ */
+function overlay_preprocess_page(&$variables) {
+  if (overlay_mode() == OVERLAY_CHILD) {
+    // When rendering a page for the overlay, skip all regions generated by
+    // blocks, except the content and help regions.
+    foreach (element_children($variables['page']) as $key) {
+      if (!in_array($key, array('content', 'help'))) {
+        unset($variables['page'][$key]);
+      }
+    }
+    // Add overlay class, so themes can react to being displayed in the overlay.
+    $variables['classes_array'][] = 'overlay';
+    // Do not include site name or slogan in the overlay title.
+    $variables['head_title'] = drupal_get_title();
+  }
+}

Seems like this ought to be possible to handle in hook_block_list_alter(), which would avoid running block callbacks at all. Or if that's no good for some reason, hook_page_alter() would at least save them being rendered/themed then thrown away, But doing it in preprocess is really, really wasteful.

xmacinfo’s picture

Status: Needs work » Postponed

Let's postpone this until we have a better implementation. Sun's review is well done and goes to the point. The overlay functionality is not ready for Drupal 7 and may need to be re engineered from scratch.

Bojhan’s picture

Status: Postponed » Needs work

Jeez, it is not nice to mark something postponed, just because you don't like it - reviews are meant to be addressed - they have not yet been, therefor this issue by far cannot be put on postponed.

eigentor’s picture

@sun
While your review adresses major issues, it puts you in a clear position to provide patches. I have seen something very similar happen to plugin manager after it had been worked on a long time.

Issues are there to be raised, but whoever raises the criticism takes on a lot of responsibility.

Yeah, the long loading time is a bit annoying, but not really that bad to me. So let's try to improve it!

markus_petrux’s picture

Both, an AJAX request and a full page request are supposed to perform a full Drupal bootstrap, so probably the difference is basically located on the amount of stuff a full page may require, but this can be reduced a lot when css/js aggregation is enabled, also page compression, etc.

Aside, there maybe times when AJAX is ok, but sometimes a modal dialog taking the content from a full page request may make things a lot easier. All you need to do is optimize the page processing to avoid all the stuff that would only be required for a real full page, but not when the same exact page is rendered within a modal dialog. This is where optimization would have to focus if this is going to get in.

When I wrote Modal Frame API, I tried to find a way to make it possible without touching core, but... when someone has the key to change core, things change a little. The whole page template processing can be tweaked to take into account the differences between a full page and an overlay.

Gábor Hojtsy’s picture

@sun:

1. On UI fanciness: we built tools for modules to (1) let links open in the overlay (2) keep all links and forms in the overlay by default but let modules specify links which open outside the overlay and this is what is required API-wise from the overlay; it is another way to display admin pages and we need a way in and out for modules; sizing is specified inside the overlay; its looks are specified in part by the framimg (that is overlay module) and in part by the theme.

2. Seven vs. the overlay: hey, you just explained that we should do stuff like we do :) Open the bottom of themes/seven/style.css and you'll see that Seven specifies overlay specific styles. You say the overlay requires Seven, but this is not at all the case. Themes can specify similar overlay specific looks so they fit into the overlay better. Steph from Top Notch Themes has an issue to let themes opt out from being turned an admin theme, and others suggested (in the Appearance page issue) to let themes opt out from being the front end theme, because doing a good UI for both is hard (see admin specific themes and try to apply them to the front end). While it would be ideal to have all themes work nicely for admins and front end themes, it is not the reality even without the overlay (the given issues were opened without consideration for the overlay even).

3. On the iframe, this was discussed above as well. Markus points out that we already *almost* only generate the output that is required to be displayed, the CSS hides only a little amount of the items on the page and PHP alters out the rest. @catch points out that we do that in a wrong way and should alter the block list instead. Rightfully. Generating the same output via an AJAX request would still require us to push all the data through themed in HTML or we'd need to reimplement the Drupal rendering system in JS to overcome downloading the whole rendered HTML. On #content, this supposed inability to enforce certain things in core and contrib came up with the toolbar issue as well. We are not doing a contrib module here, so if people do themes and those do not work with our tools they will figure out that we had an important requirement there.

4. Clicking links in the overlay, the overlay rightfully does not know about the nature of links. Therefore we have ways for modules to specify which links are not administrative and we assume all other links to be administrative on administrative pages. This is not the task of the overlay as you nicely explained, it does not know about the details and it does not need to know either.

@markus / @sun:

On minimizing the page output, modal frame API has a separate template for stuff opening in the modal frame, which we dropped in favor of reusing the original page template and just dropping some regions. This made it easier to adopt existing themes to the overlay, since the theme does not need to have specific optimization code to remove blocks generated to non-displayed regions. As said above, the overlay needs to do a better job at removing blocks earlier on, so that they are not generated. Also, I'd assume @sun run his D7 environment in a development mode without JS and CSS cache turned on, which as many people said can also be an issue here.

markus_petrux’s picture

To minimize the stuff that's generated during page template preprocess, maybe a way would be that there's an attribute that can be consulted in these preprocess functions so they know an overlay mode for the page has been requested, and then there are a few things that can be completely ignored, those that make no sense for an overlay output. This would solve the issue that we do not only not send certain stuff to overlays, but we also do nothing that consumes resources for nothing.

eigentor’s picture

While sun is taking on the role of the heretic here, he might have a point.

Stepping back for a moment from the technical implementation, we should reconsider what this overlay is trying to achieve.

1. User knows he can close this anytime to get back because he sees the page he just left through the transparent gray background
2. Help orientation
3. Provide a unified look to admin pages even when not using seven as admin theme. (black background unifies)

All user tests that have been performed so far, http://rufzeichen-online.de/user-testing-d7ux-screencasts-and-writeup clearly hint at that the users do not get point 1. Use it yourself, you'll see. Even if it is transparent, you just see pitch black. It feels like another page. (and if it would be more transparent, this would only be confusing) This leads to:

Is an overlay, that covers the entire page, anymore an overlay?
Technically yes, but we are targeting at UX.

for point 2. Orientation is a completely different issue, that should have all our focus.

So what do we gain by the overlay?
Faster loading time - no - rather slower, especially the initial load.
Better orientation? no, not atm.

As for point 3., the unified look: I actually like this a lot, as a clear "this is admin area" indicator. Still we don't need an overlay for that, but could load any admin theme into an Iframe without Javascript.

To get the user back to the page he entered the admin area from, I am sure we could find another implementation for (some kind of keeping the starting page as a destination argument?)

Technical implementation: I have seen all kinds of funny effects while using different iterations of the overlay. Pages do not load as expected, something is not loading at all, the newest is the "Window inside a window" effect, that duplicates the entire admin header (to be seen ont the first video)

Knowing about Drupal's many moving parts, even not being a coder, I'd say it is pretty hard to herd all these cats (moving parts) reliably into an overlay and have no strange effects. Is this really worth the pain?

ATM we are wasting time on a tricky technical matter that we should rather use on improving orientation, and rethinking what we are actually aiming for here.
Anyone involved in this, please do a quick user test with a non-involved person. It definitely changes your vieving angle.

Gábor Hojtsy’s picture

Testing feedback: the modules form or the admin/people filter form return to a non-overlay mode when submitted (and this is probably a pattern that applies to other forms too, but already noticed on these two).

Gábor Hojtsy’s picture

FileSize
44.11 KB

Window resizing works flawlessly most of the cases but it sometimes gets confused and can size the overlay too small or too wide. I cannot find a predictable reproduction recipe for this, so just posting this observation here. It did not seem to be related to what page I am on.

markboulton’s picture

@eigentor: Some feedback regarding your comments:

Point One: Steps have been taken to reduce the darkness of the overlay. This will help show the site beneath.

Point Two: Orientation. See below.

The overlay is response to our #1 UX principle: As a user I should know where I am. (eg, I'm in admin, overlaying my site. I can close this overlay and then my site is just there). This has performed well in all of the testing we've conducted to date. I'd argue the problems your users had were related to orientation, not necessarily the overlay. These issues of orientation will be helped enormously by the new IA, the breadcrumbs and the dashboard.

One thing that did not help the orientation was that the non-javascript version of the theme is being used in places (eg from Edit in place) - this of course is going to cause confusion. It's certainly my recommendation that this should be removed asap and be replaced with the overlay.

eigentor’s picture

@mark

Well, actually the users had no problems with the overlay. It is just they do not recognize it as such. To really verify that point I sure would have to target the tests more towards that.

The motivation of doubting the overlay is rather that of not so small technical problems. They may not be unsurmountable, but at the present they are using quite some of our limited dev power. And the main things we want to archieve do not need an overlay. We could do with or without the it. But what we have to work on is orientation inside the overlay-like interface.

What I would love to see is a lot more user testing, for we are right at the point where we need that. This would also overcome the maybe subjective outcomes of my tests.

webchick’s picture

I think what Mark's saying is that this testing already took place, and was shown to really help things, when taken in context with the whole of D7UX improvements, of which we are only about 70% there. I don't think that's a reason to throw on the brakes and not proceed further until we get *more* user testing, I think that's a reason to get this stuff polished up and in so that we can then test the whole instead of individual parts and make a sound decision at that point as to how we present admin stuff.

Bojhan’s picture

Ok so lets keep implementing

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
45.29 KB

This patch should address most, if not all, of the feedback above:

- Uses hook_block_list_alter() instead of hook_preprocess_page() now for performance.
- Works using any theme as an admin theme - does not look as nice using garland, but is acceptable, completely usable, and looks ok.
- Uses a smaller dialog width so that it is more apparent that the overlay is actually an overlay - it was too easy to miss the background before, no matter how transparent it was. I confirmed that the smaller dialog width was better using an extremely small and statistically insignificant test sample (1 person with the larger width, 1 person with the smaller width), and asking them to go from a user page to admin > config & modules > modules, enable the tracker module, and return to the original user page. I understand that this isn't really a scientific way to determine whether it's more usable this way or not, but it justified the change of width in my mind. :)
- Changed the weird redirection behavior - you were right, it was just weird, and is much better off removed and forgotten.
- Changed the resizing behavior to address the bug Gabor ran into in #97 - interesting problem involving negative or partially negative iframe widths (not a good idea).
- Added a real way to get rid of the toolbar - we were still calling toolbar_suppress(), which, unfortunately, was not an actual function and therefore did absolutely nothing.
- Fixed a few interesting JavaScript errors, which should also help to speed up the overlay.
- There is a class that can be added to links to have them open outside the overlay; this can be added automatically or manually to links coming from admin pages, but this is probably best to do in a followup patch to avoid this patch becoming huge.

Thanks everyone for the feedback, I hope we'll still be able to get this patch ready before code freeze. Also, please remember, if you're testing this patch outside the d7ux repository, you'll need to download the images from above.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review

I can't confirm those failures locally, retest?

eigentor’s picture

Ah, sounds good.
I guess the newest patch is always in the version in the google repository?

Status: Needs review » Needs work

The last submitted patch failed testing.

Gábor Hojtsy’s picture

FileSize
48.58 KB

The latest thinking on navigation around with Seven includes a breadcrumbs. (Now that #548806: No breadcrumbs in Seven is committed). So the overlay should also include a breadcrumb to help navigate around in the admin interface. Latest mockups for the overlay include the breadcrumb styled with the existing right arrow image in the theme.

Disregard the + button on the title here, that is related to customizable shortcuts. See #511286: D7UX: Implement customizable shortcuts.

Everett Zufelt’s picture

I haven't tested the most recent patch, but if the tabs are still being rendered outside of the iframe it will mean major accessibility problems for screen-reader userson the Mac.

mcrittenden’s picture

Subscribe.

eigentor’s picture

FileSize
47.47 KB
35.22 KB

Some Issues with the overlay:

when I open screens, e.g. for adding a node, the form is moved to the top a bit and the heading is lacking. This only goes away when you enter text into the title field. May be because I just dowonloaded

The resizing of the white area is a little jumpy. It's ok it works in steps, but you should include more of them, since I quickly get this (a bit wtf-y) display:

There must be something wrong with the calculation of the screen width.

+1 for reducing the width. I now clearly see it is an overlay. This might be a good compromise between confusing and making clear it is an overlay.

Though there is one issue, that is maybe unsolvable (and may be due to me using an old CRT display :P haven't counter-checked reliably on my notebook):

During the day, when the room is bright and there is much more light, the background appears almost pitch-black as I wrote earlier. But at night with only my lamp on, things are completely different, and I clearly see Garland shining through. Hard to find the right compromise here :(

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
46.42 KB

This patch should fix the problems pointed out by eigentor in #110 and by Gabor in #107. Also includes some minor CSS changes to Seven that were necessary to get the alignment to work properly both inside and outside the overlay.

I still cannot get it to fail tests (either on ubuntu or windows), so hopefully this one will pass. Otherwise, if anyone can confirm the failures / tell me which assertions are failing, that would be awesome.

webchick’s picture

Status: Needs review » Needs work

This needs a quick re-roll for the hook_page_build() patch that went in. Reviewing now.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
46.38 KB

Here's the quick reroll for the hook_page_build() patch (plus an unrelated conflict in tableheader.js).

dergachev’s picture

Status: Needs review » Needs work

This looks nice, but I'm concerned that users may accidentally lose a lot of work in the node creation form by hitting escape.
Either this should be restricted to less complex tasks, or a javascript warning should appear.
Also, perhaps "escaping" could just hide the overlay, so there's some way to bring it back?

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
46.56 KB

Fine, you want a confirmation box, here it is.

ronald_istos’s picture

Safari 4.01 pages are just stuck on "Loading..." on current head with only this patch applied.

cwgordon7’s picture

Try clearing your cache first. The problem should go away. Also make sure you're really on current head because the .once() patch broke all JavaScript in Drupal for a few hours yesterday.

Gábor Hojtsy’s picture

Works for me on Safari 4.0.3. (@istos: make sure to apply the security updates). I'd have two notes:

- I think the width of the toolbar is not as designed. It would be best to work towards implementing the design and tinker with it based on volume testing.
- The background and theming of the breadcrumbs were not as designed. At least it would be good to have it on white. We can hopefully fix the image arrow in place of » later, if not in this patch. See #107 above for the mock for the breadcrumb.

robertgarrigos’s picture

mmm trying this patch on a Mac (safari and firefox) and cannot see the cross button to close the overlay window. it should be there ,right, as I see in the screenshots?

Gábor Hojtsy’s picture

FileSize
6.47 KB

Uploading the remaining images needed to make the overlay close button, "loading" and shadows appear as intended. Copy these to the modules/overlay/images directory.

cwgordon7’s picture

FileSize
46.72 KB

And here's an updated patch for the breadcrumb styling.

robertgarrigos’s picture

Status: Needs review » Needs work
FileSize
20.01 KB

It works almost as expected on Mac (safari and firefox):

1.- I miss some padding when previewing a node (see the attached image).
2.- I cannot see the arrow in the breadcrumb. Definitely the image is missing in the previous .zip but I cannot tell if that's all what happens.
3.- When publishing a node a confirmation box appear asking permission to close the overlay because I might loose unsaved work. While this is useful when closing directly the overlay it's confusing when hitting the save button.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
46.81 KB

1. Whoops, fixed to be a more general CSS solution that should work in all cases of contributed-module junk thrown into pages.

2. This is really a seven theme issue and I would prefer to keep it out of this patch. The » would appear to be fine for the moment.

3. Yes, this was a bug, I added a Drupal overlay close skip warning flag which is used on the automatic closes.

The updated patch is back to needs review.

robertgarrigos’s picture

Status: Needs review » Reviewed & tested by the community

It works great now. thanks! we just need make sure that those images get shipped with the module also.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm. I'm getting some rather obvious funky behaviour testing this patch.

Table headers get momentarily duplicated when I flip back and forth between tabs:
Duplicate table headers

The next thing I did was go to "Find content", click the "Add new content" link, see the node form. Trying to use my trackpad to scroll down to the bottom of the form to find the save button resulted in some serious delays, but eventually made it.

Once I clicked it, I get this:
Blank screen

After about 30 seconds or so I finally get forwarded to node/2.

Performance on this seems unacceptably slow (FF 3.5/OSX). Is there something that can be done about this?

In general, it feels like this patch has not really been reviewed thoroughly before marking RTBC. :\

Everett Zufelt’s picture

Have any of the accessibility concerns in #60 been addressed in the current patch? Is Overlay set to be enabled by decault with a d7 install?

I feel that if there are still unaddressed accessibility issues that the overlay should not be enabled by default in a d7 install, if for no other reason than having it enabled may make it more difficult for some users to disable it.

ksenzee’s picture

re #126: I believe overlay will be enabled by default. However, usability issues can be addressed post-freeze, and that's definitely a usability issue.

This is a subscribe post in a cunning disguise. :)

robertgarrigos’s picture

@webchick: I did try my best to test this patch. I tested on OSX (safari 4 and firefox 3.5.2) and I'm not having, didn't have either, the problems you are having when flipping back and forth.

I did also the test of the 'find content', then click 'add content' and click on a type of node with no delays problems, both during my previous test and now again. It is true that I tried on two different sets of firefox: with and without firebug. And that with firebug enabled it is slower, yes, but many webs are slower with firebug enabled anyway. In any case, I had only a few seconds delay once of several clicks, and had no delay with firebug disabled. I've also navigated thru the overlay with no problem to other pages.

Maybe I should have asked for a test from someone else before marking this issue as RTBC, but I indeed didn't have any problem, so my test was, and is still, absolutely fine.

@Everett: sorry about this, but I did not read all the comments (my fault) and I was unaware of this. cwgordont7 states at #63 that hi doesn't know how to address it. I couldn't find any other reference to the accessibility issue, so I guess there has not been any change on this. I don't have the tools to test it myself either.

So leaving this issue as it is. Maybe someone else will do it better.... :-(

David_Rothstein’s picture

Some feedback while I subscribe...

  1. I am also seeing extreme slowness with the overlay (Firefox 3 on Linux). Most page loads in the overlay, especially the complicated ones (with forms, etc) are taking 15-20 seconds to load before it's possible to do anything with them, sometimes longer.
  2. Is Overlay set to be enabled by default with a d7 install?

    Currently the patch does enable it by default, but like anything else, that should be up for debate, no? :)

  3. One nice thing about having the overlay enabled is that it seems to solve some of the problems discussed at #546186: Migrate overall node admin theme setting to per-content type. That is, even if I configure the site so that content creation/editing pages are not shown using the admin theme, clicking on the "Add content" link in the toolbar still shows me these pages in the admin theme inside the overlay (although clicking the equivalent link on the main site does not). This is a good thing, because we obviously don't want lots of theme switching to go on inside the overlay.

    Because of that, if this patch is going to enable the overlay by default, then it should also remove this line of code that is currently in the default install profile:

    variable_set('node_admin_theme', '1');
    

    That feature is not a good one for most kinds of Drupal sites, and the overlay seems to remove whatever rationale there was for having that enabled by default in the first place.

  4. When trying the overlay using Garland (i.e., no separate admin theme), it looks pretty weird - the whole page is still rendering in the overlay rather than just the main content region. I think it would be a good idea to fix that as part of this initial patch, to be able to better evaluate what actual changes themes would need to make in order to work with the overlay.
RobLoach’s picture

Issue tags: +awesomeness

Adding the awesomeness tag, because it is awesomeness. You can remove the tag if you disagree. kkaefer and tha_sun would be good reviewers to have too ;-) .

+++ modules/overlay/overlay-parent.js	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,564 @@
+
+    // Attach on the .to-overlay class.
+    $('a.to-overlay:not(.overlay-processed)').addClass('overlay-processed').click(function() {
+

jQuery Once this stuff! :-)

$('a.to-overlay').once('overlay').click(function()) {
// We rock the socks!
});

+++ modules/overlay/overlay-parent.js	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,564 @@
+Drupal.overlay.open = function(options) {
+  var self = this;
+
+  // Just one overlay is allowed.
+  if (self.isOpen || $('#overlay-container').size()) {
+    return false;
+  }

jQuery Once might work here too ;-) .

+++ modules/overlay/overlay.module	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,245 @@
+      // Add required jQuery UI elements. Note that we don't use
+      // drupal_add_library() here, since we have no use for the CSS files added
+      // by the library.
+      drupal_add_js('misc/ui/ui.core.js', array('weight' => JS_LIBRARY + 5));
+      drupal_add_js('misc/ui/ui.dialog.js', array('weight' => JS_LIBRARY + 6));

The problem with just adding UI Core and any other jQuery UI files without the library is that it becomes problematic when we update jQuery UI through contrib (jQuery UI module in Drupal 7).

I tried using "drupal_add_library('system', 'ui.dialog') and it reverted to the default jQuery UI dialog styling. Not really wanted in this case, I guess. Maybe Overlay can be a special use case here, since it's meant to be part of Drupal core anyway.

+++ modules/node/node.pages.inc	1 Sep 2009 18:33:04 -0000
@@ -400,6 +400,15 @@ function theme_node_preview($node) {
@@ -419,6 +428,11 @@ function node_form_submit($form, &$form_
@@ -419,6 +428,11 @@ function node_form_submit($form, &$form_
     unset($form_state['rebuild']);
     $form_state['nid'] = $node->nid;
     $form_state['redirect'] = 'node/' . $node->nid;
+    // If the overlay module is enabled, use the overlay module's API to close
+    // the overlay.
+    if (module_exists('overlay')) {
+      overlay_close_dialog(TRUE);
+    }

I don't think this should really be touching Node module at all. It makes sense for what it is doing, but couldn't this live in a form alter in the Overlay module instead? We ran into this problem with Comment and Node modules a while ago and to my knowledge, the spaghetti was fixed. Can we move this stuff out of node module?

I'm on crack. Are you, too?

lut4rp’s picture

Status: Needs work » Reviewed & tested by the community

OK, I tested this with FF, Opera and Safari:
- get the images, you need to get the images (other than the latest patch file), see #1.
- clear out your browser cache, or just bypass cache for the overlay iframe, that solves MOST problems with the latest patch.

IMO, this is RTBC! \o/

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I just about locked up Dreditor, so here is my review from the bottom up, about halfway through. Still haven't gotten to major architecture stuff yet, but noticed a few kind of weird things. Have only scanned the issue, not read it in depth yet.

Also, apologies about my previous reply which a) sounded WAY snarkier than I intended, and b) was a problem on my end; I hadn't cleared my browser cache, and once I did that everything worked much better. :) Really sorry, Robert! I really wasn't meaning to take your head off! :)

Summary of major points:
- OMG this looks GREAT!*&@!@ I found no issues from a user-perspective while I was testing this patch.
- I think we should talk about where to invoke this API. Sticking calls to it in node module feels really weird.
- The hooks feel like they could use some clarification of the names as well as the PHPDoc.
- I'd like to talk about how to resolve the accessibility issues Everett brings up.

retrieving revision 1.25
diff -u -p -r1.25 tableheader.js
@@ -0,0 +1,245 @@
+      $settings = array(
+        'overlay' => array(
+          'shadowPath' => $base_path . drupal_get_path('module', 'overlay') . '/images/ovrly-shdw-bt-lt.png',
+        ),
+      );
+      drupal_add_js($settings, array('type' => 'setting'));
...
+      module_invoke_all('overlay_parent');

Why is this a setting and not just embedded in overlay css?

Also, it looks like shadowPath is not referenced anywhere else in the patch, so can likely be removed.

+++ modules/overlay/overlay.api.php	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,43 @@
+
+/**
+ * Allow modules to act when an overlay parent window is activated.
+ *
+ * @return
+ *   None.
+ */
+function hook_overlay_parent() {
+  // Add our custom JavaScript.
+  drupal_add_js(drupal_get_path('module', 'hook') . '/hook-overlay.js');
+}
+
+/**
+ * Allow modules to act when an overlay child window is activated.
+ *
+ * @return
+ *   None.
+ */
+function hook_overlay_child() {
+  // Use a different theme for content administration pages.
+  if (arg(0) == 'admin' && arg(1) == 'content') {
+    if ($theme = variable_get('content_administration_pages_theme', FALSE)) {
+      global $custom_theme;
+      $custom_theme = $theme;
+    }
+  }
+}

From reading the calling code, it was not clear enough to me exactly when/why these hooks are firing. Is it when the overlay is first being shown? Is it when the overlay appears? etc.

Both the name and the docs could use some work here. Ideally this would explain to me why I would implement this hook in my module.

+++ modules/overlay/overlay.module	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,245 @@
+ * we need to output the javascript that will tell the parent window to close

JavaScript

+++ modules/overlay/overlay.module	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,245 @@
+/**
+ * Set overlay mode and add proper Javascript and styles to the page.
+ *
+ * @ingroup overlay_api
+ */
+function overlay_mode($mode = NULL) {

Please document the possible values of $mode. Preferably with an overview of how this works, since there's a lot of discussion on this in the issue (and likely off the queue) about why things were done the way they were.

Maybe also change the name to "overlay_set_mode" if it's setting the mode.

+++ modules/overlay/overlay.module	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,245 @@
+  switch($mode) {

space between "switch" and (

+++ modules/overlay/overlay.module	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,245 @@
+      // Allow modules to act upon overlay events.
+      module_invoke_all('overlay_parent');
+      break;
...
+      // Allow modules to act upon overlay events.
+      module_invoke_all('overlay_child');
+      break;

Let's add a verb to this hook to make it more clear what's going on. overlay_parent_initialize()? That's kinda long, but something like that which tells me as a module developer what's going on in the guts of overlay module without reading the code.

+++ modules/overlay/overlay.module	1 Sep 2009 18:33:01 -0000
@@ -0,0 +1,245 @@
+      if (function_exists('toolbar_enabled') || drupal_function_exists('toolbar_enabled')) {

No more drupal_function_exists()

+++ modules/node/node.admin.inc	1 Sep 2009 18:33:03 -0000
@@ -447,6 +447,9 @@ function node_admin_nodes() {
+    // Set a class to flag to the overlay, if present, not to open the link in
+    // the overlay.
+    $options['attributes']['class'] = 'overlay-escape';

These are now ['classes'] afaik.

+++ modules/node/node.pages.inc	1 Sep 2009 18:33:04 -0000
@@ -400,6 +400,15 @@ function theme_node_preview($node) {
+  // Run all submit callbacks except the overlay, which we require to run later
+  // if present.
+  if ($key = array_search('overlay_form_submit', $form['#submit'])) {
+    unset($form['#submit'][$key]);
+  }
+  if ($key = array_search('overlay_form_submit', $form_state['submit_handlers'])) {
+    unset($form['submit_handlers'][$key]);
+  }
+
@@ -419,6 +428,11 @@ function node_form_submit($form, &$form_
+    // If the overlay module is enabled, use the overlay module's API to close
+    // the overlay.
+    if (module_exists('overlay')) {
+      overlay_close_dialog(TRUE);
+    }

I really don't like adding overlay module-specific code to Node module. Node module is a required module, and an API module. Overlay is nice shiny pretty.

Charlie pointed out in #53 that the Search module integration already works this way, so there is precedent in core. But I'm not sure... it feels like this isn't an "API" per-se. We should discuss this more.

-1 days to code freeze. Better review yourself.

webchick’s picture

Issue tags: -awesomeness

Oh, we encountered a bug. If you click "Find content", then click on a node title to view it, you'll get prompted to proceed or cancel because you'll lose your changes.

a) It'd be nice if it didn't do that, but I understand that fixing this is out of scope of this issue.
b) However, the "Cancel" button didn't have any effect, it still directed me to the page.

I know there's a contrib module somewhere that does this, so maybe you could look at that code for some inspiration?

Everett Zufelt’s picture

@webchick

I am definitely available to discuss accessibility and the overlay. Anyone interested in brainstorming solutions can reach me at everett@openconcept.ca to find a time to discuss over phone, skype or IRC.

cwgordon7’s picture

Issue tags: +awesomeness

Thanks everyone for all the feedback! Quick thought before I start addressing it all:

@Everett Zufelt - Just a quick thought, could we add one-pixel transparent images with appropriate alt texts ("Close" and the names of the various tabs, if present)? We could put those inside the overlay and they would not affect the experience of normal users but would appear for users with screen readers, right?

Also, adding back the awesomeness tag, I believe it was removed by mistake?

Everett Zufelt’s picture

@cwgordon7

The recommended approach to make content invisible to all but screen-reader users is to use class="element-invisible" whichs is defined in system.css as : .element-invisible {height: 0; overflow: hidden; position: absolute;}

I don't think that we should duplicate the tabs, as most screen-readers recognize them properly, only VoiceOver for the Mac does not recognize them, or more importantly, it recognizes them, but usrs are unlikely to find them.

I am not sure where focus is set when an overlay window opens, but can we set it to before the tabs, and not the iframe itself?

Also, is there a site where I can test the most recent patch so that I can give more detailed feedback on all of the issues I earlier identified?

robertgarrigos’s picture

@webchick: no problem :-)

webchick’s picture

Hey, Charlie! I know I owe you a review on this but I am just simply too zonked today, despite twix bars and blue gummy bears. :( It also seems like the next best step other than spending another half hour on the rest of the patch is to set up a demo site with this patch for Everett so he can take a look. Is that something you (or anyone else in this issue for that matter) could take care of?

int’s picture

Issue tags: +Exception code freeze

add tag

Everett Zufelt’s picture

I applied the patch in comment #123 to a clean copy of head. I tested with several browser / screen-reader pairs.

Test:

1. Enable overlay module.

2. Select "Content" toolbar item.

3. Select Add Content.

FF3.5 and JAWS10.0.1154

1. When the overlay loads JAWS buffer needs to be refreshed to find overlay and focus is not moved from Content toolbar menu item ** known problem with JAWS 10

2. At the bottom of the page (DOM order, the way screen-readers navigate the web) after the standard page footer I find:

a. "Content" * perhaps this and all overlay dialog titles could be wrapped in an h1 fore easier navigation. H1 is recommended as this is the beginning of completely new page content.

b. A close button (tested and working)

c. "Content" and "Comments" tabs (working and #521852: Local tasks lack semantic markup to indicate an active task will help screen-reader users tell which tab is active).

d. "overlay-element" frame. Could this frame be given a title that is more specific to the type of content loaded in the frame, for example "Administer Content Dialog"?

3. When clicking on Add Content the JAWS buffer needs to be refreshed (see bug in 1 above).

FF3.5 / NVDA 0.6p3.2

1. When the overlay loads focus is taken to the iframe element. The same elements that appear before the iframe with FF/JAWS appear before the iframe here and users would be unlikely to find them. Can the focus be set to the dialog title "Content" suggested to be a h1 heading above?

2. When clicking on "Add Content" the dialog is properly refreshed with the Add Content page and focus is taken to the top of the iframe, once again it would be better if focus was placed on the dialog title "Add Content" marked up as a h1 element.

IE6 / JAWS6

1. When the overlay loads focus is taken to just before the page footer (a couple lines above the "Content" dialog title).

2. The "Content" and "Comments" tabs do not appear.

3. Clicking on the "Close link (not a button anymore?) does not close the overlay, even after clicking Ok in the dialog which appears.

4. Clicking on Add content does load the Add Content dialog and focus is taken to just above the page footer as in 1 above.

Safari 4.0.1 / VoiceOver 10.5.7

1. When the overlay loads the focus is moved to the iframe and the user is "trapped" in the iframe. VoiceOver users an object interaction model where a user interacts with container objects in order to reduce the navigable area. It is possible for a user to stop interacting with the iframe by pressing Shift + Control + Option + Up, but users may not know that they are interacting with the iframe and may not know to do this. Another reason to place focus on the dialog title "Content" and not the iframe.

2. When I stop interacting with the iframe I find the dialog title, "Content" and "Comments" tabs and Close button (which works) directly before the iframe.

3. When I navigate back to the iframe, once again I notice the iframe has the title "overlay-element". It would likely make it easier for VoiceOver users to decide to interact with this iframe (once the focus issue in 1 above is dealt with) if the iframe had a title specific to the role of the dialog.

4. When I click on Add Content the Add Content page is loaded correctly and focus is taken to the top of the iframe (once again VoiceOver is "interacting" with the iframe).

Recommendations:

1. Wrap the dialog title in an h1 element.

2. Have focus placed on the dialog title when an overlay window is loaded.

3. Give the Overlay iframe a title specific to its current role e.g. "Content Administration dialog".

4. Figure out why the tabs are not appearing for the IE6/JAWS6 combination.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
49.13 KB

Thanks everyone for the reviews! Responses below and updated patch incorporating your feedback attached.

  • #129
    1. I am also using firefox 3 on linux and cannot reproduce. Please try clearing your browser cache, this apparently fixed webchick's similar performance problems on the mac.
    2. Up for debate, yes, but I think we want this enabled by default, otherwise no one will take advantage of it, so I have left it enabled by default for now.
    3. I took your suggestion and changed this.
    4. I fixed up garland, it now looks rather nifty in the overlay. Also apparently the block hook name had changed so I changed that as well.
  • #130
    1. jQuery onced that stuff.
    2. I don't think so... at least I don't see how. We're not providing a -processed class here, we're just saying that only one element with the id overlay-container can exist at the same time. If you'd like to demonstrate how to jQuery once this, I'd welcome it.
    3. Perhaps core should provide a way to add JavaScript from libraries without adding CSS then... either way not a problem with this patch I guess.
    4. I explained this above, node module implements the overlay module's API, not the other way around. There is a precedent for this in node_delete_multiple(), where search module is asked to update the index - search module could just use hook_node_delete(), but that would be conceptually wrong. It is a similar case here, and I don't think you can make a case that the search module is correct in doing this and the overlay module is not - either they both need to go, or they are both correct.
  • #132
    1. Removed the dead shadowPath code snippet, nice catch.
    2. Improved the documentation both in overlay.api.php and for overlay_mode(). I also renamed the hooks to _initialize as you suggested.
    3. Indeed, JavaScript (also fixed in a few other places).
    4. See #2 above.
    5. Added the space, sorry about that!
    6. Yeah, removed the reference and changed it to a simple module_exists('toolbar').
    7. Not to disagree, but not afaik. It currently works with 'class', I tried switching it to 'classes' and it didn't work, and I don't see anything special and drupal_attributes() to handle 'classes', so I kept it the same.
    8. I addressed the API issue above, but this seems to be a pretty common objection to this patch. If you've listened to my reasoning but still object, then I will remove the overlay-specific reference from node.module if we agree that the search-specific reference from node.module should also be removed. I do not think that the overlay module should be a second-class optional core module - it should have the same rights to be in node.module as the search module does.
  • #133
    1. Fixed this bug. Also added an overly-simple test that checks for the presence of a form on the overlay child page - if no forms are present it skips the prompt. This is a rather simple check but I feel any sort of more complicated unsaved-progress-alert JavaScript should go in a separate issue, and this saves you the trouble of confirming you meant to close the overlay in a number of cases.
  • #137
    1. I wrapped the dialog title in an h1 element - tricky because the span is added by jQuery UI dialogs, not our code, but I managed to override it using a jQuery replaceWith.
    2. I changed the code to place focus on the dialog title when an overlay window is loaded.
    3. I gave the Overlay iframe a title specific to it's current role, which is taken from the page title and transformed into the form of "Title" dialog.
    4. I don't know why it's not working on IE6/JAWS6 combination and am not in a position to figure it out. If you have any suggestions on what I can do to fix the tabs on IE6/JAWS6, that would be awesome, but I don't think this should be considered a critical stopper for this issue if it works in the majority of screen readers (some other Drupal JavaScript behaviors specifically abort on IE6, so there is precedent for this).

I think this is up for needs review again, as I believe I have addressed all outstanding concerns. Thanks!

markus_petrux’s picture

1. In reply to comment #132/2 in #141 above: "I changed the code to place focus on the dialog title when an overlay window is loaded."

The following CSS would prevent a border on focus in Mozilla.

/* Prevent a border on focus in Mozilla. */
#ui-dialog-title-overlay-container {
  outline: 0;
}

2. This replaceWith() trick could be moved to the open event handler of the dialog settings, in Drupal.overlay.create(). I mean just after self.iframe.$container.dialog({. That way, the jQuery html() method could be used in the place where it is now using replaceWith(). Note that bindChild() method could be called several times, after a child document has been loaded, but at that time, only the text of the title needs to change. The actual HTML tag used for the title can be set just once during the life time of the modal dialog if using the open() callback of the dialog.

3. tabInxed could probably be tabindex. I also made this mistake in modal frame api. Oops!

4. This is looking impressive! :)

catch’s picture

There is a precedent for this in node_delete_multiple(), where search module is asked to update the index - search module could just use hook_node_delete(), but that would be conceptually wrong. It is a similar case here, and I don't think you can make a case that the search module is correct in doing this and the overlay module is not - either they both need to go, or they are both correct.

Search module needs to get out of node module, so both are incorrect.

cwgordon7’s picture

FileSize
49.5 KB

Thanks, updated to incorporate the suggestions in #142 and #143.

Everett Zufelt’s picture

I tested the patch in #144 with clean head.

This patch addresses the recommendations 1-3 made in #140.

For the title of the iframe, I don't think the quotes are necessary around the name of the node e.g. '"Content" Dialog' should be 'Content dialog'.

As for 140.4 (IE6 / JAWS6), I think that this is a critical issue, I am not familiar with other JS malfunctions, but not having access to the local tasks which control the content presented in the overlay dialog does appear to be critical to me.

If someone can show me the code that makes these local tasks appear I can do some research to see if I can determine the source of the problem.

cwgordon7’s picture

FileSize
49.47 KB

The code that runs the tabs is:

    var tabs = $iFrameDocument.find('ul.primary').get(0);

    // If there are tabs in the page, move them to the titlebar.
    if (typeof tabs != 'undefined') {
      $('.ui-dialog-titlebar').append($(tabs).remove().get(0));
      if ($(tabs).is('.primary')) {
        $(tabs).find('a').addClass('to-overlay').removeClass('overlay-processed');
        Drupal.attachBehaviors($(tabs));
      }
      // Remove any classes from the list element to avoid theme styles
      // clashing with our styling.
      $(tabs).removeAttr('class');
    }

Also updated the patch to just say Content dialog rather than "Content" dialog as per the suggestion in #145.

Status: Needs review » Needs work

The last submitted patch failed testing.

marcvangend’s picture

Status: Needs work » Needs review
FileSize
49.8 KB

I noticed two issues with the breadcrumbs in the overlay.

1. The breadcrumbs contains a 'Home' link which opens your home page in the iframe. I considered adding javacript to return to the home page in the parent frame, but that would be awkward because two things happen at the same time (close overlay and load the home page in the parent). We (drupalcon codesprinters) agreed that the best solution is to take out the Home link completely in the overlay.

2. Contrary to most themes, the breadcrumbs are below the title. That's why we figured it's more logical to have the page title appended to the end of the breadcrumbs (text, not as a link) - but only in case there is a breadcrumb.

I added the following lines to overlay_preprocess_page():

    // Remove 'Home' from the breadcrumbs and add the current page title at the end.
    $overlay_breadcrumb = drupal_get_breadcrumb();
    array_shift($overlay_breadcrumb);
    if ($overlay_breadcrumb) {
      $overlay_breadcrumb[] = $variables['title'];
    }
    $variables['breadcrumb'] = theme('breadcrumb', $overlay_breadcrumb);

New patch attached.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
48.89 KB

Left an extra print 'here' in there, sorry.

Everett Zufelt’s picture

Status: Needs review » Needs work

@cwgordon7

I think that the problem for IE6/JAWS6 is with the removing and replacing the ul in the DOM. I recall speaking to the folks at http://fluidproject.org who had a similar problem. I will see if I can get more information from them and report back.

Is this the only place in the Overlay where a removed DOM element is being added back into the DOM?

Everett Zufelt’s picture

Status: Needs work » Needs review

Setting back to NR.

cwgordon7’s picture

FileSize
49.06 KB

Ok if someone here is working on the overlay at the codesprint come find me / ping me in IRC to figure out where I am. Here's a patch that incorporates both changes.

cwgordon7’s picture

@Everett - yes, that is the only case of removing and readding content to the page, as far as I know.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review

Try again.

cwgordon7’s picture

FileSize
49.22 KB

Had accidentally removed function overlay_close_dialog() in the previous patch, thanks to Manuel Garcia for finding this bug. Attached patch should fix that bug.

Manuel Garcia’s picture

Just confirming last patch takes care of that bug =) nice job

Manuel Garcia’s picture

In the comment administration part of the admin/content, if you have no comments it will cause problems hiting the update button. This however is not a bug with the overlay, but rather with comment module, and I have already filled in a bug report for it. Just for your info =)
#569196: Comment bulk update: warning if no comments selected.

cwgordon7’s picture

FileSize
48.98 KB

Rerolled for the new hook_drupal_goto_alter(), which should coincidentally fix the problem mentioned in #159, as well as a problem for the node/add form if you only had one node type.

Everett Zufelt’s picture

I did some local testing with appending removed elements with jQuery re: IE6/JAWS6. It looks like this is indeed the problem.

I'm pretty sure that there is going to have to be another solution for moving the local tasks to the title bar of the dialog, as the remove() / append method is not going to work for users of older versions of JAWS.

cwgordon7’s picture

FileSize
49.11 KB

Ok, lut4rp just tested on IE6 without JAWS, and it does not work either, so special cased IE < 7 to not move the tabs, which should solve all the accessibility issues? :)

Manuel Garcia’s picture

Just confirming that patch #160 fixes the issue mentioned in #159

Manuel Garcia’s picture

A minor thing that should be fixed is the dotted border on active title of the overlays, this can be fixed by adding:

.overlay .ui-dialog-title:active,
.overlay .ui-dialog-title:focus {
  outline: 0;
}

on overlay-parent.css.

Basically that technique can be used for any element we don't want to highlight through the standard outline that the browser uses.

Everett Zufelt’s picture

Tested the patch in #162 and would give this a +1 for accessibility. There are a few cleanup tiny accessibility improvements that can be dealt with in a new issue when this is committed.

@Manuel

I would recommend against removing the dotted border, as this may be useful to keyboard only non-screen-reader users to identify where they can find the focus.

Great work on improving the accessibility of this module.

marcvangend’s picture

@Everett: I appreciate your concerns about accessibility and the focus border, but I think it's not really doing anything there. AFAIC, there is no good reason to focus on that title after loading an overlay page. In fact, that title is outside the overlay's iframe, so I guess that screen readers would have a hard time with that anyway.

marcvangend’s picture

I think we still have to do something about the blocks/regions that are shown in the overlay. Since 'content' is an actual region, other blocks in that region are currently shown as well. Probably all blocks should be unset except system-main and system-help. I'm not attaching a new patch yet because I'd like to know what you all think.

Damien Tournoud’s picture

FileSize
140.02 KB

Overlays inside overlays: open the overlay, click administer (in the breadcrumb), click "by modules" and click on "configure permissions". Tada!

Weird overlay.png

Damien Tournoud’s picture

Another concern that I have is about the shadow. Outputting two divs from javascript just for the shadow is pretty ugly. I suggest we use CSS3 to generate the shadow, and simply do not bother with IE6/7/8. It will all look as great without the shadow anyway.

sun added that all the HTML markup that is generated by the javascript should go through Drupal.theme(), but I personally consider this to be a minor issue.

marcvangend’s picture

FileSize
49.23 KB

New patch, implementing #164 and #167.

cwgordon7’s picture

@Damien Tournoud - how do you suggest we use CSS3 to avoid the extra divs? Sorry if I'm being slow, but I don't see how that would work.

New patch fixing #168 coming when I get home tomorrow, hopefully. (I've got the code here, but CVS is insanely slow and Internet is charging per minute).

Damien Tournoud’s picture

@cwgordon7: I meant, just use the box-shadow property, and be done with it. It is supported by FF3.5/Safari 3+. The design without the shadow will be good enough for every other browser.

marcvangend’s picture

@Damien: I more or less agree with your point about the shadows; extra divs aren't my favorite and the design still looks fine without shadows. However, using the box-shadow property (or rather: -moz-box-shadow and -webkit-box-shadow) will not work with the current design, because there are rounded corners in it.

cwgordon7’s picture

According to the link, we can do rounded corners if we also use border-radius?

eigentor’s picture

as border-radius is also not supported by older browsers this may cause a headache :(
CSS-Gods to the rescue...

marcvangend’s picture

@#174: you're right, I didn't think of that. You could give the close button a property like "-moz-border-radius: 0 100% 100% 0;" to round the box and it's css drop shadow.
@#175: On older browsers, both the rounded corner and the drop shadow would disappear, so following the line of thought in #169, this will actually save us one or two headaches.

eigentor’s picture

Anyone can make a screenshot how this looks without rounded corners? Would help a decision...

xmacinfo’s picture

The main Seven admin theme uses border-radius and falls back nicely to square angles for browser that do not support border-radius.

So here we should use box-shadow as well as radius-border as well. Older browsers will fall back to what they support.

marcvangend’s picture

FileSize
33.89 KB
32.61 KB

@#177:
Here are two images, one showing what you can achieve with css3 properties box-shadow and border-radius, the other showing what that same page would look like on browsers that only support css2. These screenshots are made using Firefox 3.5 and Firebug to manipulate the css, without the shadow images from the zip in the original post.

eigentor’s picture

Hm, this is a bit of a hard one. The lack of rounding on the close button really changes aesthetics.
So let's have a look for statistics.
According to this (Firefox only counts when version is 3.5)
http://marketshare.hitslink.com/browser-market-share.aspx?qprid=2
only 14% of the people would see the nice rounded corner :(
ah, no, IE8 supports it as well?
Then it would be 29%

marcvangend’s picture

I just did some more css/firebug tests and i found a way to make the rounded corner work in every browser. I used a close button image that is rounded, but without shadow. When that is used as background image of the <a>, you can then round it's corners and apply box-shadow to create the desired shadow effect, while older browsers still see the rounded image.

By the way, when we look at today's browser statistics, remember that probably D7 will not be finished before 2010 and that it will still be in use in 2012.

eigentor’s picture

@marcvanged: this solution sounds good.
Yeah, hopefully we will see a lot of switching from Firefox 3.0 to 3.5, which would get us close to 50% :)
Have almost given up hope that Microsoft will manage to promote their own browser. Windows 7 might be a ray of hope since it generally gets good perception and maybe even I might upgrade from my beloved XP...

cwgordon7’s picture

FileSize
46.42 KB

This patch should fix all outstanding problems. It now uses box-shadow CSS property instead of images, so a new image zip is also attached. I decided not to use the border-radius CSS property because (a) it looks fine without it, you can barely notice a difference with it on, and (b) it caused the dialog to look weird when at first it had a border-radius when it was just a div and then the border-radius vanished when the actual iframe loaded. This also fixes the problem relating to links with fragments.

cwgordon7’s picture

FileSize
5.1 KB

And the images. :)

eigentor’s picture

Should we create a meta-issue for the overlay and split it up into smaller ones?

There are some recurring issues, that need to be fixed one by one. (window-in-a-window, top padding missing, a.s.o.)
The patch is not a small one and seperating it into several smaller might help.

Also it might be hard for charly to keep track of all the different issues posted here, it is easy to overlook stuff.

I ask before I do this because it does not help to have even more issues some time....

Damien Tournoud’s picture

Status: Needs review » Needs work

I think this is nearly good enough so that we could commit this, and fix the remaining issues separately.

One thing that need to be fixed: there are still remains of the div shadows in the patch.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
45.88 KB

Sorry about that, I think I removed the last remains of the div shadows in this one. The images from #184 are still the current images.

mcrittenden’s picture

+1 for committing this in its current state and catching the remaining problems in follow up issues. This patch will last quite a while longer if we try to get it absolutely perfect here.

So if testbot likes #187 patch, then let's RTBC?

Damien Tournoud’s picture

marcvangend’s picture

FileSize
3.95 KB
45.92 KB

One more iteration of the patch... I changed three things.

1) The overlay container had a CSS drop shadow, but the close button still had a drop shadow baked into the png. I applied a css dropshadow to the close button using the method described in #181. This looks good in both older and newer browsers. I'm attaching a new set of images containing a shadowless close button.

2) Change 1) made it possible to remove div.ui-dialog-titlebar-close-bg, which is why we started with the css3 properties in the first place.

3) I added a color (rgba(0,0,0,0.5)) to the box-shadow declarations. According to the CSS3 specs, user agents are allowed to define their own default color, so figured we'd better take control ourselves.

@#188: yes, I think this is RTBC now.

Status: Needs review » Needs work

The last submitted patch failed testing.

marcvangend’s picture

Status: Needs work » Needs review
FileSize
45.97 KB

Sorry, I made an error in that last patch (forgot one line of CSS). Please use this one together with the images from #190.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Something in your patch is failing to apply... are you rolling against an up to date Drupal HEAD?

sun’s picture

Some files in the patch are cut off.

cwgordon7’s picture

yeah... marcvangend, you need to fix this, I've got no idea how to reroll from your broken patch state. I'd be happy to make a reroll from my latest patch, but it seems like you've made substantial changes...

marcvangend’s picture

Status: Needs work » Needs review
FileSize
45.97 KB

Yes, I did roll against the latest head, and on my machine I was able to apply that patch on a clean checkout... Anyway, I did a new one, hope this works now.

(I guess I still have some things to learn about the simpletest process... What's the best way to test a patch before uploading it to the issue queue? I tend to test it, but obviously my method isn't fail-safe.)

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

marcvangend, some of your files are being cut off, such as overlay-parent.css(?) Please come onto IRC so we can help you debug?

marcvangend’s picture

Status: Needs work » Needs review
FileSize
47.62 KB

Sorry for the mess guys, this doesn't look pretty for someting that is supposed to be almost RTBC :-)

Anyway, I used another tool now to create the patch, let's hope this works. I'll be back tomorrow, it's time for bed now.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review

Try again, please?

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

I was not able to test this manually due to the broken patch file. However, here comes a (partial) code review upfront:

+++ overlay-parent.css
@@ -0,0 +1,139 @@
+  -webkit-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
+  -moz-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
+  box-shadow: 8px 8px 8px rgba(0,0,0,.5);
...
+  -webkit-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
+  -moz-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
+  box-shadow: 8px 8px 8px rgba(0,0,0,.5);

Please squeeze some spaces in those rgba() values.

+++ overlay-parent.js
@@ -0,0 +1,557 @@
+/**
+ * Overlay object for parent windows.
+ */
+Drupal.overlay = Drupal.overlay || {
...
+/**
+ * Open an overlay.
+ */
+Drupal.overlay.open = function(options) {
...
+/**
+ * Create the overlay.
+ */
+Drupal.overlay.create = function() {
...
+/**
+ * Load the given URL into the dialog iframe.
+ */
+Drupal.overlay.load = function(url) {
...
+/**
+ * Check if the dialog can be closed.
+ */
+Drupal.overlay.canClose = function() {
...
+/**
+ * Close the overlay.
+ */
+Drupal.overlay.close = function(statusMessages) {
...
+Drupal.overlay.redirect = function(link) {
...
+/**
+ * Bind the child window.
+ */
+Drupal.overlay.bindChild = function(iFrameWindow, isClosing) {
...
+/**
+ * Unbind the child window.
+ */
+Drupal.overlay.unbindChild = function(iFrameWindow) {

The JSDoc needs more information - why, when, how, from where; also don't forget to document @param.

+++ overlay-parent.js
@@ -0,0 +1,557 @@
+/**
+ * Sanitize dialog size.
+ */
+Drupal.overlay.sanitizeSize = function(size) {

At least the JSDoc needs more information; also don't forget to document @param.

Some inline comments describing the calculations in there wouldn't hurt either.

+++ overlay-parent.js
@@ -0,0 +1,557 @@
+      self.iframe.$container.dialog('option', {width: dialogSize.width, height: dialogSize.height});

(and elsewhere) Object/array notation should start with a space before the first element and should end with a space after the last element's value. (Does not apply to empty objects/arrays.)

+++ overlay-parent.js
@@ -0,0 +1,557 @@
+      // Animate body opacity, so we fade in the page page as it loads in. 

Duplicate "page".

+++ overlay.module
@@ -0,0 +1,285 @@
+/**
+ * Displaying an overlay parent window.
+ */
+define('OVERLAY_PARENT', 0);
+
+/**
+ * Displaying an overlay child window.
+ */
+define('OVERLAY_CHILD', 1);

I fail to see why we need to pollute defines with those.

We can just use 'parent' and 'child' as strings instead.

+++ overlay.module
@@ -0,0 +1,285 @@
+/**
+ * Implement hook_init().
+ */
+function overlay_init() {
...
+    if (isset($_GET['render']) && $_GET['render'] == 'overlay') {
+      $admin_theme = variable_get('admin_theme', 0);
+      if ($custom_theme != $admin_theme) {
+        // If system module did not switch the theme yet (i.e. this is not an
+        // admin page, per se), we should switch the theme here.
+        $custom_theme = $admin_theme;
+        drupal_add_css(drupal_get_path('module', 'system') . '/admin.css');
+      }

I don't think that this is the right place and module to switch themes - at least not unconditionally. This should be configurable. And ideally triggered by a query string containing the theme name.

+++ overlay.module
@@ -0,0 +1,285 @@
+function overlay_init() {
+  global $custom_theme;
+  // Only act if the user has access to administration pages. Other modules can
+  // also enable the overlay directly for other uses of the JavaScript.
+  if (user_access('access administration pages')) {
...
+    else {
+      // Otherwise add overlay parent code and our behavior.
+      overlay_mode(OVERLAY_PARENT);
+    }

Access to administration pages != using overlay.

Your mileage may vary, but there are countless of use-cases for Drupal. We have to cater for those.

+++ overlay.module
@@ -0,0 +1,285 @@
+function overlay_block_info_alter(&$blocks) {
+  if (overlay_mode() == OVERLAY_CHILD) {
+    // Don't show any blocks except the main page content and the help text if
+    // we're in the overlay.
+    foreach ($blocks as $bid => $block) {
+      if (!($block->module == 'system' && in_array($block->delta, array('main', 'help')))) {
+        unset($blocks[$bid]);  
+      }
+    }
+  }
+}

Woah. That needs to be toggleable or configurable.

Just because that Seven theme doesn't implement any regions that doesn't mean...

+++ overlay.module
@@ -0,0 +1,285 @@
+    // Remove 'Home' from the breadcrumbs and add the current page title at the end.
+    $overlay_breadcrumb = drupal_get_breadcrumb();
+    array_shift($overlay_breadcrumb);
+    if ($overlay_breadcrumb) {
+      $overlay_breadcrumb[] = $variables['title'];
+    }
+    $variables['breadcrumb'] = theme('breadcrumb', $overlay_breadcrumb);

Why is the current page added in the breadcrumb? That is against the current paradigm/understanding of breadcrumbs in Drupal. If we want to change that, then we should change it in drupal_set_breadcrumb().

+++ overlay.module
@@ -0,0 +1,285 @@
+/**
+ * Form after build callback.
+ *
+ * Ok, all hook_form_alter() have been processed. Now, if someone has enabled
+ * the global variable $GLOBALS['overlay_page_template'], then we want to
+ * scan the form structure in search of elements with submit handlers.
...
+function overlay_form_after_build($form, &$form_state) {

This PHPDoc description could use some rewording using less emotions.

The same applies to the inline comment in this function.

+++ overlay.module
@@ -0,0 +1,285 @@
+/**
+ * Implement hook_node_insert().
+ */
+function overlay_node_insert($node) {
+  // If we are within the overlay, close the dialog. This cannot be done within
+  // hook_form_alter() because the node module forces the submit handlers to
+  // run before the node form submit handler, so the node would not be saved if
+  // the overlay forced the page to print and close the overlay and redirect
+  // early.
+  if (overlay_mode() == OVERLAY_CHILD) {
+    overlay_close_dialog(TRUE);
+  }
+}
+
+/**
+ * Implement hook_node_update().
+ */
+function overlay_node_update($node) {
+  // If we are within the overlay, close the dialog. This cannot be done within
+  // hook_form_alter() because the node module forces the submit handlers to
+  // run before the node form submit handler, so the node would not be saved if
+  // the overlay forced the page to print and close the overlay and redirect
+  // early.
+  if (overlay_mode() == OVERLAY_CHILD) {
+    overlay_close_dialog(TRUE);
+  }
+}

Since we have hook_drupal_goto_alter() now -- proposal: populate a drupal_static() in form_alter and check that in overlay_goto_alter() to omit/alter drupal_goto() and close the dialog on the same or next request's output.

+++ overlay.module
@@ -0,0 +1,285 @@
+      // Add required jQuery UI elements. Note that we don't use
+      // drupal_add_library() here, since we have no use for the CSS files added
+      // by the library.
+      drupal_add_js('misc/ui/ui.core.js', array('weight' => JS_LIBRARY + 5));
+      drupal_add_js('misc/ui/ui.dialog.js', array('weight' => JS_LIBRARY + 6));
...
+      // This is required to get access to jQuery UI extensions to jQuery itself,
+      // such as the ':focusable' and ':tabbable' selectors. No need for the whole
+      // library, so not using drupal_add_library().
+      drupal_add_js('misc/ui/ui.core.js', array('weight' => JS_LIBRARY + 5));

This is hi-jacking the entire idea and concept of a library registry. Any alterations to those library files will not be taken into account here.

a) Just use drupal_add_library() or

b) open a very critical bug report to fix this limitation of the library registry - which was already mentioned in some other issues.

Additionally, all of this code, except of the $mode handling, should be moved into overlay_preprocess_page() - using the #attached property to properly load those things only on pages.

+++ overlay.module
@@ -0,0 +1,285 @@
+      drupal_add_css($module_path . '/overlay-parent.css');
+      drupal_add_js($module_path . '/overlay-parent.js');
...
+      // Add JavaScript to the child page.
+      drupal_add_js($module_path . '/overlay-child.js');

This should be registered and loaded as a library.

+++ overlay.module
@@ -0,0 +1,285 @@
+    case OVERLAY_CHILD:
+      // Disable admin toolbar, which is something child windows don't need and
+      // shouldn't have.
+      if (module_exists('toolbar')) {
+        toolbar_enabled(FALSE);
+      }
...
+      // Allow modules to act upon overlay events.
+      module_invoke_all('overlay_child_initialize');

This needs to be moved into a hook implementation of Toolbar module.

This review is powered by Dreditor.

eigentor’s picture

Posted a follow-up: #574338: Overlay: put all admin screens into overlay optics

The screens must be unified. I see having some screens in seven theme only (all white) and some in overlay/overlay optics as a ux bug, just the same as mark expresses in #65.

seutje’s picture

FileSize
46.08 KB

@200 the overlay files seemed to have gotten dumped in the root instead of modules/overlay

attached patch should fix that and apply to HEAD

quick visual review:

  • I'm missing a close/cancel button
  • When opening a long page in the overlay (like Add content -> Page) and then going back to a short page (like Add content), I have a ton of leftover space at the bottom, causing an unnecessary scrollbar
seutje’s picture

Status: Needs work » Needs review

go testbot!

seutje’s picture

oh, I just overlooked the images... again, sry

can we have a fallback by clicking the black background or something?

heather’s picture

FileSize
24.01 KB
15.59 KB

FYI: This patch is added to a list of patches which urgently need user testing. http://groups.drupal.org/node/26409

Closing broken?

This is the first time I tried this overlay. I clicked on the black area to try and close it. It didn't close. I am familiar with the use of lightbox/shadowboxes on various sites. These 'escape' on click of outside/black area. This could be why I am expecting this behaviour.

CSS

This spacing is affected in the site when overlay.patch is applied (enabled or not).

Spacing within the overlay:

FYI: In case it's not obvious to other noobs, you have to download head, apply the patch; dump your database and reinstall d7 to test this patch. That way, it should be enabled by default. I also tried enabling it, that works ok too.

Manuel Garcia’s picture

I've been playing around with patch #206, (btw, place the folder in images.zip inside /modules/overlay/).

I've clicked on every page and config i could get my hands on, couldn't find any bugs, so to me this is looking good, at least functionaly there are no problems.

There seems to be a weird effect when you have scrolled down on the overlay, and you scroll back up, the bar on top kinda sticks a bit before moving up, using ff 3.5... not sure its an overlay problem or what, nothing critical that could stop this patch from being commited imho =)

Can we get this in, then handle other small display bugs as separate issues perhaps? Any brave souls want to change the status to reviewed? :>

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

Taking this on to implement hopefully most of the recent feedback. Will come back with an updated patch later today.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
FileSize
101.39 KB
66.35 KB
49.5 KB

So here is the patch based on your feedback from above. Notes:

Fixed:

- Removed page title from breadcrumb based on feedback from @sun
- Fixed preprocess hooks for html.tpl.php, the head title and body class should be preprocessed there
- Removed CSS border radius and shadow on close button since it was already in the image; extended the image size to show up
- Fixed spacing issues noted by @heather
- Based on feedback from @heather/@seutje: now that we have a notification tool to warn people when they try to close overlay with forms, I've added back Charlie's code to close the overlay on click; we might still want to extend the click area of the tabs as suggested by Mark to avoid people trying to click on them and closing the overlay
- rgba() spaces added as per @sun
- more jsdoc added per @sun (probably not as much as he would have liked though)
- fixed object notation whitespaces and "page page" duplication per @sun
- removed overlay mode constants in favor of strings but I'm not sure this was the best thing to do (suggested by @sun)

Not fixed/debated:

- @seutje: When opening a long page in the overlay (like Add content -> Page) and then going back to a short page (like Add content), I have a ton of leftover space at the bottom, causing an unnecessary scrollbar
- @sun: changing the theme in overlay_init(); not sure we should allow theme name passage in the URL, that would let all kinds of pranks to happen; we could use a different variable_get() and not provide a UI for that to have an "overlay theme", and just use the admin theme for that in core (let contribs provide a UI for that setting if they want);
- @sun: on user_access('access administration pages'), not sure what you suggest; should we have a separate permission for the overlay? how would you suggest identifying that the user is allowed to see the overlay
- @sun: on the regions allowed being hardwired, I don't think it does make any sense to have a UI there since this looks to be a theme vs. overlay specific issue; sounds more like themes would want to specify which regions to show in the overlay; since themes cannot have hooks, we can only work around this by new .info file items, which sounds bad (but I don't have a clearly better idea)
- @sun's "Since we have hook_drupal_goto_alter() now -- proposal: populate a drupal_static() in form_alter and check that in overlay_goto_alter() to omit/alter drupal_goto() and close the dialog on the same or next request's output." - not sure that is a less hackish solution then what we have now
- @sun's advice to use hook_library() fails badly on all the extra stuff that a library adds; we'd need to work hard to override all that just to make the overlay look sane again, adding twice as much useless styles to the page; sure we can go on a tangent to fix the library APIs or we can just commit this and let the library API cleanup come around and rework this based on libraries once it is capable enough (see below for screenshot of how it looks with libraries == the extra CSS added)
- @sun's advice to using #attached to add the JS in our preprocess seem to break down on the core preprocess being called before ours so the CSS/HTML string is already built once we get called to build that (from http://drupal.org/node/223430)
- @sun's advice on making libraries of the overlay files is not compatible with his advice to use preprocess (no way to add in libraries with #attach, right?), so let's clear up what is the real goal here; many ways to do the same perfectly applies
- on whether toolbar knows about the overlay or overlay knows about the toolbar to disable it in the overlay, I'm not entirely sure, we can do either way; others' opinions?

In pictures, here are the problems of the patch before my fixes in Safari:

Note that the tab spacing I did not fix (it is a long lingering issue and is not major). The others I did fix.

Here is how it looks if we add the whole jQuery UI library. We could add tons of overrides on the CSS, so we override the loaded useless CSS with our override CSS and we have twice as much useless CSS added. Instead, by not adding that useless CSS, we don't get this "nice" look:

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

The locale.test part from previous patches appears to have been removed from this one? Also, there is a way to add libraries using #attached_library (see #505084: Add #attached_library FAPI property for drupal_add_library()), but I don't believe we want to do that because we don't want to add the library-specific CSS files.

Note that the locale tests fail because this patch pushes the count of objects up to 10, which is not properly captured by the original regular expression (though single-digit integers are).

Gábor Hojtsy’s picture

Uhm, ok, my bad. I did not notice the relevance of the locale patch. Can be added back in for sure.

seutje’s picture

awesomeness!

tried Gábor's patch in #212 and I have a few additional notes:

  • When having the overlay open, and clicking another link, I noticed that the loading image is shown in the middle of the overlay, but before it resizes to a rly tiny bar, making the image kinda invisible and misplaced, also wouldn't it be better to change the title text to the loading text untill the full content is loaded?
  • when expanding a collapsed fieldset (I tried on a certain block's config page), there is an excess of whitepace at the bottom and when the fieldset is closed again, this whitespace stays. It doesn't stack every time u open a fieldset though, but this occasionally pushes the content up causing the top of the page to fall out of the viewport
  • the blocks admin page is rendered in the overlay, but in garland with all regions exposed (not sure if this is part of this issue)
  • when hitting a 403 (I know, unlikely but still), there's this big, fat and ugly "Access denied" shown, no css is added at all and there's a bunch of empty div's in the markup (again, not sure if this is part of this issue)
  • the hover state of the active tab makes the text white... on a white background
webchick’s picture

At sun and ksenzee's request, I went ahead and committed a modules/overlay directory to core with an empty overlay.info file. This should hopefully make patching easier from here on out.

Gábor Hojtsy’s picture

@seutje: on the blocks admin, I've alerted many people to this before; it is basically an issue with how our blocks admin works at the moment. We either accept this "misbehavior" of the overlay showing different themes, or institute a blocks UI which does not require theme switching. I'm not sure this theme switching blocks UI was user tested with a different admin theme, but I'd guess it is confusing. (It freaks me out to see the theme change for sure). While it makes sense to show the theme regions live in the theme, nowadays complex themes have all kinds of funky regions, which might not even appear on all pages, so it might not scale anymore either.

seutje’s picture

@Gábor: yea I kinda figured, the theme suddenly switching kinda freaks me out as well and it makes me wonder why I keep giving my regions proper descriptions. But it made me wonder about the whole "filtering certain regions" thing we're noticing when combining this and the dashboard

WorldFallz’s picture

Just want to comment that I see a lot of WTF in the forums regarding the theme changing on the block config page-- and frequently get asked if there's anyway to prevent it. I would think stopping that behavior, especially in light of the difficulty it seems to be causing this important d7 feature, is no great loss. Might be enough to provide the ability for a theme to display a graphic showing the block regions for those themes that want/need it.

xmacinfo’s picture

Drupal now comes with two enabled themes, which causes confusion. However this is normal behavior since each theme can have it's own set of regions. When setting up blocks for various enabled theme, each theme having their own complete set of block settings is very important.

Drupal just display blocks in their proper context.

Removing the context for block settings would be a return to the past.

catch’s picture

I'd agree with removing the theme switching - where you have regions which are never designed to be displayed on the same page, having them both pop up on this one can be a pain. In usability testing we had people trying to drag blocks into the regions, thinking they'd assigned blocks correctly because the regions were showing up and various other snags with displaying the regions there - I don't think it makes sense to display them until we can drag things into them, which is a separate issue with it's own complications, but our half-way version we have now doesn't really help.

seutje’s picture

@catch: which is a separate issue with it's own complications

is there already an issue opened for in-page block editing? cause I've been looking for it but can't seem to find it

Gábor Hojtsy’s picture

Whatever we do with blocks, it should be a separate issue IMHO. We are already trying to cover a lot of bases. Opened #581118: Blocks admin user interface should not do theme switching.

@sun: I've also opened #581120: Libraries can only be added in their entirety to discuss the library issue. I did not mark it "very critical" since I don't necessarily agree it is critical. Feel free to continue that discussion there.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
50.81 KB

Additionally to opening side issues, I've also worked on this issue more to fix some of the outstanding issues.

- @sun: on the block altering, I've now implemented block region removal in the overlay based on overlay_regions[] from the .info file; the two default (hard-wired) regions content and help are included with the overlay module, so no change to .info files required; these two special regions are treated special in other places of Drupal too; tested with Garland and it works fine there too; this also allows modules like dashboard to add more regions to the displayed ones, so the dashboard items are shown in the overlay
- @seutje: that white hovering issue was due to the recent patches having a malformed CSS; I've restored the active hover style that was at the end of the CSS but was not in the patches recently due to somehow broken files
- @webchick: I've rerolled the patch with the latest HEAD, so it is delta from the empty overlay.info
- @seutje: on your loading comment, Damien already opened a follow up issue at #574164: Fix transitions between pages inside the overlay
- I went back to as far as Damien's feedback in #168, because that looked like a critical issue, and tried to reproduce that without luck; I don't get a toolbar in the overlay; looks like your patched setup was somehow confused on which one is the parent and child in terms of the overlay setup; can you still reproduce that?

Damien Tournoud’s picture

@Gabor: the issue was caused by links with fragments, and has been fixed by Charlie in #183.

Bojhan’s picture

Can anyone explain to me why the last patch is so broken? It didn't seem so broken before.

1. You get a lot of whitespace, when you go from Configuration & modules, to say Content.

2. Are you sure you want to close this dialog? on /content? Do we even need an JS error? Its highly unlikely you accidentally click that tiny x.

3. The tabs are not connected to the overlay anymore?

4. Overlay scrolling seems broken, see http://screencast.com/t/pUmwioYJ

A note, I want to test this patch in next weeks usability tests.

Gábor Hojtsy’s picture

@Bojhan:

1. Cannot reproduce this issue.

2. You seem to have missed several iterations of the patch. That JS alert for example was introduced above several iterations ago in discussion and seems to be a highly simplified version of the dirty forms module.

3. From the looks of your screencast.com video they look as intended from the mockups, even if a tiny bit farther from the overlay in the disabled tabs' case, but definitely on the overlay in the active tab's case. The exact padding inbetween the overlay and tabs and between the tabs is for some reason different depending on the browser you use since we added those tabs, but we did not manage to find out why or fix this issue. It was certainly not of highest priority.

4. Cannot reproduce this issue. Could be very well due to the slowness of the computer. Seen similar scrolling errors in all kinds of web apps before when my CPU was highly busy.

Bojhan’s picture

FileSize
10.58 KB
9.35 KB

1. I did a clean install, I go to Configuration & Modules - then to Content. And I have a lot of whitespace at the bottom, below the overlay. I don't know what else I can do to help you reproduce it ( Vista, Firefox 3.5).

2. Sadly, I can't follow the design discussion that is happening in between all the code discussion. I completely disagree with this movement, we do not see this in our current administration - nor is it required by this interaction. Closing means closing, people know when you close, you do not save and you lose your data. But beside that debatable point, it should not be triggerd on Content (admin/content) administration.

3. Our current.

tabs.png

Mark's

marks.png

Unless I am not seeing this correctly, but that is not representing mark his mockup. I am using firefox 3.5, it shouldn't exist in that browser the least? I understand its not a priority, but given the orientation issue with tabs already - I think its wise to make sure they are connected to the body visually.

4. Seems somewhat weird if this is due to a slow PC, I am running Intel Core 2 Quad Q9100. I didn't have much running at that time. Tried it out, with one browser and tab open - and the same effect.

xmacinfo’s picture

@Bojhan "Closing means closing, people know when you close, you do not save and you lose your data. But beside that debatable point, it should not be triggerd on Content (admin/content) administration." --> This has been introduced for two reasons:

1. User can click on the dark part of the overlay to close the window.
2. User can press esc to dismiss the overlay.

And incidentally, the dark part of the overlay is more visible than before, the whole content narrower.

In proper UI design, we should not close a modal dialog, or here the overlay, by clicking beside it.

Also, we must make a difference about dismissing a dialog or closing a dialog.

Pressing esc should dismiss, which is in essence cancel. While closing the dialog or overlay should check if the form have been saved, even clicking on a tiny close button.

The only change I would love to see is to remove the clicking on the dark overlay to close the window.

That said, I have not tried the last version. :-)

Gábor Hojtsy’s picture

1. Ok I can reproduce that. That is space below the overlay, not in. I was looking at the overlay itself.

2. It is currently displayed on all pages which had forms, so you don't loose your input. We did not see this in our current administration, but closing a window there was about closing a browser window/tab, and not merely clicking at the side of the screen. I'm not saying I support that this alert is in there, I personally don't like it either.

3. As said, there are "pixel issues" here. Mark's mockups also distance the inactive tabs with one pixel on each side, and as said, the distance between the tabs and the overlay is different depending on what browser you use. This is clearly a bug.

4. Uhm, not a machine issue then. Interesting I did not see this before though.

Bojhan’s picture

@xmacinfo 1. Does not exist in current patch, 2. seems somewhat as an edge case.

2. Oke, I am trying to revisit why it was introduced - I think charlie did in #141 , but there seems to be no clear reasoning as to why. We can probably remove this feature, and bring it back in a new issue - if its truly that desired.

4. I will try to see, what is causing it.

xmacinfo’s picture

@Bojhan 2. Was introduced because there are now too many ways to close the overlay and that some users did click at the wrong place and thus lost their changes. So since this patch does not close when clicking on the dark overlay then I guess we can remove this warning.

markus_petrux’s picture

In regards to the warning that appears when closing the dialog... I believe I missed if this comes from D7UX, but I did this in Modal Frame API. However, it only fires if a form in the child window has been changed (requires Dirty Forms module).

I think it's somehow annoying to see such a warning if you haven't changed anything. The warning might be there if things have changed because that means the user had a initial intention to change, but closing would loose that changes, so it makes sense. But not if things have not changed.

~~~~~

This is another issue: it seems the Overlay module could be used elsewhere by contrib modules. But there are features that cannot be configured, rather they have been hard coded with all this admin interface enhancements in mind, which is incredible, but maybe contrib modules wish to enable draggable feature, dialog size, etc.

Gábor Hojtsy’s picture

Guys, the current patch includes code to close the overlay when the background is clicked, as that was requested by many people above and outside the issue (again). Looks like we are in a big limbo on this one. The full code to achieve this is:

+      // Close the overlay if the background is clicked.
+      $('.overlay-shadow, .ui-dialog, .ui-widget-overlay, .ui-dialog-titlebar').bind('click', function() {
+        try {
+          self.close();
+        }
+        catch (e) {}
+        return false;
+      });

On the dirty forms integration, I've said before that I believe that would be a different issue, and I'd personally support a more appropriate dirty forms integration, instead of the current "minimal approximation". That is all up to the core committers to decide though, depending on the usability need I guess. So I think we could remove the popup from this issue and keep debating elsewhere.

On the reuse of the overlay, I'm seeing the overlay as a specific implementation of jQuery UI dialogs, so I'd expect people to reuse the jQuery UI dialogs, not the overlay. Any contributions to the patch to make it more reusable are welcome however obviously :) But I'm not sure how would people reuse it and what parts would they want to reuse anyway.

markus_petrux’s picture

On the reuse of the overlay... for example, instead of:

  self.iframe.$container.dialog({
    modal: true,
    autoOpen: false,
    closeOnEscape: true,
    resizable: false,
    title: Drupal.t('Loading...'),
    dialogClass: 'overlay',
    open: function() {
      // code here...
    },
    beforeclose: function() {
      // code here...
    },
    close: function() {
      // code here...
    }
  });

It could be like this:

  var defaultOptions = {
    url: options.url,
    width: options.width,
    height: options.height,
    autoFit: (options.autoFit == undefined || options.autoFit),
    onOverlayClose: options.onOverlayClose,

    // Allow the caller pass jQuery UI Dialog options.
    customDialogOptions: options.customDialogOptions || {}
  }

  self.options = $.extend(defaultOptions, options);
  var dialogOpen = function() {
    // code here...
  };
  var dialogBeforeClose = function() {
    // code here...
  };
  var dialogClose = function() {
    // code here...
  };

  var dialogOptions = {
    modal: true,
    autoOpen: false,
    closeOnEscape: true,
    resizable: false,
    title: Drupal.t('Loading...'),
    dialogClass: 'overlay',
    open: dialogOpen,
    beforeclose: dialogBeforeClose,
    close: dialogClose
  };

  self.iframe.$container.dialog($.extend(dialogOptions, self.options.customDialogOptions));

And now callers could enable the draggable option in jQuery UO Dialog, for example.

Another thing that could be done to give more power to callers would be to add optional onFoo() callbacks here and there. For example, onChildLoad(), onChildUnload(), etc.

This kind of things will be hard to achieve if not planned ahead, I think. Imagination is endless, and the whole overlay thing is so new (in core itself) that I'm sure there will be a lot of great things from here.

Gábor Hojtsy’s picture

@markus_petrux: as I've said, any such improvements are welcome in a patch update :)

xmacinfo’s picture

Guys, the current patch includes code to close the overlay when the background is clicked, as that was requested by many people above and outside the issue (again). Looks like we are in a big limbo on this one.

OMG, this code is still there, then. This is why it was requested to have a dirty form function.

Lets remove this code as well as the warning and lets discuss those in a new issue later.

markus_petrux’s picture

Status: Needs review » Needs work

@Gábor: Ok, I'll check out HEAD, apply the latest patch in #225, and see if I can post a patch asap.

markus_petrux’s picture

Status: Needs work » Needs review
FileSize
57.08 KB

Ok, here's a patch. Changes are:

1) Removed the onclick event attached to the background (comment #235).
2) Allow external scripts override jQuery UI Dialog options (comment #236). This is new customDialogOptions option for overlay.
3) Added new overlay option onOverlayOpen. This is a callback invoked when the dialog is opened.
4) Added new overlay option onOverlayCanClose. This is a callback that can be used to allow external scripts decide if the dialog can be closed (Dirty Forms could take advantge of this).
5) Added ability to pass arguments to onOverlayClose callback through overlay_close_dialog() invoked server side. This is a feature of Modal Frame API that was not present here, and this allows external scripts do something when the dialog is closed based on server side response.

I believe I'm not missing any other change I just made.

Aside... one bug I think there is here is in overlay_form_submit(). When this submit callback is invoked, if the dialog close has been enabled, then it will terminate execution, but this short circuits other FAPI submit handlers. In Modal Frame API I was just setting $form_state['redirect'] to FALSE, so that other submit handlers can still be processed, Drupal.settings will be sent when FAPI redraws the form, and child.js will close the dialog. I was afraid to fix this, however, because I'm not sure why it has been changed from what Modal Frame API has.

Another issue related to this change, is that overlay_close_dialog() is tied to form submit handlers, and that means a child window cannot be closed from a simple GET request with no form submit.

Gábor Hojtsy’s picture

FileSize
50.25 KB

Thanks for the changes! Your patch included a default.install.orig file. Here is a quick reroll which now lacks that.

Gábor Hojtsy’s picture

FileSize
13.76 KB
52.09 KB

Another patch update now with actual bugfixes included. I worked in my fixes to markus' update, so this patch includes all latest changes. Fixed added for the two problems identified by Bojhan:

- Tinkered a lot more with the positioning of the tabs on the overlay; looks to be consistently working in Firefox 3.5 (Mac) and Safari 4 (Mac). Obviously tests in other browsers are welcome.
- Fixed the issue with resizing of the background dim. It did not take the window size into account and always kept the existing dim background size as a maximum (when the overlay shrank, that never "won"). Now takes into account the page body size and the window size when calculating, so it now shrinks the background.

This is how the tabs align on my machine in both browsers I've tested with:

I'd also like to note that Firefox 3.5 seems to exercise my CPU in an amazing way when the overlay is on. Even if I don't do anything just leave the overlay open. This does not happen in Safari. Since others did not note it above, I'd assume it might be some local issue (one of my Firefox extensions maybe interfering).

leisareichelt’s picture

Gabor, just confirming our conversations elsewhere re: clicking on background to close overlay window.

The reason we advised against this (ref: #45 and Bojhan #51) is because it is very easy to make an error and to close your window accidentally and it is very difficult to recover from this error.

I think we should avoid having click on background to close and instead we should address the usability of the close button - if it is not being found then we need to ensure that it is designed in such a way that it is more noticeable/accessible as you scroll down.

Also, I don't think it is a safe assumption that people know when they close a window that their data is unsaved - in these days of autosave, I don't think that is entirely true at all, and certainly in the scenario where people have been creating article content etc. we really want to make sure that people are deliberately NOT saving, and assuming that they will want to (in most cases) preserve their content.

catch’s picture

The problem with the prompt on closing is pages like admin/content which are technically a form (you can delete / publish stuff from there), but you're never actually entering any data, and a lot of the time will be using it for search/filter and going to articles directly rather than using the bulk update options at all. On those pages, we definitely don't want a message when you close - a lot of people won't even realise it's a 'form' and could get very confusing.

Gábor Hojtsy’s picture

FileSize
51.85 KB

Ok, warning and skipWarning code removed. We can revisit this in a follow up patch and hopefully add a more proper dirty forms type of check where we can exclude certain forms and only warn if there was something entered. That would basically be the integration of the above mentioned dirty forms module. Let's open that can of worms in a follow up issue then.

Attached patch removes the warning.

markus_petrux’s picture

Let quote myself from comment #240:

Aside... one bug I think there is here is in overlay_form_submit(). When this submit callback is invoked, if the dialog close has been enabled, then it will terminate execution, but this short circuits other FAPI submit handlers. In Modal Frame API I was just setting $form_state['redirect'] to FALSE, so that other submit handlers can still be processed, Drupal.settings will be sent when FAPI redraws the form, and child.js will close the dialog. I was afraid to fix this, however, because I'm not sure why it has been changed from what Modal Frame API has.

Another issue related to this change, is that overlay_close_dialog() is tied to form submit handlers, and that means a child window cannot be closed from a simple GET request with no form submit.

Maybe this could be addressed with this version of overlay_form_submit():

function overlay_form_submit($form, &$form_state) {
  $settings = &drupal_static(__FUNCTION__);

  // Close the overlay only if specifically requested.
  if (($args = overlay_close_dialog()) !== FALSE) {
    if (!isset($settings)) {
      $settings = array(
        'overlayChild' => array(
          'closeOverlay' => TRUE,
          'statusMessages' => theme('status_messages'),
          'args' => $args,
        ),
      );
      // Tell the child window to perform the redirection when requested to.
      if (!empty($form_state['redirect'])) {
        $settings['overlayChild']['redirect'] = url($form_state['redirect']);
      }
      drupal_add_js($settings, array('type' => 'setting'));
    }
    // Tell FAPI to redraw the form without redirection after all
    // submit callbacks have been processed.
    $form_state['redirect'] = FALSE;
  }
}

And then we don't need those implementation of hook_node_insert() and hook_node_update().

[EDIT] since we have the chance to touch core here, another possible way to resolve this issue would be to add a new #after_submit callback to FAPI so that it is invoked after all submit handlers have been invoked.

Gábor Hojtsy’s picture

Markus: I don't know of a specific reason for not letting the rest of the submit handlers act, so it sounds like a bug indeed. Your suggestion code seems to be a better approach. Again, feel free to intervene ;)

markus_petrux’s picture

FileSize
49.98 KB

Ok, so here's patch in #245 updated with a slight variation of the code in comment #246. Implementation of hook_node_insert() and hook_node_update() are now gone, and this is resolved inside overlay_form_submit( ) itself.

Also fixed wrong condition related to onOverlayCanClose:

-    if (self.options.onOverlayCanClose(self)) {
+    if (!self.options.onOverlayCanClose(self)) {
markus_petrux’s picture

FileSize
49.97 KB

Oops! I attached a wrong version of my changes. Sorry.

Gábor Hojtsy’s picture

Superb, let's get those test results flowing again!

sun’s picture

sun’s picture

FileSize
52.77 KB

I can't see why #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable doesn't make it work. The idea is to use the APIs of the stuff you want to use. Just like in Drupal.

Revamped.

However, this is still very far from being ready.

Gábor Hojtsy’s picture

The patch suggested there still adds unused/useless CSS files which we might need to override (I did not actually test it yet).

sun’s picture

Like the friendly jQuery UI core developers explained over there, jQuery UI is supposed to be used that way. Removing the ui.theme.css removes the theme. The remaining styles are for base functionality only. This patch tries to re-invent the wheel by using own base styles.

That needs to be redone, but that's also minor compared to the real issues.

For starters:

1) Overlay is Overlay. No more and no less.

2) Everything with "toolbar" in it has to move into toolbar module. toolbar.js, to be more concrete.

3) Toolbar module triggers (resp. implements support for) Overlay. Only within the toolbar, to be precise.

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

Damien Tournoud’s picture

I don't get what this all ui.theme.css debate is all about. jQuery adds a "overlay" class to the dialog because we are using dialogClass: 'overlay'. It's simple enough to override the specific directives we need to make the overlay a "special dialog".

RobLoach’s picture

FileSize
49.89 KB
177.98 KB

A little better, and you can see how drupal_add_library can help.....

+++ modules/locale/locale.test	25 Sep 2009 06:26:51 -0000
+++ modules/locale/locale.test	25 Sep 2009 06:26:51 -0000
@@ -236,7 +236,7 @@ class LocaleTranslationFunctionalTest ex
@@ -236,7 +236,7 @@ class LocaleTranslationFunctionalTest ex
     $this->clickLink(t('edit'));
     // We save the lid from the path.
     $matches = array();
-    preg_match('!admin/config/regional/translate/edit/(\d)+!', $this->getUrl(), $matches);
+    preg_match('!admin/config/regional/translate/edit/(\d+)!', $this->getUrl(), $matches);
     $lid = $matches[1];
     // No t() here, it's surely not translated yet.
     $this->assertText($name, t('name found on edit screen.'));
Index: modules/node/node.admin.inc

This touch isn't needed, is it?

+++ modules/node/node.admin.inc	25 Sep 2009 06:26:51 -0000
+++ modules/node/node.admin.inc	25 Sep 2009 06:26:51 -0000
@@ -447,6 +447,9 @@ function node_admin_nodes() {
@@ -447,6 +447,9 @@ function node_admin_nodes() {
   foreach ($result as $node) {
     $nodes[$node->nid] = '';
     $options = empty($node->language) ? array() : array('language' => $languages[$node->language]);
+    // Set a class to flag to the overlay, if present, not to open the link in
+    // the overlay.
+    $options['attributes']['class'] = 'overlay-escape';
     $form['title'][$node->nid] = array('#markup' => l($node->title, 'node/' . $node->nid, $options) . ' ' . theme('mark', node_mark($node->nid, $node->changed)));
     $form['name'][$node->nid] =  array('#markup' => check_plain(node_type_get_name($node)));
     $form['username'][$node->nid] = array('#markup' => theme('username', $node));

Do we have to add a class to the node module here? Not every Drupal site out there will be using the Overlay module, nor need this overlay-escape class.

+++ modules/overlay/overlay-child.js	25 Sep 2009 07:07:11 -0000
@@ -0,0 +1,149 @@
+
+    // Make sure this behavior is not processed more than once.
+    if (self.processed) {
+      return;
+    }
+    self.processed = true;
+
+    // If we cannot reach the parent window, then we have nothing else to do
+    // here.
+    if (!self.isObject(parent.Drupal) || !self.isObject(parent.Drupal.overlay)) {
+      return;
+    }

we could probably once() this.... $('body').once('overlay', function() {
// Other code here.
});

+++ modules/system/system.module	25 Sep 2009 06:47:04 -0000
+++ modules/system/system.module	25 Sep 2009 06:47:04 -0000
@@ -973,7 +973,6 @@ function system_library() {
@@ -973,7 +973,6 @@ function system_library() {
     ),
     'css' => array(
       'misc/ui/ui.core.css' => array(),
-      'misc/ui/ui.theme.css' => array(),
     ),
   );

With this, if you want to display any jQuery UI element, you have to manually add ui.theme.css, which will in turn break the Overlay anyway. We should be working with jQuery UI's CSS as opposed to against it.

+++ themes/garland/style.css	25 Sep 2009 06:26:51 -0000
@@ -508,6 +508,15 @@ body.two-sidebars #footer {
 
+/* Don't display any header elements when within the overlay, and adjust the page height accordingly. */
+body.overlay #header * {
+  display: none;
+}
+
+body.overlay {
+  margin-top: -80px;
+}
+

Should this be in one of the overlay's CSS files?

............................... Here's another patch that uses ui.theme.css, but builds ontop of it. It's not perfect, but it doesn't require other jQuery UI elements to register their own theme each time.

RobLoach’s picture

FileSize
167.86 KB

Uhh, wrong screenshot. Sorry... Here it is!

Status: Needs review » Needs work

The last submitted patch failed testing.

markus_petrux’s picture

Status: Needs work » Needs review

In regards to the following:

/**
 * Add the isObject() method to the overlayChild object for convenience.
 */
Drupal.overlayChild.isObject = parent.Drupal.overlay.isObject;

This is executed when we do not know yet if we have a parent window with a Drupal.overlay object. Would it be possible to add the isObject() method to drupal.js? (it would have to be in jQuery itself, but it isn't).

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@Rob Loach: Charlie added that locale fix. For some reason the locale module is broken with the overlay patch applied (I'll send over virtual hugs to people who find that out why). So your patch fails on that locale test now. I've tried to remove that above seeing it was superfluous, but see above.

@Damien Tournoud: the issue is that adding whole libraries adds CSS which will never get used and needs to be overwritten. While we do have the specific classes we need, jQuery UI's generic classes are also there and matched from their code.

@Damien Tournoud, @sun, @Rob Loach: So we include twice as much CSS and make debugging that harder in the name of reusing our APIs, right? Well, if that is the *correct* way, I'm not gonna go against you helping us fix this up! However, in that lite and given Rob's comment above, #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable seems to be a no-go, since removing that will cause problems with others adding jQuery UI components to the page for other reasons.

Damien Tournoud’s picture

@Gábor Hojtsy: you could argue that we need to design a Drupal-specific ui.theme.css, but simply not including the default jQuery UI css just for the sake of saving a few bytes makes very little sense to me.

[Edit] example of what I mean: what if the base ui.theme.css is already included, because another UI element is used in the same page? Your partial CSS file will brake horribly.

markus_petrux’s picture

In response to sun at #254: I agree that overlay could be a module that implements an API, then other modules can make use of it. All the stuff related to integration of overlay in Drupal administration could be a separate module that depends on overlay.

This is also a method to ensure overlay api is well designed to cover different use-cases, at least those that are implemented for core would work.

It is now difficult to see if overlay api can be reused elsewhere.

cwgordon7’s picture

@Gabor/Rob Loach: As I understand it, the locale test fails without that change because the overlay module adds a few additional Drupal.t() calls which adds strings to the "to be translated" string list which stores the strings in the database by lid. The additional Drupal.t() calls bumps the locale test's test string's lid into the double digits, which exposes a preexisting bug in the test's regular expressions, which only captures the first digit of the lid from the URL. Thus, with this patch, the test needs to be corrected in order to prevent the failure from occurring.

sun’s picture

Status: Needs work » Needs review
FileSize
53 KB

I'm not sure on what Rob Loach's last patch was based, but it had some strange changes in it.

This one is based in #252, cleaning up a lot of stuff.

Next step is to move away Toolbar-stuff into Toolbar module.

RobLoach’s picture

Looking through my patch, it seems that I forgot to fully clean the development environment, whoops.

With the new patch, you need to add back ui.theme.css and then stick this into overlay-parent.css:

.overlay.ui-widget-content {
  background: none;
  border: none;
}

.overlay .ui-widget-header {
  background: none;
  border: none;
}

We need to work with ui.theme.css as if we add any other jQuery UI element along with its CSS, it would break the overlay. We have to build ontop of it.

bensnyder’s picture

finally subscribing after 1 month of following.

I'm glad sun laid out some direction in #254... I was a little lost as to how far along this issue was.

Keep up the good work fellas! Looks incredible!

Gábor Hojtsy’s picture

FileSize
57.55 KB

@sun: thanks for working on the patch. Here is my testing feedback:

- it does not work in Safari; the symptom is that the loading screen keeps being displayed; this did not happen with any previous patch I've tested
- it has minor style glitches in Firefox 3.5 (where it does work), maybe not target of immediate fixing while your clean up APIs, but noting just in case:

For the spacing designed for breadcrumbs / top of overlay, see the image above in #107: http://drupal.org/node/517688#comment-1972556.

Otherwise seems to work and look as before on Firefox.

RobLoach’s picture

FileSize
128.61 KB
52.24 KB

This patch builds off of ui.theme.css so that the Overlay doesn't break when other jQuery UI elements are active.

Gábor Hojtsy’s picture

I'd second that #574156: Overlay: submitting a node should close the overlay and friends will need a rapid resolution once this patch is perfected and committed. Watching test videos from http://vimeo.com/groups/drupalux/videos prove that the node showing up in the overlay can be very confusing. People go back and forth between viewing and editing the node in the overlay and on the website UI. We've had a solution for that above for quite some time but it was shot down in the name of API cleanliness without a better solution in sight, and moved to a side issue to fix later. People doing user testing with this patch will obviously test the somewhat broken user experience in very core things like creating a node. That does not make it easy to project the results to a place where we have these fixed workflows in place. Tossing the fixes for those even later will get us even less user testing of the actual user experience we aspire to achieve.

Also, multiple user tests proved perfectly that the overlay closure warning is a huge annoyance even after a few minute's use, so let's not argue about putting that back in :)

Dries’s picture

I tested the patch and, based on limited testing, it seems to work well. Haven't reviewed the code yet.

Gábor Hojtsy’s picture

The blocks page theme switching WTF which was said to be a major problem for the overlay now has a patch at http://drupal.org/node/581118#comment-2106588

Anonymous’s picture

Subscribe

catch’s picture

RCS file: /cvs/drupal/drupal/modules/node/node.admin.inc,v
retrieving revision 1.67
diff -u -r1.67 node.admin.inc
--- modules/node/node.admin.inc	18 Sep 2009 00:12:47 -0000	1.67
+++ modules/node/node.admin.inc	29 Sep 2009 17:46:56 -0000
@@ -447,6 +447,9 @@
   foreach ($result as $node) {
     $nodes[$node->nid] = '';
     $options = empty($node->language) ? array() : array('language' => $languages[$node->language]);
+    // Set a class to flag to the overlay, if present, not to open the link in
+    // the overlay.
+    $options['attributes']['class'] = 'overlay-escape';
     $form['title'][$node->nid] = array('#markup' => l($node->title, 'node/' . $node->nid, $options) . ' ' . theme('mark', node_mark($node->nid, $node->changed)));
     $form['name'][$node->nid] =  array('#markup' => check_plain(node_type_get_name($node)));
     $form['username'][$node->nid] = array('#markup' => theme('username', $node));

Please put this in an overlay_form_alter() instead of hard-coding markup into node.module which will be output regardless of whether overlay.module is enabled or not - this may or may not be helped by #595982: Use #tableselect instead of ugly theming on admin users and admin nodes.

This was mentioned in sun's review in #47 and Rob Loach's review n #256 but doesn't seem to have been changed by the patch.

Anonymous’s picture

Since the output of l() is being put into the form elements, it seems like it would be a much cleaner task to simply surround the code referenced in #273 with if (module_exists('overlay')) { }... Thus that's what I did. If that is not acceptable, I'll try to go about it another way.

ksenzee’s picture

Introducing overlay code into node.module is pretty tight coupling. If it's too unwieldy to handle it with a form_alter as catch suggested, could we just add the overlay-escape class with Javascript?

Anonymous’s picture

I don't really think that would be a good approach.

Since the rendered link is being added to the form via l() I only see two approaches that form_alter could take:
1) Copy the entire section of code that generates the links, making the one small change. This goes to create code duplication.
2) Use a regex to add 'overlay-escape' to the links. That sounds like it would be serious overkill. Regexs aren't exactly notorious for a good developer experience nor for speed.

As far as the javascript aspect, I'm not sure if it would work. If it did, however, adding a class to only one set of links on one certain page... well, that seems to have the same downfalls as the regex. I can't see a reason to use lots of code and sacrifice speed to remove one single line of code. Not to mention there is always the possibility that the user would click on the link before the page had finished loading. (So the javascript wouldn't have run yet.)

Honestly, I think that #274 and #268 both work acceptably well. I just don't see any other way to remove that one bit of coupling without greatly increasing code complexity.

chx’s picture

Status: Needs review » Needs work

The patch is rolled without -p so I do not even know which function is being changed with that coupling.

chx’s picture

OK, so right now we are at the point where we want to add a class regardless of overlay is installed or not, maybe there will be an awesome_overlay module who knwows. Then what the class indicates? admin exit. Now, let's find a class name... give us a few minutes.

David_Rothstein’s picture

All these proposed solutions have a major problem: They are one-off solutions that only apply to this particular form. Whereas on a typical Drupal site, there might very well be other content listings displayed in the overlay as well. So we'd be introducing a weird inconsistency to only close the overlay for this one listing (or alternatively, a weird requirement to ask every contrib module to think about duplicating the solution)....

More important question, though: Why do we want to do this at all? I tried this patch, clicked on "Content" in the toolbar, and it opens the overlay and shows me the content listing. OK, so far, so good. But since it's a content listing, my first inclination is to browse through some of the things there. So I click on the first piece of content I see. What happens? Suddenly, I am thrown out of the overlay without warning and dumped onto a different page altogether. Very disorienting. OK, now I'm done looking at this piece of content and want to go back to my list and browse through some of the other items. So I hit my browser's back button, and it doesn't even take me back there. I'm a bit stuck.

I suggest we simply don't do that at all, and then the above implementation problems are neatly avoided :) Closing the overlay without the user hitting the close button is something that should happen only very very rarely, when there's a clear, deliberate reason to do so.

seutje’s picture

like viewing a node after it was created?

and would that also apply to editing a node?

imo, it makes a lot more sense to be thrown out of the overlay and onto the page the node lives at, when submitting the form. And I think that can be done trough the submit handler, no?

casey’s picture

@#279
Another option: show a confirmation message in the overlay that the node is created and give the user a list of actions; view node (close overlay), create new node (stay in overlay), ...

marcvangend’s picture

@#281: IMHO confirmation screens that don't do anything else (except redirecting) are not really user friendly. In that case I'd rather present those options on the bottom of the node form. For instance, you could have multiple buttons, like this:
|Save| |Save & close| |Save & add new|
Another option would be to have just one 'save' button and let the user choose the after-save-behavior with radio buttons:

|Save|     After save:
           O Close overlay
           O Edit
           O Create new
markus_petrux’s picture

It seems to me all these matters would be easier to approach if there was a module that provides the overlay as an API, and then... a) another module that uses the Overlay API to extend/enhance core modules, or b) these features are implemented in core modules themselves depending on whether the Overlay API module is enabled. I would say a) would be better because there would be no need to mess with overlay *inside* the other core module, and it would also easily show how can the Overlay API can be used in contrib/custom module directly, or to extend other modules.

Anonymous’s picture

@#279: That actually sounds like a much better solution. I'll see if I can roll out a patch later today that fixes that.

@#282: I agree with your take on confirmation screens. It's probably safe to leave closing the overlay to the close button and assume that user wants to stay there unless they choose to close it.

@#283: If we want to actually get this in to D7 (and this thread not be a waste of time) then I think we'll have to skip on going with an API. I think that would require too much rewriting.

Gábor Hojtsy’s picture

@JoshuaRogers: I'm not agreeing with you that we should only close the overlay on hitting the close button. When a user clicks on a node link or submits a node, we should not get to the actual rendering of the node *in the overlay*. The overlay is supposed to be used for administrative interactions, not viewing content. The admin theme will most probably not have provisions (theme functions, images, styles) to display the node as intended in the admin overlay. (The same causes previews being a failure in the overlay as noted in the issue starter).

David_Rothstein’s picture

Let's separate the discussion of creating content vs viewing content. My comment in #279 was only about viewing content -- and the specific case of the node listing at admin/content that is causing so much trouble.

I agree that creating content is probably a special case where automatically closing the overlay does make sense. However, the code required for that would be completely different anyway.

The overlay is supposed to be used for administrative interactions, not viewing content.

I think that is going to be impossible to enforce without rewriting all of Drupal. There is no such clear separation, and there are cross-links all over the place.

The admin theme will most probably not have provisions (theme functions, images, styles) to display the node as intended in the admin overlay.

Right, I was thinking about that as well. Can we show the main site theme in the overlay if a node is being viewed there (but only the main content part of the page - not the other blocks)? This does get very tricky.

Anonymous’s picture

@#277: The attached patch has been rerolled to with -p

sun’s picture

This patch won't fly if Toolbar-stuff isn't separated away into Toolbar module and Overlay becomes a plain Overlay module, without any technical limitation for "administrative stuff".

Achieving this won't be hard at all. I'll try to get my hands dirty with that ASAP. Dries already asked, but I'm still overly busy with mission critical API clean-ups.

Gábor Hojtsy’s picture

The overlay is supposed to be used for administrative interactions, not viewing content.

I think that is going to be impossible to enforce without rewriting all of Drupal. There is no such clear separation, and there are cross-links all over the place.

Right. Modules need to be aware of a possible overlay and need to tell the overlay how to work with the content, if special handling is required. Just like modules are aware of a possible edit mode, and provide data which might not be displayed at all (ref: the d7ux attached links patch). The fact that overlay is optional might make it look like we are adding cruft to modules which will not get used when the overlay is not used, but this same "collaboration pattern" is used in contrib all the time. Optional support for certain modules is all over the place.

The admin theme will most probably not have provisions (theme functions, images, styles) to display the node as intended in the admin overlay.

Right, I was thinking about that as well. Can we show the main site theme in the overlay if a node is being viewed there (but only the main content part of the page - not the other blocks)? This does get very tricky.

The defining principle of the overlay is that it separates administration from the main site. This will not work for all sites, and not all sites need to use the overlay obviously. But if we disregard this defining principle and start to display whatever we happen to have in the overlay, it is gonna hurt more then help. We don't need to make this work for every imaginable use case in the world. You know Drupal has this **modularity** which comes in very handy, when you need something which is not implemented in core the way you want it to be. Be it a drop-down admin menu you'd want to see, a different logging module, or a replacement for the (admin) theme or a different admin UI wrapper.

I'm getting that @sun et. al. take API-ification of the overlay as an important step, and we'll see what modules and sites will make use of that and do non-administrative stuff with the overlay. However, if that looks the same as the admin interactions, as explained above, it could confuse more then help the user. Let's at least try to get the core implementation of the overlay reflect our goals with it. If overlay treatment in node module or user.module is a no-go (as in "code for an optional core module should not be in there") in terms of responsibilities of modules, then the overlay needs to alter in these places, and alter node listings, user listing, etc. If that is not yet possible, it is a good exercise anyway to make those places alterable. That would be for the benefit of any contrib module as well.

David_Rothstein’s picture

The defining principle of the overlay is that it separates administration from the main site.

I wouldn't disagree with that so much as saying that "administration" is hard to define. Viewing content can definitely be an administrative activity - what I tried to do in #279 (basically, browse through content from an administrative screen) is I think pretty common.

I would suggest that the defining feature of the overlay is really that it focuses you on a task that you went there to do, and when you are done that task, you go back to where you were before. So if I get thrown out of the overlay at random times and the parent page gets redirected somewhere else, that seems to really interrupt that flow.

I think if other people tried out something similar to what I did in #279, that would be helpful. I am only one person :)

Noyz’s picture

Coming in late to this, but my opinion is to close the overlay. I agree with Gabor in #285 - the reason for the overlay was to try to enforce the separation of administering a site. Sounds like this might not be impossible in all cases, but we should adhere to the pattern wherever possible. Additionally, I think we should assume that if content is clicked, it's clicked for a reason - albeit to view it, or edit it. If the content stays in the overlay - viewing it would be tainted by Seven (constrained by width), and editing it would be impossible.

Back button not working stinks, but getting back to your list of content is only one click away "find content"

Linea’s picture

subscribe

Anonymous’s picture

In that case, how does #287 look?

David_Rothstein’s picture

If the content stays in the overlay... editing it would be impossible.

It would? Why?

Back button not working stinks, but getting back to your list of content is only one click away "find content"

That page is not the only place where content is listed. Think of something like a View, which might be deep within the admin structure and therefore not possible to get back to via a single click.

Also, somewhat minor point: I believe it's the case (but can't confirm since this part of core seems to be broken right now) that if you had sorted your content on the "find content" page, drop out of the overlay, and then go back by clicking on that link - it wouldn't remember your sorting and you'd have to redo it... but that might just be a bug to fix.

Noyz’s picture

It would? Why?

Editing would be impossible in that editing content is supposed to be done in place. That model kind of breaks down if you're in the overlay.

That page is not the only place where content is listed. Think of something like a View, which might be deep within the admin structure and therefore not possible to get back to via a single click.

Sorry, I thought we're talking about looking at your content via /admin/content/node, and then clicking on a piece of content to view it? Looking at your listing of Views, clicking on the View, then not being able to get back to the view would take longer yes.

I still think closing the overlay is the correct thing to do. Comparing evils, it's much worse to pull the content into the overlay, possibly have a theme within a theme within a theme, and jeopardize the very thing the overlay is meant to do - separate admin from content.

ksenzee’s picture

I agree with Noyz -- it's horribly messy to start displaying content within the overlay. I also agree with David that after you view a piece of content, you need to be able to get back to your last overlay state. If we can find a solution that satisfies both concerns, perfect. If not, I think losing the overlay state is the lesser of two evils.

I can think of two solutions that preserve overlay state, while still letting us close the overlay when viewing content:

1) Keep track of the last page viewed in the overlay, and provide some kind of button or link that says "Reopen overlay." This could even look like a rolled-up or minimized version of the overlay.
2) Make it so that after you've viewed the content, the back button reopens the overlay at the last overlay state.

I ran these by Noyz offline, since he's an actual interaction designer, unlike me. He didn't care for #1. So I investigated solution #2, and I believe it's doable. We could do what Gmail does and change all the overlay-enabled links to fragments. So if you're on the homepage, and you have a link to /admin/content that's meant to open in the overlay, we'd stick a # in there, and your link would be to "http://example.com/#admin/content". Clicking on that link would not trigger a page reload, but it would create an entry in the browser history. We could then set it up so that any link with a fragment that matched an actual menu path would open the overlay for that path.

The downside here is that the URLs might be a bit ugly. An overlay of the Find content page, opened from a node view page, would look like "http://example.com/node/4#admin/content". Thoughts?

Meanwhile, I've rerolled the patch without .orig and .rej files, and made a couple of minor typo-level fixes.

JacobSingh’s picture

Okay, did a re-roll due to the massive API change to all theme functions and the move to tableselect.

Anonymous’s picture

FileSize
41.17 KB

I've been trying to bust this bug, but no success so far: If I attempt to open the overlay directly after installing D7, I get a "page not found" every time. Clicking on the dashboard button again brings up the actual dashboard. I can only get it to happen if I click the dashboard directly after installing. Granted, it's not a severe bug. Just kind of annoying though.

David_Rothstein’s picture

@JoshuaRogers: I think that might be a separate bug, likely related to this: http://drupal.org/node/557542#comment-2120098

Anonymous’s picture

@#296: It makes sense to me. I don't think the links have to be pretty if they are meant to open in the overlay. It seems like a reasonable price to pay.

I'm not sure if that is reassuring or not. (I'll pretend that it is for now.) I'll try to get some work done on implementing #296 part 2 this weekend.

Anonymous’s picture

@#296: Thinking about it a little bit more, I have some (probably obvious) questions. Still, I'd like to figure it out before trying to code.

1) Suppose it open "admin/content" in the overlay and that I go to "admin/comments" from there. Next, assume that I close the overlay. Would the browser history list http://example.com/node/4#admin/content as the most recent place in the history?

2) Assume that we're on a multipage form or that we are looking at a list with a particular filter and order: (Unpublished pages listed by author, for instance.) If we close the overlay and then choose back to reopen it, what would happen? (For that matter, any page that has POST content.)

3) On the node add form, if the user closed and then went back, would they be presented with a blank node add form?

Not trying to be stubborn. Just trying to find any issues before coding (in hopes to find a way around them in advance.)

ksenzee’s picture

1) Suppose it open "admin/content" in the overlay and that I go to "admin/comments" from there. Next, assume that I close the overlay. Would the browser history list http://example.com/node/4#admin/content as the most recent place in the history?

Ideally the fragment -- and thus the URL -- would change with every click, so no.

2) Assume that we're on a multipage form or that we are looking at a list with a particular filter and order: (Unpublished pages listed by author, for instance.) If we close the overlay and then choose back to reopen it, what would happen? (For that matter, any page that has POST content.)

I think POST content is gone when you reopen the overlay. Ideally content listings would set a cookie. I think.

3) On the node add form, if the user closed and then went back, would they be presented with a blank node add form?

Yes, but they'd have gotten a dirty form warning before they closed, so caveat form-filler-outer.

If anyone objects to sticking fragments in the URL to fix the back button, please speak now before we spend too much time working on this....

Anonymous’s picture

It sounds good to me.

David_Rothstein’s picture

Fragments in the URL make sense to me also.

If I'm not mistaken, with an extra tweak or two this would also make it possible to link to overlay pages or bookmark them in your browser? If so, that's a huge win.

sun’s picture

In case you didn't notice: We just crossed the #300 mark.

To get this done:

1) Remove all lines relating to Toolbar from modules/overlay/* and insert them appropriately into modules/toolbar/*

2) Forget about fragments, and whatever dream you have about awesome UX experience. Such features should have been discussed in the first 100 follow-ups. If at all, follow-up issue, or, welcome to Drupal 8.

3) Stop discussing. Let's do it now.

Anonymous’s picture

Plus, it should allow for any links in the overlay that are opened into new tabs to also appear in overlays (not just as the content that should appear in it.

markus_petrux’s picture

@JoshuaRogers in #284: "@#283: If we want to actually get this in to D7 (and this thread not be a waste of time) then I think we'll have to skip on going with an API. I think that would require too much rewriting."

Well, if Overlay in D7 is not implemented as an API, it will be hard to reuse, and so I'll have to create a version of Modal Frame API for D7. If reusability it is not provided by Overlay in core, then Overlay in core is just void to a few modules that exist in contrib as well as in custom code.

marcvangend’s picture

Let's not forget the reason why we started on the overlay module. The main purpose of Overlay is not to make other modules (Popups API, Modal Frame API, Lightbox2, CTools' modal dialog or whatever else) redundant. Even though it would be really nice if we get a flexible API out of this, our focus should be on UX. Quoting Leisa Reichelt on http://www.d7ux.org/d7ux-initial-concepts-direction/:

We see the overlay as a fantastic way to provide a clean interface for these tasks whilst keeping the user in the context of the site for which they are performing those tasks, rather than taking them away into an ‘admin section’.

Anonymous’s picture

@sun: "Forget about fragments, and whatever dream you have about awesome UX experience." I'd say that having a good UX is worth a little discussion. Yes, we're over #300 posts, but how many of those were "+1", "Subscribe" or TestBot crying itself to sleep? No reason to get testy. (Well, other than the pressure that comes with the impending doom of freeze.) ;)

Having said that, I agree that we need to get this completed. I'll be working on your #1 and seeing if I can't get something done with fragments this weekend. We are low on time, but there might still be enough to get good a UX and good design. I'll see what I can't get done this weekend.

Bojhan’s picture

FileSize
2.68 KB
4.3 KB

So this is my fifth review or so, and I am surprised to still see the same bugs from review 1. I think most critical issues should be solvable, yet with only 4 days to get this sucker in I think almost none of them are truly crazy UX issues - that can't be fixed with follow ups. I think what I am mostly concerned with is that all the tiny details we still haven't fixed, add up to a "still work in progress" feeling.

Smaller issues

  • Alignment of listed items
  • Alignmentstructure.png

  • Spacing of tabs (See Mark's screenshot)
  • tabsspacing.png

  • You can't scroll, by using your drag mouse button in the overlay (Which is normal behavior of scroll)
  • When you open the shortcut bar, it falls on top of overlay
Anonymous’s picture

I plan on working on this solid up until the 15th. I'll see if I can't try to take out as much of #310 as possible while I'm at it.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
50.58 KB

This patch should take care of #305-1 as much as reasonably possible.

Next I will work on the issues #310 and see if the fragment thing could end up working.

Gábor Hojtsy’s picture

On fragments: simply appending the path as a fragment sounds limiting as to how other modules can use the fragment. That would lock down using fragments for overlay only. Also, fragments like #admin would not fly well, when you can actually have an id='admin' element on the page. This can happen to any single level path at least. If we consider we only display admin paths in the overlay ever (which I'm a big promoter of), then we might only have this issue strictly with the admin word. Also, Drupal paths (admin filters, etc) can usually also take arbitrary GET parameters. How would those represented in the fragment? I'd agree that this sounds a bit late to implement. The in-overlay back button experience works due to the iframe we deliberately choose (for this among other reasons).

@Bojhan: your rendering error on scrolling screenshot seems to point to the horizontal scrollbar on appearance; I was unable to reproduce the horizontal scrollbar or the scrolling bug, you noted earlier and could not find others to reproduce either

@Bojhan: closing overlay on node creation was in the patch for long, but was removed in the name of API purification; I do not agree with this and David and Noyz as well as ksenzee was discussing that lately; same treatment should happen to users when created

@Bojhan: blocks page issue already at #581118: Blocks admin user interface should not do theme switching

@Bojhan: alignment of items as well as spacing in the overlay was fixed multiple times but keeps getting broken due to CSS reorganization happening by multiple parties :|

Dries’s picture

So what is left to do on this issue? After 300 issues, it is hard not to loose track ... a quick summary would be really helpful.

As far as I can see, the one thing that is still up for discussion is the "behavior problem" with regard to breaking out of the overlays (eg. when user clicks on a content link from an overlay, should they be taken out of the overlay or not). Is that correct, or is that considered to be a follow-up patch?

Gábor Hojtsy’s picture

@Dries:

- @sun discussed in #305, that toolbar related code should go to toolbar, not overlay, and he had no outstanding issues mentioned here; Joshua attempts to accomplish this in his latest patch
- @sun previously mentioned that all "administration" related ties should be removed from the overlay (see next point too, which is ver similar);
- @markus_petrux notes that we should make the overlay an API and a UI, decoupling the two, but many of us noted (including @marcvangend nicely explaining above), that the overlay is not supposed to replace other dialog tools (and we added jQuery UI, which has a nice dialog component for people to reuse, which we actually reuse for our own purposes), so its not like all popup/dialog modules will need to be written from scratch for D7, but that the overlay is a special UI; unlike other dialogs, it grows and scrolls with the page, it fits into most of the screen estate and is designed for multi-page administrative interactions
- @David_Rothstein, myself, @ksenzee, @Noyz and @Bojhan were arguing about closing the overlay when a non-admin endpoint is requested; David suggests not closing, everybody else suggested closing (which would be in line with the plan for the overlay)
- related to this is how node previews and other actions are handled, which was stated as a problem to solve in the initial issue starter, but due to the API and (to lesser extent) basic level interaction questions, we did not even reach to talk about such problems yet
- the back button and "open is new tabs" interaction was also discussed and using fragments was briefly suggested to be able to move about with the overlay opening on certain links even if bookmarked or opened in a new tabs; which would allow the seamless integration of the overlay into the browser UX, which was deemed too late to some extent above
- there are also other relatively smaller issues mentioned above by @Bojhan (and he also covers some of the above)

Looks like we need a definite direction in terms of breaking down the overlay to API and implementation (so we'd have another level of API on top of jQuery UI dialogs, instead of going directly to the overlay) as well as the basic interaction of "outer links" (non-admin page links) in the overlay.

Hopefully this summarized the issues well, let's add more items, if not complete.

David_Rothstein’s picture

David suggests not closing, everybody else suggested closing (which would be in line with the plan for the overlay)

I'm fine with closing if that's what everyone else wants - however, it means the "back button is broken" issue is then critical to solve. The overlay experience is broken if you get taken out of it without warning and have no reasonable way to get back to where you were.

For the name-spacing of fragments, maybe it's ugly, but can't we preface all the fragments with "#overlay-" or something like that? And I could be missing something, but why are GET parameters a problem - can't they just remain GET parameters?

Gábor Hojtsy’s picture

@David: ok, how would the URL (incl. GET parameters) look when I am on "search?q=Paris" and have an overlay open which displays "admin/people/events?region=Europe".

David_Rothstein’s picture

Well, ideally something like "search?q=Paris&region=Europe#admin/people/events" but OK, I see how there are edge cases here.

Gábor Hojtsy’s picture

@David: then how would we know which query string part is for which page? You have probably anticipated, but we can also play the idea with search?q=Paris with an overlay displaying admin/peeople/events?q=Prague, which easily breaks down.

ksenzee’s picture

We could encode the ?q= bit when it's in the fragment URL. I agree that it might break fragments for other modules though.

eigentor’s picture

Hm the overlay looks strange recently.
No close button...

Also the "meta" tab and the second save button have vanished, and all meta settings are in vertical tabs at the bottom.
What have I missed?

Ack, saving a node opens Garland in the overlay, help!

mcrittenden’s picture

I for one wouldn't be the least bit upset if fragments didn't make it in. It's a pretty good chunk of work with very little payoff. I can't think of a single situation in which I'd miss them if they weren't there.

David_Rothstein’s picture

@mcrittenden: So do you have a different solution for the problem where you click on a link, get taken out of the overlay suddenly, and don't have any clear way to get back to where you were?.....

webchick’s picture

Note: If you are are interested in testing this patch, make sure to download the images from http://drupal.org/files/issues/images_9.zip and stick them in modules/overlay/images.

I burned 5 minutes looking for these, so trying to save the next person some grief. ;)

Gábor Hojtsy’s picture

@eigentor: The node UI changes you mention (meta tab, more submit buttons) were never part of this patch and did not get code freeze exception either, so will probably not happen. Issue at #472126: D7UX: Move buttons to right area, add content and meta selector. The close button will only appear if you also copy the image, see Angie's note in #324.

ksenzee’s picture

Assigned: Unassigned » ksenzee

I discussed this with Noyz, David, Joshua and a few others this morning, and we decided the overlay approach is worth trying. We couldn't think of any other way to fix the problem of getting your overlay content back. I'll come back with a patch as soon as I can.

ksenzee’s picture

Um, duh, the *fragment* approach.

/me casts about for the nearest caffeine source...

mcrittenden’s picture

@David_Rothstein: Yeah, my solution is to not worry about it. IMO an overlay should be kept as an overlay, and not as its own mini site. Yes, I know, I'm probably alone on this one, but every vote counts.

At most, let's make it a follow-up. It's time for this to get in there. (what sun said)

sun’s picture

oh boy. If you keep on derailing this issue with more and more wishlists and feature requests, then I can assure you that it won't make it into D7.

If the Overlay would provide a proper API that is invoked by Toolbar module, then we'd have a clean and simple thing with sufficient possibilities to implement any special URL fragment or whatever handling in a follow-up patch.

So pretty please, please focus on the initial implementation and get that implementation right.

What we need *now*, is a proper implementation. Any issues and tweaks around CSS styling, images, borders, and whatever can be done after 10/15. Likewise, bugs in JS or CSS can be dealt with after. But all of that can only be done afterwards if there will be a solid implementation in a core-worthy and commit-worthy state on 10/15. The current implementation is not.

ksenzee’s picture

Assigned: ksenzee » Unassigned

Fixing the close overlay interaction is neither a wishlist nor a feature request. If it doesn't get fixed, the overlay is fundamentally broken. I have the time and JS ability to handle fragments. I do not have what it takes to reimplement the entire overlay as an API. Anyone who cares to work on it at the same time as I am is welcome to join me on github or ignore my patches entirely.

seutje’s picture

even if you were to implement fragment history with this initial patch, you might wanna considering implementing one of the perfectly decent existing jquery plug-ins instead of re-inventing the wheel, but I agree that "initial" should be initial and we should build off that

Dries’s picture

- I don't think the overlay modules should provide a generic reusable API for all overlay needs in Drupal, but we should have an easy to use API for other modules to use. I agree with @sun in #305 that we should focus on clean separation and APIs first (if we haven't already).

- We should close the overlay when a non-admin endpoint is requested. I agree with ksenzee that this makes for an odd user-experience, but we could classify this as a UX bug that needs fixing after 15/10. We can fix the 'back'-button stuff later.

So where are we from a separation / API point of view?

Gábor Hojtsy’s picture

Question is what is the spec for that API.

From the Drupal vs. overlay point of view, modules that wish to open links in the overlay can mark those links with a "to-overlay" class. Links and forms used in the overlay will keep opening in the overlay. If a link is not to be opened in the overlay, modules can use the "overlay-escape" class on the link. If a form submission is to close the overlay, overlay_close_dialog() is to be invoked in the form submission process to pass that information on. The underlying page title is moved over to above the overlay, the tabs are moved over to display on the overlay, these can be manipulated via normal Drupal means (with title and tabs for the page being displayed).

Other then that, the overlay implements a very specific use case of the generic jQuery UI dialogs plugin, in that it overshadows the whole page, is not possible to move around, is not scrolling but expands to scroll with the page, has special treatment for titles and tabs coming from the page, will still need treatment for workflows like node previews temporarily closing the overlay in-between multiple edits of the node, etc. While the opening/closing mechanisms provided also allow opening whatever page in the overlay, making it a generic page displayer on top of your Drupal site, from a UX perspective, it is a very specific user interaction, which was designed for administering your site.

I'm absolutely unsure as to what level of reuse would be advocated in terms of breaking this down to an API and an implementation. It is already an implementation of the generic jQuery UI Dialog plugin for the needs of this UX problem. It is so specific in how it looks and behaves that I don't get the point where it could be broken into an API and implementation above jQuery UI dialogs but below its current overlay functionality. Unfortunately use cases were not enumerated yet on the issue either to help figure out what kind of API would be needed for that at all.

I hope this extended explanation of the matter helps figuring out the answer finally.

sun’s picture

The use-case is fairly simple:

Instead of Toolbar module, Administration menu may want to trigger the Overlay. The current code always assumes Toolbar and performs some hard-coded tweaks to make it look right when displayed next to Toolbar.

That has to go, and requires some larger changes in the setup, positioning, and handling of the overlay.

ksenzee’s picture

@sun: Thank you for the specific use case. I'll tweak the JS and CSS and see if I can't get rid of the remaining hardcoded toolbar stuff. Admin menu and modules like it will still have to have some way of specifying elements that are not to be hidden by the overlay. The best I can come up with is "overlay-displace-top" and "overlay-displace-bottom" classes, which admin menu etc. will have to add to their markup. Other ideas welcome. (Sure would be nice to have hooks in JS... hm...)

Re the question of how to maintain overlay state or fix the back button, Gábor pointed out that we have a blue "ribbon" bar UI element that was never implemented in the edit in place patch -- mostly because it's not actually edit in place. It would work well here as a place to remind the user they're still in an administrative action, and to provide a link for them to reopen the toolbar. It might be preferable to using fragments to fix the back button. Dries, my understanding is that it's okay to consider it a UX bug that we lose overlay state, and to fix it later in a separate issue. Note that we might want to use either URL fragments or this blue ribbon element to solve the problem. If I'm wrong, and we need to fix it today, correct me quickly. :)

xmacinfo’s picture

Please open a new issue for the "blue ribbon". :-)

David_Rothstein’s picture

  1. If it can be fixed after October 15 (as Dries suggests) that would be great, and a new issue is appropriate, but I don't think I can say this any better than Katherine already said above...

    Fixing the close overlay interaction is neither a wishlist nor a feature request. If it doesn't get fixed, the overlay is fundamentally broken.

    And we don't currently have a clear path forward for the solution.

  2. Also, regarding the "overlay-escape" class, we discussed this a bit above -- are we prepared to have this be something that every module author thinks about adding manually? In other words, every time someone uses the l() function, do they need to think about whether the page they're linking to is a non-administrative page, and if so, remember to do this:
    l(t('Some title'), 'node/' . $node->nid, array('attributes' => array('class' => array('overlay-escape'))));
    

    If any modules don't do that, "non-administrative" content will wind up getting displayed in the overlay sometimes anyway, whether we want it to or not.

    I'd suggest that if it's that fundamental, this class be added internally inside the l() function (just like that function currently adds the 'active' class to links now).

  3. A minor point: This patch has lots of references to the global $custom_theme variable which won't work correctly anymore (actually, many of them would not have worked anyway when combined with certain contrib modules, and the overlay theme switching would have been messed up).

    To really fix this, we'll probably have to wait until some followup work at #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed is done and committed -- in the end, overlay_init() will probably have to be replaced with something like overlay_custom_theme(). Not something to really worry about now, but writing it down while I remember it.

ksenzee’s picture

The attached takes care of the hardcoded toolbar positioning. There really wasn't much left to change there. @sun, let me know if it's acceptable to you. I'm also including a diff of the changes since #312 to make it easier to review.

I'll open a separate issue for the ribbon and post a patch there soonish.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

I'm on testing this patch and working to solve some of the outstanding issues.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
FileSize
54.98 KB

This patch fixes many outstanding bugs:

- restoring the breadcrumbs (and its styling as per the mocks); this was using the new theme hook params badly
- restoring the node form submission close behavior; this was due to element hook changes
- fixed the overlay to work again in Safari; this was broken due to how the links were constructed
- changed the click handler for .to-overlay links to not modify the link href => allows to right-click open in new tab/window, without that opening with the overlay rendering treatment; also fixes the link handler to support having fragments on the links, etc.
- changed the link processor in the child to attach a click handler instead of modifying the links; reused the same code as in the parent, abstracted that out to a method on its own
- tried to fix tab styling (again) using pixel based font, line and margin/padding sizes; works good in Firefox again and only little broken in Safari (the inactive tab is 1px farther then should be on each side)

Also attempted to work on these bugs but could not figure them out:

- $custom_theme is not supported anymore (but code is left in the patch for now to work with it), so the node add form is showing up in Garland in the overlay (eh!); David talks about the follow up work required to make this function in the patch which removed $custom_theme at http://drupal.org/node/553944#comment-2139652 (it's an acknowledged limitation and it does not seem to be possible at the moment to make theme switching work with the overlay)

- the submission callback is called multiple times for some reason in the form handling process, so the node submission does not redirect to the right node path (it keeps being on the same page where you are); this is due to only the second invocation of the callback knowing the redirection properly, but being too late to store that for the JS

- attempted to make the admin user creation form also close the overlay and redirect; that redirect worked (which makes the node redirect suspicious for some local problem), BUT it redirects now to creating another user, not to the user URL (unlike node creation), so we need to think about this more instead of just applying this outright (I did remove this code from the patch, it was just checking $form['#id'] == 'user-register-form' by side of checking the node form)

Existing bugs which I reproduced again but did not look into them:

- in Safari, the overlay shrinks in width on successive page loads to width, where it does not shrink anymore; this does not happen in Firefox; I'd assume it takes the width setting at a wrong time, when animating

- when a tall page was requested and a less tall page follows in the overlay, the background keeps being tall; I've tried to look into this before by using callbacks on the resizing animator, but it did not work out for some reason

Bojhan, I still cannot reproduce your scrolling bug and/or your Appearance page bug.

Handing this off.

seutje’s picture

#340: in Safari, the overlay shrinks in width on successive page loads to width, where it does not shrink anymore; this does not happen in Firefox; I'd assume it takes the width setting at a wrong time, when animating

what the hell? I remember fixing this for FF during the D7UX spring in Utrecht, and it seems to be pretty much the same thing
it gets smaller by 17 pixels every time, which is exactly the width of the scrollbar

but that's obviously more of a follow-up thing

ksenzee’s picture

I think everything's now separated cleanly. This patch adds the following:

- All admin links now trigger the overlay, not just links inside the toolbar. I believe this is the correct behavior, but it does seem a little odd on local tasks (node view is in the parent, node edit is in the overlay...). At any rate, there's no longer a need for the "to-overlay" class.
- @David 337.2: Point taken. sun said something similar in IRC, I believe. Which links are "admin" is now defined within Drupal.overlay.isAdminLink(). Modules that need to override the default algorithm for determining an admin link may add a list of links or a regex to Drupal.overlay.links.admin[modulename] or Drupal.overlay.links.notAdmin[modulename]. It's not very performant, although we can add some static caching. Better solutions are welcome (for the next few hours...).

I'm now tracking down bugs (and there's debug code in here). One bug I'm aware of in addition to the ones Gábor pointed out:

- Dashboard's customize feature has a "Done" button (a simple input type=submit that submits to the same page, just as a way to refresh the page) and it reloads the overlay contents, toolbar and all. (yoroy pointed this out: http://skitch.com/yoroy/ndgqk/doubledouble). I'm guessing a JS link that simply refreshed the page would cause the same problem. Still thinking about how to fix this. You know some contrib module is going to do something similar, so I'm not sure changing Dashboard's behavior is a real solution.

Talked to Dries yesterday and he would like the back button to work after the overlay is closed, so it's looking like fragments are the way to go, as opposed to the blue ribbon. I'll work on that after the overlay is otherwise committable.

ksenzee’s picture

aargh. my patch numbering always seems to be off...

cweagans’s picture

Could we possibly get the overlay in without the URL fragments and then get those in later as an 'accessibility bug'?

Gábor Hojtsy’s picture

@cweagans: I think that is the plan if the fragment based code does not work out today.

Anonymous’s picture

#342: "Better solutions are welcome (for the next few hours...)." Okay, this probably isn't better. It is an idea though. We could add one item to the hook_menu. 'admin' => TRUE or 'admin' => FALSE.

It would allow us to specify any path as admin or not. This could easily be reused by any module that cared whether or not something was considered an "admin item."

Okay. Horrible idea. Don't hurt me!

Dries’s picture

Yes, we can get the overlay in without the URL fragments. We can follow-up with the fragments later.

ksenzee’s picture

@Joshua: I was pretty enthusiastic about this idea, and I mentioned it to Gábor, but he pointed out that every time somebody l()'s to a path we'd have to load its menu item. That's probably a no-go. :/

Yeah, fragments are not happening today. So far all I've had time for is separation of code and bugfixing.

ksenzee’s picture

FileSize
55.46 KB

This includes a fix for the dashboard brokenness, but I fixed it in dashboard.module, not in overlay. :/ Without the ?render=overlay GET parameter, there simply is no way for a page to know it's being loaded in the overlay -- short of setting a session variable, which I wasn't ready to commit to. The fix in dashboard module will work for now.

/node/add links now trigger the overlay as well.

Gábor Hojtsy’s picture

@ksenzee: Using sessions would come with a few problems, mainly that you could not browse in a non-overlay way parallel to your administration. Your action to use overlay should not have an effect on your parallel browsing experiences for the same site IMHO.

As for the done button, it can very well be a button styled link. I know I experimented with making the customize dashboard button a simple link originally, which would be styled like a button, but it became a link anyway. I know this is not a generic solution, but for these interactions, the modules will really need to be aware of a possible overlay. Just like modules are aware of other possible enhancements like drag table headers or AHAH updates on the page which affect how they write code to blend in with the rest of Drupal.

ksenzee’s picture

Yeah, I made the button a link and restyled it to look button-y. You're right that other modules will have to be overlay-aware to some degree. And I agree on sessions. I think we could work it so that the session info was detailed enough that your other browsing window wouldn't be affected, but I don't like session in general at all. So I'm happy to take a pass on that one.

Working on the redirect bugs at the moment.

cweagans’s picture

Found a bug: If enable the overlay module, go away from the modules page, then come back to the modules page and disable the overlay module, the resulting page is still loaded in the overlay.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

The patch works excellently for me. :)

Something to think about after initial commit: editing a node and then clicking save, should it really close the overlay, or should it return you to where you were? (In my case, the find content screen.)

Either way, that can definitely be worked on later.

Dries’s picture

Status: Reviewed & tested by the community » Needs review

I get this error when trying to save something from within the overlay: Fatal error: Cannot use assign-op operators with overloaded objects nor string offsets in ./drupal-cvs/modules/overlay/overlay.module on line 95

Dries’s picture

Some extra small feedback.

+++ misc/tableheader.js
@@ -18,8 +18,9 @@ Drupal.behaviors.tableHeader = {
+      // Clone thead so it inherits original jQuery properties. Hide the table

'thead' is a typo?

+++ modules/overlay/overlay.api.php
@@ -0,0 +1,54 @@
+ *
+ * The
+ * parent window is initialized when a page is displayed in which the overlay
+ * might be required to be displayed, so modules can act here if they need to
+ * take action to accomodate the possibility of the overlay appearing within

That wraps funny.

+++ modules/overlay/overlay.module
@@ -0,0 +1,310 @@
+function overlay_library() {

It would be good to have a description somewhere of the overlay-parent vs overlay-child separation.

+++ modules/overlay/overlay.module
@@ -0,0 +1,310 @@
+  global $custom_theme;

Do we need to add a @todo here as well? $custom_theme no longer exists?

+++ modules/overlay/overlay.module
@@ -0,0 +1,310 @@
+  $variables['classes_array'][] = "overlay-displace-top";

This could use some phpDoc. Just mention that adding this class pushes the overlay down below the toolbar.

+++ modules/overlay/overlay.module
@@ -0,0 +1,310 @@
+ * the list of submit handler and add our own at the end, so we can affect the

... list of submit handler_s_ ?

+++ modules/overlay/overlay.module
@@ -0,0 +1,310 @@
+ * redirection done at the end of the form processing if we are in the overlay
+ * children.

I think we should better document why we add our own handler, and what that handler does.

xmacinfo’s picture

ksenzee’s picture

FileSize
56.11 KB

Addressed all of Dries's feedback above, except docs on the parent-child distinction, which cweagans is working on. #354 is fixed (boy is it hard to chase HEAD tonight!)

Redirection is still wonky from the node save form. We're adding our submit handler as an after_build callback on both the submit button and the node form itself, which is why it gets called twice. I'm pretty sure we can tweak the conditions under which we call overlay_close_dialog() so that it doesn't get called until we have the right redirect path, but I haven't had time to look at it yet. quicksketch has kindly offered. :)

webchick’s picture

Yoink! Taking this for a spin. *cracks knuckles*

webchick’s picture

Status: Needs review » Needs work

(commit-blocker) I was able to break this right away with the following steps:

1. Click "Dashboard"
2. Click "Customize"
3. Click "Done"

Ta-da!

Recursion

(note: ignore my screwed up shortcut bar; that's from the other patch. I'm speaking here of the recursion.)

However, it's gotten smarter since the last time I triggered this recursion bug since it only works one level deep instead of infinite. :)

(commit-blocker not this patch) When I clicked "log out" I got:

Fatal error: Call to undefined function menu_load_links() in /Users/webchick/Sites/core/modules/shortcut/shortcut.module on line 214

And now my Drupal install is completely hosed! :)

Going to jump back to shortcuts for awhile.

webchick’s picture

Status: Needs work » Needs review

Omg. IGNORE me. PEBCAK error.

Going back to shortcuts anyway though since David needs to get to bed.

Anonymous’s picture

FileSize
56.86 KB

This patch should remove the bug where removing the overlay module while in the overlay causes a second toolbar to appear.

markus_petrux’s picture

The following may generate a javascript error because when it loads, you're not yet sure a parent window exists. This could happen if someone loads a child on new window.

+
+/**
+ * Add the isObject() method to the overlayChild object for convenience.
+ */
+Drupal.overlayChild.isObject = parent.Drupal.overlay.isObject;
+
+/**
+ * Add the isAdminLink() method to the overlayChild object for convenience.
+ */
+Drupal.overlayChild.isAdminLink = parent.Drupal.overlay.isAdminLink;
+
+/**
+ * Add the addOverlayParam() method to the overlayChild object for convenience.
+ */
+Drupal.overlayChild.addOverlayParam = parent.Drupal.overlay.addOverlayParam;

In regards to isObject(), I think a nice place to put this would be in Drupal object, so it can be reused elsewhere.

webchick’s picture

So I had every intention of reviewing this patch in detail tonight, but I have officially run out of steam. So just as with the shortcuts patch, I'd like to see how far this can get in ~24 hours. It feels pretty darn close, though it also seems a bit easy to break.

Brief list of weirdness, some of which might be purely fatigue-related. I'll post a more thorough review tomorrow:
- This patch makes node/add and node/X/edit not in the admin theme. Why?!? It's very jarring.
- Even more jarring is when Seven opens up in some overlay panels and Garland in others. Though ksenzee informed me that this is because of a follow-up bug from the hook_menu custom theme patch not being able to set the theme properly.
- At random intervals, I am able to get two copies of the toolbar going: one on the outside, one within. I think it has something to do with being on a page like admin/X/X already and then clicking one of the shortcut bar items.
- The interaction of the overlay when first enabling and disabling the module is a bit whack.
- In FF I was not seeing the close bar, in Safari I was.

Gábor Hojtsy’s picture

@webchick:

(commit-blocker) I was able to break this right away with the following steps:

1. Click "Dashboard"
2. Click "Customize"
3. Click "Done"

Ta-da!

Tried to reproduce this both in Firefox and Safari, but can't. Also, errors in shortcuts are definitely not related to this patch, since shortcut module is neither in code nor in this patch.

@markus_petrux: do you suggest just copying the code to both JS files or set up a common JS loaded before the parent and child.js?

markus_petrux’s picture

Re: "do you suggest just copying the code to both JS files or set up a common JS loaded before the parent and child.js?"

I would implement the isObject() method in drupal.js.

The other 2 methods in child.js that are assigned to the one in parent.js are not really necessary because we can use those defined in the parent window.

For example, we can do this:

          var linkURL = parent.Drupal.overlay.addOverlayParam($(this).attr('href'));
          parent.Drupal.overlay.load(linkURL);
          return false;

instead of:

          var linkURL = Drupal.overlayChild.addOverlayParam($(this).attr('href'));
          parent.Drupal.overlay.load(linkURL);
          return false;

and the same could be applied to isAdminLink() in child.js

All we need is a version of isObject() method that can be easily reused, here and it would also be nice if it could be reused elsewhere, IMHO.

Gábor Hojtsy’s picture

FileSize
59.01 KB

Ok, drupal.js got isObject added. Others are invoked via parent.Drupal.overlay.*.

Tested more based on @webchick's feedback:

- This patch makes node/add and node/X/edit not in the admin theme. Why?!? It's very jarring.
- Even more jarring is when Seven opens up in some overlay panels and Garland in others. Though ksenzee informed me that this is because of a follow-up bug from the hook_menu custom theme patch not being able to set the theme properly.

David et al. worked hard to clean up theme swicting in Drupal 7, thereby removing any possibility for us to force these pages to be in the admin theme. These two issues are the same. This happened a few days ago, and discussed at http://drupal.org/node/553944#comment-2139652

- At random intervals, I am able to get two copies of the toolbar going: one on the outside, one within. I think it has something to do with being on a page like admin/X/X already and then clicking one of the shortcut bar items.

As I've said above, I cannot reproduce this neither in Firefox, neither in Safari.

- The interaction of the overlay when first enabling and disabling the module is a bit whack.

I don't see this with Joshua's latest update. The reloaded module page does not try to use the overlay right after being disabled, and when it is enabled, the overlay starts to be used from the page when the next link is clicked. What interaction do you expect here, which is not implemented?

- In FF I was not seeing the close bar, in Safari I was.

Cannot reproduce. I can reproduce a 1px "distance" of the close button though in Safari. A rounded border is seen where it should not be. That is probably not what you see and is minor and fixable.

Let's continue the reviews! :)

marcvangend’s picture

@webchick:
I don't really see what you mean with "The interaction of the overlay when first enabling and disabling the module is a bit whack".
However your remark made me think (or maybe you actually meant something like this): after enabling the module, wouldn't it be good for the UX to immediately show the effect? I'd say that redirecting to the home page and re-opening the module page in the overlay is a really convincing way to say "the overlay module has been enabled".

mcrittenden’s picture

FileSize
14.12 KB
7.3 KB

A couple things from testing in FF3.5 (on Ubuntu Linux)

Whenever I submit a form inside of the Overlay (ex: save on Modules page or save on Appearance page or Submit on the add node form), I get the following error. Reproduced numerous times...the submit always works correctly but this error shows up inside of the overlay afterwards:

Fatal error: Cannot use assign-op operators with overloaded objects nor string offsets in /var/www/drupal/modules/overlay/overlay.module on line 57

Useless screenshot of that attached. Is that one just me?

Also, whenever I click on something in the toolbar and then in the overlay click to a different tab of that section (example: Configuration and Modules in the toolbar and then the Modules tab, or Appearance and then the Configure tab), the top few pixels gets cut off inside of the overlay. Screenshot attached

Looking really awesome, though. I'm really happy with how fast it loads. Scrolling is a bit painful because the overlay seems to fight with the toolbar (the toolbar goes over top of it for a split second until the toolbar pops up again), but that seems to be a browser thing that we can't really help much. Awesome work, guys.

P.S. How do I put images into these? Input filter strips <img>.

effulgentsia’s picture

subscribing

ksenzee’s picture

Assigned: Unassigned » ksenzee

I think Gábor is offline now, so I'll pick this back up. I'm pulling what I think is all the remaining feedback together, in increasing order of severity.

  1. #368: Fatal error: Cannot use assign-op operators with overloaded objects nor string offsets in /var/www/drupal/modules/overlay/overlay.module on line 57

    I was seeing this last night with earlier versions of the patch, but I think I fixed it. Can anyone reproduce it today?

  2. #363: This patch makes node/add and node/X/edit not in the admin theme. Why?!? It's very jarring. Even more jarring is when Seven opens up in some overlay panels and Garland in others. Though ksenzee informed me that this is because of a follow-up bug from the hook_menu custom theme patch not being able to set the theme properly.

    As Gábor said, these are the same problem, and we really can't fix it in this patch. We can get rid of the variable_set('node_admin_theme', 1) since it's not really related to this module's functionality.

  3. #363: In FF I was not seeing the close bar, in Safari I was.

    I can't reproduce this either.

  4. #340: when a tall page was requested and a less tall page follows in the overlay, the background keeps being tall; I've tried to look into this before by using callbacks on the resizing animator, but it did not work out for some reason

    This seems minor enough to be a followup.

  5. #366: I can reproduce a 1px "distance" of the close button though in Safari. A rounded border is seen where it should not be. That is probably not what you see and is minor and fixable.

    Can someone with a Mac look into this? If not, I'm assuming we can fix it after commit.

  6. #340: in Safari, the overlay shrinks in width on successive page loads to width, where it does not shrink anymore; this does not happen in Firefox; I'd assume it takes the width setting at a wrong time, when animating

    I think seutje is on this.

  7. #355: It would be good to have a description somewhere of the overlay-parent vs overlay-child separation.

    cweagans pointed out in IRC that this separation is in fact so unclear that you can't just read the patch and write the docs. You have to have your head totally around the patch. So I'll work on this today.

  8. #363: The interaction of the overlay when first enabling and disabling the module is a bit whack.

    I like marcvangend's idea in #367 about redirecting home and reopening the modules page in the overlay.

  9. #368: Also, whenever I click on something in the toolbar and then in the overlay click to a different tab of that section (example: Configuration and Modules in the toolbar and then the Modules tab, or Appearance and then the Configure tab), the top few pixels gets cut off inside of the overlay.

    I'll install FF3.5 and try to reproduce. It's not happening for me on FF3.0/Ubuntu.

  10. #340: the submission callback is called multiple times for some reason in the form handling process, so the node submission does not redirect to the right node path (it keeps being on the same page where you are); this is due to only the second invocation of the callback knowing the redirection properly, but being too late to store that for the JS

    This is still a problem AFAIK, and is why we're getting weirdness on node creation. I'll keep working on it today. I believe I can fix it.

  11. #363: At random intervals, I am able to get two copies of the toolbar going: one on the outside, one within. I think it has something to do with being on a page like admin/X/X already and then clicking one of the shortcut bar items.

    This is a serious problem. I am not even sure how to fix it. It's the same root problem as the dashboard problem I "fixed" in #349, and that I was stewing about in #342. With good reason, as it turns out. 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, which is added via Javascript to all the admin links on a page. The overlay module checks for that parameter on hook_init(). If it's not there, all we get is a regular Drupal page, not a specially formatted overlay child page with no toolbar.

    We originally saw this problem in the dashboard, but Batch API causes it too, and so will anything else that redirects without going through drupal_goto() or a FAPI redirect. I'm racking my brain for ways to solve this globally. Solutions welcome.

marcvangend’s picture

@ksenzee: as discussed in IRC, here's my patch against D7 HEAD + #366, which adds the behavior described in #367. I'm attaching it as a txt file so hopefully I will not confuse the testbot.

ksenzee’s picture

FileSize
61.11 KB

marcvangend, you rock! I'm attaching an updated patch with those changes included. Here's what it addresses:

#370/7: The best documentation we had for the parent/child separation was for overlay_mode(), so I clarified and expanded that, and added references to it throughout. Also expanded phpdoc throughout.

#370/8: marcvangend came up with a session-based approach that works great. Could probably stand some people banging on it though (session makes me nervous). Eventually, when we get the back button fixed with fragments, we can get rid of the session variable and simply redirect to example.com/#overlay=admin/config/modules.

#370/11: I couldn't find a way of handling this globally that wasn't a total embarrassing hack. So I resorted to whack-a-mole, and just fixed the redirection method for Batch API, which was the only situation in which I could reproduce the bug. This could also use some people banging on it, in case I missed a spot.

I couldn't reproduce #370/9 on the same browser and platform, so I'd say it's a followup if we run into it after commit.

Remaining todos:
#370/4-6: I think these are minor enough to qualify as followups.
#370/10: Node creation weirdness. I keep putting this off. :) It's the only commit-blocker left, though, as far as I can tell.

Status: Needs review » Needs work

The last submitted patch failed testing.

ksenzee’s picture

Assigned: ksenzee » Unassigned
Status: Needs work » Needs review
FileSize
61.45 KB

Hm. There are changes to batch API in the patch, and I would normally assume that's why the testbot is choking on install, but as it happens I can't install vanilla HEAD without Javascript. So I'm at a loss to troubleshoot. I'll let testbot have another whack at it.

At any rate, node creation now redirects correctly. That's the last major issue I was aware of. Reviewers have at it.

Status: Needs review » Needs work

The last submitted patch failed testing.

marcvangend’s picture

@#374:
Unfortunately I'm not very good with the testing framework (yet). Even if there is a good reason why the automated tests are failing, that needs to be fixed. I mean, some simpletests would have to be rewritten to match changes in batch API, right? I don't feel capable enough to take the next step, which worries me because we need to keep going and not let it slip out of our hands in the last minute.

Gyt’s picture

FileSize
64.14 KB
21.49 KB

Last patch work well for me. Only problem is broken "Add content" link in shortcut bar within overlay (screenshot attached).
I also attempted to attach patch #374 for new HEAD.

drifter’s picture

Status: Needs work » Needs review

setting to 'needs review' for the bot to have a go at #377

Status: Needs review » Needs work

The last submitted patch failed testing.

drifter’s picture

#377 is seriously broken - overlay.module gets installed in the drupal root instead of /modules/overlay.

Manually installing #374 works OK.

mfer’s picture

FileSize
12.45 KB

As Drifter points out, #377 is really broken. When I tried to apply it there was a rejected section in dashboard.css and fuzz factor is needed to apply the patch. Also, the overlay module is placed in root directory folder. Only overlay.info is in the overlay module folder.

I think I fixed these two issues. But, drupal head isn't installing for me so I stopped testing here. The attached patch is an untested attempt to fix the fuzz and to put the overlay module in it's folder.

Gyt’s picture

Status: Needs work » Needs review
FileSize
65.08 KB

Hm, new patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

drifter’s picture

Status: Needs work » Needs review
FileSize
59.16 KB

So I'm struggling to re-roll one of the above patches that will pass both manually installing and PIFR. #382 was missing a newline at the end, but even when I fixed it, I couldn't install manually (got some error about MergeQuery not loaded).

Gabor's #366 was the last patch that I remember PIFR passing without a "Failed to install HEAD". So I'm trying to reroll that one. I succeeded installing it, however, the admin overlay just doesn't show. I tried disabling and re-enabling it - is there some setting I need to change also to make it appear?

Anyhow, attaching a rerolled #366 and hoping for the best.

marcvangend’s picture

Drifter, you re-rolled against #366, but you did not include my addition from #371?
That one is (as requested by ksenzee, who was also working on a patch on that moment) not a complete patch, but a patch against HEAD + #366 so it was easier to merge into ksenzee's patch that became #372.

I propose that we first see how your #384 performs with the test bot (although it bothers me that you say the overlay doesn't show - unfortunately I don't have time to test it right now) and then roll #371 back in.

[edit] Great, the test has already finished, thanks Drifter. Let's roll #371 back in now.[/edit]

drifter’s picture

FileSize
61.36 KB

OK, here we go, one step at a time - Gábor's #366 and marcvangend's #371 rerolled together.

[edit] Overlays are now opening for me - I have a bit of an unusual setup, Apache with VirtualDocumentRoot, so clean URLs are disabled if I don't edit .htaccess. This apparently also breaks the overlay, though it should work with them off. Something to debug later.

drifter’s picture

FileSize
64.78 KB

Now rolling ksenzee's #374 - the one with the Batch API changes.

Status: Needs review » Needs work

The last submitted patch failed testing.

drifter’s picture

Status: Needs work » Needs review
FileSize
62.32 KB

#374 reroll failed to install HEAD (as with the original) - though a manual install works for me. Now trying #374 without the batch.inc and form.inc changes, just to see if it would pass.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

We discussed this extensively in IRC with drifter, and I'm trying to roll in some of the improvements we discussed and fixes for issues we found.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
FileSize
65.53 KB
28.85 KB
29.29 KB

So worked on more of the issues @drifter and I identified. The installer overlay autoloading and the isAdminLink function was not in any way compatible with non-clean-URLs, so not surprising it did not work that much for @drifter. Attempted to fix that by looking at the q=param of the URL if a base path is otherwise not available beyond Drupal's root.

Also, fixed a minor issue with Safari close button rendering that I mentioned above.

Finally, worked on fixing some of the issues with "Add to shortcuts" links. They showed up in the page body at a very obscure place in the iframe, but were designed to show up by the title.

Now fixed to show up by the title (even if not yet perfectly aligned). This also gets them apply the shortcut link theming from the parent page, so the plus button and the description works as expected. Shortcut specific code here is not nice, but I had no better idea on how could we implement the specific spec here.

However, shortcutting actual pages in the overlay does not work due to an integration bug I've noted weeks ago on the shortcuts issue. We get "render=overlay" on the shortcut link due to the overlay having that query string argument. The plus button also got a target="overlay-element", so that results appear in the overlay. Anyway, basically, shortcutting anything in the overlay is going to be broken. Even more so, shortcutting anything, which has a query string is broken, because the whole path + query string is stored. So even if we'd special case the "render=overlay" item to be automatically removed, it would still stop you from being able to shortcut any page with query string arguments (pagers, filters, etc).

Worked off of #387 (including the form.inc and batch.inc changes), so let's see what is broken now.

Also note that I found an easy and perplexing way to get Garland and the toolbar in the overlay. Just go to "Add content" and try to shortcut that. You get a 403 page (amazingly) and Garland and the toolbar. You can shortcut many other pages (eg. Find content - with the above bug of that shortcut not being actually useful), but not this one.

Unassigning in the interest of hopefully getting others to look at this again, not that I do not intend to work on it more.

Status: Needs review » Needs work

The last submitted patch failed testing.

dmitrig01’s picture

+++ misc/drupal.js	19 Oct 2009 12:20:10 -0000
@@ -344,4 +344,11 @@ Drupal.theme.prototype = {
+ */
+Drupal.isObject = function(something) {
+  return (something !== null && typeof something === 'object');

something isn't a great variable name. how about item or object?

+++ modules/dashboard/dashboard.js	19 Oct 2009 12:20:10 -0000
@@ -65,7 +65,7 @@ Drupal.behaviors.dashboard = {
   setupDrawer: function () {
-    $('div.customize .canvas-content').prepend('<input type="button" class="form-submit" value="' + Drupal.t('Done') + '"></input>');
+    $('div.customize .canvas-content').prepend('<a class="button" href="">' + Drupal.t('Done') + '</a>');
     $('div.customize .canvas-content input').click(Drupal.behaviors.dashboard.exitCustomizeMode);
$('div.customize .canvas-content').prepend($('<a class="button" href="#">' + Drupal.t('Done') + '</a>').click(Drupal.behaviors.dashboard.exitCustomizeMode));
+++ modules/overlay/overlay-child.js	19 Oct 2009 12:20:10 -0000
@@ -0,0 +1,138 @@
+
+    // Install onBeforeUnload callback, if module is present.

what's the onbeforeunload callback, and why does it have any business in core?

Other than that, looks good. I hvae no idea why the testbot is failing it.

dmitrig01’s picture

Also, the add to shortcuts link still appeared in the body and it doesn't work.

[Edit: the link being generated is something liek: http://localhost/head/admin/config/system/shortcut/shortcut-set-1/add-li...

Gábor Hojtsy’s picture

@dmitrig01: did you try my latest patch and read my comments on shortcuts there?

boombatower’s picture

Per request from Gábor Hojtsy I attempted to look into the bot not being able to install the patch...

Applied patch locally and was not able to install with javascript disabled which is most likely why the bot cannot. It only install 36 tables (38 with js) and then after the profile screen I get a white screen of death with (also with js on so maybe head just broke):

Fatal error: Class 'DrupalQueue' not found in /home/boombatower/software/drupal-7/modules/update/update.install on line 13 
boombatower’s picture

Ok, last comment is outdated. Confirmed the bot sees the config page unlike the rest of us.

This is due to the URLs they end up on.

Firefox ends up with batch API parameters on the end of the url...the bot does not. If you install manually and then remove them...wala.

A look at what the bot sees: http://imagebin.ca/view/C_k5UnM.html.

I am thus assuming:

1) simpletest browser does not interpret the redirect right and FF does, 2) the batch API is broken

ksenzee’s picture

So we have a couple of major issues left.

The first is that our fixes to Batch API seem to break something in the installation process. That's been really hard to debug, since installation works fine with Javascript on (either with or without this patch), and without Javascript, installing HEAD is completely broken (with or without this patch). In #398, boombatower narrowed in on why testbot can install HEAD with JS disabled but other browsers can't. I've opened an issue for that at #608826: Install breaks without Javascript enabled if you have has_js cookie set.

The other issue is that shortcuts and overlays do not play nicely together. I'll reproduce Gabor's summary from #391:

However, shortcutting actual pages in the overlay does not work due to an integration bug I've noted weeks ago on the shortcuts issue. We get "render=overlay" on the shortcut link due to the overlay having that query string argument. The plus button also got a target="overlay-element", so that results appear in the overlay. Anyway, basically, shortcutting anything in the overlay is going to be broken. Even more so, shortcutting anything, which has a query string is broken, because the whole path + query string is stored. So even if we'd special case the "render=overlay" item to be automatically removed, it would still stop you from being able to shortcut any page with query string arguments (pagers, filters, etc).

<snip>

Also note that I found an easy and perplexing way to get Garland and the toolbar in the overlay. Just go to "Add content" and try to shortcut that. You get a 403 page (amazingly) and Garland and the toolbar. You can shortcut many other pages (eg. Find content - with the above bug of that shortcut not being actually useful), but not this one.

I'm working on the shortcut issue, but if someone else were to tackle Batch API it would be lovely. I *think* it's committable without the Batch API fix, but it sure would be nice to get it all wrapped up today.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
524 bytes
65.43 KB

OK, I believe this fixes the batch API testbot problem, but we won't know for sure until we let the testbot run it, so let's see what happens...

As can be seen from the interdiff, this was a one line fix - but it took an embarrassing amount of time to actually find :)

ksenzee’s picture

FileSize
63.91 KB

Yay! Nothing like having people around to fix your stupid mistakes. :/ Thanks a bunch David. Here's an updated version that fixes the menu_get_item weirdness, so you can add shortcuts correctly.

sun’s picture

Category: task » feature
Status: Needs review » Needs work

1) Overlay is displayed ("replacing" the original page) when installing it manually.

2) (bogus)

3) Fieldsets, JS, and scrolling in general is horribly slow within the Overlay.

4) Firefox currently eats 50% of my computers resources.

5) The browser freeze is reproducable. Firefox bumps to 100% (one processor, or 50% multi-threaded) as soon as an Overlay is opened. CPU decreases to normal after closing. (FF 3.5 on Windows 2003 (XP)).

6) (bogus)

7) When disabling Toolbar module from within the Overlay, it is still there. WTF? ;)

webchick’s picture

4) and 5) seem to be a Windows-only situation, since only Bojhan and sun so far have reported it. Bojhan's on FF 3.5 (also tried 3.1) and Vista.

sun’s picture

+++ includes/batch.inc
@@ -109,11 +109,20 @@ function _batch_progress_page_js() {
+  // Add ?id=x to the batch URL.
+  $url_options = $batch['url_options'];
+  if (isset($url_options['query'])) {
+    $url_options['query']['id'] = $batch['id'];
+  }
+  else {
+    $url_options['query'] = array('id' => $batch['id']);
+  }
@@ -189,7 +198,17 @@ function _batch_progress_page_nojs() {
+  // Add ?id=x&op=y to the batch URL.
+  $url_options = $batch['url_options'];
+  if (isset($url_options['query'])) {
+    $url_options['query']['id'] = $batch['id'];
+    $url_options['query']['op'] = $new_op;
+  }
+  else {
+    $url_options['query'] = array('id' => $batch['id'], 'op' => $new_op);
+  }
+  
+  $url = url($batch['url'], $url_options);

This needs more docs, because it makes no sense at all without.

So please elaborate on "Why?" (in-code, not on-issue).

I'm on crack. Are you, too?

David_Rothstein’s picture

5) The browser freeze is reproducable. Firefox bumps to 100% (one processor, or 50% multi-threaded) as soon as an Overlay is opened. CPU decreases to normal after closing. (FF 3.5 on Windows 2003 (XP)).

I've something similar with Firefox on Linux too (there, it's "Xorg" that jumps to 100% of CPU). Around 200 comments ago, I remember it being discussed that this was due to browser caching and that clearing the cache fixes it.... Still, seems like a pretty weird and disturbing problem.

+    $link_path = preg_replace('/\?.+/', '', $_GET['link']);

Doesn't this prevent any shortcut from containing any query string at all? As a temporary fix to get this in, it makes sense though :) It seems to me like the real bug is in menu_get_item() and possibly in shortcut_valid_link() also, though.

ksenzee’s picture

#402/1: I'm not entirely sure what you're seeing here, but I'm getting the expected behavior on a fresh install, on both FF3.0/Ubuntu and FF3.0/XP. I tried it on both the default profile (after disabling the overlay module) and on the minimal profile (after setting Seven as the admin theme). In both situations, when I enable the overlay module, the modules page reopens in the overlay and redirects the parent window to the homepage.

#402/3-5: I'm using FF3.0/XP on a 2.5-year-old Dell (Intel Core2 Duo 2.4 GHz, 4GB RAM) and it's positively snappy. As in a joy to use. I can't reproduce any of these performance problems. I could try serving the site on the XP box as well to see if that breaks, but FF on XP as a client has no performance issues that I can detect. IE7 seems fine too.

#404: This is not new code. Perhaps we could open a new issue for better documentation of Batch API. As it is, the only inline docs that are there are the ones I added.

#402/7: Good point. This is the same root problem as the fact that when you add a new shortcut (or otherwise edit the toolbar) the changes don't show up until the next full refresh of the parent window. We need an AJAX refresh of just the toolbar under certain conditions. I'd like to do this as a followup; I'm not sure whether or not the new AJAX framework supports something like this.

ksenzee’s picture

David_Rothstein: I talked to chx in IRC and according to him, menu_get_link() is not meant to be passed a query string ever. So yeah, that would be a separate issue.

webchick’s picture

I spent about two hours with this patch tonight. Here's a list of the things I found. This is a UI review only. I'll be looking at the code shortly.

Note that no decisions will be made on this patch tonight. Dries is en route to DC tomorrow morning at the crack of dawn, and he wants time to take a thorough look at this before we make any sort of calls. So for now, here's my review. Skip down to the summary section if you don't care about the nitty-gritty details (and I wouldn't blame you!)

First thing I tried was applying this patch to an existing installation of Drupal 7 and turning on the module. That did not end well. ;)
http://img.skitch.com/20091020-fxiuynsx17pcu97waikm2x1hxh.png

However, things look fine on a fresh installation, and Katherine confirmed that she did make sure this patch worked on expert profile, so there is nothing that inherently ties it to Default profile, unlike Shortcuts. Probably some caching bug.

The first thing I tried was seeing if I could break shortcuts with this patch. There were two issues:

1. I get a shortcut add link on pages where the link is already in my current shortcut bar. This bug exists in HEAD as well. See #609100: Should not be able to re-add same shortcut to same set twice.

2. I managed to accidentally delete all of my shortcuts. ;) This bug exists in HEAD as well. See #609122: shortcut_set_save() no longer deletes existing links when a new set of links is passed in.

3. After adding a shortcut to the shortcut bar, it doesn't immediately appear. This is going to require some kind of AJAX fanciness, and may involve bug fixes to the new AJAX library in Drupal 7. I therefore think I agree with pushing this off to a follow-up issue.

I then tried to break the dashboard. Dragging/dropping blocks worked fine. The overlay even deals with me clicking "Configure block," changing the title of the block, and redirecting back to the Dashboard page properly. Very nice! :D

I found a couple of places where core fails to give feedback upon changing configuration, but these also exist in HEAD: #609108: Menu admin page should say something when you save #609128: Dashboard should provide feedback when clicking "Done"

At one point I also received "The block Powered by Drupal was assigned to the invalid region -1 and has been disabled." However, I have no idea what steps I went through to get that error, and so I wasn't able to verify whether or not it exists in HEAD. :( This wouldn't be a commit-blocker in any case, though, since on the whole the dashboard seems to work surprisingly well!

I *think* I might've reproduced the horizontal scrollbar issue. Try setting your window to 1024x768 or whatever. Load up the overlay. Now, shrink your window size down a bit. Even though the content area scales down appropriately (yay!), I'm still getting a horizontal scrollbar as though the full-size content area was still there.

Scrollbar

This is quite minor, and fixing this can be a follow-up patch.

Now, to break edit in place. This I actually was able to do. Main page, click "Configure block" on "Powered by Drupal" block. Change some settings or whatever. Click save. You're taken to the front page (garland) loaded in the overlay:

Front page in overlay

This one is definitely less than ideal, since it's something that people will probably hit a lot since those links are so prevalent. I take it we need another instance here of whatever fixed it for nodes.

On this topic, I do notice that in general, the overlay contents seems to load sllowwwlllyyy. I'm not sure what that's about. It could be that I'm running xdebug profiling, perhaps. :P But it does make certain interactions, such as creating a node, pretty awkward:
1. I see the flash of the "Add to shortcuts" link on the node form when I first go there.
2. After submitting the form, I see the form re-presented to me in the overlay, then I see the default front page with no content (where I started from), and finally I see the contents of node/1.

I also noticed that neither of the user menus work while in the overlay:

Overlay links

I haven't read all 400+ replies :P but I could actually see the argument for this being "by design" at one point or another, so that certain links on the toolbar wouldn't take you back to the front-end theme. I still found it pretty jarring, however. I wonder if there's a way to either fix this, or somehow indicate visually that these links are temporarily inactive.

Then this gave me all sorts of other mean ideas, like clicking a node title from the node listing at admin/content and running cron. Yet, these all seemed to work fine. Darn! ;) I did find #609140: Rewrite outdated references to taxonomy_term_node and #609146: Tons-O-Notices running cron by playing around with this, but those are bugs in HEAD as well. I even tried changing the admin theme, but that also worked.

Also confirmed that the latest version plays nicely with Batch API. Yay! No double-toolbars in SimpleTest anymore!

I definitely see what Bojhan and others were talking about ~200 replies ago with the fact that you can't link to a page in the admin panel anymore, and that the behaviour of the overlay is inconsistent when you do:

Edit node overlay

Edit node URL

As you can imagine, this also has "interesting" implications for the back button, in that you can get 4-5 levels deep in your administrative interface, hit back, and be taken to whatever page you were at *before* the page where you originally clicked the overlays. Very disorienting.

This is the main part of the patch that, frankly, is worrying... as in its current state it results in a net loss in usability in the admin panel. I realize we have contingency plans around using URL fragments, but I still am not sure how this will work in practice to both ensure a consistent user experience, fix the back button, and allow URLs to be bookmarkable. As to whether this is a commit-blocker, I'm afraid I don't know. But it is definitely a huge WTF (in fact, I'd even upgrade it to a full-on WTFBBQ, to use a JohnAlbin phrase ;)).

UI REVIEW SUMMARY

  • Overall, the patch works great. I tried to break it in many different ways, and only managed to do so in a couple of them.
  • I also think shipping this kind of interaction in core would raise the bar for open source CMSes and get them competing with us on usability, which would be a nice change. ;)
  • It is rather slow, at least in places (though this could also be my development environment).
  • The fact that shortcuts don't show up immediately upon adding them is a regression and usability WTF. However, I'm comfortable kicking this to a follow-up patch that does not have 400+ replies, since this is going to involve some AJAX trickery.
  • The fact that the user links aren't clickable when the overlay is open is a critical issue that I'd like to see fixed in this patch.
  • The fact that clicking "configure block" on the home page breaks overlays is a critical issue I'd like to see fixed in this patch.
  • A variety of smallish visual bugs, such as mis-alignments, unnecessary scrollbars, etc. I'm comfortable with all of these being handled as follow-ups.
  • The biggie of the breaking of linkable URLs and the back button. This is definitely a critical issue, that's too big to fix here, and I'm nervous kicking it to a separate issue.

That's where things sit right now with me. I'll dive into the API next.

RobLoach’s picture

FileSize
216.9 KB

Ran into an overlay inside an overlay when I was playing around with the dashboard.

marcvangend’s picture

@ Sun in #402-1:
I think you missed the discussion in #367, #370, #371 & #372.

webchick’s picture

Meh. Sorry, but I totally ran out of steam for tonight. :( Here's a partial review as far as I got before my eyeballs started burning.

Summary: Major red flags around the Drupal.overlay.isAdminLink() stuff. Probably a commit blocker. :\ There's also some lengthy bit of logic going on in overlay_init() which looks a bit suspect, though I'm not sure what else to do there. On the whole, though the code in this patch is really well-documented (particularly the JS stuff, which I appreciate :D).

+++ includes/batch.inc
@@ -109,11 +109,20 @@ function _batch_progress_page_js() {
+  // Add ?id=x to the batch URL.
+  $url_options = $batch['url_options'];
+  if (isset($url_options['query'])) {
+    $url_options['query']['id'] = $batch['id'];
+  }
+  else {
+    $url_options['query'] = array('id' => $batch['id']);
+  }
@@ -189,7 +198,17 @@ function _batch_progress_page_nojs() {
+  // Add ?id=x&op=y to the batch URL.
+  $url_options = $batch['url_options'];
+  if (isset($url_options['query'])) {
+    $url_options['query']['id'] = $batch['id'];
+    $url_options['query']['op'] = $new_op;
+  }
+  else {
+    $url_options['query'] = array('id' => $batch['id'], 'op' => $new_op);
+  }
+  
+  $url = url($batch['url'], $url_options);

Let's document this please. I don't understand why you need to go through all of these hoops.

+++ misc/drupal.js
@@ -344,4 +344,11 @@ Drupal.theme.prototype = {
+/**
+ * Check if the given variable is an object.
+ */
+Drupal.isObject = function(something) {
+  return (something !== null && typeof something === 'object');
+};

Wow, really? jQuery doesn't have one of these? jQuery sucks! :P

+++ modules/locale/locale.test
@@ -237,7 +237,7 @@ class LocaleTranslationFunctionalTest extends DrupalWebTestCase {
-    preg_match('!admin/config/regional/translate/edit/(\d)+!', $this->getUrl(), $matches);
+    preg_match('!admin/config/regional/translate/edit/(\d+)!', $this->getUrl(), $matches);

Minor, but I could commit this fix in about 30 seconds if it were a separate issue.

+++ modules/locale/locale.test
--- /dev/null
+++ modules/overlay/overlay-child.js

I don't really parse JS, so am pretty useless for a review here... but I wanted to say the comments in this file are very good, and helped me understand what was happening. :)

+++ modules/overlay/overlay-parent.js
@@ -0,0 +1,764 @@
+Drupal.overlay.isAdminLink = function (url) {

Ok, now this function? This function is pretty ghetto. :\ (To be fair, ksenzee admitted as much in #drupal earlier. :))

The first thing that makes me nervous here is that module developers (who are PHP wizards) need to learn *JavaScript* in order to add/exempt their links from the overlay. Eek. :\ But additionally, we're basing this off some crazy regex pattern stuff.

It'd be much better if there was a hook (or /maybe/ a generic "admin" => TRUE type of extension to hook_menu()...) that allowed module developers to declare *in PHP* which links should be overlay-enabled.

I realize that this must get tricky though when we talk about keeping the overlay code logically separated from the rest of core. So I can see why this approach was taken. But this code seems very hackish, and is probably a commit blocker. :\

+++ modules/overlay/overlay.api.php
@@ -0,0 +1,52 @@
+ * @return
+ *   None.
...
+ * @return
+ *   None.

(minor) We don't need @return None if there's nothing to return.

+++ modules/overlay/overlay.info
@@ -0,0 +1,7 @@
+files[] = overlay.module

overlay.install is not listed here.

+++ modules/overlay/overlay.module
@@ -0,0 +1,386 @@
+  // @todo: custom_theme does not exist anymore.

Is this still true? You seem to have used it elsewhere without this todo.

+++ modules/overlay/overlay.module
@@ -0,0 +1,386 @@
+ * @param $set
+ *   If set, will set the current close dialog mode to the given state.
+ *   Use FALSE to disable close dialog mode. Otherwise, the argument will
+ *   be forwarded to the onOverlayClose callback of the overlay.

I had to read this quite a few times and didn't quite get it. One problem is the name of $set for that variable. Maybe $active or something would work better?

+++ modules/shortcut/shortcut.admin.inc
@@ -519,19 +519,25 @@ function shortcut_link_delete_submit($form, &$form_state) {
+        'link_title' => $_GET['name'],

Don't we need to check_plain() this?

A: Oops. No. This is handled down below thanks to t().

+++ modules/toolbar/toolbar.module
@@ -53,6 +70,10 @@ function toolbar_preprocess_html(&$vars) {
+function template_preprocess_toolbar(&$variables) {

PHPDoc please. Also this and the corresponding bit of code in toolbar.tpl.php could be committed very quickly separately, to make this patch a bit easier to review.

+++ themes/garland/style.css
@@ -508,6 +508,15 @@ body.two-sidebars .region-footer {
+/* Don't display any header elements when within the overlay, and adjust the page height accordingly. */

(minor) Wrap comments at 80 chars.

I'm on crack. Are you, too?

webchick’s picture

@Rob Loach: Could you post your steps to reproduce? I tried several times to get overlays to do that again, and failed. :(

seutje’s picture

Wow, really? jQuery doesn't have one of these? jQuery sucks! :P

it was on the roadmap for 1.4 -> http://docs.jquery.com/JQuery_1.4_Roadmap#Core
but it looks like it's pretty broken -> http://dev.jquery.com/ticket/4946

marcvangend’s picture

@ Webchick #411:

There's also some lengthy bit of logic going on in overlay_init() which looks a bit suspect, though I'm not sure what else to do there.

If you're referring to the two-stepped $_SESSION['overlay_just_enabled'] variable over there: ksenzee and myself weren't too happy about the use of sessions there either, but I couldn't find a better way to do it. The thing is: we want to redirect after a form has been processed, but you can't implement that behavior with a form alter because the module would have to alter the form before it is enabled. By the way, this code would get a little cleaner once the use of url fragments is implemented, because we could just redirect to [basepath]#overlay-admin/config/modules.

Gábor Hojtsy’s picture

@webchick/@sun/@David_Rothstein: I could reproduce the slowness on Firefox 3.5 on Mac. I think the peformance of the overlay depends on the exact browser version as much as the browser brand. Overlay works snappy in Safari but is noticably slow in Firefox 3.5. (Even if I just leave Firefox 3.5 open with the overlay and do something else, it uses huge amounts of CPU resources).

@webchick: "The block Powered by Drupal was assigned to the invalid region -1 and has been disabled." bug can be reproduced without the overlay and was caused by region support added to the block configure forms. Posted your reproduction note on http://drupal.org/node/503782#comment-2127414

@webchick: on shrinking overlay width, I can reproduce that in Safari, but not in Firefox 3.5. It does shrink multiple times but stops after a while. I bet it is due to how the width is calculated in relation to the animation and other effects.

@webchick/@ksenzee: I've had the impression that the in-overlay back button behavior works, but it indeed does not. That is simply solved by replacing doc.location.replace(url) with doc.location.href = url; in Drupal.overlay.load(). replace() does brake back button behavior. This does not solve back-ing into an overlay state from a node submission or from clicking a node in a node listing though.

Looking at the other feedback too.

Gábor Hojtsy’s picture

FileSize
68.54 KB

Here is an updated patch with the following fixes based on feedback from @webchick:

- documented the batch API changes (new 'url_options' batch setting now documented, its use to retain existing query string values is documented as well)
- @return none removed,
- overlay.install added to files list
- changed docs and variable name on overlay_close_dialog(), the trick is that this is used to set the overlay mode AND possibly pass on a value to the close callback
- added doc to template_preprocess_toolbar()
- wrapped garland CSS

Also worked in the fix to the back button problem when inside the overlay.

Fixed a reappearance of a bug I fixed before. The "to-overlay" class was used in some places in the overlay JS, but it is not a trigger anymore. So I fixed those places. This makes the active toolbar buttons to turn off their down-state when you close the overlay again. As expected.

I've also took considerable time trying to figure out why the user menu is not clickable when the overlay is on. I don't think it is intended, but could not figure out what is blocking that. Grm.

On isAdminLink(), yeah, we could go right into the menu API and have an admin => TRUE key on each menu item, but while it would be mostly overlay specific code in the menu, as documented above it would also require loading the menu item for every possible link displayed via url() or l() (and sometimes, when links are generated via JS from url() values passed in JS settings, this would not even be possible at all). Now loading menu items for all links on a page just to decide whether they are admin or not seems crazy. Also, trying to figure out each case what kind of menu item node/3245/subscriptions/manage belongs to. Would require the whole path -> menu key parsing logic to run before loading the menu item even. Is that an acceptable penalty? Any better ideas?

seutje’s picture

@@ -65,7 +65,7 @@ Drupal.behaviors.dashboard = {
    * Helper for enterCustomizeMode; sets up drag-and-drop and close button.
    */
   setupDrawer: function () {
-    $('div.customize .canvas-content').prepend('<input type="button" class="form-submit" value="' + Drupal.t('Done') + '"></input>');
+    $('div.customize .canvas-content').prepend('<a class="button" href="">' + Drupal.t('Done') + '</a>');
     $('div.customize .canvas-content input').click(Drupal.behaviors.dashboard.exitCustomizeMode);
 
     // Initialize drag-and-drop.

I see the input was replaced with a regular link, but it seems like the things looking for an input weren't adapted to reflect these changes...

perhaps this would be better:

$('<a>').addClass('button').attr('href', '').text(Drupal.t('Done')).prependTo('div.customize .canvas-content').click(Drupal.behaviors.dashboard.exitCustomizeMode);

or even

$('<a href="" class="button">').text(Drupal.t('Done')).prependTo('div.customize .canvas-content').click(Drupal.behaviors.dashboard.exitCustomizeMode);

or would that make it less readable?

David_Rothstein’s picture

Regarding the isAdminLink() function, I have to say that as a PHP developer who is a bit shaky on JavaScript, looking at that code did not scare me nearly as much as I would have thought based on reading the above :)

If we decide to keep it in JavaScript (which seems to make some sense in theory, since the overlay is a JS feature), here are three things I can think of to improve it:

  1. Seems like there is some code duplication between the "admin" and "nonAdmin" sections that could be removed to make the function simpler and more readable?
  2. Can the overlay module eat its own dog food - i.e., rather than hardcoding within isAdminLink() the fact that node/* and admin/* are considered admin pages for the purposes of the overlay, can it implement its own "hook" instead and put that in Drupal.settings.overlay.admin[modulename] like it recommends module developers do? That way, as a module developer who doesn't know JavaScript very well, I have an example in core that I can emulate.
  3. Instead of regular expressions, can we use a syntax that is more like the menu system? E.g., node/%/edit and node/%/delete... The isAdminLink() function could then convert that into a regular expression internally.

Any thoughts on whether those changes would make it committable?

webchick’s picture

You know, I didn't think about:

On isAdminLink(), yeah, we could go right into the menu API and have an admin => TRUE key on each menu item, but while it would be mostly overlay specific code in the menu, as documented above it would also require loading the menu item for every possible link displayed via url() or l() (and sometimes, when links are generated via JS from url() values passed in JS settings, this would not even be possible at all).

And also:

If we decide to keep it in JavaScript (which seems to make some sense in theory, since the overlay is a JS feature)...

I was also thinking about this in the shower today, and for the most part it's far more common to define menu paths that /are/ admin paths than are not. So 'admin' => TRUE on every single thing would get really annoying.

I really like David's suggestions in #418. That would help this code indeed feel a lot less like a hack and more like an API of sorts. Is there someone who can work on that today by chance?

Incidentally:

I've also took considerable time trying to figure out why the user menu is not clickable when the overlay is on. I don't think it is intended, but could not figure out what is blocking that. Grm.

This isn't a commit-blocker for me, since it's just a bug, but it is definitely a release blocker. We just want to be careful of how many of these release blockers we're accumulating, since that number has gone up significantly in the past week with all the exception patches. ;)

sun’s picture

OK. The progress on this issue simply can't be followed anymore. There seem to be quite a couple of outstanding implementation issues and bugs that need to be fleshed out properly. Comment paging at #300+ adds to the nightmare, and we are even slowly getting to a 3rd page. :(

Here is what I propose we do now:

  1. Create an issue titled "Overlay API changes", which contains a summary of the required changes and a patch that only consists of those other changes for Batch API, regex, and stuff. That patch, we will commit first and can be done (and polished) really quick.
  2. Create an issue titled "Overlay implementation", which contains an initial in-depth summary of all remaining issues in the form of a list; some example items:
    1. When the overlay is displayed, it sucks up all CPU on Firefox 3.5 (perhaps only Windows).
    2. The targeting of administrative links does not work reliably. For example, when ... The current implemention (is doing)...
    3. Certain form submissions are displaying a non-administrative page in the overlay. For example when the overlay is triggered by a contextual link (...) The current implementation (is doing)...
    4. ...

    This will be the main issue we will work on to get the implementation right. We will try to get as many menu system, form API, JavaScript, and AJAX experts into that issue to find a working solution ASAP.

    Due to the split from 1), this won't touch or change any APIs at all, because it's mainly JavaScript only. Note that everything committed after this one must be considered as API change.

  3. Create an issue titled "Overlay styling issues", which contains an initial summary of the already identified styling issues. This issue, we will start to work on after 2) has been properly resolved and committed.

All of those issues we tag with "Overlay".

Especially the initial summary for 2) will need some serious love and work to get it right. Only if the summary is right from the start, we are able to point people there and let them directly make any sense of the implementation issues to think about proper solutions.

effulgentsia’s picture

Following up on #420: I created an issue for the API changes: #610204: Overlay API changes. I'll start preparing the patch for #2 and #3, but the sooner the API changes can be committed, the sooner those patches can pass testbot, so please review #610204: Overlay API changes.

effulgentsia’s picture

Links to the other two issues for after #610204: Overlay API changes lands:
- #610234: Overlay implementation (someone, please summarize on that issue what still needs to be resolved). Even though that patch can't be testbot'd until the API issue lands, developers can still try to troubleshoot outstanding problems if they can understand what those problems are without reading 400+ comments.
- #610238: Overlay styling issues

Gábor Hojtsy’s picture

Issue tags: +overlay

@webchick: your horizontal scroll bar in #408 is an artifact of the background webpage(!) being wider then the viewport, not related to the overlay being wide. Not at all sure how to make this not result in a scrollbar. Including this in my summary for #610234: Overlay implementation.

mgifford’s picture

Oh my gosh this is a long issue queue! Is the patch in #416 the best one to review? How close is this to getting into core? Seems like a pretty powerful function if we're close.

I"m confused about if this issue is now a duplicate of:

#610204: Overlay API changes

or

#610234: Overlay implementation

I'm assuming the latter. I want to look at the accessibility issues here.

ksenzee’s picture

@mgifford: The latest patch is at #610234: Overlay implementation. I suggest testing the patch in #48. In terms of functionality, all that's left to do is get the toolbar buttons to work in situations where you've just edited the toolbar.

mgifford’s picture

Thanks. I've done some testing of #48. This is too long a thread for me to read it all. Gabor's video helped - http://www.vimeo.com/7451703

Can we mark this issue as a duplicate or closed since discussions have moved and this thread is no longer active (I hope - 426 comments is way too many).

Bojhan’s picture

Status: Needs work » Closed (fixed)

Closing this issue, we will continue discussion at #610234: Overlay implementation

xqbzzr’s picture

I havent read all 400 threads and i dont know, if this has been reported before, but right now i am testing d7 dev of 12/9 on an older pc and the dashboard-overlay is a huge performance killer! The fan speeds up to max and i am barely able to work with d7. I am looking for a way to deactivate the dashboard so i can continue testing.
Please keep in mind that eye-candy should never hinder performance.
Thank you for reading!

David_Rothstein’s picture

@xqbzzr: This issue is no longer active. Please see the other overlay issues, and in particular #615130: Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems

In the meantime -- or any time you want, really :) - you can install Drupal using the "minimal" profile which does not enable the overlay module.