This patch is a continuation of the following patches:

Add dynamic AHAH submission properties to Forms API - http://drupal.org/node/154398
Add javascript 'more' button to Poll (AHAH enable) - http://drupal.org/node/155870

This patch extends the abilities of the new ahah.js to apply to most types of form elements, not just buttons. Now on any javascript event to a form element (click, change, blur) Drupal can perform a request in the background, retrieve additional HTML and inject it directly onto the page. With this patch, all this will be available for form elements without writing a line of javascript. For a better description of AHAH, read the initial patch description.

Changes in this patch include:
- Adding the standard jQuery forms library to Drupal. The javascript is uncompressed in this version, but in the final version we'll likely compress it, since this is a standard distribution like jQuery itself.
- The addition of this library allows us to remove redirectFormButton(), createIframe(), and deleteIframe() from drupal.js.
- upload.module's attachments now actually work in Safari again.
- The blocks page is the entry point for the new features. Upon changing a weight or region, the page is dynamically updated reflecting the new regions and weights.
- New formAPI property #ahah_selector. Allows the AHAH replacement to be attached to multiple items on the page, not just the one on which the Form property is applied. This property was necessary to add in a a scenario such as the blocks page, where there could be hundreds of AHAH enabled elements. A single selector can get them all at once, rather than adding a javascript setting for every unique ID.
- New "special" javascript class 'ahah-new-content'. If content within the new HTML contains this class, the #ahah_effect will only apply to this content, not the entire chunk of new HTML.

Demo video: http://quicksketch.org/node/504

The video demonstrates nearly all the new features. The select elements are all AHAH enabled, by using the #ahah_selector property. On #ahah_event = 'change', a request is made to Drupal for an updated form. The entire table is replaced with the updated version, but the changed row has the 'ahah-new-content' class, so it receives the #ahah_effect = 'fade'.

This patch is dependent upon jQuery 1.1.3.1, where improvements were made when applying effects to table rows and cells. The update to jQuery 1.1.3.1 patch must be applied first.

This patch makes changes to both theme functions and menu hooks, so cache tables must be cleared and menu_rebuild() executed before testing.

Comments

webchick’s picture

Status: Needs review » Needs work

D'oh. No patch...

profix898’s picture

This is really amazing stuff. Subscribing. Any chance to get this into D6 post-freeze?

quicksketch’s picture

Status: Needs work » Needs review
FileSize
56.75 KB

Mwahahah patches are good :)

mikey_p’s picture

Status: Needs review » Needs work

Loading admin/build/block results in white page, logs:

PHP Fatal error: Cannot unset string offsets in /Users/mdp/www/drupal-6.x/includes/form.inc on line 359

this is with patch #56 from http://drupal.org/node/146462

profix898’s picture

1. function block_theme(): 'block_admin_display_form' => array('arguments' => array('form' => NULL, 'blocks' => NULL, ...), is missing the two additional parameters (blocks, theme). Thats should solve mikey_p's issue.
2. function block_admin_display_js(): variable $changed_block is not necessarily defined for use in '$form[$changed_block]['attributes']['class']'. This part should be wrapped in if (isset($changed_block)) {...}.

Otherwise it works like charm :) I'm not enough js guru to comment on the js part, but from a visual inspection the php part looks fairly well designed. I didnt have time to play with other fapi elements (e.g. textfields, ...) in this context. Your patch adds ahah properties to more elements than are actually used in the blocks 'demo'. Did you write a tiny module to test the additional element's behaviour? The password strength validator doesnt seem to be using the new textfield/paasword ahah property, right? Shouldnt that be changed as well?

profix898’s picture

Password strength indicator is client-side only stuff. Ignore that.

Frando’s picture

Status: Needs work » Needs review

Functionality review:
Great! It works like a charm. Shuffling blocks between regions and assigning different weights both work as expected. This is a huge usability enhancement.

The issue in #4 was solved for me after clearing the menu cache and rebuilding the menu. Can't reproduce afterwards, so this is not a bug.

Quick code review:
The implantation looks really clean and neat. Replacing the Drupal.redirectFormButton and all this iframe code with the jQuery forms plugin sounds good - I never really liked this iframe code, and the jQuery forms plugin appears to be really clean and greatly documented. I've been reading the patch pretty much from top to bottom, and couldn't find anything I didn't like. Great work.
I don't think profix's first issue is valid. AFAICS, block_theme is correct the way it's in the patch.
And regarding $changed_block, I think that is OK too, as the Ajax submit handler only is called when the form is being changed. So, there is always a changed block, AFAICS.

Stefan Nagtegaal’s picture

First of all, this is a very nice usability enhancement and should be noted as such. (Perhaps then, it could make it into drupal 6 also)

Something I do not like, is that the progressbar is displayed after the "Region"-column. i think it would be better to have it displayed on top of the table, or to:
- fade the current table row out;
- display the progressbar;
- hide progressbar when AJAX-reorganisation is done;
- display the "new" tablerow, where it is supposed to appear;

