#states not affecting visibility/requirement of managed_file

There appears to be two issues with the managed file. This is my understanding of it, but please correct me if I have it wrong.

The managed file input is loaded via ajax and appears within an 'ajax-wrapper' div
By the time it is loaded it appears that this bit of core/misc/states.js

Drupal.behaviors.states = {
  attach: function (context, settings) {
  }
};

which assigns the fields as Dependants of the triggering input (in this case the select) has already been run. The ajax loaded file_managed field is then not assigned as a Dependent. Because of that, when the 'state:visible' event is triggered, the ajax loaded file_managed is uneffected.

The 'data-drupal-states' attribute is not being added to the ajax loaded file_managed input
With a file input instead of file managed, a 'data-drupal-states' attribute is added to the input with json encoded settings specifying which element the Dependent is dependent upon and when it should be made visible. This information is not available in the ajax loaded element, so even if triggered the attach function (above) again, it wouldn't know how to handle this input.

Possible solution
Perhaps we can attach the data-drupal-states directly to the ajax-wrapper container so regardless of what happens within the file_managed, we can be sure the states are obeyed. I am not sure if that might have other unintended consequences so perhaps the above two issues need to be addressed independently.

The attached patch follows my possible solution listed above and appears to correctly hide the managed file on load as well as on change of the field it depends upon.

Reviewed!

1. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -348,8 +349,20 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   +    // Add states to ajax wrapper to states.js can potentially attach this
   

   The second "to" should be "so"

2. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -348,8 +349,20 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   +      // Add a preceeding space.
   +      $attributes = ' ' . $attribute;
   

   This should not be necessary, the Attribute class should be adding that. In fact I'm fairly sure there will be a double space currently.

Thanks both for the review! I have updated the patch based on your feedback tstoeckler; you're right, it does add the space already.
If you are happy with it, good to switch to 'reviewed & tested by the community'?

I can't say anything about the JS changes, but other than that looks good. Since this was RTBC before, marking as such.

Thanks everyone!

I think we should add a JS test for this, especially if it's a major bug. The states system is definitely something where JS tests help.

Oh and @Saphyel, when you review a patch, can you document what you reviewed and how? Code, manual testing, etc. Screenshots are always good for visual issues like this; see: https://www.drupal.org/contributor-tasks/add-screenshots

Thanks also @scott_euser for the careful explanations!

@xjm Thanks! I will work on adding a JS test for it, probably tomorrow or Wednesday.

Updated with a javascript test.

Fixed docblocks in last patch. Interdiff to last major change as well to make it easier to review.

Wonderful work, well done everyone!

Below my feedbacks.

1. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -352,8 +353,17 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   +    // Add states to ajax wrapper so states.js can potentially attach this
   +    // element as a Dependent.
   +    $attributes = '';
   +    if (isset($element['#states']) && !empty($element['#states'])) {
   +      $attributes = new Attribute([
   +        'data-drupal-states' => json_encode($element['#states']),
   +      ]);
   +    }
   +
        // Prefix and suffix used for Ajax replacement.
   -    $element['#prefix'] = '<div id="' . $ajax_wrapper_id . '">';
   +    $element['#prefix'] = '<div id="' . $ajax_wrapper_id . '"' . $attributes . '>';
   

   $attributes is first a string, then an Attribute() instance. Why don't we make it an Attribute() instance since definition, and maybe we create it with [id=$ajax_wrapper_id] and so print all prefix element attributes in the same way?

2. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileManagedStateTest.php
   @@ -0,0 +1,60 @@
   +  protected function setUp() {
   +    parent::setUp();
   +  }
   

   I don't think this is needed then, as it's overriding the parent::setUp() just to call it. :D

3. +++ b/core/modules/file/file.js
   @@ -36,6 +36,10 @@
   +      $(document).trigger('state:visible');
   

   This looks to handle only the visible, so I tried some other states and something looks broken. Apparently all states are applied to the parent wrapper and not to the input element itself. So some states work (visible, invisible, disabled) but other like required don't.
   States changes should affect the element and not the parent.

Hi,

I have used Patch #14.

