This patch is a rethinking of #193311: Ajax Popups in Drupal 7: Adding Modal Dialogs to Help, Confirmations and Filter tips (Unified). This time I am attempting to do only what is described in the title - to add modal dialogs to confirmation pages and popup for filter tips and help. The code is a stripped down version of my Drupal 6 Popups API module, but is much less ambitious than that module. While the Popups API attempts to solve the general problem of ajax forms, this patch skirts the issue entirely. Most importantly, this patch works today, as is, without requiring any changes to the Drupal 7 rendering pipeline, or the inclusion of new jQuery libraries.

What this patch provides:
* A simple technique for putting info pages into modal popups, and confirmation pages into modal dialogs.
** Add "drupal_add_popup()" call to page, and set class="popup" for info pages and class="popup-confirm" for confirmation pages.
* Confirmations dialogs on delete of Blocks, Menus, Content Types, URL aliases and Text formats.
* Modal popups for "More help" links, and "More information about formatting options"
* Progressive enhancement. Everything works as normal if javascript is turned off.
* A working solution with a simple logic flow that can be understood by non-ajax experts.
* A themable dialog or message box. The xhtml can be crafted using the standard Drupal theming system. The look and feel can be adjusted in a theme's css files.
* A function, Drupal.popup.showPopup(title, body, buttons, width), for javascript functions to call to create a message box in preference of "alert".
* A starting point.

Limitations of this patch:
* It does not attempt handle general ajax form submission. This is a fascinating problem. The d6 Popups API has some of it solved. Nedjo and quicksketch have proposals in place that should solve other parts. I have a couple of half worked out patches. It is a big issue and might not get solved for Drupal 7.
* It does not use json. Which means that the full page is rendered, sent over the wire, and then all but the needed parts are thrown away. This is inefficient, but can be improved when #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.) lands.
* It does not warn you if are about to lose your work. The older version of this patch could offer to save your work for you before opening up a destructive popup.
* It does not call attachBehaviors on the ajax loaded content in the dialog box.
* It does not do ajax reloading of the current page. Instead it reload the entire page. Again, this is worth revisting when #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.) lands.
* It only works as-is with the Stark theme. It requires the id="page-title" for the page title, and id="content-content" for the page's content, which is the semantics declared in modules/system/page.tpl.php. I will be following up with patches to bring the other core themes into compliance.
* The default popup is not particularly attractive. I was advised on the last iteration of this patch to leave the styling to the themes. However, I found the uptake of the d6 Popups module increased substantially when I made the default look sexier, so I am conflicted.
* It does not solve #114546: Confirmation alerts done with JS/AJAX (delete confirmation page and any others). This patch only works on links, not submit buttons. However I believe this would get us 80% of the way.
* I have been keeping half and eye on #299050: Help System - Master Patch, and, ironically, it looks like there is an emerging consensus to use the term "popup" to mean an old school popup window. In that case I will need to rename the functionality in this patch. There has already been a bunch of bikeshedding on this topic, but so it goes. I'd use "lightbox", but that might not be nice for the people maintaining the existing lightbox modules. So I am leaning towards a completely new term, like "ajaxbox".
* This patch does not use jQuery UI, again in an effort to avoid controversy. Interestingly Rob Loach has proposed creating a jQueryUI skin for the d6 module.

Note: The two image files go in the misc folder.
Note2: In case you missed it above, you will need to test with the stark theme.

Files: 
CommentFileSizeAuthor
#43 popups_revamp.patch19.39 KBRobLoach
Failed: 10876 passes, 13 fails, 0 exceptions
[ View ]
#40 popups.patch24.47 KBRobLoach
Passed: 10889 passes, 0 fails, 0 exceptions
[ View ]
#35 popbox-nb.patch22.49 KBstarbow
Failed: Failed to apply patch.
[ View ]
#33 popbox-nb.patch23.38 KBstarbow
Failed: Failed to apply patch.
[ View ]
#28 popbox2.patch24.79 KBstarbow
Failed: Failed to apply patch.
[ View ]
#28 popbox-error.png16.93 KBstarbow
#25 popupsdrupalhead.png92.15 KBRobLoach
#23 popbox2.patch24.07 KBstarbow
Failed: Failed to apply patch.
[ View ]
#22 popbox2.patch23.91 KBstarbow
Failed: Failed to apply patch.
[ View ]
#22 popbox-icon.png400 bytesstarbow
#21 popbox.patch24.63 KBstarbow
Failed: Failed to apply patch.
[ View ]
#9 popbox-block.png76.79 KBstarbow
#9 popbox-text-format-tips.png87.26 KBstarbow
#6 popbox.patch22.41 KBstarbow
Failed: Failed to apply patch.
[ View ]
#4 popup_lite.patch23.08 KBstarbow
Failed: Failed to apply patch.
[ View ]
#2 popup_lite.patch23.75 KBstarbow
Failed: Invalid PHP syntax in modules/system/system.admin.inc .
[ View ]
popup.png400 bytesstarbow
ajax-loader.gif8.04 KBstarbow
popup_lite.patch23.75 KBstarbow
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in popup_lite.patch.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new23.75 KB
Failed: Invalid PHP syntax in modules/system/system.admin.inc .
[ View ]

Weird that testbot is reporting a PHP syntax issue in a css file. Dmitri suggested I rearrange the patch and see what the bot thinks.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new23.08 KB
Failed: Failed to apply patch.
[ View ]

Ok, another try after following the instruction over at http://drupal.org/node/75242 to get correct eclipse's line returns.

Any screegrabs to share?

Title:Popups Lite: Adding Modal Dialogs to Confirmations, Filter tips and HelpPopbox (Popups Lite): Adding Modal Dialogs to Confirmations
StatusFileSize
new22.41 KB
Failed: Failed to apply patch.
[ View ]

I spent some time looking that the big help system patch #299050: Help System - Master Patch, and it looks like the have the help system under control. Right now it looks like they are going with old school popup windows, which work fine for their use case. But I think I do need to change the name of the modal system I am proposing. I am going with Popbox, a combination of popups (the old name) and lightbox (the best known term for the ajax based approach).

This new patch has the name change, and removes the popbox behavior from the help links.

Subscribe.

Could you please post some screenshots?

StatusFileSize
new87.26 KB
new76.79 KB

Issue tags:+JavaScript

Looks promising.

As for the name, since you'll targeting core feature for Drupal 7, I would suggest using "modal", "popmodal" or "modalbox".

@xmacinfo: if we get to the point where someone tells me "This patch is ready to set to RTBC, but we need to change the name", then I will be happy to reopen the naming discussion. Until then, I'd rather focus on functionality and code style.

Subscribing.

Adding code to handle node delete, as requested in #114546: Confirmation alerts done with JS/AJAX (delete confirmation page and any others). Slightly more complicated logic needed to handle a submit button vs a simple link, and the fact that there are multiple submit buttons and we only want to popbox behavior on one, but not too bad.

I would love to get some people other than me testing.

Issue tags:+Usability, +modal dialog

Thanks Tao, this is great and will improve usability in Drupal. Crossing my fingers that this makes it in!

Will be able to give this a careful review in a few days time, presuming it isn't already done by then. :)

Issue tags:+ui-pattern

and another tag

I support this -- it belongs in core, and it will help improve some usability issues. Haven't reviewed the code yet.

Issue tags:+Patch Spotlight

Something we should consider in this patch is skinning the popups. Although we could apply a different template in the theme layer, the popups should have the ability to change skins no matter what Drupal theme we're using. This might belong in a later patch though, due to the kittens.

Note: #372378: Popups Skins - Facebook looking popups.

@Dries (and anyone who has opinions on this issue) - last year in boston when I showed you the code I had then, I had the html of the basic popup being defined in the popups.js, inside a Drupal.theme.prototype call. You suggested that themer did not understand how to use the javascript Drupal.theme call, and I should move it into a tpl.php file. I did this and it mostly works but it lead some very subtle bugs. Now popups_add_popups (or drupal_add_popbox in this patch) calls theme(). And the D6 version of the module calls popups_add_popups in hook_init. And calling theme() a hook_init function causes the blocks admin pages to blow up. Like I said subtile.

Also, I have learned in the last year that people like the retheme the "loading" layer as well as the popup itself, and they have all figured out Drupal.theme (see the patch Rob mentions).

So, I am inclined to move the popbox.tpl.php into popbox.js as Drupal.theme.prototype.popbox, unless you still have strong objections.

Status:Needs review» Needs work
StatusFileSize
new24.63 KB
Failed: Failed to apply patch.
[ View ]

D'oh. There was supposed to be a patch attached to #14.

Status:Needs work» Needs review
StatusFileSize
new400 bytes
new23.91 KB
Failed: Failed to apply patch.
[ View ]

Ok, here is an improved version of the last patch, with some foward ports of improvements to popups api for d6, and fair amount of head chasing.

# Easier skinning of loading image (from http://drupal.org/node/364712)
# :input on refocus (from http://drupal.org/node/385732).

Also, here is the renamed icon image to put in the /misc folder.

StatusFileSize
new24.07 KB
Failed: Failed to apply patch.
[ View ]

Ok, here is a patch that removes popups-popbox.tpl.php, moving the theming to a Drupal.theme.prototype.popboxTemplate function in popbox.js.
Just to review my logic of why I think this is better.
* It is simpler and less buggy.
* We have a system for theming javascript, we should use it and/or improve it.
* Most people that want to them the popbox dialog can do what they want with css
* Those who want effect elaborate enough to need to modify the html, usually also want to change the loading image, so they end up having to use the javascript theming approach anyway.

To make it easier for themers to change the html of the dialog and loading image, I have added a feature to check if there is a popbox.js file in the current theme, and if so, include it in the page automatically. I have a demonstration of how this works over at #387156: Update garland to support popbox, which is a companion patch to this one.

Instructions for testing:
* Copy popbox-icon.png (from #22) and ajax-loader.gif (from the initial patch) into /misc.
* Switch to the Stark theme (unless you are also testing #387156: Update garland to support popbox)
* Create a new block, url alias, text filter or menu item.
** The delete link to your newly created item should trigger a popbox confirmation dialog.
* Create a new page
** The "More information about formatting options" link in the Text format field set should trigger a popbox.
* After you create the new page, and then edit it
** The "Delete" button should trigger a popbox confirmation dialog.

StatusFileSize
new92.15 KB

Nicely done, a couple of things:

  1. The popups should work with or without the popup support included in the theme. If someone makes a new Drupal theme, they'll discover that they're popups read "null" and "null" (see attached screenshot) and be pretty upset. Keeping that in mind for the future is good (#387156: Update garland to support popbox), but I think popups should work with or without the theme support. Popups shouldn't depend on the theme, they should "just work" (tm).
  2. This might be some bike shed discussion, but "Popbox" is less descriptive then "Popup". I understand that "Popup" is being used for the help patch, but that kind of popup is quite different than this popup. That's a full window popup, while this is an embedded popup. I think we should just keep it as "popup", as both kind of mean the same thing and could be used in parellel (i.e. make the help system use these popups instead of the full window).
  3. Check your whitespace ;-)
  4. What are your plans for Popups API in Drupal 7 once this is in? What hooks are introduced in this patch that will help the module and add popup support around the whole Drupal administration section? Although it's good to add the "popups-confirm" class to register the popups, how could these be added by a contrib module? hook_form_alter?
  5. I believe #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) might help a bit as you're adding multiple JavaScript and CSS files in one function. If the manager was in HEAD, the call would end up being drupal_add_js('popups', 'plugin').

Great work! I'd love to see this progress.

Thanks
1) The theme does not need to include a popbox.js file, but it does need to use the correct id's for the page title and page content (page-title and content-area). If you go to the status page, it will pop up a box and explain it to you, but I will improve the error message on the page you show.
2) I am still where I was at #12. Also using a different term helps me shift contexts when I jump from one code base to another, especially now that they have diverged so much.
3) Thanks for the tip. Does coder catch whitespace errors?
4) I haven't got a clear stratagy yet for Popup API in D7 yet. I am really hoping this is just the first popbox patch, and we can get more of the popup API's power in to D7.
5) I am keeping an eye on that patch. Seems like it is still a ways from solidifying.

Coder catches white space errors.

I'm eager to see what will come out with you patch. It will be a nice addition to Drupal.

StatusFileSize
new16.93 KB
new24.79 KB
Failed: Failed to apply patch.
[ View ]

Here is the patch with an improved error message. I am attaching a screenshot.
I also found and removed some extra whitespace I had in common.inc, but coder doesn't exist for D7 yet, so any tips on tracking down bogus whitespace is appreciated.

are we aware of IE6 ;)

IE6...I've definitely heard of it. Didn't some people use that as a browser back in the 90's or something?

;) yep... that one... it is (sadly) still in use...
wikipedia states it still has a marketshare of 20%

and see also: http://webaim.org/projects/screenreadersurvey/

But seriously folks, this has been tested on IE6 and works fine. If have found bugs in your IE6 testing, please report them.

StatusFileSize
new23.38 KB
Failed: Failed to apply patch.
[ View ]

This is just a test of NetBeans 6.5 patch creations. Please ignore.

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new22.49 KB
Failed: Failed to apply patch.
[ View ]

