It seems that #states with radios doesn't work quite right.

It works when the page is loaded, but doesn't change when the radio is changed.

So in the example below, if 'one' is selected, the checkbox is unchecked on page load. If 'two' is selected, the checkbox is checked on page load. Works OK that way. But when you change the radios, it doesn't change the checkbox.

  $form['dummy_radios'] = array(
    '#type' => 'radios',
    '#options' => array('one' => 'one', 'two' => 'two'),
  );
  $form['dummy_checkbox'] = array(
    '#type' => 'checkbox',
    '#title' => 'dummy checkbox',
    '#description' => 'checked if two checked above',
    '#states' => array(
      'checked' => array(
        'input[value=two]' => array('checked' => TRUE),
      ),
    ),
  );
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Confirming.

sun’s picture

Status: Active » Needs review
FileSize
1.9 KB

Works for me.

sun’s picture

Now also explaining that condition. :)

sun’s picture

Assigned: Unassigned » sun
rfay’s picture

Status: Needs review » Needs work
FileSize
1.27 KB

Doesn't work for me. Firefox 3.5, Linux.

Attached is a demonstration module. Use the "Bug Demo: #states radios element not working" menu entry.

What it *does* do: If you reload the page, the correct action is taken for the selected item. But not if you change it on the page.

If you think I should have different syntax in the example, I'm sure you'll let me know :-)

rfay’s picture

kkafer said in his drupalcon presentation that radios were not supported at this point. @sun, have you had a chance to take a look at the demonstration module in #5 to see if you could make it go?

sun’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

Thanks for that demo module. Attached patch works for me.

sun’s picture

oh, but you need to use the following in your code instead:

  $form['sample_radios'] = array(
    '#type' => 'radios',
    '#title' => t('A selection'),
    '#options' => array('one' => 'one', 'two' => 'two'),
  );
  $form['input_1'] = array(
    '#type' => 'textfield',
    '#title' => t('Only shows when "one" is selected in radios'),
    '#states' => array(
      'visible' => array(
        #':input[value=one]' => array('checked' => TRUE),
        ':input[name="sample_radios"]' => array('value' => 'one'),
      ),
    ),
  );
  $form['input_2'] = array(
    '#type' => 'textfield',
    '#title' => t('Only shows when "two" is selected in radios'),
    '#states' => array(
      'visible' => array(
        #':input[value=two]' => array('checked' => TRUE),
        ':input[name="sample_radios"]' => array('value' => 'two'),
      ),
    ),
  );
rfay’s picture

Worked for me!

The attached patch is just sun's #7 again, with a demonstration added to form_test.module. I also changed the hook_menu() entries to remove the #type => MENU_CALLBACK, as I believe the various items should be easily accessible for manual testing.

All a part of my campaign to make sure that issues are understandable to the reader and manually testable, whether or not we can test them with the current PIFR system.

To test this patch, enable form_test.module (drush enable form_test) and access the menu named "#states Radio button demonstration" (/form-test/radios-demonstration). Then click the radio buttons and watch the textfield element change.

Thanks for your effort on this, sun.

sun’s picture

Hm. Although I fully understand your intention, I'm not sure whether it is a good idea to add demonstration code resp. manual testing code in a uncontrolled and unplanned fashion like this. I would recommend to remove this addition for now and start a separate issue to discuss and shape up a proper, agreed-on way to do this. For example,

- you made various menu items visible, which I'd agree with, but perhaps doesn't make sense for all.

- the code duplicates what is in Examples module already.

- the added form constructor is specific to #states with radios. Doesn't make sense for me, as there could be a single one that allows to manually test all kind of #states for various elements.

...and probably more.