Another possibility is to use craqbox, to display a modal-window for the progressbar with some descriptive message (we could use this and the new AHAH-system through a lot of other area's through drupal too!).

Another nice thing would be, that the layout is directly updated to display the (de)activated blocks in order in their respective region inside the drupal site. but perhaps, that is out of the scope for this patch.

Very nice work after all.. Now if only jQuery would work consistently across the browsers, this would truly rock..

profix898’s picture

@Frando: You are right about block_theme, but the isset($block_changed) still seems required for me. Trying to set a block from RegionX to none generates the following warning: notice: Undefined variable: changed_block in /modules/block/block.module on line 359. without the isset wrapper.

I think we should think about the progress indicator. It somehow doesnt look right. Not sure it is the position it occurs at or the image itself. There should be a different indicator for this purpose than the progressbar IMO. There is no 'real' progress to track here, its only two states 'working/updating' or 'ready', but no position to visualize. How about something like this or the this?

quicksketch’s picture

FileSize
33.09 KB

Thanks much for the reviews all! This isn't nearly as much an "API" change as UI, so my hopes are high for inclusion in Drupal 6.

@profix898 Re: #5 above.

1. function block_theme(): 'block_admin_display_form' => array('arguments' => array('form' => NULL, 'blocks' => NULL, ...), is missing the two additional parameters (blocks, theme).

Yep, thanks! Fixed in this patch.

2. function block_admin_display_js(): variable $changed_block is not necessarily defined for use in '$form[$changed_block]['attributes']['class']'. This part should be wrapped in if (isset($changed_block)) {...}.

Ah yes. $changed_block should always be defined. This explains the weird fade effect when disabling a block. Thanks for pointing this out. It's fixed in this patch so $changed_block is always set.

@Stefan Nagtegaal Re: Progress Bar. I'd like the progress bar to be there, but as unobtrusive as possible. Switching out for a throbber seems like a possibility. Fading out and in the entire block could actually take longer than the request itself. I'd prefer to keep the effects to a minimum. We could move the progress bar to another location, but keeping it next to the element changed makes the most sense to me, especially considering this will need to be a generic solution throughout all of Drupal (see the current upload.module implementation and the ahah poll patch).

This patch fixes the two issues raised by profix898, and compresses the updated forms.jquery.js 1.0.1 for a more realistic patch for core.

profix898’s picture

I'd like the progress bar to be there, but as unobtrusive as possible.

I completely agree with this statement. After thinking about this a bit more its seems like the right location to display the update indicator as close as possible to the changing element. And I also agree that it shouldnt trigger any modal overlay windows or fadein/out the whole block. We should however try to replace the progress bar with the throbber image as it better fits here (see #9) IMO.

Otherwise the new patch looks good. No more errors/warnings/notices. This is a really huge usability improvement and it makes it a lot easier for developers to use ahah stuff in contrib as well.

Stefan Nagtegaal’s picture

I think, the best way to get this in, is to split your patch into 3, so:
1) for jQuery updating to 1.1.3.1;
2) for the AHAH FAPI extension;
3) Your regions/blocks improvements;

The first, and third could be considered usability, and the second a bug/inconsistency.

quicksketch’s picture

This patch does touch on a lot of code, but it's already been dependent on several other patches. The patch includes a minor API change (though large underhood changes), and one example of how the new API functions (the blocks page). I figure with an example it's hard to convince it's a usability improvement. If requested I'll gladly split this up into separate patches for the changes to javascript and the changes to the blocks page.

jQuery 1.1.3.1 upgrade is already a separate patch: http://drupal.org/node/146462

Stefan Nagtegaal’s picture

Quicksketch, can you pls join #drupal for a moment, I want to discuss it with you... I'm there...

Stefan Nagtegaal’s picture

Nice, as I type I'm testing the patch in IE 6/7/Safri 3b/FF 2 on Windows Vista and Safari 2/3b/, FF 2, opera 9 and Camino on OS X.

Stefan Nagtegaal’s picture

Status: Needs review » Needs work

Does not apply anymore, needs updating...

moshe weitzman’s picture

nice video! would love to get this in. subscribe

rickvug’s picture

+1 - This would be a very noticeable usability improvement for end users.

BioALIEN’s picture

I like this, subscribing. Will mess around with this when I get some free time.

merlinofchaos’s picture

This is very important to another usability patch I am working on; subscribing.

Stefan Nagtegaal’s picture

Quicksketch, are you planning on updating this issue? Many people would like to see the patch updated, and have it hit the trunk. the usability improvements are very nice!

Frando’s picture

Any news here?

Dries’s picture

This patch needs to be re-rolled because http://drupal.org/node/162871 went in. Thanks. :)

quicksketch’s picture

Yes, I'm super busy but I'll get to it soon. Steven has some outstanding complaints about the way AHAH is written, but since I haven't seen them written anywhere I'll keep them separated from this patch. We can tweak the code once the functionality is in place.

I haven't yet heard any debate about the inclusion of the jQuery Forms library. It's certainly better than our current per-jquery, super-rigged redirectFormButton, but if there is opposition (or support), they should speak now.

merlinofchaos’s picture

Steven's objections that I know of:

