Ajax Popups in Drupal 7: Adding Modal Dialogs to Help, Confirmations and Filter tips (Unified)

Several things

1) can you roll the patch with the -p option?
2) no tabs
3) you need to re-think the admin_get function - why not make the path like /dialog or something and set access to TRUE

I'd like to see something similar used as well to keep people from leaving forms without saving changes. This approach is going to need some refinement however.

-this.dialog_width = 480;
-This should be defined by a CSS class rather than hard coded.

- All the markup in the javascript file should live within a Drupal.theme.prototype function.
- This approach will only affect the links on the page that have the 'popup' attribute attached to them. I think it'd make more sense to find all form elements on the page, see if they've been modified, then throw the alert when the user tries to leave the page (back, forward, or any link on the page).

This seems like a good idea.

The JSTools module had something like this in 4.7 which is no longer maintained. See project page and look for the sub module called Formcheck. Perhaps some ideas can be re-used from that module (though I think it was built pre-jquery...).

For some reason, I couldn't get the modal pop-ups to work on FF 2.0.0.9 on my Mac.
Not sure why.

But it worked in Safari, and I created a screencapture demo to show off for those who haven't applied the patch.
http://blip.tv/file/get/Kentbye_tech-ModalDialogDialogs_patch4txt872.mov

One suggestion might be to have the modal dialog box centered vertically.
I know you were able to do this with the Quick Admin Menus modal box.

A weird behavior happened during the making of this video that I'm not sure is from the new drag-and-drop or if it's from this patch.
I was not able to reproduce it unfortunately.

* Move a block to a new position
* Click "Configure"
* Click "Save Changes"

* Move a block to a new position
* Click "Configure"
* Click "Discard Changes and Continue"
* Click the back button
Note that the dialog box still shows up here.

* Click close
* Click Refresh
Note that the weight column now mysteriously appears here. I couldn't reproduce this behavior though.

Sounds good Tao. Thanks for the update.

Just wanted to point out that I know that Google intercepts the browser's back button -- like when trying to leave the Blogger comment page after you've filled any portion of a form element. Go here, type a few letters into the comment window, and then hit try hitting the back button.

You'll see an alert come up saying:
"Are you sure you want to navigate away from this page?
You have unsaved changes.
Press OK to continue or Cancel to stay on the current page."

For a closer look at what they're doing, set a Firebug break at line 117 of https://www.blogger.com/app/scripts/formcheck.js & go then from there.

I still couldn't get the modal pop-ups to work on FF 2.0.0.9 on my Mac.
But they worked great in Safari 3.0.3

Here's an updated 45-second video:
http://blip.tv/file/get/Kentbye_tech-Modal_Dialog_V2_218.mov

I like the vertical positioning much better, and I see the dialog text is a bit cleaner.

UPDATE: Also, one thing that I noticed is that if I make a weight change, click on the configure button, and then save the form -- then it redirects to the same form page instead of the configure page that I had clicked. If there was some way to redirect to the intended page, then I think that'd make sense.

Also something to consider is that Dries announced today that they are intending on releasing a Drupal6 Beta3 by Wednesday, which doesn't leave much more time... So maybe deciding what the core features and functionality and essential improvements would be good.

This looks like a good improvement, but I'm not entirely convinced it should go in Drupal 6. It might be better to hold this off to Drupal 7, and to do a more extensive modal dialogification of Drupal. While I appreciate the work and effort, I'm tempted to change the version to Drupal 7.

Well, looks like it was too little, too late for this cycle.

But at least this is in better shape for when D7 opens up.

A couple of design comments to consider.
* Possibly center the buttons at the bottom
* Give a little bit more margins & breathing room so that it's more of a square than rectangle.
* Try a single colored background -- like all black white text
* Eliminate the Upper right "close [X]" since it replicates Cancel

The code and framework are obviously important here, but I think the aesthetic is also just as important and should be given plenty of time for lots of feedback and mock-ups.

Looking foward to seeing modal pop-ups integrated into the workflow production of new nodes, menu items, confirming deletes and anywhere else that currently takes 2-3 additional page loads. There is a lot of UI streamlining that could be done with this modal framework. As a example, here again is a demo of starbow's Quick Admin Menu module for adding in new menu items: http://blip.tv/file/get/Kentbye_tech-QuickAdminMenuDemo680.mov

subscribing.

