Views add dialog filter searches on label instead of title

@othermachines nice find! Manually tested this and you are spot on.

Fix makes sense, we need some Javascript tests for this though. There is some basic search setup in #2754985: [backport] Add JavaScript test coverage for adding an exposed filter in Views UI but that tries to do more and it's postponed on the extra things it's trying to do. So maybe some dedicated tests for the handler filtering would be great here.

Are you suggesting pulling out the filter-related stuff into a simpler test?

Well yeah, looking at it now, combining the handler search testing with the exposed filter test makes little sense. The search needs its own dedicated test, and in that test I think its fine to add a search for the 'date' string and see that it currently fails to give the expected result.

Marked #2847555: Searching for fields in views_ui doesn't work as expected as duplicated from this issue.

So I have a different problem, that is not fixed by this issue. If you search for ID, the results are not really representative.

This new patch sort the handlers table by string length so if you type a few letters of a word, you will see first the shorter strings and will have more chance to find what you are looking for first.

I think it isn't duplicated. This one is bug fixing. Another one is search result improvement. #8 improvement may take a longer time to discuss in details.

I also created an issue to seek a common workaround for all these search fields.
#2846950: Narrow down Instant Search Field result in CORE

Patch #3 looks good. It's ready to go.

I've tested patch #3 and #8 against 8.4.x.

Patch 3

The results for #3 are as follows:
Before patch:

After patch:

This works as described.

Patch 8

Patch #8 also performs a fix for this problem but it also has some other side effects.

Before patch:

After patch:

As you can see the order of the fields has change.

Therefore I would suggest moving patch #8 into Narrow down Instant Search Field result in CORE and commit #3 to fix the immediate problem.

Setting to RTBC for patch #3 and hiding #8 from display.

As discussed in #5-#7, this still needs tests before we can RTBC it.

On reading up on this I agree with @droplet that this is a bug fix and #8 is an improvement, sorry @dagmar for implying that they might be the same thing. An improvement very much worth doing though!

@othermachines nice!

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
@@ -0,0 +1,80 @@
+    $web_assert->assertWaitOnAjaxRequest();
...
+    $session->wait(1000);

This should be updated to use the new preferred assertions that were added in #2837676: Provide a better way to validate all javascript activity is completed

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
@@ -0,0 +1,80 @@
+    // Test that the number of rows returned is less than total rows.
+    $this->assertTrue(count($visible_rows) < count($all_rows), 'Number of rows returned is less than total rows.');

An assertion to ensure it has the same number of "Updated" & rows at the beginning is required.

Small cleanup

- Used the new wait... methods.
- Added doxygen
- Added coverage for filterVisibleElements() by first checking the count before filtering. That covers @droplet's input right?
- Removed assertion messages as they're discouraged and fixed the deprecation.
- Changed deprecated assertFieldByName() implementation.

+++ b/core/modules/views_ui/js/views-admin.js
@@ -483,18 +483,18 @@
-        $label = $option.find('label');
+        $title = $option.find('.title');

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
@@ -0,0 +1,96 @@
+    // Assert the count is equal before filtering.
+    $all_rows = $page->findAll('css', '.filterable-option');
+    $visible_rows = $this->filterVisibleElements($all_rows);
+    $this->assertTrue(count($visible_rows) == count($all_rows));

Thanks @michielnugter! It's very close but it isn't what I wanted.

The reason we ran into the current problem is the word "Update" was added to label at some point. I bet that was an improvement of Accessibility. And that can be "Update" and "Add" in the future code changes. Or a checked box changed to "Checked Label" rather than "Update Label"..etc. At that point, it has potential to bring the bug back.

To prevent this happen again, we must check the word "Update" that match the rows at the beginning instead.

jQuery('.filterable-option').length === jQuery('.filterable-option:contains("Update")').length

It would be great to find for test a way to set unique words in .title and .description tags and testing search by them.

Ok, so that sounds like we should make a custom global field handler in a test module, give that a distinct title and description and test for that.

Thanks @vaplas.

Instead of rebuilding a search system in the test case, another little tweak we could do:

1. Additional to above test.
2. Use JS (or any simple way) to change a label in Row A to "UniqueSearchWordWithoutSpaces"
3. Use JS to change a .title in Row B to "UniqueSearchWordWithoutSpaces"
4. Perform a search of "UniqueSearchWordWithoutSpaces"
5. Check if the result is Row B and only 1 result

I looked at the JavaScript and it is hardcoded to look at the .title and .description tags. Because it is hardcoded a test should cover it, we want to cover each line of code in a test.
If the HTML is changed without updating the JavaScript this test will catch that and indicate the JavaScript (and this test) will also need an update.

So actually to make it complete, we need to inspect the HTML as part of the test and search on both title and description seperately. To make it complete we should use a custom global field to have better control over the text we want to search for.

Working on something, will finish it tomorrow and post it then.

Ok, new version.

I added a test module that adds two fields with a unique and searchable title and description.

The title and description are tested seperately now. I think the test now covers everything in JavaScript and I don't think any deeper inspection makes the test better. If the HTML is changed without the JavaScript this test will fail thus catching the broken functionality.

+++ b/core/modules/views_ui/tests/modules/views_ui_test_field/views_ui_test_field.views.inc
@@ -0,0 +1,29 @@
+      'id' => 'views_test_field_1',
...
+      'id' => 'views_test_field_2',

Ha! Sneaky, providing a title and description without an actual working plugin. I like!

1. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
   @@ -0,0 +1,134 @@
   +      'administer views',
   

   Only this permission is needed. The other three can go.

2. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
   @@ -0,0 +1,134 @@
   +    // Assert the count is equal before filtering.
   

   'Assert all rows are visible before filtering'. Otherwise we need to specify what 'the count' is equal to.

This gives excellent base coverage. We are now missing the 'date' search though. I would expect an additional test where we search for 'date' and find neither test fields (so something that would fail without the fix).

Updated test:

- Permissions fixed
- Reintroduced the label filter test using the form alter @vaplas suggested, thanks for that!
- Cleaned up double assertions and comments. @vaplas: The waitFor() doesn't fail if the assertion never passes, a recheck after the waitFor is required because of that.

Important warning though, I'm experiencing random failures on this test, the same as in #2832672. We need to fix this before we can commit this patch.. I'll postpone this one once it fails/passes.

Nice cleanup! You're completely right about the overcomplicated way of testing now that we have 2 fields to check on..

Commented line, whoops.. Sorry about that.

Still we have to wait with RTBC untill the related issue is fixed I think..

It's the right time to go I think.

Ignore my note on the related bug and no-commit: The random fail only occurs if you update the PhantomJS Driver. It's safe to commit this, without the driver upgrade I had no random fails..

Updated issue title and added small generic comment on the description to indicate the fix/test is not aimed at fixing the date search specifically but at searching the correct element.

Committed and pushed f974370 to 8.4.x and c78d01c to 8.3.x. Thanks!

diff --git a/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
index a2ddbda..32b1a40 100644
--- a/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterOptionsTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\views_ui\FunctionalJavascript;
 
-use Behat\Mink\Element\NodeElement;
 use Drupal\FunctionalJavascriptTests\JavascriptTestBase;
 
 /**

Fixed unused on commit.