The use of eval() is incorrect and unnecessary.
The iframe is needed for upload.js because normal js form handling can't handle files for security reasons. I don't know if this limitation escapes forms.js or not, but it needs to be investigated.

moshe weitzman@drupal.org’s picture

any progress here? nate?

KarenS’s picture

Subscribing. Once working it would be great to get something like this into CCK's Manage Fields screen so you can dynamically change field weights and groups.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
40.62 KB

That's right folks, the new patch is here!

Compatibility-wise this patch:
- Updates everything to the latest HEAD, included the sweeping changes that happened to block module's templating.
- Renames block-admin-display.tpl.php to block-admin-display-form.tpl.php, since the name of that theme function changed.
- Makes Safari work with upload.module for the first time since 4.7! Unfortunately, it also causes Safari to crash on the blocks page. Cause is TBD.
- Upload module is fully compatible with these new changes, with the exception that upload module isn't saving files correctly right now anyway: issue #168813.

Code-style:
- Removed all instances of eval() from ahah.js. Instead doing $object[method_name](param1, param2); to execute dynamic function names.
- Remove all hard-coded css from ahah.js, now classes are assigned and defined in system.css to handle the default styles.
- Once again, redirectFormButton() is removed entirely from drupal.js. The jQuery forms library in a compressed form is added in it's place. Now at version 1.0.3
- jquery.forms.js switches to using to an iFrame if a file is being uploaded, though special considerations are necessary, such as the returned string cannot be an XMLObject (see the jQuery forms library code samples). This patch takes the returned string and runs it through Drupal.parseJson() if necessary to convert it to JSON.

I've left the blocks part of the patch intact for ease of testing. I'll be glad to split if necessary. Thanks very much for all the positive feedback. I've been very busy and the consistent attention made this too hard to resist. Everyone please give this patch a good testing!

anders.fajerson’s picture

Status: Needs review » Needs work

Tested block and upload in Opera 9, Safari 3 and Firefox 2.

Blocks:

Blocks doens't work in Safari (Windows/3.0.3):
notice: Undefined index: region in W:\www\drupal-cvs\modules\block\block.admin.inc on line 128.
notice: Undefined index: region in W:\www\drupal-cvs\modules\block\block.admin.inc on line 130.

notice: Undefined index: weight in W:\www\drupal-cvs\modules\block\block.admin.inc on line 129.

Add block page doesn't work for all browsers, and gives this notice: notice: Undefined index: block_add_block_form in W:\www\drupal-cvs\includes\theme.inc on line 48.

Upload:
When pressing Attach with no file selected it's still possible to submit. "0 0" is added after the Attach-button or under a previously added file.

UI:
I also aggree that the blue "throbber" would be prefered as an indicator for block changes. I would also be cool if the there could be *some* kind of effect on the block that appears in a new place. Usually I'm negative to effects but here it would serve as a visual indicator to the user where the block actually ended up. *Fast* fade in or something like the "yellow fade in" tecnique could be used for that specific block, and it wouldn't interrupt the user.

It would be nice if the progressbar didn't cause the colums to be pushed left when displayed. It's a bit visually distracting.

quicksketch’s picture

FileSize
40.9 KB

Thanks fajerstarter, what browsers does the upload module present these issues? I don't see it in Safari or Firefox (Mac). I mentioned that the blocks page doesn't work in Safari, but looks like you got more information than I could. Mac Safari just crashes when trying to change a block. :P

This patch fixes the Add Block page, which was caused by a change in the menu handlers. Thanks!

Stefan Nagtegaal’s picture

Perhaps it is wise to enable Safari's hidden 'Debug' menu? When Safari is not running, type in the Terminal the following line:
% defaults write com.apple.Safari IncludeDebugMenu 1

Then start Safari, and enjoy the new Debug-menu.
To disable the menu (why would you want that?):
% defaults write com.apple.Safari IncludeDebugMenu 0

Hope I gave a helping hand here...

quicksketch’s picture

The Safari crashing bug has identical symptoms to the post described in the jQuery forums here: http://www.nabble.com/jQuery-1.1.3.1-Safari-Crashes-t4228093s15494.html

The crashing problems should be resolved in 1.1.4, which is due to be released any day now. Could we intentionally overlook Safari for the time being?

I also forgot to respond to fajerstarter's last comment on throbbers and effects. The fade effect is already being used on new content on the blocks page. It happens quickly, but it's definitely there. On throbbers, I'm hesitant to use the circle because it would mean either using it for all AHAH effects, or making an option in the FAPI for one or the other. Take into consideration file uploads are included in this group. Which would make the most sense?

quicksketch’s picture

Status: Needs work » Needs review
anders.fajerson’s picture

FileSize
8.36 KB

The upload problem (see attached screenshot) appears in all tested browsers (Windows: Opera 9, IE6/7, Safari, Firefox2). To reproduce simply press the Attach button without selecting a file first.

Here is the firebug response for attaching "no file":

