This patch adds a generic form dependency system to Drupal. This allows you to specify dependencies of one form element on another in a declarative way like this:

'#dependencies' => array(
  'enabled' => array(
    '#edit-menu-has-menu' => array('checked' => TRUE)
  )
),

This dependency specification means: The form element is enabled when the element with the ID edit-menu-has-menu is checked. Intuitively, you could also specify FALSE which results in the opposite behavior.

#edit-menu-has-menu is a CSS selector, that means you can specify arbitrary elements (they don’t have to be FAPI elements). Each state (visible, enabled, checked, …) of form element can depend on an arbitrary number of other elements. The opposite state can be specified by prepending an exclamation mark. States can be aliased, for example, enabled is just an alias for !disabled. Recursive aliases are possible.

The system is open and module developers and themers can easily expand the available states and implement their own state handlers. Module developers and themers can override actions for single form elements, e.g. if they want to implement a different way of hiding and showing one particular form element, they can do that.

This is a first draft at this system and not yet complete. However, it’s already in a state where it can be reviewed. Code freeze is next week ;)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kkaefer’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Subscribing. This is really very cool. I've noticed that there is an application of this during install.php, seems like another perfect use case for this. Are there any other applications of Select (or other) in Drupal core?

+++ b/misc/dependencies.js
@@ -0,0 +1,286 @@
+var _ = Drupal.dependencies = {};
+_.debug = 'console' in window && false;
+
+/**
+ * Attaches the dependencies.
+ */
+Drupal.behaviors.dependencies = {
+  attach: function (context, settings) {
+    _.initializing = true;
+    _.postponed = [];

Having a "var _" generates some pretty ugly code. Could we namespace this in Drupal.dependencies or something?

This review is powered by Dreditor.

mfer’s picture

/me hits subscribe button.

kkaefer’s picture

@RobLoach: It’s already namespaced to Drupal.dependencies since var _ = Drupal.dependencies, however, when I used Drupal.dependencies throughout the code, it became hard to read

RobLoach’s picture

Issue tags: +Usability, +DX (Developer Experience), +dependencies
FileSize
17.99 KB
  • Moves from straight IDs for the form elements to the form elements name as although the ID might change, we know the name of the form element won't.
  • Renames _ to "dependencies" so that the code looks reasonable.
  • Updates to HEAD now that #505084: Add #attached_library FAPI property for drupal_add_library() is in.
  • Adds dependencies on admin/config/people/accounts in the e-mails section and for Enable user pictures. Seems to have some trouble when applying to multiple form items though.
  • Adds the dependencies on admin/config/regional/settings for the user time zones. Again, has trouble when applying to the parent item. You guys see what's wrong with it?
+    $form['menu']['has_menu'] = array(
+      '#type' => 'checkbox',
+      '#title' => t('Add menu item'),
+      '#description' => t('Check if you want to assign a menu item to this content.'),
+      '#default_value' => !empty($item['link_title']),
+    );

Why are we adding this? Couldn't it enabled/disable the other elements based on if the title is empty?

kkaefer’s picture

FileSize
18.93 KB

Seems to have some trouble when applying to multiple form items though.

The reason why this didn’t work: The handler that shows/hides elements looks for a .form-item (because you don’t just want to hide a form element while leaving the description/title visible). In this patch, I added the form element type wrapper which automatically creates a wrapper element with the appropriate ID and the class form-wrapper. I also changed the handler to not only look for .form-item but for .form-item, .form-wrapper.

kkaefer’s picture

Damien Tournoud’s picture

Names are only unique within a
element. Do you guarantee the the correct element is hit?

kkaefer’s picture

No, it’s not guaranteed. But the current JS code is also using IDs for targeting elements and is thus also prone to potentially changing IDs. This never happened, though.

RobLoach’s picture

FileSize
22.9 KB

Hits up the theme settings page. It toggles the logo and icon settings if the checkboxes are checked.

kkaefer’s picture

FileSize
25.91 KB

Patch now includes facilities for expanding/collapsing fieldsets as well as monitoring the collapsed/expanded state of a fieldset (e.g. you can now check a checkbox when a fieldset expands)

yched’s picture

Status: Needs work » Needs review

needs review ?

Status: Needs review » Needs work

The last submitted patch failed testing.

kkaefer’s picture

Status: Needs work » Needs review
FileSize
25.83 KB

Updated patch to be hopefully compatible with the testbot

Bojhan’s picture

wait... wowowowow! Totally awsome.

moshe weitzman’s picture

Very nice. Organic groups has some homegrown js that does this now. Would be great to use core stuff.

Can a form element be visible if a radio button has two values out of a possible 5 values (for example). Guess I am wishing for things right now. Not a prerequisite for proceeding.

kkaefer’s picture

Good point; I didn't test this with radio buttons yet; there might still be some logic missing for them, but I it's not a conceptual problem of this approach.

yoroy’s picture

kkaefer just pointed me to this. Very, very nice. This gives us ways to really start exploring progressive disclosure in the UI.

RobLoach’s picture

#444344: jQuery .once() method for adding behaviors once was just committed, so we could probably take advantage of that here.

catch’s picture

Issue tags: +API change

Only a few hours left to go, unless this can sneak in after freeze as a usability improvement. Could do with review from someone with javascript chops (i.e. not me).

kkaefer’s picture

Technically, it's not an API change but an API addition ;)

catch’s picture

Issue tags: +API addition

There's a tag for that too ;)

kkaefer’s picture

Issue tags: -API addition
FileSize
27.14 KB

More documentation

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

This can probably go in after freeze if needed under the usability category. Code looks quite readable and proper to me. I tested and it works well.

We can get radio buttons later if needed.

kkaefer’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
23.82 KB

Renamed "dependencies" to "states" since RobLoach and I think that it's a better name. Removed the menu item checkbox in previous patches since that was just for demonstration purposes.

kkaefer’s picture

Issue tags: +API addition

didn't mean to remove that

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

states works for me. back to rtbc.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

kkaefer’s picture

FileSize
23.85 KB
kkaefer’s picture

Status: Needs work » Needs review
Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Since it was only to fix the broken test back to RTBC

sun’s picture

+++ includes/form.inc
@@ -2347,6 +2347,38 @@ function theme_vertical_tabs($element) {
+function form_process_wrapper($element, &$form_state) {
+  $element['#id'] = form_clean_id(implode('-', $element['#parents']) . '-wrapper');
+  return $element;
+}

I think this should be a theme preprocess function instead of a FAPI #process.

+++ misc/states.js
@@ -0,0 +1,387 @@
+var states = Drupal.states = {};
...
+// An array of functions that should be postponed.
+states.postponed = [];

Why isn't 'postponed' initialized directly when Drupal.states is initialized?

+++ misc/states.js
@@ -0,0 +1,387 @@
+// Change `false` to `true` to enable debug information.
+states.debug = 'console' in window && false;
+

Please remove this debugging facility.

+++ misc/states.js
@@ -0,0 +1,387 @@
+        new states.Dependant({
...
+          dependees: settings.states[selector][state]
...
+states.Dependant.prototype = {
...
+  initializeDependee: function (selector, dependeeStates) {

"dependant", "dependees", and "dependee" are, wow, a bit hard to distinguish when reading this code. Can we find some better names here?

For the very same reason, the module dependency system uses the terms "requires" and "required by" everywhere now.

+++ misc/states.js
@@ -0,0 +1,387 @@
+ *   - dependees: An object with dependency specifications. Lists all elements
+ *         that this element depends on.

Wrong indentation here.

+++ misc/states.js
@@ -0,0 +1,387 @@
+// Comparison functions for comparing the value of an element with the
+// specification from the dependency settings. If the object type can't be
+// found in this list, the === operator is used by default.
+states.Dependant.comparisons = {
...
+// This list of states contains functions that are used to monitor the state
+// of an element. Whenever an element depends on the state of another element,
+// one of these trigger functions is added to the dependee so that the
+// dependant element can be updated.
+states.Trigger.states = {
...
+
+// This list of aliases is used to normalize states and associates negated
+// names with their respective inverse state.
+states.State.aliases = {
...
+// states.State: prototype
+states.State.prototype = {
...
+// Global state change handlers. These are bound to `document` to cover all
+// elements whose state changes. Events sent to elements within the page
+// bubble up to these handlers. We use this system so that themes and modules
+// can override these state change handlers for particular parts of a page.
+{
...
+// These are helper functions implementing addition "operators" and don't
+// implement any logic that is particular to states.
+{

Should use JSDoc (multi-line comment) syntax like for functions here.

I'm on crack. Are you, too?

catch’s picture

Status: Reviewed & tested by the community » Needs work
RobLoach’s picture

Issue tags: +awesomeness
FileSize
24.51 KB

Updated to HEAD, along with some of Sun's notes above. He also brought up that we have "wrapper_callback" now, which might be a good thing for the wrapper Form API type to use.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
24.54 KB

As discussed with kkaefer and tha_sun, we switched the "wrapper" Form API element type to "container" to not get too confused with other "wrapper" stuff, like "wrapper_callback", "theme_wrappers", etc. The "container" type still uses the CSS class of "*-wrapper" though, to match with the rest of the Drupal coding standards.

sun’s picture

Tagging for later review. I'd really appreciate if all of you could review some of the other API clean-up issues needing review in turn.

Jacine’s picture

Wow, this is really great.

neclimdul’s picture

This is very cool, something else we can pull out of views into core.

I talked to several people on IRC about some concerns I have with the current patch though. Summary, I'm worried about the DX of the #state form element property. The structure seems some what confusing and full of magic values.

+    '#states' => array(
+      'invisible' => array('input[name="user_mail_status_canceled_notify"]' => array('checked' => FALSE)),
+    ),
  • What does "invisible" do? Is it from a enumerated set of values or can it be called anything?
  • What does "checked" mean? What values can it have? What happens if I put more elements in the array like array('checked' => FALSE, 'funky' => TRUE)?
  • What can I put in instead of "input[name="user_mail_status_canceled_notify"]" and since its an array too what happens with more than one entry?

I'm ever so slightly leery of "#states" too. I understand now that it is defining states that the form element could have but it leaves some vagueness too. I had to look very hard to understand it was defining states of the element rather than actions that should happen for states of the page or states of element on the page triggered by some change to the current element, etc. This may just be extension of the vagueness I found in the definition array though. If not, it does kinda bring up the question of whether we're over generalizing this at the cost of clarity/usefulness.

Couple side notes, the issue is about dependencies when we're adding an abstract javascript state/trigger framework. Should we redefine the goal? Also, JS refers to things as dependant and dependee. Seems to just confuse the goal of the code after the name change in #26.

Overall, this is a very cool and powerful tool! The only reason I've voiced these particular concerns is I think addressing them could make this something general developers can use on a regular basis rather than something you pull in your jQuery/FAPI Drupal Jedi Master to figure out and so seldom used.

litwol’s picture

Sub

@todo: Describe my concerns how i consider this front end idea just being an eye candy and not "feature complete" as well as expand into extending this feature to backend FAPI field dependency and validation.

update: I had a conversation with tha_sun and kkaefer explaining to me that what i had in mind is really a separate issue/feature and should not stall current patch in any way. After the conversation i agree with them and will continue in a separate issue for D8.

mattyoung’s picture

.

sun’s picture

+++ modules/system/system.admin.inc	5 Oct 2009 20:36:12 -0000
@@ -507,6 +517,9 @@
       '#type' => 'fieldset',
       '#title' => t('Shortcut icon settings'),
       '#description' => t("Your shortcut icon, or 'favicon', is displayed in the address bar and bookmarks of most browsers."),
+      '#states' => array(
+        'invisible' => array('input[name="toggle_favicon"]' => array('checked' => FALSE)),
+      ),
+++ includes/common.inc	5 Oct 2009 20:36:12 -0000
@@ -4454,6 +4454,12 @@
+  // Add the dependency information for this form element.
+  if (!empty($elements['#states'])) {
+    drupal_add_js('misc/states.js', array('weight' => JS_LIBRARY + 1));
+    drupal_add_js(array('states' => array('#' . $elements['#id'] => $elements['#states'])), 'setting');
+  }

ok, some further discussion in IRC brought us to the point that we always want to use $element['#parents'] of the triggering element instead of manually assigning a DOM selector.

I'm not entirely sure how we can accomplish this cleanly, but the first option that crosses my mind is to directly put a reference to the target element's #parents in here, i.e. something along the lines of:

  '#states' => array(
    'invisible' => array(
      'trigger' => &$form['toggle_favicon'],
      'conditions' => array('checked' => FALSE),
    ),
  ),

Hence, the dependent element(s) would store a reference to the triggering element, so all changes to the triggering element are carried over. A minor downside would be that the triggering element would always have to be defined before dependent elements.

Alternatively, but that would require some more processing when rendering:

  '#states' => array(
    'invisible' => array(
      'trigger' => array('toggle_favicon'),
      'conditions' => array('checked' => FALSE),
    ),
  ),

So 'trigger' would contain a list of array parent keys that build up the path to the triggering element. The patch in #594650: Provide central $form_state['values'] clearance contains some funky code that samples how this could be processed efficiently during rendering.

Speaking of rendering, this really should be a form process (or even better #after_build ?) function and shouldn't live directly in drupal_render().

Please always roll your patches with diff -up. Otherwise, we can't see what's changed.

+++ includes/form.inc	5 Oct 2009 20:36:12 -0000
@@ -2392,6 +2392,38 @@
+function theme_container($element) {
+  return '<div class="form-wrapper" id="' . $element['#id'] . '">' . $element['#children'] . '</div>';
+}

I think this class name should be "form-elements-wrapper" or similar, because we do not wrap a form, but multiple elements.

+++ misc/states.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,390 @@
+var states = Drupal.states = {
...
+Drupal.behaviors.states = {
...
+states.Dependant = function (args) {
...
+states.Trigger = function (args) {
...
+states.State = function(state) {

Honestly, next to the "Dependant" and "dependee" nightmare, "states" is the next misnomer to me.

Although it would be kind of a name-clash, this JavaScript behavior/functionality really directly maps to what we all know as Trigger module and Actions in PHP already.

In fact, a JS Yedi Master could easily enhance Trigger module's UI to also serve as configuration cockpit for JavaScript triggers/actions. Actually, the more I think about it, the more I think it would be pretty easy... ;)

So, I'd like to see us changing "states" to "triggers", resp. Drupal.triggers.

I'm on crack. Are you, too?

dww’s picture

There are a few places in core that do this manually already which could be cleaned up and simplified (e.g. #144919: use jQuery to hide user picture settings when disabled), many more places in core that would benefit from this, and there are TONS of places in contrib that would use a core solution. I have a bunch already with homebrew JS in my contribs, and that's just a tiny scratch on the surface. I haven't looked at the patches yet, but a huge +1 on the concept.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

subscribe

kkaefer’s picture

What about this kind of system:

drupal_trigger($form['blah'])
  ->becomes('invisible')
    ->when($form['toggle_favicon'])->is('unchecked')->andIs('enabled')
    ->andWhen($form['foo'])->is('empty')
  ->becomes('disabled')
    ->when($form['foo'])->is('!empty');
RobLoach’s picture

Although chaining is great, I don't think it will work here as it has to fit into the Form API array itself so that the states/triggers are cached. What we are discussing here seems to be split up into two different issues:

Form API to jQuery selector translation
With the patch currently, you have to provide a jQuery selector for the element you want to act on. What sun, litwol and kkaefer introduced in their latest comments are the ability to pass in Form API elements rather then their target jQuery selectors. This is a fantastic feature, but I'm not sure how we'd do it, as there are just so many different selectors that we could be conditioning on. We could even be having an effect on something that's not particularly part of the Form API, for example.
Base States/Triggers Form API structure
It seems what people are getting confused with is the base structure of how the states/triggers are passed. If we were to add more documentation on this, it might help. Remember that there can be multiple states that an element can take, on multiple different conditions.

Currently, the $form['#states'] array looks like this:

$form['myelement']['#states'] = array(
  'invisible' => array( // State
    'input[name="toggle_logo"]' => array( // Trigger
      'checked' => FALSE // Condition
    ),
  ),
);

As you can see, since they are arrays, we can have multiple conditions, on multiple triggers, with multiple states. If we were to add more documentation to the patch, would it help get this distinction in?

kkaefer’s picture

Sorry, forgot to post the rest of the code. This might make it clearer; the settings are still stored in the FAPI, the chaining thing is just a frontend to creating that array.


function drupal_trigger(&$element) {
  if (!isset($element)) {
    return new Drupal_FAPI_Triggers();
  }
  elseif (!isset($element['#triggers'])) {
    $element['#triggers'] = new Drupal_FAPI_Triggers();
  }
  return $element['#triggers'];
}

class Drupal_FAPI_Triggers {
  private $triggers = array();
  private $elements = array();
  
  private $state;
  private $index;
  
  public function becomes($state) {
    $this->state = $state;
    if (!isset($triggers[$state])) {
      $triggers[$state] = array();
    }
    return $this;
  }
  
  public function when(&$element) {
    if (!in_array($element, $this->elements, TRUE)) {
      $this->elements[] = &$element;
    }
    $this->index = array_search($element, $this->elements);
    
    if (!isset($triggers[$this->state][$this->index])) {
      $this->triggers[$this->state][$this->index] = array();
    }
    return $this;
  }
  
  public function andWhen(&$element) {
    return $this->when($element);
  }
  
  public function is($state, $value = TRUE) {
    $this->triggers[$this->state][$this->index][$state] = $value;
    return $this;
  }
  
  public function andIs($state, $value = TRUE) {
    return $this->is($state, $value);
  }
  
  public function __toString() {
    $json = array();
    
    foreach ($this->triggers as $state => $conditions) {
      $json[$state] = array();
      foreach ($conditions as $element => $condition) {
        $json[$state]['#' . $this->elements[$element]['#id']] = $condition;
      }
    }
    return json_encode($json);
  }
}
RobLoach’s picture

Title: FAPI: Dependency system » FAPI: JavaScript States system
Status: Needs work » Needs review
FileSize
26.17 KB

Having a different interface around it that accomplishes the exact same thing has a term in the Drupal world, and this term is Bikeshed. The attached patch updates to HEAD, and adds more documentation to the cases where it's used.

  $form['email_canceled']['settings'] = array(
    '#type' => 'container',
    '#states' => array(
      // Hide the settings when the cancel notify checkbox is disabled.
      'invisible' => array(
        'input[name="user_mail_status_canceled_notify"]' => array('checked' => FALSE),
      ),
    ),
  );

Seriously, the above isn't confusing at all. Let's just get this feature in and stop bikeshedding it. These arn't #triggers, they are #states that are applied to form elements when certain conditions are met.

dww’s picture

Taking a quick break from hacking on #538660-111: Move update manager upgrade process into new authorize.php file (and make it actually work) to allow a few folks to comment, I saw this in my tracker. +1 to Rob's summary of where we're at with this, and +10 to the sample FAPI array he posted in #49 to demo the new functionality. That looks incredibly clear and self documenting. I was getting worried with all the talk of a generic "trigger" system in here. This looks very clean and good. I don't have time or bandwidth to closely review the code, but if it does what it says it does, I think this should go in. I can't tell you how happy I'd be if we had a unified solution to this problem in core, instead of having countless implementations and reimplementations scattered across core and contrib both.

RobLoach’s picture

FileSize
27.74 KB
  • A lot more documentation
  • At the request of Moshe, moves the drupal_render code to drupal_process_states()
  • Instead of calling drupal_add_js directly, it now processes the states into the #attached array, which ends up being used in drupal_process_attached
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Terrific improvements. Read this one through again. Lovely patch.

Bojhan’s picture

Wew! Thanks for the great work.

yoroy’s picture

I'm looking forward to seeing some live examples of this stuff in action! :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
27.25 KB

Updated to HEAD.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

and back to rtbc

quicksketch’s picture

I *love* this patch. As you can see from the changed code, nearly half the patch is removing old manual implementations of the same things over and over again. I can think of a half-dozen places in my contrib modules where we replace it with this approach, not to mention many more places that we don't do this (since it's so tedious right now) but it'll be a no-brainer afterwards.

So a code review of this JS. I can understand all the code, and it's immaculately formatted. It contains some really crazy code in places though such as:

// Calling functions and reducing the array at the same time.
(states.postponed.shift())();

// Using new jQuery 1.3 methods
.closest('.form-item, .form-wrapper')[e.value ? 'addClass' : 'removeClass']('form-disabled');

However this isn't a problem, it only takes a second to figure out and I think it just underscores kkaefers prowess with the language. I became a better programmer just by reading it, seriously.

However, a few minor (minor) gripes:

- Redundant articles (a an):

+ * An object representing a an element that depends on other elements.

Missing brackets (they're included everywhere else):

+    // Replace the state with its normalized name.
+    if (this.name in states.State.aliases)
+      this.name = states.State.aliases[this.name];
+    else
+      break;

Otherwise, this looks spectacular. Can we get a quick reroll?

Bojhan’s picture

FileSize
27.25 KB

fix

quicksketch’s picture

FileSize
27.25 KB

Darn whitespace. ;-)

quicksketch’s picture

Status: Reviewed & tested by the community » Needs review

Bumping this back down. After applying the patch, er, nothing seems to happen. The log message doesn't automatically check the "Create new revision" option, the Logo theme settings don't hide. Either we broke something somewhere or I don't know how to apply this patch.

quicksketch’s picture

FileSize
27.26 KB

Alright, found it, a missing bracket after #60 and #61 (whoops). Anyone want to give this one last confirmation?

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Works!

sun’s picture

Looks good.

kkaefer’s picture

Thanks for helping to bring this home!

Dries’s picture

Status: Reviewed & tested by the community » Needs work

This looks good. Committed. Can we get a CHANGELOG.txt entry?

webchick’s picture

Issue tags: +Needs documentation

tagging.

RobLoach’s picture

sun’s picture

Issue tags: -API change, -API clean-up

.

Dave Reid’s picture

Since there still isn't any documentation on this, does anyone know how this works for making something dependent on a specific option(s) in a select field?

yched’s picture

#71 : I couldn't make it work either. Doesn't seem to work right now.

peterpoe’s picture

I did some tests on D7-alpha1, and it appears that states.js "knows" about the existence of these states (in states.State.aliases):

'enabled'
'invisible'
'invalid'
'untouched'
'optional'
'filled'
'unchecked'
'irrelevant'
'expanded'
'readwrite'

But the only states handlers actually implemented are: enabled/disabled, optional/required, invisible/visible, checked/unchecked, expanded/collapsed.
The optional/required state is actually useless since it only adds/removes a class on the form item – the form validation is untouched.

This is a form that I used to test the states API (it's identical to the forms in Drupal core, like the one in #49):

  $form['selector'] = array(
    '#type' => 'checkbox',
    '#title' => t('Selector'),
    '#default_value' => FALSE,
  );
  $form['dependent_text'] = array(
    '#type' => 'textfield',
    '#title' => t('Dependent text'),
    '#states' => array(
      // Hide the Dependent text field when the Selector checkbox is unchecked.
      'invisible' => array(
        'input[name="selector"]' => array('checked' => FALSE),
      )
    ),
  );

I would like to help testing this great new feature, but I don't know where it is heading, since states.js has not been updated in 3 months and no new patches have appeared here.

YK85’s picture

subscribing

rfay’s picture

subscribing. I note that kkaefer has proposed a session on this.

Rob Loach is also threatening a new example for the Examples module demonstrating the States system.

Somehow we need to come out of this with decent docs.

hctom’s picture

#71:

Hi Dave,

I found out, that you may use a following approach for select fields (using a single select value... I'm not sure if this would work with multi-select fields):

$form['selector'] = array(
  '#type' => 'select',
  '#options' => array(0 => 'option1', 1 => 'option2')
);

$form['dependent_container'] = array(
  '#type' => 'container',
  '#states' => array(
    '!invisible' => array(
      'select[name="selector"]' => array('value' => '1')
    )
  )
);

Unfortunately, the 'value' trigger is only fired on key up... so if you change the select field using the keyboard, everything, works fine... if you do it by mouse, nothing happens :-(

@all: is something planned to extend the 'value' trigger, so it is gets trigger by mouse clicks on select fields or radio button groups? I'd really appreciate that, so you may also use those form elements to build dependent containers.

Thanx in advance & cheers

hctom

yoroy’s picture

Issue tags: +ui-pattern

Another tag…

casey’s picture

@hctom I think what you point out is related to #684602: IE BUG 1: menu options don't show up automatically

hctom’s picture

@casey: no I don't think so, as it deals with checkboxes, not select fields...

And the states.js code says the following around line 250:

states.Trigger.states = {
  // ... some more code
  value: {
    'keyup': function () {
      return this.val();
    }
  },
  // ... some more code
}

As far as I see, this means that 'value' only gets triggered by a 'keyup' event. or am I completely wrong?

Dave Reid’s picture

Ok yep I can confirm using the code in #76 does technically work, but its only triggered by keyboard, not mouse click. Very very bad. Who's up for fixing it?

Dave Reid’s picture

Also not sure how I can get the same invisible state based on more than one possible value in a select (if value was '1' or if value was '2').

Dave Reid’s picture

I changed 'keyup' to 'change' in states.js and it worked perfectly for me.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
445 bytes

RobLoach and I got a workaround for multiple values in a select by utilizing both an 'invisible' state and a 'visible' state which worked in my case, but I suspect others may want to be using multiple values.

Here's the patch I had to get this to respond to clicking and changing a select field. The 'value' trigger is not used elsewhere in core, so it's manually tested with my XML sitemap D7 port with both clicking and using keyboard (tab to field, select value, tab to next field).

hctom’s picture

@Dave: I'm not sure, if the keyup also needs to be a triggering event, too... otherwise this wont work for text input values :-(

Dave Reid’s picture

FileSize
473 bytes

Well let's just add both then. Tested and this worked if I was typing in a textfield and had:

    '#states' => array(
      'invisible' => array(
        'input[name="mytextfield"]' => array('value' => 'hello'),
      ),
    ),
hctom’s picture

yeah.. that's a nice one.. thanx for that.. I hope it will make it into the core code as soon as possible :-)

cheers

hctom

webchick’s picture

It'd be great to get kkaefer's take on these changes, if possible.

kkaefer’s picture

I'll comment on the weekend.

kkaefer’s picture

@peterpoe: This is correct. The states `states.js` knows about are merely aliases for yet to be implemented handlers. It's also correct that `states.js` does not influence server side validation whatsoever. The only thing `states.js` does is purely presentational. You still have to code the server side validation code yourself. However, one could imagine a system which takes the specifications for the client side validation and performs these validations also on the server side level.

@hctom: Currently, the value callback is only implemented for textfields. It's not a problem to extend states beyond that and also fire the value callback when the user changes a select box. In `states.Trigger.states`, these functions are defined. Note that it also doesn't yet work fully for textfields either: When the user pastes content with the context menu (i.e. without using keyboard shortcuts), the change is not detected.

@Dave Reid: The second patch should be good.

Regarding the multi-value dependency... I tried various ways of storing this data in JSON so that it's still readable and didn't come up with a solution that allows other operators than AND (of course, it's possible with a more complicated data structure). A workaround is to use !!visible, !!!!visible, !!!!!!visible and so on.

joachim’s picture

Subscribing.

I'm afraid I don't have time right now to add anything more constructive, other than to note that Views 2 has something like this in its options forms for handlers; are we using a similar structure for the FormAPI arrays here?

catch’s picture

Per #89, #85 is RTBC.

catch’s picture

Status: Needs review » Reviewed & tested by the community

really.

sun’s picture

Let's get this sucker in.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

jjeff’s picture

Status: Fixed » Needs work

It is possible to fill textfields without triggering a 'keyup' event. A good example of this is to double-click on an empty textfield in Firefox. Firefox will show a list of previously entered values on fields like this one in the past. You can click on a value from the list and that value will be entered into the field -- and you have clicked no keys.

When this happens, the associated state-change Javascript never fires.

My guess is that states.Trigger.states.empty needs to work similar to states.Trigger.states.value, binding to both the 'keyup' and 'change' events.

kkaefer’s picture

jjeff, the same is true for right-click, paste.

jjeff’s picture

Additionally, I think that the documentation in drupal_process_states() is wrong.

According to my research we should probably break the $elements list into two sections:

Possible states for listener fields (all are boolean)

  • disabled/enabled
  • required/optional
  • visible/invisible
  • checked/unchecked
  • collapsed/exapanded

Possible states for caller fields (these trigger changes in the listener elements)

  • empty/filled (boolean)
  • checked/unchecked (boolean)
  • collapsed/exapanded (boolean)
  • value (string)

I realize that I'm making up the words "caller" and "listener", but there doesn't seem to be any precedence here for anything other than just calling everything "states"... or "dependant" and "dependee" (neither of which are English words).

Of course, the major problem is that drupal_process_states() is simply the PHP function which processes these states for the FAPI. These states are extendable with Javascript and so it's not really the right place to put this documentation. The right place for this documentation *would* be in states.js, but as far as I can tell, the API module doesn't parse .js files. So maybe the documentation in drupal_process_states() should just mention that these are the states offered as part of the core implementation of the states system.

Do the above lists look correct?

yoroy’s picture

Status: Needs work » Needs review

review of #97 requested :)

jjeff’s picture

Status: Needs review » Needs work

Note:

We've been making a new Lullabot video about Javascript handling in Drupal. In preparation for a chapter about the State System, I've been creating a demonstration module. I've found a lot of unexpected behavior with this library. I will try to submit my module when I can to show the unexpected behavior, but I wanted to just note here that we've been seeing some problems.

This code should really be checked in a real world situation (examples.module?) as soon as possible.

sun’s picture

Not sure whether it makes sense to keep on re-opening this issue... Can we link to follow-up issues from here instead?

The first one exists already:

#667944: Javascript #states cannot hide fieldsets, radios, checkboxes

andrewmacpherson’s picture

Issue tags: +FAPI #states

starting a tag for the follow-ups...

kkaefer’s picture

casey’s picture

I agree with jjeff in #97 states.js can use better self-explanatory variable names (also the variable name "value" is used for multiple purposes) and more documentation.

rfay’s picture

So #states is not even in the Form API Reference? What's up with that?

If somebody can just do the tiniest writeup on it, I'll put it in the Form API Reference!

Is there a handbook page at all?

If somebody wants to roll a simple teaching example, I'll commit it to Examples module.

Great things like this should not be left unexplained.

rfay’s picture

I added the existing documentation from common.inc into the
Form API Reference.

This should show up there within a few hours. Please review what I put. It's not enough, but it's a start.

BTW: I was successful using states in my D7 port of Amazon module today. Thanks.

States is also now in the Examples for Developers project "Form Example" module. Demonstrated at http://d7.drupalexamples.info/form_example/states.

rfay’s picture

Status: Needs work » Fixed
Issue tags: -Needs documentation

I'm going to call this fixed for now - FAPI reference has it, and Examples has an example.

There's lots to do in the future to make it really sing, but this is a great enhancement to the Drupal world.

Followups at:
#767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states
#735528: FAPI #states: Fix conditionals to allow OR and XOR constructions
#767268: #states doesn't work correctly with radio buttons
#767212: #states can't hide/show fieldsets

Status: Fixed » Closed (fixed)

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

andypost’s picture

lulolulito’s picture

I M tired of trying to fix my site, I need a drupal programmer, will pay!
I need to fix http://www.dealsinformant.com

Please help

hctom’s picture

@lulolito: What about using Drupal instead of Wordpress... i guess that would fix all your problems at once ;-)

But thanx for spamming!!!