This patch solves the usability issue on the blocks page created when a user has made changes and then clicks a link such as "configure" and loses all of their work. It does so by introducing the infrastructure for modal dialogs, and creating an Unsaved dialog that can be applied to links on a page with unsaved changes. The Unsaved dialog offers the user the opportunity to save their work, to proceed and lose their changes, or to cancel. This technique could be applied to many situations in the administration pages, but I am starting with Blocks as a solid use case, and also because I am so inspired by Nate/quicksketch's amazing "drag and drop for blocks" patch.

This patch also uses the new dialogs infrastructure to create a View-page-in-popup dialog. This patch demonstrates this popup on the blocks page's configure links. The popup effect is requested by adding a 'popup' => true option to l(). Again, this technique could be applied many places on various administration pages. It would be particularly powerful in places like the menu page's delete item links.

This approach of adding popup dialog behavior to links degrades gracefully to just opening the link as normal when javascript is not present.

I have tested this patch against the windows XP versions of:
Firefox 2.0.0.9
Safari 3.0.3
IE 7.0.5730

Comments

dmitrig01’s picture

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

dmitrig01’s picture

Status: Needs review » Needs work
quicksketch’s picture

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

greggles’s picture

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

KentBye’s picture

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.

starbow’s picture

Thanks so the feedback! I will start working on a new patch tomorrow.

Dimitri: I am using eclipse to roll my patches, and I don't think it has a way to set -p. I will look into getting another tool, but it might take a bit. I will definitely track down and kill the tabs. And you are right about the admin_get function needing some more thought - maybe /raw as the path. I think it has other uses that just the dialog - I would have killed for a function like it when I was working on Ahah stuff.

Nate: Thanks for the tip about Drupal.theme, and I take your point about moving the default width into css. But I think you misunderstand the 'popup' option. It is separate from the warn-on-page-change functionallity, which works on all links on the page (except for those without an href, or with href='#'). It would be cool to have this also intercept the browser's "Back" button. The 'popup' => true option is for loading the page that a link would lead to into a popup (ex: when you click on a block's configure link, it opens the configure page in a popup).

Kent: It is easy to center a block vertically if you know it's height ahead of time, trickier otherwise. And it looks bad when the dialog is bigger that the page, because the top goes above the top of the window. But I have an idea...

KentBye’s picture

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.

starbow’s picture

Status: Needs work » Needs review
FileSize
13.19 KB
12.89 KB

This patch is mostly fixes.

* moved the html in dialogs.js into Drupal.theme functions.
* renamed the /admin/get calling admin_get to /get-raw calling system_get_raw_content.
* Got vertical centering working in cases where the dialog is smaller than the window.
* Removed all the tabs.
* Ran dialogs.js through jslint - not perfect but better.

I also added drupal_add_basepath() to common.inc. I think this would be a great function to have even if the rest of this patch goes nowhere.

And I spent some time playing with ajaxSubmit from jquery.form.js to see if I could do the page save and then forward to the originally requested link, but had no luck. It doesn't seem to encode the form_token or the 'op'. ajaxForm works, but I can't figure out a js way to trigger it.

I am including two versions of the same patch. The first one is generated by a window's cvs (with -p, but I worry it has bad line returns in it), the second by generated by eclipse (without -p) .

KentBye’s picture

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.

Dries’s picture

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.

starbow’s picture

Turns out it is much, much simpler way to protect against losing unsaved changes. Using the onBeforeUnload event, we can warn on links and on the back/next/reload buttons, and it only takes around 5 lines of code. I have pulled that out into a new issue: http://drupal.org/node/193799

And since this patch is heading for Drupal 7, I broke out drupal_add_basepath into a separate issue as well: http://drupal.org/node/193804

@Dries: Yeah, I understand I got in the game a little too late. I can roll the general dialog building functionality into a Drupal 6 module and see if it gets any momentum. At his BADCamp talk Drumm said that now was the prime time to start thinking about D7 usability.

starbow’s picture

Version: 6.x-dev » 7.x-dev
KentBye’s picture

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

Anonymous’s picture

I'd like to be involved with this.

I've been working on modal windows for a while and have created multiples from ground up for a few clients already.

My latest was working on one that allowed me to put ANY drupal menu item into a modal window which was fully accessible and usable from the modal window including having the forms in the modal process from ajax (ajaxsubmit.module from jstools was used).

What I had was a modal.tpl.php which was like a page.tpl but with less stuff like no html tags and head tags and etc.. but just enough needed for a basic AHAH method.

