Form field #states not woring with entity reference fields

Adding the mentioned patch.

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.

Hi @sukr_s. Turns out that my "Steps to reproduce" instructions were not thorough enough. In order to replicate the situation I'm experiencing, you need to have the media_entity_browser module installed together with its dependencies. These were my exact steps today when I reproduces the issue:

1. Install a fresh version of Drupal 10.3.2.
2. Enable the media and media_library modules that come with core.
3. Install and enable the media_entity_browser module together with its dependencies: composer require 'drupal/media_entity_browser:^2.0'; drush en media_entity_browser.
4. Disable the "Auto open entity browser" setting at: /admin/config/content/entity_browser/media_entity_browser_modal/edit.
5. Edit the article content type and add the previously mentioned fields:field_image_caption_short (short text field) andfield_main_media (entity reference to media, bundle=image, limit=1).
6. Edit the form display for the article content type and set the widget for the Main media field to: Entity browser -> Media entity browser (modal).
7. Create a custom module and add the previously posted hook to it.
8. Add an new article. Not that the field_image_caption_short is always displayed wheter or not the field_main_media field is referencing an image or not.
9. Apply the patch I posted in #2 and repeat step 7. Everything should work as expected.

I'm guessing you we're trying to replicate my issue with the "Autocomplete" widget, which indeed seems to work as intended. Also, said widget formats the selector name to field_main_media[0][target_id] instead of field_main_media[target_id], just as you said. I triple checked that the selector is indeed field_main_media[target_id] when using the media entity browser. My guess is that anytime any kind of JS functionality is involved, the #states break because of the missing detach method.

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.

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 ✨ Don't show the revision log message on creation of content, media, etc. Needs work . 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

You need to make sure that JavaScript behaviours are reattached after the AJAX callback. You can do this by ensuring that the ajax['callback'] method returns the form correctly.

use Drupal\Core\Form\FormBase;
use Drupal\Core\Form\FormStateInterface;

class AjaxStatesForm extends FormBase {

  /**
   * {@inheritdoc}
   */
  public function getFormId() {
    return 'ajax_states_form';
  }

  /**
   * {@inheritdoc}
   */
  public function buildForm(array $form, FormStateInterface $form_state) {
    $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' => $this->t('Check me to reload Select Field field'),
      '#ajax' => [
        'callback' => '::buildAjax',
        'wrapper' => 'js_states_test_wrapper',
      ],
    ];

    $form['js_states_test']['select_field'] = [
      '#type' => 'select',
      '#title' => $this->t('Select Field'),
      '#options' => [0 => 0, 1 => 1],
      '#default_value' => 0,
      '#ajax' => [
        'callback' => '::buildAjax',
        'wrapper' => 'js_states_test_wrapper',
        'event' => 'change', 
      ],
    ];

    $form['js_select_field_textfield'] = [
      '#type' => 'textfield',
      '#title' => $this->t('This field should show when 1 is selected in Select Field'),
      '#states' => [
        'visible' => [
          ':input[name="select_field"]' => ['value' => 1],
        ],
      ],
    ];

    $form['#attached']['library'][] = 'core/drupal.ajax';

    return $form;
  }

  /**
   * AJAX callback to rebuild the form section.
   */
  public function buildAjax(array &$form, FormStateInterface $form_state) {
    return $form['js_states_test'];
  }

  /**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state) {
  }
}

This approach ensures that #states are reattached correctly after the form is reloaded via AJAX.

@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 🐛 Form API #states property/states should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations) Needs work 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.

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 🐛 Using a computed twig as a value for a visibility condition breaks in Drupal 10.3 Active ). 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 🐛 Using a computed twig as a value for a visibility condition breaks in Drupal 10.3 Active 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 (: