🇨🇦Canada @tame4tex

Thanks @mikelutz and @smustgrave

It looks like the test only feature wasn't failing because of 🐛 Functional Javascript test false postive and missing browser output Active .

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

Apologies for the delay in jumping back in. There’s a lot to unpack, so please bear with me.

Clarifying a Misconception

@kristiaanvandeneynde thank you for taking the time to further review the MR. In #43 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work , you state:

Your code still adheres to that but adds two redirects to every login request, even when most sites won’t ever run into the problem explained here. That is a performance hit we cannot incur for an edge case.

This is not the case. The MR only introduces a redirect in situations where an error message would currently be shown, specifically:

1. If a user attempts to log in with cookies disabled.
2. If an anonymous user visits a URL containing the check_logged_in query parameter (see the issue summary, #19 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work , #37 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work , #56 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work ).

The redirect logic in \Drupal\user\Authentication\Provider\Cookie::onKernelRequestVerifyCookies() only runs if verifyCookies is TRUE, and that is only set to TRUE by \Drupal\user\Authentication\Provider\Cookie::applies() in the same condition that currently triggers the error message. Thus, the performance impact is minimal and I think justified to prevent incorrect error messages.

Addressing the Alternate Solution (Re: #39 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work )

@kristiaanvandeneynde, thanks for your suggestion. It does resolve the issue, but it also removes the error message in cases where it should still appear, specifically, when users have cookies disabled and cannot log in.

When I mentioned in #37 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work that I was open to suggestions, I was hoping for ideas on simplifying the MR while still ensuring the error message appears correctly when needed and doesn’t appear when it shouldn’t.

Re:

You'd have to specifically design your site as a single-page dashboard where you don't strip the query arguments after login for this to even remotely be a re-occurring problem.

I understand the concern that this might only affect single-page dashboards, but in our case, that’s not true. Our client’s site has over 1 million pages and 6000+ active users. It does, however, have "dashboard" type pages that "manager" type users visit to gain an overview of what other users are doing. So it's not that we only have one-page site, rather we have users that tend to only visit one page.

I don't feel like our client's site is that unusual. Sure, virtually all pages are access controlled, and we have an EventSubscriber that redirects anonymous users to the login form, but apart from that we do our best to work with core as much as possible as it is easier to maintain.

Re: #54 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work & JavaScript-Based Solutions

@larowlan, thank you for the input. Unfortunately, for us this issue occurs when users save/share URLs with check_logged_in or reload such URLs when logged out. That’s why your suggestion wouldn’t work for us. Due to discussions in 🐛 Login fails and no warning is issued if cookies are not enabled Fixed (particularly #272 🐛 Login fails and no warning is issued if cookies are not enabled Fixed ), I also aimed to avoid a JavaScript-based approach.

Re: #56 🐛 "To log in to this site, your browser must accept cookies from the domain" error message displayed when user goes back and reload the page Needs work

@jody lynn, thanks for sharing your experience, and you raise a good point. Although, I am really hoping we can come to a consensus on a solution where we don't have to choose between showing an error message incorrectly vs not showing error message when we should.

Seeking Clarity on “Feature Request” vs. “Bug”

Since the current behaviour results in incorrect error messages, I’d love to better understand what qualifies this as a Feature Request rather than a Bug?

Moving Forward: Specific Feedback on Complexity

For those who feel the MR is too complex, could you point out specific areas where you see unnecessary complexity? Are there parts that could be simplified while still achieving the intended behaviour? This would give us something constructive to work with.

This bug occurred for me while setting up the TFA TOTP validation plugin which I have set as the default plugin. I have no other validation plugins enabled but I do have the TFA Trusted Browser login plugin enabled.

I can confirm the patch provided in #4 🐛 One setup step remaining, two QR Code Scans required Active fixes the issue. Thanks for the fix @tmiguelv.

Thank you for the review @smustgrave and I will make the fix once there is confirmation this issue is not going to be closed.

Thank you for your comments @kristiaanvandeneynde. While I appreciate the points raised, I agree with @oily and do believe this is a bug, at least for the types of sites we manage.

I can understand how this might not be a significant issue for Drupal sites where most users browse multiple pages after logging in. However, it does create challenges in scenarios where:

1. Most users on the site are authorized (i.e. most of the content is not accessible to anonymous users),
2. A number of those users are NOT browsing to other pages after logging in, and
3. There is a backend process that logs users out (e.g. for security after a period of inactivity)

We have sites where users are logging in to check a report or dashboard and often don't browse away from the page they are redirected to after logged in. You can recreate this scenario by doing the following:

1. On a Drupal test site, visit the login form with a destination redirect ie /user/login?destination=node.
2. Log in and open a separate window to log out, simulating the backend process logging out the user.
3. Return to the original window and reload the page, simulating the user’s attempt to refresh their report.
4. Notice the incorrect error message that is displayed.

This behavior has led to confusion among users and, in our experience, unnecessary client support time spent addressing their concerns. I would appreciate it if the "working as designed" designation could be reconsidered.

As one of the authors of the MR, I reviewed the original issue in detail (as outlined in comment #24) and explored several simpler options, though unfortunately without success. That said, I remain open to any informed suggestions for alternatives that address this use case.

Thank you for taking the time to consider this feedback. I hope we can find a resolution that improves the user experience for affected sites.

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.

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.

tame4tex → changed the visibility of the branch 11.x to hidden.

tame4tex → changed the visibility of the branch 10.3.x to hidden.

tame4tex → changed the visibility of the branch 3397718-to-log-in-10.3.x to hidden.

So this turned out to be a challenging bug to fix!

After reviewing the original issue ( 🐛 Login fails and no warning is issued if cookies are not enabled Fixed ) which added the code in the first place, I noted a few mentions of a double redirect approach to solve the original issue. I decided to attempt to implement that approach but only if the request contains the check_logged_in query parameter and there is no session for the request. So the extra redirects would only ever happen for users who have cookies disabled or happen to directly access a url with the check_logged_in query parameter when they are logged out. So it would rarely be experienced by regular users.

This resulted in adding a new /user/verify-cookies page which does the checking for a cookie added to the response and displays an error message if cookies are disabled or redirects the user to their original request if cookies are enabled.

Open to any feedback on the title and text displayed on this new page. I am also wondering if it should NOT be within the user module and instead in a more generic location given it could be used by other functionality or contrib modules that need to verify cookies are enabled.

I also added this new page to robots.txt as it shouldn't be crawled.

With the new approach I was unable to successfully test when cookies were disabled using the existing Functional UserLoginTest as cookies seem to be enabled after the POST request to the user login form. Therefore I switched it to a Functional Javascript test and modified the chrome options to ensure cookies were disabled.

I have updated the Issue Summary with the proposed resolution.

Ready for review and feedback

Ah thanks for removing that @oily, it was cruft that I forgot to remove while I was testing possible solutions. Thanks also for the insight.

Going to dig in now for an hour and see what I can come up with.

This bug has also been causing us some grief. I have added a test for the bug using the steps to reproduce outlined in the issue summary.

The bug can even be replicated by simply visiting user/login?check_logged_in=1 as an anonymous user.

Where we have experienced the issue is users being redirected after login to a page that contains the check_logged_in, then for whatever reason they are logged out but they reload the page that contains the query parameter and the confusing and incorrect error message appears.

I have reverted the previous fix in the merge request as it was causing the existing testCookiesNotAccepted() test to fail. If cookies are disabled then the session will never contain the 'uid' so the error message is not displaying when it should.

As far as how to solve the issue, I can't think if a way to do so with the current query parameter approach without doing something hacky like setting the check_logged_in to a timestamp and then checking the time and only displaying the error if it is less than X seconds old.

As an alternative to a query parameter, could we instead in use a kernal.request event listener and check if the request is for the user.login route using the POST method, indicating the user login form is being submitted, and display the error message if the session doesn't exist.

I will work on this more tomorrow and see if that is a viable approach unless I get other feedback in the meantime.

tame4tex → made their first commit to this issue’s fork.

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!

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.

Updated the MR to fix the failing test. The only change was to the test itself.

Also added a new MR https://git.drupalcode.org/project/autologout/-/merge_requests/68 for the 2.x branch, which is exactly the same as the previous MR.

Rather than using one of the uploaded patch files, obtain the patch from the MR by following the instructions on https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → . This should apply to the latest 1.x and 2.x versions. If it doesn't please comment below or update the MR.

tame4tex → changed the visibility of the branch 2.x to hidden.

tame4tex → made their first commit to this issue’s fork.

Not sure on the best path forward here but it highlights the need for more comprehensive testing of different data types. Happy to help with this once there is a consensus on direction.

Just as a head up...

Given what I have found out in 🐛 Encrypted decimal field value not saved on drupal version 10.2.x+ Active where NULL is already inadvertently being returned by FieldEncryptProcessEntities::getUnencryptedPlaceholderValue for a decimal field, I am guessing changes would also need to be made to ProcessEntities::setEncryptedFieldValues for this not to be problematic.

Also based on 🐛 ProcessEntitiesTest is allowing unexpected results to slip through Needs work more testing beyond ProcessEntitiesTest is definitely need.

That said, I agree with the motivation behind this change.

I have created MR !73 to add both testing and a fix for this bug.

Modifying ProcessEntitiesTest was not enough to test for this bug as it seems this unit test is allowing unexpected results to slip through. I have added a separate issue for this 🐛 ProcessEntitiesTest is allowing unexpected results to slip through Needs work .

I therefore added Functional testing to ensure there is adequate coverage for this bug.

tame4tex → created an issue.

tame4tex → created an issue.

I have added a new MR !72 for the 3.2.x branch which includes tests for the bug.

Let me know if there is anything else needed to get this committed. Thanks!

Updated the MR to add a a pageshow event listener which will prevent the bug from occurring in the first place. As suggested on https://web.dev/articles/bfcache.

I still think it is worth the extra measure to keep the validation and sanity check in place "just in case".

I have added MR !91. This was inspired by on_configure_form_every_entity_is_selected-3199534-02.patch and although it doesn't specifically fix the bug, it prevents the bug from causing unexpected data changes by throwing a validation error when the bug conditions occur.

The problem with patch #2, apart from needing to be modified to apply to updated code, is it just displays the error message No items selected.. After getting the error, if the user continuously selects options without reloading the page, the same error message will keep appearing. So I updated the error message to provide a link to reload the page , along with helpful info to prevent the problem form occurring in the first place.

Ideally the code needs to be modified to correctly handle back/forward cache, but I believe that is best served in another issue as it is more complex and will most likely need greater input from @graber.

So it took some fiddling with the chrome options to get back/forward cache working but the test is now failing as expected.

https://git.drupalcode.org/issue/views_bulk_operations-3199534/-/pipelin...

I will take a look at the patch from #2 tomorrow and create a MR with a potential fix.

I have been able to replicate this bug on version 4.3.1.

In our instance, we had custom functionality that added a hidden element to the Views Exposed Form that was unique for each new form build.

Here is the workflow of the cause of the bug:

1. When the VBO form was submitted again after returning via the browser back button, the exposed form input had changed, triggering $this->tempStoreData['list'] to be cleared in ViewsBulkOperationsBulkForm::updateTempstoreData().
2. Then in ViewsBulkOperationsBulkForm::viewsFormSubmit() because the selected key was not in $this->tempStoreData['bulk_form_keys'] nothing got re-added to $this->tempStoreData['list'].
3. Then finally in ViewsBulkOperationsActionProcessor::executeProcessing() because $data['list'] is empty it gets set to all view results and ALL view results get processed!!

If we removed the custom hidden element, the bug didn't occur.

Even though it is a specific set of circumstances that would trigger the bug, I feel it is a critical issue given that data loss (e.g. with delete node action) or security issues (e.g. with unblock users action) could occur.

I have been able to create a FunctionalJavascript test locally but it requires the browser to be in non headless mode. Not sure if it will work with Gitlab CI but I will add a MR for this test shortly.

@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!

Updated title to also include Result Summary @total token.

This has also broken any site that used @current_record_count of @total in a Results Summary when using a "Some" pager.

I have opened 🌱 "Some" Pager Plugin and [view:total-rows] Regression Issue Active to address this.

I have updated the Issue summary with the additional regression issue and the two possible resolutions I have thought of so far.

I have also added a MR for Option #1 which is now my preferred.

BC is definitely a consideration but I would consider it less so compared to the regression issues resulting from the change.

It would only impact sites that started using the [view:total-rows] token from version 10.1.0 because prior to that version it returned the total number of items before the pager limit was applied.

Similar to the token issue, we have a site where the usage of Displaying @current_record_count of @total on a block using the "Some" pager is now broken due to this change.

This is what it looked like from D8 all the way to D10.1.0

This is how it looks now

On further thought, on reading the issue summary of 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info again, using @current_record_count in their header rather than [view:total-rows] would have solved their problem and there would have been no need for the code change.

So on further thought maybe the changes in 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info should be rolled back and we instead update the description of [view:total-rows] token to explain it is the total number of items before any limiting by the page is applied.

Right now I am just proposing options, hoping to get feedback on preferred approach.

For those interested, I have added an issue and MR to re-add the functionality that switches a select widget to an autocomplete widget when the number of options hits a predetermined limit.

See ✨ Views EntityReference filter: Add functionality to switch from select to autocomplete widget when a max number of options reached Active

I have added MR 9866 to add this feature. Code was inspired by functionality that existed in https://www.drupal.org/files/issues/2020-09-30/2429699-351--on-9_1_x.patch → on 📌 Add Views EntityReference filter to be available for all entity reference fields Closed: duplicate .

Another reminder that you MUST follow the instruction on ✨ Continuation Add Views EntityReference filter to be available for all entity reference fields Needs work under the "How to use" heading to set up the EntityReference filter first for this to work.

tame4tex → created an issue.

Actually NVM, sorry my bad, I see where it is being applied.

This is not an issue with this code, it is a method further along the call tree in \Drupal\views\Plugin\EntityReferenceSelection\ViewsSelection::getReferenceableEntities that is not respecting the limit. I will investigate further and open a separate issue if need be.

Switching back to Reviewed & tested by the community. Sorry for the noise!

I can't see where in the code the Select list maximum number of options of 100 gets applied. I just did a manual test with 103 options and they all appeared in the select list.

Is this an oversight? Should the number of options be limited to 100?

Switching back to Needs Work (Sorry!)

Given there wasn't a lot of work involved, and to hopefully speed things along, I have created a MR with the proposed resolution.

Even though this should fix the related issues, it doesn't automatically close them as testing should be added for the specific bug experienced, ie visibility of the More link and how caching effects this visibility.

I feel the solution pursued so far may possibly be the wrong approach.

It was the solution to 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info that triggered this regression. It was fixing the value returned for the token [view:total-rows] that resulted in PagerPluginBase::$total_items always being set to the number of limited query results for the "Some" pager.

But what if this assumption was incorrect, what if [view:total-rows] should not always return number of limited query results. For example, if I am using the "Some" pager, limited to 5 results when there are 20 and I have a "More link" enabled, I think it would be valid for me to want to add the text "Displaying 5 or 20 results". In which case I would want [view:total-rows] to return 20.

If the code is changed to only override PagerPluginBase::$total_items in the postExecute() if a "More link" is not enabled, then this issue would be fixed.

I have opened another issue ( 🌱 "Some" Pager Plugin and [view:total-rows] Regression Issue Active ) to discuss this proposal as it may require other actions unrelated to this issue.

tame4tex → created an issue.

So I finally had some time to revisit this issue and add states testing.

I have added the states testing in a separate issue https://www.drupal.org/project/time_field/issues/3479769 📌 Add form conditional states testing Active to keep this issue as simple as possible.

It turns out my initial assumption was incorrect, states is actually working. It was our custom code that interacts with states that was failing due to expecting an empty string would mean empty.

Regardless, I am still going to work on the proposed resolution to see if NULL will work. In my opinion it is a much better approach than relying on 86401, if for no other reason than to get rid of the console warnings.

I have added the testing and opened the merge request.

Contrary to my initial thoughts in https://www.drupal.org/project/time_field/issues/3463912 🐛 The integer 86401 should not be used to represent an "empty" time element Active , javascript states is actually working.

Ready for review.

tame4tex → created an issue.

This is no longer an issue in 3.2.x so it can be closed.

Closing this as a duplicate to https://www.drupal.org/project/drupal/issues/3468860 🐛 Form field #states not woring with entity reference fields Active where there is a proposed fix to address ajax related states issues.

Please review the MR on that issue to see if fixes your problem.

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!

All failing tests are fixed. Good for review.

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.

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 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.

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.

Sure we could list the file create time but that is overkill for our current usage. It is simply a "nice to have". I guess we got used to the original naming convention and found it helpful at times.

It doesn't seem like I am going to win you over, no worries, thanks for considering.

Ugh ... sorry. Never mind this was already fixed in https://www.drupal.org/project/signaturefield/issues/3422334 🐛 Library definition license is missing URL Fixed .

Any chance of getting an updated release soon? Thanks!

tame4tex → created an issue.

In our usage it provides some helpful context as to when the signature was created by just looking at the filename. We don't have to then go and check the file create date.

What are the benefits of keeping the current approach?

Assuming this wont be addressed in core, I have added a MR to fix the extra empty column issue.

I have refactored paragraphs_preprocess_field_multiple_value_form quite a bit in an attempt to improve readability and improve robustness.

I have manually tested on both paragraph widget types and all looks to be ok.

Added related issue

As @nsavitsky confirmed this is not completely fixed.

template_preprocess_field_multiple_value_form() is automatically adding an extra table column for a form element item's '_actions'. \Drupal\Core\Field\WidgetBase::formMultipleElements() adds the '_actions' array key and populates it with the item's remove button.

Given \Drupal\paragraphs\Plugin\Field\FieldWidget\ParagraphsWidget::formMultipleElements() does not add '_actions' to its items, we end up with this extra empty column.

The question is should this be fixed in core or paragraphs? Should core automatically add a column for '_actions' without confirming that items do actually have that key?

I believe this has been fixed by Stop using $error->arrayPropertyPath (3333974) → and is in release 1.18. Thus I am closing as duplicate.

tame4tex → created an issue.

I have created MR with the requested change

tame4tex → created an issue.

Added a comment to the MR as I don't think we should be adding an empty page parameter if we don't need to.

tame4tex → created an issue.

Attached patch from MR for those that prefer a patch file.

tame4tex → created an issue.

Could the issue be more easily solved in JS by determining if the element is in a modal and then getting the parent form object to set as the `dropdownParent `?

The attached patch is working for me in paragraphs, modals and regular forms without the need for a new wrapper element.