Having the tpl would also drastically improve this dialog for themers so full control of markup is available.

going to think about all this more and review the patch more.. i just quickly watched the video but liked what i saw.

starbow’s picture

Status: Needs review » Postponed

I have move my work on this over to an independent module until it is time to start 7.x work.
http://drupal.org/project/popups

aaron’s picture

subscribing.

starbow’s picture

The game is afoot.

The original patch has evolved into a Drupal 6 module (http://drupal.org/project/popups), and now I am breaking it out into three separate issues.

This issue will continue to address the goal of using modal dialogs to enhance administration pages.

http://drupal.org/node/218820 - will provide the core modal dialog API

http://drupal.org/node/218830 - will provide an unthemed=true parameter for getting the contents of a page (and results of a form submission) in a stripped down format that is easy to render in a dialog.

Actual code is soon to follow.

shane’s picture

Title: Adding Modal Dialogs to Blocks Admin » Popups: Adding Modal Dialogs to Admin Pages
Component: block.module » javascript
Category: feature » task
Status: Postponed » Active

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

shane’s picture

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’s picture

@shane: Thanks for the link correction. I have changed it.

As for performance, I haven't seen the issues you are describing. I normally test on Windows XP and OS X. Does your setup have this issue with other modal dialog implementations, or just mine?

Really, the speed of popping up a page in a dialog (or even submitting a form) should be very close to the speed of clicking through to the next page with the popups disabled. If you have firebug installed, I would be interested to know where it is spending the time.

shane’s picture

@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’s picture

Status: Active » Needs review
FileSize
23.29 KB

http://drupal.org/node/218820 & http://drupal.org/node/218830 have been updated with working code. Taken together with this patch they are a working proposal for modal dialogs in Drupal 7.

I am particularly pleased to have worked out the multiform flow issues, so that wizards and preview and validation errors can all be handled properly, moving smoothly from one dialog to another. This enhancement will be backported to the Drupal 6 module.

Note: Drupal 7 info files still need core = 6.x

starbow’s picture

@shane. Which popup? And how does 3.04 sec compare to the amount of time it takes to open that page without the popup?

Also, we should probably more this over to the Drupal 6 module issue queue, now that the Drupal 7 patches are available.

starbow’s picture

FileSize
23.32 KB

Tweaking to stay insync with: http://drupal.org/node/218830#comment-730743

starbow’s picture

Modifying to consume XML instead of HTML, to stay in sync with http://drupal.org/node/218830#comment-752048

XML seems like a more appropriate transport wrapper, since Drupal 7 will probably have improved XML handling and XML webservices.

Susurrus’s picture

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.

Anonymous’s picture

i agree. json i find much better to deal with.

xml is bloat that most do not need and especially for most cases.

I really wish i had more time over the past while to help on this issue. I hope to review and get more involved during drupalcon.

starbow’s picture

@Susurrus & Steve M - the XML wrapper here is already super simple, and jQuery makes it trivial to access it on the other end. I haven't played with json much, so I have trouble imagining it could be any simpler to work with than this approach, but I would be interested in seeing how embedding and extracting HTML in JSON works if one of you wants to code it up. It would be really interesting to see the speed comparison.

Anonymous’s picture

are you going to be at drupalcon? would love to sit down and go over this if you are.

starbow’s picture

Steve: Yep, I am here and happy to chat. I have been trying to find a good time of a popup BOF, but everytime I put it in a slot, something else I really want to be part of goes in the same slot. Maybe we could try and grab lunch together?

PS - Everyone I talked to today was pushing json, so I guess I will give it a try.

starbow’s picture

Ok, I rewrote it in JSON (ok, it was pretty easy), but XML still has an advantage: I can use the same code to wrap either the Drupal XML fragment, or an entire non-drupal HTML page in a dialog. This is a feature I have been working on. $.get will look at the mime type to decide if it is XML or HTML. Looks like $.getJSON just dies if the page is HTML.

Anonymous’s picture

nice.

im sitting in a hacking room (first one).

I'm totally up for sitting down with you and going over things.

I realized now that I was actually standing with you yesterday but didnt even realize it lol :)

starbow’s picture

My connectivity is iffy, but I will be in the BOF room today at 3:30. This would be easier if I knew what you look like :)

Anonymous’s picture

i kinda went awall today.. oh the fun of running a business :S

we gotta meet up tomorrow at least.

any themers looking for a job ? lol

starbow’s picture

