Add filter group for a View fails

Confirmed. Moved to View thread to see if any duplicated reports.

Add a test

Trigger testbot run

Triaging at DrupalCon New Orleans 2016

Triaged with @dbyers55.

Steps to reproduce are well documented. No duplicate issues were found. Issue is still relevant but needs more work. The patch is adding a JavascriptTestBase class for future testing of the issue. Reproduced in 8.2.x.

Thanks @catapipper and @dbyers55 for helping triage this issue! (Updating issue credit to include triagers.)

I can't reproduce this locally on OSX with Safari. I get the javascript error but it doesn't interfere with anything. Can anybody reproduce this on other platforms/browsers?

Updated the test so it should pass, and locally for me it passes, so my PhantomJS setup agrees with me. Let's see what the d.o. test bot thinks.

Even if it's not reproducible, the test alone is very much worth it to get this in (javascript tests for Views UI, yay!)

Hmm really working for me locally.

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\views_ui\FunctionalJavascript\FilterCriteriaTest

Test run started:
  Saturday, June 4, 2016 - 23:34

Test summary
------------

Drupal\Tests\views_ui\FunctionalJavascript\FilterCriteriaTes   1 passes                                      

Test run duration: 38 sec

Trying a different waiting strategy, since there is a weird double loading of the dialog.

Did some brainstorming with @dawehner on IRC and maybe the new group is not visible on d.o. because it somehow doesn't fit in the dialog.

So lets give it some more room by using a View with less filters set up. This passes locally too, but lets see what happens here.

You SHOULD start with UNTOUCHED VIEW to test. Comment #1 ~ #3 still validated.

Wrong returns from backend it seems:
https://jsonblob.com/57569226e4b01190df73cc84

It may be related:
#2647916: Views ajax modals stop working in certain scenarios: due to JS settings caching, AJAX responses may set wrong ajaxPageState.libraries, causing problems for subsequent AJAX requests/responses

You SHOULD start with UNTOUCHED VIEW to test.

I don't understand what you are trying to say.

The test in #3 will always fail because it tries to test isVisible on an element that isn't visible (the Add / Or rearrange button)

One other wait assertion to try.

For illustration, this is a screenshot taken during the test after the 'add group' click and wait.

Trying to figure out a way to do screenshots on d.o.

I think this is related

Adding more possible related issues

Updated test to use assertWaitOnAjaxRequest(). This still passes locally using both phpunit directly and run-tests.sh

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
@@ -0,0 +1,55 @@
+    $dropbutton->click();
+    $add_link = $page->findById('views-rearrange-filter');

It seems this part doesn't fast enough to response the next click in testbots.

