View inside core modal doesn't refresh correctly when reopened

Could you share with us the error messages?

I had this same problem, but I think it is because the drupalSettings.views.ajaxViews and Drupal.views.instances objects become stale and Views attempts to perform actions on a removed View. The attached patch fixed my exposed filter View inside a dialog when opening the dialog multiple times.

I am looking at this in the DrupalCon 2018 Nashville sprint. Anyone have a exported view that I can import to try this out?

I started looking at this but I realize that I don't have enough information to proceed. If anyone can give me full information on how to reproduce the issue I'd really appreciate it. Thanks!

I saw the same bug when working on #2962525: Create a field widget for the Media library module, and #8 fixed it for me.

To replicate:

1. Install the Standard profile.
2. Apply latest combined patch from #2962110: Add the Media Library module to Drupal core.
3. Create some Media.
4. Add a Media field to the Article content type, and choose the "Media library" field widget in the form display tab.
5. Visit /node/add/article and open the Media library. Use the exposed form to filter results.
6. Close and re-open the Media library modal, then try to use the exposed form again.
7. See an error in your browser's console.

- es6 cleanup.

+++ b/core/modules/views/js/ajax_view.es6.js
@@ -21,6 +21,20 @@
+          if ($(selector, context).length) {

From testing this patch I noticed that "context" is sometimes the top level .js-view-dom-id-* element. Should we change this check to $(selector, context).length || $(context).is(selector)?

This is a hard blocker for the Media Library being shippable in 8.6 (specifically #2962525: Create a field widget for the Media library module), so I'm escalating it.

This needs a test. However, I think we can leverage the existing Media Library view for this purpose (it has AJAX-ey exposed filters), plus a test module which can open it in a modal.

I wrote a test leveraging the existing test infrastructure for the Media Library module, which confirms the failure. The patch in #12 doesn't seem to fix it, though. That said...at least we have a test now.

Whoops; forgot to remove the Selenium stuff from MediaLibraryTest.

I did some test coverage tweaks and this seems to be working now. Uploading test only (FAIL) patch as well as interdiff.

The main problem was that dialogs have a feature that hides buttons within the dialog, then clone them to a statically positioned button pane. That means there were two "Apply Filters" buttons on pages, and the hidden was being clicked, which caused GastonJS to throw an error. Targeting the filter button in the buttonpane fixed the bug.

+++ b/core/modules/views/js/ajax_view.es6.js
@@ -21,6 +21,20 @@
+  Drupal.behaviors.ViewsAjaxView.detach = (context, settings, trigger) => {
+    if (trigger === 'unload') {
+      if (drupalSettings && drupalSettings.views && drupalSettings.views.ajaxViews) {
+        const ajaxViews = drupalSettings.views.ajaxViews;
+        Object.keys(ajaxViews || {}).forEach((i) => {
+          const selector = `.js-view-dom-id-${ajaxViews[i].view_dom_id}`;
+          if ($(selector, context).length) {
+            delete Drupal.views.instances[i];
+            delete drupalSettings.views.ajaxViews[i];
+          }
+        });

This looks sane overall! The views-view.html.twig always add the necessary css class.

Needs a reroll.

Re-roll.

Restoring RTBC once green.

Here we go again...

Re-roll, putting back to RTBC because no code changed.

It seems odd to add important views testing to the media library module. I think we should be testing this from the views module.

Discussed with @alexpott on Slack and it was decided that it's best for the test to be in Views now (rather than move/copy it in a follow-up) since that is a critical (and stable) module -- therefore, tests for it should not live or depend on an experimental module. Kicking back for that.

EDIT: Alex had this advice, as far as what the test view should be listing (i.e., what entity type):

See core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view_render.yml for example - this is a view on views_test_data

I moved the test coverage to the Views module, in the existing exposed filter test.

Ah, left debug code in yet again.

Lost me review when adding that tag, whoops.

1. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -21,6 +21,20 @@
   +  Drupal.behaviors.ViewsAjaxView.detach = (context, settings, trigger) => {
   ...
   +      if (drupalSettings && drupalSettings.views && drupalSettings.views.ajaxViews) {
   +        const ajaxViews = drupalSettings.views.ajaxViews;
   

   Is this how you're supposed to get at this value these days? Not using settings from the function params?

2. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -21,6 +21,20 @@
   +        Object.keys(ajaxViews || {}).forEach((i) => {
   +          const selector = `.js-view-dom-id-${ajaxViews[i].view_dom_id}`;
   +          if ($(selector, context).length) {
   

   Would be good to have a JS expert sign-off on this part.

3. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -21,6 +21,20 @@
   +            delete Drupal.views.instances[i];
   +            delete drupalSettings.views.ajaxViews[i];
   

   This part I'm good with, +1 :)

For #42.1: Normally settings would be used, but this logic is mimicking the logic in Drupal.behaviors.ViewsAjaxView.attach and using the global drupalSettings. We would need to change both functions to use settings, and I'm not sure what the impact of that would be.

➡ #42.1 - You really should be using the settings passed when the detach function is called. If you look in Drupal.detachBehaviors, you'll see settings = settings || drupalSettings;. You can see you're already using drupalSettings already.

➡ #42.2/3 - See additional comments below.

1. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -21,6 +21,20 @@
   +        const ajaxViews = drupalSettings.views.ajaxViews;
   

   Since we've updated our eslint version, there are some new rules. This should be destructred by default. const { views: { ajaxViews } } = drupalSettings;

2. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -21,6 +21,20 @@
   +        Object.keys(ajaxViews || {}).forEach((i) => {
   

   If ajaxViews is ever not a thing, use an empty object so we don't blow up the rest of the page. 👍

3. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -21,6 +21,20 @@
   +            delete Drupal.views.instances[i];
   +            delete drupalSettings.views.ajaxViews[i];
   

   This really only works if no more references to the same property are held. You might consider setting this to null or something else.

Addressing #44:

1. Fixed.
2. :)
3. We discussed this in chat and will keep delete for now.

Interdiff looks right to me and addresses all feedback from @tim.plunkett and @drpal. RTBC once green on all backends.

+++ b/core/modules/views/js/ajax_view.es6.js
@@ -13,14 +13,28 @@
+          const selector = `.js-view-dom-id-${ajaxViews[i].view_dom_id}`;

what if someone overrides the template that provides this class? I realise we're mostly concerned with media library, which would use an admin theme with known templates, but I think this could also impact front-end views, with unknown templates. In those cases is it kind of like contextual links - you accept that your stuff will break? We already store a reference to the element at the time of instantiation (in Drupal.views.instances[i]) can we use that instead?

@larowlan This class is also used in Drupal.views.ajaxView, so if a theme overrides it AJAX views will already break. As I understand it, classes prefixed with "js-" should not be modified or removed by themes, which is why (almost) all classes used in JS selectors are prefixed.

ok, thanks

adding review credits for reviews that shaped the patch

diff --git a/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php b/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php
index db16071939..c6cbaf5764 100644
--- a/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php
+++ b/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php
@@ -98,7 +98,6 @@ public function testExposedFiltering() {
     $this->assertFalse($session->getPage()->hasButton('Reset'));
   }

-
   /**
    * Tests if exposed filtering via AJAX works in a modal.
    */

Fixed on commit

Committed as ac310db and pushed to 8.7.x.

Cheerily picked as ab1b9a6 and pushed to 8.6.x

Hi there!

After updating a Drupal core, I faced an issue with the ajax exposed forms at views.
All the views are ok, after first ajax form submit but the second submit looses ajax.

Can anybody confirm this behavior?

Narrowed down the issue, the root cause is that the Drupal.behaviors.ViewsAjaxView.detach
function clears the view instance from Drupal.views.instances.

As a workaround I added a function nuller at the top of my theme's js attach...

Drupal.behaviors.themeName = {
  attach: (context, settings) => {
    if (Drupal.behaviors
       && Drupal.behaviors.ViewsAjaxView
       && Drupal.behaviors.ViewsAjaxView.detach) {
          Drupal.behaviors.ViewsAjaxView.detach = function() {}
    }
  }
}

@nicothezulu I also ran into this issue. See my patch here: #2968207: Allow multiple instances of the same exposed filter form on a single page