Add dynamic AHAH submission properties to Forms API

-p missing from the patch, almost impossible to review.

Why only changing the elements you changed? What's an #ahah_path? And there are TAB characters in the patch.

oh, sorry i see #ahah_path explained, now. Still tehre is much to be explained. For example you have "Currently only buttons may have AHAH events attached to them, and the only event supported is on submit." and yet checkbox and radio is changed. I am confused.

Now, I like this but let's see what Eaton has on it.

subscribe

Just took this patch for a spin and file uploading is working fine. The changes here make sense -- in the future, we may figure out a cleaner, more generic way of handling the configuration/setup side of the AHAH stuff, but this is a big step forward for core. Getting this piece in, and evolving it as we learn more, is a good thing. RTBC.

I've also changed the category from 'feature request' to 'task' -- this is another piece in the puzzle for FAPI3's clean and secure AHAH support.

Whoops.

I just did a quick check and had missed an issue -- the code does not currently work with clean URLs enabled...

False alarm. I had hosed my install with another unrelated patch and it interfered with my re-testing. I just ran it through on a clean install and it works fine. Thanks! ;)

I haven't reviewed this patch yet, but its documentation is lacking. In the issue, you carefully explain what AHAH is, and what parameters are mandatory. None of this knowledge is carried over to the actual code/patch.

I'll review the patch when the documentation is a bit more fleshed out.

This raises an important question: where SHOULD we document behavior that is controlled by FormAPI properties? There are no new functions added by this patch to serve as a base for PHPDoc details, etc.

Dries, actually this is precisely what contrib/docs/developer/forms_api_reference.html is for. That file should be updated once this goes in to note the new FAPI properties. The new properties, and some minor changes to upload.js to make it the generic ahah.js, are the only real changes made by the patch.

After reviewing, there *is* no good place to put the documentation in the patch itself.

The documentation patch looks a bit weird with those hardcode/nested font-tags.

However, I think we should extend the PHPdoc.

1. Specifically, let's extend theme_form_ahah() so developers/designers know how to use this function. Explain what 'AHAH' is and explain when they might want to overwrite this specific function. Thanks!

2. At the top of ahah.js, explain what ahah is and what this file is about.

3. Is there a reason to put '.prototype.' in the function names in ahah.js?

The documentation patch looks a bit weird with those hardcode/nested font-tags.

To his credit, that's the format of the existing reference document. Reformatting that entire document to be cleaner HTML is probably outside the scope of just this issue.

However, I think we should extend the PHPdoc.
1. Specifically, let's extend theme_form_ahah() so developers/designers know how to use this function. Explain what 'AHAH' is and explain when they might want to overwrite this specific function. Thanks!

Ideally, they'll never override this at all -- like theme_node(), its purpose is to handle core functionality that needs to happen during the theming process automatically. The only time they would want to override the function is if they were swapping out an alternate implementation of ahah.js. They would never directly call theme('ahah',...) either; instead, this injects client side script and handles the wrapper div for the AHAH-controlled chunk of HTML, if I understand correctly.

We could make a note in the PHPDoc for that function that it's used internally by FormAPI, not unlike theme_checkboxes or theme_textarea. But we still face the difficulty of documenting behaviors that are driven by arbitrary array properties in our #structures. it's enough to make one long for PHP classes... :-)

I good explanation at the top of ahah.js would definitely be helpful, though.

Reviewed and re-checked; the new comments make it clear what the theme function is for, and the changes to the doc make it pretty straightforward, adding new stuff to form elements. Very cool!

OK, this looks sane. Thanks for the updates/clarifications. I'll commit this patch on Monday, unless a jQuery expert comes in an veto's the approach.

I reviewed another jQuery patch today -- a behaviors patch that Nedjo was working on. I'd like to see that patch land as well. Not sure, but there might be some conflicts. If you have time, maybe take a look at that one too. See http://drupal.org/node/120360.

This is a nice addition and well coded. I haven't tested, just looked over the code.

I particularly like how this patch takes an existing behavior that's hard-coded to a specific use case and generalizes it so that it fully meets the original use case but is also flexible enough to meet a range of other ones. This is exactly what we need to be doing.

Comments:

1. Theme use

theme_form_ahah() is not for theming (it returns no rendered content), so a theme function feels wrong. If the aim is to enable overriding of the ahah handler, maybe this needs to be done at the module level.

2. Drupal.redirectFormButton