Look for me at the code sprint.

quicksketch’s picture

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.

starbow’s picture

FileSize
3.04 KB

Updating to stay in sync with changes at #218830: Popups in Drupal 7: Plugable renderers for generating content. If you are subscribed to this issue, but not that one, please read http://drupal.org/node/218830#comment-764167. It's a biggy.

Overview of changes in this patch:

  • Moved from XML to JSON.
  • Using request headers instead of url param to request json format.
  • Form submissions are now two step. Hopefully this isn't noticeably slower that the old submit and redirect behavior.
    • Step one submits the form as a POST and gets the status of the submit as a JSON object.
    • Step two requests the next page as a GET.

BTW: I am now totally on-board with json and have abandoned the goal of putting html pages in popups as a potential security hole.

Crell’s picture

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

starbow’s picture

@Larry, as this patch evolves, I am definitely going to need a FAQ. Currently this patch is one particular use case for ajax popups in core, and the issue that addresses what actual code infrastructure to use to provide ajax popups is over at #218820: Popups: Adding the core modal dialog API.

But, to answer your question, not going with jqModal is purely pragmatic. I am trying to get this into core, and generally the core committers have preferred small, custom for Drupal javascript utilities vs larger, more general libraries. Also, jqModal seems to be single licensed under the MIT free license. I also like jQuery Improptu, but it is licensed under Apache. And thickbox is just too big. I am keeping a close eye on the jQuery UI dialog plugin, but am waiting for the 1.5 release. Besides, a modal dialog box isn't rocket science, and if we go custom, we can do cool stuff like having the theme system generate the dialog's html.

About the security issue, sorry I wasn't clear. The problem isn't HTML in the popup (which is really what I am doing), it is doing an Ajax load of HTML content from an arbitrary site. Firefox doesn't (currently) allow ajax loads from external sites, so I would need to include a proxy as part of the popup infrastructure, and that just seems like an invitations to XSS attacks.

starbow’s picture

Title: Popups: Adding Modal Dialogs to Admin Pages » Ajax Popups in Drupal 7: Adding Modal Dialogs to Help, Confirmations and Filter tips (Unified)
FileSize
70.31 KB

This is both a big patch, but it is also less ambitious than the last couple. Based on advice from Dries and Nate, I have moved the hook_popup rule engine out of this patch, and into a contrib module. This patch uses the much simpler mechanism of requiring a link that wants the popup behavior to have a class of 'popup'. And I have narrowed the scope of where the popups are attached, to some (hopefully) uncontroversial links: more help, delete/reset confirmations, and the oh-so-nasty filter tips link in the node edit form.

So the popuppage module goes away. popuppage.js gets folded into popupapi.js, and popuppage.module gets folded into system.module, common.inc and theme.inc. Which makes this patch really hard to seperate from #218830: Popups in Drupal 7: Plugable renderers for generating content and #218820: Popups: Adding the core modal dialog API, since they involve the same files. So this patch just includes everything, reunifying the popup effort.

The new, combined popupapi.js has been cleaned up a lot.

  • A big change is that the html template for the popup is now being run through the standard theming system, for easy modification.
  • Also, I have removed the use of position: fixed, which is both good news for ie6 users and it also fixes the issue with the invisible cursor.
  • I have moved to overflow: auto, instead of making the dialog box the full height of the content.
  • And I added some styling to show that a link will open a popup, which needs work. It currently uses the footnote symbol, for want of a better idea.
  • Clicking the overlay, or pressing the ecs key dismisses the popup.
  • Inspired by the Teleport module, I added a hot key for help: ctrl-?
  • Lastly, I modified the look of the popup to match garland (but no color.module integration yet).

Note: this patch also includes a workaround for a bug in tableheader.js #234377: tableheader.js: breaks on Drupal.attachBehaviors().

ezra-g’s picture

Subscribing + trying the patch. This is great work!

starbow’s picture

FileSize
70.31 KB

This patch has the clean ups suggested by Dries at: http://drupal.org/node/218820#comment-770334

I also ran popup.js through jslint for good measure.

This patch is much smaller, as a big chunk of obsolete code accidentally got included in the last patch (the popuppage directory is now entirely gone).

And I have removed the footnote symbol on the popup links, as being too distracting. Hopefully there will be a good icon for this some time in the future. In the meantime, I am appending "[Popup]" to the title of the link.

dharamgollapudi’s picture

Subscribing...

starbow’s picture

FileSize
8.04 KB