{ "status": true, "data": "\x3cdiv class=\"form-item\" id=\"edit-upload-wrapper\"\x3e\n \x3clabel for=\"edit-upload\"\x3eAttach new file: \x3c/label\x3e\n \x3cinput type=\"file\" name=\"files[upload]\"  class=\"form-file\" id=\"edit-upload\" size=\"40\" /\x3e\n\n \x3cdiv class=\"description\"\x3eThe maximum upload size is \x3cem\x3e1 MB\x3c/em\x3e. Only files with the following extensions may be uploaded: \x3cem\x3ejpg jpeg gif png txt html doc xls pdf ppt pps odt ods odp\x3c/em\x3e. \x3c/div\x3e\n\x3c/div\x3e\n\x3cinput type=\"submit\" name=\"attach\" id=\"edit-attach\" value=\"Attach\"  class=\"form-submit\" /\x3e\n\x3cinput type=\"hidden\" name=\"vid\" id=\"edit-vid\" value=\"0\"  /\x3e\n00" }

A fix could be to check if the file field is filled in before submitting (not tested):

if ($('#edit-upload').val()) (
// Do as usual
)
else (
this.focus // Set focus to field or do something else.
)

That would also hinder the progress info to appear when no file is selected.

I noticed that the block page works in nightly builds of Safari (WebKit-r24872). Seems wise to wait for 1.1.4 though and hope for a fix for current versions...

The effect on new content is only visible in Safari nightly in my tests. In Firefox the speed has to be decreased, try $(".ahah-new-content", new_content)[this.showEffect](1100); // ahah.js line 149. In Opera9 and IE7 it never works. IE6 is kind unpleasant all over (table is too wide (probably related to the new tableheader) and everything jumps).

alippai’s picture

Subscribing.

alippai’s picture

