In Drupal 7 we imagined that we would use blue buttons whenever there are two buttons shown. This to make the primary one stand out more, and thereby help the user be more efficient. This all comes from the principle of adding appropriate visual weight, by using the 'blue' for the primary save action we allow the user to be a moment faster by determinate the right button not by label or placement, but by color.

There is quite some practice and theory behind this, a good starting resource on this can be found at http://www.lukew.com/resources/articles/PSactions.asp

I have identified a few places where this occurs:

  • admin/structure/types/manage/*
  • admin/structure/menu/manage/menu-*/edit
  • admin/structure/menu/item/*/edit
  • admin/structure/taxonomy/*/edit
  • taxonomy/term/*/edit/structure/taxonomy/*
  • user/*/edit

This is dependent upon #1238124: The blue 'save' button is gone, bring it back! (or take it away again)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

apaderno’s picture

Status: Postponed » Active
Dave Reid’s picture

I think that we should be able to do something like: div.form-actions > input.form-submit:first-of-type and this behavior is automatic for whichever button is first when using $form['actions']['#type'] = 'actions';

Bojhan’s picture

FileSize
856 bytes

ok first try

David_Rothstein’s picture

seutje’s picture

Status: Active » Needs work
+++ b/modules/node/content_types.incundefined
@@ -226,6 +226,7 @@ function node_type_form($form, &$form_state, $type = NULL) {
     '#weight' => 40,
+	'#attributes' => array('class' => array('button-primary')),

we don't indent using tabs

Powered by Dreditor.

Xano’s picture

In IRC we thought of adding a #primary property to button and submit (and possibly image?) elements, adding semantic meaning. In the process callback a CSS class can be added, similar to processing #disabled.

Xano’s picture

Title: When two buttons are shown, make the first one blue » Mark primary form buttons

Also note that Seven is not the only core theme and we may also want to change this in Bartik.

aspilicious’s picture

FileSize
2.84 KB

I made a patch. I'm not sure this is what we all agreed on :).
And I think I put some code in the wrong place. (see comments)

But it works :D

This patch also needs to change the css slector.

aspilicious’s picture

Status: Needs work » Needs review

Needz review

Bojhan’s picture

Title: Mark primary form buttons » Provide a property to mark the primary button (and turn it blue!)
Component: Seven theme » forms system

Gonna change the title, because this sounds more human-readable :)

xmacinfo’s picture

Why adding a “primary” class to the button?

All we need is to have a :first-child target in CSS to make the first button blue.

Bojhan’s picture

Because I am told by many Drupal developers, that a system that assumes makes mistakes. In this case, we only want to do this when their is a visual conflict (e.g two buttons, a submit and delete). Additionally we don't know how this applies to contrib, take for example views - which has two buttons of fairly equal importance on its creation screen, making one the primary will be confusing.

aspilicious’s picture

FileSize
2.39 KB

This should be a better patch

Xano’s picture

Status: Needs review » Needs work
+++ b/includes/form.inc
@@ -3593,6 +3593,9 @@
+    $element['#attributes']['class'][] = 'form-button-primary';

Use "form-control-primary"? That's more flexible and can be used for controls that are no buttons as well.

+++ b/includes/form.inc
@@ -3864,6 +3871,10 @@
+  // Add a class for primary a primary form element.

Comment is not correct. It's a bit redundant IMHO as well.

+++ b/modules/node/node.pages.inc
@@ -270,6 +270,7 @@
+    '#primary' => TRUE,

Default to FALSE?

22 days to next Drupal core point release.

Jeff Burnz’s picture

Subscribe.

aspilicious’s picture

Status: Needs work » Needs review

- Making it #default TRUE means every $form element is primary by default. :s
- That class "form-button-primary" is defined in a button theme function AND it follows the same pattern as the disabled button.

Ok, I agree the comment is incorrect but I still would like more feedback on the code first. Thats why I'm putting this back to needs review.

sun’s picture

Status: Needs review » Needs work

As much as I'd like to see this, I know that we're technically not really prepared for it.

+++ b/modules/node/node.pages.inc
@@ -270,6 +270,7 @@
+    '#primary' => TRUE,

While a custom #property is indeed the easiest way to go, it imposes a *huge* workload (not only on core) to update every single form to include it (at the right place).

The only differentiation between form buttons we currently have is #type 'submit' and 'button' - whereas the latter implies some very weird form API validation and submission logic that almost no one understands.

Aside from that, there's also another issue: According to the HTML spec, browsers, in fact, choose the "closest" submit button in a form, when pressing enter, say, in a text input. The spec actually defines that the browser should choose the closest button to that input. (which is a serious WTF that not even html5 clarified, 'cos there can be more than one submit button having the identical distance to the input...)

In other words: With the coloring, users are going to expect that it's going to be the default form action. But that IS NOT the case.

Sorry, again, for shedding so much dark light on this. I really really wish this would be easier. :-/

xmacinfo’s picture

…when pressing enter, say, in a text input.

I was always under the impression that the default form submit behaviour of a form would not be a button, but the action attribute in the form tag:

<form action="action.php" …>

If you disable JavaScript and the action attribute, pressing enter in a field will not submit the form. We need an action attribute for the event to fire (no JavaScript regular form submit).

With JavaScript the default form submit should be the same with pressing enter in a field or pressing the the type = submit button.

Having more than one button assigned as type = submit input/button is not recommended. All other buttons should be of type=button. When developing web apps for a non-Drupal web app, it was recommended to use a single type = submit and use JavaScript to assign other action to the other buttons.

If any Drupal form have more than one Submit button (attribute type=submit), I think we should fix the forms. So that in the end we would only have a single Submit button (attribute type=submit).

Do we have any Drupal standard way of creating form buttons (submit, reset on button)?

Having a single type=submit button on each form would make is easy to display the button in blue.

Jacine’s picture

Component: forms system » markup
sun’s picture

Only type="submit" is supposed to submit a form. type="button" is supposed to do nothing. That's 100% following the spec.

In other words: You can't submit a form without input type="submit". type="button" is pretty much senseless in the scope of Drupal.

JS-driven form buttons don't make any sense in Drupal, since buttons like "Delete" or "Preview" actually have to invoke server-side callbacks, regardless of whether JS is enabled or not. Given a 3-5% rate of non-JS users, Drupal core cannot rely on interactions that are only accessible via JS.

Lastly, to clarify it once more: A browser (not Drupal) chooses the "closest" submit button to the currently focused element. As you can probably imagine, these implementations are 100% different. Try yourself by pressing the ENTER key after editing a poll choice in a poll node form, in different browsers.

You might (not sure) experience that the behavior is identical, but if that's the case, then it's because we already committed a very ugly patch that attempts to introduce consistency through custom JS.

xmacinfo’s picture

You can't submit a form without input type="submit".

I believed that was not true. A form can be submitted while pressing enter in a field, provided that the form have an action attribute. So we could basically build a form without any submit button.

I may be rusted since I have not build any form outside of Drupal since a long time ago. :-)

seutje’s picture

@#18

Having more than one button assigned as type = submit input/button is not recommended.

where is this coming from? I've never heard this before, not saying I've been at this for a long time, but the HTML4 spec says otherwise - A form may contain more than one submit button. and the HTML5 spec doesn't even mention multiple submit buttons afaict, letalone them being a problem

@#17

According to the HTML spec, browsers, in fact, choose the "closest" submit button in a form, when pressing enter, say, in a text input.

