JS #states behavior does not have a detach method

Hello alexander tallqvist , I tested the patch, and it worked for me.

The main issue here is that we do not have a detach method for the states script, which causes the various issues we can see here and the linked issue.

Haven't looked in details what's needed but we might need some extensive refactor to make everything work. First step here would be to make a failing test showing the issue so that we're sure we don't break it again. We have pretty extensive test coverage for this functionality so refactor will be safe as long as the tests are green.

@alexander tallqvist I'm unable to reproduce the problem. I tried with Entity reference - media and with media reference. With entity reference it worked fine, though I had to make changes in the alter function. The field selector needed to be
field_main_media[0][target_id] instead of field_main_media[target_id]

With media reference, it did not work. The field selector generated with target_id doesn't exists.

Can you share some more details on how to reproduce the problem.

I had the problem with states visibility on a custom form (which was in a modal with ajax reload) and I confirm that the patch fixes my problem :-)

The patch that exist is not the appropriate fix, it's broken in a different way :)

Thanks for the info @nod_ any idea which fix to apply yet ?

Works for me :-)

with the patch it will work however between the version of drupal 10.3.0 and 10.3.1 there were quite a few changes to the files:
-state.php
-state.js
- ...

see the release note for 10.3.1 here
https://git.drupalcode.org/project/drupal/-/commit/8905837f693c4f0341dd9...

I agree with nod_ that you will have other problems later.

Thanks for starting that code and writing a test! Haven't gone through in details but I can spot a few things already, see comments in MR.

changes as per MR comments

Thank you for your efforts so far but this doesn't completely fix the issue. I still have an issue with an element that is added by ajax and is the states dependee of an element that is not affected by ajax.

Here is a modified version of the form in the issue description that will highlight the bug.

Steps to reproduce

1. Create with form with Ajax and states as below.

    $form['js_states_test'] = [
      '#type' => 'details',
      '#open' => TRUE,
      '#prefix' => '<div id="js_states_test_wrapper">',
      '#suffix' => '</div>',
    ];
    $form['js_states_test']['reload'] = [
      '#type' => 'checkbox',
      '#title' => 'Check me to reload Select Field field',
      '#ajax' => [
        'callback' => '::buildAjax',
        'wrapper' => 'js_states_test_wrapper',
      ],
    ];
    $form['js_states_test']['select_field'] = [
      '#type' => 'select',
      '#title' => 'Select Field',
      '#options' => [0 => 0, 1 => 1],
      '#default_value' => 0,
    ];
    if ($form_state->getValue('reload')) {
      $form['js_states_test']['dependent_checkbox'] = [
        '#type' => 'checkbox',
        '#title' => 'Only visible if reload is checked',
      ];
    }
    $form['js_select_field_textfield'] = [
      '#type' => 'select',
      '#title' => 'Select should show when 1 is selected in select_field after ajax',
      '#states' => [
        'visible' => [
          ':input[name="select_field"]' => ['value' => 1],
        ],
      ],
    ];
    $form['js_checkbox_field_textfield'] = [
      '#type' => 'textfield',
      '#title' => 'Field should hide when dependent_checkbox is checked after ajax',
      '#states' => [
        'invisible' => [
          ':input[name="dependent_checkbox"]' => ['checked' => TRUE],
        ],
      ],
    ];

2. Click on "Check me to reload Select Field field" checkbox
3. Select value 1 for "Select field"
4. BUG: 'Select should show when 1 is selected in select_field after ajax' is not visible when it should be
5. Click on "Only visible if reload is checked" checkbox
6. BUG: "Field should hide when dependent_checkbox is checked after ajax" is not hidden when it should be

NOTE: This is a very simplified version of the problem we are experiencing. I know the "dependent_checkbox" doesn't have to be added via ajax and can easily be made visible via states based on "reload" and then there is no bug, but that is not possible in our situation. This is just an easy way to demonstrate the bug.

Proposed resolutions
In addition to the current MR !9300 changes which fixes #4 above, I suggest removing the dupal-once attribute if one or more of the dependees are not present in the DOM. This will ensure the states of that element get processed again when the DOM is updated.

Thoughts?

I will add suggested code as comments to the MR !9300.

I have updated the MR and added more comprehensive testing to highlight the ajax related bugs that still exist.

I moved the testing added to its own new method.

These new tests should cause the MR to fail as expected.

I have a working solution to fix all remaining issues that I am just doing final cleanup on and will commit and comment on shortly.

