This patch adds a generic form dependency system to Drupal. This allows you to specify dependencies of one form element on another in a declarative way like this:
'#dependencies' => array( 'enabled' => array( '#edit-menu-has-menu' => array('checked' => TRUE) ) ),
This dependency specification means: The form element is enabled
when the element with the ID edit-menu-has-menu
is checked. Intuitively, you could also specify FALSE
which results in the opposite behavior.
#edit-menu-has-menu
is a CSS selector, that means you can specify arbitrary elements (they don’t have to be FAPI elements). Each state (visible
, enabled
, checked
, …) of form element can depend on an arbitrary number of other elements. The opposite state can be specified by prepending an exclamation mark. States can be aliased, for example, enabled
is just an alias for !disabled
. Recursive aliases are possible.
The system is open and module developers and themers can easily expand the available states and implement their own state handlers. Module developers and themers can override actions for single form elements, e.g. if they want to implement a different way of hiding and showing one particular form element, they can do that.
This is a first draft at this system and not yet complete. However, it’s already in a state where it can be reviewed. Code freeze is next week ;)
Comment | File | Size | Author |
---|---|---|---|
#85 | 557272-states-selects-D7.patch | 473 bytes | Dave Reid |
#83 | 557272-states-selects-D7.patch | 445 bytes | Dave Reid |
#63 | 557272.patch | 27.26 KB | quicksketch |
#61 | 557272.patch | 27.25 KB | quicksketch |
#60 | 557272_5.patch | 27.25 KB | Bojhan |
Comments
Comment #1
kkaefer CreditAttribution: kkaefer commentedScreencast is at http://dump.kkaefer.com/dependencies.mov
Comment #3
RobLoachSubscribing. This is really very cool. I've noticed that there is an application of this during install.php, seems like another perfect use case for this. Are there any other applications of Select (or other) in Drupal core?
Having a "var _" generates some pretty ugly code. Could we namespace this in Drupal.dependencies or something?
This review is powered by Dreditor.
Comment #4
mfer CreditAttribution: mfer commented/me hits subscribe button.
Comment #5
kkaefer CreditAttribution: kkaefer commented@RobLoach: It’s already namespaced to
Drupal.dependencies
sincevar _ = Drupal.dependencies
, however, when I usedDrupal.dependencies
throughout the code, it became hard to readComment #6
RobLoachWhy are we adding this? Couldn't it enabled/disable the other elements based on if the title is empty?
Comment #7
kkaefer CreditAttribution: kkaefer commentedThe reason why this didn’t work: The handler that shows/hides elements looks for a
.form-item
(because you don’t just want to hide a form element while leaving the description/title visible). In this patch, I added the form element typewrapper
which automatically creates a wrapper element with the appropriate ID and the classform-wrapper
. I also changed the handler to not only look for.form-item
but for.form-item, .form-wrapper
.Comment #8
kkaefer CreditAttribution: kkaefer commentedBtw, I maintain this patch at http://github.com/kkaefer/drupal/tree/dependencies
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedNames are only unique within a
element. Do you guarantee the the correct element is hit?
Comment #10
kkaefer CreditAttribution: kkaefer commentedNo, it’s not guaranteed. But the current JS code is also using IDs for targeting elements and is thus also prone to potentially changing IDs. This never happened, though.
Comment #11
RobLoachHits up the theme settings page. It toggles the logo and icon settings if the checkboxes are checked.
Comment #12
kkaefer CreditAttribution: kkaefer commentedPatch now includes facilities for expanding/collapsing fieldsets as well as monitoring the collapsed/expanded state of a fieldset (e.g. you can now check a checkbox when a fieldset expands)
Comment #13
yched CreditAttribution: yched commentedneeds review ?
Comment #15
kkaefer CreditAttribution: kkaefer commentedUpdated patch to be hopefully compatible with the testbot
Comment #16
Bojhan CreditAttribution: Bojhan commentedwait... wowowowow! Totally awsome.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedVery nice. Organic groups has some homegrown js that does this now. Would be great to use core stuff.
Can a form element be visible if a radio button has two values out of a possible 5 values (for example). Guess I am wishing for things right now. Not a prerequisite for proceeding.
Comment #18
kkaefer CreditAttribution: kkaefer commentedGood point; I didn't test this with radio buttons yet; there might still be some logic missing for them, but I it's not a conceptual problem of this approach.
Comment #19
yoroy CreditAttribution: yoroy commentedkkaefer just pointed me to this. Very, very nice. This gives us ways to really start exploring progressive disclosure in the UI.
Comment #20
RobLoach#444344: jQuery .once() method for adding behaviors once was just committed, so we could probably take advantage of that here.
Comment #21
catchOnly a few hours left to go, unless this can sneak in after freeze as a usability improvement. Could do with review from someone with javascript chops (i.e. not me).
Comment #22
kkaefer CreditAttribution: kkaefer commentedTechnically, it's not an API change but an API addition ;)
Comment #23
catchThere's a tag for that too ;)
Comment #24
kkaefer CreditAttribution: kkaefer commentedMore documentation
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedThis can probably go in after freeze if needed under the usability category. Code looks quite readable and proper to me. I tested and it works well.
We can get radio buttons later if needed.
Comment #26
kkaefer CreditAttribution: kkaefer commentedRenamed "dependencies" to "states" since RobLoach and I think that it's a better name. Removed the menu item checkbox in previous patches since that was just for demonstration purposes.
Comment #27
kkaefer CreditAttribution: kkaefer commenteddidn't mean to remove that
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commentedstates works for me. back to rtbc.
Comment #30
kkaefer CreditAttribution: kkaefer commentedComment #31
kkaefer CreditAttribution: kkaefer commentedComment #32
Bojhan CreditAttribution: Bojhan commentedSince it was only to fix the broken test back to RTBC
Comment #33
sunI think this should be a theme preprocess function instead of a FAPI #process.
Why isn't 'postponed' initialized directly when Drupal.states is initialized?
Please remove this debugging facility.
"dependant", "dependees", and "dependee" are, wow, a bit hard to distinguish when reading this code. Can we find some better names here?
For the very same reason, the module dependency system uses the terms "requires" and "required by" everywhere now.
Wrong indentation here.
Should use JSDoc (multi-line comment) syntax like for functions here.
I'm on crack. Are you, too?
Comment #34
catchComment #35
RobLoachUpdated to HEAD, along with some of Sun's notes above. He also brought up that we have "wrapper_callback" now, which might be a good thing for the wrapper Form API type to use.
Comment #36
RobLoachAs discussed with kkaefer and tha_sun, we switched the "wrapper" Form API element type to "container" to not get too confused with other "wrapper" stuff, like "wrapper_callback", "theme_wrappers", etc. The "container" type still uses the CSS class of "*-wrapper" though, to match with the rest of the Drupal coding standards.
Comment #37
sunTagging for later review. I'd really appreciate if all of you could review some of the other API clean-up issues needing review in turn.
Comment #38
JacineWow, this is really great.
Comment #39
neclimdulThis is very cool, something else we can pull out of views into core.
I talked to several people on IRC about some concerns I have with the current patch though. Summary, I'm worried about the DX of the #state form element property. The structure seems some what confusing and full of magic values.
I'm ever so slightly leery of "#states" too. I understand now that it is defining states that the form element could have but it leaves some vagueness too. I had to look very hard to understand it was defining states of the element rather than actions that should happen for states of the page or states of element on the page triggered by some change to the current element, etc. This may just be extension of the vagueness I found in the definition array though. If not, it does kinda bring up the question of whether we're over generalizing this at the cost of clarity/usefulness.
Couple side notes, the issue is about dependencies when we're adding an abstract javascript state/trigger framework. Should we redefine the goal? Also, JS refers to things as dependant and dependee. Seems to just confuse the goal of the code after the name change in #26.
Overall, this is a very cool and powerful tool! The only reason I've voiced these particular concerns is I think addressing them could make this something general developers can use on a regular basis rather than something you pull in your jQuery/FAPI Drupal Jedi Master to figure out and so seldom used.
Comment #40
litwol CreditAttribution: litwol commentedSub
@todo: Describe my concerns how i consider this front end idea just being an eye candy and not "feature complete" as well as expand into extending this feature to backend FAPI field dependency and validation.update: I had a conversation with tha_sun and kkaefer explaining to me that what i had in mind is really a separate issue/feature and should not stall current patch in any way. After the conversation i agree with them and will continue in a separate issue for D8.
Comment #41
mattyoung CreditAttribution: mattyoung commented.
Comment #42
sunok, some further discussion in IRC brought us to the point that we always want to use $element['#parents'] of the triggering element instead of manually assigning a DOM selector.
I'm not entirely sure how we can accomplish this cleanly, but the first option that crosses my mind is to directly put a reference to the target element's #parents in here, i.e. something along the lines of:
Hence, the dependent element(s) would store a reference to the triggering element, so all changes to the triggering element are carried over. A minor downside would be that the triggering element would always have to be defined before dependent elements.
Alternatively, but that would require some more processing when rendering:
So 'trigger' would contain a list of array parent keys that build up the path to the triggering element. The patch in #594650: Provide central $form_state['values'] clearance contains some funky code that samples how this could be processed efficiently during rendering.
Speaking of rendering, this really should be a form process (or even better #after_build ?) function and shouldn't live directly in drupal_render().
Please always roll your patches with diff -up. Otherwise, we can't see what's changed.
I think this class name should be "form-elements-wrapper" or similar, because we do not wrap a form, but multiple elements.
Honestly, next to the "Dependant" and "dependee" nightmare, "states" is the next misnomer to me.
Although it would be kind of a name-clash, this JavaScript behavior/functionality really directly maps to what we all know as Trigger module and Actions in PHP already.
In fact, a JS Yedi Master could easily enhance Trigger module's UI to also serve as configuration cockpit for JavaScript triggers/actions. Actually, the more I think about it, the more I think it would be pretty easy... ;)
So, I'd like to see us changing "states" to "triggers", resp. Drupal.triggers.
I'm on crack. Are you, too?
Comment #43
dwwThere are a few places in core that do this manually already which could be cleaned up and simplified (e.g. #144919: use jQuery to hide user picture settings when disabled), many more places in core that would benefit from this, and there are TONS of places in contrib that would use a core solution. I have a bunch already with homebrew JS in my contribs, and that's just a tiny scratch on the surface. I haven't looked at the patches yet, but a huge +1 on the concept.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe
Comment #46
kkaefer CreditAttribution: kkaefer commentedWhat about this kind of system:
Comment #47
RobLoachAlthough chaining is great, I don't think it will work here as it has to fit into the Form API array itself so that the states/triggers are cached. What we are discussing here seems to be split up into two different issues:
Currently, the $form['#states'] array looks like this:
As you can see, since they are arrays, we can have multiple conditions, on multiple triggers, with multiple states. If we were to add more documentation to the patch, would it help get this distinction in?
Comment #48
kkaefer CreditAttribution: kkaefer commentedSorry, forgot to post the rest of the code. This might make it clearer; the settings are still stored in the FAPI, the chaining thing is just a frontend to creating that array.
Comment #49
RobLoachHaving a different interface around it that accomplishes the exact same thing has a term in the Drupal world, and this term is Bikeshed. The attached patch updates to HEAD, and adds more documentation to the cases where it's used.
Seriously, the above isn't confusing at all. Let's just get this feature in and stop bikeshedding it. These arn't #triggers, they are #states that are applied to form elements when certain conditions are met.
Comment #50
dwwTaking a quick break from hacking on #538660-111: Move update manager upgrade process into new authorize.php file (and make it actually work) to allow a few folks to comment, I saw this in my tracker. +1 to Rob's summary of where we're at with this, and +10 to the sample FAPI array he posted in #49 to demo the new functionality. That looks incredibly clear and self documenting. I was getting worried with all the talk of a generic "trigger" system in here. This looks very clean and good. I don't have time or bandwidth to closely review the code, but if it does what it says it does, I think this should go in. I can't tell you how happy I'd be if we had a unified solution to this problem in core, instead of having countless implementations and reimplementations scattered across core and contrib both.
Comment #51
RobLoachdrupal_process_states()
drupal_add_js
directly, it now processes the states into the#attached
array, which ends up being used indrupal_process_attached
Comment #52
moshe weitzman CreditAttribution: moshe weitzman commentedTerrific improvements. Read this one through again. Lovely patch.
Comment #53
Bojhan CreditAttribution: Bojhan commentedWew! Thanks for the great work.
Comment #54
yoroy CreditAttribution: yoroy commentedI'm looking forward to seeing some live examples of this stuff in action! :-)
Comment #56
RobLoachUpdated to HEAD.
Comment #57
yoroy CreditAttribution: yoroy commentedand back to rtbc
Comment #59
quicksketchI *love* this patch. As you can see from the changed code, nearly half the patch is removing old manual implementations of the same things over and over again. I can think of a half-dozen places in my contrib modules where we replace it with this approach, not to mention many more places that we don't do this (since it's so tedious right now) but it'll be a no-brainer afterwards.
So a code review of this JS. I can understand all the code, and it's immaculately formatted. It contains some really crazy code in places though such as:
However this isn't a problem, it only takes a second to figure out and I think it just underscores kkaefers prowess with the language. I became a better programmer just by reading it, seriously.
However, a few minor (minor) gripes:
- Redundant articles (a an):
Missing brackets (they're included everywhere else):
Otherwise, this looks spectacular. Can we get a quick reroll?
Comment #60
Bojhan CreditAttribution: Bojhan commentedfix
Comment #61
quicksketchDarn whitespace. ;-)
Comment #62
quicksketchBumping this back down. After applying the patch, er, nothing seems to happen. The log message doesn't automatically check the "Create new revision" option, the Logo theme settings don't hide. Either we broke something somewhere or I don't know how to apply this patch.
Comment #63
quicksketchAlright, found it, a missing bracket after #60 and #61 (whoops). Anyone want to give this one last confirmation?
Comment #64
Bojhan CreditAttribution: Bojhan commentedWorks!
Comment #65
sunLooks good.
Comment #66
kkaefer CreditAttribution: kkaefer commentedThanks for helping to bring this home!
Comment #67
Dries CreditAttribution: Dries commentedThis looks good. Committed. Can we get a CHANGELOG.txt entry?
Comment #68
webchicktagging.
Comment #69
RobLoachFollow up issue: #622432: DX: Start using the "container" form type.
Comment #70
sun.
Comment #71
Dave ReidSince there still isn't any documentation on this, does anyone know how this works for making something dependent on a specific option(s) in a select field?
Comment #72
yched CreditAttribution: yched commented#71 : I couldn't make it work either. Doesn't seem to work right now.
Comment #73
peterpoe CreditAttribution: peterpoe commentedI did some tests on D7-alpha1, and it appears that states.js "knows" about the existence of these states (in states.State.aliases):
'enabled'
'invisible'
'invalid'
'untouched'
'optional'
'filled'
'unchecked'
'irrelevant'
'expanded'
'readwrite'
But the only states handlers actually implemented are: enabled/disabled, optional/required, invisible/visible, checked/unchecked, expanded/collapsed.
The optional/required state is actually useless since it only adds/removes a class on the form item – the form validation is untouched.
This is a form that I used to test the states API (it's identical to the forms in Drupal core, like the one in #49):
I would like to help testing this great new feature, but I don't know where it is heading, since states.js has not been updated in 3 months and no new patches have appeared here.
Comment #74
YK85 CreditAttribution: YK85 commentedsubscribing
Comment #75
rfaysubscribing. I note that kkaefer has proposed a session on this.
Rob Loach is also threatening a new example for the Examples module demonstrating the States system.
Somehow we need to come out of this with decent docs.
Comment #76
hctom#71:
Hi Dave,
I found out, that you may use a following approach for select fields (using a single select value... I'm not sure if this would work with multi-select fields):
Unfortunately, the 'value' trigger is only fired on key up... so if you change the select field using the keyboard, everything, works fine... if you do it by mouse, nothing happens :-(
@all: is something planned to extend the 'value' trigger, so it is gets trigger by mouse clicks on select fields or radio button groups? I'd really appreciate that, so you may also use those form elements to build dependent containers.
Thanx in advance & cheers
hctom
Comment #77
yoroy CreditAttribution: yoroy commentedAnother tag…
Comment #78
casey CreditAttribution: casey commented@hctom I think what you point out is related to #684602: IE BUG 1: menu options don't show up automatically
Comment #79
hctom@casey: no I don't think so, as it deals with checkboxes, not select fields...
And the states.js code says the following around line 250:
As far as I see, this means that 'value' only gets triggered by a 'keyup' event. or am I completely wrong?
Comment #80
Dave ReidOk yep I can confirm using the code in #76 does technically work, but its only triggered by keyboard, not mouse click. Very very bad. Who's up for fixing it?
Comment #81
Dave ReidAlso not sure how I can get the same invisible state based on more than one possible value in a select (if value was '1' or if value was '2').
Comment #82
Dave ReidI changed 'keyup' to 'change' in states.js and it worked perfectly for me.
Comment #83
Dave ReidRobLoach and I got a workaround for multiple values in a select by utilizing both an 'invisible' state and a 'visible' state which worked in my case, but I suspect others may want to be using multiple values.
Here's the patch I had to get this to respond to clicking and changing a select field. The 'value' trigger is not used elsewhere in core, so it's manually tested with my XML sitemap D7 port with both clicking and using keyboard (tab to field, select value, tab to next field).
Comment #84
hctom@Dave: I'm not sure, if the keyup also needs to be a triggering event, too... otherwise this wont work for text input values :-(
Comment #85
Dave ReidWell let's just add both then. Tested and this worked if I was typing in a textfield and had:
Comment #86
hctomyeah.. that's a nice one.. thanx for that.. I hope it will make it into the core code as soon as possible :-)
cheers
hctom
Comment #87
webchickIt'd be great to get kkaefer's take on these changes, if possible.
Comment #88
kkaefer CreditAttribution: kkaefer commentedI'll comment on the weekend.
Comment #89
kkaefer CreditAttribution: kkaefer commented@peterpoe: This is correct. The states `states.js` knows about are merely aliases for yet to be implemented handlers. It's also correct that `states.js` does not influence server side validation whatsoever. The only thing `states.js` does is purely presentational. You still have to code the server side validation code yourself. However, one could imagine a system which takes the specifications for the client side validation and performs these validations also on the server side level.
@hctom: Currently, the value callback is only implemented for textfields. It's not a problem to extend states beyond that and also fire the value callback when the user changes a select box. In `states.Trigger.states`, these functions are defined. Note that it also doesn't yet work fully for textfields either: When the user pastes content with the context menu (i.e. without using keyboard shortcuts), the change is not detected.
@Dave Reid: The second patch should be good.
Regarding the multi-value dependency... I tried various ways of storing this data in JSON so that it's still readable and didn't come up with a solution that allows other operators than AND (of course, it's possible with a more complicated data structure). A workaround is to use !!visible, !!!!visible, !!!!!!visible and so on.
Comment #90
joachim CreditAttribution: joachim commentedSubscribing.
I'm afraid I don't have time right now to add anything more constructive, other than to note that Views 2 has something like this in its options forms for handlers; are we using a similar structure for the FormAPI arrays here?
Comment #91
catchPer #89, #85 is RTBC.
Comment #92
catchreally.
Comment #93
sunLet's get this sucker in.
Comment #94
webchickCommitted to HEAD. Thanks!
Comment #95
jjeff CreditAttribution: jjeff commentedIt is possible to fill textfields without triggering a 'keyup' event. A good example of this is to double-click on an empty textfield in Firefox. Firefox will show a list of previously entered values on fields like this one in the past. You can click on a value from the list and that value will be entered into the field -- and you have clicked no keys.
When this happens, the associated state-change Javascript never fires.
My guess is that
states.Trigger.states.empty
needs to work similar tostates.Trigger.states.value
, binding to both the 'keyup' and 'change' events.Comment #96
kkaefer CreditAttribution: kkaefer commentedjjeff, the same is true for right-click, paste.
Comment #97
jjeff CreditAttribution: jjeff commentedAdditionally, I think that the documentation in drupal_process_states() is wrong.
According to my research we should probably break the $elements list into two sections:
Possible states for listener fields (all are boolean)
Possible states for caller fields (these trigger changes in the listener elements)
I realize that I'm making up the words "caller" and "listener", but there doesn't seem to be any precedence here for anything other than just calling everything "states"... or "dependant" and "dependee" (neither of which are English words).
Of course, the major problem is that drupal_process_states() is simply the PHP function which processes these states for the FAPI. These states are extendable with Javascript and so it's not really the right place to put this documentation. The right place for this documentation *would* be in states.js, but as far as I can tell, the API module doesn't parse .js files. So maybe the documentation in drupal_process_states() should just mention that these are the states offered as part of the core implementation of the states system.
Do the above lists look correct?
Comment #98
yoroy CreditAttribution: yoroy commentedreview of #97 requested :)
Comment #99
jjeff CreditAttribution: jjeff commentedNote:
We've been making a new Lullabot video about Javascript handling in Drupal. In preparation for a chapter about the State System, I've been creating a demonstration module. I've found a lot of unexpected behavior with this library. I will try to submit my module when I can to show the unexpected behavior, but I wanted to just note here that we've been seeing some problems.
This code should really be checked in a real world situation (examples.module?) as soon as possible.
Comment #100
sunNot sure whether it makes sense to keep on re-opening this issue... Can we link to follow-up issues from here instead?
The first one exists already:
#667944: Javascript #states cannot hide fieldsets, radios, checkboxes
Comment #101
andrewmacpherson CreditAttribution: andrewmacpherson commentedstarting a tag for the follow-ups...
Comment #102
kkaefer CreditAttribution: kkaefer commented#735528: FAPI #states: Fix conditionals to allow OR and XOR constructions
Comment #103
casey CreditAttribution: casey commentedI agree with jjeff in #97 states.js can use better self-explanatory variable names (also the variable name "value" is used for multiple purposes) and more documentation.
Comment #104
rfaySo #states is not even in the Form API Reference? What's up with that?
If somebody can just do the tiniest writeup on it, I'll put it in the Form API Reference!
Is there a handbook page at all?
If somebody wants to roll a simple teaching example, I'll commit it to Examples module.
Great things like this should not be left unexplained.
Comment #105
rfayI added the existing documentation from common.inc into the
Form API Reference.
This should show up there within a few hours. Please review what I put. It's not enough, but it's a start.
BTW: I was successful using states in my D7 port of Amazon module today. Thanks.
States is also now in the Examples for Developers project "Form Example" module. Demonstrated at http://d7.drupalexamples.info/form_example/states.
Comment #106
rfayI'm going to call this fixed for now - FAPI reference has it, and Examples has an example.
There's lots to do in the future to make it really sing, but this is a great enhancement to the Drupal world.
Followups at:
#767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states
#735528: FAPI #states: Fix conditionals to allow OR and XOR constructions
#767268: #states doesn't work correctly with radio buttons
#767212: #states can't hide/show fieldsets
Comment #108
andypostThere's a bug with states #1091852: Display Bug when using #states (Forms API) with Ajax Request
Comment #109
lulolulito CreditAttribution: lulolulito commentedI M tired of trying to fix my site, I need a drupal programmer, will pay!
I need to fix http://www.dealsinformant.com
Please help
Comment #110
hctom@lulolito: What about using Drupal instead of Wordpress... i guess that would fix all your problems at once ;-)
But thanx for spamming!!!