as much as I agree with that, it doesn't mean we can't use JS to poly-fill the ~95% of users and prevent them from hitting the "remove" buttons on images when hitting enter on some random textfield (cause u know, browsers don't rly follow the spec and just submit using the first button they encounter from the top of the form...)

I feel like there is nothing wrong with trying to fill the gap on things where a technical specification or browser behaviour in general "falls short" on what a user would expect, those ~5% users might have an annoying experience, but it definitely beats having 100% users getting an annoying experience...

JohnAlbin’s picture

Sun's information about "closest button" is interesting. But pre-Drupal when I did "enter to submit" I always noticed that the form's action was taken but no submit buttons were marked as clicked. Anyways, given browser inconsistencies, I think this information should inform (but not dictate) a solution.

We should carefully document are UI recommendation while still being flexible. I suggest the first submit button on the form should be the "primary" one when there are two or more submit buttons. On most forms that means the primary one will be the closest to the rest of the form.

Bojhan’s picture

So what do we do next? Neither sun nor JohnAlbins comment seem to suggest an appproach.

nod_’s picture

Avtually with vertical tabs and all it's usually one of the latest button that's relevant.

So This is more or less needed for #1510532: [META] Implement the new create content page design no?

mrfelton’s picture

Title: Provide a property to mark the primary button (and turn it blue!) » Ability to mark the form buttons to be of a specific type (so that they can be styled differently!)
Status: Needs work » Needs review
FileSize
10.25 KB

Coming from #1510532: [META] Implement the new create content page design (see comment #286) we have come up an approach that gives us the flexibility to not only mark the primary button, but also other buttons - by using the #button_type attribute.

Attached patch does the following (which may or may not be too much for a single patch - I'm tempted to remove a lot of the css and move that to a follow up issue)

  1. Adds additional logic into theme_button that injects new button and button-* specific classes on buttons that have the #button_type attribute set.
  2. Sets #button_type of 'delete' and 'submit' on all EntityForms in EntityFormController.
  3. Fixes the user account edit form so that it uses a standard 'delete' button instead of one called 'cancel' (this was marked as a @todo in ProfileFormController, and is needed to ensure that the delete button also get the button-delete class.
  4. Adds basic button element styling for seven. Currently, it styles the buttons in the blueprint-esque styles as per the mockups in #1510532: [META] Implement the new create content page design. In some ways it may make sense to remove a lot of that css from this patch and do that in a wider Seven style update patch (point #5 above), although that would mean we would need some temporary styles for the new buttons and given that the end goal is to make them look like they do in that thread I'm not sure there is merit to that approach or not.

The result of all that is the ability to use #button_type to give form buttons special classes such as 'button-primary', 'button-delete' etc, some nice new classes on entity form submit buttons (primary submit buttons get 'button-primary' and delete buttons get 'button-delete'), and some styling in the seven theme to make use of the new classes so that form buttons are styled nicely (blue for submit, red for delete).

mrfelton’s picture

Updated patch does away with all of the fancy blueprint styling, and just implements a basic blue for primary and red for delete. The blueprint styling for Seven can be handled in a follow up.

Bojhan’s picture

I would keep from adding the red, thats not part of any concept so far.

Could you add a screen?

mrfelton’s picture

FileSize
33.86 KB
51.36 KB

Attached is a screenshot with just this patch applied. The red came from #1510532: [META] Implement the new create content page design (eg http://drupal.org/node/1510532#comment-6381930) - it is part of the concept for the improved node add page. If it is going to be done there, then it should be done for all delete buttons.

Standard:
Standard

Mobile:
Collapsed for mobile

Status: Needs review » Needs work

The last submitted patch, 1238484-27-button-type.patch, failed testing.

xmacinfo’s picture

I like the gray and blue button. But I hate the red button. We do not need that much colors. And color blind users may not see any difference here.

I would stick with the original plan. But nevertheless, having the ability to colors each button in a different color could be a nice addition for some specific cases.

mrfelton’s picture

Status: Needs work » Needs review
FileSize
4.44 KB

@xmacinfo, if you don't like the red you might want to raise your hand in #1510532: [META] Implement the new create content page design and say so, because it looks like that is the way it's going. In any case, I have removed the red styling from this patch, since it's unrelated to this issue, though I have left the button-delete class on it.

Xano’s picture

//Edit: @mrfelton beat me to it. I left the red in, but fixed the testing errors (the patch changed the "form-button-disabled" class to "button-disabled". This new class wasn't used anywhere in core, but did break the tests)

mrfelton’s picture

Ok, here is the same again, but with revised class names for the buttons. Now we are using form-button and form-button-*, which is more consistant with the naming of other form elements such as form-button-disabled, form-submit, form-email, form-file etc etc.

mrfelton’s picture

Oh, and I removed the red!

mrfelton’s picture

Updated patch removes some extra css that was added for a new .form-actions-group class. That class is not added or used here, but is one that we will be adding in the NodeFormController as part of an additional follow up patch / spin off from #1510532: [META] Implement the new create content page design. I'm removing it from here, as otherwise we'll end up with patch dependencies which will slow down testing.

kika’s picture

Title: Ability to mark the form buttons to be of a specific type (so that they can be styled differently!) » Ability to mark the form buttons to be of a specific type (so that they can be styled differently)

Now with way less "!". Great job @mrfelton -- will review when I make it home.

kika’s picture

I have mixed feelings towards #delete. It's definitely needs to happen but in current state in tends to freak out people when coloured #1510532: [META] Implement the new create content page design-stylee and so far we do not have a good pattern to style AND place those "dangerous" links properly. "Red URL"-like approaches are suggested but that has to yet to reach consensus.

I'd say rip off all #delete stuff for now and let's make it a follow-up issue (either new or continuation of this one). So we can move #primary onwards without clobbering it with discussion about approach to "dangerous / secondary" buttons. Or we are going too too crazy with this splitting thing?

Sidenote: there was an issue about converting all admin links to a proper semantic <button> element and style them as links - this might be related to this issue but I can not find it. @bojhan -- care to help?

mrfelton’s picture

Latest patch only includes the form-button-delete class. It doesn't do any styling, so should be no controversy there. If people want to style the delete buttons in their own themes they are free to do so.

Bojhan’s picture

That is http://drupal.org/node/1719640, thanks for ripping out the delete styling.

From UX point of view this is good to go, can we get some more markup reviews?

kika’s picture

Status: Needs review » Needs work
+++ b/core/themes/seven/style.cssundefined
@@ -714,6 +713,26 @@ div.filter-options select {
+.form-button-delete {
+  float: right; /* LTR; @todo RTL */
+  margin-right: 0; /* LTR; @todo RTL */
+  width: auto;

Do we need it when we plan introduce delete button styling in "stealth style"? Placement is as visual as styling the button itself.

nod_’s picture

Status: Needs work » Needs review

If we're talking about inputs/buttons this css isn't needed:

+.form-button:link,
+.form-button:visited,

This rule seems kinda expensive for what it's supposed to be doing. Can't we go with input at least?
+.form-actions > * {

nod_’s picture

Status: Needs review » Needs work
mrfelton’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

Thats a good question. Perhaps not. I guess that styling could be moved to #1751754: Implement new form style for Seven, based on blueprint mockups. really. Updated patch attached.

mrfelton’s picture

Alterations to css based on @nod_'s comments

aspilicious’s picture

1)
This needs work for RTL. I don't want this /* LTR; @todo RTL */ in core.

2)

-a.button {
+.form-button {

This changes all the styling of all the links with a button class. I don't think we want to do that. I think the field_ui cogwheel is such a button (but I can be wrong).

nod_’s picture

I did not find anything matchin a.button in core when I tried to look for it (for an other issue as well).

kika’s picture

Adding 'needs accessibility review' tag.

kika’s picture

Issue tags: +d8ux

Even more tags.

attiks’s picture

Status: Needs review » Needs work

I like the idea, but see #46-1

Random thoughts:

  • Is there some documentation so module developers now how to use this?
  • Will this work with AJAX based forms adding new buttons? Isn't there a risk of ending with to 'primary' buttons?
+++ b/core/modules/user/lib/Drupal/user/ProfileFormController.phpundefined
@@ -21,16 +21,10 @@ class ProfileFormController extends AccountFormController {
-    $element['cancel'] = array(
-      '#type' => 'submit',
-      '#value' => t('Cancel account'),
-      '#submit' => array('user_edit_cancel_submit'),
-      '#access' => $account->uid > 1 && (($account->uid == $GLOBALS['user']->uid && user_access('cancel account')) || user_access('administer users')),
-    );
+    $element['delete']['#type'] = 'submit';
+    $element['delete']['#value'] = t('Cancel account');
+    $element['delete']['#submit'] = array('user_edit_cancel_submit');
+    $element['delete']['#access'] = $account->uid > 1 && (($account->uid == $GLOBALS['user']->uid && user_access('cancel account')) || user_access('administer users'));
 

any reason this went from an array to separate lines?

ry5n’s picture

Do we need change these classes to form-button?

I built the original patch using a SMACSS/OOCSS approach (here's a great intro to the rationale from Nicholas Gallagher). From that perspective, a .button is anything that looks/works like a standard button in the design system, whether it's in a form or not. If there is a form-specific variant of our default button, it would get something likeclass="button button-form". I do understand the worry (@aspilicious in #46) about styles affecting unrelated UI elements, and I appreciate that there are other constraints here (like the rest of Drupal!). But, all other things equal, .button *should* make anything look like a default button – in fact, that should be its explicit purpose.

Things that don't look like standard buttons should not have the button class, even if they are button-like UI elements. If I were starting a brand-new project, I might implement buttons and cogs like so: <button class="button">Save</button> and a <button class="dropdown-toggle"><i class="icon icon-cog">configure</i></button>. If I've done things right, I could mix-and-match, later creating a button that toggles a dropdown menu with <button class="button dropdown-toggle">More</button>. And yes, the button with a class of button is intentional :)

@attick: yes, this should be documented but let's get it built first! Also, I don't know how much we can do to prevent two primary buttons and the like. They're just classes, and this kind of UI detail is up to module devs I think (?). We should be able to avoid such problems in Core modules. Finally, the PHP coding standards don't seem to have an opinion on writing FAPI/Render API arrays (?) but the single array declaration looks cleaner to me.

As noted in #46 we can't commit this without adding RTL styles. I will try to work on this patch this weekend.

mrfelton’s picture

The reason why I used single lines for the array, was because we are not creating a new array, but modifying an existing one - $element['delete'] already exists. We are just changing it's type and a few other properties.

Regarding the RTL stuff, I have never used an RTL system, so have no idea what is supposed to happen there. If someone else could do that bit that would be great.

I didn't use .button as that would be inconsistant with the rest of core. Core prefixes classes like this with form- (form-submit, form-item, form-type-radio etc).

attiks’s picture

#52 You're absolutely right, I missed the removal of these lines

-    // @todo Actually the cancel action can be assimilated to the delete one: we
-    // should alter it instead of providing a new one.
-    unset($element['delete']);
ry5n’s picture

Status: Needs work » Needs review
FileSize
5.67 KB

New patch:
- Added RTL styles and removed the @todos
- Added back styling for the .form-button-primary class which seems to have been lost along the way.
- Instances of a.button have been checked and changed to the appropriate class (usually .form-button).

nod_’s picture

Status: Needs review » Needs work

Looks good to me and could help with solving #1797438: HTML5 validation is preventing form submit and not fully accessible (attaching a JS behavior to a button type to bypass HTML5 validation).

I'm happy to see the a.button rules go away (they are not used anywhere).

I'd RTBC it if the styles in bartik didn't looked like crap. Open up the comment form, it's the 90" :) Beside that it's good to go for me.

Maybe one tweak could be replacing:

    $element['delete']['#type'] = 'submit';
    $element['delete']['#value'] = t('Cancel account');
    $element['delete']['#submit'] = array('user_edit_cancel_submit');
    $element['delete']['#access'] = $account->uid > 1 && (($account->uid == $GLOBALS['user']->uid && user_access('cancel account')) || user_access('administer users'));

by

    $element['delete'] = array(
      '#type' => 'submit',
      '#value' => t('Cancel account'),
      '#submit' => array('user_edit_cancel_submit'),
      '#access' => $account->uid > 1 && (($account->uid == $GLOBALS['user']->uid && user_access('cancel account')) || user_access('administer users')),
    ) + $element['delete'];

But that's minor :p

nod_’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
7.63 KB

Ok rerolled without the "tweak". Straightforward is good.

The style in bartik is from the new millennium now.

Bojhan’s picture

Tempted to mark this RTBC, given nod_ mentioning its pretty much ready. Any other reviewers?

attiks’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -96,6 +96,12 @@ protected function actionsElement(array $form, array &$form_state) {
+      $element['delete']['#button_type'] = 'delete';
+    }
+
+    if (isset($element['submit'])) {
+      // Give the primary submit button a #button_type of primary.
+      $element['submit']['#button_type'] = 'primary';

Question about this code: this means that it is not possible to have a delete button as primary? Shouldn't it be possible.

It also means that all primary buttons are styled the same, regardless of their subtype. I think it's better to make 'primary' a separate property ['#primary'] or something like it, this will make it easier for themers to style the buttons, since they always have a class for the subtype and an extra class if it's the primary button.

Leaving at NR, because of comment #17

attiks’s picture

Status: Needs work » Needs review
ry5n’s picture

As a follow-up, but not now, it would be useful to be able to define multiple simultaneous sub-classes on the same button, the best example would be wanting a .form-button-small variant for use in tight spaces. It makes sense that there might be a need for a small primary button, so class="form-button form-button-small form-button-primary".

However, we don't need this for delete buttons in Seven. There are few forms where the delete action is also the main action, and when that's the case we don't need a separate visual cue. Just give it the standard delete button appearance, so just class="form-button form-button-delete". The biggest realisation I've had around this is that classes don't and should not describe content semantics, they should only describe visual semantics. We can save ourselves a lot of grief by thinking that way.

ry5n’s picture

Just a quick clarification: when I built the giant patch for http://drupal.org/node/1510532 I didn't run into any situations that desperately needed multiple button sub-classes, so it's not just delete buttons I'm referring to when I suggest we can handle this in a follow-up. I believe the approach here is sound for now and we should get it RTBC'ed and move on ASAP.

sun’s picture

Are the screenshots in #29 still current? Looks like a lot has changed since then?

But more critically:

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -96,6 +96,12 @@ protected function actionsElement(array $form, array &$form_state) {
+      $element['delete']['#button_type'] = 'delete';
...
+    if (isset($element['submit'])) {
+      // Give the primary submit button a #button_type of primary.
+      $element['submit']['#button_type'] = 'primary';
     }

#button_type is a property internally used by Form API to validate and process form submissions.

Form API groups all buttons into $form_state['buttons'] keyed by their #button_type, and the two 'submit' and 'button' stacks are later used for advanced processing.

I'm not sure whether it is safe and appropriate to re-use/abuse #button_type for visual frontend styling. I'd have to dig deeper into the call chains and usage of the #button_type property. Would be good to get an additional verification from @effulgentsia.

sun’s picture

Assigned: Unassigned » effulgentsia

Hrm. Apparently, grepping core for #button_type only yields values of 'submit'. And even the Form API reference documents it as "CSS class". However, various spots in form.inc and ajax.inc use it as a condition for functional form submission processing, so it would be anything but safe to set #button_type to, say, NULL (because you want to remove the "CSS class"). I'm not sure what happened there. Hopefully @effulgentsia knows more.

ry5n’s picture

@sun I was about to post the same thing, minus the grepping around. I read the docs before I came up with this approach and… um… yeah let's get @effulgentsia to weigh in. We can't use this with confidence if it's tied to form processing.

ry5n’s picture

By the way, this is what the patch looks like visually, now.

1238484-narrowscreen-states
1238484-widescreen-plus-states

effulgentsia’s picture

Sorry I haven't responded here yet. I will in the next day or two.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

So some day way back when (Drupal 4.7, IE6 days), system_elements() (the precursor to system_element_info()) set $type['submit']['#button_type'] to 'submit' and $type['button']['#button_type'] to 'button'. Back then, I think we also had theme_submit() rendering <input type="submit"> and theme_button() rendering <button>, but then changed theme_submit() to call theme_button() and changed theme_button() to render <input type="$element['#button_type']">, so we stopped using <button>, but still distinguished <input type="submit"> from <input type="button">. Per #62, we partitioned the processing of these buttons in $form_state, and also had a workaround for IE sometimes not posting which button was clicked, in which case we assumed the one clicked was the first 'submit' button, but not the first 'button' button.

At some point, we stopped doing all that, so now, all we output is <input type="submit">. So now, all we use #button_type for (other than CSS) is to know whether an element is a button (rather than writing in_array($element['#type'], array('submit', 'button', 'image_button'))). That means that in principle it's okay for this issue to start using #button_type for front-end needs. But, I'm concerned about this potentially leading to a situation where someone accidentally sets #button_type on an element that's not a button: that will then mess up form processing. If others share this concern, we should either find a new property name to use for this issue's purposes, or else change our remaining form processing code to use different logic (e.g., a new #is_button boolean property) for knowing if an element is a button.

Related to that, I find $type['button'] useless, and would support removing it entirely. All it is is something that's a 'submit' button as far as HTML is concerned, but something for which we do not run submit handlers server-side (because #executes_submit_callback is FALSE). This doesn't really make any sense: I think it's a left over from when we actually were dealing with <input type="button">. Also, any button can set #submit to an empty array, making #executes_submit_callback itself a uselessly redundant API.

ry5n’s picture

@effulgentsia Thank you for the detailed reply.

It sounds like ideally we would remove the button_type property and use something else for this patch. My main worry with that is the risk of further delay. This is one of a number of issues aimed at bringing the improved content creation page into core, and this particular issue needs resolution well before feature freeze if that's going to happen in an organized way.

On the other hand, I'd rather not build new features on top of what is essentially cruft.

So, I'm unsure. One thing does occur to me: since this is mainly a front-end concern, should there be a custom property for this at all? Can/should we just use [#'attributes']['class']?

mrfelton’s picture

> So, I'm unsure. One thing does occur to me: since this is mainly a front-end concern, should there be a custom property for this at all? Can/should we just use [#'attributes']['class']?

Well, it is a lot about the front end - being able to style the buttons differently. But, it's also about semantically describing the purpose of the button within the FormAPI representation of the form, which could in theory be used to determine other properties of the button or how the button behaves (although in core nothing else about the button changes, other than the style).

effulgentsia’s picture

There are two things I have occasionally thought would be useful to do for FAPI buttons:

- One is to better separate #value from button label. It's frustrating to see code like if ($op == t('Cancel')) in core or in contrib, where the operation value is translated, because the button label needs to be translated. It also means a hook_form_alter() that just wants to relabel a button also implicitly affects form processing. I don't know if the better approach is to introduce something like #title and use that for label (i.e., '#value' => 'cancel', '#title' => t('Cancel')), or to introduce something like #operation (i.e., '#value' => t('Cancel'), '#operation' => 'cancel'). Normally, I'd say the first is clearly better, but that would disconnect #value from HTML's value attribute, which is its own WTF. Anyway, the details of this can be for a separate issue: just putting the idea out there.

- The other is to introduce the concept of a "default" button, which would be the one presumed to be clicked when someone hits ENTER in a text field, and for programmatic/scripted form submissions that don't specify the button that was clicked. I'm pretty sure we have in issue in the queue for this already, and will post a link when I find it. Views actually has a custom implementation of this already (search "defaultButton" in #1805996-30: [META] Views in Drupal Core) which would be great to abstract.

The reason I mention the above is that I think both of those relate to #69: semanticly describing buttons for both front-end use and back-end use, and the specific semantics of both of those seem related to what is wanted for front-end use in this issue.

So, how about this?
- Make this issue not use #button_type, and instead use #attributes['class'] directly.
- Open a follow up to retire #button_type.
- Open follow ups for the value and default semantics I mention above, and when those are resolved, have them add the corresponding HTML classes.

Is that a reasonable way to unblock #1510532: [META] Implement the new create content page design in the short term, but address #69 long term?

mrfelton’s picture

@effulgentsia - for the most I agree, and think that what you propose is reasonable and would provide a way to progress this issue and get something committed, by moving the discussion of the other related points elsewhere.

One question I have though - are there other places in core where specific css classes are added to specific elements purely in order to aid styling, outside of the job of semantically describing those elements? I thought that generally classes are added to elements automatically because those elements carry some specific semantic purpose. Not the other way around.

nod_’s picture

We're dropping IE6/7 we can now use button to separate the label (or HTML or anything inside the button element) and it's return value.
<button type="submit" name="op" value="save">Enregistrer</button>

And you get in the post, $_POST['op'] == 'save'.

That this won't work in IE if it's in the IE7 compatibility mode but since we're dropping that we should just get rid of input type="submit" entirely. anyway, We'd be breaking IE6/7 real bad in that case, as in totally broken :p

sun’s picture

Simply using #attributes][class was my first consideration, too. However, one of the possible purposes here would be to allow modules/themes to potentially change the assigned "default type" of a button. That makes a relatively clear separation from (arbitrary) CSS classes.

I'd therefore suggest to simply move forward with a new #button_class property.

That unblocks this issue, and you can leave the fix/removal of the other Form API mess to Form API maintainers. :)

To clarify on existing issues:

re #70.1a: #852520: Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels without having to manually specify a unique #name

re #70.1b: #116939: Add a Cancel link on forms

re #70.2: Stunningly, I'm not able to find that either. Was related to Poll module, IIRC. Did we fix it already, mayhaps?

re #72:
#1719640: Use 'button' element instead of empty links
#991774: Change confirmation form's "Cancel" link to a button

mrfelton’s picture

Agree with Sun on this. Just assuming for a second that we are back on the wagon with the patch here in #56, are there any other changes that it would need in order to be considered RTBC?

ry5n’s picture

I’d also like confirmation on the next steps. Sounds like we’re sticking with #button_type (existing patch as-is) on the understanding that we follow-up on #70/73 in another issue.

nod_’s picture

This is the issue I want, not the ones linked for me in #73: #1671190: Use <button /> form element type instead of <input type="submit" />.

#70.2 I remember the discussion but can't find the issue either.

ry5n’s picture

BTW, if we’re sticking with #button_type for this issue, the latest patch in #56 looks good. Bartik’s css now styles the new button classes, that was the only issue left.

effulgentsia’s picture

My interpretation of #73 is to change this issue from overloading the existing #button_type to instead use a new #button_class for its purposes and leave #button_type as always saying 'submit', as it does in HEAD, until we retire it. Seems like that should be an easy reroll of #56, no?

ry5n’s picture

@effulgentsia I'm unfamiliar with the inner workings of the Form API: is adding a new FAPI property as easy as just using it (plus docs updates)?

sun’s picture

Sorry for not emphasizing it, but I actually recommended:

move forward with a new #button_class property.

i.e., leaving #button_type intact. At least, I think that's the most safe and most reasonable approach and direction for now.

That will require some adjustments to the patch. Aside from that, I think the color scheme (at least the one visible in #65) could use some more visual tweaking for the individual states - I wonder whether the :hover/:focus state shouldn't actually get lighter instead of darker? But then again, we could also leave further design adjustments to a follow-up issue...

sun’s picture

@ry5n: Yes, custom #properties can be assigned nilly-willy, as long as they do not conflict with formal/existing/other properties. #button_class does not conflict with anything else, to my knowledge.

+++ b/core/includes/form.inc
@@ -3941,7 +3941,13 @@ function theme_button($variables) {
+  if (!empty($element['#button_type'])) {
+    $element['#attributes']['class'][] = 'form-button-' . $element['#button_type'];

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -96,6 +96,12 @@ protected function actionsElement(array $form, array &$form_state) {
+      $element['delete']['#button_type'] = 'delete';
...
+      $element['submit']['#button_type'] = 'primary';

AFAICS, these would simply be renamed.

ry5n’s picture

@sun Aha, that's great. I can probably re-roll this myself, then.

Also: I feel the same about the actual button styling. From a graphic communication perspective, light colors 'advance' and dark ones 'recede' from the visual plane. For that reason, focus/hover states that darken the item drive me crazy (looking at you, Twitter Bootstrap). But as long as we get the new styling from the content creation page into D8, they will override these.

mrfelton’s picture

Ok, so here is the patch from #56 but using a new #button_class attribute instead. Also, I don't see any reason for the buttons.png file in Seven (which was used to provide a sprite containing 3 flat shades of grey used for the grey button states). Unlike the buttons.png file in Bartick, there are no fancy gradients or anything in that image that couldn't be done cross browser with pure css (although I'm not sure on Core's policy towards gradients... can they not also be done with css only these days? Does Bartik really need a background image for the buttons?). So, I removed the buttons.png file from Seven, and replaced with simple background-color properties. The result is same, with one less image to load.

I did have a go at messing with the colours and the hover state light vs dark etc, but it didn't really seem to work to me. It seemed confusing because for example, when you hover over the vertical tabs they go dark, so you would expect that when you hover over the buttons they also go dark. Should be consistant one way or the other, and I don't think we need to get into that here, considering there is a whole new style on the way anyway.

ry5n’s picture

@mrfelton You’re faster than me :)
Just tested this, it works. I’m in favor of removing that image, too.

mrfelton’s picture

Previous patch didn't consider image buttons (theme_image_button). Updated patch does, ensuring that they also get the same set of classes. Also, there was no mention of the new #button_class in the doc string for the two theme functions. Added that too.

ParisLiakos’s picture

+++ b/core/includes/form.incundefined
@@ -575,7 +575,7 @@ function form_state_keys_no_cache() {
-/**
+/**f

oopsie

besides that..this seems rtbc

ParisLiakos’s picture

patch that fixes #86.
I also tested it a bit in firefox and chrome everything looks as supposed

ry5n’s picture

Status: Needs review » Reviewed & tested by the community

@rootatwc I’ve read and tested the latest patch. Looks good to go. Tentatively marking RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

My initial reaction looking at #button_class is "ewww" because it implies we have other properties like #textarea_class and #radios_class and whatnot, which we do not. $element['#attributes']['class'] is the standard way of extending classes on elements, and I do not understand why we think buttons should be special. I've tried to read this issue very carefully, and don't quite understand how we landed on this as a solution.

Up in #70, effultentsia said:

"So, how about this?
- Make this issue not use #button_type, and instead use #attributes['class'] directly.
- Open a follow up to retire #button_type.
- Open follow ups for the value and default semantics I mention above, and when those are resolved, have them add the corresponding HTML classes.
"

That makes total sense to me.

in #71 mrfelton counters with a question about whether we use classes elsewhere to convey both semantic information (this button is "primary") as well as styling information ("and it should be blue"). I would argue that yes, we do. For example, .element-invisible is both a visual change ("don't show anything visual") and a semantic change ("but DO show it to screen readers").

Then sun in #73 suggests:

"Simply using #attributes][class was my first consideration, too. However, one of the possible purposes here would be to allow modules/themes to potentially change the assigned "default type" of a button. That makes a relatively clear separation from (arbitrary) CSS classes."

So I get that, but to me that sounds like nothing more than a helper function or so that just auto-sets #attributes['class'] to one of N pre-defined values. I like this better than willy-nilly attaching:

+    $element['#attributes']['class'][] = 'form-button-' . $element['#button_class'];

...because this allows contrib to make up their own cracked-out standards for what button types there should be. While that should be possible (just like contrib can introduce an .element-invisble-oh-but-not-quite class), I believe that we're better served by core laying out a standard set of button types that contrib is expected to conform to in 98% of cases.

Can we discuss this a bit more?

sun’s picture

a helper function or so that just auto-sets #attributes['class'] to one of N pre-defined values

In order to do that, the class in #attributes['class'] that is meant to be the #button_class would have to be always and consistently set with a defined array key like so:

  '#attached' => array('class' => array(
    'nilly',
    'willy',
    'button' => 'delete',
  )),

That's impossible to rely on.

because this allows contrib to make up their own cracked-out standards for what button types there should be.

That's an explicitly wanted aspect and feature of this. Drupal core, by design, is not able to predefine all possible button classes.

There will always be the fallback to just .form-submit or .form-button, in case a theme is not aware of a certain button class. That will inherently yield the exact same look and feel as we have today; simply no differentiation.

That does not mean that modules should not properly classify their buttons though. Instead, it means the opposite: Everyone classifies appropriately, and the theme decides what is being done with that. Core has no business in decisions there.

Again, this is an explicitly wanted design aspect.

webchick’s picture

K, then it seems like we should name this attribute #button_priority or something (since we can't call it #button_type, as that has another meaning in the current FAPI) since "class" implicitly implies "CSS class" and you are distinctively de-coupling those by design. Curious what Alex's take is.

sun’s picture

err, no, I'm not really decoupling them. It is still a class, and it is still a CSS class.

The difference is that we only ever want one and exactly one of all possible #button_class classes on a button. As such, it is a class of a certain type, which in and by itself can appear in infinite variations. But it should only appear once on a single button.

If a technical backend equivalent is needed, then imagine 1) a taxonomy term field on a node, 2) the vocabulary consists of unlimited entities, but 3) the widget should only allow to select one. Thus, you cannot use the free tagging widget for it (the equivalent of #attributes['class']). You need to restrict the field values to single cardinality and use a (single) select widget. That is what #button_class is.

mrfelton’s picture

button_class does have the word 'class' in it, and it does result in a css class being added to the code. The word 'class' is used for more than just css classes. In this case we have a a button of a certain type - or, put another way, one that confirms to a certain class. button_class seems valid to me. button_priority on the other hand is too use case specific. The class of the button could be used to infer more than just it's priority. Really, it's button_type that we want, but this seems to be off the table for some legacy reasons.

effulgentsia’s picture

Really, it's button_type that we want

Agreed. This patch changes it to #button_type, and per #67, introduces a new #is_button for what #button_type used to be.

Bojhan’s picture

It looks like we all reached consensus on this, can anyone review it so we can bring it to RTBC?

@effulgentsia Thanks for working on this! @sun, @mrfelton for the reviews :)

attiks’s picture

FYI Applying patch works but is giving offsets

Hunk #2 succeeded at 1101 (offset 15 lines).
Hunk #3 succeeded at 1117 (offset 15 lines).
Hunk #4 succeeded at 1256 (offset 15 lines).
+++ b/core/themes/seven/style-rtl.cssundefined
@@ -154,6 +154,19 @@ ul.action-links a {
+@media screen and (max-width: 37.5em) { /* 600px */

+++ b/core/themes/seven/style.cssundefined
@@ -806,6 +818,23 @@ div.filter-options select {
+@media screen and (max-width: 37.5em) { /* 600px */

question: config/seven.breakpoints.yml is using 'screen and (min-width: 40em)', so why are you using 37.5em here?

ry5n’s picture

@attiks Any media queries aren't really related to this patch. The ones you cite are were probably not cleanly split out from the meta issue (#1510532: [META] Implement the new create content page design), and in any case are already being overridden by lines 787 to 795 in Seven's style.css.

@media screen and (max-width: 600px) {
...
  .form-actions input,
  .form-wrapper input[type="submit"] {
    float: none;
    margin-right: 0;
    margin-top: 10px;
    padding-bottom: 6px;
    padding-top: 6px;
    width: 100%;
  }
}
ry5n’s picture

This patch the removes the styles that make buttons full-width for viewports narrower than 37.5em. My intention here is to streamline the patch for the core feature so we can get this committed.

The mobile styles should be added back later as part of follow-up and (presumably) use configurable breakpoints.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Good for me, can you link the follow up in this issue.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Excellent patch! Unfortunately though I think this probably needs some minimal test coverage.

chx’s picture

Status: Needs work » Needs review
FileSize
15.11 KB

As part of the "we love our frontend developers and help them wherever we can" initative, here's a test. It's not a lot.. but it does the job. And, there's no preg. That's a plus. Never parse HTML with preg, remember :)

sun’s picture

I reviewed this patch once more in-depth, and wasn't really happy with the proposed changes yet. However, I also have some ideas that might help us to resolve this issue. I'm happy to help to codify what I'm suggesting. Also, while I think I followed this issue since its beginning, I might have missed (or forgotten) earlier comments, so please bear with me if I'm duplicating details that have been hashed out earlier already.

+++ b/core/includes/form.inc
-  $element['#attributes']['class'][] = 'form-' . $element['#button_type'];
+  $element['#attributes']['class'][] = 'form-button';
+  if (!empty($element['#button_type'])) {
+    $element['#attributes']['class'][] = 'form-button-' . $element['#button_type'];
+  }
+  else {
+    $element['#attributes']['class'][] = 'form-submit';
+  }

+++ b/core/themes/bartik/css/style-rtl.css
 input.form-submit,
-a.button {
+.form-actions .form-button,
+.form-actions .form-submit {

I have a few remaining problems with this:

1) The new selectors are overqualified.

2) .form-button actually duplicates .form-submit in many selectors, because .form-button is the same element as .form-submit.

3) The backend code always applies .form-button now. .form-submit is only applied if no #button_type has been specified. Otherwise, .form-button-[type] is applied. I'm questioning why we need .form-submit at all, and/or, why the classes are so inconsistent (i.e., why not .form-button-submit instead of .form-submit?).

4) The generic a.button styles are removed from Bartik here, but those were actually a step forward towards generic button styles. As such, the changes here are in direct conflict with my changes in #1167350-37: Action links were ignored . I'd like us to retain a.button here.

5) Continuing on 4), I don't quite understand why these submit/button classes are prefixed with 'form-' in the first place. I'd like to see a simple input.button and also a.button (whereas the latter is out of scope for this issue — if the current patch wouldn't attempt to remove the styles).

6) Lastly, after looking through the entire patch once more, the #button_type => 'primary' looked actually a tad strange — especially in the hunk that has a #button_type => 'delete' right above it. The latter made sense to me, since it uniquely classifies the button as a "delete" button. But the classification as "primary" did not.

6.1) Uniquely classifying buttons by their "type" is essentially a different goal and topic than classifying buttons by their priority. The issue title indicates that the goal of this issue is the former, classifying buttons by their type. Thus, I think a value of "primary" does not make much sense, and the value should be replaced with "save" or similar.

6.2) If, however, the additional goal of prioritizing buttons is packed into this issue, and we only arrived at the conclusion that we need to classify buttons by type, because we want to prioritize them, then I'm questioning the entire proposed solution.

6.2.1) First and foremost, hard-coding the primary/prioritized button into form definitions will not reliably work for all situations in which it should work. E.g., considering a comment form that has a Save and Preview button. Privileged users can Save directly, potentially turning it into the primary/prioritized button. However, for unprivileged users, the Preview button is the primary button, unless the comment has been previewed at least once, in which case the Save button becomes the primary button. Figuring out which button should be the primary button for all forms and properly codifying that will be a daunting task to perform. On top, form-altering all of those definitions won't be an easy task either (e.g., because your module injects a new button into forms that should be the primary button instead of the default, or, because your theme wants to do things differently).

6.2.2) @attiks already questioned in #58 whether the Delete button on a delete form shouldn't be the primary button, and I agree, that button is the primary operation for a delete form. This excellently demonstrates the conflict between classification vs. prioritization.

6.3) I would think that the prioritization aspect could be solved in a much more simple and automated way: Make Form API automatically classify the first button in a button set (actions) as the primary button. Thus, if you did not preview your comment yet, then the #access to the Save button is FALSE, so the Preview button is automatically the primary button. Likewise, because the Delete button is the first in the delete form, it is the primary button. Is there any reason why this wouldn't work? I think it would additionally help with ensuring a consistent button order across forms.

6.3.1) If that would work, then I'm even questioning whether we'd need any Form API changes at all here, because all we're asking for is this: .form-actions .button:first-child

6.4) Regardless of prioritization, I liked the classification aspect of this change proposal, since I would love to be able to target .button-delete in my themes. However, I would also think that this aspect can be solved in a much more simple and automated way: Automatically use the array key for the form button element (last value in #array_parents) as #button_type, unless #button_type is explicitly specified. Thus: we typically have those form button elements in 'submit', 'preview', and 'delete' form array keys — those would be automatically taken over as #button_type (unless manually specified), which in turn would yield .button-[type] classes.

What do you think?

sun’s picture

FileSize
11.37 KB
17.07 KB

Alright. Here's what I propose, based on #102:

  1. Automatically take over the form button's array key in the form definition as #button_type, unless manually specified.

  2. Introduce custom #primary TRUE/FALSE property for #type 'actions'. (Defaults to FALSE)

    The first button within a set of form actions should denote the primary button. But for now, this needs to be limited to only some form actions, because we're using #type 'actions' for many alternative actions within a form/page; e.g., on /admin/content and similar pages, the "Filter" button of the exposed filters is wrapped in a #type 'actions' container. Without the addition of #primary, we'd have too many primary buttons on some pages.

    I'd rather consider this property to be temporary and added a @todo to improve the situation. We should either remove the wrapping #type 'actions' container from standalone/alternative actions in a form, or we should provide a new, specialized #type next to #type 'actions' that allows to denote a set of form actions that are entirely secondary in the first place.

  3. Restored .button, and instead of removing, advanced on it.

    That said, the .form-submit class is still required, because various JavaScript depends on it. This dependency was not clear at all and rather a nasty surprise. ;) I've added a @todo to clean this up, laters. Markup purists will kill us for the extra divitis-alike class, so let's make sure to create an issue for that. :)

Status: Needs review » Needs work

The last submitted patch, form.button-type.103.patch, failed testing.

ry5n’s picture

@sun I'm going to try to address all your points here. Let me first address the more self-contained issues in #102, then move on to the more complex ones.

1) The new selectors are overqualified.

+++ b/core/themes/bartik/css/style-rtl.css
 input.form-submit,
-a.button {
+.form-actions .form-button,
+.form-actions .form-submit {

I think this is actually still stray CSS that is supposed to be for #1751606: Move published status checkbox next to "Save"; we should be able to remove this. Good catch.

Regarding 2) and 3): As you note in #103, the .form-submit class is required for legacy reasons. If we want to change that, it should be a follow-up.

OK, now for the more complex issues.

This issue stems from a simple observation: it's helpful to users to visually distinguish between UI actions. Examples are:

  1. Visually emphasize actions that complete the task at hand ('primary' actions)
  2. Visually distinguish or even de-emphasize actions that destroy stuff ('delete' actions)

(Commenters: this would be a good time to read/re-read the Luke Wroblewski article cited in the issue summary.)

The key thing here is that we are concerned with visual communication, not with a strict classification of button types or priority. We're talking about a design system, or visual language, that we can apply to help users better understand their choices.

If we're going to differentiate buttons, the first thing I want to avoid is sending the wrong signals. As Luke found in his study, people generally had an equal success rate with un-differentiated buttons, but that proper differentiation improved their confidence with the UI. When choosing what to emphasize, the primary action should in general be the action that completes the task at hand, meaning if we are on node/add/article then the primary action is the one that adds the article, in other words, 'save'. However, actions that delete data should never be encouraged by the design system, so 'button-primary' should never be used for delete actions, even if the purpose of the form is to delete something. Again, the goal here is a better user experience, not an abstract taxonomy of buttons or priorities. If we automate this (say by assuming the first button is primary), we could easily end up with the wrong action (even a delete action!) being marked primary, and that would be worse than doing nothing at all. So let's *not* automate it. Differentiating buttons can be entirely opt-in, and nothing is saying we need to do this for every possible form in Drupal.

All of what I've written here is consistent with the patch in #101. I believe it's a simple solution to the problem laid out in this issue and I am not really keen on re-engineering it 3 weeks before feature freeze.

That said, I am not opposed to improving the implementation after feature-freeze. In particular, I'm all for generalizing .form-button to something like .button. But IMO that discussion should be a meta about modernizing Drupal's *entire* front-end architecture, not just one class. That is why this exact issue had classes like .button and .button-primary months ago, and now has .form-button and .form-button-primary.

ry5n’s picture

@sun Also, re: using a value from #array_parents to assign classes. I do not at all like coupling visual semantics to Form API properties that have other functions. (I am not a Form API expert so correct me if I misunderstand the implications).

ry5n’s picture

Status: Needs work » Needs review
FileSize
15.33 KB

Until there's more consensus on this issue, I just wanted to post a patch following on from #101 that cleans up the CSS issues @sun caught in #102. Specifically, this patch:

  • Removes *all* the out-of-scope form-actions layout styles;
  • Removes over-qualified selectors from Bartik's RTL sheet;
  • Adds a missing hover style for Bartik's .form-buttons.
sun’s picture

Well,

  1. I'm OK with temporarily going with .form-button classes here, but only if we do not remove Bartik's existing styles for a.button, because that's a regression and a move into the wrong direction from my perspective.
  2. We need to take over the @todo and the unconditional addition of the .form-submit class in form_process_button() from #103, since, as the @todo clarifies, the .form-submit class must exist for various JS to work. The latest patch only adds it under certain conditions, which breaks JS. This means that we will always set both .form-button and .form-submit on a form button, but 1) .form-button will be renamed to just .button in a follow-up issue, and 2) the JS dependencies on .form-submit should be eliminated entirely in a separate follow-up issue.
  3. I still cannot make sense of the "dual-purpose" of #button_type — the values "primary" and "delete" have entirely different meanings. A button can be the primary, or not, so that's a Boolean value. The value "delete" does not fit into that concept. Also, there should only be one primary button within a set of buttons. If the only point of this issue is to achieve "primary" buttons, then we should remove the "delete" value for now, IMHO.
  4. To actually prove that the primary button concept works from a backend/form definition perspective, I want to see the actions in the CommentFormController to be converted accordingly as part of this patch — as mentioned in #102, this will reveal the problem that properly determining and defining the primary button within a form definition requires extra logic.
ry5n’s picture

@sun OK, I think we're almost there then. Regarding #108:

  1. I don’t think that's a problem; the a.button class should ultimately be replaced by .button, we agree on that;
  2. Oops, you're right, somehow I forgot that .form-submit was not always added, and yes, we should move to just .button, and drop .form-submit (in follow-up);
  3. I think the catch here is that I am thinking of these as just styling hooks. In my mind, you could 100% legitimately have #button-type 'large' producing <input type="submit" class="button button-large">, whereas I think yourself and others are thinking of these as potentially having other functions in back-end logic. If that's the case, I'm not sure how to proceed, because then 'button-primary' really should only be allowed once on a form (I think). There are lots of other implications which is likely why you're worried :). If on the other hand these are just styling hooks, the only benefit that I can think of for #button_type over ['#attributes']['class'] is that it makes it easier to maintain a standard set of button classes.

    (Also, if it helps I would be completely fine changing .button-delete to something a little more semantic, like .button-danger.)

  4. This is what I'd propose:

    
    protected function actions(array $form, array &$form_state) {
       …
        // No delete action on the comment form.
        unset($element['delete']);
    
        // Make the primary action emphasized, when it appears.
        $element['submit']['#button_type'] = 'primary';
    
        // Only show the save button if comment previews are optional or if we are
        // already previewing the submission.
        $element['submit']['#access'] = ($comment->cid && user_access('administer comments')) || $preview_mode != DRUPAL_REQUIRED || isset($form_state['comment_preview']);
    
        …
        return $element;
      }
    

    I don't think it's an issue to not always have a primary (emphasized) action, especially when there is only one action in total. The 'primary' style should only be applied when there is clearly one action that is the main action, the one that completes the task at hand and isn't some kind of exception (like delete).

Fabianx’s picture

Status: Needs review » Needs work

As per #109 2. back to NW.

In the end I do not care if primary is a boolean or not, but I think to redefine #button_type to have "semantic" meaning (opposed to just providing the class of the parent element) is a good choice.

ry5n’s picture

I've thought about this some more, and although my fundamental notion hasn't changed a lot, I hope the following manages to provide more rigour. So:

  1. I still don't think we can automate this. If we're going to differentiate buttons for a better user experience, a person has to make the decision to make sure the correct button_type is assigned. So it should be opt-in per-form in core, and opt-in per-form in contrib. If it's not clear what button type should apply, we shouldn't apply one.

  2. I agree we could use a clearer definition of what button_type means (its semantics in context of the Form API and its use on the front-end). How about this:

    button_type classifies a button based on its consequences, specifically its consequences in relation to user goals.

    Two values of #button_type are recognized by core themes: 'success' and 'danger'. A 'success' button is the one that completes the main task the form exists to support and where no further interaction is required. 'Success' buttons should be visually emphasised by themes to guide the user to a successful interaction. Multi-step forms should reserve 'success' buttons for their final step. A 'danger' button is a special type, and should be used whenever user-supplied content is deleted, no matter what other button_type may otherwise apply. Themes should de-emphasize the 'clickability' of 'danger' buttons and use other visual signals (icons, colour) to indicate that their effects may not be what the user wants.

    Additional button_type values may be provided by modules and will result in matching html classes added to Drupal's output. However, the button_type property should not be used to apply general html classes to buttons; use ['#attributes']['class'] instead.

I'd like to get input from both Form API experts, especially those with contrib experience, as well as @Bojhan or @yoroy for an interaction design perspective. We can support additional button_types in core if there are use cases for them. These are just the two that are obvious to me.

Bojhan’s picture

I am not exactly sure what to give feedback on, on the automating part? I'd agree that this is something that would be hard to automate, although "Save" probably is a 80% primary case there are plenty cases where its another button.

ry5n’s picture

@Bojhan Thanks for your input.

So what does everyone think about #111 (2)? Does that provide a clear meaning for the Form API property and satisfy our front-end goals? Do we need any other mechanisms here or is this a simple rename of 'primary' to 'success' and 'delete' to 'danger'?

ry5n’s picture

Fixing tags

effulgentsia’s picture

Commenters: this would be a good time to read/re-read the Luke Wroblewski article cited in the issue summary.

One thing I see in reading that article is a clear suggestion to always put the primary/success button first. If that's true, then what's the reason for needing a 'success' type rather than #102.6.3.1?

A 'delete' or 'danger' type seems useful though, especially if its styling should take priority over the primary/success styling. What kinds of 'danger' are there other than 'delete'?

sun’s picture

I seriously have no interest in getting bashed for mentioning it again, but, here:

http://twitter.github.com/bootstrap/base-css.html#buttons


Preventing us from re-inventing all that shit was the entire and only purpose of #1801582: Add a new, non-default framework-based theme to Drupal core, ever. But. literally. no. one. understood.

Fabianx’s picture

#116: Yeah, that is nice! :-)

But isn't that totally compatible with the approach of #button_type here?

Just use: primary, danger for this patch, maybe move #primary button to be the first one always (re-sort or add #weight) and introduce the rest in follow-up?

Seriously: Can we get back to making a new patch here based on #109.2?

Or are there any more general problems with this patch?

effulgentsia’s picture

So, Twitter Bootstrap defines "primary" and "success" as different types and makes "danger" more prominent, not less (#111.2 suggests to de-emphasize danger buttons). Zurb Foundation has no notion of "primary", but has a de-emphasized "secondary". It also has "success", but it has an "alert" instead of a "danger".

So, do we want "delete" buttons as "danger", "alert", "secondary", or what? And do we want to make all single-button forms set their button to "primary", or rather make multi-button forms set all but one to "secondary"?

sun’s picture

ZURB does not seem to follow, know, or adhere to the UI concept/theory being quoted in the summary at all. The fact that they have a button type "secondary" makes that pretty clear, as it is the exact opposite of the concept/theory. Overall, the concept/solution provided by Foundation, emphasizing buttons by default, does not make much sense to me; not only from a UI/design perspective, but also from a technical perspective, especially when taking Drupal's modularity into account (additional buttons in any form would have to be explicitly de-emphasized).

Fabianx’s picture

#118: Bootstrap has "link" to de-emphasize.

But in my opinion it is the task of theme developer to define danger as something emphasized or not emphasized, we just provide the means to add semantic information as one primary categorization besides classes. And I think #button_type is a good variable for that.

And here in Drupal we have chosen to de-emphasize danger as part of this patch, but maybe that is follow-up if there are problems with that?

Edit: The patch does not include the styling as pointed out below and we de-emphasize not danger, but buttons marked with #button_type danger to have them stand out less.

ry5n’s picture

I second #117. We can debate names later. Even adjusting weights can be a follow-up. Let's go with these #button_type for now: a) 'primary' and b) 'danger' or 'delete' (I honestly don't care which).

One thing I will say is we should not switch to doing this with just css, something like .action-links *:first-child. That's bad because we can't automate good UX suggestions, and we want our front-end architecture to be resilient, modular and re-usable, not tying button styles to a particular markup context like that.

About how to treat 'danger' buttons (emphasize or de-emphasize), I don't think there's a rule or clear best practice. My suggetion in #111 is only one idea. We can decide this for each of our core themes, and we can do it later, too.

ry5n’s picture

@Fabianx BTW this patch does not actually style 'delete' or 'danger' buttons at all, just adds a class. That was already decided to be a follow-up a long while ago.

Fabianx’s picture

#122: Lets go with primary and danger for now as they resemble best practices it seems.

Can you re-roll the patch, but fix #109.2?

ry5n’s picture

@Fabianx Working on it.

sun’s picture

Status: Needs work » Needs review

"primary" and "danger" works for me, too, and finally clarifies the concept. :) I still want to see #108 to be incorporated though.

Furthermore, we probably want to document the possible values somewhere (and I'd almost suggest to take over TB's entire list, since that seems to be very sensible) — however, we do not have a good place to for putting such custom property docs currently. I'd suggest to add it to the theme_button() function phpDoc.

ry5n’s picture

@sun Great! I'm incorporating #108 too.

ry5n’s picture

New patch:

- changes #button_type 'delete' to 'danger' ('primary' unchanged);
- ensures that .form-submit is always added;
- retains .form-button as the base class for now, since unifying all buttons across core, including Views, looks bigger than this issue;
- retains a.button in Bartik's css in parallel with .form-button;
- documents the values for button_type that are styled in core themes;
- implements the button_types for actions in CommentFormController as per #108 (4).

Final important change: I realized that image buttons should not get styled by css that targets .form-button, since image buttons are an alternative to styling a button's appearance, and shouldn't get the borders, rounded corners, gradients etc. that might get applied to .form-buttons. So theme_image_button adds .image-button and .image-button-* now, rather than .form-button and .form-button-*. CSS in Bartik and Seven has been adjusted accordingly.

Status: Needs review » Needs work

The last submitted patch, core--form-button-types--1238484--127.patch, failed testing.

ry5n’s picture

Status: Needs work » Needs review
FileSize
19.57 KB

Fix failing tests from changing image_button classes. Let's see if that worked.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Great, works for me, thanks @ry5n! :)

I'd recommend to get this in now and:

  1. if necessary, tweak the primary/danger button styling for Bartik/Seven in a dedicated follow-up issue.
  2. you and me and everyone else interested will follow up with #1845728: Refactor and introduce generic button CSS styles for forms and links ;-)
ry5n’s picture

Woo! I like the look of that issue status :)

As for tweaking the button styles, that definitely needs to be done. #1751754: Implement new form style for Seven, based on blueprint mockups. was the original issue earmarked to implement the new form styles, including buttons. However, I think we should start by designing a style guide for Seven as a whole, and am planning to open an issue for that.

I’m also excited about #1845728: Refactor and introduce generic button CSS styles for forms and links, although we should keep in mind #1804488: [meta] Introduce a Theme Component Library. Buttons could be our very first official Theme Component :)

webchick’s picture

Title: Ability to mark the form buttons to be of a specific type (so that they can be styled differently) » Change notice: Ability to mark the form buttons to be of a specific type (so that they can be styled differently)
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Awesome, great work on this folks!! :D

Committed and pushed to 8.x.

This will need a change notice to inform people of this new property, esp. considering it means something totally different than it did in D7. :)