I have updated the MR and added to the great start by sukr_s. I was going to make code suggestions to the MR but there were a lot of changes so decided to commit to expedite things. I hope you don't mind.

All tests are now passing and its ready for review.

Summary of changes made to the MR:

1. Renamed the statesObjects property to processedDependees to hopefully be more descriptive of what it is.
2. Modified the states.Dependent function to keep track of any dependees not in the DOM so they can be rechecked on attach. Also ensured Dependents are re-evaluated if none of their dependees are in the DOM to ensure states are reprocessed when dependees are removed via ajax.
3. Fixed issue in Drupal.states.Dependent.destroy where it was removing the state:${dependeeStates[i]} event associated with other Dependents
4. Fixed issue where duplicate entires were being added to statesObjects property when states were reprocessed on ajax
5. Fixed issue in Drupal.states.Trigger.destroy where trigger related event listeners were not being removed.
6. Added functionality to the state of the attach function to ensure states are reprocessed for dependee elements that are added to the DOM during ajax

I found another bug.

If the ajax added dependee includes the form in its selector it doesn't work. It is completely valid for the dependee selector to include a form identifier if there are multiple forms on a page that have the same fields.

This bug is caused by using the find() method on the context when checking if the dependee is now present in the dom. This won't work when the selector contains form because on ajax reload the form will be the context root .

We need to instead use the jQuery Selector $(), which seems valid given that is what is used to apply the event listeners and also what was used to determine if the element is in the dom in the first place.

I have added testing for this bug and fixed it in the latest MR commits. But it looks like the changes has caused other failing tests. I'm investigating now.

All failing tests are fixed. Good for review.

So this fix has an unexpected affect in that the revision log message textfield is no longer displayed on node create forms.

I think what is happening with the fix is the states added to the revision_log_message in \Drupal\Core\Entity\ContentEntityForm::addRevisionableFormFields() is now being correctly handled because the revision checkbox is not present in the DOM so the field shouldn't be visible.

I wonder if it was intended to be shown or if this is a bug that slipped through. I found this possible related issue https://www.drupal.org/project/drupal/issues/3096906. Either way it would need to be addressed before this fixed is merged in.

Would be great to get a maintainers input on this. Thanks!

thanks for the work, just wanted to say that the issue is on my plate but it needs a good chunk of time to review. It's on my todo but can't say exactly when I'll have the focus to tackle it.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I've got a similar situation with a webform where one field controls the visibility of another and it is replaced by an Ajax call. I tried out both Patch #2 and the MR, and I can confirm that the MR still hasn't fixed the issue, but Patch #2 has.
Drupal version: 10.3.1

@andre.bonon are you able to provide code example of your problem as I am trying to ascertain how it is different to the current test.

Unfortunately Patch #2 is not the correct solution due to #3347144: Form API #states property/states should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations) and as nod_ has explained in previous comments. Therefore we need to work on an alternate solution and getting more info on your problem would be extremely helpful.

@mdsohaib4242 was your comment for @andre.bonon or does it relate to the current MR in some way? Thanks!

Ugh I have found another bug in the code.
The testing code is also becoming quite unwieldy. I am going to spend some time of the weekend to refactor the testing code and add testing and hopefully a fix for the new bug.

I was experiencing a bug where states were not applied to fields that are added via ajax when their trigger also targets another field that is not added via ajax.

I have added testing for this bug and a fix.

Back to Need Review!

What a gnarly bug :D

Just to say this patch has applied cleanly to Drupal 10.3.10 and resolved our client's issue:

1. Field primary_reason has an Ajax callback that populates & returns the secondary_reason field.
2. The additional_information field should show depending on the value chosen in the secondary_reason field, determined using the #states array. This was not working.

Now the patch is applied, the additional_information field is correctly showing/hiding.

JS test appears to consistently be failing, re ran twice.

Resolved the threads