Let's see if a run through dos2unix fixes things.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

People still use IE6? Strange. I stopped using it when Firefox was still called Firebird.

The w3schools website also says IE6 has 18.5% of the market share as of Jan 2009: http://www.w3schools.com/browsers/browsers_stats.asp

Status:Needs work» Needs review

My experiments with NetBeans did not work out. The patch at #28 is still the active one.

Status:Needs review» Needs work
StatusFileSize
new24.47 KB
Passed: 10889 passes, 0 fails, 0 exceptions
[ View ]

This updates the patch to HEAD correctly. Also be sure to watch your whitespace line endings ;-) . We should probably postpone this issue until #299050: Help System - Master Patch is in so that we don't step on each other's feat for usability/help.

I think you should open up a 7.x-1.x-dev of http://drupal.org/project/popups with popups_admin.module to add all the administration links to see how contrib would work with this patch. It would be a great testing ground. I can't have commit rights there, can I?

@Rob Loach Thanks!
Looks like there is now a coder branch for 7, so hopefully that will help me with my whitespace bad habits.
I think your idea of a 7 branch for the popup api module is a good one, but it should wait abit. I am in the middle of a fundamental restructuring in the 2.x branch. Currently, there can only be one popup in the dom at a time. The change will do away with that limitation. I think I will want to forward port that work to this patch, since it opens up all sorts of possibilities.
Giving anyone else commit access makes me nervous, especiaaly about coordinating big changes like this without stepping on each other, but all your contributions so farhave been great, so I am willing to talk about it. Maybe on IRC?

The thing I don't like about this patch is that drupal_add_popbox() takes no arguments. In my eyes, we should be adding Drupal.settings.popbox to the JavaScript so that popups arn't limited to .popbox-confirm. Instead of changing the class the link we want to popupify, it would be cool to pass the jQuery selector straight to drupal_add_popbox(). Any thoughts?

<?php
function drupal_add_popbox($selector = '.popbox-confirm', $additional_options = array()) {
 
$settings = array('popbox' => array(
   
$selector => $options,
  ));
 
drupal_add_js($settings, 'setting');
 
// ... add other JavaScript.
}
?>

This way, the popup markup doesn't have to be in the HTML, while the functionality is still there, and there is no reason to have to modify the markup in order to add popups.

Status:Needs work» Needs review
StatusFileSize
new19.39 KB
Failed: 10876 passes, 13 fails, 0 exceptions
[ View ]

I did a bunch of refactoring. The most prominent one is that drupal_add_popbox() now expects the jQuery selector of what item to popupify. Whether it be an input button, or link, it will handle it appropriately. Also updated to HEAD, moved the CSS to a popbox.css, cleaned up the source, etc.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Postponed

subscribe

subscribe

Status:Postponed» Active

I'm don't think this should remain postponed... at this point we risk being postponed to D8. The new issue for the Help system popups states its use case is so different from here that it requires a separate issue (#401460: Window browser (popups) for Help system) which suggests this one should not be waiting on that one either. Meanwhile, it seems to me that while a jQuery .once function (#444344: jQuery .once() method for adding behaviors once) might make this patch easier, that is really no reason to delay this since it is not actually a requirement and again it could lead to this patch being postponed for a long time. I really want to see at least basic modal dialog functionality in D7. Marking as active... let's get an initial patch committed.

See also http://drupal.org/node/87994#comment-1782994 for a patch that appears to provide a jQuery UI-based alternative patch to implement the basic popups suggested in this current issue.

Yeah, it seems to me, based on the work that Gabor has been doing on Drupal 7 usability and the recent inclusion of jQuery UI, that this patch is pretty much dead and the conversation has moved on to other approaches. C'est la vie.

Status:Active» Closed (won't fix)

based on starbow's comment above...

Status:Closed (won't fix)» Closed (duplicate)

Status:Closed (duplicate)» Needs review

popup_lite.patch queued for re-testing.

Status:Needs review» Closed (duplicate)

This patch is way to old to test as is. Also, another solution have been implement.

If you feel that this patch needs to have another look, please open a new issue and target Drupal 8.

is it done??? Can I use the popups for Drupal 7 now?

Status:Closed (duplicate)» Needs review

popup_lite.patch queued for re-testing.

@shartz, this issue is closed. Please read the previous comments that explain that this issue was closed in favor of #87994: Quit clobbering people's work when they click the filter tips link.

Status:Needs review» Closed (duplicate)