The version that went in with #557272: FAPI: JavaScript States system unfortunately was a bit premature and didn’t include support for OR: it was only possible to write AND conditions. This patch fixes this behavior while maintaining full backwards compatibility (all valid existing states descriptions work in exactly the same way with this patch).

Instead of just ANDing all conditions together, this patch makes states.js aware of arrays. All elements in an object literal ({ ... }) are ANDed (like now) and all elements in a array literal ([ ... ]) are ORed. This allows you to write conditions like:

"enabled": [
  {"select[name=\"date_default_timezone\"]": { "value": "America/Winnipeg" }},
  {"select[name=\"date_first_day\"]": [{ "value": "2" },{ "value": "3" }]}
]

In PHP, this would be:

array(
  "enabled" => array(
    array("select[name=\"date_default_timezone\"]" => array("value" => "America/Vancouver" )),
    array("select[name=\"date_first_day\"]" => array(array("value" => "2"), array("value" => "3"))),
  )
)

Note that it’s possible to use the array notation (without =>, thus with numeric indexes in PHP) both around selectors and within selectors. In the above example, the element is enabled when either the default timezone is Vancouver or when the user selects Tuesday or Wednesday as week start days (full sample code attached).

The XOR operator is supported as well: when writing an OR statement, you can insert the string 'xor' at any position to make the OR exclusive:

array(
  "enabled" => array('xor',
    array("select[name=\"date_default_timezone\"]" => array("value" => "America/Vancouver" )),
    array("select[name=\"date_first_day\"]" => array(array("value" => "2"), array("value" => "3"))),
  )
)

is possible, but

array(
  "enabled" => array(
    array("select[name=\"date_default_timezone\"]" => array("value" => "America/Vancouver" )),
    'xor',
    array("select[name=\"date_first_day\"]" => array(array("value" => "2"), array("value" => "3"))),
  )
)

works just as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kkaefer’s picture

FileSize
6.31 KB
Dave Reid’s picture

Hooray! Subscribing. :)

YK85’s picture

subscribing - this new feature will vastly expand the usability of conditionals, thanks!

YK85’s picture

I have a question

Example:
Field A has allowed values 'red', 'green', 'blue'
Field B has allowed values 'one', 'two', 'three'
Field B is controlled by value 'green' of Field A

User selects 'green' in Field A and Field B is shown, then user selects value 'three'
User then unselects 'green' from Field A and Field B goes hidden

Question:
When the node form is submitted, does value 'three' get submitted?

mattyoung’s picture

.

kkaefer’s picture

@yaz85: states is not about controlling what values are submitted (it never was and never will be). It's only about presentation.

catch’s picture

Subscribing, this does indeed make it applicable to a lot more situations.

YK85’s picture

I see, thanks for the kind explanation.
I hope Conditional Fields module will be available Drupal 7 =)

casey’s picture

I am not sure about the syntax. Little bit of a WTF. On the other hand, its short.

kkaefer’s picture

@casey: the goal is to also provide a kind of wrapper for that, like the DBTNG layer does for SQL conditions.

RobLoach’s picture

Beer OR Coffee!......... Translation: Subscribe!

kkaefer’s picture

*needs reviews*

casey’s picture

Ok by me, but I don't have too much experience with conditional elements. Patch does however need a lot more documentation (especially examples; or do they need to go to a handbook page).

Maybe merlinofchaos should give this a review (there is something similar in ctools am I right?)

rfay’s picture

subscribing

sun’s picture