effulgentsia’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -204,6 +204,9 @@ protected function actions(array $form, array &$form_state) {
+    // Mark the submit action as the primary action, when it appears.
+    $element['submit']['#button_type'] = 'primary';

Minor comment for a follow up: this shouldn't be needed, since the parent class does it.

But, is this the semantics we want for Comments? That "Save" is always primary, and that even in a situation where you only have "Preview", that Preview is not primary? I'm okay with that, but wanted to double-check that that is what's intended.

ry5n’s picture

@webchick Yay! Thanks to everyone who worked on this, reviewers, folks who wrote tests or helped with form API internals. Cheers!

I'll write a change notice.

@effulgentsia Yes, that's the intention. However, it would be great to get a double-check from the UX team that this solution makes sense for forms that have variable buttons in their actions area.

sphism’s picture

I'll just add in my 2 cents. (I realise it's a bit late in the day for this)

Personally I try not to use fixed things like 'primary' and 'danger'

For example, say you have a delete button which is correctly marked as danger - with the matching most dangerous looking button theme.

What happens if you have a button next to it that's more dangerous, like 'Wipe Database' or whatever. Then you'd want to add a 'More Danger' flag or something.

If you take this problem back to it's bare bones then the issue is with the 'Visual Weight' of a thing.

So how about you just have a parameter for '#visual_weight' which you assign a number, eg

  • 2 might be important
  • 1 might be less important
  • 0 might be default
  • -1 might be a little dangerous
  • -2 might be dangerous

