Problem/Motivation

With #3294720: The attachBehaviors() for document is only called after Big Pipe chunks are processed the Drupal Behaviors are being applied for entire page twice.
This causes, that states' rules are initiated multiple times and this causes that states sometimes works unexpected if BigPipe is available.

Steps to reproduce

Create a form containing complex states logic:

    $form['option'] = [
      '#type' => 'radios',
      '#title' => 'Option',
      '#options' => [
        'option_1' => 'option_1',
        'option_2' => 'option_2',
      ],
      '#default_value' => 'option_1',
      '#required' => TRUE,
    ];

    $form['variant'] = [
      '#tree' => TRUE,
    ];

    $form['variant']['option_1'] = [
      '#type' => 'radios',
      '#title' => 'Variant for option_1',
      '#options' => [
        'variant_1' => 'variant_1',
        'variant_2' => 'variant_2',
      ],
      '#default_value' => 'variant_1',
      '#required' => TRUE,
      '#states' => [
        'visible' => [
          ':input[name="option"]' => ['value' => 'option_1'],
        ],
      ],
    ];

    $form['variant']['option_2'] = [
      '#type' => 'radios',
      '#title' => 'Variant for option_2',
      '#options' => [
        'variant_1' => 'variant_1',
        'variant_2' => 'variant_2',
      ],
      '#default_value' => 'variant_1',
      '#required' => TRUE,
      '#states' => [
        'visible' => [
          ':input[name="option"]' => ['value' => 'option_2'],
        ],
      ],
    ];

    foreach($form['option']['#options'] as $option_key => $option_title) {
      foreach($form['variant'][$option_key]['#options'] as $variant_key => $variant_title) {
        $key = $option_key . '__' . $variant_key;
        $form['info'][$key] = [
          '#type' => 'container',
          '#states' => [
            'visible' => [
              ':input[name="option"]' => ['value' => $option_key],
              ':input[name="variant[' . $option_key . ']"]' => ['value' => $variant_key],
            ],
          ],
          'text' => [
            '#markup' => 'info for option ' . $option_title . ' variant ' . $variant_title,
          ],
        ];
      }
    }

And for visitor with BigPipe enabled (f.e logged in) the first click on any radio button will lead that text line will not be visible at all.
Other clicks on any radio will work as expected.

Proposed resolution

Use `once` to select element with defined states to apply them.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#20 3347144-test-only.patch619 bytesmstrelan
#3 3347144-3.patch1.63 KBEvaldas Užkuras

Issue fork drupal-3347144

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Evaldas Užkuras created an issue. See original summary.

Evaldas Užkuras’s picture

Evaldas Užkuras
Primary language Lithuanian
Location Kaunas, LT
CreditAttribution: Evaldas Užkuras commented
Related issues: +#3294720: The attachBehaviors() for document is only called after Big Pipe chunks are processed
Evaldas Užkuras’s picture

Evaldas Užkuras
Primary language Lithuanian
Location Kaunas, LT
CreditAttribution: Evaldas Užkuras commented
Status: Active » Needs review
FileSize
3347144-3.patch1.63 KB

Here's a patch adding use of `once` as I proposed.

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
CreditAttribution: rodrigoaguilera at Ymbra for Ymbra commented
Version: 9.5.x-dev » 10.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests

Thank you for creating the issue and providing a patch.
Bugs should be fixed in the latest dev version of Drupal and then backported to older versions like 9.5.x when applicable.
Since D10 doesn't use es6.js files anymore the patch needs to be different for D10.

Also we should provide one of this scenarios as a js test to be able ensure the behaviour stays consistent in the future

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon as a volunteer commented

I can confirm this issue atleast in 9.5 and 9.2, the patch at #3 resolves it for me, atleast in my situation BigPipe is not used when anonymous/not-logged-in, but on logged in, bigpipe is activated and then I get missing form states, btw @evaldas-užkuras thanks!!

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon as a volunteer commented
Title: States should use once to apply it's rules » States should use once to apply its rules

dgtlmoon opened merge request !4044

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon as a volunteer commented

There is this one which is sort of related https://www.drupal.org/project/drupal/issues/3337995 but was closed

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm at DevBranch, Drupal Ukraine Community commented

I'm a bit surprised that #states wasn't using once(), it's even declared as dependency to the drupal.states library.
This is definitely a bug which affects BigPipe and AJAX use cases. The patch looks correct (didn't test it though).

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon as a volunteer commented
Status: Needs work » Needs review
dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon as a volunteer commented

Just throwing this out there, maybe `states` doesnt bind with the right 'context' ? https://www.drupal.org/project/drupal/issues/3337995#comment-15060748

I already have trouble convincing front-end developers to use the context parameter in our custom modules (most of them just ignore it since don't understand its logic). Now I have to convince them to use jQuery once. When the rest of the javascript world is moving away from jQuery we are forcing people to use it. It doesn't make any sense.

Edit: I just realized that Drupal has its own version of once that is compatible with vanilla javascript but I still think we need a javascript API easier to understand for frontend developers that aren't specialized in Drupal.

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon as a volunteer commented
Title: States should use once to apply its rules » States should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations)
dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon as a volunteer commented
Title: States should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations) » Form API #states property/states should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations)
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Did not test.