Apologies that I can't contribute more to this, but I at least want to report that the MR does not fix the issue in my case. We're dealing with the version of the issue that has been reported in the Webform module queue(https://www.drupal.org/project/webform/issues/3481569). In the meantime we implemented the patch that reverts the patch that broke all of this. Extremely frustrating that such a breaking issue has existed in core for so long. My team was stuck on 10.3.0 waiting for this to get fixed, and finally had to settle for a horrible revert just to get core up to date.

Thanks for moving this forward @tame4tex, i checked the tests they make sense so +1 for them. Webform overrides some of the states API methods so any patch here will need an update on webform side

I replicated #3481569: Using a computed twig as a value for a visibility condition breaks in Drupal 10.3 and made a test locally and the MR fixes the issue. all good here.

For the JS  I think it's solid, elements and event handlers are cleaned up so memory shouldn't leak. I wish we could make the code simpler but I don't have a better way (:

Ran the test 3 times and the javascript fail each time. Moving to NW for that.

Thanks for the review @sophie.sk, @nod_, @smustgrave!

@nod: I too wish the code could be simpler. I have a hunch there might be a simpler approach but I think it would require major refactor of the existing code. I think it best to commit a fix for these Ajax bugs with as minimal change to existing code as possible and then we could potentially revisit states.js as a whole in future issue.

On that note, I have found another situation that is not correctly handled by the code which I have added a test and fix for.

If a trigger includes a form identifier, which would be necessary if multiples of the same form are on the page, states is no longer correctly applied when the trigger value is affected by Ajax. It was a simple fix in the detach method. The fix also highlighted an error in a test that needed to be corrected.

I will address the failing tests mentioned by @smustgrave in a separate comment as they are still failing and I am investigating.

So the failing tests are similar to the \Drupal\Core\Entity\ContentEntityForm::addRevisionableFormFields() issue mentioned in comment #24.

The tests are failing because form elements in \Drupal\migrate_drupal_ui\Form\CredentialForm are not visible when they are expected to be.

In \Drupal\migrate_drupal_ui\Form\CredentialForm numerous elements have a visible states declared based on the source_connection input having an empty value. Here is an example:

      '#type' => 'details',
      '#title' => $this->t('Source database'),
      '#description' => $this->t('Provide credentials for the database of the Drupal site you want to upgrade.'),
      '#open' => TRUE,
      '#states' => [
        'visible' => [
          ':input[name=source_connection]' => ['value' => ''],
        ],
      ],
    ];

This problem is the source_connection input is not always present in the DOM due to its #access setting:

    $form['source_connection'] = [
      '#type' => 'select',
      '#title' => $this->t('Source connection'),
      '#options' => $options,
      '#default_value' => array_pop($default_options),
      '#empty_option' => $this->t('- User defined -'),
      '#description' => $this->t('Choose one of the keys from the $databases array or else select "User defined" and enter database credentials.'),
      '#access' => !empty($options),
    ];

With the new code to better handle ajax added and removed elements, if an element has visible states declared and the associated triggering element is not present in the DOM then the visible state is not applied. The this.reevaluate(); code at line 226 is the cause of this change.

Previously, if the triggering element wasn't present, the states declaration was essentially ignored, so the element would always be visible.

The current fix feels more correct to me, in that for the state to be applied, the trigger element must first present in the DOM and then the trigger condition must be TRUE. Otherwise, the inverse state is applied. However, this is a change in states behaviour, so it would be good to get maintainer feedback.

I did try and refactor the code to ignore states if the triggering element wasn't present, but I was unable to find a solution that also correctly handled the states associated with a trigger element being added and removed via ajax.

In an attempt to keep this issue progressing forward I have refactored \Drupal\migrate_drupal_ui\Form\CredentialForm to fix the failing tests and all tests are now passing.

The changes to the migration form make sense. The tests in Drupal\Tests\migrate_drupal_ui\FunctionalJavascript\SettingsTest all pass, so that looks good to me as a migration maintainer.

Thanks @mikelutz for that

Checking the MR appears all threads are resolved

However when I run the test-only feature it passes when it should fail so not sure if the bug is being covered.

Thanks @mikelutz and @smustgrave

It looks like the test only feature wasn't failing because of #3494332: Functional Javascript tests are silently skipped on Gitlab CI due to selenium standalone configuration.

I have rebased the MR and it is now correctly failing.

I applied the changes in MR9300 to a site running 10.4.3 and now an element that has both ajax and states dependees only runs its ajax handler once. I'm going to keep digging but if that sparks any ideas please let me know.

Commenting out line 624 of the adjusted states.js stops the ajax event from being removed.

I was unable to get the test form to repeat the behaviour I'm seeing - possibly because it uses a checkbox as the ajax trigger rather than a select element, however I did discover the cause of our issue. The trigger events needed to be namespaced so that other events weren't stripped off along with them.

I have pushed a very small amendment to that effect.

Ah, got it.

Doing this to the test form does demonstrate the problem. wasn't sure if I should push the change though

