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.

Files: 
CommentFileSizeAuthor
#416 overlay-517688-416.patch68.54 KBGábor Hojtsy
#409 overlayinsideoverlay.png216.9 KBRobLoach
#401 overlay-517688-401.patch63.91 KBksenzee
Passed: 14288 passes, 0 fails, 0 exceptions View
#400 overlay-517688-400.patch65.43 KBDavid_Rothstein
Passed: 14255 passes, 0 fails, 0 exceptions View
#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
Failed: Failed to install HEAD. View
#389 374-reroll-nobatchapi.patch62.32 KBdrifter
Passed: 13475 passes, 0 fails, 0 exceptions View
#387 374-reroll.patch64.78 KBdrifter
Failed: Failed to install HEAD. View
#386 366plus371-reroll.patch61.36 KBdrifter
Passed: 13489 passes, 0 fails, 0 exceptions View
#384 366-reroll.patch59.16 KBdrifter
Passed: 13457 passes, 0 fails, 0 exceptions View
#382 517688-374-overlay2.patch65.08 KBGyt
Failed: Failed to apply patch. View
#381 drupal-overlay.patch12.45 KBmfer
Failed: Failed to install HEAD. View
#377 addlink.png21.49 KBGyt
#377 517688-374-overlays.patch64.14 KBGyt
Failed: Failed to install HEAD. View
#374 517688-374-overlay.patch61.45 KBksenzee
Failed: Failed to install HEAD. View
#372 517688-372-overlay.patch61.11 KBksenzee
Failed: Failed to install HEAD. View
#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
Failed: Failed to apply patch. View
#361 517688-361-overlays.patch56.86 KBJoshuaRogers
Failed: Failed to apply patch. View
#357 517688-357-overlay.patch56.11 KBksenzee
Failed: Failed to apply patch. View
#349 517688-349-overlay.patch55.46 KBksenzee
Failed: Failed to apply patch. View
#342 517688-341-overlay.patch53.87 KBksenzee
Failed: Failed to apply patch. View
#342 changes_since_340.patch.txt6.23 KBksenzee
#340 517688-340-overlay.patch54.98 KBGábor Hojtsy
Failed: Failed to apply patch. View
#338 517688-337-d7ux-overlay_0.patch51.68 KBksenzee
Failed: Failed to apply patch. View
#338 changes_since_312.patch.txt4.08 KBksenzee
#312 517688-312-d7ux-overlay.patch50.58 KBJoshuaRogers
Failed: Failed to apply patch. View
#310 Alignmentstructure.png4.3 KBBojhan
#310 tabsspacing.png2.68 KBBojhan
#298 pagenotfound.png41.17 KBJoshuaRogers
#297 517688-297-d7ux-overlay.patch50.34 KBJacobSingh
Failed: Failed to apply patch. View
#296 517688-296-d7ux-overlay.patch50.36 KBksenzee
Failed: Failed to apply patch. View
#287 517688-287-d7ux-overlay.patch58.14 KBJoshuaRogers
Failed: Failed to apply patch. View
#274 letsusejqueryuicorrectly.patch52.28 KBJoshuaRogers
Failed: Failed to apply patch. View
#268 LetsUsejQueryUICorrectly.patch52.24 KBRobLoach
Failed: Failed to apply patch. View
#268 Screenshot128.61 KBRobLoach
#267 OverlayFirefox.png57.55 KBGábor Hojtsy
#264 drupal-overlay.264.patch53 KBsun
Failed: Invalid PHP syntax in modules/locale/locale.test. View
#257 Screenshot-1.png167.86 KBRobLoach
#256 Screenshot-2.png177.98 KBRobLoach
#256 overlay.patch49.89 KBRobLoach
Failed: 13268 passes, 1 fail, 0 exceptions View
#252 drupal-overlay.252.patch52.77 KBsun
Failed: Failed to apply patch. View
#249 overlay-517688-249.patch49.97 KBmarkus_petrux
Failed: Failed to apply patch. View
#248 overlay-517688-248.patch49.98 KBmarkus_petrux
Failed: 13469 passes, 1 fail, 0 exceptions View
#245 overlay-517688-245.patch51.85 KBGábor Hojtsy
Failed: 13687 passes, 1 fail, 0 exceptions View
#242 overlay-517688-242.patch52.09 KBGábor Hojtsy
Failed: Failed to apply patch. View
#242 TabsForMe.png13.76 KBGábor Hojtsy
#241 overlay-517688-241.patch50.25 KBGábor Hojtsy
Failed: 13698 passes, 1 fail, 0 exceptions View
#240 overlay-517688-240.patch57.08 KBmarkus_petrux
Failed: Failed to apply patch. View
#229 tabs.png9.35 KBBojhan
#229 marks.png10.58 KBBojhan
#225 overlay.patch50.81 KBGábor Hojtsy
Failed: Failed to apply patch. View
#212 overlay_n.patch49.5 KBGábor Hojtsy
Failed: 12990 passes, 1 fail, 0 exceptions View
#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
Failed: Failed to apply patch. View
#200 d7ux_overlay_39.patch47.62 KBmarcvangend
Failed: Failed to install HEAD. View
#197 d7ux_overlay_38.patch45.97 KBmarcvangend
Failed: Failed to apply patch. View
#192 d7ux_overlay_37.patch45.97 KBmarcvangend
Failed: Failed to apply patch. View
#190 d7ux_overlay_36.patch45.92 KBmarcvangend
Failed: Failed to apply patch. View
#190 images.zip3.95 KBmarcvangend
#187 d7ux_overlay_35.patch45.88 KBcwgordon7
Failed: Failed to apply patch. View
#184 images.zip5.1 KBcwgordon7
#183 d7ux_overlay_34.patch46.42 KBcwgordon7
Failed: 13677 passes, 1 fail, 0 exceptions View
#179 overlay_css2.png32.61 KBmarcvangend
#179 overlay_css3.png33.89 KBmarcvangend
#170 d7ux_overlay_32.patch49.23 KBmarcvangend
Failed: 13682 passes, 1 fail, 0 exceptions View
#168 Weird overlay.png140.02 KBDamien Tournoud
#162 d7ux_overlay_31.patch49.11 KBcwgordon7
Failed: 13688 passes, 1 fail, 0 exceptions View
#160 d7ux_overlay_30.patch48.98 KBcwgordon7
Failed: 13709 passes, 1 fail, 0 exceptions View
#157 d7ux_overlay_29.patch49.22 KBcwgordon7
Failed: 13666 passes, 1 fail, 0 exceptions View
#153 d7ux_overlay_28.patch49.06 KBcwgordon7
Failed: 13671 passes, 1 fail, 0 exceptions View
#150 d7ux_overlay_26.patch48.89 KBcwgordon7
Failed: 12943 passes, 4 fails, 0 exceptions View
#148 d7ux_overlay_26.patch49.8 KBmarcvangend
Failed: 13672 passes, 1 fail, 0 exceptions View
#146 d7ux_overlay_25.patch49.47 KBcwgordon7
Failed: 13655 passes, 1 fail, 0 exceptions View
#144 d7ux_overlay_24.patch49.5 KBcwgordon7
Failed: 13702 passes, 1 fail, 0 exceptions View
#141 d7ux_overlay_23.patch49.13 KBcwgordon7
Failed: 13706 passes, 1 fail, 0 exceptions View
#123 d7ux_overlay_22.patch46.81 KBcwgordon7
Failed: 13686 passes, 1 fail, 0 exceptions View
#122 overlay.png20.01 KBrobertgarrigos
#121 d7ux_overlay_21.patch46.72 KBcwgordon7
Failed: 13682 passes, 1 fail, 0 exceptions View
#120 d7ux-overlay-images-2.zip6.47 KBGábor Hojtsy
#115 d7ux_overlay_20.patch46.56 KBcwgordon7
Failed: 13675 passes, 1 fail, 0 exceptions View
#113 d7ux_overlay_19.patch46.38 KBcwgordon7
Failed: 13680 passes, 1 fail, 0 exceptions View
#111 d7ux_overlay_18.patch46.42 KBcwgordon7
Failed: Failed to apply patch. View
#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
Failed: 12360 passes, 4 fails, 0 exceptions View
#97 ResizingFunny.png44.11 KBGábor Hojtsy
#80 d7ux_overlay_13.patch43.75 KBcwgordon7
Failed: 12356 passes, 4 fails, 0 exceptions View
#79 d7ux_overlay_12.patch43.76 KBcwgordon7
Failed: Failed to apply patch. View
#77 d7ux_overlay_11.patch42.95 KBcwgordon7
Failed: 12036 passes, 8 fails, 1 exception View
#75 d7ux_overlay_10.patch42.91 KBcwgordon7
Failed: Failed to apply patch. View
#74 d7ux_overlay_09.patch40.38 KBcwgordon7
Failed: Failed to apply patch. View
#66 scrollbar33.02 KBJuliaKM
#66 missing_close74.36 KBJuliaKM
#63 d7ux_overlay_08.patch40.45 KBcwgordon7
Failed: 12067 passes, 8 fails, 1 exception View
#56 d7ux_overlay_07.patch38.51 KBcwgordon7
Failed: 12038 passes, 8 fails, 1 exception View
#53 d7ux_overlay_06.patch38.41 KBcwgordon7
Failed: Failed to install HEAD. View
#50 d7overlaycamino.png89.15 KBleisareichelt
#39 tabs.png9.96 KBBojhan
#37 d7ux_overlay_06.patch38.41 KBcwgordon7
Failed: Failed to install HEAD. View
#35 d7ux_overlay_05.patch38.24 KBcwgordon7
Failed: Failed to install HEAD. View
#21 overlay_517688.patch34.13 KBdrewish
Failed: Failed to apply patch. View
#10 overlay.png63.28 KBcatch
#3 d7ux_overlay_03.patch35.29 KBcwgordon7
Failed: Failed to apply patch. View
#2 d7ux_overlay_02.patch35.28 KBcwgordon7
Failed: Failed to apply patch. View
Overlay.jpg84.05 KBGábor Hojtsy
d7ux-overlay-images.zip49.63 KBGábor Hojtsy
d7ux-overlay.patch35.46 KBGábor Hojtsy
Failed: 12381 passes, 4 fails, 0 exceptions View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to install HEAD. View

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
Failed: Failed to install HEAD. View

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
Failed: Failed to install HEAD. View

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
Failed: 12038 passes, 8 fails, 1 exception View

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
Failed: 12067 passes, 8 fails, 1 exception View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: 12036 passes, 8 fails, 1 exception View

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
Failed: Failed to apply patch. View

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
Failed: 12356 passes, 4 fails, 0 exceptions View

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
Failed: 12360 passes, 4 fails, 0 exceptions View

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
Failed: Failed to apply patch. View

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
Failed: 13680 passes, 1 fail, 0 exceptions View

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
Failed: 13675 passes, 1 fail, 0 exceptions View

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
Failed: 13682 passes, 1 fail, 0 exceptions View

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
Failed: 13686 passes, 1 fail, 0 exceptions View

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
Failed: 13706 passes, 1 fail, 0 exceptions View

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
Failed: 13702 passes, 1 fail, 0 exceptions View

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
Failed: 13655 passes, 1 fail, 0 exceptions View

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
Failed: 13672 passes, 1 fail, 0 exceptions View

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
Failed: 12943 passes, 4 fails, 0 exceptions View

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
Failed: 13671 passes, 1 fail, 0 exceptions View

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
Failed: 13666 passes, 1 fail, 0 exceptions View

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
Failed: 13709 passes, 1 fail, 0 exceptions View

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
Failed: 13688 passes, 1 fail, 0 exceptions View

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
Failed: 13682 passes, 1 fail, 0 exceptions View

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
Failed: 13677 passes, 1 fail, 0 exceptions View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to install HEAD. View

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
Failed: Failed to apply patch. View

@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
Failed: 12990 passes, 1 fail, 0 exceptions View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: 13698 passes, 1 fail, 0 exceptions View

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
Failed: Failed to apply patch. View

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
Failed: 13687 passes, 1 fail, 0 exceptions View

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
Failed: 13469 passes, 1 fail, 0 exceptions View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: 13268 passes, 1 fail, 0 exceptions View
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
Failed: Invalid PHP syntax in modules/locale/locale.test. View

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
Failed: Failed to apply patch. View

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

JoshuaRogers’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.

JoshuaRogers’s picture

FileSize
52.28 KB
Failed: Failed to apply patch. View

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?

JoshuaRogers’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.

JoshuaRogers’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.

JoshuaRogers’s picture

FileSize
58.14 KB
Failed: Failed to apply patch. View

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

JoshuaRogers’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

FileSize
50.36 KB
Failed: Failed to apply patch. View

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

FileSize
50.34 KB
Failed: Failed to apply patch. View

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

JoshuaRogers’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

JoshuaRogers’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.

Pages