I've had numerous problems with this function. It was designed for the narrowly-conceived use case of upload.js, where we have an additional submit button in a part of a form. The function creates problems when, e.g., form submission may be triggered by actions other than clicking on a submit button (like hitting enter, or trying to submit programmatically), situations that are complicated by cross-browser issues.

When we try to attach behaviors to the form button, they get missed when users e.g. hit enter to submit. This doesn't happen in upload.js (there are no text fields in the upload UI) but is likely to occur as soon as we try to attach behaviors to other forms.

Specifically, in the Active Edit module, I load inline form snippets (click on a node title, load a form for editing just the title, submit, the title is updated). When I tried to do this with redirectformbutton, it failed if a user hit enter to submit the form (regular form submission, full page reload).

After much work trying to use redirectFormButton, I ended up replacing its use with a function called Drupal.redirectFormSubmit, in jstools.js, which is much more flexible. It doesn't wait until a user hovers over a button to attach the behavior, but presumably could be used in that way if needed. I wonder if the same should be done here.

3. Variable names with $

  var $wrapper = $(this.wrapper);
  var $button = $(this.id);
  var $progress_element = $(this.progress.element);

Variables shouldn't have the $ signs.

4. basePath

drupal_add_js(array('system' => array('basePath' => base_path())), 'setting');

This setting doesn't seem to be used in the patch.

5. Name of behavior

This is not a generic AHAH framework but rather is specific to form submission. We should find a more descriptive name. In general we name behaviors for their action rather than their method. Some suggestions: submit, formSubmit, activeSubmit.

6. Full form submission

upload.js is premised on the limited use case of submitting a form so that we can update only a portion of the UI prior to subsequent regular form submission.

What will happen if developers try to use the ahah behavior for full, regular form submission?

(I've implemented full AJAX form submission in the ajaxsubmit module (part of Javascript Tools). There I've tried to work through questions like returning status messages, resetting form elements with error markers, etc. This might be useful for reference, though probably in a follow up patch.)

Altogether, very nice work.

Probably the only one of my comments here that would require a lot of work to address is 2, redirectFormButton. It might be okay to apply this now and leave fixing of redirectFormButton for later. It would be useful first to confirm that this issue exists.

Steps to reproduce this problem:

a. Create a subform that needs to be submitted separately and contains a textfield.
b. Hit enter from within the text field.
c. Observe result. If I'm following the code correctly, it will be regular form submission rather than the expected AHAH.

As for the relation with http://drupal.org/node/120360, I don't foresee any problems. We've already tested upload.js's conversion, and this should be almost identical.

Can we modify FAPI so setting #ahah_path would be optional, and even don't need a (required) menu item/callback. This way we could make access check consistent and avoid security issues. The approach is simple, drupal_get_form, drupal_render, etc should check if it is an AHAH request and return a sliced JSON form if so.

Almost every form items already have wrappers (the ones that call theme_form_element()). Can we use these wrappers, so instead of the #ahah_wrapper we use #ahah_element. For group of form fields we could use fieldsets.

$form['dynamic'] = array(
  '#type' => 'fieldset',
);
$form['dynamic']['text'] = array(
  '#type' => 'textfield',
  '#title' => t('Text'),
);
$form['submit'] = array(
  '#type' => 'submit',
  '#value' => t('Submit'),
  '#ahah_path' => 'my_module/js',
  '#ahah_element' => 'dynamic',
);

Note: Fieldset is one (and probably the only) form field that currently doesn't have wrappers.

Why the AHAH stuff is restricted only to 'button' and 'submit' form files (this is what the docs says, at least)? It would be great and not so complex to implement, bringing those capabilities to other form fields, such as 'select', it would allow server side dynamic select fields, e.g. you have two selects. When you change one, the other is updated.

$form['select_one'] = array(
  '#type' => 'select',
  '#title' => t('Choose'),
  '#options' => array(1, 2, 3),
  '#ahah_element' => 'select_two',
  // The following property should be optional.
  '#ahah_event' => 'change',
);
$form['select_two'] = array(
  '#type' => 'select',
  '#title' => t('Choose'),
  '#options' => array(1, 2, 3),
);

Note: Obviously, it would need to some logic to change options for the second 'select' based on the option selected on the first.

To avoid using (and get rid of) the Drupal.redirectFormButton function, one option adding the 'official' jQuery plugin 'forms'. AFAIK, it automatically detects if the form is an multipart/upload form and if so, it uses the iframe approach, if don't it uses standard jQuery.ajax calls.

I agree with Nedjo that the term 'ahah' is not so descriptive. I was wondering if we could we just call it #bind_to. So with the changes above, plus changing #ahah_element / #ahah_wrapper to #bind_to we would have:

$form['dynamic'] = array(
  '#type' => 'fieldset',
);
$form['dynamic']['text'] = array(
  '#type' => 'textfield',
  '#title' => t('Text'),
);
$form['submit'] = array(
  '#type' => 'submit',
  '#value' => t('Submit'),
  '#bind_to' => 'dynamic',
);

So the other property names could be use the #bindings namespace, e.g. #bindings_effect, #binding_path, etc. Or, instead, the #bind_to property could be set to an array:

$form['submit'] = array(
  '#type' => 'submit',
  '#value' => t('Submit'),
  '#bind_to' => array('element' => 'dynamic', 'effect' => 'fade', 'path' => 'custom/path', 'event' => 'click'),
);

And finally, are there other core forms we can add these capabilities to?

Anyway, great patch! I just think we could make this more extensible by implementing some of the features above if we have time (2 days from the code freeze :( ).

@quicksketch

Thanks for the responses, However, please try to consider more closely the potential merits of comments. It feels like a poor use of commenters' time if you don't. There's little sense in my having to repeat the same comments. That said, here goes.

1. Theme function

The theme function has a drupal_add_js(), and therfor adds markup to the page.

It "returns no rendered content", as I commented. It adds JSON data, which is not themed. We have many existing calls to drupal_add_js() (and drupal_add_css()) that are not in theme functions. Should these all be moved to the theme layer? I think it's obvious they shouldn't.

Rather, the point here is to focus on the question of enabling override. Is this appropriate in the theme (and therefore does it justify this bending of the purpose of theme overrides)? My feeling is, no. That's not what the theme system is for. It would be better to leave this out and make this a non-theme function. Or, as I said, "If the aim is to enable overriding of the ahah handler, maybe this needs to be done at the module level."

2. redirectFormButton

In system.module, the default events are only set on
buttons, #ahah_event = 'submit'.

My point is that developers trying to use this - attaching a behavior to a button - will get unexpected results (no AHAH) and potentially broken behaviors (if the JS-processed content breaks without AHAH) due to non-button-click submits. Is this true? Can you try the test I suggested and report back? If it is true, this should at least be documented. Ideally it would be fixed.

3. Variable names with $

Setting variables with dollar signs is common practice when setting variables that are jQuery objects

This is not a convention used as yet in Drupal core. If you want to introduce it, you should take the time to convert as well the many existing variable references to jQuery objects in core. It will also need documenting.

5. Name of behavior

As stated above, the idea is to make this work on all form elements, so ahah is appropriate.

Your comment misses my main points. Here's mine again:

This is not a generic AHAH framework but rather is specific to form submission. We should find a more descriptive name. In general we name behaviors for their action rather than their method. Some suggestions: submit, formSubmit, activeSubmit.

To restate: this is not AHAH in general, this is a specific behavior related to form submit buttons. It may later be expanded to other form elements, but that doesn't mean that 'ahah' is descriptive. What would be? I offered some suggestions. You might come up with others.

6. Full form submission

If the user attached this to the actual form submit button, it should work fine. Because this simply redirects the form action to a hidden
iframe (in drupal.js), the entire form is submitted normally

As I mentioned, the specific problems of full form submission including returning status messages and form element errors. Without solutions for these problems, the current ahah.js applied to a regular submit button presumably would break e.g. validation (there is no way to inform a user of the difference between failed validation and successful submission). So it probably wouldn't work fine. Probably not a big problem at this point. But something to note, and probably to document.

Again, these comments are offered in the context of strong support for the patch. Thank you for your work.

quicksketch + nedjo = rocking.

The idea that poll.module could use actual dynamic updating excites me in ways that are almost scary.

If you're making these changes, shouldn't upload.module also be changed so:

print drupal_to_js()

is changed to use the new drupal_json() function which sends the right headers?

Also, is this patch doesn't seems to deal at all with form validation issues due to changing form elements - do you avoid that by only doing submits?

Also, is this patch doesn't seems to deal at all with form validation issues due to changing form elements - do you avoid that by only doing submits?

I'm not sure what you mean. Do you mean things like, required fields being added to the form, or complex cross-field validation rules being broken by the addition or subtraction of fields? quicksketch is working on a patch to poll.module (currently working, but depending on this patch) that alters the cached copy of the form whenever client-side changes are made, to keep the validation in sync.

How that gets handled woudl really depend on the application of this code. It's probably best to handle it separately.

Would it make sense to roll the poll.module enhancements into the patch as a demonstration of the new functionality? That might help folks get an idea of how it's going to be useful. Or, just keep the patch at the level of functionality it has currently...

For the poll module, have you looked at the code I have in the book module patch?

I picked Jeff's brain for an hour to get the basic ideas of how to do it, and after stumbling around for a while and getting the starting JS from Dmitri it works well now to replace a second select based on the first: http://drupal.org/node/146425 See book.js and the AJAX callback function in the patched book.module.

Thanks quicksketch for your further work.

In my view the latest patch successfully addresses the issues I identified. I'm not wholly convinced by the name ahah.js but it does for now and I don't have better suggestions. The remaining need is in noting some issues or shortcomings in documentation for the upgrade or for use of the functions:

a. this shouldn't yet be applied to regular submit buttons
b. there may be issues with non-AHAH submission being triggered by user actions in AHAH subforms

The second issue will require a future patch to refactor and improve Drupal.redirectFormButton.

Installed and tested. Cleaner and slicker. Thanks for the feedback, nedjo.

Waaaait a minute. Those last two patches were the *same!* ;)