Index: core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
--- a/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php	(revision 62764410c50313e7fd0068cde2f5f78e8c846d23)
+++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php	(date 1740698679180)
@@ -773,7 +773,9 @@
     ];
     // Ajax trigger.
     $form['ajax_reload'] = [
-      '#type' => 'checkbox',
+      '#type' => 'select',
+      '#options' => [0 => 0, 1 => 1],
+      '#default_value' => 0,
       '#title' => 'Check me to reload elements with the states_ajax_test_wrapper',
       '#ajax' => [
         'callback' => '::buildAjax',
@@ -818,6 +820,7 @@
         '#states' => [
           'enabled' => [
             ':input[name="checkbox_trigger"]' => ['checked' => TRUE],
+            ':input[name="ajax_reload"]' => ['value' => 1],
           ],
         ],
       ];

Yes that means there is a dependent/dependee pair that share both ajax and states relationships. It comes into play (at least for us) when ajax is used to update large sections of form that can have portions which use state to manage visibility based on the ajax trigger. And the form in question *did* work in 10.2

Just found an issue using this patch. When you have 2 forms on the same page, the states no longer seem to work.
When I remove the patch everything works as expected.
When I remove one of the forms, everything also works as expected.

I was not yet able to pinpoint the exact issue, but it seems to be caused by the patch I created from the MR.

Sounds like not fully ready based on #47

I don't see any issue on using the patch from MR having 2 forms on same page.
@seanb if possible can you add code snippet to demonstrate the issue.

It seems my issues were caused by an AJAX element in the form, that did not have proper arguments to make it unique. Which caused issues updating the form and caused the states of the wrong form to be detached. So it seemed related but the actual cause was something else. Sorry I didn't report back earlier, kinda forgot about this comment.

Setting back to needs review, functionally it works fine for me :)

Thanks for the update @seanb

Yes so about the code, it works, I don't see any memory leak (with a moderate usage) so that's good. The touchy subject is from #38:

The current fix feels more correct to me, in that for the state to be applied, the trigger element must first present in the DOM and then the trigger condition must be TRUE. Otherwise, the inverse state is applied. However, this is a change in states behaviour, so it would be good to get maintainer feedback.

I'm a bit uncomfortable with that change, it does feel like a big change and people will wonder what's going on. I'd like to have some webform or other heavy users of #states involved here because I don't want to break all #states forms that are not correctly written. Like if you have a typo in the states config, it used to work and now it'll be hidden because of the typo.

I agree it's better this way, but given how old this thing is, the less than ideal way might be the way to go.

We need a change record with code examples to show when the condition in #38 is likely to occur and code sample to show how to fix it.

Tried to simplify some things.

For the credential forms we don't need to change too much logic, we need to reverse some conditions and it works as expected. Famous last words, didn't work as expected. Seems to work with manual testing, not in tests.

https://git.drupalcode.org/project/drupal/-/merge_requests/9300/diffs#no...
I have used this patch in my project. I am facing below issue which was not there earlier:
I have a form 1 in which email field is required only when 'email' is selected.
$form['contact_option'] = [
'#type' => 'radios',
'#options' => [
"email" => $this->t("E-Mail"),
"phone" => $this->t("Phone"),
],
'#title' => $this->t('Contact me'),
'#required' => TRUE,
'#weight' => 15,
'#attributes' => [
'class' => [
'contact-option',
],
],
];

$form['email'] = [
'#type' => 'email',
'#title' => $this->t('Email Address'),
'#default_value' => "",
'#weight' => 20,
'#states' => [
// Show this field as required if the contact option is email.
'required' => [
':input[name="contact_option"]' => ['value' => 'email'],
],
],
];

form 2 extends form 1 and make it's email field required. Also unset contact options. Below is the code.

$form = parent::buildForm($form, $form_state, $email_destination);
unset($form['contact_option']);
$form['email']['#required'] = TRUE;
but after appling this patch email required field is not working.

@drupalite1411 the issue is with your code and related to #38. The #states declaration (which is now invalid) is conflicting with your #required declaration.

The states code has become more robust and is more sensitive to coding errors. Given contact_option no longer exists on the form, the #statesdeclaration is invalid and should be removed. If the triggering element cannot be found on the form (e.g. contact_option), the inverse state is applied (e.g. optional).

The code in your child form should be

    $form = parent::buildForm($form, $form_state);
    unset($form['contact_option']);
    $form['email']['#required'] = TRUE;
    unset($form['email']['#states']);