+++ modules/simpletest/tests/form_test.module
@@ -1082,3 +1072,38 @@ function form_test_two_instances() {
+
+

Duplicate newline here.

+++ modules/simpletest/tests/form_test.module
@@ -1082,3 +1072,38 @@ function form_test_two_instances() {
+ *
+ * Although we do not have testing for javascript, we can certainly include
+ * demonstration code in the module. To demonstrate, access the menu entry
+ * and click the first or second radio button. Either input_1 or input_2 will
+ * show depending on which you click.

We can drop this description.

+++ modules/simpletest/tests/form_test.module
@@ -1082,3 +1072,38 @@ function form_test_two_instances() {
+function form_test_states_radios($form, &$form_state) {
+

Initial newline should be removed.

+++ modules/simpletest/tests/form_test.module
@@ -1082,3 +1072,38 @@ function form_test_two_instances() {
+    '#title' => t('A selection'),
...
+    '#title' => t('Only shows when "one" is selected in radios'),
...
+    '#title' => t('Only shows when "two" is selected in radios'),

We should remove t() here.

+++ modules/simpletest/tests/form_test.module
@@ -1082,3 +1072,38 @@ function form_test_two_instances() {
+    '#states' => array(
+      'visible' => array(
+        ':input[name="sample_radios"]' => array('value' => 'one'),      ),
+    ),
...
+    '#states' => array(
+      'visible' => array(
+        ':input[name="sample_radios"]' => array('value' => 'two'),      ),
+    ),

Invalid multi-line array syntax.

94 critical left. Go review some!

rfay’s picture

Thanks for the good thinking, @sun.

I've been talking up this idea with various people and got good response from the likes of webchick and dries, but without a clear plan or approach, so this is a good beginning discussion.

Hm. Although I fully understand your intention, I'm not sure whether it is a good idea to add demonstration code resp. manual testing code in a uncontrolled and unplanned fashion like this. I would recommend to remove this addition for now and start a separate issue to discuss and shape up a proper, agreed-on way to do this. For example,

I would agree with you... except that what we currently do with all test code is to add it in an uncontrolled and unplanned fashion. If we had an overriding plan for test code, we should follow that plan, but it's not our current way.

- you made various menu items visible, which I'd agree with, but perhaps doesn't make sense for all.

I took a look and in this case I think all should be visible. The general approach of making a menu entry visible can't do any damage that I know of. But I'm certainly open to conversation.

- the code duplicates what is in Examples module already.

Actually the purpose of this type of code and the purpose of Examples are different, especially in the development process. The purpose here is to demonstrate, where the purpose in Examples is to communicate (and demonstrate, I suppose). So the idea here is just to put code in that can be used in the context of this issue.

- the added form constructor is specific to #states with radios. Doesn't make sense for me, as there could be a single one that allows to manually test all kind of #states for various elements.

I would think that as we add related tests/demonstrations, we would add to this form constructor. My intent was to use the same approach we currently use with tests.

And of course all the rest of your comments are style, and you're the master of style.

If I knew a better place to discuss this I'd put the discussion there, but for now I'll just try to convince you :-)

The intent: Deliberately make issues manually testable, whether machine-testable or not. We waste mindspace if we don't do this.

The style: Just folllowing what we currently do for tests.

What do you say?

sun’s picture

well, we can let the core maintainers decide. However, we still need to fix the issues I mentioned.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +FAPI #states
FileSize
8.91 KB

Fixed those issues. Let's fix this, please.

rfay’s picture

Yay, I agree RTBC. Thanks, sun.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm. This patch is a bit of a monster. I don't really agree with the whitespace changes and have no idea where it's documented that making this stuff less readable is a good idea. I agree in principle about removing MENU_CALLBACK from the test module, but am of mixed feelings about it being done here and not in other test modules, because it creates inconsistency.

Could we pull that stuff out and just fix this bug?

rfay’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.4 KB

Rerolled without the demonstration test. Pushing to RTBC as it already was.

This one will slightly conflict with #767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states, so if it goes first that one will need a reroll.

rfay’s picture

Put the demonstration test back in, removed the MENU_CALLBACK changes. Per IRC communication/confusion/bafflement with webchick :-)

Still RTBC.

sun’s picture

webchick had the same reaction as me, and so I think we should defer the demonstration code stuff to a separate issue, as mentioned before.

Let's just fix the bug here. Documentation for #states in common.inc is completely revamped in another issue already.

rfay’s picture

@sun, webchick wanted the demonstration stuff, and asked me to put it back into #17. I agree that if we pull the changes to the docblock it will make life easier - but we'll have to add the stuff about radios over there.

sun’s picture

webchick can and should overrule me, for sure. But my gut feeling still is that we should coordinate the addition of "demonstration code for manual testing" a bit more. If I'm not mistaken, not all testing modules can be enabled safely, i.e. without potentially doing harmful stuff to your site. And when I somehow manage to enable a testing module for manual testing, then I'd probably expect some kind of sane structure...

For example, I recently started to work on a "proper" markup_test module in my sandbox, which tries to introduce and build a sane technical structure for manually testing + reviewing issues for the "markup" component, but with the goal to do it in a generic and extensible way in the end. Ultimately, when there's some more code and experience with that module, it will be worth to have its own d.o project, as you will likely be able to use the same testing code to ensure proper styling + behaviors in contributed and custom themes you build.

That markup_test module, as well as the Examples module, as well as the suggested demonstration code here, may be partially but not fully duplicating efforts, but I'm not sure yet. What rather matters for me is that we don't add arbitrary code to Drupal core with no clear vision, structure, and behavior -- at least not at this stage of the release cycle.

rfay’s picture

Created #785362: Create more extensive guidelines for "mock module" creation and style

I definitely agree that we can duplicate code with the demonstration technique... and that we have been doing that regularly since we began the whole simpletest thing. I don't think it's necessarily bad.

rfay’s picture

I have no objection to any of the patches above. My intent was to do a first proof-of-concept of a manual test that could accompany a patch. #17 does that.

#18 has just the fix, nothing else, and will not collide with the documentation being prepared in #767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states, although that one will have to be updated with the caveat about using 'value'.

#17 has the fix with some documentation and with the demonstration manual test.

#16 has the fix with some documentation and without the manual test.

#13 has the fix with some documentation and with the manual test, and with setting type=MENU_CALLBACK on some of the items.

It's not worth getting stuck on this. @webchick, please choose one and go for it. We'll work this out in the future. When this is fixed, a demonstration will go into Examples modules, so this issue doesn't probably strictly need the demonstration.

sun’s picture

@rfay: While we're waiting for a commit here, I'd happily welcome your review on #783438: #states doesn't work for #type item :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. Committed #18 to HEAD!

tstoeckler’s picture

Status: Fixed » Needs work

Can someone confirm this actually works on current HEAD? I just updated my HEAD and I cannot get it to work.
I would provide sample code, but to see if I wasn't making any stupid mistakes I copied the exact code from the documentation and tried again with no success. Also using 'checked' => TRUE works for me (except that it doesn't make much sense for radios).
Thanks.

rfay’s picture

Status: Needs work » Fixed
FileSize
2.65 KB

@tstoeckler, works for me. Attached is the module I just used for testing it.

Use the menu entry: Bug Demo: #states radios element not working (bug_demo/states_radios)

tstoeckler’s picture

#states radios element not working

That's how it's called, and in fact it does not work for me. Todays HEAD.

rfay’s picture

@tstoeckler, we all want it to work correctly, and believe me we don't doubt that you may have a bug. Are you saying that the test module I gave doesn't work for you? If so, please provide details that may help to sort this out. Or is there another construct that doesn't work? If so, please adjust the test module and give us a demonstration.

tstoeckler’s picture

Damn...
My jQuery version was corrupted. Don't ask me how that happened. Got a fresh copy of jQuery and the demo module works perfectly now, as well my own code.
Sorry for the noise.

Status: Fixed » Closed (fixed)

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

jason.fisher’s picture

I know this is closed now, but this could be extended to also properly support "checked" for radio buttons, as per the original request.

Would that be a patch on this existing issue, or a new issue?

  checked: {
    'change': function () {
      if (this.length > 1) {
        // Returns true if any radio elements are selected.
        return this.filter(':checked').attr('checked') || false;
      }
      return this.attr('checked');
    }
  },

  // For radio buttons, only return the value if the radio button is selected.
  value: {
    'keyup': function () {
      // Radio buttons share the same :input[name="key"] selector.

That addition allows the second example here to work exactly as the first (which the already committed patch addresses):


  $form['radio_driver'] = array(
    '#type' => 'radios',
    '#title' => t('Radios to drive a textfield'),
    '#options' => array("1" => t('One'), "2" => t('Two'), "3" => t('Three'))
  );  

   $form['textfield_hidden_by_noradio_noval'] = array(
    '#type' => 'textfield',
    '#title' => t('This should be disabled when any of the radio elements are selected, using !value => false.  This is like: radio != ""'),
    '#states' => array(
      'disabled' => array(
        ':input[name=radio_driver]' => array('!value' => false),
      ),
    ),
  );

   $form['textfield_hidden_by_noradio_checked'] = array(
    '#type' => 'textfield',
    '#title' => t('This should be disabled when any of the radio elements are selected, using checked => true.  This is also like radio != ""'),
    '#states' => array(
      'disabled' => array(
        ':input[name=radio_driver]' => array('checked' => true),
      ),
    ),
  );

vipul.jadvani’s picture

The last submitted patch, 9: drupal.states_radio_buttons_767268_08.patch, failed testing.