#states not applied to link elements

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

This will need a test case to show the issue

Did not test patch yet.

Adding unit test for field links using states
Patch #19 didn't work with me, it was necessary to do a small fix in state.js, see changes interdiff file

Tested on Drupal 10.1.x with a standard install.
Picked a random module with a form, in this case admin_toolbar, and added

$form['checkbox'] = [
  '#type' => 'checkbox',
  '#title' => t('Show link'),
];
    $form['link'] = [
      '#type' => 'link',
      '#title' => t('Link to front page'),
      '#url' => Url::fromRoute('<front>'),
      '#states' => [
        'visible' => [
          ':input[data-drupal-selector=edit-checkbox]' => ['checked' => TRUE],
        ],
      ],
    ];

Verified the data-drupal-selector wasn't there and the link was appearing.
Applied the fix and the attribute was there and the link was hidden until I checked the box.

Good job with the tests!

Updating attribution

1. +++ b/core/misc/states.js
   @@ -714,9 +714,15 @@
   +      // Check if target is a link.
   +      if (e.target.tagName === 'A') {
   

   This feels a bit too hardcoded, and the comment doesn't explain *why* we are doing this. Maybe we should add .js-form-item to the link itself when #states is used, then I think we could leave the JavaScript alone?

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -103,11 +104,13 @@ protected function doCheckboxTriggerTests() {
   +    $this->assertFalse($link->isVisible());
   

   In 🐛 #states cannot check/uncheck checkboxes elements Fixed we added a third round of tests in this method to ensure the state was correctly reset after unchecking the checkbox again; we should add the same check for this change.

I made that work for 9.5 by add 'container' element and put states here. Link added inside 'container'.

Working on issues

Hello @longwave,
I added the third round validating the checkbox. Also, I updated the commend on state.js explaining better why we update the element.

I agree with you that's is too hard-coded. But at the same time, in this case, should be hard coded, because we need to check if the element is a link tag. What do you think if we keep it as it is but explain why we are doing this?

About your suggestions I think is a good idea, maybe we should have a new js class to state.js handle these cases instead of checking the element tag, in the future, we could have more elements using this feature by just adding this class. My only concern about this idea is the link state won't work out of the box, we should add a new class to the link form element. I guess that we can't use the already existing class(js-form-item, .js-form-submit, .js-form-wrapper) because the links are not wrapped by them.

Regards.

joaopauloc.dev → closed merge request !3968

joaopauloc.dev → closed merge request !3969

Seems the points brought up in #29 have been addressed.

The comment for the A tag case needs rewording, but I'm not sure what to suggest.

Change makes sense.

If the committer get to it first need to remove duplicate // from the states.js, but didn't think it was enough to hold up.

@joaopauloc.dev if you see this before the committer mind removing that please.

Thanks

Removed.
Thanks @smustgrave.

Added comment to the MR

I'm triaging RTBC issues → .

After reading the issue summary I wonderws if drupal_process_states, mentioned in the proposed resolution, was still in use. So, I searched and found and found that it was deprecated in 2019, #3000068: Deprecate drupal_process_states() → . I am tagging for an issue summary update.

Reading the comments, I see that @lauriii has asked for a less risky solution. I think that warrants setting this back for work.

I am setting back to NW.

joaopauloc.dev → closed merge request !3970

• New mr created from 11.x
• Added support to js/state for links in Link element as @lauriii suggested

Removing summary update tag as that was done in #49

Nice! The current approach looks less scary. I posted few comments on the MR but I think we're getting there

Seemed to have some test failures so rerunning

Have reran the tests a few times but seems something is breaking.

Fixed the unit test that was failing.

Didn't see a test-only patch and the test-changes feature of gitlab is blocking me.

So ran locally without the changes to core/misc/states.js core/lib/Drupal/Core/Render/Element/Link.php

There was 1 failure:

1) Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest::testJavascriptStates
Failed asserting that true is false.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/web/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:226
/var/www/html/web/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:65
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Great!

So reviewing the change itself, shame we need specialty code for links in the states.js but confirmed it does work using the example in the IS.

All threads appear to be resolved.

Great work!

Added some comments about the comments.

Comments fixed again.

Seems threads have been resolved.

Tested again following the IS.
Took that code snippet and added to the Performance Form.
States did not work, as both elements rendered.
Applied the MR and states work perfectly.

lauriii → closed merge request !4706

Committed 9b3d27c and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

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