I often hit this error in my own projects. We dropped a timer there ( http://codeception.com/docs/modules/WebDriver#waitForElementVisible )

For behat:
http://docs.behat.org/en/v2.5/cookbook/using_spin_functions.html

(or waitForAjaxToFinish may work, waiting until the CSS changes)

But in #2, I can reproduce bug with human HANDs and mouse :s

Well according to the test result it fails on the last assertion FilterCriteriaTest.php on line 52.

So that is inline with the described bug in the issue summary. But like I said before, I can't reproduce it by hand, nor can I reproduce this test failure on my local setup. So without the ability to reproduce anything, I'm leaving this to others to fix. I just wanted to update the existing patch to use the latest available methods.

I have same problem!
I can confirm the bug is present in Drupal 8.2.1.
It can easily be reproduced by making a fresh install for drupal at simplytestme and following the steps above:
https://simplytest.me/project/drupal

After you clicking "Create new filter group" I have some different behaviors; Sometime the new filter group disappears just after some seconds, another time it did in fact stay there. To me it seems like the group disappears if you move your mouse a lot after clicking "Create new filter group", but remains there if you try keep the mouse still for some time after clicking.

After some hassle I managed to drag 1 field inside the new filter group, however after I click "Apply" and then "Save" it seems like the filter group is not saved after all!

After I managed to drag 2 field inside the new filter group and then it finally worked. When I try to remove one filter group again, these changes are not applied. It seems like the filter group will not be saved if there is only 1 field inside it, I'm not sure if this is related.

The problem was that the form was submitted twice, this was visible by the dialog disappearing and appearing.

I checked the JavaScript and the problem was with the way the form was submitted in the clickAddGroupButton method. The mousedown event also triggers the submit thus triggering only this event is enough. The submit event was the problem as it triggered the second, invalid (missing the clicked button, causing all kinds of problems), submit.

Please verify this fixes the issue.

Now without the newline.

@michielnugter nice!

+++ b/core/modules/views_ui/js/views-admin.js
@@ -736,9 +736,7 @@
       // Due to conflicts between Drupal core's AJAX system and the Views AJAX
       // system, the only way to get this to work seems to be to trigger both
       // the mousedown and submit events.

At the very least we should update this comment. But apparently this was specifically needed at some point.

Looking at #2168893: Views filters groups adding and removing is broken, for the clickRemoveGroupButton function, this double trigger was added because it didn't work without it. And before that fix there was a comment in the clickRemoveGroupButton function:

// For some reason, here we only need to trigger .submit(), unlike for
// Drupal.viewsUi.RearrangeFilterHandler.prototype.clickAddGroupButton()
// where we had to trigger .mousedown() also.

So for some reason the need to trigger two things has switched from add to remove??

Hmm, missed the comment. Weird that the solution in the mentioned issue was accepte, it's a very dirty solution and actually a workaround for the actual bug.

I dug into it a little deeper.

The add button has a mousedown event handler in:
- (!) misc/ajax.js line 532
- views_ui/js/ajax.js line 43

The remove button misses the misc/ajax.js event handler, which handles the ajaxified submit. As I understand this is caused by either:
- An incorrect way of initializing the submitbutton, causing it not to be recognized by the ajax framework.
- Not passing the new drupal settings to JavaScript after adding the new group.

The trigger for the 'submit' event works around the problem of missing the ajaxified submit by triggering the form submit, which was already ajaxified.

I haven't had the time yet to dig into it deeper to figure out which it is.

It seems it was the first option, the submitbutton was not ajaxified. I made a small change to ajaxify it and and I removed the other 'submit' event trigger. It still works here and the test passes, let's see if it still passes here as well.

I also removed the comment as it's nothing special anymore..

+++ b/core/modules/views_ui/js/views-admin.js
@@ -733,9 +733,6 @@
-      // Due to conflicts between Drupal core's AJAX system and the Views AJAX
-      // system, the only way to get this to work seems to be to trigger both
-      // the mousedown and submit events.
       this.addGroupButton.trigger('mousedown');

I always wonder what that is in the beginning. There is few place in CORE used this pattern.

+++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
@@ -129,6 +129,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          '#ajax' => ['url' => NULL],

I didn't dig into source yet. Will it explain why it's random failing (in real human testing & real browser)?

Will it explain why it's random failing

Hmm yeah, good question.

+++ b/core/modules/views_ui/js/views-admin.js
@@ -750,7 +745,7 @@
+      this.table.find('#' + event.data.buttonId).trigger('mousedown');

This will of course need additional testing, but I feel that making both buttons operate the same way is a good addition and within scope here.

The random failures have a somewhat likely explanation. Currently, the click on the add button triggers 2 AJAX calls to the server (in this order):

1. A correct one with a triggering element. This respons will add a filter group
2. An incorrect one without a triggering element. This respons will not change anything in the form.

If the second request finishes last, the form doesn't contain the new filter group.

I reverted the patch to test the behavior and got exactly what I expected. Sometimes it works, sometimes it doesn't. It completely depends on which request finishes last. When applying the patch again the double AJAX load is gone and both the buttons work consistently and without any flickering.

On the other note: I searched core for the incorrect 'submit' event trigger, it's nowhere to be found luckily.

Now with additional testing for the remove button. I also moved the user creation and variable setting to the setUp() method.

... And better naming and more comments after lendude's feedback.

@michielnugter nice clean up.

We have a test, we have a fix, we have a good explanation of the randomness.

Looks great and nice to have a proper js test for it.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!