Button text not changed on form error

The trouble seems to be that if a form error is thrown on preview, the form is not rebuilt. Rather it just uses the cached version and the call to drupal_add_js() is never fired. Here's a solution that puts the script inline right after the button using #suffix. Because the javascript is now actually part of the form, it's guaranteed to be rendered.

Sorry that last patch included cruft from another issue. Here's a clean version.

This is actually an important realization given th future of the Drupal 6 branch and the new AHAH/AJAX capabilities. Do you think that this could be a significant problem for forms? As a developer working with FAPI, do you think there is a compelling case for some sort of intermediate trigger to be fired when the cached copy of a form is re-displayed?

I'm not sure if it's necessary or not. Of course it'd be nice to have something in place just in case, though I'd prefer to use the method included in this patch to avoid having to call the javascript code twice (once on form build and once on cache load). Though I have to admit, I expected the hook_form() to be fired every time, even with the #cache flag set. This could cause some serious problems for modules that need to load a large JS file or add a form-specific CSS file.

After some longish conversations in #drupal, it became obvious that this bug was the tip of the iceberg. Any form that adds custom behaviors via Javascript, and adds the necessary script during its build phase, will LOSE those scripts during previews, form errors, and so on. Basically, whenever the cached version of the form is pulled up, its JS will be left behind.

The simplest solution is to carry the JS information along with the form structure and weave it in at theming/rendering time. Why not just put a custom #theme function on the entire form? The Javascript bits in particular are not related to display but to functionality. Themes should be able to override the appearance of a form like this one without breaking the functionality of the [more] button, and implementing it in a custom #theme function would leave us in that unenviable position.

The solution? Allow $form['#scripts'] to contain a list of script snippets or files to be included once the form is actually rendered to HTML. Each value in #scripts is just an array of parameters to be passed on to drupal_add_js() -- this is the same pattern we follow in $form_state['redirect'], which is passed directly to drupal_goto().

The end result? The script data is preserved even when the form is cached, we DON'T embed hard-coded JS directly into the structure of the form, the scripts themselves can still be changed via form_alter(), and form-level #theme functions don't collide with the JS code.

This patch, as far as I can tell, solves the problem not just for poll module but for other contrib modules that will need JS sugar on top of their relatively complex forms.

Changing title to reflect the real nature of the bug...

You yourself have nicely argued why this needs to be in template_preprocess_form and not theme_form . Patch forthcoming.

So, if you have overwritten theme_form , your theme won't break. Hail to preprocess.

Now it starts by showing "More choices" and it doesn't change.

In that case..

template_preprocess isn't really acceptable for this because it only gets fired when the form is being rendered *as a template*. Should we have both preprocess AND theme_form do this? So as to catch plain old theme_form() calls AND templated forms? Anyone who implements a custom form theme will end up calling drupal_render($form) at the end of their custom function anyhow, so overriding theme_form() in PHP won't kill this functionality.

Also, I spotted a 'whoops' with the jQuery -- I'd killed the document isready bits, which obviously made it less ... worky.

To clarify, Eaton was not fixing core JS but his own copy. I can not really see how could we utilize template_preprocess here alas so this is good as it stands.

I think form-specific CSS is probably still best handled in the form's theme function, or by conditionally including the CSS on the menu callback pages where it's used. I could be wrong, but JS is a bit of a special case. Thoughts, anyone?

Also, the jQuery fix that I mentioned should actually be *in the patch* for it to work. Attached is the proper one. When I moved the script snippet to the #scripts array, I changed the quoting style and broke the jQuery code. Whoops! Returning that line to its original quoting style, and the script works. Yay.

Er, needs review. Not needs work.

Plopping the javascript in theme_form() seems like an unnatural place to me. I think avoiding the theme layer was the entire reason why I didn't just put the javascript in theme_poll_choices(), which would have been a similar approach.

When we did the #ahah properties, we got into a discussion about where they should be added to the page. Since they're adding javascript to the page also, maybe this is a very similar scenario. So *maybe* (really not positive) we should put the addition of javascript to the page in a form_expand() function and add it to the #process array for form elements, rather than putting it in the theme layer.

IMO, javascript is a theme layer gadget, and that *is* the appropriate place for it.

I tend to agree that theme_form() is the best place for this. Even forms with their own theme function use theme_form() to output the full wrapper -- only overriding it with mythemename_form() will bypass this code...

After more testing, I'm going out on a limb and RTBCing it again. The previous version was RTBC'd and this one only fixed a typo.

After the long discussion on IRC I tended to like this idea.
But after test driving this it turns out #scripts cannot be unset or changed in the theme.

