Behaviors get attached to removed forms

I can reproduce this, it is contributing to the problem described at #1401038: Wysiwyg not showing, Node Form in a Ctools Modal.

Here is a reroll for D8.

Hmm, I recently had this cause problems on Firefox.

TypeError: Value does not implement interface Node.

This comes from jQuery.contains, and it seems that FF stricly enforces that it must be a DOM object.

$.contains(document, this.form[0]) works fine in FF...

Actually, that makes some sense.
jQuery.contains() expects two DOM elements.
http://api.jquery.com/jQuery.contains/ is VERY explicit about the first argument being a DOM element, but it does say the second should be as well.

It seems that Chrome is more liberal with this, but FF is being strict.

This code has changed from D7 to D8, it's now this.$form to clarify that its a jQuery object, and the following lines in the attachBehaviors call use this.$form.get(0), which is exactly what we need here.

Not as sure what to do with D7, since I think the param passed to attachBehaviors is technically also wrong...!

We are re-rolling the patch in #3 to be up to date with 7.39 of core.

Sorry got that last patch wrong. Try try again.

Wow. Had a crazy interaction with another patch we are using. Sigh. So sorry about all the noise.

Here is a reroll for 8.0.x-dev.

This would also fix a problem with .once() in views ui #2493945: Views filters dragging in the grouping view is broken.

i have rerolled de patch #9 against 8.0.x

See attached rerolled patch from #14. This was rerolled against latest 8.2.x, please let me know if 8.3.x would be more suitable.

Tested it against 8.2.x. It applied cleanly and fixed my problem with #2493945: Views filters dragging in the grouping view is broken

Actually there's still something a bit wonky with views grouping.

Base scenario: have views exposed filters. I want some of them AND-ed and some OR-ed.

Scenario 1 (works):

1. Add all the filters to the view.
2. Click on 'rearrange filters' button
3. Add a new filter group
4. Move some of the filters to the new group, put an 'OR' on the second group
5. Save
6. Everything is saved as expected.

Scenario 2:

1. Add all the filters to the view.
2. Click on 'rearrange filters' button
3. Add 2 new filter groups
4. Move the filters to the two new groups - I have an OR on the 2nd group, AND on the 3rd (shouldn't matter)
5. Save
6. Some of the filters are moved, some refuse to move.

FYI, I had 6 filters in total - 2 fixed ones in the first group, and 2 exposed filters in each of the other 2 groups. Each group was 'AND'-ed together, but within the group the fields were not necessarily.

Adding the groups one at a time, saving and then repeating, didn't seem to have any effect.

I updated the patch to fix the issue with views, there was another check requiring the form-is-in-dom validation.

Also, I posted a work-in-progress test in #2493945: Views filters dragging in the grouping view is broken which I propose to move here (see attachments). It uses the views filters dragging to validate the patch is working. I marked the other issue as a duplicate of this one as this fix (after my addition) also fixes that issue.

Note: this patch requires the fix in #2717629: Add filter group for a View fails to pass.

I'm not sure with the test yet. I'd like other opinions (@droplet?) on how to proceed:

- The incorrect behavior is only reproducable in specific places in Drupal core. Is writing a test (like the current one using views filter dragging) based on a specific case ok?
- It's a problem with ajax.js which currently does not have any functional JavaScript testing (right?). It's ideal to have it and include coverage for this patch. This however is quite a large untertaking and will force this patch to wait for a long time while it's nice to have it in. Maybe accept it for now (and clean up the test) and open a new issue for the ajax coverage?

There's still something awry with the views filter dragging between groups. It's still not behaving as expected with some filters movement not being saved. I haven't figured out a pattern yet, but might be something to do with their index within the group. For example I couldn't add a 2nd filter to a filter group at the bottom, but adding it to the top of the group worked.

There's now also something else weird happening when I add 2 empty filter groups to views. The first gets added, but adding the 2nd one either causes the page to reload with my first new group removed, or reloads with no modal dialog open at all.

I also can't add any new groups (brand new clean view to be sure) and have had to revert back to the patch from #18 in order to get any groups.

Ok I had an out of date patch for #2717629: Add filter group for a View fails. Updating that fixed the weirdness I was experiencing with the #24 patch. However, still experiencing the issue from #25.

My scenario: 5 filters (2 fixed, 3 exposed)

steps:

1. Add 2 new filter groups.
2. Leave the 2 fixed filters (filter #1 and #2) in group #1.
3. Move filter #3 to group #2 (this is an OR)
4. Move filters #4 and #5 to group #3

Result: everything is ok except for filter #5 which stays in group #1.

Caveat: if I move filter #5 to the end of group #3 it doesn't work as above, but if I move it to the top of the group, it stays in group #3. Then after a save, I'm able to move it to the desired position at the end of the group.

It's much better than without the patch, and is usable with some workarounds, but not 100% there yet.

As discussed on IRC: the views issue is split into a seperate issue as it's not directly related to this issue.

#2825446: Moving a filter to the end of a Filter group in filter rearrange doesn't work but does allow dropping in invalid regions

@slasher13 The included test is build based on the test in #2717629: Add filter group for a View fails and includes the test for that issue and will therefore fail here. Not sure how to proceed, I asked droplet on IRC to check this issue to see what the best approach is for getting this fix committed including the test.

Update for the patch as #2717629: Add filter group for a View fails has been committed.

The test should pass now.

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
@@ -66,6 +66,20 @@ public function testFilterCriteriaDialog() {
+    // Validate dragging to an invalid location doesn't work.

Are we dragging to an invalid location?

Which of the three changes to ajax.js is covered by this test? Probably not all three right?

Thanks for the review.

I'll update the test with correct comments. Good point about the coverage. I checked which changes are tested and only 2 of the 3 changes are excuted (I placed breakpoints to verify).

I'm having some trouble with the third, this is a check in the error callback, I haven't figured out a way to make a request fail consistently yet.

I've been trying for a while now to trigger the error form the UI but no such luck.

If a complete coverage is to be attained I think a completely separate test for ajax.js with a test-module which contains all the required callbacks and forms is required. This however, is quite a large undertaking and maybe out of scope for this issue.

I'm very curious to see other opinions on this.

Patch contains a comment fix and incorrect indent fix.

Patch no longer applies to 8.2.x (after 8.2.7. security update) and i need this for my composer workflow.

Reroll for 8.4.x and ES6

This fixes the test. This still fails (unsurprisingly).

edit: interdiff looks weird, ignore it.

The fail in #44 is due to the hack to test for the alert. If we use a proper alert, it works fine. Let's see how the testbot handles this....

Do we need the jQuery version of this? DOM has the contains method https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

@nod_ ++, that seems to work fine too (locally anyway)

Does it needs manual testing? Looks like better to add a js test

Just want to highlight that with recent patches from this issue we're able to get #2825446: Moving a filter to the end of a Filter group in filter rearrange doesn't work but does allow dropping in invalid regions working correctly.

I might be wrong, but it appears to me that this does indeed have JS tests (since the folder says FunctionalJavaScript), so tentatively marking RTBC, since #51 indicates that this solves the problem.

And since this blocks a major bug (#2825446: Moving a filter to the end of a Filter group in filter rearrange doesn't work but does allow dropping in invalid regions), escalating to major as well.

+1 to RTBC. This indeed has JS tests. Removing the stale tag. Code looks good and clean. Tests + bot are happy. This is blocking fixes for other important bugs.

Thanks,
-Derek

Committed 57f1f1a and pushed to 8.7.x. Committed 18296172dc and pushed to 8.6.x. Thanks!

Backported to 8.6.x because the JS changes are minimal and not risky as far as I can work out. https://developer.mozilla.org/en-US/docs/Web/API/Node/contains shows that Node.contains() is supported in all major browsers.

diff --git a/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
index f131fbfe16..a9ec2475b6 100644
--- a/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
@@ -86,7 +86,7 @@ public function testFilterCriteriaDialog() {
   }
 
   /**
-   *  Use the 'And/Or Rearrange' link for filters to open a dialog.
+   * Uses the 'And/Or Rearrange' link for filters to open a dialog.
    */
   protected function openFilterDialog() {
     $assert_session = $this->assertSession();
diff --git a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php
index 6e45572965..f2e9577048 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php
@@ -132,4 +132,5 @@ protected function assertWaitPageContains($text) {
     });
     $this->assertContains($text, $page->getContent());
   }
+
 }

Fixed coding standards on commit.

We have found out that after the update to 8.6.8 the following issue is received on ajax.
In IE11 the document doesn't contain the contains function.

The function is in document.body.contains.

So will switching from document.contains to document.body.contains fix the issue?

I changed document.contains to document.body.contains locally and it work in all browser.

@gaydamaka thanks for the quick fix. Looks good to me.

We already use document.body.contains in ajax,js so +1

  Drupal.ajax.expired = function () {
    return Drupal.ajax.instances.filter(function (instance) {
      return instance && instance.element !== false && !document.body.contains(instance.element);
    });
  };

This looks to cover all uses of document.contains, it does add a new line at the end of the file, not sure if that's needed on JS files or not, but that can be fixed on commit depending on what is the preferred standard.

Patch attached fixes the ajax.js file so that it it generated by the js build process. Do use make you changes to the .es6.js files and then do yarn run from /core and you can build the js.

+1 on this change. The document object in IE doesn't have a contains() method and this is the correct fix in this case. There shouldn't be any downsides or risks making this change.

Updating the version to reflect the first version of Drupal core the original patch landed in.

Committed a fix and pushed to 8.7.x and 8.6.x. Thank you everyone for reacting to this so quickly!