For those that are testing the unified patch, to get the full effect, put ajax-loader.gif in your misc folder.

floretan’s picture

FileSize
445 bytes
68.46 KB

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.

webchick’s picture

Status: Needs review » Needs work

Tried applying this but got the error:

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

webchick’s picture

Status: Needs work » Needs review
FileSize
68.46 KB

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

floretan’s picture

FileSize
68.46 KB

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.

urlisse’s picture

Category: task » bug
Status: Needs review » Needs work

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

penguinchix0r’s picture

FileSize
25.56 KB

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.

webchick’s picture

Category: bug » task
FileSize
66.78 KB

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

floretan’s picture

FileSize
445 bytes
67.49 KB

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.

floretan’s picture

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.

starbow’s picture

Drat. I don't know what happened, but the patch in 42 is the same as the patch in 40, and is a bad roll. Like I said in 42, the popuppage module has been completely eliminated. Testers - sorry to have wasted your time. Give me a minute.
...
Ok, make that a couple minutes. Looks like we are now using json_encode, so I have to stop pretending and actually update my PHP to 5.2.

starbow’s picture

FileSize
44.25 KB

Ok, this is the patch that #42 claimed to be, rerolled against HEAD as of 50 minutes ago. All signs of the popuppage module should now be completely eradicated.

@flobruit: This patch is actually two patches in one. The changes necessary to do the json rendering are best considered and discussed over at #218830: Popups in Drupal 7: Plugable renderers for generating content. The patch includes those changes, but is meant to focus on the javascript/css to create ajax popup dialogs, and the specific use of those dialogs to improve the help system and confirmation pages.

PS. I gave the patch the hairy eyeball and it looks good to me, but it is created with Eclipse on WinXP, so it might need some linefeed filtering. Let me know.

floretan’s picture

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

starbow’s picture