jQuery 1.2 is out (and it's attached to drupal), so you can continue with this issue (I found this patch today, but IMO it's now RTBC - I didn't test it, only read the comments - with the value checking)...

quicksketch’s picture

FileSize
38.63 KB

I found the problem with Safari crashing. This line:

var new_content = $('<div>' + response.data + '</div>');

won't work in Safari because it cannot handle regular expressions on strings greater than 8K, see http://dev.jquery.com/ticket/1152.

To fix, I'm using this line of jQuery:

var new_content = $('<div></div>').html('<div>' + response.data + '</div>');

though I'd like to clean that line up a bit.

Another fix for Safari requires the parseJson function be added back into drupal.js. Sorry for the troubles here, but it looks like it'll be necessary when using the jquery.form.js library and Safari. The problem reported by fajerstarter above for the upload module has been fixed in another patch. Please review and post any problems!

quicksketch’s picture

FileSize
41.81 KB

After some face-to-face feedback from Drupalcon, here's an updated patch with a much improved UI.

- The message to remind user's to click the "Save" button has been changed from 'message' to 'warning', making it yellow for greater attention.
- The progress bar image has been changed to the rotating circle throbber.
- The progress bar has been moved to be next to the element the user changed.

scor’s picture

I applied the patch on a subdirectory drupal install, and the throbber.gif doesn't show up because it's fetching /misc/throbber.gif instead of /subdirectory/misc/throbber.gif.
url(../../misc/throbber.gif) should be used in the block.css file instead of url(/misc/throbber.gif).

Once this is fixed and I select a value in a list, the listbox is disabled, and the throbber keeps spinning... nothing happens. I checked my logs and I got a POST request to /subdirectory/
Is there another patch to install, or is it supposed to work out of the box?

quicksketch’s picture

FileSize
41.82 KB

Thanks scor for the CSS bug. Here's an updated patch with the correct patch.

The problem you're seeing is likely because the menu system has not recognized the new menu path. Did you try rebuilding the menu by placing menu_rebuild() in your index.php? You can also enable/disable a module to clear the menu system.

scor’s picture

I actually applied the patch before installing drupal. throbber ok now.
But I still have the same problem, and I found that if I activate the clean URLs, then it works. can you try with clean URLs off?

quicksketch’s picture

Thanks again scor for the thorough review. I'm seeing the same behavior with Clean URLS turned off. This turned out to be a problem with jQuery's $.ajax() method trying to help us out with the submitted data. If you passed in a url like '/?q=admin/build/block/etc', it would try to help you out by changing the url to '/', and adding '&q=admin/build/block/etc' to your data variable.

Conveniently, this 'feature' was added recently and has been removed again in the latest release of jQuery, 1.2.1. After applying this patch http://drupal.org/node/176222, which is currently help up by Konquerer, everything works as expected. Though I'll work on fixing the Konquerer problem in that patch, let's not hold up this one because of that problem also.

alippai’s picture

quicksketch’s picture

Confirmed that the latest head version of jQuery makes the blocks page work correctly without clean urls.

webchick’s picture

I did a functionality run-through in Safari, Firefox, and Opera on the Mac. Tested the blocks admin page, file uploads, and update system, with and without clean URLs enabled. Seems to work. I find the way the things just "appear" in their new spots in Opera/Safari a bit jarring. But this is something we could tweak later, methinks.

However, I think we need someone more skilled at JS, like kkaefer or Steven, to look this over for code-level review.

webchick’s picture

Btw, I had better results testing this patch from a clean install... even with the menu_rebuild() and cache clearing, there were still some problems with display, etc. Might've just been my head being out of date.

quicksketch’s picture

On visibility, we can add a class to mark new rows with a separate background or text color. We could also keep a history of all the changed rows if wanted. But as webchick suggested, let's work on getting it in first then we can talk about recommended colors and clarity.

kkaefer’s picture

Disclaimer: I have not applied the patch and tested the functionality.

I looked through the code, especially the JavaScript parts. Most things look pretty sane and clean. However, var new_content = $('<div></div>').html('<div>' + response.data + '</div>'); needs some cleaning up: We don’t need that additional div inside the html() method.

I think the additions are overall a good idea, however I’m inclined to not vote for a commit, given that we are that late in the release cycle. The patch also renames a tpl file. I’m not sure if that’s a good idea. Is this change required? What I like, though, is the removal of vast .css() statements and the use of classes instead.

quicksketch’s picture

FileSize
41.8 KB

Kkaefer pointed out to me that

var new_content = $('<div></div>').html('<div>' + response.data + '</div>');

adds a redundant div around the added content. This patch now is just

var new_content = $('<div></div>').html(response.data);

so that we have a single wrapper div around newly added content. It also cleans up that line to a state that I'm now happy with it.

quicksketch’s picture

Component: forms system » javascript
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

I'd like to have a core committer go over this request, so I'm bumping up in priority for D6. I personally consider this to be a critical task, as the current implementation of ahah.js was meant to be a test bed, not the final version in Drupal. If it ships as-is, I would be very disappointed in my ability to get an adequate version shipping with Drupal 6.

The changes to the blocks system are purely a UI improvement, but not necessary for this patch to go in. The changes to the ahah framework however, are so very important because our current implementation is difficult to read, contains several inline styles, and doesn't work in Opera, and only under certain circumstances in Safari. The new implementation removes our dependence on the browser-incompatible and crufty redirectFormButton(). It also has the convenient side-effect of making upload module work in Opera (since it is dependent on AHAH framework now).

eaton’s picture

FileSize
41.89 KB

I'm pretty unqualified to talk about the jQuery and CSS aspects of this patch. I noticed two specific things that might be worth tweaking:

  1. AHAH Properties on form elements are stored in a series of prefixed #ahah_property values. How difficult would it be to put these in as a single #ahah property with an array of flags? ie, $element['#ahah']['event'] = 'foo'. I originally argued that we shouldn't nest them like that (months ago) but a conversation with Steven in Barcelona convinced me that I might be smoking crack. Is this something that would be profoundly difficult, or..?
  2. On Safari, changing blocks around works, but the table that contains the block admin form pops out to an odd width and stays there until the form is submitted. Screenshot attached.
  3. Anyone willing to poke at the CSS and see if we can keep the width of the form from changing when the throbber appears? The subtle pop to a wider call width makes me cringe, though it's a CSS tweak that can probably go in after the underlying functionality patch if necessary. I'd just hate to see it go untouched by release.

With those nitpicks out of the way, and the caveat that I'm not any kind of jQuery expert, I think the patch really opens up a lot of potential for form interaction in D6. I'm hoping it gets in.

quicksketch’s picture

FileSize
14.35 KB

Thanks for the review! Responses to 1-3 in Eaton's review.

  1. I mostly agree here. But I'd like to keep this discussion to a separate patch.
  2. I don't know what the best approach would be here. Hard-code % values for the table in the theme or blocks.css? If admissible, I'd like to address this problem at a later point also.
  3. Fixed. Not sure why I didn't think of this earlier. After reading, I realized it's possible to prevent the width adjustment mentioned. This patch adds a right-margin (or left-margin if RTL language) to the select elements in the table. When the progress-disabled class is added, the margin is removed and the table size stays the same.
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Still discussed and developed => not RTBC.

bennybobw’s picture

Looks like the latest patch isn't applying well. It's adding files that are already there.

scor’s picture

I notice the same thing, I also get 2 hunks failing on system.module
are you sure you attached the right patch, this one seems to be coming from the ahah part 1 (Add dynamic AHAH submission properties to Forms API). the numerotation is not consistent, it should logically be something like drupal_ahah2_7.patch, not drupal_ahah_9.patch :)

quicksketch’s picture

FileSize
42.47 KB

Oh dear, I did attach my patch from part 1 rather than the updated one for this issue. Here's the correct patch.

Dries’s picture

  1. # AHAH Properties on form elements are stored in a series of prefixed #ahah_property values. How difficult would it be to put these in as a single #ahah property with an array of flags? ie, $element['#ahah']['event'] = 'foo'. I originally argued that we shouldn't nest them like that (months ago) but a conversation with Steven in Barcelona convinced me that I might be smoking crack. Is this something that would be profoundly difficult, or..?

    If this is considered to be an important clean-up, I encourage you to clean this up as part of this patch.

  2. Can we rename jquery.form.js to jquery-form.js?
  3. I've yet to test this patch but I'll do that later on.
profix898’s picture

