I'll quote part of a comment from #116939: Add a Cancel link on forms that triggered this one:

Form buttons should be wrapped in an additional container to achieve a consistent styling, but also allow for a "Cancel" link, as contained in confirmation forms (which use additional tweaks to get the styling right). Wrapping form buttons into a parent element 'actions' (or similar) also allows for additional styling, for example, applying CSS Sliding Doors to buttons and links to achieve a more modern look.

(read #43 to #47 in the other issue for the full backstory, it has sun, quicksketch and me agreeing enthousiastically :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

yoroy’s picture

asked around a bit on how this would work, chx and yhahn replied:

currently a form element has no knowledge of its children
we can add a #type 'buttom-wrapper'

and then have $form['buttons']['#type'] = 'button-wrapper';

and put all the relevant submit buttons under that element
and then $form['buttons']['submit'] = array(....)

if you hit system_settings_form, node / add / edit, and a few other you've covered a good portion of core forms
and then you can theme_button_wrapper as much as you want,.

mortendk’s picture

Issue tags: +markupmarines

This would be pretty sweet and we could then remove the "unnecessary" div inside the forms ( http://drupal.org/node/495480)+ would give us some better control over the submit, delete cancel button (if they are put in the bottom of the page)

+1

Stefan Nagtegaal’s picture

I totally agree..
Let's combine it together with #165567: Usability: Float default submit buttons to the right and the other to the left which I submitted ages ago. Let the buttons which reflect the default action of the page ("Save $foo", etc) on the right (when the theme is LTR, or the other way around when it's RTL) and the other buttons on the left (like "Cancel", or "Reset to defaults)'.

sun’s picture

Sorry, but no. Please stay on focus. Form buttons can appear everywhere in a form, see admin/settings/performance as an example. This issue should focus on wrapping form buttons into a container - to allow for consistent styling. If you mix this issue with any other, I can promise you it won't make it into D7.

#theme_wrapper might work out. Maybe it also works with some magic applied, but probably not. (see also example hack in #116939-48: Add a Cancel link on forms)

Ultimately, a consistent wrapper for form buttons will probably depend on developers implementing it (i.e. '#theme_wrapper' => 'form_actions'). However, to do that, module developers might have to rework their form structure - referring to aforementioned hack again:

  $form['preview'] = array('#type' => 'button', '#value' => t('Preview'));
  $form['submit'] = array('#type' => 'submit', '#value' => t('Save'));
  $form['cancel'] = array('#markup' => l(t('Cancel'), referer_uri()));

becomes:

  $form['actions'] = array('#theme_wrapper' => 'form_actions');
  $form['actions']['preview'] = array('#type' => 'button', '#value' => t('Preview'));
  $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Save'));
  $form['actions']['cancel'] = array('#markup' => l(t('Cancel'), referer_uri()));
Jacine’s picture

This would be awesome!

sun’s picture

Issue tags: +API clean-up

Introducing a new tag for feature freeze: API clean-up.

RobLoach’s picture

#557272: FAPI: JavaScript States system introduces a "container" type. So instead, you could have:

  $form['actions'] = array('#type' => 'container');
  $form['actions']['preview'] = array('#type' => 'button', '#value' => t('Preview'));
  $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Save'));
  $form['actions']['cancel'] = array('#markup' => l(t('Cancel'), referer_uri()));
gausarts’s picture

Looking for this feature. Thanks

RobLoach’s picture

zroger’s picture

I like rob's approach of using a simple container.

For another use case...

In jQuery UI dialog, you can specify a buttons object, which the Dialog will display in a consistent area, at the bottom left of the dialog. Having a consistent wrapper would allow you to hide the default buttons and delegate the submission of the form to the jQuery UI dialog buttons. Without a consistent wrapper though, there is no way to do this reliably.

Checkout the d6 version of dialog api (http://drupal.org/project/dialog) for an example of this. In order to achieve this in d6 though, i've resorted to form_altering a class onto the buttons that I want to use.

RobLoach’s picture

Status: Active » Needs review
FileSize
17.37 KB

This adds a container around all form buttons with a class named "form-actions". We need this in, badly. Those lost form buttons drive me nuts. I'm probably missing some buttons here, so some reviews would be nice.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
28.19 KB

yay, thanks!

That was still quite inconsistent though... So let's go with this instead? :)

sun’s picture

FileSize
28.21 KB

This one should fix the exceptions.

sun’s picture

So what's left?

+++ modules/user/user.admin.inc	10 Dec 2009 00:59:00 -0000
@@ -72,16 +72,17 @@ function user_filter_form() {
-  $form['filters']['buttons']['submit'] = array(
+  $form['filters']['actions'] = array('#type' => 'container', '#attributes' => array('class' => array('form-actions')));
+  $form['filters']['actions']['submit'] = array(
     '#type' => 'submit',
     '#value' => (count($session) ? t('Refine') : t('Filter')),
   );

@@ -916,7 +920,7 @@ function theme_user_filters($variables) 
-  $output .= '<div class="container-inline" id="user-admin-buttons">' . drupal_render($form['buttons']) . '</div>';
+  $output .= '<div class="container-inline" id="user-admin-buttons">' . drupal_render($form['actions']) . '</div>';

We want to consolidate extra DIVs like this one. I.e. we can apply the #id and extra .container-inline class to the container's #attributes already :)

Aside from that, this looks very RTBC-ish!

This review is powered by Dreditor.

RobLoach’s picture

FileSize
35.11 KB

To make this change, we needed to add support for the "#id" property....

@@ -72,16 +72,21 @@
     );
   }
 
-  $form['filters']['buttons']['submit'] = array(
+  $form['filters']['actions'] = array(
+    '#type' => 'container',
+    '#attributes' => array('class' => array('form-actions', 'container-inline')),
+    '#id' => 'user-admin-buttons',
+  );
+  $form['filters']['actions']['submit'] = array(
     '#type' => 'submit',
     '#value' => (count($session) ? t('Refine') : t('Filter')),
   );
@@ -916,7 +924,7 @@
   $output .= '</dd>';
 
   $output .= '</dl>';
-  $output .= '<div class="container-inline" id="user-admin-buttons">' . drupal_render($form['buttons']) . '</div>';
+  $output .= drupal_render($form['actions']);
 
   return $output;

I also noticed we were missing some buttons in Locale module, so I added those.

sun’s picture

FileSize
41.72 KB

Merged that last patch with mine, as we worked on this at the same time.

Additionally:

- Removed totally senseless theme functions for filter forms.

- Applied consistent pattern: One-line syntax for 'actions' if it's the regular thing, multi-line if there are special tweaks contained (such as .container-inline)

Furthermore, I double-checked the actual output of most forms.

Looks ready to fly for me.

sun’s picture

FileSize
43.61 KB

Very interesting effect, when you remove a theme function without the registry entry in hook_theme(). Is that a bug elsewhere?

Anyway, this one fixes the notices.

sun’s picture

Assigned: Unassigned » sun
RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Definitely like the clean up of the theme functions. Unless there's anything else missing from this patch, I'm setting this to RTBC.

yoroy’s picture

I'm just glad I opened this issue :-)
Sure hope to see this committed.

sun’s picture

This patch is backed 100% by themers.

Yes, we want. :)

dman’s picture

Nice. This is a sensible improvement

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs themer review, -d7ux-design-question, -markupmarines, -API clean-up

Re-test of drupal.form-actions.20.patch from comment #19 was requested by webchick.

Status: Needs review » Needs work
Issue tags: +Needs themer review, +d7ux-design-question, +markupmarines, +API clean-up

The last submitted patch, drupal.form-actions.20.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
42.76 KB

Re-rolled against HEAD.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Hm. This came after code freeze and is not fixing a critical bug. OTOH, I noticed Dries committed a patch that added such a container to node forms, so I suppose we could do this too, to be consistent....

Ok. Committed to HEAD.

Needs to be documented in the module upgrade guide as a new standard.

sun’s picture

Thank you! This is really exciting and will drastically improve #D7TX! TX? Theming experience! ;)

jbrown’s picture

On admin/content 'Filter' now has a container, but 'Update' does not.

I think buttons at the bottom of a form should have a different class so they can have a margin-top.

Adding margin-top to buttons within the form screws up the layout.

jide’s picture

There may be some more buttons to wrap, searching for $form['submit'] or $form['save'] in all files reveals a bunch of them.
But there may be reasons for those to have been left unwrapped ?

sun’s picture

Nope, not intentionally. It's also possible that forms were changed in the meantime, without knowledge of this new standard here.

TwoD’s picture

Subscribing, how can I help?
Btw, grep -Ern "\['(submit|save|preview|delete|undo|reset|)'\] = " . | grep -Fv -e "['actions']" gives the below list of hits (minus a comment or two). Should all those be wrapped?

TwoD’s picture

FileSize
7.54 KB

Whoa, list was a lot longer than I thought. Attaching instead.

jide’s picture

When that gets in, what do you think about making form['actions'] weight to 100 everywhere by default ?

This has been done for system_settings_form() in #645758: system_settings_form() should set a high weight for buttons, so it would be the good place for that.

seutje’s picture

subscribe

sun’s picture

Status: Needs work » Needs review
FileSize
35.69 KB

I have absolutely no idea how all of those were able to slip through.

Attached patch should fix all of them.

sun’s picture

FileSize
35.59 KB

Sorry, one wrong change in menu.admin.inc.

Status: Needs review » Needs work
Issue tags: -Needs documentation, -Needs themer review, -d7ux-design-question, -markupmarines, -API clean-up

The last submitted patch, drupal.form-actions.38.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
Issue tags: +Needs documentation, +Needs themer review, +d7ux-design-question, +markupmarines, +API clean-up

#38: drupal.form-actions.38.patch queued for re-testing.

sun’s picture

FileSize
36.12 KB

This one should fix the failing tests.

Dave Reid’s picture

Status: Needs review » Needs work

The $form['actions'] keeps getting collapsed onto one line when it shouldn't. It's an array that exceeds 80 characters, so it should be wrapped properly.


Plus theres a few places where it's even changed unnecesssarily:
+++ modules/contact/contact.pages.inc	17 Feb 2010 12:13:12 -0000
@@ -235,10 +235,7 @@ function contact_personal_form($form, &$
-  $form['actions'] = array(
-    '#type' => 'container',
-    '#attributes' => array('class' => array('form-actions')),
-  );
+  $form['actions'] = array('#type' => 'container', '#attributes' => array('class' => array('form-actions')));

Here

+++ modules/path/path.admin.inc	17 Feb 2010 00:22:10 -0000
@@ -122,10 +122,7 @@ function path_admin_form($form, &$form_s
-  $form['actions'] = array(
-    '#type' => 'container',
-    '#attributes' => array('class' => array('form-actions')),
-  );
+  $form['actions'] = array('#type' => 'container', '#attributes' => array('class' => array('form-actions')));

Here as well.

Powered by Dreditor.

sun’s picture

@Dave: See #18 for the reasoning.

Dave Reid’s picture

I see the reasoning but it still doesn't make sense why it needs to violate a coding standard. We don't gain anything by having it on one line.

sun’s picture

Status: Needs work » Needs review
FileSize
36.12 KB

@Dave: AFAIK, there is no such coding standard. The only remotely related is "wrap at 80 chars", but that doesn't necessarily apply to form arrays. I think the one-line notion makes sense in this case, especially provided the reasoning above.

Testbot seems to be stuck in the retest request, so I'm re-attaching the last patch.

Dave Reid’s picture

Sorry sun. I hate to fight about a minor detail of an important patch, but standards matter to me. :/

http://drupal.org/coding-standards#array:

Note that if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level:

RobLoach’s picture

Status: Needs review » Needs work

I've noticed that as an unwanted side effect, this makes it difficult to inject new stuff above the actions.... Check this out:


function mymodule_form_block_admin_configure_alter(&$form, &$form_state) {
  $form['Additional Awesome'] = array(
    '#type' => 'fieldset',
    '#title' => t('Additional Awesome'),
    '#description' => t('Choose how awesome this block is...'), 
    '#collapsed' => TRUE,
    '#collapsible' => TRUE,
    // Not putting this in will place it after the buttons.
    // Putting in -1 will put it all the way at the top of the page.
    // Putting 1 will place it below the buttons.
    '#weight' => -1,
  );
}

Hmmm, maybe we need a consistent #weight that's applied to the action buttons?

sun’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

I agree. Let's come back to the previous patch after ruling this out.

Did I forget anything in here?

sun’s picture

FileSize
31.47 KB

RTBC?

sun’s picture

oh YAY! This revealed a quite critical bug in form API! :)

chx’s picture

Status: Needs review » Needs work

Prior to handling, to is missing.

And http://webchick.net/patch-reviewers-are-not-clairvoyant

jide’s picture

+1 for a high default weight, see #35.

jide’s picture

@chx : something missing in your comment ?

sun’s picture

Status: Needs work » Needs review

#50: drupal.form-actions.50.patch queued for re-testing.

Danny_Joris’s picture

Great idea guys. I'm looking for a way to make a wrapper in D6. I'm using image replacement for buttons quite a lot using this technique: http://drupal.org/node/692782#comment-2508628 . It works great, except is difficult to theme the button positions when their positions have to be set to 'absolute'. This is why I need a wrapper around buttons.

Thumbs up!

sun’s picture

sun’s picture

FileSize
51.74 KB

- Fixed the grammar in that inline comment.

- Fixed previously missing/new occurrences of $form['submit']. Entirely left out test modules though.

Status: Needs review » Needs work

The last submitted patch, drupal.form-actions.57.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: -Needs themer review, -d7ux-design-question, -markupmarines
FileSize
51.77 KB

Fixed that failing test.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Checked a bunch of forms and the wrapper is there. Setting this to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mlncn’s picture

Issue tags: -Needs documentation

Couldn't find the moving the node submit buttons to the actions array committed by Dries (mentioned by webchick in #28), but i think documentation can be covered here.

Added the below to Converting 6.x modules to 7.x.

Form submit buttons consistently grouped in actions array

(Issue) All forms in Drupal now have an actions array to hold the buttons at the bottom of forms relating to submission. Use this in forms provided by contributed modules also. In node forms, this replaces the submit grouping array, which modules implementing hook_form_alter() or hook_form_FORM_ID_alter() to modify, remove, or add to these buttons will have to take into account.

For instance, this is an edited excerpt from node.pages.inc's adding a Save button to the node submission form:

  $form['actions'] = array('#type' => 'actions');
  $form['actions']['submit'] = array(
    '#type' => 'submit',
    '#value' => t('Save'),
    '#submit' => array('node_form_submit'),
  );

Removing Needs Documentation tag but please review this documentation!