Sorry - the above link ( http://drupal.org/projects/popups ) doesn't seem to work ...

I think the correct link is (singular "project"):

http://drupal.org/project/popups

I'm curious if anyone has tackled performance of the modal popups? I'm not sure if my experience with them is unique to my environment (OpenSuSE 10.2, Intel 2.0 ghz dual core, 4 gig of memory, SATA 7200 RPM disks, FireFox 2.0.0.10) - but I've encountered nothing but HORRIFYING performance with modal popups. Often freezing my machine up for 5 to 20 seconds while the page tries to load. This is also the case with Drupal RC3 and the 6.x-1.0-beta2 version of the popups module.

I should also mention my server is a dedicated system for this, which is a dual Quad Core Intel XEON 3.0 Ghz with 16 Gig of memory, a RAID 5 hardware array with SAS 15k rpm 3.0gbps disks, and the network between my client and server are gigabit ethernet links. I'm pretty certain my client and server aren't bottlenecked in any way.

@starbow: I sent you private message via Drupal Contact - I have a firebug session output from the connection. It takes 3.04 seconds to do a very very simple modal popup page. Let me know where to send that information?

starbow: Why wouldn't you just pass a JSON object? PHP 5.2 has json_encode and json_decode built in and JS can evaluate JSON much faster than XML.

I can't believe I haven't subscribed to this yet :P

Tao and I kicked around ideas all DrupalCon. I'm hoping to hop in on this development also.

Wow, I can't believe I didn't notice this issue yet either given how my clients seem to ask for modal popups of content about every 3rd page. :-)

I have not tried the patch yet, but I am curious. jQuery has a very nice popup tool called jqModal that is very small but extremely flexible. I've used it on a series of Drupal 5 sites (with jqUpdate) and the entire code to generate a popup, besides jqModal itself, is frequently under 10 lines of JS. Why not just leverage that plugin, and offer module developers a size-efficient tool for doing all sorts of extra fanciness?

Also, how is putting HTML pages in popups a security hole? To be fair, when I've had to do that I've used an iframe, but HTML fragments in a popup are also a very common task, say for image galleries or help text.

And yeah, subscribing. :-)

Subscribing + trying the patch. This is great work!

Subscribing...

The icon used for the "close" button (red circle with a cross) is also used on the module page where it means "incompatible" and on the watchdog logs where it means "error", which is different from the meaning that we want here.

Here's the patch from #42 updated to apply to the current HEAD, plus an image path to a new image popup-close.png (should reside in /misc) created by etmaguire for that purpose. This image can be licensed under the GPL.

Tried applying this but got the error:

patch unexpectedly ends in middle of line
patch: **** malformed patch at line 1890:

This one applies at least. Haven 't tested it yet.

I manually deleted some files from the output of 'cvs diff' and didn't leave a new line at the end of the patch file. Eclipse doesn't complain but the command-line cvs client does. Here's the same patch with a new line character that works from the CLI.

edit: this is a cross-post with webchick's comment, the two patches are probably similar.

I applied the patch to HEAD, but the minute I enable the module, I get the following PHP Error:

Fatal error: Cannot redeclare theme_popup_save_dialog() (previously declared in /var/www/drupal_head/includes/theme.inc:1622) in /var/www/drupal_head/modules/popuppage/popuppage.module on line 104

webchick suggested I add this patch. I removed two duplicate function declarations so at least there is no fatal error. Still doesn't seem to do anything. Also I get these notices on the modules page:

notice: Undefined index: arguments in /Users/nini/Sites/head/includes/theme.inc on line 296.
notice: Undefined index: arguments in /Users/nini/Sites/head/includes/theme.inc on line 296.

I'm leaving this as 'code needs work' since the change doesn't seem to actually make the code work yet, it just prevents fatal errors.

Also, in Firefox 3 on Mac, this patch appears to mess up the sticky table headers.

This was my bad, I updated the patch from #42 and only tested the part of it that goes into the system, and didn't test the popuppage module. It turns out that it is this module that caused the error mentioned above.

I fixed the functions causing these errors and reapplied the little usability change for the "close popup" icon. Applies to HEAD cleanly and doesn't cause php errors, but the separation between what goes directly into the system and what goes in the popuppage module is not totally clean and needs more work.

I have been working on this issue all day off and on, and I'm obviously not the only one who has spent a lot of time reviewing and rewriting messy patches. This shows two things:

1. There is a lot of interest in the usability improvement that this patch can provide.
2. This patch is too big to be properly handled as one big change.

What I strongly suggest is decomposing this patch into smaller steps that can be applied one by one. The most obvious one is the popuppage module which could be left out for now and added at a later date.

Patch applies cleanly using the command-line cvs client on linux, no issue with linefeeds. The popups work fine, but I would wait until #218830: Popups in Drupal 7: Plugable renderers for generating content is fixed to do an in-depth review of this patch (I'll also add the new "close" icon then).

tracking

I'm not sure if it's a bug or something that just isn't done, but when I initiate a popup by clicking 'more help' or 'more information about formatting options', the page turns dark and the cursor changes to a 'loading' icon, however I don't see any popup.

Are there any dependencies for this patch?

Could someone please make a screencast and some screenshots of this please? It would invite more people to review and to verify the behaviour which should be expected.

If I could do it myself I would, but unfortunatly I'm working on a Mac from around... say... prehistorical era, so no screencasting for me..

Stefan

See #5 for screencast.

I'd be happy to make a screencast but I'm unable to start until this weekend. If that's a reasonable time frame, please let me know.

@starbow, I was using Firefox 2 on Ubuntu, I'll check for JS errors the next time I get a chance.

I'm reviewing this at the moment.. Till now it works as expected on Mac Safari 2/3, FF's and Opera..

@starbow: This is looking good, but could you reroll without the other patch? As it is it's difficult to review.

This is #55 without the renderers patch (http://drupal.org/node/218830) rolled into it, to which I'm proposing a slight different approach.

Okay, as I said yesterday I did a review of the patch. Here we go:

I still don't like the name popup/popups. You are using the two "popup" and "popups" in the code instead of picking one default. Can we please call this something like "dialog", "modal" or even something else?
Inside your code comments you are also using "modal"/"modals" and "dialog"/"dialogs", which is really inconsistent.

popup-template.tpl.php:
Just make that popup.tpl.php (I still prefer modal.tpl.php or dialog.tpl.php). We don't have node-template.tpl.php either, and If I remember right the "tpl"-part in our theme files stands for "template".

What is the difference between class="popup" and class="popup-form" ? Why are they seperated? Can't we have one class for both modals?

When you remove a popup/modal/dialog you fade it out (fast). But when a popup/modal/dialog comes in, it suddenly is there. Could fade that in also (fast!)?

In the popup-template.tpl.php you do:
+

print base_path() misc/watchdog-error.png" />

That image is so ugly, please remove that. And simply make a text link on the top-right to close the popup/modal/dialog.

In system.css:
- try to follow coding standards please, as you are using a lot of tabs instead of double spaces for indetation;
- try to keep the standard styling as minimal as possible please. Fancy shit needs to be done in themes, not in standard module/api CSS files. If you want to, feel free to contact me for having it integrated/themed in Garland. I would love to do that;
- You are using *a lot* of "weird" colors for the popup-thing, please just use the colors we use by default in drupal and or plain black/white. Again, fancy things are for themes to handle;
- follow coding guidelines: + float:left => float: left;
- + line-height: 0pt => line-height: 0; No need for units, as '0' still stays zero, no matter what unit we use;
- +#popup input[type="text"]:focus, #popup input[type="password"]:focus, #popup textarea:focus. Why are you styling those? they are not styled at all in Drupal core, so please keep things consistent;
- +#popup div.messages Why are you overriding the messages that appear in a popup? Leave them to themers;
- +a.popup-processed:after The :after pseudo-element doesn't work in IE. Fix that by positioning it with jQuery, or remove it completely (I think it's useless anyway as the code inside it is commented out);

In system.admin.inc:
- the settings really need to go (as the comment in the file already says so). The jQuery selectors which should be filled in are way to technical for normal (end) users to understand;

I'm no jQuery nor JavaScript expert, but:
+ $popup.css('left', -9999);
doesn't seem right without a unit. This is probably causing FF to not display properly. Use:
+ $popup.css('left', -9999em); or something like that.

+ $popup.css('top', top).css('left', left); // Position the popup to be visible.
What does this do? Not sure about this one, but shouldn't that be something like:
+ $popup.css('top', 0).css('left', 0); // Position the popup to be visible.

Hope you appreciate my review, and I'm really excited about this patch. Drupal feels a lot faster with the right usage and implementation of modal windows (or whatever you want to call it).