@webchick (#51): I have tested the lastest patch in FF3b4 and I am not seeing any issues with tableheader. Please let me know if you are still seeing any issues with #55 patch.

bcn’s picture

tracking

starbow’s picture

Status: Needs work » Needs review
Rowanw’s picture

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?

Stefan Nagtegaal’s picture

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

webchick’s picture

See #5 for screencast.

starbow’s picture

@Rowanw - sounds like a bug to me. What browser are you using? Can you see your javascript error messages?

@Stefan & @webchick - the screencast up in #5 is extremely out of date, and doesn't real represent the current behavior. I will try to crank out a new one before too long (or if someone else wants to jump on it, that would be fantastic).

ezra-g’s picture

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.

Rowanw’s picture

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

starbow’s picture

@ezra-g: works for me. Thanks.
@Rowanw: You might need to install the Firebug extension. Thanks.

Stefan Nagtegaal’s picture

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

nedjo’s picture

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

quicksketch’s picture

FileSize
34.55 KB

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

Stefan Nagtegaal’s picture

Status: Needs review » Needs work

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:
+

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

Rowanw’s picture

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’s picture

@Stefan - Thank you so much for the in depth review! Very helpful stuff.

The name: At this point the name for this feature is "popup". Any reference to "popups" is a bug, unless it is there for grammatical correctness. I don't want to rename it all to modal, because non-modal dialogs are useful too, and I think this patch should work for them as well. I can see rename it all to "dialog", if enough people think that is worth doing.

popup-template.tpl.php -> popup.tpl.php = yes.

popup vs popup-form: class="popup" triggers a popup that does not change the initial page. class="popup-form" triggers a popup that might change the current page, and therefore checks to see if there are unsaved changes on the current page. If there are, it first triggers a popup that offers to submit the page as is, before showing the dangerous form. This is one of the bits I am really proud of.

Fading: I am not totally happy with the fade out. Sometimes it looks good, sometimes it stutters. I agree that if we keep the fade, it should also apply to the fade in.

The close image: My initial version just had the text, but I got a lot of (strong) suggestions to replace it with an icon. What do you think of the icon from #52?

system.css:
Sorry about the tabs and spacing. I will fix.
I will move the Garland-based coloring into a separate patch, and submit that to the Garland queue.
Yes to the rest of your comments.

The settings: I agree that these need to go. As I said in http://drupal.org/node/218820#comment-772882, to do this, we need all core themes to use #content-content to define their content areas (like the page.tpl.php in the D6 system directory).

$popup.css('left', -9999) works fine, but I agree that -9999px looks better.

$popup.css('top', top).css('left', left); is correct. top and left are javascript variables that have been calculated in the preceding lines of code.

My next step is to evaluate Nate's proposal, but your feedback will be rolled into the next patch I do. Thanks again.

Stefan Nagtegaal’s picture

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

Rowanw’s picture

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.

ezra-g’s picture

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?

starbow’s picture

Looks like you should wait on the screencast until I reroll with Stefan's feedback.

dharamgollapudi’s picture

Subscribing...

Stefan Nagtegaal’s picture

@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

rickvug’s picture

subscribing

dharamgollapudi’s picture

Subscribing....

smoothify’s picture

subscribing

starbow’s picture

Status: Needs work » Needs review
FileSize
39.96 KB

Ok, here is the patch I promised, which integrates Stefan's feedback (http://drupal.org/node/193311#comment-792092), and Nate's super-cool (if controversial) rethinking of the core theme function (http://drupal.org/node/218830#comment-791590).

I stripped down the popup's css to be as generic as possible, and still be clear, and removed the close icon. It would be great to get some color.module aware popup styling into Garland's css, but that if for another patch.

I also removed the fade effect, since it remained inconsistent.

I also removed the popup settings page. Hopefully Nate's garland page.tpl.php clean up will get into D7, and if not, theme creators can always use variable_set.

@ezra-g: if you are still up for doing a screencast, it should be good to go (fingers crossed).

dmitrig01’s picture

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 = '
'. drupal_render($form['help'][$key]) .'
'; + $description = '
' . drupal_render($form['help'][$key]) . '
';

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 '';

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.

dmitrig01’s picture

Status: Needs review » Needs work

ahem.

starbow’s picture

Wow, I just looked at Gabor's latest patch (http://drupal.org/node/145551#comment-836323) over at #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.) and it looks like it has a lot of overlap with Nate's theming proposal.

@dmitri01 - thanks for the feedback.

starbow’s picture

Unfortunately, I don't think it makes sense to push ahead with this patch until there is some agreement of the proper mechanism to get Drupal to output JSON.

ezra-g’s picture

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

quicksketch’s picture

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.

pwolanin’s picture

I'm planning to rework the rendering here instead:

http://drupal.org/node/134478

starbow’s picture

Boy, with #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.) postponed and #134478: Refactor page (and node) rendering not seeming to be going anywhere, I have no idea how to push this patch forward.

skilip’s picture

Subscribing

nedjo’s picture

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.

starbow’s picture

I agree with you that it would be a good idea to break this up into smaller patches. This actually patch started as three separate patches, but there was no way to test them independently. Your suggested splits might solve that problem.

I have been going back and forth about jQuery UI. I think it is the right way to go, but Dries was pretty excited about being able to use the Drupal theming system to define the xhtml for the dialog box. I doubt that is possible with jQuery UI, but I haven't looked into it seriously yet.

And, really, whenever I get excited about putting more work into this patch, I remember that there has been no forward progress in almost a year towards getting Drupal to render JSON.

skilip’s picture

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

starbow’s picture

I hadn't seen that one. There are now dozens of modules that operate in the popup/lightbox/ajax forms space. Nedjo is trying to rally the troops over at groups.drupal.org/javascript.

webchick’s picture

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.

starbow’s picture

@webchick: Knowing that is a big motivation boost. But realistically, I can't see putting more time into this patch until there is some movement over at #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.). And now it seems like I should also be waiting on #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) and #315035: Add jQuery UI elements to core. If jQuery UI gets included in core, it would be silly not to use it here.
Do you know if the D7 cycle will be the same at D6's, with a primary code freeze followed by a usability freeze? If so, work on this patch might be most effectively done in that usability window.

webchick’s picture

Status: Needs work » Postponed

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

babbage’s picture

Subscribing. (Will be back soon.)

starbow’s picture

Status: Postponed » Closed (duplicate)

Ok, I declare this patch dead. Long live #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations. The new patch is an attempt to skip all the controversial parts of this patch and just get the most basic set of functionality in place. Sadly it does jettison a lot of what makes this patch exciting, but it is not dependent on any big changes to the Drupal rendering pipeline, it should be simple to understand and debug, and all the cool ajax form stuff can be layered back on top of it in later patches.

starbow’s picture

I have extracted two more simple patches from this monster patch.
#376147: Split drupal_goto into smaller, modular functions
#376142: add hook_form_process_success to drupal_form_process
If these three get in, we have a nice foundation to build some more sophisticated interactions.
Still crossing my fingers for #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.) but we might be able to make do without it.