The code seems to make sense, but most of the comments seem daunting. Some of them read like documentation on a fancy hack (but the code doesn't look like a hack).

Likewise, I feel the functions could use a bit more unique names - no difference on first sight here:

+++ misc/states.js
@@ -167,6 +165,113 @@ states.Dependant.prototype = {
+  verifyConstraints: function(constraints, selector) {
...
+  checkConstraints: function(value, selector, state) {
...
+  getDependees: function() {

Also some coding style issues, but I guess they don't matter for now.

Lastly, these changes should also be covered in drupal_process_states().

90 critical left. Go review some!

rfay’s picture

Sad as it is, this could definitely be considered API-change or feature material that should go into D8 or contrib.

Dave Reid’s picture

I would like to make sure at least that it is possible this can be extended in conrib. If not, then major :(

kkaefer’s picture

Yes, this could be extended in contrib.

sun’s picture

IIRC, we just need to fix some docs here. The rest seems to make sense. So instead of fearing, let's get that done? :)

aspilicious’s picture

Ok for the comments:

Dont forget to put a newline between the @param and @return block.
Function doc blocks always start with a 3th person verb.

It will save you a reroll ;)

agentrickard’s picture

FileSize
9.22 KB

Patch re-roll against HEAD. Need this for a D7 project, and think it needs to get in, personally.

Status: Needs review » Needs work

The last submitted patch, 735528-states-or.patch, failed testing.

agentrickard’s picture

Upgrade path test fail?

agentrickard’s picture

Status: Needs work » Needs review

#21: 735528-states-or.patch queued for re-testing.

agentrickard’s picture

OK, the patch is green. Any takers?

YK85’s picture

I don't know how to test this but I hope it makes it into Drupal 7 so that contrib modules like Conditional Fields module can use it and have a nice UI for administrators to use. Thank you

dawehner’s picture

What about renaming "checkConstraints" to "verifyChildConstraints".

I looked at

+        result = ternary(result, this.checkConstraints(constraints[i], selector, i));
+        if (result === false) return false

This does not look like drupal codestyle for me.

Damien Tournoud’s picture

Priority: Normal » Major

Bumping. Views needs this to be able to completely adopt this new system, it is likely that other systems will need this too.

See #667950: Start using javascript states, deprecate dependent.js.

rfay’s picture

Version: 7.x-dev » 8.x-dev

@Damien, while I fully support this, it seems to me to be a way-too-late API change (that we all hoped would get in in time). With respect, bumping to 8.x.

agentrickard’s picture

@rfay

A "late" change that has sat at 'green' for, what, 4 months? Unless this behavior can be added in contrib, let's reconsider.

rfay’s picture

I won't object to you trying to get it in, so feel free to push it back to D7. It just would be against all the published guidelines at this point.

* This is an API change and an added feature.
* It may have been "green", but unfortunately there are no tests of any kind for it since it's javascript, so it would be *very* hard for it not to be green.
* It has never been RTBC, not once.
* It has hardly gottten a significant review of any kind.

I want this feature too. It just doesn't fit with the policies we have for new features at this point in D7.

kkaefer’s picture

I agree that it needs intensive testing and can't simply be committed as-is. We can fix this in contrib with a small module that just overwrites the code. It wouldn't break other modules because the changes I made are fully backwards compatible so code written against the core version of states would also work with the contrib module.

agentrickard’s picture

Issue tags: +JavaScript

As long as @kkaaefer confirms it can be done in contrib, I withdraw my objections to the bump to 8.x.

But really, leaving patches 'needs review' for months is horribly counter-productive.

redndahead’s picture

Wim Leers’s picture

Subscribing. Excellent work, Konstantin!

I tweeted about the limitations in the current incarnation of #states in D7: http://twitter.com/#!/wimleers/status/22376963142324226, which points to https://gist.github.com/4f219d2b878ad673fe40. Konstantin then pointed me to this issue. This obviously fixes the OR and XOR use cases, but it does not yet fix the type problem.

For D7, we'll have to use a contrib module then. But at least we can get this in D8 very early, so yay!

sun’s picture

Unless @webchick is going to allow this to still get into 7.1, I'd be happy to add it to Edge.

In any case, the code needs to be backwards-compatible, so existing usage of #states does not break.

sun’s picture

Version: 8.x-dev » 7.x-dev

We should still try to get this into 7.1.

Regardless of that, I've created a separate patch to at least make "native" OR conditions work for checkboxes in #1057748: #states selector matching multiple checkboxes does not trigger state

peronas’s picture

Just a quick note I applied the latest patch for this and everything worked great in every browser except IE (surprise!).

IE seems to be choking on the use of indexOf on arrays at line #90 and #191. While Firefox, Chrome, Safari all implement indexOf for arrays it appears IE7 and IE8 do not.

I was able to get this working in IE by implementing the workaround noted here https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Arr... it's a bit of an ugly solution but it does work.

Has anyone else experienced issues when testing this with IE7/8? If so is there a cleaner way to make this work for IE as well?

sun’s picture

http://api.jquery.com/jQuery.inArray/ - identical to .indexOf(), but uses fallback code in the case when Array.indexOf() doesn't exist.

redndahead’s picture

FileSize
9.2 KB

This replaces the indexOf. Didn't quite get this from the help docs, but I believe the return values of inArray are the same as indexOf.

sun’s picture

Status: Needs review » Needs work

The most important question is whether this change is 100% backwards compatible. To prove that, we likely need to test every usage throughout core and all examples in the Examples project.

+++ misc/states.js	15 Feb 2011 04:57:42 -0000
@@ -83,16 +85,18 @@ states.Dependent.prototype = {
+      if ($.inArray(state, dependeeStates) < i) return;

@@ -167,6 +167,115 @@ states.Dependent.prototype = {
+        if (constraint && (or || result)) return or;
...
+        if (result === false) return false; // Optimization; result can't get falser anyway.

Needs to follow control structure coding standards.

Powered by Dreditor.

rfay’s picture

Shucks, at one time, about the time of the discussion in #767268-9: #states doesn't work correctly with radio buttons and #785362: Create more extensive guidelines for "mock module" creation and style I had a pretty full set of demo code for #states (much more than Examples). But we never found a place for it, and I can't find it. Probably attached to one of the other issues we were doing at that time, but I don't find it easily.

rfay’s picture

OK, I found it. I've pushed the module to github where anybody's welcome to work on it and make it more comprehensive. It hasn't been looked at for 10 months.

https://github.com/rfay/states_exercise

You can have commit privs for the asking. sun, redndahead, and kkaefer already have commit.

anruether’s picture

subscribe

redndahead’s picture

Anyone up to trying to get this finished at drupalcon? I'm in Mississippi right now and would be happy to huddle on this to get a plan together to get this RTBC.

redndahead’s picture

Status: Needs work » Needs review
FileSize
9.08 KB

Here is a more git friendly diff. It also incorporates Sun's comments. Looks like what we have left is to really do testing.

agentrickard’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Moving

localhost’s picture

subscribe. excellent patch.

peterpoe’s picture

I've been using this patched version of states.js in Conditional Fields 3 for the past few weeks and I'm happy to report that I haven't found any related bug.

redndahead’s picture

#46: 735528-states-2.patch queued for re-testing.

pp’s picture

subscribe

cpliakas’s picture

Ran into a need for this for Facet API. A big +1 for a D7 backport.

redndahead’s picture

Status: Needs review » Reviewed & tested by the community

I think if the module maintainer of conditional fields can be testing it and found no problems then I think we can take this to RTBC.

joelstein’s picture

Subscribing. Would love to see this in D7.

Ryan Palmer’s picture

+1 for a D7 backport. Easily increases the usefulness of #states by 10x. If the patched version is backwards compatible, I think we should make an exception and let it in to 7.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

The reports above say this breaks BC compatibility, so I don't understand how we can commit this to D7. (Sorry, I misread. It said "API change," not BC breaking.) I need more info about how existing code would be affected.

And in any case, this patch still needs extensive testing, which has not happened beyond one module developer saying they're using it in a release of a module, which has only a dev release with some 140 users. But there are no details here about how this feature is used in CF, nor what kind of testing these 140 sites are giving it, if any.

I'd like to see reports back from using rfay's code at https://github.com/rfay/states_exercise, with a number of different browsers.

Shadlington’s picture

Subscribing. Would love to see this get in D7.

garrettrayj’s picture

Subscribing. I too want this in D7.

geerlingguy’s picture

Subscribe, would be awesome for D7, but would also be happy with it in contrib for now.

[Edit: Patch applied cleanly to 7.2. It seems to be working fine on three separate forms where I was using #states the old/normal way, at least in Chrome and Safari.]

joelstein’s picture

#46 works great for me!

Boobaa’s picture

Subscribing. It's a shame I haven't run into this issue some months ago, and I forged something completely on my own to achieve these results.

james.williams’s picture

subscribe

Fidelix’s picture

I hope this gets on D7. Subscribing...

geshido’s picture

Subscribe. +1 for D7 port!

citricguy’s picture

Subscribe. +1 for D7 port!

xjm’s picture

Tagging issues not yet using summary template.

mr.york’s picture

Subscribe. +1 for D7 port!

Niklas Fiekas’s picture

Subscribe.

zkday’s picture

+1 for D7 port!
Subscribe.

mrsinguyen’s picture

+1 for backport to d7

drupov’s picture

subscribe

webchick’s picture

This patch isn't getting committed at all unless people stop +1ing and start testing. :\

geerlingguy’s picture

From #59, it was and is working fine on a few sites, though I'm not actually using the OR or XOR feature the patch enables anymore; but it definitely doesn't interfere with any existing functionality.

peterpoe’s picture

Ok, taking it headfirst :)

I have rewritten from scratch the states_exercise module with the following features:
- Now ALL possible permutations of states/conditions/form elements are testable, organized in subtabs and fieldsets.
- Only relevant states/conditions are tested. E.g.: avoid wasting time testing if a fieldset is correctly disabled when a textfield is checked.
- Added four tabs for multiple conditions testing, with AND, OR, XOR and mixed operators. (The latter specially could use more work).
There are some TODOs in the code that highlight issues with the api that I've noticed and things that would benefit from further testing.

While writing the module, I have found a bug with the implementation of XOR:
(states.Dependent.verifyConstraints(), line 191)

      // This constraint is an array (OR or XOR).
      var or = $.inArray('xor', constraints) < 0;
      for (var i = 0, len = constraints.length; i < len; i++) {
          var constraint = this.checkConstraints(constraints[i], selector, i);

The 'xor' meta-contraint is checked with the others, which is not good.
Patch attached where constraints[i] is skipped when it's 'xor'.

Note: while testing using Firefox Nightly, I sporadically encountered a seemingly random "function invert is undefined" error on line 165, which is weird. On FF 6 and Chrome the error never appears, so I suspect it might be a browser bug.

Have fun!

dystopia’s picture

Using OR with #74 on a live D7 web app now. No problems! Saved my life too :)

Thanks!

rfay’s picture

@kkaefer says this can be easily implemented in contrib. By far the best and most effective path to get this into D7 at least is for somebody to roll it as a contrib. Will anybody step up to the plate?

sun’s picture

As already mentioned some time ago, a patch for Edge would be happily accepted.

magnusk’s picture

The current model is that the list of conditions are part of an AND formula. I have several situations in a current project where I'd like more flexibility and have resorted to custom jQuery interventions.

It would seem that if we are aiming at making the conditions more flexible we should strive for full boolean arithmetic and nested (parenthesized) formulas; maybe inspired by LISP expressions. Having said that, I confess I don't quite understand the proposed format. Does it allow to impose the following types of formulaic conditions:

- A or ( B and C )

- ( A or ( B and C ) ) and D

- A and ( not B )

Also, is it possible to have a condition on a set of radio buttons that makes X visible iff some (any) button has been pressed, but X remains invisible as long as none of (a subset of) the buttons has been pressed?

rfay’s picture

Status: Needs review » Needs work

Will need a reroll after the core patch went in.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
9.16 KB

Can anyone think of a use case in Drupal core where this could be used?

rfay’s picture

If you build it they will come.

magnusk’s picture

Is it possible to use any boolean expression?
For instance, how do I express a condition like: A and (not B)

rfay’s picture

@magnusk AND and NOT are already fully supported; This issue is about OR. Please don't go off-topic. Some resources include the Form Example in Examples and http://randyfay.com/states

magnusk’s picture

I understand the starting point is OR in addition to AND. I'm asking about integration with NOT for generalized boolean expressions, because I don't see how it is being done in the above patch. For instance, turning to one of De Morgan's laws, how would one express conditions such as either "NOT ( a OR b )" or "(NOT a) AND (NOT b)" ? I've looked at the APis, as well as the examples and the link to your post, I've coded various forms, and I don't see the patch supports it.

rfay’s picture

I think you're asking about grouping, and doubt that we have that.

Right now, we just have AND (and you get not by using the opposite verb). It would be good to have grouping and a complete expressive language here, but I'm not sure we're going to get that out of this patch.

Martijn Houtman’s picture

Using #74 on a live site with no problems. Thanks! Would be great if backported :-)

xjm’s picture

#80: 735528-80-conditionalstates.patch queued for re-testing.

xjm’s picture

Assigned: Unassigned » xjm

Cleaning up docs a little.

xjm’s picture

Status: Needs review » Needs work

Things to clean up:

+++ b/core/misc/states.jsundefined
@@ -40,12 +40,14 @@ Drupal.behaviors.states = {
+ *   - constraints: An object with dependency specifications. Lists all elements
+ *     that this element depends on. It can be nested and contain arbitrary
+ *     AND and OR clauses.

Probably clearer to say "can" a second time.

+++ b/core/misc/states.jsundefined
@@ -142,16 +152,8 @@ states.Dependent.prototype = {
+    // Check whether any constraint for this dependant/state is satisifed.

"Dependent" is misspelled. Also, is it just "dependent state" perhaps, no slash?

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+   * Checks whether a constraint is satisified by evaluating the appropriate
+   * child constraints.

This should be one line.

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+   * @param constraints
+   *   An object or an array of constraints.

For clarity, perhaps this should read "A constraint object or an array of constraint objects?" Is that accurate?

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+    if ($.isArray(constraints)) {
+      // This constraint is an array (OR or XOR).
+      var or = $.inArray('xor', constraints) < 0;
+      for (var i = 0, len = constraints.length; i < len; i++) {
+        if (constraints[i] != 'xor') {
+          var constraint = this.checkConstraints(constraints[i], selector, i);
+          // Return if this is OR and we have a satisfied constraint or if this is
+          // XOR and we have a second satisfied constraint.
+          if (constraint && (or || result)) {
+            return or;
+          }
+          result = result || constraint;
+        }
+      }
+    }

Maybe it's just because it's JavaScript, but I find the code here very dense and confusing.

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+        if (result === false) return false; // Optimization; result can't get falser anyway.

Let's move the inline comment up a line. Also, what does "Can't get falser" mean?

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+   *   The state to check for this constraint. If undefined, resolving continues.
+   *   If both selector and state aren't undefined and valid non-numeric strings,
+   *   a lookup for the actual value of that selector's state is performed.
+   *   state is /not/ a State object but a pristine state string.

I think these lines are also a character too long. Also, let's remove the // around "not."

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+    // Normalize the last parameter. If it's non-numeric, we treat it either as a
+    // selector (in case there isn't one yet) or as a trigger/state.

The first line of this inline comment is a character too long; "a" should wrap to the next line.

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+   * Reuses the verify function to gather information about all required triggers.

Slightly too long. Let's just say "Gathers information about all required triggers."

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+      // Return nothing (=== undefined) so that the constraint loops are not broken.

Comment is too long and should wrap.

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+    // This call doesn't actually /verify/ anything but uses the resolving

Again, let's remove the pseudo-italic //.

+++ b/core/misc/states.jsundefined
@@ -166,6 +168,119 @@ states.Dependent.prototype = {
+    // mechanism to go through the constraints array, trying to "lookup" each

"Look up" should be two words.

+++ b/core/misc/states.jsundefined
@@ -300,7 +415,7 @@ states.State = function(state) {
- * Create a new State object by sanitizing the passed value.
+ * Creates a new State object by sanitizing the passed value.

Technically out of scope, but I'll leave it since I don't recall any API cleanup slated for misc/. :)

-14 days to next Drupal core point release.

xjm’s picture

Status: Needs work » Needs review
FileSize
5.83 KB
9.23 KB

As per #89. merlinofchaos suggested renaming the bewilderingly-named or to hasXor, so hopefully that makes the bit of code that confused me a bit clearer.

xjm’s picture

Issue tags: +Needs manual testing

We don't have automated test coverage for the states system (see #237566: Automated JavaScript unit testing framework), so this should be tested manually. @peterpoe has provided an excellent module with a full set of #states test forms in #74. Pretty self-explanatory; just enable the module and click through stuff making sure it does what the labels say. :)

rfay’s picture

I'd sure like to see states_exercise as a project instead of a tarball. I would gladly have given commit access to the states_exercise sandbox. I've now granted commit there to @peterpoe.

xjm’s picture

Issue tags: -Needs manual testing

Alright, I just spent entirely too much time testing this manually for every element/combo using the module from #74 . Noticed the following bugs with either the test module or the states system itself (haven't bothered checking which), regardless of whether the patch was applied:

  • Checkboxes elements do not get checked nor disabled in response to conditions
  • Radios also do not get disabled in respond to conditions
  • Fieldsets do not have the proper state on page load, but they do respond properly when a condition is set
  • Multiselect seems not to work at all as a trigger
  • ALL of radios and checkboxes are marked as required (rather than just the parent label)
  • The checkboxes tab's conditions don't make sense (bug in the module)

...so these things probably deserve a check for whether followup is needed. Issues might already exist if any of the above are core bugs.

Before the patch is applied, OR, XOR, and mixed conditions do not work (obviously).

After the patch is applied:

  • OR, XOR, and a mix of conditions all work properly for all elements, with the exception of checkboxes/radios being disabled/checked (as described above).
  • There are no regressions with states for any elements.
  • There are no regressions with the AND condition.

So, I think this patch is good to go. Phew! Could use a final code review.

nod_’s picture

Status: Needs review » Needs work

Just here to warn that in JS you need to check
typeof yourvar === 'undefined'
(the triple = is a bit overkill, typeof is always a string, but it doesn't hurt anyone). instead of
yourvar === undefined
because undefined is actually a variable you can set, doing undefined = true; somewhere in the JS will probably break this patch on most browsers.

And as long as self.values[selector] is an object (which it is) you don't need to explicitly set self.values[selector][state.name]. If you access it with yourobject[something] it won't crash and it'll be undefined when you try to access the property. And I got a problem with doing var result = undefined;, var result; is the same. If you want to explicitly declare and set the value to nothing, null should be used.

About this bit:

+    else if (constraints.constructor === Object) {
+      // This constraint is an object (AND).
+      for (var i in constraints) {

It'd be a bit safer to do

+    else if ($.isPlainObject(constraints)) {
+      // This constraint is an object (AND).
+      for (var i in constraints) {
+        if (constraints.hasOwnProperty(i) {

Just to be picky, with inArray, it should be checked for === -1 instead of < 0 and there is an if without {}.

I'm setting this as need work because crappy 3rd party script could easily break this JS the way it is. Nothing to say about the functionality though ;)

xjm’s picture

@nod_ Would you be able to reroll the patch with how the JavaScript should be, perchance? :)

xjm’s picture


So in testing this @nod_ found a difference between D7 and D8. With this #states array (from states_exercise_multiple_conditions_mixed()):
[removed]
The fact that 'xor' has a key of 0 causes an error on D8 (because D8 apparently returns it as the integer 0 rather than the string "0" that D7 does). JS error message:
Uncaught TypeError: Object 0 has no method 'charAt'
Per @nod_ 'a call to state.toString() "fixes" the problem'. It sounds like this might be an underlying issue with the design of the feature, though. It's not immediately obvious to me why the string/integer difference is there between D7 and D8. I found only the following commit related to JS stuff that has landed against D8 but not D7, and it doesn't seem at all related:
#1214344: "misc/progress.js" is not being included when "Aggregate JavaScript files." is used
Edit: Found also:
#865536: drupal_add_js() is missing the 'browsers' option
Edit 2: Confirmed that neither of the above resolve the error when reverted.

Disregard all of this. This only happens when the patch isn't applied.

nod_’s picture

Actually I'm pretty sure I'm wrong about what I said. I'll try looking into it more because it just doesn't work for me (D7 or D8).

xjm’s picture

Hmmm, it works fine for me in D7 and D8, and the JS error goes away when the patch is applied.

nod_’s picture

Status: Needs work » Needs review
FileSize
14.57 KB

After much mistakes from my part, here is the updated patch. It's in the same state as #93 with fixes from #94.

Basically I replaced undefined with null where it should be used, switched value === undefined to typeof value === 'undefined', I removed two { and two } that were not used and some extra ;. Both would have stopped any minification attempt (had to reindent quite a few lines, hence the patch size).

xjm’s picture

+++ b/core/misc/states.jsundefined
@@ -173,6 +175,124 @@ states.Dependent.prototype = {
+   * Evaluates child constraints to detemine if a constraint is satisfied.

ksenzee pointed out this typo, which I think is mine. ("Detemine")

nod_’s picture

fix the typo and got rid of var self and $.each(). had to sneak in a couple of $.proxy() for everything to work.

Not a required change but it's cleaner that way.

darrell_ulm’s picture

Tested the *OR* (only) conditional with the testing module on this page after the patch at
https://drupal.org/comment/reply/735528/5477246#comment-5477246

Worked for (did ~5 random tests each):
Textfield
Textarea
Select
Multiple select
Checkbox

Did not work (i.e. disable the dep. field for):
Checkboxes
Radios
Fieldset

ryan.gibson’s picture

On the "States/conditions" tab with the "Select" sub-tab selected:

Condition: "Value is ON"

Dependee Select: "On"

Things I noticed:

"State: Disabled"

Checkboxes and Radio Buttons are enabled when they should be disabled.

"State: Visible"

Under the Fieldset, it is closed and empty, not sure if that is right or not.

"State: Checked"

Under "Checkboxes", neither on nor off is checked.

"State: Expanded"

The field is collapsed.

xjm’s picture

ryan.gibson’s picture

Tab: "States/Conditions"

Sub-tab: "Multiple Select"

Value is "ON"

Dependee Multiple Select
"On"

"State: Disabled" Doesn't work.
"State: Required" Doesn't work.
"State: Invisible" Doesn't work.
State: Checked" Doesn't work.
State Expanded Doesn't work.

ryan.gibson’s picture

States/Conditions tab and Radios Sub-tab

1. The condition select box didn't seem to trigger the states even though it changed the dependee radio to "on"
2. When I left the condition select box alone and manually selected the dependee radio button to "on" the following are my results:

"State: Disabled": "Checkboxes" and Radio Buttons are still enabled.
"State: Optional": Doesn't Work (They show as required)
"State: Checked": "Checkboxes" aren't checked.

ryan.gibson’s picture

"Conditions (AND)"

Textfield

Works

Textarea

Works

Select

Works

Multiple Select

Works

Checkbox

Works

Checkboxes

Broken

Radios

Broken

Fieldset

Broken

frazras’s picture

Assigned: ryan.gibson » Unassigned
Issue tags: -Needs manual testing

Firstly: Thanks to webchick and xjm for giving me my first drupal core contribution opportunity on IRC
!

Here are my observations on the checkbox section of states/conditions (pardon my level of detail if it is too much):
On page load for the collapsed fieldset "Condition: Checked"

  • The dependee checkbox is initially selected, and the "Condition: Checked" fieldset is initially collapsed. but when the dependee checkbox is deselected - nothing happens.
  • I then expanded the "Condition: Checked" fieldset
  • Upon subsequent selection it enables/disables, the TextField, Textarea, Select, Multiple Select, and the single check box NOT the Checkboxes nor Radio buttons
  • Upon subsequent selection it makes optional/requires, the TextField, Textarea, Select, Multiple Select, The single check box, the Checkboxes and Radio buttons
  • Upon subsequent selection it makes visible/invisible, the TextField, Textarea, Select, Multiple Select, The single check box, the Checkboxes and Radio buttons plus the empty fieldset which is seen for the first time in this section of the test suite
  • Upon subsequent selection it makes checked/unchecked, the single check box NOT the Checkboxes. NB: Focus is set to the checked single check box in this pair of (checked/unchecked) fieldsets whenever the dependee checkbox is subsequently selected after the first initial deselect
  • Upon subsequent selection it collapses/expands, the empty fieldsets

The exact converse is true for the "Condition: Unchecked" fieldset inclusive of all the anomalies
The dependee checkbox starts in the selected state on page load however the form elements are all in the unselected state, so changing the dependee checkbox to unselected has no effect.

Hope this helps :-)

Cameron Tod’s picture

So Conditions (mixed) seems to work fine. I'm having odd results with the XOR tests which are a bit of a headache to test, but I'll persevere.

xjm and I have been collaborating on this spreadsheet to keep track of tests - it makes it a bit easier to see where we're at.

Cameron Tod’s picture

XOR conditions seem to work well for me using the patch from 101. Have updated the spreadsheet accordingly.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, I tested the textfield, textarea, and checkboxes again with #101 applied and confirmed the same results as in #93. I also reviewed all the testing above and confirmed that there were no regressions from that version of the patch nor from HEAD.

So... drumroll please... I believe this is RTBC.

I will also open followups against the states exercise sandbox for things we found in testing this issue.

Thank you so much to everyone who helped test this patch thoroughly, and to @nod_ for the work on cleaning up the JavaScript.

sun’s picture

#101: core-states-js-735528-101.patch queued for re-testing.

sun’s picture

Status: Reviewed & tested by the community » Needs review

#1057748: #states selector matching multiple checkboxes does not trigger state landed, but doesn't seem to conflict with this patch.

+++ b/core/misc/states.js
@@ -354,72 +477,71 @@ states.State.prototype = {
-{
-  $(document).bind('state:disabled', function(e) {
+$(document).bind('state:disabled', function(e) {
...
-{
-  // Bitwise AND with a third undefined state.
-  function ternary (a, b) {
+// Bitwise AND with a third undefined state.
+function ternary (a, b) {

I was a bit skeptical about these changes, but manual debugging confirmed that the scope is still limited.

+++ b/core/misc/states.js
@@ -84,26 +86,33 @@ states.Dependent.prototype = {
-    $.each(dependeeStates, function (state, value) {
...
+    for (var i in dependeeStates) {
+      if (dependeeStates.hasOwnProperty(i)) {
@@ -200,9 +322,11 @@ states.Trigger.prototype = {
-      $.each(trigger, function (event, valueFn) {
...
+      for (var event in trigger) {
+        if (trigger.hasOwnProperty(event)) {

This should be reverted to $.each().

xjm’s picture

This should be reverted to $.each().

Several people I've spoken with on IRC disagree with this. Could you provide more justification?

nod_’s picture

Status: Needs review » Reviewed & tested by the community

On #113 xjm rtbc #101. #101 being the #99 patch where I replaced all $.each with filtered loops.

Whatever flavor: #99 as well as #101 is RTBC. Take a pick and we'll follow up on the $.each thing later.

xjm’s picture

For clarity: The patches that have been tested manually here are #90 and #101. #99 has not been tested.

Edit: If the only reason to revert the $.each() changes is that we want to change them in a different issue, that seems irrelevant to me because this is new code. If there is some technical reason to use $.each(), I'd hope to see that documented in this issue.

sun’s picture

FileSize
3.83 KB

The conversion of $.each() to a manual for..in loop with hasOwnProperty() tries to account for the possibility of a globally extended Object.prototype. This might only be caused by some external JavaScript. Since no other code in Drupal accounts for that possibility, the local change in states.js won't help much, since the other code will still break.

Both Drupal and jQuery currently consider globally extended prototypes not as an issue (or rather, as bad practice/broken code). jQuery core does not seem to be interested in supporting it; see http://bugs.jquery.com/ticket/5499 - I followed up on that issue nevertheless.

We're using a JS framework to write code consistently. We can certainly discuss options to account for extended prototypes, but that should happen in a separate issue, so we can properly attack all details, come to an agreement, and ideally find a way to ease conversion of all existing code (e.g., considering the approaches stated the in the jQuery ticket).

Attached is the interdiff between #99 and #101. As @nod_ stated, there's no functional difference between the two, except for the $.each() changes (and that very minor typo fix).

ksenzee’s picture

I don't see any reason to use $.each, and there are good reasons to avoid it; it's a performance hog, if nothing else. What's wrong with an old-fashioned for loop?

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I think I'm going with the patch that has 100+ people manually testing every possible permutation. :) We can always deal with for /.each in a follow-up. Seems like making that change core-wide makes more sense than trying to enforce a non-existing pattern here.

Committed and pushed #101 to 8.x. 7.x will need a back-port. Won't make it in time for this week's release, but hopefully in time for the next one!

GREAT job and HUGE thanks to everyone who put long hard hours testing this sucker!

nod_’s picture

Awesome.

About the $.each and other js style/perf issues have a look here #1415788: Javascript winter clean-up. There is also a need for revi… ok, i'll wait a while before asking :)

nod_’s picture

Status: Patch (to be ported) » Needs review
FileSize
16.69 KB

Here is a D7 patch, it had issues applying because of the useless { and } that were removed toward the end of the file. I did a quick check for each tab (not sub-tab) of the #states module. It's all still working.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Oh, this is totally RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +JavaScript, +Needs issue summary update, +FAPI #states, +Needs backport to D7

The last submitted patch, core-states-js-735528-124.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
16.33 KB

reroll

nod_’s picture

Sorry, size was different because I forgot to reindent 4 lines, this one should be the same.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, this looked like a bit of a messy reroll and did not have the same number of insertions/deletions as the original, so I just spent a bunch of time reading all that icky JS very closely. :)

+++ b/misc/states.jsundefined
@@ -352,72 +475,69 @@ states.State.prototype = {
+        .closest('.form-item, .form-submit, .form-wrapper').toggleClass('form-disabled', e.value);

This one line is different between the D8 patch that went in and D7. I checked with nod_ about it and it's because a JS cleanup patch went into both D7 and D8 between the D8 commit for this issue and now: #1428538: Use jQuery toggle =/

The only other differences between this and the D8 patch are a couple superfluous newlines that went into D8 but aren't in this patch, and one additional point of indentation.

nod_ also retested with the testing module and said things worked as expected, so it's on his head if this breaks D7. (Kidding!) RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

LOL nice. :)

Committed and pushed to 7.x. Thanks! We have about a month before this gets out into the wild to catch any regressions.

Fidelix’s picture

OH! Thank you all! Thank you!

sebas5384’s picture

Hi this patch rocks!

Which patch can i use in a D7 stable release?

Thanks!

xjm’s picture

Issue tags: +7.13 release notes

This will be included in the next stable release unless something changes. My guess is the patch in #124 might apply to 7.12 in the meanwhile.

xjm’s picture

Title: FAPI #states: Fix conditionals to allow OR and XOR constructions » Change notification: FAPI #states: Fix conditionals to allow OR and XOR constructions
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Novice, +Needs change record

It occurs to me we should add a change notification for this. Tagging novice for that task.

xjm’s picture

Status: Active » Needs work

@ryanissamson is working on the change notification.

bleen’s picture

I've been playing with the test module in #74 and I dont see anyway to change a state based on a select having a value of FOO or a value of BAR. Only an example of selectA has a value of FOO and selectB has a value of BAR. Something along these lines (but not this):

...
'#states' => array(
  'visible' => array(
    ':input[name="xxx"]' => array('value' => 'foo OR bar'),
  ),
),
...

Is this supported? If so can someone give me an example?

xjm’s picture

Status: Needs work » Needs review

The snippet in #137 is trying to match the string literal 'foo OR bar'. I'd try something like:

     array(
        array('thing' => array('value' => 'foo')),
        array('thing' => array('value' => 'bar')),
      ),

Haven't tested it myself.

Setting NR for the change notification (see the nice link up at the top of the issue, yay).

tim.plunkett’s picture

Title: Change notification: FAPI #states: Fix conditionals to allow OR and XOR constructions » FAPI #states: Fix conditionals to allow OR and XOR constructions
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

I think the change notification is clear enough.

Status: Fixed » Closed (fixed)

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

attiks’s picture

Category: task » bug
Priority: Major » Critical
Status: Closed (fixed) » Needs work

This is breaking existing themes, the patch at http://drupalcode.org/project/drupal.git/commitdiff/bb3f7e97c33671539d61... includes changes to misc/states.js that changes the span to an abbr tag. Related D8 issue #1162802: Asterisk * used for required or changed marker should be in abbr not span, where Dries stated not to backport that change to D7. form.inc is still outputting the span tag.

+++ b/misc/states.jsundefined
@@ -352,72 +475,69 @@ states.State.prototype = {
-  $(document).bind('state:required', function(e) {
-    if (e.trigger) {
-      if (e.value) {
-        $(e.target).closest('.form-item, .form-wrapper').find('label').append('<span class="form-required">*</span>');
-      }
-      else {
-        $(e.target).closest('.form-item, .form-wrapper').find('label .form-required').remove();
-      }
+$(document).bind('state:required', function(e) {
+  if (e.trigger) {
+    if (e.value) {
+      $(e.target).closest('.form-item, .form-wrapper').find('label').append('<abbr class="form-required" title="' + Drupal.t('This field is required.') + '">*</abbr>');
+    }
+    else {
+      $(e.target).closest('.form-item, .form-wrapper').find('label .form-required').remove();
     }
-  });
+  }
attiks’s picture

Status: Needs work » Needs review
FileSize
678 bytes

Patch to fix it

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Patch is good. Was my mistake.

attiks’s picture

new tests needed for testswarm: #1493960: Tests to implement

xjm’s picture

Argh, also my bad for not catching that in review. Thanks for all the work you're doing with QUnit, also. :)

attiks’s picture

@xjm, it's thanks to testswarm that we found this ;p

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow. I clearly missed that, too. Thank you SO much for catching that, attiks!!

Committed and pushed to 7.x.

neochief’s picture

Status: Fixed » Needs review
FileSize
681 bytes

I've just caught another flaw, which might affect group conditions due to unproper usage of variables context. The scope of "selector" and "case" variables in function states.Dependent.prototype.initializeDependee() currently is Closure, which causes bad things when you process group conditions in that function, since Closure scope gets variables from last group.

Berdir’s picture

@neochief: Sounds like this might need to be fixed in 8.x as well? Then it should be fixed there first and then backported. Also, I'm not sure, but it sounds like that might be a non-critical follow-up which could be handled better in a separate issue.

xjm’s picture

Priority: Critical » Major
Status: Needs review » Fixed

I agree, let's open a followup for that. (nod_ has confirmed the issue.) I'll file an issue and add the link here shortly. Thanks!

Edit: #1542676: FAPI #states scope issue in Dependent prototype

Additional followups for non-regressions should also be filed as separate issues. Thanks!

Status: Fixed » Closed (fixed)

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

swentel’s picture

It looks like there was a subtle bug < 7.14 which made OR statements working.
I have following code in Display Suite which works fine in D7.12, but not anymore in D7.14.

  $return['page_option_title'] = array(
    '#type' => 'textfield',
    '#title' => t('Title'),
    '#default_value' => isset($settings['page_option_title']) ? $settings['page_option_title'] : '',
    '#description' => t('The title, you may use substitutions in this title.'),
    '#weight' => 101,
    '#access' => $entity_type != 'ds_views',
    '#states' => array(
      'visible' => array(
        ':input[name="page_option_type"]' => array('value' => '2'),
        ':input[name="additional_settings[ds_page_title][ds_page_title_options][page_option_type]"]' => array('value' => '2'),
      ),
    ),
  );

So in case 'page_option_type', which is a select, has a value of 2, this textfield is visible or not. Depending on the context, the select is printed in the root of the form, sometimes not, but they are never printed both.

- Edit - I'll just throw out new releases. Funny subtle workaround I had apparently ;)

sun’s picture

@swentel: If that worked previously, then that was a bug. Trivial OR conditions can be achieved by ordinary OR selectors; e.g., here:

    '#states' => array(
      'visible' => array(
        ':input[name="page_option_type"], :input[name="additional_settings[ds_page_title][ds_page_title_options][page_option_type]"]' => array('value' => '2'),
      ),
    ),
guldi’s picture

I suggest the change notification to be extended by #138.

As this suggestion was already made in #138 and rejected in #139 I don't change the issue status. I'm bringing it up again because it would make great sense.

Jelle_S’s picture

ignore this, wrong issue, that's the problem with having different tabs open :-)

fietserwin’s picture

Opened #1655114: FAPI #states: required span added multiple times as a regression of this issue. Can anybody who has a good insight in how the code of states.js ought to work have a look over there?

fietserwin’s picture

Issue #1655114 is now continued in #1592688: #states can cause the form "required" mark to appear more than once on the same element. We would love to see the people who introduced the part of the code below to join the discussion:

+    for (var i in dependeeStates) {
+      if (dependeeStates.hasOwnProperty(i)) {
+        state = dependeeStates[i];
+        // Make sure we're not initializing this selector/state combination twice.
+        if ($.inArray(state, dependeeStates) === -1) {
+          continue;
+        }

_nod wrote it this way, but he just changed a $.each into a for loop. The $.each was already in the first patch from kkaefer.

tmsimont’s picture

Status: Closed (fixed) » Active

This doesn't seem to be working as the change record advertises. I'm using drupal 7.19.

I have a form item that I want to be visible if a dependee checkboxes group checkbox value is selected, OR a dependee select box has a value. I've tried all of these codes on the #states array:

1: 2 key/value array pairs child to 'visible' state, with nothing in between

'visible' => array(
  ':input[name="dependee_select[und]"]' => array("value" => "trigger_value") ,
  ':input[name="dependee_checkboxes[und][key]"]' => array("checked" => true),
),

2: 2 key/value array pairs child to 'visible' state, with 'xor' in between

'visible' => array(
  ':input[name="dependee_select[und]"]' => array("value" => "trigger_value") ,
  'xor',
  ':input[name="dependee_checkboxes[und][key]"]' => array("checked" => true),
),

3: 2 key/value array pairs child to 'visible' state, with 'or' in between

'visible' => array(
  ':input[name="dependee_select[und]"]' => array("value" => "trigger_value") ,
  'or',
  ':input[name="dependee_checkboxes[und][key]"]' => array("checked" => true),
),

4: 2 key/value array pairs child to array child to 'visible' state, with nothing in between

'visible' => array(
  array(
    ':input[name="dependee_select[und]"]' => array("value" => "trigger_value") ,
    ':input[name="dependee_checkboxes[und][key]"]' => array("checked" => true),
  )
),

5: 2 key/value array pairs child to array child to 'visible' state, with 'or' in between

'visible' => array(
  array(
    ':input[name="dependee_select[und]"]' => array("value" => "trigger_value") ,
    'or',
    ':input[name="dependee_checkboxes[und][key]"]' => array("checked" => true),
  )
),

6: 2 key/value array pairs child to array child to 'visible' state, with 'xor' in between

'visible' => array(
  array(
    ':input[name="dependee_select[und]"]' => array("value" => "trigger_value") ,
    'xor',
    ':input[name="dependee_checkboxes[und][key]"]' => array("checked" => true),
  )
),

All 6 of these variations yield the exact same result. The "dependent" form item that has the a "#states" array with any 1 of the 6 above values consistently behaves as if both conditions are joined with an AND clause, not an OR clause. Both of the conditions need to be met for the dependent form item to become visible.

According to the documentation, one of these 6 (especially #4 and #6, which are right out of the change record) should give me a state that makes the "dependent" form item visible when either the "dependee_select[und]" value is "trigger_value" OR the "dependee_checkboxes[und][key]" value is checked. But, this is not the case. All 6 of these examples will only make the "dependent" form item visible if BOTH conditions are met.

I've tried all of these examples without the [und] as well, which gives me nothing at all, because it is required.

It looks like at some point the "OR" conditions worked, but some patches have been comitted since then, and now it definitely doesn't work. Perhaps it's related to the fact that I'm depending on a checkbox in a checkboxes group?

This kind of OR grouping DOES work on the same form (with a different form element, and I did isolate the above tests to make sure there's no conflict):


      'visible' => array(
        array(
          ':input[name="dependee_select[und]"]' => array(
            array("value" => 'trigger_1'),
            array("value" => 'trigger_2'),
          ),
        ),
      ),

incaic’s picture

Just an FYI:

jquery_update module replaces states.js with their own version. This may be the cause of why the OR operation doesn't work for you. Sorry, I don't have a real solution yet, other than linking to the core's states.js file (misc/states.js) from within jquery_update/replace/misc/1.7.

dawehner’s picture

Please create a new issue in the jquery_update issue queue, as it's a bug of that module.

incaic’s picture

Unfortunately, don't have time debug and find exactly what are the problems right now, but there are a few bugs out of issues with using the latest jquery_update's states.js.

http://drupal.org/node/1967270
http://drupal.org/node/1448490
http://drupal.org/node/1386294

jason.fisher’s picture

Recursive conditions are not currently supported, correct? i.e.:

thisfield = 1 OR (anotherfield = 3 AND thatfield = 5)

or:

thisfield = 1 OR (anotherfield = 3 AND thatfield = 5 AND (xfield = 2 OR yfield = 3))

xjm’s picture

Status: Active » Closed (fixed)

@tmsimont, can you please open a new issue? Thanks!

tmsimont’s picture

tmsimont’s picture

Issue summary: View changes

Fixed html entity > in code tags.

SocialNicheGuru’s picture

Issue summary: View changes

The issue is marked closed/fixed but has it been committed to Drupal 7.27?

tombisho’s picture

Please forgive me if this is a simple question, but I can't work out if I should expect OR to work for #states if I am using Drupal 7.26. It doesn't seem to work when using syntax like this:

<?php
'visible' => array(
  array(
    ':input[name="dependee_select[und]"]' => array("value" => "trigger_value") ,
    ':input[name="dependee_checkboxes[und][key]"]' => array("checked" => true),
  )
),
?>

I can't seem to work out if I need a patch and if so, which patch. Any help greatly appreciated.

nevergone’s picture

#166: Do not use Drupal 7.26. Please update your sites.

stephenls’s picture

#166 This is the array structure required for "or" conditions. In this example, 'conditional' will be visible if either of the two specified radio options are selected.

$form['piggy_activity'] = array(
      '#type' => 'radios',
      '#title' => 'This little piggy...',
      '#weight' => 1,
      '#options' => array(
        1 => '...went  to the market.',
        2 => '...stayed home.',
        3 => '...had roast beef.',
        4 => '...had none.',
        5 => '...cried wee wee wee all the way home.',

      ),
    );

    $form['conditional'] = array(
      '#type' => 'textfield',
      '#title' => 'How do you feel about this piggy\'s contribution, (or lack of contribution) to household chores?',
      '#weight' => 2,
      '#states' => array(
        'visible' => array(
          array(
            array(':input[name="piggy_activity"]' => array('value' => 1),),
            'or',
            array( ':input[name="piggy_activity"]' => array('value' => 2),)
          ),
        ),
      )
    );
edutrul’s picture

this works for me as 'or':

'#states' => array(
'visible' => array(
':input[name="render_type"]' => array('value' => 1),
':input[name="render_type"]' => array('value' => 2),
)),

happy codding

sime’s picture

Issue tags: -JavaScript +JavaScript

You might find yourself wandering around in circles so you should read https://www.drupal.org/node/1464758#comment-13949961