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.
Comment | File | Size | Author |
---|---|---|---|
#155 | 1493956-states-or-xor-and.patch | 7.97 KB | Jelle_S |
#148 | states-or-735528-148.patch | 681 bytes | neochief |
#142 | i735528-142.patch | 678 bytes | attiks |
#129 | core-states-js-735528-128.patch | 16.55 KB | nod_ |
#128 | core-states-js-735528-127.patch | 16.33 KB | nod_ |
Comments
Comment #1
kkaefer CreditAttribution: kkaefer commentedComment #2
Dave ReidHooray! Subscribing. :)
Comment #3
YK85 CreditAttribution: YK85 commentedsubscribing - this new feature will vastly expand the usability of conditionals, thanks!
Comment #4
YK85 CreditAttribution: YK85 commentedI 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?
Comment #5
mattyoung CreditAttribution: mattyoung commented.
Comment #6
kkaefer CreditAttribution: kkaefer commented@yaz85: states is not about controlling what values are submitted (it never was and never will be). It's only about presentation.
Comment #7
catchSubscribing, this does indeed make it applicable to a lot more situations.
Comment #8
YK85 CreditAttribution: YK85 commentedI see, thanks for the kind explanation.
I hope Conditional Fields module will be available Drupal 7 =)
Comment #9
casey CreditAttribution: casey commentedI am not sure about the syntax. Little bit of a WTF. On the other hand, its short.
Comment #10
kkaefer CreditAttribution: kkaefer commented@casey: the goal is to also provide a kind of wrapper for that, like the DBTNG layer does for SQL conditions.
Comment #11
RobLoachBeer OR Coffee!......... Translation: Subscribe!
Comment #12
kkaefer CreditAttribution: kkaefer commented*needs reviews*
Comment #13
casey CreditAttribution: casey commentedOk 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?)
Comment #14
rfaysubscribing
Comment #15
sunThe 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:
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!
Comment #16
rfaySad as it is, this could definitely be considered API-change or feature material that should go into D8 or contrib.
Comment #17
Dave ReidI would like to make sure at least that it is possible this can be extended in conrib. If not, then major :(
Comment #18
kkaefer CreditAttribution: kkaefer commentedYes, this could be extended in contrib.
Comment #19
sunIIRC, we just need to fix some docs here. The rest seems to make sense. So instead of fearing, let's get that done? :)
Comment #20
aspilicious CreditAttribution: aspilicious commentedOk 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 ;)
Comment #21
agentrickardPatch re-roll against HEAD. Need this for a D7 project, and think it needs to get in, personally.
Comment #23
agentrickardUpgrade path test fail?
Comment #24
agentrickard#21: 735528-states-or.patch queued for re-testing.
Comment #25
agentrickardOK, the patch is green. Any takers?
Comment #26
YK85 CreditAttribution: YK85 commentedI 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
Comment #27
dawehnerWhat about renaming "checkConstraints" to "verifyChildConstraints".
I looked at
This does not look like drupal codestyle for me.
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedBumping. 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.
Comment #29
rfay@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.
Comment #30
agentrickard@rfay
A "late" change that has sat at 'green' for, what, 4 months? Unless this behavior can be added in contrib, let's reconsider.
Comment #31
rfayI 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.
Comment #32
kkaefer CreditAttribution: kkaefer commentedI 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.
Comment #33
agentrickardAs 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.
Comment #34
redndahead CreditAttribution: redndahead commented#1001172: form field states multivalue patch marked as duplicate
Comment #35
Wim LeersSubscribing. 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!
Comment #36
sunUnless @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.
Comment #37
sunWe 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
Comment #38
peronas CreditAttribution: peronas commentedJust 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?
Comment #39
sunhttp://api.jquery.com/jQuery.inArray/ - identical to .indexOf(), but uses fallback code in the case when Array.indexOf() doesn't exist.
Comment #40
redndahead CreditAttribution: redndahead commentedThis 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.
Comment #41
sunThe 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.
Needs to follow control structure coding standards.
Powered by Dreditor.
Comment #42
rfayShucks, 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.
Comment #43
rfayOK, 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.
Comment #44
anruethersubscribe
Comment #45
redndahead CreditAttribution: redndahead commentedAnyone 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.
Comment #46
redndahead CreditAttribution: redndahead commentedHere is a more git friendly diff. It also incorporates Sun's comments. Looks like what we have left is to really do testing.
Comment #47
agentrickardMoving
Comment #48
localhost CreditAttribution: localhost commentedsubscribe. excellent patch.
Comment #49
peterpoe CreditAttribution: peterpoe commentedI'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.
Comment #50
redndahead CreditAttribution: redndahead commented#46: 735528-states-2.patch queued for re-testing.
Comment #51
pp CreditAttribution: pp commentedsubscribe
Comment #52
cpliakas CreditAttribution: cpliakas commentedRan into a need for this for Facet API. A big +1 for a D7 backport.
Comment #53
redndahead CreditAttribution: redndahead commentedI 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.
Comment #54
joelstein CreditAttribution: joelstein commentedSubscribing. Would love to see this in D7.
Comment #55
Ryan Palmer CreditAttribution: Ryan Palmer commented+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.
Comment #56
webchickThe 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.
Comment #57
Shadlington CreditAttribution: Shadlington commentedSubscribing. Would love to see this get in D7.
Comment #58
garrettrayj CreditAttribution: garrettrayj commentedSubscribing. I too want this in D7.
Comment #59
geerlingguy CreditAttribution: geerlingguy commentedSubscribe, 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.]
Comment #60
joelstein CreditAttribution: joelstein commented#46 works great for me!
Comment #61
BoobaaSubscribing. 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.
Comment #62
james.williams CreditAttribution: james.williams commentedsubscribe
Comment #63
Fidelix CreditAttribution: Fidelix commentedI hope this gets on D7. Subscribing...
Comment #64
geshido CreditAttribution: geshido commentedSubscribe. +1 for D7 port!
Comment #65
citricguy CreditAttribution: citricguy commentedSubscribe. +1 for D7 port!
Comment #66
xjmTagging issues not yet using summary template.
Comment #67
mr.york CreditAttribution: mr.york commentedSubscribe. +1 for D7 port!
Comment #68
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSubscribe.
Comment #69
zkday CreditAttribution: zkday commented+1 for D7 port!
Subscribe.
Comment #70
mrsinguyen CreditAttribution: mrsinguyen commented+1 for backport to d7
Comment #71
drupov CreditAttribution: drupov commentedsubscribe
Comment #72
webchickThis patch isn't getting committed at all unless people stop +1ing and start testing. :\
Comment #73
geerlingguy CreditAttribution: geerlingguy commentedFrom #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.
Comment #74
peterpoe CreditAttribution: peterpoe commentedOk, 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)
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!
Comment #75
dystopia CreditAttribution: dystopia commentedUsing OR with #74 on a live D7 web app now. No problems! Saved my life too :)
Thanks!
Comment #76
rfay@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?
Comment #77
sunAs already mentioned some time ago, a patch for Edge would be happily accepted.
Comment #78
magnusk CreditAttribution: magnusk commentedThe 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?
Comment #79
rfayWill need a reroll after the core patch went in.
Comment #80
RobLoachCan anyone think of a use case in Drupal core where this could be used?
Comment #81
rfayIf you build it they will come.
Comment #82
magnusk CreditAttribution: magnusk commentedIs it possible to use any boolean expression?
For instance, how do I express a condition like: A and (not B)
Comment #83
rfay@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
Comment #84
magnusk CreditAttribution: magnusk commentedI 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.
Comment #85
rfayI 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.
Comment #86
Martijn Houtman CreditAttribution: Martijn Houtman commentedUsing #74 on a live site with no problems. Thanks! Would be great if backported :-)
Comment #87
xjm#80: 735528-80-conditionalstates.patch queued for re-testing.
Comment #88
xjmCleaning up docs a little.
Comment #89
xjmThings to clean up:
Probably clearer to say "can" a second time.
"Dependent" is misspelled. Also, is it just "dependent state" perhaps, no slash?
This should be one line.
For clarity, perhaps this should read "A constraint object or an array of constraint objects?" Is that accurate?
Maybe it's just because it's JavaScript, but I find the code here very dense and confusing.
Let's move the inline comment up a line. Also, what does "Can't get falser" mean?
I think these lines are also a character too long. Also, let's remove the // around "not."
The first line of this inline comment is a character too long; "a" should wrap to the next line.
Slightly too long. Let's just say "Gathers information about all required triggers."
Comment is too long and should wrap.
Again, let's remove the pseudo-italic //.
"Look up" should be two words.
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.
Comment #90
xjmAs per #89. merlinofchaos suggested renaming the bewilderingly-named
or
tohasXor
, so hopefully that makes the bit of code that confused me a bit clearer.Comment #91
xjmWe 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. :)Comment #92
rfayI'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.
Comment #93
xjmAlright, 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:
...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:
So, I think this patch is good to go. Phew! Could use a final code review.
Comment #94
nod_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 setself.values[selector][state.name]
. If you access it withyourobject[something]
it won't crash and it'll be undefined when you try to access the property. And I got a problem with doingvar 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:
It'd be a bit safer to do
Just to be picky, with inArray, it should be checked for
=== -1
instead of< 0
and there is anif
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 ;)
Comment #95
xjm@nod_ Would you be able to reroll the patch with how the JavaScript should be, perchance? :)
Comment #96
xjmSo 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 integer0
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.
Comment #97
nod_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).
Comment #98
xjmHmmm, it works fine for me in D7 and D8, and the JS error goes away when the patch is applied.
Comment #99
nod_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
withnull
where it should be used, switchedvalue === undefined
totypeof 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).Comment #100
xjmksenzee pointed out this typo, which I think is mine. ("Detemine")
Comment #101
nod_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.
Comment #102
darrell_ulm CreditAttribution: darrell_ulm commentedTested 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
Comment #103
ryan.gibson CreditAttribution: ryan.gibson commentedOn 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.
Comment #104
xjmRestoring tags. :)
Comment #105
ryan.gibson CreditAttribution: ryan.gibson commentedTab: "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.
Comment #106
ryan.gibson CreditAttribution: ryan.gibson commentedStates/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.
Comment #108
ryan.gibson CreditAttribution: ryan.gibson commented"Conditions (AND)"
Textfield
Works
Textarea
Works
Select
Works
Multiple Select
Works
Checkbox
Works
Checkboxes
Broken
Radios
Broken
Fieldset
Broken
Comment #109
frazras CreditAttribution: frazras commentedFirstly: 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 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 :-)
Comment #110
Cameron Tod CreditAttribution: Cameron Tod commentedSo 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.
Comment #112
Cameron Tod CreditAttribution: Cameron Tod commentedXOR conditions seem to work well for me using the patch from 101. Have updated the spreadsheet accordingly.
Comment #113
xjmAlright, 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.
Comment #114
xjmExisting issues for most of the bugs we found during testing:
Comment #115
sun#101: core-states-js-735528-101.patch queued for re-testing.
Comment #116
sun#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().
Comment #117
xjmSeveral people I've spoken with on IRC disagree with this. Could you provide more justification?
Comment #118
nod_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.
Comment #119
xjmFor 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.Comment #120
sunThe 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).
Comment #121
ksenzeeI 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?
Comment #122
webchickI 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!
Comment #123
nod_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 :)Comment #124
nod_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.Comment #125
xjmOh, this is totally RTBC.
Comment #126
webchick#124: core-states-js-735528-124.patch queued for re-testing.
Comment #128
nod_reroll
Comment #129
nod_Sorry, size was different because I forgot to reindent 4 lines, this one should be the same.
Comment #130
xjmAlright, 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. :)
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.
Comment #131
webchickLOL nice. :)
Committed and pushed to 7.x. Thanks! We have about a month before this gets out into the wild to catch any regressions.
Comment #132
Fidelix CreditAttribution: Fidelix commentedOH! Thank you all! Thank you!
Comment #133
sebas5384 CreditAttribution: sebas5384 commentedHi this patch rocks!
Which patch can i use in a D7 stable release?
Thanks!
Comment #134
xjmThis 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.
Comment #135
xjmIt occurs to me we should add a change notification for this. Tagging novice for that task.
Comment #136
xjm@ryanissamson is working on the change notification.
Comment #137
bleen CreditAttribution: bleen commentedI'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):
Is this supported? If so can someone give me an example?
Comment #138
xjmThe snippet in #137 is trying to match the string literal
'foo OR bar'
. I'd try something like:Haven't tested it myself.
Setting NR for the change notification (see the nice link up at the top of the issue, yay).
Comment #139
tim.plunkettI think the change notification is clear enough.
Comment #141
attiks CreditAttribution: attiks commentedThis 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.
Comment #142
attiks CreditAttribution: attiks commentedPatch to fix it
Comment #143
nod_Patch is good. Was my mistake.
Comment #144
attiks CreditAttribution: attiks commentednew tests needed for testswarm: #1493960: Tests to implement
Comment #145
xjmArgh, also my bad for not catching that in review. Thanks for all the work you're doing with QUnit, also. :)
Comment #146
attiks CreditAttribution: attiks commented@xjm, it's thanks to testswarm that we found this ;p
Comment #147
webchickWow. I clearly missed that, too. Thank you SO much for catching that, attiks!!
Committed and pushed to 7.x.
Comment #148
neochief CreditAttribution: neochief commentedI'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.
Comment #149
Berdir@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.
Comment #150
xjmI 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!
Comment #152
swentel CreditAttribution: swentel commentedIt 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.
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 ;)
Comment #153
sun@swentel: If that worked previously, then that was a bug. Trivial OR conditions can be achieved by ordinary OR selectors; e.g., here:
Comment #154
guldi CreditAttribution: guldi commentedI 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.
Comment #155
Jelle_Signore this, wrong issue, that's the problem with having different tabs open :-)
Comment #156
fietserwinOpened #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?
Comment #157
fietserwinIssue #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:
_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.
Comment #158
tmsimont CreditAttribution: tmsimont commentedThis 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
2: 2 key/value array pairs child to 'visible' state, with 'xor' in between
3: 2 key/value array pairs child to 'visible' state, with 'or' in between
4: 2 key/value array pairs child to array child to 'visible' state, with nothing in between
5: 2 key/value array pairs child to array child to 'visible' state, with 'or' in between
6: 2 key/value array pairs child to array child to 'visible' state, with 'xor' in between
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):
Comment #159
incaic CreditAttribution: incaic commentedJust 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.
Comment #160
dawehnerPlease create a new issue in the jquery_update issue queue, as it's a bug of that module.
Comment #161
incaic CreditAttribution: incaic commentedUnfortunately, 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
Comment #162
jason.fisher CreditAttribution: jason.fisher commentedRecursive 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))
Comment #163
xjm@tmsimont, can you please open a new issue? Thanks!
Comment #164
tmsimont CreditAttribution: tmsimont commented#2013210: FAPI #states: Fix conditionals to allow OR and XOR constructions
Comment #164.0
tmsimont CreditAttribution: tmsimont commentedFixed html entity > in code tags.
Comment #165
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedThe issue is marked closed/fixed but has it been committed to Drupal 7.27?
Comment #166
tombisho CreditAttribution: tombisho commentedPlease 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:
I can't seem to work out if I need a patch and if so, which patch. Any help greatly appreciated.
Comment #167
nevergone CreditAttribution: nevergone commented#166: Do not use Drupal 7.26. Please update your sites.
Comment #168
stephenls CreditAttribution: stephenls commented#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.
Comment #169
edutrul CreditAttribution: edutrul as a volunteer and at Anexus commentedthis 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
Comment #170
simeYou might find yourself wandering around in circles so you should read https://www.drupal.org/node/1464758#comment-13949961