Pager is not initialized in nested views in Drupal.views.ajaxView

+++ b/core/modules/views/js/ajax_view.js
@@ -38,6 +35,35 @@
+    // Sort views by nesting level descending. The goal is to start with the innermost.

Line exceeding more than 80 characters
Applying the patch, please review.

I can confirm that https://www.drupal.org/files/issues/2858890-2.patch does work for us! Nice work Blazey

We can now write tests that leverage javascript. Yay! So we should add a test here to ensure we don't regress and have properly fixed this. See https://www.drupal.org/docs/8/phpunit/phpunit-javascript-testing-tutorial

Patch #13 won't apply on 8.4

Patch #13 rerolled against 8.4.2

diff --git a/core/modules/views/js/ajax_view.js b/core/modules/views/js/ajax_view.js

These changes are against the transpiled code. Please make your changes against the .es6.js files and then transpile them. For more information have a look here, Drupal core now using ES6 for JavaScript development.

EDITED: Skip this patch... look at #26

Patch #22 wasn't applicable on 8.5.x.
Now I've rerolled it against 8.5.1, with additional ajax_view.es6.js code transpiled to js

Sorry, #25 was a bad file... reuploaded as #26

Applied the patch to Drupal 8.5.3 and can confirm it fixes the Ajax pager in nested views. I tested: 1 nesting level, tested normal mini pager and a Calendar view with Calendar pager.

1. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -15,9 +15,8 @@
   +      Drupal.views.sortByNestingLevel(drupalSettings.views.ajaxViews).forEach(function (item) {
   

   Arrow function.

2. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -15,9 +15,8 @@
   +        Drupal.views.instances[item.key] = new Drupal.views.ajaxView(item.value);
   

   Need to add an exception because `ajaxView` is incorrectly cased for a constructor.

3. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -33,6 +32,36 @@
   +  Drupal.views.sortByNestingLevel = function (ajaxViews) {
   

   Should be an arrow function.

4. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -33,6 +32,36 @@
   +    var ajaxViewsArray = [];
   

   const.

5. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -33,6 +32,36 @@
   +    for (var i in ajaxViews) {
   

   Don't use a `for...in` loop, use an actual iterator.

6. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -33,6 +32,36 @@
   +      if (ajaxViews.hasOwnProperty(i)) {
   

   If you use `Object.keys()` you won't need this.

7. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -33,6 +32,36 @@
   +        ajaxViews[i].selector = '.js-view-dom-id-' + ajaxViews[i].view_dom_id;
   

   Template strings.

8. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -33,6 +32,36 @@
   +        ajaxViewsArray.push({
   +          key: i,
   +          value: ajaxViews[i],
   +          nestingLevel: $(ajaxViews[i].selector).parents('.view').length
   +        });
   

   Since you are just pushing a value, this entire thing can be replaced with `Array.map()`

9. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -33,6 +32,36 @@
   +    // Sort views by nesting level descending. The goal is to start with the
   +    // innermost.
   

   Incorrect multi-line comment format.

10. +++ b/core/modules/views/js/ajax_view.es6.js
    @@ -33,6 +32,36 @@
    +    return ajaxViewsArray.sort(function (a, b) {
    +      return b.nestingLevel - a.nestingLevel;
    +    });
    

    You can probably just chain the `.sort()` and return it. Also use an arrow function for the handler.

11. +++ b/core/modules/views/js/ajax_view.es6.js
    @@ -72,7 +101,7 @@
    +      selector: settings.selector,
    

    You've changed this to `settings.selector` however the original variable is still around.

I've applied the patch in #26 and it fixes the problem for me.
My use case: I've a node where a field is using paragraphs so one can choose between different paragraph bundles. One of the bundles uses an entity reference field, to reference blocks. The block I'm referencing is made with views, it's a news feed with teasers, using views infinite scroll. Without the patch, the teasers are replaced when clicking load more, instead of being added to the bottom. With the patch it works.

Unable to apply the patch :

  - Installing drupal/views_infinite_scroll (dev-1.x 74c9ba8): Cloning 74c9ba8668 from cache
  - Applying patches for drupal/views_infinite_scroll
    https://www.drupal.org/files/issues/2018-03-29/2858890-26.patch (Drupal.views.ajaxView is not initializing pagers in nested views)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2018-03-29/2858890-26.patch

Argh, bad patch. Missed new files. Ignore.

Patch in #26 was not applying vs 8.6. This hopefully fixes that, but doesn't address the code cleanup cited in #30.

Second try updating #26 for 8.6.x. Seems good (minus the changes needed from #30).

- reroll for 8.7.x
- resolves my review from #30

I ran into this bug myself.

Here is the review of the current patch.

+++ b/core/modules/views/js/ajax_view.es6.js
@@ -50,6 +53,29 @@
+    ajaxViews
+      .map(ajaxView => ({

When trying out this patch I ran into an issue: ajaxViews is an object so .map fails.

Any news on this?

My patch works for me with 8.7.7.
Sorry for only changing the ajax_view.js but ! needed a quick fix.

Patch #42 works for me but doesn't apply correctly.
Rerolled against 8.7.x branch.

This is just a re-upload of #36 to prevent people from rerolling stuff that stripped away most of the relevant code in this fix.

#36 seems like the way forward since that has test coverage and es6, if there is an argument to use #42 please post a comment as to why we would use that. Just posting a short cut when there is an actual fix under discussion seems a little counter productive.

Sorry Lendude, you're right.
In my case #36 not working correctly (same problem as #38). #42 works perfectly but doesn't apply (using composer with cweagans/composer-patches). #43 is same patch as #42 but apply flawlessly. I'm sorry that it strips away relevant code but, as specified in #42, it's a quick fix.
Thank you.

Isn't this related / duplicate? #2918352: After update to Drupal 8.4 VIC replace results instead of appending

Both have the nested views problem don't they? If yes, we should have a look and perhaps close the other?

Hm I'm not sure Anybody. But what I can say: The patch #43 works well with 8.8.1.
It would be great if this fix can be commited.

I just tested patch #43 and #45 in combination with patch #19 from #2918352: After update to Drupal 8.4 VIC replace results instead of appending.

Patch #45 does NOT work in the current state!
Patch #44 (even if it's a workaround) fixes the problem!

Shouldn't we set this major, as it regular views ajax pager functionality is broken (for all pager types!) under given circumstances? For us it happened when using the view in layout_builder.

Patch #43 works for me but doesn't apply correctly.
Rerolled by 9.1.x branch.

Setting priority to major as it prevents dependent modules from working like here: #2918352: After update to Drupal 8.4 VIC replace results instead of appending (also major issue)

The ES6 file should be edited to keep changes after transpilation in build steps. Made some optimisations, e.g. for...in should be avoided.
The once() call should go to the items itself, to be able to exclude them once the parent view get's initialised. There's no need to call once() on the view itself for the pager. I tried to align it a bit to the exposed form method.

AJAX test was failing because of unwanted parameter.

Is the AJAX test somehow outdated? "selector" is not a new key in element settings, but yet it is considered "unwanted" parameter!?

2858890-58--8.9.x.patch seems to work great with #42 from #2833734: Add pager per each Views attachment display

I also confirm for Drupal 9.1.x + #2858890-58 + #2833734-42 are fixing the issue.

I can also confirm #58 to work with 9.15! Any idea how to proceed here with the failed patch or who to ask for the problems?

2833734 #42 + 2858890 #58 It shows up the pager, however, when I use the infinite scroll pager, and I have a block with an attachment (after) than when I load for the main block or the attachment more items with the pager, it's added to the other list too. That's not good. But I wonder if this is not a bug in the infinite scroll to target wrongly the container.

Patches #2833734-42 + #2858890-58 are fixing the issue for Drupal 9.2.x.

#58 doesn't apply anymore for Drupal Core 9.3.x

Tried re-rolling patch. Please review.

Fixing missing parentheses/semicolons in the patch from comment #70

This one worked for me with 9.3.0 version. Thanks for adding the patch.

Any plans to push things forward in core and views infinite scroll to get this nasty and years old major bug away?

What would be needed is feedback if the current approach is the right way to go or if it should be solved differently in general? Who can decide that?
I'd like to help in code to fix this, but it doesn't make sense without feedback for the current patch...

This patch seems to be used by many Drupal users and is rerolled again and again... to be honest we couldn't ever use views_infinite_scroll correctly without the two patches mentioned above...

Attaching a new patch fixing missing line in attachExposedFormAjax, causing exposed filters not firing ajax callback and reloading the whole page.

Patches 74, 75 make one issue on ajax at a time to add media fields to nodes.

I can confirm what #77 is saying. Patches 74 and 75 cause a critical problem with media library on Drupal 9.3.

To reproduce this, create a content type with an image media reference and use the media library widget.

When you "Insert selected" in the media widget to add an image, it causes an extra irrelevant ajax request which responds with a redirect to a 404.

After that, the image seems to be inserted okay but then you can't save the node. A similar irrelevant ajax request happens when you click Save. This crashes, preventing the node from being saved. I think a selector needs to be more specific.

Patch 71 seems to work okay on Drupal 9.3.

The attached patch fixes the problem explained in #78.
In a forEach() callback, the first argument is the item, not the second one. The jQuery selector was getting every submit input in the page, now it only applies to the current form.

This works great for me. The Media Library brokenness was a real trip, but I think we're all set here now.

Thank you very much @prudloff, what about the failing 18 Tests? #44 had all tests green.
@if-jds: This can't be RTBC'd until all tests pass!

Indeed #74, #75 broke the media library widget and lead to: #3263896: Media library browser selection returns statuscode 303 on AJAX
Huge thanks to @nsavitsky for pointing me into this direction!

It's quite major to have either load more AJAX or the media library broken, so I've tried to fix this. I simply made it use the new code if the views infinite scroll container is inside the view. There may be a better way of doing this but at least in my testing it now works in both scenarios.

I can confirm that #81 fixed my problems with the media library widget. We also tried #86 that fixed the problems with the filter but broke pagination.

This is just an update of @Rob230's patch above. The new patch:

1. Re-instates the functions that the code in the non-Infinite Scroll "else" was still using, but which were removed elsewhere in the patch - This should solve the pagination issue Nelson mentions in #89
2. Adds in an exclusion for Entity Browser tab's hidden buttons to prevent them from being targeted by the "attachExposedFormAjax" function. Without this, it wasn't possible to switch tabs once the fix in point 1 was implemented.

There is a further issue here, whereby entity browser's filters were broken. I tracked down the problem to the changes to the Drupal.views.ajaxView.prototype.attachExposedFormAjax function, specifically the fact it now takes form as an argument. In entity_browser.view.js it calls attachExposedFormAjax but the argument passed is 0.

Obviously this is a core bug so we can't modify the entity_browser module. The issue here is that a breaking change has been made to Drupal functions which other modules may be calling.

My fix for this is just to add a check on the form value and if it's empty set it to what it would be in the unpatched core: this.$exposed_form

The concern is are there other places where the prototype functions have been changed which other modules might be using.

Confirming the issues with entity_browser mentioned in #90 and #91.

#91 looks like a clever solution and works for me. So setting this back to "Needs review". Thanks @Rob230 once again!

RTBC+1 from my side and I agree with the major priority due to the current situation.

#91 sadly does not apply anymore.

Rerolled #91 against 10.1.x as MR. Please let's proceed in MR's instead of patches for the work to be committed. Patches for 9.x versions are of course welcome based on the MR.

MR LGTM!

Retesting it for Drupal 10.1.x to be 100% sure.

Crazy, ESLint validation is finally part of Drupal's Jenkins validation, very nice!!

Hm, I get a different ESLint errors/warnings, when running ESLint locally using "core/.eslintrc.json" in Drupal 10.1.x:

/var/www/html/drupal-2858890/core/modules/views/js/ajax_view.js
    6:2   warning  Unexpected unnamed function                                  func-names
   16:43  warning  Unexpected unnamed function                                  func-names
   22:56  error    A constructor name should not start with a lowercase letter  new-cap
   66:37  warning  Unexpected unnamed function                                  func-names
   89:27  warning  Unexpected unnamed function                                  func-names
   99:7   error    Use array destructuring                                      prefer-destructuring
  172:3   warning  Missing JSDoc for parameter 'form'                           valid-jsdoc
  179:59  warning  Unexpected unnamed function                                  func-names
  205:55  warning  Unexpected unnamed function                                  func-names
  214:53  warning  Unexpected unnamed function                                  func-names
  230:57  warning  Unexpected unnamed function                                  func-names
  264:50  warning  Unexpected unnamed function                                  func-names

✖ 12 problems (2 errors, 10 warnings)

Otherwise, the MR seems fine!

I've updated the patch from @Rob230 to work with Drupal 9.5.1

Tried to fix CCF for #101.

Rebased against the most recent 10.1.x codebase.

With patch #103, works after cleared cache. However, issue reappears after some time. Caching issue?

@Anybody can you cherry-pick https://git.drupalcode.org/project/drupal/-/merge_requests/3125/diffs?co... to the 9.5.x branch? I think only the issue branch creator can cherry-pick for some reason.

EDIT: Here is the commit directly: https://git.drupalcode.org/project/drupal/-/commit/4474784449c38cae17790...

Unsure about patches #101 and #103. There is a 9.5.x branch in https://git.drupalcode.org/issue/drupal-2858890/-/tree/2858890-9.5.x-vie.... Are your patches based on the diff of this branch? Let's not spread further confusion and finally resolve this year-old issue. Please work on the provided issue branches, thanks! 🙂

@ckng can you confirm this issue still exists with the diff of 9.5.x applied as a patch, once @Anybody cherry picks @nsciacca "once" change?

https://git.drupalcode.org/project/drupal/-/merge_requests/3126.diff

@Grevil: No, sadly no such option. Can only be done manually or by patch, sorry.

Ok something went wrong here...

Alright! Everything should be cleared up now! I manually applied the relevant changes of https://git.drupalcode.org/project/drupal/-/merge_requests/3125/diffs?co... by @nsciacca and fixed the 11.x branch again! The main problem, was that 11.x ditches the old "ajax_view.js" in favour of "ajax_view.es6.js".
Please review!

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.

How annoying... "MR !3126" can not be applied to 11.x as it is the 9.5.x feature branch... Going to close the MR for now in hopes the bot won't set it to "Needs work" again.

Can the issue summary be updated with steps to reproduce. Also the link is pointing to 8.4 so definitely outdated.

Also as a bug will need a test case.

OK, it seems that all patches after "drupal-2858890-79.patch" by @prudloff explicitly check for the ".views-infinite-scroll-content-wrapper". This definitely be avoided inside a core issue.

It also seems that patch 86 was originally created without a reason? From the conversation, the original problem with the broken media library got fixed with patch 79 and there were only tests left to fix. Then suddenly patch 86 got introduced with the same goal to fix the media library, which introduced the check for ".views-infinite-scroll-content-wrapper".

I'll create a second branch with the changes from patch 79 and see how that goes.

EDIT: Note, that all failing tests for patch 79 are unrelated deprecation notices.

Let's see if tests succeed on that branch as well (1 failing test is expected, which needs to be refactored).

Alright! Infinite scroll works again in conjunction with the latest issue fork changes from #2918352: After update to Drupal 8.4 VIC replace results instead of appending for 11.x! :)

Now let's see, what the tests say, and then we can create a fork, to backport the changes for 10.1.x as well!

(We of course also need independent tests, not including any infinite scroll stuff)

I don't know, how to properly test this, so I am leaving this issue on "Needs work" for people to create tests.

(For all infinite scroll users, coming from #2918352: After update to Drupal 8.4 VIC replace results instead of appending, this core patch seems to not be needed any more for Drupal 10.x)

I am removing the "major" priority, as it wasn't major to begin with.

Update to my comment in #127. The issue in #2918352: After update to Drupal 8.4 VIC replace results instead of appending isn't a thing anymore, it works just fine in current 2.0.x.

Meaning that the manual testing I did for this issue was incorrect... 🫠

So if anyone runs into this issue, please test the current issue branches, and see, if it fixes the problem for you:

• "2858890-2858890-views-ajax-fix-init-in-nested-views-patch-79-10.1.x" => for 10.1.x
• "2858890-views-ajax-fix-init-in-nested-views-patch-79" for 11.x

If so, it would be really nice, to add the steps you took inside the issue summary's "Steps to reproduce" section. And if you can also write tests, please verify the fixed behavior through tests!

Thanks.

rebased 2858890-views-ajax-fix-init-in-nested-views-patch-79 branch from 10.1.x to make the changes applicable.

The patch/MR works on 10.1 however it breaks the entity_browser modal pop up. Anyone else use both and found a solution? I tried using the patch in 91 but it didn't work to fix the pager issue on the newer code base.

Update: I added some additional selectors to exclude in the attachExposedFormAjax .not to fix the issue with Entity Browser:
.not('[data-drupal-selector=edit-reset],[data-drupal-selector^="edit-tab-selector"],.is-entity-browser-submit')

I opened another MR for the 10.2.x branch as neither existing MR/patch was applying cleanly. Includes my modification with the extra selectors that were breaking Entity Browser.

Merge request !5867 doesn't apply to Drupal 10.2.2.

Edit: actually it does work. I don't know what the problem was before. I downloaded the diff again via wget and it applied.

It looks like patches used to have tests but they were dropped starting with comment 42.
I added them back.

left a small comment on the MR but leaving in review

Since it's been 2 months left some comments on the MR around the tests.