Views hardcodes exposed filter block form ID's which breaks AJAX when the same form is shown multiple times on one page

There seemed to be a regression in the regex in patch #72, I have updated it so it applies for 9.5.x and does not break

update patch for the corresponding version D10.1 there. sorry i couldn't able to create interdiff due to some system issue i guess.

Updated patch for version D9.5.9.

We have been using this patch in our projects for a long time but we also noticed that there were some weird problems with the views exposed filters. In our case it was scrolling the wrong view to focus after the exposed form was submitted.

We debugged the issue and noticed that it was not enough that the view had a unique ID in it but we needed to also make the drupal-data-selector unique.

In our case we basically created an input preprocess and checked if the input is a exposed views filter submit and then changed the drupal-data-selector to be unique:

$variables['attributes']['data-drupal-selector'] = $variables['attributes']['data-drupal-selector'] . '-' . strtolower($unique_str);

We have a common function for the unique id but this might be worth checking in this issue also.

Rerolled for 10.x branch and attached a static patch.

For the CC Failure. Also MR should be updated for 11.x please.

Fixed CC as mentioned in #85.

The view_html_id set in drupalSettings is not used anywhere. Is this still needed? If it is cruft left over from the original approach, then it should be removed.

There is issue with the patch from #86:
`.${$(this.progress.element).attr('class').replace(/\s/g, '.')},`,
this produces selector with extra comma at the end such as :
.ajax-progress.ajax-progress-throbber,
and this leads to
Uncaught Error: Syntax error, unrecognized expression: .ajax-progress.ajax-progress-throbber,
that breaks every ajax call with progress

@vflirt The concatenation has been fixed in the MR. Attached is a static patch of the D10 MR 4386 at commit e07684fd.
Pending Work: open MR for D11.x

hiding old patches to eliminate any confusion

Added D11 MR 4767
Updated both MRs with a fix for erroneous removals of "++" in core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php from original patch.

Attached static patches:
- D10: 2894747-D10-MR4386-6ac0afe9--20230913.diff
- D11: 2894747-D11-MR4767-5d67bf5e--20230913.diff

Attaching static patches for the MRs with passing tests.

Noticed this was adding additional classes to the block also but IS mentions just using a unique ID.
Example I turned the content view filters into a block placed on the /admin/content page
Before
views-exposed-form block block-views block-views-exposed-filter-blockcontent-page-1
After patch
views-exposed-form views-exposed-form-content-page-1 block block-views block-views-exposed-filter-blockcontent-page-1

The extra class is being added to the $form in "core/modules/views/src/Form/ViewsExposedForm.php:" (see below). Then views takes all of the $form classes and puts them on the wrapping block DIV instead of the FORM elemtn.

Patched "core/modules/views/src/Form/ViewsExposedForm.php:"

$form['#attributes']['class'][] = $clean_form_id;

All of the form look ups in the patch are using the new "starts with" ID selector so this new class may not be needed.

I have tested some of the patches attached in this issue (#84, #86 and #93) and all of them work with ajax disabled. Having 2 exposed forms with ajax enabled avoid the exposed form block from being replaced (both don't get replaced). Without the patch at least one of the blocks get replaced but with the patch none of them.

Might be related to my custom code, but I noticed that with #93 the ajax event handler is bound multiple times. This happens because `this.$exposed_form` contains all forms. I fixed this by replacing `this.$exposed_form` with a constant and added the form as argument to the `attachExposedFormAjax` function.

recrit → changed the visibility of the branch 2894747-views-hardcodes-exposed to hidden.

@Matthijs
(1) This issue has started with issue branches so please do not submit patches.
(2) If you do submit a patch, you should provide an interdiff.txt showing what you have changed since the previous patch. I attahced an interdiff for reference: interdiff-2894747-98-93.txt.

@Matthijs
I applied the intent of your changes to the MR's for 11.x (MR 4767) and 10.1.x (MR 5595).

(1) I kept this.$exposed_form being set. Some other JS or contribute modules may expect it to be set. Example: entity_browser contrib module.
(2) Calling Drupal.views.ajaxView.prototype.attachExposedFormAjax with a single element will not work since it is expecting to setup all forms and build the array this.exposedFormAjax = [];. I handled this by directly calling this.attachExposedFormAjax(); to setup all forms, and moving the once() check to the actual submit buttons instead of the exposed form in order to ensure that the actual buttons never have more than one ajax instance.

Attached are static patches for any composer builds.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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 rebased the 11.x MR

@recrit Thanks. It didn't want to change to the MR because I wasn't sure if the issue was caused by my custom code and existed for others as well.

@Matthijs Thanks, makes sense. Please try the latest changes to the MR to verify that it fixes your issue as well.

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

smustgrave → changed the visibility of the branch 10.1.x to hidden.

Removing tests tag as they've been added

There was 1 failure:
1) Drupal\Tests\views\Kernel\ViewElementTest::testViewElementEmbed
Failed asserting that actual size 0 matches expected size 1.
/builds/issue/drupal-2894747/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-2894747/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-2894747/core/modules/views/tests/src/Kernel/ViewElementTest.php:129
/builds/issue/drupal-2894747/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 3, Assertions: 18, Failures: 1.

