#states can cause the form "required" mark to appear more than once on the same element

Can you post the code showing the bug please? So that it can be reproduced.

Also, should be fixed on D8 first. #states js is the same for D8 and D7 so reproducing in either works.

I'm not really a JS guy, but couldn't this use .once()?

you're right this should be using .once.

There is two issues really, ctool should be passing an argument to Drupal.attachBehaviors() and this code doesn't use once.

Can you update the patch please? I can't rtbc if i'm involved :)

I did some further research into this issue and #1655114: FAPI #states: required span added multiple times. They are closely related, so I will close the other one as a duplicate, hoping that this issue will solve both cases.

Test cases

Manual test case for D7 where #8 works. Add the following lines to image.field.inc, function function image_field_formatter_settings_form(), $element['image_link'], line 517:

    // manual test case 1 #1592688: #states can cause the form "required" mark to appear more than once on the same element begin
    '#states' => array(
      'required' => array(':input[name="fields[' . $field['field_name'] . '][settings_edit_form][settings][image_style]"]' => array(array('value' => 'thumbnail'), array('value' => 'medium'))),
    ),
    // manual test case 1 #1592688: #states can cause the form "required" mark to appear more than once on the same element end

Manual test case for D7 where #8 does not work. Add the following lines to image.field.inc, function function image_field_formatter_settings_form(), $element['image_link'], line 517:

    // manual test case 2 #1592688: #states can cause the form "required" mark to appear more than once on the same element begin
    '#states' => array(
      'enabled' => array(':input[name="fields[' . $field['field_name'] . '][settings_edit_form][settings][image_style]"]' => array(array('value' => 'thumbnail'), array('value' => 'medium'))),
      'required' => array(':input[name="fields[' . $field['field_name'] . '][settings_edit_form][settings][image_style]"]' => array(array('value' => 'thumbnail'), array('value' => 'medium'))),
    ),
    // manual test case 2 #1592688: #states can cause the form "required" mark to appear more than once on the same element end

How to test?

Make sure you have a content type with an image field. Go to "manage display" for that content type and open the settings for the image field. The logic behind the sates is something like "if you select thumbnail or medium as image style you must specify a link, otherwise it will not be linked". So play a bit with the settings and see what happens.

The code I abused for the test cases seems to be pretty much the same in D8. So I guess the test cases can be used in D8 as well.

Findings

• It is ajax related. I could not get a similar test case on the node add/edit form to fail.
• The patch from #8 works when there is only 1 attribute (e.g. 'required') to change.
• The patch from #8 fails when there are more than 1 attribute (e.g. 'enabled' and 'required') that have to react to an event.

The solution thus looks to be something like "the once behavior should be on the "element/attribute combination", not on the element alone." Can we pass in the "attribute" to once: once('states-required'), once('states-enabled'),etc. Do we have this info in the attach behavior?

One more "needs work":
Looking at the code in initializeDependee (in states.js):

initializeDependee: function (selector, dependeeStates) {
    ...
    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;
        }

$.inArray(state, dependeeStates) will always equals i!

So it seems that this code tries to achieve something similar asw eare trying to achieve here, but in an incorrect way. BTW: this code was introduced by #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions.

Replying to #11:
Yes, that was simple and works, but still has a problem:

If you also set the #required option at the server side, and you should as you may never rely on client-side (javascript based) validation only, you will get 2 markers if the settings you start with lead to the #required set to be true.

Changed the test case to show this:

    // manual test case 2 #1592688: #states can cause the form "required" mark to appear more than once on the same element begin
    '#required' => $settings['image_style'] === 'thumbnail' || $settings['image_style'] === 'medium',
    '#states' => array(
      'enabled' => array(':input[name="fields[' . $field['field_name'] . '][settings_edit_form][settings][image_style]"]' => array(array('value' => 'thumbnail'), array('value' => 'medium'))),
      'required' => array(':input[name="fields[' . $field['field_name'] . '][settings_edit_form][settings][image_style]"]' => array(array('value' => 'thumbnail'), array('value' => 'medium'))),
    ),
    // manual test case 2 #1592688: #states can cause the form "required" mark to appear more than once on the same element end

Test: set style to thumbnail or medium, save and reopen the settings form again.