That's more like it. Tested and approved with both the upload and poll modules, and the two cases you mentioned work. Hot.

I'm working on updating this for the Drupal.behaviors patch that went in.

I have to head out (holiday long weekend here, family vacation). I've drafted an update but not tested it. Here is the revised js file. If someone could pick up where I've left off that would be great.

[21:05] Dries_: the AHAH/Poll related patches, would those be UI, or can I mark 'em as postponed? It's OK either way ;)
[21:05] eaton: these are usability improvements

Hm, once again I forgot the HTML filter. So switch eaton and Dries - these aren't the speakers name (HTML filter ate them) but the adressed nick.

I think this qualifies as a usability improvement -- especially with the poll module improvements.

Seeing an IRC quote of Dries asking me how to classify an issue can only improve my reputation!

*slips Frando a $20*

seriously, though. the poll.module improvements that this makes possible are really exciting. I think it's the first clean update we've made to poll.module since version... Um. Something? :-)

Committed to CVS HEAD. Thanks.

He: the documentation patch, comment #18 above, never got committed.
http://drupal.org/node/154398#comment-573549
6 months after the fact, there is still no proper documentation for this feature.

Documentation problem. The patch is not against 6.x core, but the contribution CVS hosted documentation.

This is nuts.
I will commit this.

Reformatted to match current documentation and committed.

Looks like something has changed since those docs were written. The actual form builders in use look like:

modules/poll/poll.module:239:    '#ahah' => array(
modules/poll/poll.module-240-      'path' => 'poll/js',
modules/poll/poll.module-241-      'wrapper' => 'poll-choices',
modules/poll/poll.module-242-      'method' => 'replace',
modules/poll/poll.module-243-      'effect' => 'fade',
modules/poll/poll.module-244-    ),

So there is only 1 form element (#ahah) not several (#ahah_path, #ahah_event, etc).

here's a major fixup of this mess.

committed this version - but still needs review. PLEASE!

I read everything, checked that all the links work.

I corrected some typos, improved the grammar in some places.
I added a list of default values in #ahah['event'].

since everywhere the elements are in alphabetical order, I reordered the list:

#ahah
Used by: button, image button, submit, checkbox, password, radio, select, textarea, textfield

Thus I found that select was missing from the list with default values, and radios was missing from the list above.
I have not touched the list of default values at the top of the page, but I have added radios and select to the copy of the list in #ahah event.

http://cvs.drupal.org/viewvc.py/drupal/contributions/docs/developer/topi...

Here is the only reason I don't set this issue as fixed:

#ahah['path'] has Used by: button, submit but the other #ahah elements don't have such a list. Is this list complete? Should it be deleted?

And thanks bdragon for getting the ball rolling, and for Peter for taking the time to update the docs.

@beginner - it wasn't clear to me whether this worked with radios (based on form.inc) so I left it off. If it does work (is there an example?) then obviously it should be included

#ahah['path'] does not need such a list - I'll take it off.

#ahah['progress'] was totally undocumented - added a section for that.

By experimentation and examination of the code, #ahah seems not to work for radios or checkboxes (correct me if I'm wrong). So those are excluded.

Attached additional changes committed.

@ #68:

Confirmed by visual inspection.

The #process functions expand_checkboxes() and expand_radios() do not do any ahah handling.

@bdragon - are you satisified with the docs now?

Changed the component to reflect the new component categorization. See http://drupal.org/node/301443