The fix of using unique ID makes 100% sense just trying to think of a scenario where that could cause a breaking change for some, but ultimately any break would mean a site was broken this whole time.

RTBC+1

I have tested MR4767 and it works well on the front-end, but the backend is breaking for Media Library widget.

Once you try to insert a media on a node with Media Library form selection widget and use the text search there to find the media file, it redirects you to "/admin/content/media-widget/image" with 403 status.

If the MR patch is removed, it works well again.
Not sure whether the AJAX is not being re-attached to the form elements, but with the MR patch applied the pager for Media Library form still works well. It's just this search box that is giving hard time so far.

Good catch! Think we will need additional test coverage for this.

@veronicaSeveryn Please test the latest MR patches. I updated the "once()" calls so that it should process as expected.

Attached static patches for any composer builds to use.

Pending: New tests for the media library (if needed).

#79 patch not solving the issue for me after updating the core
The same patch merged with 2968207-40.patch for Drupal 9.5.11

Ported 2894747-10.1.x-MR5595-c96a7b79.diff for Drupal 10.2.3

Patch #116 contains JS syntax error so it doesn't work.

Not sure about this dude at 116 and his spelling errors ;)

In all seriousness, thank you to @lobodacyril for the heads up.
Here is the corrected patch

3rd times the charm?

Just fyi would recommend using MRs as they are faster and have quicker review turnaround.

we're currently working on the new version of facets, in the 3.x branch to make it work only as views exposed filters. This issue blocks us from having feature parity with the 2.x branch.
We plan on using the https://www.drupal.org/project/configurable_views_filter_block → module, which would also benefit from this patch.
Adding the contrib project blocker tag to reflect that.

The latest patch and the MR patches posted in #113 still break the media library search

hiding all 10.2 patches. The 11.x MR and patch in 113 🐛 Views hardcodes exposed filter block form ID's which breaks AJAX when the same form is shown multiple times on one page Needs review applies cleanly to 10.2.x.

The patch from #113 is not applicable to 10.2, here's the patch #120 with small but important fix

+    if (once('exposed-form', this.$exposed_form).length) {
+      this.attachExposedFormAjax.bind(this)
+    };

replaced with

+    if (once('exposed-form', this.$exposed_form).length) {
+      this.attachExposedFormAjax();
+    };

otherwise this.attachExposedFormAjax.bind(this) just binds this, but not call the function, so exposed forms don't work by AJAX

Following on from the feedback from #55, updated the logic so that only the relevant progress elements were removed as a side-effect.

The previous change inadvertently broke integration with Gutenberg 2.8 when editing Media blocks, because it removed the progress bar that already exists in the React lifecycle before it should've causing it to crash.

Resolved a rejection in core/modules/views/js/ajax_view.js when applying the previous 10.1.x patch to the 10.3.x branch.

sonfd → changed the visibility of the branch 10.5.x to hidden.

Patch in #127 looks like it includes a lot of additional code, hiding.

Attaching patch file with changes from MR !10945

I added a fix for the failing Unit tests in the merge requests for 10.5.x en 11.x (and also on the 10.1.x for consistency).
Let's wait and see if the tests pass now.

Updated the issue summary.

I reviewed the issue together with @wannesdr at drupal mountain camp. I had some remarks, which I shared in person. Wannes is fixing those as we speak. I think this issue looks good and I will rtbc this when the last things are fixed.

Good point @borisson_, removed the t function calls.
Thanks for the review & feedback!

I have tried to figure out if we are still missing something, but to me it seems like this is complete.

I was wondering if this would break styling on existing websites, but it won't, because the ID would already change when using ajax.