Solution: always remove any current required marker first: states.js, line 492 (in D7). (Note that this always works for every situation, the other solutions can be seen as just a performance optimization to prevent to many events being fired and handled.)

$(document).bind('state:required', function(e) {
  if (e.trigger) {
    $(e.target).closest('.form-item, .form-wrapper').find('label .form-required').remove();
    if (e.value) {
      $(e.target).closest('.form-item, .form-wrapper').find('label').append('<span class="form-required">*</span>');
    }
  }
});

The patch also does not solve/remove the mentioned portion of bogus code. But as that code in essence seems harmless, we may defer that to a follow-up issue.

A last possible problem I see is that states.js allows to use aliases like visible versus invisible and required versus optional. Are these handled correctly? Thus if I have both an required and an optional state on a certain element, won't I get 2 markers if the required state evaluates to true and the optional to false? (Not with the solution introduced above in this comment ... but what if we solve that differently?)

+++ www/core/misc/states.js
@@ -19,7 +19,7 @@
     for (var selector in settings.states) {
       for (var state in settings.states[selector]) {
         new states.Dependent({
-          element: $(selector),
+          element: $(selector).once('states'),

The location where .once() is invoked does not seem right to me.

With this code, a new Dependent is actually instantiated on subsequent calls, but it gets no element (an empty jQuery object) assigned. That seems bogus to me.

Additionally, it is perfectly legit to bind multiple different #states behaviors to a single element. For example, consider two other elements that both depend on the checked state of a single checkbox, and similar scenarios. This behavior currently works in HEAD and must be retained.

Furthermore, we also need to carefully consider the expectations of combined #states with #ajax here:

In that case, it is possible that an Ajax response contains new #states settings, which may need to override existing #states for a particular element; i.e., changing its behavior, since the element was replaced or whatever through the Ajax behavior.

Fixing tags per http://drupal.org/node/1517250

I applied the suggested change in #17 and can confirm it does work. We were actually seeing a lot more than two required asterisks (it added three or four every time), and the fix above (applied to D7) resolved the issue for me.

Needs summary

Any progress on a patch for D7 here? We're waiting on this issue for launch.

@#17:
if (element.size() > 0) {
could be replaced with
if (element.length) {

$.fn.size is just a silly wrapper around grabbing the length property: https://github.com/jquery/jquery/blob/master/src/core.js#L184

For those looking for a quick solution before this gets committed / backported to D7, try this CSS:

label .form-required {
   display: none;
}
label .form-required:last-child {
   display: inline;
}

For those looking for a quick solution before this gets committed / backported to D7, try this CSS:

label .form-required {
   display: none;
}
label .form-required:last-child {
   display: inline;
}

Issue summary by ergophobe for status as of Jan 29, 2013

formatting changes

removing Issue summary initiative tag

We've experienced similar issues with states system and here is a patch for D7 states to better handle AJAX requests and multiple calls to behaviors attaching on the same content in general. Major differences:

• Add instance counting to the states.Dependent instances and use instance ID for the dependee flag that lets us know if state event handler have been attached to this dependee or not. Full flag key is built from instance ID + state name + dependee selector.
• Save states.Dependent instance to the element data and try to use existing instance. Instance key contains dependent selector + state.
• Move initialization to a separate method, so it could be called multiple times on the same instance.

Solution is still far from perfect, because it doesn't handle changed settings passed with AJAX requests, it just re-attaches event handlers to new dependees properly.

handling of #states has been changed, can you please retest to see if the problem still exists?

nod_: assuming this refers to D8?

I can find manual test instructions for D7 but not for D8, if anyone can provide these i am willing to test.

formatting

Posting #17 as a patch

Setting back issue to previous status

Hi,

I did a small change to the patch in #29 as it was preventing the application of more then one state.

I particularly had a case where I was using visible and required states on one element and the second one was not applied.

I can verify that this solves the problem in Drupal 7. Attached patch is for jQuery Update module for anyone using that. Will try to reproduce this in D8 later.

This file will be removed asap from jquery update. See #1448490: Remove the states.js overwrite as it was fixed upstream

The following test patch for Drupal 8 adds some form elements to the user login form. Interestingly I've found that #states are apparently not run at all on the part of the form that is being inserted via Ajax. I didn't debug that very much though, so there might be something wrong with the example code as well.

Can we just get this issue fixed in D7 if we don't know we can reproduce it even in D8 (and it is done in a completely different way).

Let's try the patch from #29 here on #2130501: Cosmetic issues on the node/add/project-module page (could also try it on 7.x in general). making that one active/needs review again.

#29 worked great in #2130501: Cosmetic issues on the node/add/project-module page.

Here's a reroll of 31 . Also attached is a Drupal 7 test module where you can test several scenarios on scratch/states_issue . The modification in #31 of #29 is necessary to allow both visible and required states on the form element.

To test with the module, check "Fetch the first name textfield via AJAX", then play with the hide and require checkboxes. The disable checkbox on top is to demonstrate another issue, for which I will reopen #2226405: FAPI #states: dependent element added via AJAX initializes incorrectly if dependee has been initialized earlier , because this is quite a confusing issue atm.

Apologies, here's the test module (see 42).

Added flag to each element then States init.
So 'new states.Dependent()' never twice executed. This fixed problem with CTools modal forms.

Patch still applies to D7. Seems like a simple fix. Would be great to get this RTBC & into Core.

It's a long time I had a look at thi sproblem, but my intuition (on quickly comparing #42 and #44) says that #42 is correct and that #44 is incorrect.

- What's wrong with once()? In Drupal I see that pattern more often than the "data pattern".
- The once is per selector per state, whereas the data is per selector. The latter seems wrong to me.

So I would like to go for the patch from #42 but with the following remark:

+++ b/misc/states.js
@@ -19,11 +19,14 @@ Drupal.behaviors.states = {
+        var element = $(selector).once('states-' + state);
+        if (element.length) {

once() accepts a function as 2nd parameter to execute on the elements that "passed" the once. So we can combine this into something like:

$(selector).once('states-' + state, function() {
    new states.Dependent({
      ...
    }
  });

I was using #42 with jQuery 1.8 (from https://drupal.org/project/jquery_update) and it breaks the WYSIWYG module. There's a JavaScript error on the page: "Syntax error, unrecognized expression: .states-!enabled-processed". After downgrading to jQuery 1.7 it works fine though.

Also are we sure this issue isn't still valid for Drupal 8? Doesn't look like it's been tested recently.

I tried the tests from #10 and #34 and could NOT reproduce this in D8, while #10 still fails in d7. So, I think it is safe to continue solving it for D7 here.

Regarding the !enabled: this seems to be allowed, so we should cater for that. Luckily for us, there's already a function that handles this for us:
states.State.sanitize(): returns a states.State object whose property name will be sanitized (will not contain !). So attached patch should solve #46 and #47 (and is thus based on #42).

#48 still works well, can anyone else review? Would be good if we could get this to RTBC!

Applied and tested #48, which resolves the issue more elegantly than #2125563: 'Required' state doesn't work correctly for a form element, which is now closed duplicate to this.

Marking RTBC.

After having bitten by this issue quite a few times and having this patch applied on a couple of live sites I'm no longer sure whether this patch is a good idea. The problem is that with this #states are no longer re-applied after Ajax requests. So when using #states in ajaxified forms this can lead to unwanted results. If an Ajax requests results in new form elements, for example, these will never participate in the #states system at all because all those behavior have already run (once). That has bitten me in combination with the Addressfield module for example.

I think in this case it really makes sense to fix the actual "symptom" of this issue only, i.e. the double required asterisks that appear after form elements when making form elements required via #states. Before adding such an asterisk we should check whether such an asterisk already exists and if so, simply bail.

Don't want to block this, though, just wanted to give my perspective because I used to very much endorse this issue but no longer do.

Also I'm not entirely sure why this issue targets 7.x. In #34 I investigated whether this issue still exists in D8 and that was never conclusively disproven. I suspect it does, to be honest. If that were to be true this must be fixed in Drupal 8 first per our process.

Differences in D7 and D8 states.js are quite minimal (ignoring white space), so I guess #53 is correct.

#51 is more difficult:
(not sure if the case distinguishing below is correct, as context will differ, but settings might also differ)
- If dependee and all dependents are refreshed: no problem, once is no longer set and the events are gone, so code will run.
- If nor the dependee, neither any of the dependents are refreshed: no problem
- If dependee is refreshed, but a dependent not: the once is gone, so events will be refreshed. Some events will remain, but those address
a no longer existing target dependee and thus can be ignored.
- if dependee is not refreshed, but dependents are: the once remains but the events are gone: incorrect.

Perhaps we should do an unbind and then execute the current code as normal.

This issue has already been fixed in Drupal 8.0.x

Here is the code in D8

  $(document).on('state:required', function (e) {
    if (e.trigger) {
      if (e.value) {
        var $label = $(e.target).attr({'required': 'required', 'aria-required': 'aria-required'}).closest('.form-item, .form-wrapper').find('label');
        // Avoids duplicate required markers on initialization.
        if (!$label.hasClass('form-required').length) {
          $label.addClass('form-required');
        }
      }
      else {
        $(e.target).removeAttr('required aria-required').closest('.form-item, .form-wrapper').find('label.form-required').removeClass('form-required');
      }
    }
  });

Regarding states working with Ajax, here is some thoughts:

- When I use form #ajax to implement field change event, the return element is this field element. However in the Drupal.behaviors.states the context is actually the form element instead. So we cannot use context to restrict which element needs to be applied with state.
- use jQuery.once() to restrict that apply state once will make state broken after form AJAX request.

Solution:
1. A decent solution is to avoid state apply more than once. In order to work with AJAX, we have to detach state change and restore the form element status to the initial status and then apply state again after AJAX. But this is way too complex to implement. For example we have to store initial state and compare.

2. A simple way (preferred)
Do not add restriction and apply state again after AJAX request. This should not add too much overhead and works well for all bind events except for 'required'. So we just need to check if '*' has already been added before appending a new one.

When I check Drupal 8 and I have found this has already been implemented in D8 as the same way.

I have attached a patch for D7.

A test use case is:

I have a gift subscription form to let users bug gift and send to their friends. This form allows them to notify gift to their friend either by email or by mail.

So if user chooses by email, an email field shows up as required field. Or user have to enter address fields with all required fields appear.

In the address fields user can select country and state list will show up depending on what country is via AJAX.

This (#55) may explain why we were not able to show this error still exists in D8. BTW: According to this thread on Stack overflow, the check is superfluous, jquery already checks this. For D7 it would be needed as we are adding a DOM element, not a class.

If we are going for symptom solving, then be aware that all these bound events add up quickly and eventually will harm client side performance, even if the events all do the same and do not have visual side effects.

#55 works for me.

Confirming #55 works.

- OK, let's go for symptom solving as per #51 and because solving it neatly would be extremely difficult as per #54.
- Tested as per #58 and #59
- Reviewed by me.
=> RTBC

Why aren't we doing this the same way Drupal 8 does?

The Drupal 8 code caches the selector in a variable so we don't needlessly run it twice, and also has a code comment explaining what's going on:

        var $label = $(e.target).attr({'required': 'required', 'aria-required': 'aria-required'}).closest('.form-item, .form-wrapper').find('label');
        // Avoids duplicate required markers on initialization.
        if (!$label.hasClass('form-required').length) {
          $label.addClass('form-required');
        }

Especially given that we're solving the symptom for now rather than a direct fix, I think we should make sure we don't waste extra cycles running the same selector twice.

Update patch with object cache suggested by @David_Rothstein

This patch addresses the concerns of #61.

BTW: the D8 code that #61 refers to is a bad example: the comment is superfluous as it is done purely with classes, not by adding a child element (twice) and addClass() already checks if the class is there so the hasClass() is superfluous as well (see e.g. http://stackoverflow.com/questions/13358887/should-i-use-hasclass-before...). But that should be a different issue.

Very much like #62 - elegant.

Hi guys, patch #62 worked for me.

+1 for the fix in #62

Committed to 7.x - thanks!

Nice and simple fix in the end, although since it didn't quite solve the full breadth of the problem implied by the original issue title, I'm retitling it now. I guess we can create a new issue if we want to keep looking for a more comprehensive fix.

Adding related issue #1091852: Display Bug when using #states (Forms API) with Ajax Request, which addresses the original problem this issue did not: the double-binding of the #states behavior.