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.
Comment | File | Size | Author |
---|---|---|---|
#43 | popups_revamp.patch | 19.39 KB | RobLoach |
#40 | popups.patch | 24.47 KB | RobLoach |
#35 | popbox-nb.patch | 22.49 KB | starbow |
#33 | popbox-nb.patch | 23.38 KB | starbow |
#28 | popbox2.patch | 24.79 KB | starbow |
Comments
Comment #2
starbow CreditAttribution: starbow commentedWeird that testbot is reporting a PHP syntax issue in a css file. Dmitri suggested I rearrange the patch and see what the bot thinks.
Comment #4
starbow CreditAttribution: starbow commentedOk, another try after following the instruction over at http://drupal.org/node/75242 to get correct eclipse's line returns.
Comment #5
kika CreditAttribution: kika commentedAny screegrabs to share?
Comment #6
starbow CreditAttribution: starbow commentedI 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.
Comment #7
quicksketchSubscribe.
Comment #8
skilip CreditAttribution: skilip commentedCould you please post some screenshots?
Comment #9
starbow CreditAttribution: starbow commentedComment #10
starbow CreditAttribution: starbow commentedComment #11
xmacinfoLooks promising.
As for the name, since you'll targeting core feature for Drupal 7, I would suggest using "modal", "popmodal" or "modalbox".
Comment #12
starbow CreditAttribution: starbow commented@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.
Comment #13
RobLoachSubscribing.
Comment #14
starbow CreditAttribution: starbow commentedAdding 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.
Comment #15
ChrisBryant CreditAttribution: ChrisBryant commentedThanks Tao, this is great and will improve usability in Drupal. Crossing my fingers that this makes it in!
Comment #16
babbage CreditAttribution: babbage commentedWill be able to give this a careful review in a few days time, presuming it isn't already done by then. :)
Comment #17
yoroy CreditAttribution: yoroy commentedand another tag
Comment #18
Dries CreditAttribution: Dries commentedI support this -- it belongs in core, and it will help improve some usability issues. Haven't reviewed the code yet.
Comment #19
RobLoachSomething 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.
Comment #20
starbow CreditAttribution: starbow commented@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.
Comment #21
starbow CreditAttribution: starbow commentedD'oh. There was supposed to be a patch attached to #14.
Comment #22
starbow CreditAttribution: starbow commentedOk, 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.
Comment #23
starbow CreditAttribution: starbow commentedOk, 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.
Comment #24
starbow CreditAttribution: starbow commentedInstructions 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.
Comment #25
RobLoachNicely done, a couple of things:
drupal_add_js('popups', 'plugin')
.Great work! I'd love to see this progress.
Comment #26
starbow CreditAttribution: starbow commentedThanks
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.
Comment #27
xmacinfoCoder catches white space errors.
I'm eager to see what will come out with you patch. It will be a nice addition to Drupal.
Comment #28
starbow CreditAttribution: starbow commentedHere 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.
Comment #29
alexanderpas CreditAttribution: alexanderpas commentedare we aware of IE6 ;)
Comment #30
starbow CreditAttribution: starbow commentedIE6...I've definitely heard of it. Didn't some people use that as a browser back in the 90's or something?
Comment #31
alexanderpas CreditAttribution: alexanderpas commented;) 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/
Comment #32
starbow CreditAttribution: starbow commentedBut seriously folks, this has been tested on IE6 and works fine. If have found bugs in your IE6 testing, please report them.
Comment #33
starbow CreditAttribution: starbow commentedThis is just a test of NetBeans 6.5 patch creations. Please ignore.
Comment #35
starbow CreditAttribution: starbow commentedLet's see if a run through dos2unix fixes things.
Comment #36
starbow CreditAttribution: starbow commentedComment #38
bsherwood CreditAttribution: bsherwood commentedPeople 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
Comment #39
starbow CreditAttribution: starbow commentedMy experiments with NetBeans did not work out. The patch at #28 is still the active one.
Comment #40
RobLoachThis 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?
Comment #41
starbow CreditAttribution: starbow commented@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?
Comment #42
RobLoachThe 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 todrupal_add_popbox()
. Any thoughts?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.
Comment #43
RobLoachI 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.Comment #45
RobLoach#444344: jQuery .once() method for adding behaviors once will help a lot.
Comment #46
frankcarey CreditAttribution: frankcarey commentedsubscribe
Comment #47
andypostsubscribe
Comment #48
babbage CreditAttribution: babbage commentedI'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.
Comment #49
babbage CreditAttribution: babbage commentedSee 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.
Comment #50
starbow CreditAttribution: starbow commentedYeah, 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.
Comment #51
joshmillerbased on starbow's comment above...
Comment #52
babbage CreditAttribution: babbage commentedReplaced by #87994: Quit clobbering people's work when they click the filter tips link.
Comment #53
murtza CreditAttribution: murtza commentedpopup_lite.patch queued for re-testing.
Comment #54
xmacinfoThis 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.
Comment #55
shartz CreditAttribution: shartz commentedis it done??? Can I use the popups for Drupal 7 now?
Comment #56
shartz CreditAttribution: shartz commentedpopup_lite.patch queued for re-testing.
Comment #57
cweagans@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.
Comment #58
xmacinfoComment #58.0
Damien Tournoud CreditAttribution: Damien Tournoud commentedLocking old issue. https://drupal.org/node/1242530