But was previously tagged for tests which still need to happen. May be worth posting in #testing channel for suggestions where to add.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented

This was a tricky one to debug, but doing so eventually led to me here today. Here's an example form that is affected by this bug which I think can be used as basis for a regression test of the fix:


    $form['select'] = [
      '#type' => 'select',
      '#title' => 'Select',
      '#options' => [
        'one' => 'one',
        'two' => 'two',
        'three' => 'three',
      ],
      '#default_value' => 'one',
    ];
    $form['checkbox'] = [
      '#type' => 'checkbox',
      '#title' => 'Checkbox One',
    ];
    $form['textfield'] = [
      '#type' => 'textfield',
      '#title' => 'Textfield',
      '#states' => [
        'visible' => [
          ':input[name="select"]' => ['value' => 'one'],
          ':input[name="checkbox"]' => ['checked' => TRUE],
        ],
      ]
    ];

In this example, if the select box has the first option selected AND the checkbox is checked, the textfield should be shown. However, on page load w/ Big Pipe enabled, checking the checkbox (which meets conditions) does NOT show the textfield. You have to uncheck and then recheck it before it finally works.

Big Pipe causes two more invocations of Drupal behaviors, resulting in a total of 3 invocations with the form items in the context. That's enough to cause issues with the logic with the 3 duplicate states behaviors defined.

FeyP’s picture

FeyP CreditAttribution: FeyP as a volunteer and at werk21 commented

I just closed #3383972: States JavaScript should use the once library, causes problems with Big Pipe as a duplicate of this issue. There is some information on how to reproduce with the contributed Webform module in the IS of that issue and MR with a similar but slightly different approach to fixing this.

mstrelan made their first commit to this issue’s fork.

mstrelan’s picture

mstrelan CreditAttribution: mstrelan at PreviousNext for Service NSW commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Enabled big_pipe module on existing JavascriptStatesTest. Test-only job should show it failing.

mstrelan’s picture

mstrelan CreditAttribution: mstrelan at PreviousNext for Service NSW commented
FileSize
3347144-test-only.patch619 bytes

Well GitLab CI made a liar out of me. For some reason it decided to skip the test instead of running it and letting it fail. Attaching a patch to see what Drupal CI thinks.

FWIW this is the result when running the test-only changes locally:

$phpunit --filter=JavascriptStatesTest
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing 
F                                                                   1 / 1 (100%)

Time: 00:28.863, Memory: 357.00 MB

There was 1 failure:

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

/data/app/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/data/app/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/data/app/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:462
/data/app/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:70
/data/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 1, Assertions: 205, Failures: 1.
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Hiding #20 so bot doesn't pick it up.

Going to post in gitlab channel but I see this test-only job failed correctly https://git.drupalcode.org/issue/drupal-3347144/-/jobs/471592

But change seems good, had to look up the param changes but looks good.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

catch credited pirvudoru.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

  • catch committed 51efb1b0 on 10.2.x 
    Issue #3347144 by dgtlmoon, mstrelan, Evaldas Užkuras, bkosborne, FeyP,...

  • catch committed d6b6ad8f on 11.x 
    Issue #3347144 by dgtlmoon, mstrelan, Evaldas Užkuras, bkosborne, FeyP,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Status: Fixed » Closed (fixed)

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

attheshow’s picture

attheshow CreditAttribution: attheshow at Richland Library commented

Related regression ticket is here: https://www.drupal.org/project/drupal/issues/3416398

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Priority: Normal » Critical
Status: Closed (fixed) » Active

The regression in #3416398: Regression on entity reference field states seems like it might be worse than the bug with big pipe that this fixed - I'm considering rolling this back in 11.x and 10.2.x, so we can try again.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented

Hmm, tricky. So it seems the problem is that states sometimes needs to be applied again because the dependents may have changed, right?

In that case, I guess we need a way for states behavior to replace the existing state that was there before so it doesn't create duplicate state definitions.

  • catch committed 09f0d50a on 10.2.x 
    Revert "Issue #3347144 by dgtlmoon, mstrelan, Evaldas Užkuras, bkosborne...

  • catch committed 37718084 on 11.x 
    Revert "Issue #3347144 by dgtlmoon, mstrelan, Evaldas Užkuras, bkosborne...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Priority: Critical » Major

Discussed this briefly with nod_ in slack and we both think reverting is probably the best approach here - more sites are still on 10.1.x than 10.2.x, so reverting the bug fixed here won't be a regression for them, but the regression introduced with be a new bug when those sites update. Also doesn't seem like there's an easy fix for this in either direction, we probably can't use once, and might need to make the states javascript re-entrant instead, no idea what that means in practice.

Version: 10.2.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext commented
Status: Active » Needs work
Issue tags: +Needs tests

Rerolling and marking as needs tests to cover the bug found in #3416398: Regression on entity reference field states

sukr_s’s picture

sukr_s CreditAttribution: sukr_s as a volunteer commented

This issue is a duplicate of #3386191: #states not working correctly when built from a logical combination of multliple fields