I'm with Eaton that we should combine all #ahah properties in a single array, eg. $element['#ahah']['event'] = 'foo'. The various #ahah_* properties (ie. effect, method, wrapper) are actually options required for AHAH rather than true properties of the form elements IMO.
Otherwise this patch is really awesome, it cleans up the crappy JS (redirectFormButton, ...) and open the door for amazing applications in D6.

quicksketch’s picture

Status: Needs review » Needs work

Much thanks to Dries and Gábor for their reviews.

It seems like we're pretty much at a consensus that #ahah should be a single array type property. I'll reroll this patch with those changes (it shouldn't be difficult at all).

As for renaming, I don't have a problem with it really, but it's going against the jQuery file-naming convention that I've seen. Most libraries that add new methods to the jquery library are given this sort of file name including jCarousel, Accordion, Forms, and others. But then again interface and thickbox are just called interface.js and thickbox.js. I guess it's a matter of whether or not we think renaming files from other projects is a good idea in the future.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
44.64 KB

Here's a patch with a single #ahah property as an array of options. I'll wait to hear confirmation on renaming the file before updating for that change.

quicksketch’s picture

FileSize
44.26 KB

I found a side effect of the single array structure. It means that user's will need to set an event for the javascript to be triggered. Previously, we had set #ahah_event defaults for every form element (such as 'change', 'click', 'blur', etc). Now with the single array format, most definitions for AHAH will look like:

  $form['element'] => array(
    '#type' => 'button',
    '#value' => t('More fields'),
    '#ahah' => array(
      'event' => 'click',
      'path' => 'some/path',
      'wrapper' => 'div-wrapper-id',
    ),
  );

I removed the default event properties to make usage consistent. I think it makes #ahah more clear, since developers will be immediately made aware of the #ahah[event] property. Previously it was a property that would rarely need to be changed since a reasonable default was set. With a single #ahah property however, setting default #ahah[event] wouldn't be as helpful since developers will probably prefer to set the entire #ahah property at once with a new array.

webchick’s picture

Yeah, I don't think we should rename jquery.form.js, as that's the file name from the original source: http://www.malsup.com/jquery/form/#download

kkaefer’s picture

Nate, just add an array with the default values (+= operator) to the #ahah property at some point. This will add the array properties which are not yet present without overwriting already present values.

quicksketch’s picture

FileSize
44.95 KB

Thanks kkaefer for that suggestion. I put in the defaults into a switch statement in form_expand_ahah(), so a default event is added if $element['#ahah']['path'] is set, but $element['#ahah']['event'] is not. This patch is now functionally equivalent to the version before #ahah was split into an array.

bennybobw’s picture

Status: Needs review » Needs work

After applying this patch in head, when I try to go to admin/build/block. I get:

Cannot unset string offsets in forms.inc line 385

But if I add a module, and go to admin/build/block it works.
I'm thinking it might have to do with rebuilding the menu?

webchick’s picture

Status: Needs work » Needs review

Yes, please note to test this patch you'll need to clear your cache tables and also place a menu_rebuild() in index.php (for one page load only). That string offset things happens when one of these two steps isn't followed.

It might be easier to test on a fresh install.

anders.fajerson’s picture

1. Rows change place even if not changed weight. To reproduce: 1. Set primary to "left region". 2. Change the weight for any of the "disabled blocks". The left region blocks then randomly changes place with eachother.

Minor stuff:

2. A rare one, related to tableheader: if you change a block and instantly resizes the browser window (tested in Firefox), a JS error is thrown. Removing tableheader.js "fixes" this.

3. Usability: On a page with many blocks and a small screen, the yellow status text will be scrolled out of the viewport. I like that it pops up (makes it more noticable), but it might be better to always show it so there is at least a chance to see it once when scrolling down a long page.

4. Usability: Make "Save button" italics in the status text?

5. CSS: The padding on the status page is now 2px, it should not override the standard .messages padding.

I'd love to test the actual API of this as well, are there any documentation available yet?

quicksketch’s picture

FileSize
45.5 KB

Great feedback all, fajerstarter. Replies:

1. Rows change place even if not changed weight. To reproduce: 1. Set primary to "left region". 2. Change the weight for any of the "disabled blocks". The left region blocks then randomly changes place with eachother.

Fixed in this version. It turns out enabled blocks have never been sorted by weight and then title, like one might assume. The order has always been random with 2 blocks at the same weight, though it was "consistently random". I'm not sure how this is, because they're sorted with usort(). According to the PHP docs for that function,

Note: If two members compare as equal, their order in the sorted array is undefined. Up to PHP 4.0.6 the user defined functions would keep the original order for those elements, but with the new sort algorithm introduced with 4.1.0 this is no longer the case as there is no solution to do so in an efficient way.

2. A rare one, related to tableheader: if you change a block and instantly resizes the browser window (tested in Firefox), a JS error is thrown. Removing tableheader.js "fixes" this.

This seems to be a problem with tableheader.js trying to use it's behavior on content that is no longer on the page. I wrote http://drupal.org/node/179937 to patch the problem, but I think this might be indicative of a larger problem we'll need to solve in D7.

3. Usability: On a page with many blocks and a small screen, the yellow status text will be scrolled out of the viewport. I like that it pops up (makes it more noticable), but it might be better to always show it so there is at least a chance to see it once when scrolling down a long page.

I'd like to handle this in an entirely different manner. I'd like to have a behavior for 'savechanges'. This behavior would alert a user when leaving a page without saving the changes. It would present the user with the option to save the changes, do not save changes, or cancel and not leave the page at all. Anyway, I'll say we post this as a separate patch also.

4. Usability: Make "Save button" italics in the status text?

Fixed.

5. CSS: The padding on the status page is now 2px, it should not override the standard .messages padding.

I didn't change anything with the status message CSS. I think this is the intentional styling when using the messages div outside of the messages area. You get similar message divs when your system is checked for Clean URL support during the install.

I'd love to test the actual API of this as well, are there any documentation available yet?

I put some up in the original patch: http://drupal.org/node/154398#comment-266514, though looks like it's not very accessible any more. The patch version should still work. It'll need to be updated to include new functionality in this version of course.

scor’s picture

bennybobw: to force the rebuild of the menu, you can also just enable or disable any module, it may be easier than editing index.php and adding menu_rebuild().

Nate: in block_menu(), since you don't use drupal_get_form on admin/build/block any longer, you need to add
'page callback' => 'drupal_get_form',
to the child items admin/build/block/configure and admin/build/block/delete menu items. As it is, you can't access the configure or delete page of the blocks.

Dries’s picture

I tested this patch with Firefox and it looks good.

The blocks in the 'Disabled' region of the block administration page didn't seem to be "moving". That looks like a bug -- if it is not, the behavior is confusing and inconsistent with the other regions. All the other regions seem to work.

+  // Attach the AHAH events to the submit button. Set the form id as the wrapper
+  // and set the selector to every select item in the form.

This comment wasn't 100% to me. It would be great if you could explain (in the code comments) _why_ you are attaching the AHAH events to the button.

I think this patch should go into Drupal 6 beta2 ... after it has been confirmed to work on nearly all browsers.

scor’s picture

I applied the last patch ahah2_11 and then installed drupal. On Ubuntu Firefox 2.0.0.6, I notice a slight jump of the height of the row when the throbber appears. It first appears on the left under the select field, and then floats to the right.
See this screencast: http://openspring.net/files/issues/ahah2_11-jumpy.ogg
I noticed a similar problem in IE7.

No such problem on firefox/windows, opera/ubuntu and konqueror/ubuntu.

recidive’s picture

FileSize
44.9 KB

I've re-rolled the patch trying to solve the problem described on #71.

I've changed the css from:

#blocks select {
  margin-right: 18px; /* LTR */
}