So a themer has no choice but to accept the javascript that any such form declaration makes.

Yes, a themer can add their own javascript to 'clobber' the defined javascript. Yes, the #scripts parameter can be changed in a hook_form_alter() - but that is not enough. A themer should be able to override this param and decide which script (if any) gets loaded.

andre

This appears to be based on some confusion about how some of the trickier theming bits work.

Andrew is using the mytheme_poll_node_form() funciton to control how the poll form is rendered. By design, theme functions like that have to call drupal_render($form); at the end to clean everything up and ensure that the form is wrapped correctly.

It's important to remember that form-specific overrides affect the inside of a form and not the form tag itself and its properties. It's that final call to drupal_render() that actually ends up triggering theme_form() itself. hook_form_alter(), or an override of the theme_form() function itself, is the only way to change these types of properties.

This limitation applies to #scripts, #action, #redirect, and any property of the form tag itself. This issue isn't the place to solve the broader question of whether theme functions for specific forms shoudl be able to alter the <form> tag itself. This should be documented, but it's not something specific to this patch.

Also, I apologize... that's Andre, not Andrew :)

I'm not sure I like this change. It's yet another API hook to learn. Why do we insist on caching the form on preview anyway? It's not like it happens a lot.

It's important to remember that form-specific overrides affect the inside of a form

It is precisely the reason why I have a concern with this particular patch in the context of the original issue that lead to discovering that forms lose javascript added during build phase.

Javascript is being added to the POLL form to manipulate a form element inside the form!

It follows then - that if javascript is being added to manipulate a specific form's contents, it should be added in such a way as to allow a form-specific theme function (e.g. mytheme_foo_form()) to override/change it.

Agreed - this is not the place to figure out whether a theme function should be able to override things like #action, #redirect, or #scripts for that matter - but whether such a $form element is the right place to declare javascript.

I'm not sure I like this change. It's yet another API hook to learn. Why do we insist on caching the form on preview anyway? It's not like it happens a lot.

We crossed that bridge a while ago when we eliminated the 'double-building' of forms. Basically, whenever we go through the process of building a form over and over, we keep a copy of it. That means the code in the form's constructor function that does things OTHER than constructing the form won't get called. drupal_add_js() and drupal_add_css() being two examples. I don't think this is an API hook, really -- just a place to put the data you would normally pass into drupal_add_js() to defer it until the rendering stage.

if there are objections to the existence of the #scripts element, I think we need to go the other direction, and simply say that ANY form that adds CSS or JS to control custom behaviors MUST use its theme function to call drupal_add_js() and drupal_add_css(). It's frustrating since overriding any theme functions will inadvertantly blow away JS functionality, and it will make it a good deal more difficult to cleanly add JS effects onto specific forms via hook_form_alter(), but it doesn't require the addition of any new properties.

So I moved it to the theme function, and it's actually LOADS simpler. I feel really silly now. It's also not attached to the theming function for the form itself, rather for the AHAH-controlled portion of the form. Since most AHAH/AJAX forms will by definition require some custom theming this is probably the best solution for now and requires no tweaking.

Please keep in mind that because of http://drupal.org/node/111719, all JS/CSS code in core that relies on HTML IDs is currently broken during form previews. So. That isn't working but it has nothing to do with this issue. ;)

Ah well, I suppose the theme layer will be home to drupal_add_js() for some time to come. In this particular case, I'd agree that the change is purely cosmetic and therefor should be done in the theme, despite it being done via javascript.

I don't think the theme layer will be the proper place for ALL javascript. It's the funny gray area between functionality and appearance. After the original AHAH discussion, we found that the drupal_add_js() for #ahah properties should not be added in the theme layer, because it's a central part of the functionality in the form, and it operates so that it directly modifies the contents of the $form on the server side, through a javascript call. It'd be quite a stretch to say that the adding of the AHAH javascript to the page is a theme layer gadget when it's actually affecting the form. So in that case, #process and and an _expand() function was the correct thing to do. In this case however, the theme layer is appropriate because it's not changing anything about how the form actually works.

So there we have it, no one defined place for javascript. Maybe we'll find a place for it in a separate issue, but as it is #29 solves the problem in an understandable and expected way.

Note: in my test, the patch worked fine but didn't apply exactly. Here's a re-roll, but it has my vote for RTBC.

Latest patch works like a charm.

I think the solution in this specific instance is ideal. It solves the issue and it is now very easy for a themer to override the javascript or disable it altogether. It also sets a good example for this kind of form related javascript tool.

Let's set this back to our previous title, the changes to FAPI don't need to be made for this issue.

Indeed, this looks simple and logical, committed.