Following are my code snippet, When i apply patch then entire field set get disable.
When i check using firebug then i got to know that the fieldset has apply "display:none" attribute.

$form['test'] = [
'#title' => t('Login Button Settings'),
'#type' => 'details',
'#open' => TRUE,
];

// Login link type.
$form['test']['test1'] = [
'#type' => 'select',
'#options' => [
'' => t('- Select - '),
'text' => t('Text'),
'image' => t('Image'),
],
'#default_value' => $settings->get('o365_user_auth_login_type'),
'#title' => t('What type login link you want to show?'),
];

// Login link text.
$form['test']['test2'] = array(
'#type' => 'textfield',
'#title' => t('Login Link Text'),
'#default_value' => $settings->get('o365_user_auth_login_link_text'),
'#states' => [
'visible' => [':input[name="test[test1]"]' => ['value' => 'text']],
],
);

// Image link
$form['test']['test2'] = [
'#title' => t('Image Login Link'),
'#type' => 'managed_file',
'#description' => t('The uploaded image will be displayed on this page using the image style choosen below.'),
'#default_value' => $settings->get('o365_user_auth_login_link_image'),
'#upload_location' => 'public://o365_user_auth/',
'#states' => [
'visible' => [
':input[name="test[test1]"]' => ['value' => 'image'],
],
],
];

please help me.

Thanks,
Ajay

Related issue, with working patch.
https://www.drupal.org/node/2871794

I've updated the patch so the state settings are applied to the element as well as wrapper. This now works for required, visible, and all other options. I've attempted to update the tests to cover required as well so we can see the two cases (where the wrapper needs to change + where the form element needs to change) - not sure if tests will pass though - need to get phantomjs working in my docker containers.

I've fixed the other feedback items, thank you for reviewing.

Hey @scott_euser have you seen work on the related issue?
Can we merge the two issues, and close the duplicate?

Hey @gambry, just took a look at it - seems to not be solving the whole issue. Have posted a comment there suggesting we close that.
Not sure if we should bring this bit into here:

diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index a6fc3f6..d0bfe07 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -1220,7 +1220,6 @@ function file_managed_file_save_upload($element, FormStateInterface $form_state)
 function template_preprocess_file_managed_file(&$variables) {
   $element = $variables['element'];
 
-  $variables['attributes'] = [];
   if (isset($element['#id'])) {
     $variables['attributes']['id'] = $element['#id'];
   }

Have taken a look, that results in invalid html actually. Essentially results in this:

<div data-drupal-selector="edit-managed-file" data-drupal-states="{&quot;visible&quot;:{&quot;:input[name=\u0022toggle_me\u0022]&quot;:{&quot;checked&quot;:true}}}" id="edit-managed-file-upload" class="js-form-managed-file form-managed-file">
  <div id="ajax-wrapper" data-drupal-states="{&quot;visible&quot;:{&quot;:input[name=\&quot;toggle_me\&quot;]&quot;:{&quot;checked&quot;:true}}}"><input data-drupal-selector="edit-managed-file-upload" type="file" id="edit-managed-file-upload" name="files[managed_file]" size="22" class="js-form-file form-file" data-drupal-states="{&quot;visible&quot;:{&quot;:input[name=\u0022toggle_me\u0022]&quot;:{&quot;checked&quot;:true}}}">
    <input class="js-hide button js-form-submit form-submit" data-drupal-selector="edit-managed-file-upload-button" formnovalidate="formnovalidate" type="submit" id="edit-managed-file-upload-button" name="managed_file_upload_button" value="Upload">
    <input data-drupal-selector="edit-managed-file-fids" type="hidden" name="managed_file[fids]">
  </div>
</div>

Notice 2 id's 'edit-managed-file-upload' where there should of course only be one on the page.

Re-rolling without any change to file.js as that was a lingering change simply from saving the file (ie, code editor automatically added in new line at end of file that was missing), but no need to keep that in scope. Otherwise identical to #20. Ready for review.

When a managed_file field is inside a fieldset, the entire fieldset is hidden when using visible or invisible states on the instead of just the field.
Have not tested this using other states.

I believe that @ajayNimbolkar in #16 is describing a similar issue while using 'details' as a wrapper instead of 'fieldset'.

Patch from #24 is working great for me, thanks for all your work on this scott_euser!

I encountered this problem when creating a custom field widget where managed_file fields along others should be visible according to a select filed. Applying this patch made the whole custom field disappearing because states.js now changed the visibility of the entire fieldset. Because of state.js function $(e.target).closest('.js-form-item, .js-form-submit, .js-form-wrapper').toggle(e.value) the fieldset appeared als closest to the ajax-wrapper element. I discovered, that the upload element didn't had states by default. So for my needs, it was enough to add states to just the upload input element without the ajax wrapper, because states.js can still get the states of these elements and toggle them accordingly. I placed it right below the $element['upload'] definition, to keep it in line. Starting from line 303

        // The file upload field itself.
        $element['upload'] = [
            '#name'             => 'files[' . $parents_prefix . ']',
            '#type'             => 'file',
            '#title'            => t('Choose a file'),
            '#title_display'    => 'invisible',
            '#size'             => $element['#size'],
            '#multiple'         => $element['#multiple'],
            '#theme_wrappers'   => [],
            '#weight'           => -10,
            '#error_no_message' => true,
        ];
        if (!empty($element['#accept'])) {
            $element['upload']['#attributes'] = ['accept' => $element['#accept']];
        }
        // add #states to upload element
        if (isset($element['#states']) && !empty($element['#states'])) {
            $element['upload']['#states'] = $element['#states'];
        }

Hi Team,

I have checked patch #24 with Drupal 8.5.8 & PHP 7.2. Its working for me.

Thanks

I am using #states for in a node edit form, with this code

$form["field_ad_image"]["widget"][0]['#states'] = [
      'visible' => [
        ':input[name="field_interstitial_ad[value]"]' => ['checked' => TRUE]
      ],
      'required' => [
        ':input[name="field_interstitial_ad[value]"]' => ['checked' => TRUE]
      ]
    ];

Using the patch #24 the field `field_ad_image` get visible if the `field_interstitial_ad` is checked, if not get hidden, but with the required is not working right because the field get the required mark but anyway you can save the form without upload any image.

Drupal/core 8.6.4 , PHP 7.2.5

tagging, relevant for WCAG guideline 3.3 Input assistance.

Fixing tags

Adding re-roll tag. Looks like the current patch does not apply to 8.8.

Patch #37 works for required state when field has no uploaded file. But with file uploaded 'upload' element is hidden and field label's 'for' points to nothing, so required state does not work. I made a label and states to refer to 'fids' element in this case.

P.S. You need a patch from https://www.drupal.org/project/drupal/issues/1091852 to have states working with AJAX elements

I have re-rolled patch #39, and removed core from info file to fix core_version_requirement issue.

Re-rolled patch #44, Updating prefix for dependencies.
Added interdiff for same.

Fixed the fail tests.
Changed Class "Drupal\FunctionalJavascriptTests\JavascriptTestBase" to "Drupal\FunctionalJavascriptTests\WebDriverTestBase".

Fixed the fail tests. Added "$defaultTheme" and "$modules" property changed to protected.

Patch #24 is working for me in Drupal 8.9.16 and PHP 7.1.

Patch #24 is working for me with Drupal 9.2.9 and PHP 7.4 for require state.

Doing a review.

The Issue Summary begins with a statement that this issue is identical to another one, and the one has been fixed. I am not sure what this issue is fixing. Tagging for an issue summary update, see Write an issue summary for an existing issue for guidance.

There was a gap between Sept 2019 and July 2021 after which there have been rerolls only and no code reviews. The last code review was in #15, in May 2017.

This patch no longer applies, tagging for a reroll.

I applied the patch so I could run the test but then I saw it was a FunctionalJavascript test which I am not set up for. So I followed the steps by hand. I installed the test module and navigated to the form. The toggle button had no effect so I stopped. Because of that I am adding a tag for steps to reproduce.

Not sure if this should be Major, but leaving for now.

Re-roll patch #47 for 9.4.x-dev

Fixed failed test of patch #55.

Fixed failed tests of patch #56.

hi, There
I am using drupal 9.2.7 and i have an issue with managed_file. I used a custom form with managed_file, it creates a directory and uploads the document but in submitForm function, it gives only a "blank array". there are no details about fid, file type, size, etc on submit function. i am using $form_state->getValues('file_name'); as below code-

$form['select_file'] = [
'#type' => 'managed_file',
'#title' => $this->t('choose file'),
'#multiple' => FALSE,
'#description' => t("Document should be in .jpeg, .jpg, .png, .gif, .txt, .docx and .pdf format!"),
'#upload_location' => 'public://Applicant-file/applicant_'.$id.'',
'#upload_validators' => array
(
'file_validate_extensions' => array('pdf jpg jpeg png gif txt docx '),
'file_validate_size' => array(25600000),
),
];

public function submitForm(array &$form, FormStateInterface $form_state) {
$select_file = $form_state->getValue('select_file');
................................
................................
}

Slight update to the issue summary to note that while this may have been working in #1118016, it doesn't work right now.

Reviewed the code and tested the patch on 9.5.x

- The patch applies cleanly
- All the issues mentioned above are already taken care off
- Tests pass, would be good to add test only patch
- states work as expected after the patch is applied

+1 for RTBC

@ameymudras: Did you actually test to see if the field worked correctly, i.e. that you could not submit a form without selecting a file?

@DamienMcKenna I had tested the form submit without adding a file which worked fine. I tested again and noticed an issue:

- When we select Audio, file is marked as required with asterisks but it lets me submit the form. As in the required field is not respected here.

I reverted a bunch of hunks from #57 that were out of scope code style and other refactoring. Here's a patch with only the actually necessary parts of it, both test-only (fails locally) and full (passes). No longer needs reroll.

Re: #64:

- When we select Audio, file is marked as required with asterisks but it lets me submit the form. As in the required field is not respected here.

This is why I think #states should drop support for toggling required. As documented, this #states toggle has nothing to do with form validation. It literally just paints or erases your little red * next to the field. 😂 That's it. This "works as designed".

Back to NR.

Thanks! -Derek

Patch on #65 works for me on Drupal 9.3.19. Thanks!

Patch from #65 is working for me too - on 9.4.4. Cheers!

Reviewed the code and tested the patch on 9.5.x

- The patch #65 applies cleanly
- Issue summary is clear and explains the problem
- Was able to replicate the issue with the code snippet provided in summary
- After applying the patch field works correctly and was able to submit the form
- Did a quick code review and no issues were identified

Marking this issue as RTBC unless there are anything else pending here. Not including screenshots to avoid duplicate ones

@ameymudras, thanks for reviewing this patch. To answer your question, yes there is more work to do here. Have a look at the tags, this issues is tagged for a summary update and for steps to reproduce. That was asked for in #54, 10 months ago and has yet to be done.

Setting back to NW for an issue summary update. At the very least the proposed resolution needs to be completed to explain the agreed solution so that any other reviewers and the committers know what should be implemented. Steps to repI've added the standard template to make it a bit easier. It shouldn't take too long for someone to update.

Think this is most closely tied to 3.3.1.

Using Drupal 10.0.9

When putting these inside a fieldset, the fieldset is targetted and set to display none when changing the values.
It does work without a fieldset. So the test and code should be updated to also address this issue.

Yes, we really want to avoid using display:none; unless we really want it to be invisible to everyone including screen reader users.

The same issue seems to be occurring on Webforms. Conditional logic options based on state of `managed_file` field does not seem to work.
Steps to reproduce:

1. Create a webform with a file field.
2. In Webform configuration (Build > Elements section), choose 'Edit' option for 'Submit button(s)'.
3. Switch to 'Conditions' tab.
4. Set a condition as: Visible - If - File Field (is) - Filled
5. Save webform settings.
6. Go to the webform. Observe that submit button is hidden, based on configured condition.
7. Upload a file - see that 'Submit' button is still NOT visible (but it should be)

There is a working workaround for that scenario:
1. Add custom webform handler and use it on affected webform.
2. Define alterForm() method and build conditional visibility there.
Example:

$form['elements']['actions']['#states'] = [
      'visible' => [
          '.webform-submission-activity-tasks-form :input[data-drupal-selector="edit-presentation-fids"]' => ['filled' => TRUE],
      ],
    ];

In terms of Webforms there is another issue I've caught which is related to this one. I've created a separate thread: https://www.drupal.org/project/webform/issues/3399906#comment-15308665

The patch #65 not working for me when I am trying to add a #required state.
Drupal version 9.5.9

Patch #65 works for me i am using it on D-9.5

- Issue summary has been updated.
- Root cause of #1118016: conditional visibility of a managed_file using #states attribute does not work not working has been identified and fixed. Different from the patch.
- Test case reused from #2847425-65: #states not affecting visibility/requirement of managed_file and included.

Reviewing the proposed solution seems to describe the problem or steps to reproduce vs what the fix was. So leaving both issue summary and steps to reproduce tags.

Also some comments on the MR.

- Updated the IS
- Updated the proposed changes

cleaning tags.

Patch #65 only allows to add the red asterisk next to the upload field but I can submit without uploading any file.

NW for #86

required in #states only places a * label against the field. It does not do any validation. It's designed as such. see #65. Please open a new ticket for implementing mandatory check when required is set in #states.

If follow up is needed it should be created but #65 also mentions this may be works as designed.

@smustgrave there are two issues here
1. visibility i.e. hide / show does not work like other fields. This is a bug and has been fixed in the MR.
2. required does not perform the mandatory check. This is mentioned as "works as designed". Therefore I suggested to open a new ticket for performing required validation in 88.

if it's agreed to open a new ticket for required validation, the title & problem needs to be updated and a new ticket needs to be opened.

Patch #65 does something weird to drupal/webform 6.2.2. Without the patch my app using webform works fine (behat unit tests). After applying the patch webform seems to miss a button. Remove the patch, all is ok again. It's weird, since there are no managed files in the webform.

Update:
Patch #65 will need a reroll for Drupal 10.1.3. When you apply patch #65 to Drupal 10.1.3 you will end up with

Fatal error: Cannot use Drupal\Core\Template\Attribute as Attribute because the name is already in use in www/core/modules/file/src/Element/ManagedFile.php on line 19

since Drupal 10.1.3 already added use Drupal\Core\Template\Attribute on a different line, and after the patch you have a duplicate use statement.

Patch #65 rerolled for Drupal 10.3.1 (I guess >= 10.3)

Reroll of patch #92, because the Attribute class was not imported and caused the following error:

Error: Class "Drupal\file\Element\Attribute" not found in Drupal\file\Element\ManagedFile::processManagedFile() (line 380 of core/modules/file/src/Element/ManagedFile.php).

As work around it can be hidden by:

    $form['files_container'] = [
      '#type' => 'container',
      '#states' => [
        'visible' => [
          ':input[id="edit-subject"]' => [
            ['value' => $this->t('Question')],
            'or',
            ['value' => $this->t('Complaint')],
          ],
        ],
      ],
    ];

    $form['files_container']['images'] = [
      '#type' => 'managed_file',
      '#title' => $this->t('Choose an image (jpg/png)'),
      '#description' => $this->t('You may select up to 5 files.'),
      '#multiple' => TRUE,
      '#upload_validators' => [
        'file_validate_extensions' => ['png jpg jpeg'],
        'file_validate_size' => ['4194304'],
      ],
      '#cardinality' => 5,
    ];

I created the followup issue for required not working on file inputs: #3513308: Required attribute on file inputs not working

I also incorporated changes from the latest patch into the MR.

Believe all feedback has been resolved on this one. Test coverage is failing in 2 spots https://git.drupalcode.org/issue/drupal-2847425/-/jobs/4682940

IS seems to line up to me.

LGTM

If this gets committed, we might need to reopen #3213580: File upload fields are not working through conditionnal logic.

Ran the test-only job and confirmed it fails.

I skimmed the comments and didn't find unanswered questions. On reading the MR I made one suggestion to a comment and left some questions, all pretty simple. I would usually just accept the suggestion but since there were some questions I left that for now. Because of the questions, setting this to NW.

Amending attribution.