To:

#blocks select {
  margin-right: 15px; /* LTR */
  float: left;
}

Now the throbber shows up direct on the right, but the column is still being stretched a little bit. This is mainly due to that margin being set on the select element. Moving this to the div:class=form-item should solve the problem, but this would require changes in js, e.g. to set the class progress-disabled on that element instead. Despite this minor issue, I think the patch is ready to go.

scor’s picture

Which browser do you use? this doesn't seem to solve the problem on ubuntu firefox.
if the throbbers were displayed when the page loads but hidden with the visibility: hidden CSS rule, they'll take the room they need. all we'd have to do to display them is to switch to visibility: visible when necessary.

recidive’s picture

Hi, I use firefox 2.0.0.5 on OpenSuse (Mozilla/5.0 (X11; U; Linux i686 (x86_64); pt-BR; rv:1.8.1.5) Gecko/20061023 SUSE/2.0.0.5-1.1 Firefox/2.0.0.5).

The throbber is not being hidden/unhidden, they are actually added/removed.

scor’s picture

The throbber is not being hidden/unhidden, they are actually added/removed.

Oh, yeah I know that ;) but I just made a suggestion to fix the issue: display them hidden so that their space is already allocated, and make visible when needed.
Another idea may be just to use absolute positioning.

quicksketch’s picture

FileSize
46.74 KB

Here's a very good shot at full browser compatibility, plus a few changes requested.

The blocks in the 'Disabled' region of the block administration page didn't seem to be "moving". That looks like a bug -- if it is not, the behavior is confusing and inconsistent with the other regions. All the other regions seem to work.

I totally agree. Since we've touched on the sort function now anyway, this is a very logical change. It's fixed in this version.

This comment wasn't 100% to me. It would be great if you could explain (in the code comments) _why_ you are attaching the AHAH events to the button.

I've replaced with this more verbose comment:

  // Attach the AHAH events to the submit button. Set the AHAH selector to every
  // select element in the form. The AHAH event could be attached to every select
  // element individually, but using the selector is more efficient, especially
  // on a page where hundreds of AHAH enabled elements may be present.

I think it's a great change, as people will often go hunting for examples in core.

As for browser compatibility, I added CSS to make the throbber appear properly and nearly identical in IE6/7, Safari, Firefox (Mac/Windows) 1.5 and 2.0, and Opera 9 (Mac/Windows/Ubuntu). There is no shift in column width or row height in any of these browsers. Unfortunately I couldn't find a fix to the jumpy issue in Firefox on Ubuntu reported by scor in #71.

Because I couldn't bear to remove the nice fade effect from other browsers, I put in a special case for Safari when it comes to using effects on table rows:

  // Determine what effect use and what content will receive the effect, then
  // show the new content. For browser compatibility, Safari is excluded from
  // using effects on table rows.
  if ($('.ahah-new-content', new_content).size() > 0 && !($.browser.safari && $("tr.ahah-new-content", new_content).size() > 0)) {
    ...
  }

This makes it so Safari no longer does the improper table resizing when showing the new row. I think it's important to include this special case for table rows, since we'll probably want to add the fade effect back to upload module now that we have the ability to do that through the AHAH framework.

To help the effects-challenged browsers (nearly everything except Firefox it seems), I've added color highlighting on rows. In #45, webchick suggested that the addition of new rows was a bit jarring. Though in #47 I said I'd include this in a later patch, perhaps adding it now will put to rest any concerns about losing track of changes. It should also help with people leaving the page without saving, though I still plan on implementing a sort of javascript alert I described in #67 in a separate patch.

webchick’s picture

FileSize
89.74 KB

Oh. My. GOD!

I LOVE this!!

For those who don't know how to apply patches, I've attached an annotated screen shot so you can see how awesome this is.

I've tested the blocks interface thoroughly, as well as file uploads, in OS X Safari 2.0.4, Firefox 2.0.0.7, and even Opera 9.0.2 for kicks. Also tested both with and without JS enabled. It seems like to me that everything is working.

If we can get someone to chime in on IE 6/7, I think this sucker is RTBC.

Stefan Nagtegaal’s picture

Status: Needs review » Reviewed & tested by the community

Tested on IE 6/7: Everything works..
Nice patch and usability boost, wish we had this before. I can think of *a lot* of other places where this would definatly be needed to ensure our usability gets to a higher level.

This is RTBC

anders.fajerson’s picture

I'm not willing to remove the RTB, but some nitpicking (to use Webchick words ;) ):

* I really can't see any fade effect (WindowsXP: IE6, IE7, Safari, Opera 9, Firefox 2). It sort of flashes in Firefox 2, which usually means jQuery is trying to fade in/out, but sort of fails. Or my eyes are just too slow...

* I would add the (very nice) star to the status message: "* Your settings will not be saved..."

* Opera 9 is really jumpy. Not sure what's causing it, seems random. Sometimes the whole screen flashes, sometimes the row is just increased in height. NOT a show stopper to me.

The color and star makes all the difference, great!

quicksketch’s picture

fajerstarter, the fade is something that is a little hit or miss. I can clearly see the fade in Firefox (Mac), and a sort of delayed appearance in Opera. Either way, it's not actually breaking the layout so I'm not heavily concerned. I get the feeling that it's something that will improve with time, through updates to jQuery and various browsers. Increasing the effect interval time seems to have no effect on the browsers that don't show the fade currently.

It's better to include the effect for those browsers that can display it and since it doesn't cause a problem in other browsers, I'd like to keep the patch as-is. If a more effective way of fading in a table row comes to light, I'd love to apply it to the AHAH implementation.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

The interface improvements look great, and it also seems that you fixed all concerns Dries had, so committed. Thanks for all the hard work.

eaton’s picture

Hooray for cool UI effects that degrade gracefully! Thanks for all the awesome work on this. :D

anders.fajerson’s picture

Status: Fixed » Needs work

Sorry for not seeing this earlier, but I thinks it sneaked in a bug:

Clicking "configure" on the block admin page gives this error and no config page:

notice: Undefined index: block_admin_configure in W:\www\drupal-cvs\includes\theme.inc on line 48.

I'll open a new issue if it's not related to this (or should I have done it anyway?).

Dries’s picture

Great job people. I also like the '+ 1' picture that Angie threw in -- made me smile. :)

Gábor Hojtsy’s picture

fajerstarter: indeed, I can reproduce the bug and it is most probably caused by this change. Let's fix the issue!

scor’s picture

Status: Needs work » Needs review
FileSize
883 bytes

I mentioned this bug in #69 already.
the patch fixes the configure and delete page (for custom blocks).
(don't forget menu_rebuild() before testing)

anders.fajerson’s picture

Status: Needs review » Reviewed & tested by the community

Great, works.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

anders.fajerson’s picture

So, drag and drop in Drupal 7? ;)

http://drupal.org/node/181066

quicksketch’s picture

Next targets for AHAH in core:

http://drupal.org/node/157752 (poll module, code needs review)
http://drupal.org/node/181125 (book module, stub issue)
http://drupal.org/node/181126 (menu module, stub issue)

These patches shouldn't need to change the AHAH framework any further, though it's likely that they'll expose small issues with ahah.js as we use it in more places. The poll module patch I posted this morning includes one such fix. So for the ahah-excited, please go review. :)

scor’s picture

for the first link I guess you meant
http://drupal.org/node/155870 (poll module, code needs review)

Anonymous’s picture

Status: Fixed » Closed (fixed)