But you can still add 5 for super important and -10 for Do Not Click This Button

Also with this being a number and not a string like primary or danger - I wonder if you could tie this directly into SASS, so the #visual_weight is just a SASS variable ??? I'm not sure...

But anyway, then this visual weight could be added to ALL FORM fields and be a really powerful addition to FAPI.

Just my 2 cents

sun’s picture

Title: Change notice: Ability to mark the form buttons to be of a specific type (so that they can be styled differently) » Ability to mark the form buttons to be of a specific type (so that they can be styled differently)
Priority: Critical » Normal
Status: Active » Fixed

Change notice: http://drupal.org/node/1848288

I've included some initial interface pattern guidelines there, based on my own understanding of the concept, which I hope is correct. Happy to discuss + correct in case something is wrong.

sun’s picture

Change notice: http://drupal.org/node/1848288

I've included some initial interface pattern guidelines there, based on my own understanding of the concept, which I hope is correct. Happy to discuss + correct in case something is wrong.

jibran’s picture

I have added screen shot from #65

Bojhan’s picture

Status: Fixed » Active

This needs to be documented in http://drupal.org/ui-standards per gate guidelines.

ry5n’s picture

I've reviewed the change notice and it looks good. API changes are documented and the UI guidelines are right on the money I think, except possibly this:

Only mark a destructive button with "danger" if it appears as an alternative option within a set of other buttons.

I wonder if 'danger' actions should always be styled the same, no matter what?

star-szr’s picture

Issue tags: -Needs change record

Well there have been no objections to the change notice since November, removing tag.

What about the docs mentioned in #139? It looks like http://drupal.org/node/1850248 could really use an update.

xjm’s picture

Title: Ability to mark the form buttons to be of a specific type (so that they can be styled differently) » [docs update] Ability to mark the form buttons to be of a specific type (so that they can be styled differently)
Issue summary: View changes

Clarifying the outstanding task in the title, since this was done a long time ago. :)

mgifford’s picture

Anonymous’s picture

I would like to see this in core soon, it's annoying to not be able to output buttons with the form API (if I'm understanding that this issue addresses that deficiency).

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 6ed4e11 on 8.3.x
    Issue #1238484 by mrfelton, ry5n, aspilicious, Bojhan, Xano, nod_,...

  • webchick committed 6ed4e11 on 8.3.x
    Issue #1238484 by mrfelton, ry5n, aspilicious, Bojhan, Xano, nod_,...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 6ed4e11 on 8.4.x
    Issue #1238484 by mrfelton, ry5n, aspilicious, Bojhan, Xano, nod_,...

  • webchick committed 6ed4e11 on 8.4.x
    Issue #1238484 by mrfelton, ry5n, aspilicious, Bojhan, Xano, nod_,...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.6.x-dev » 8.7.x-dev

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Title: [docs update] Ability to mark the form buttons to be of a specific type (so that they can be styled differently) » Ability to mark the form buttons to be of a specific type (so that they can be styled differently)
Version: 9.4.x-dev » 8.0.x-dev
Status: Active » Fixed

This issue was committed in 2012 and re-opened for an update to documentation. While this is not a bug the Bugsmash Initiative has found that issues that have been committed and re-opened tend to linger and that making a followup so the issue can be closed improves the chances it will be resolved.

So, restoring the fixed status of this, at the time it was fixed, and opening a new issue to look into the documentation.

The follow up is #3280324: Document that forms buttons can be of a specific type

Status: Fixed » Closed (fixed)

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