I'm looking forward as a themer to implement this into Garland and as a coder to review any more upcoming patches!

Stefan

Re: my previous problem - the latest patch has solved it.

Suggestions:
* The close icon should be swapped for the icon in #52, at least for now.
* Why is there an empty <a> around the close icon? It should also trigger the pointer cursor.
* The close icon should have a title attribute - title="<?php t('Close') ?>"
* A lot of the current CSS can be removed, for example the text colour is set to black even though Garland uses gray.
* Ditto Stefan regarding the wording, however all three names have distinct meanings...

Modal: Forces the user to choose an action
Dialog: Asks the user to enter information
Popup: A window that appears on top of the current window

Maybe there should be a distinction between modal, dialog and popup - for example a different appearance. Modals and dialogs could be yellow, while Popups could be blue.

Bugs (Firefox 2.0.0.13 - Ubuntu):
* Firebug reports a 404 for the destination page when a modal link is clicked, e.g. 'tips' or 'block'
* The path to ajax-loader.gif has two forward slashes before 'misc'
* There's a short delay between clicking the close icon and the modal actually closing, I didn't notice any fading at all (which would probably look choppy anway).

@starbow: popup vs modal vs dialog: then dialog gets my vote. Popups have a very negative reputation, and I think dialogs are a better reflection to what you are trying to achieve here.

Even though the word popup is mostly associated with advertisements, I think it's the only term that describes this patch correctly. As I said in my last comment, each term has a distinct meaning and should be used only where appropriate.

If the popup forces the user to click a button then it's called a modal. If the popup presents a textfield then it's called a dialog. If it's just displaying informational text then it should be called a popup, because it's not a modal or dialog. So in my opinion, regardless of the 'reputation' of the word, everything should be referenced as a popup.

I've applied the patch in #55 and in Firefox 2 and 3 B5, Safari 3 on OSX and get no popup behavior on the "more help" links nor the expected "Warning: please confirm" popups after re-arranging the blocks layout and attempting to follow a link away from admin/build/block without saving my changes.

With #69 , hunk 1 fails . I re-rolled the patch after manually making the one-liner change to theme.inc but my patch was 20kb, 16kb smaller than Quicksketche's. I tried using the -p0N flag as well as doing a 'fake' cvs add to add the new template and popups.js file but to no avail.

After applying patch #69 and the corresponding patch at http://drupal.org/node/218830 , I get the expected popup on "more help" links but none of the expected "Warning: please confirm" dialogs. On first glance, it looks like we're not attaching to non popup links, but I could be missing something.

In any case, should make a demo screencast of the help text popups or wait to clarify these issues?

Subscribing...

@starbow: I am very excited and really looking forward to review your next patch! Feel free to e-mail me (usng the contact form) once there is a new patch to have a look over..

Stefan

subscribing

Subscribing....

subscribing

Being nit-picky:

+    'popup' => array(
+      'template' => 'popup',
+    ),

should be all on one line.

<?
/**
* Implements hook_theme_modes.
*/
?>
Should be "Implementation of hook_theme_modes().", same with system_after_process_form.

+    case 'page': // render('page', $content);
+      return drupal_json(array(
+        'status' => 'ok',
+        'title' => drupal_get_title(), // TODO: filter.

Looks like some commented out code and TODOs...

+      print drupal_json(array(
+        'status' => 'redirect',
+        'path' => $url,
+        'form_state' => $form_state,
+      ));

I'm not sure if printing $form_state is secure....

-        $description = '<div class="module-help">'. drupal_render($form['help'][$key]) .'</div>';
+        $description = '<div class="module-help">' . drupal_render($form['help'][$key]) . '</div>';

Please keep patches focused, don't stuff other things (like code style fixes) in.

+  if (!isset($goto) || ($goto !== FALSE)) { 
+    // If $do_goto is set, call drupal_goto and do the redirect & exit.        
+    if ($do_goto) {
+      if (isset($goto)) {
+        if (is_array($goto)) {
+          call_user_func_array('drupal_goto', $goto);
+        }
+        else {
+          drupal_goto($goto);
+        }
       }
-      else {
-        drupal_goto($goto);
+      drupal_goto($_GET['q']);

This looks odd. You're checking if $goto is not set and then checking if it is set.

+function theme_render() {  

Should this start with a _?

+  return '<div class="more-help-link">' . t('[<a class="popup more-help" href="@link">more help...</a>]', array('@link' => check_url($url))) . '</div>';

Can't we keep the text & formatting the same?

+  $request_mode = isset($_SERVER['HTTP_X_REQUESTED_WITH']) ? $_SERVER['HTTP_X_REQUESTED_WITH'] : 'standard';  

Lots of trailing whitspace.

+// TODO - update comments to reflect breaking drupal_goto into two functions.
+function drupal_get_goto_url($path = '', $query = NULL, $fragment = NULL) {

So why don't you update them?

+          var buttons = {
+           'popup_save': {title: json.buttons.save, func: function(){Drupal.popup.savePage(a, options);}},
+           'popup_submit': {title: json.buttons.submit, func: function(){Drupal.popup.removePopup(); Drupal.popup.openPath(a, options);}},
+           'popup_cancel': {title: json.buttons.cancel, func: Drupal.popup.close}
+          };

You need a couple more spaces. function() {

+      var pageIsDirty = $('span.tabledrag-changed').size() > 0;
+      var willModifyOriginal = options.updatePage;
+      if( pageIsDirty && willModifyOriginal ) {

Hmm, I don't like the hardcoding of pageIsDirty. I also don't like willModifyOriginal. If you're using a variable only once, in an "if" statement, you might as well jsut put whatever you are using. Also the spacing on the if is wrong.

+  var $popup = $(Drupal.theme('popupDialog', title, body, buttons));

This is good, like to see use of Drupal.theme

+  $('body').append( $popup ); // Add the popup to the DOM.

No spaces before the ) and after (

+  if (popupHeight > (0.9 * windowHeight) ) { // Must fit in 90% of window.

No space before )

+  Drupal.popup.open( message, body, buttons );

same thing.

In summary JSLint is not always right. Sorry

+/*****************************************************************************
+ * Appearence Functions (overlay, loading graphic, remove popup)     *********
+ *****************************************************************************/

We don't need that. Thanks

+//Drupal.popup.getOverlay = function() {
+//  return $('#popup-overlay');
+//};

If it's commented out, it shouldn't be there.

+//  return $overlay;

This is a few lines above, but same deal.

+/****************************************************************************
+ * Theme Functions   ********************************************************
+ ****************************************************************************/

see above.

Extra line at end of modules/system/popup.tpl.php

Those are all the comments I could find about the code.

ahem.

@starbow et al - I sincerely apologize for my lack of response here. Somehow my drupal.org subscriptions were all set to "none" instead of "own issues." I would have happily produced a screencast for this issue. Given the lack of agreement about how to get Drupal to properly output JSON and the fact that there are multiple issues with proposals, what's the best way to proceed towards a consensus?

I closed out my approach to pluggable rendering formats, marked as duplicate. We can reroll this patch to work with #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.). Thanks for pointing that out Tao.

I'm planning to rework the rendering here instead:

http://drupal.org/node/134478

Subscribing

Looking at this again, I think this patch could be broken up.

First, we need a way to load a form via AHAH. Not all forms are in the main content area of a form (e.g., forms in blocks), and even if they are that main content area may contain more than the form. We need a method for loading just the form, regardless of where it occurs on a page.

Second, we need the ability to validate and submit forms via AHAH--completely apart from any UI considerations. Such a patch could draw on this existing patch as well as the ajaxsubmit module, http://drupal.org/project/ajaxsubmit and other sources.

Third, I think we'd do best to use the jQuery UI dialogs rather than inventing our own. When this patch began jQuery UI was unstable. It's a lot better now. If we're introducing a dialog library to core, we need a standard, flexible dialog rather than a custom one tied to a particular UI and application.

Have you noticed the Floating Windows module ? This module supports inline form handling. It is not yet themeable, however this could be accomplished rather easily. Maybe it's just good for ideas....

FWIW, I'd be excited to see this functionality in core. Splitting the patch up along nedjo's lines feels like a pretty good way to break the stalemate.

I can't say for sure (code freeze related stuff is really Dries's call), but if I had to take a wild guess, I'd say that will probably be what happens (UI-related changes are okay for a certain period of time after code freeze).

It sounds sensible to work on those other issues first, marking postponed for now. I'd especially love to have your input on the jQuery plug-in manager issue since you're one of our foremost JS experts. :)

Subscribing. (Will be back soon.)