Add a search field to the test overview page

Yes, we intentionally designed the JS to make it re-usable later on. I think we want to move the behavior into a new JS file; e.g., /misc/tablefilter.js

In http://drupal.org/project/admin_ux (which I'm using persistently), I've replaced the entire test label with the class name. Since the d.o testbot does not output the human-readable labels anymore, it only makes sense to do that in the local UI, too.

Closely related: #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

:)

It works, but it's doing more than it has to on the tests page; the tests page is simpler. So the JS can be even simpler than it already is.

To generalize this, we'd have to make things too abstract to make all aspects configurable I think.

OTOH, I think the JS on this page should do automatic uncollapsing of collapsed rows, if a collapsed row matches?

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,61 @@ Drupal.behaviors.simpleTestSelectAll = {
+    var $rowsAndDetails, $rows, $details;
+
+    function hidePackageDetails(index, element) {
+      var $details = $(element);
+      var $visibleRows = $details.find('table:not(.sticky-header)').find('tbody tr:visible');
+      $details.toggle($visibleRows.length > 0);
+    }

Unnecessary for tests filtering.

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,61 @@ Drupal.behaviors.simpleTestSelectAll = {
+        // Hide the package <details> if they don't have any visible rows.
+        // Note that we first show() all <details> to be able to use ':visible'.
+        $details.show().each(hidePackageDetails);

Unnecessary for tests filtering.

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,61 @@ Drupal.behaviors.simpleTestSelectAll = {
+      $rowsAndDetails = $table.find('tr, details');

Unnecessary for tests filtering.

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,61 @@ Drupal.behaviors.simpleTestSelectAll = {
+      $details = $table.find('details');

Unnecessary for tests filtering.

related #1919558: add the test class to make it easier to match with the bot

My laptop is pretty quick, so I didn't notice any real slowness. That said, this is an identical approach to the module page, so it should be fixed for both if there is a huge problem.

The only visual bug is if your search doesn't match anything, the modules page hides everything but this still shows the table header, which looks very strange.

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,62 @@ Drupal.behaviors.simpleTestSelectAll = {
   }
 };
 
+
+/**

+++ b/core/modules/simpletest/simpletest.pages.incundefined
@@ -9,6 +9,28 @@
 function simpletest_test_form($form) {
+
+
+  // JS-only table filters.

Extra blank lines

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,62 @@ Drupal.behaviors.simpleTestSelectAll = {
+    var searching = false;
...
+        searching = true;
...
+      else if (searching) {

This seems like a performance improvement that could be mimicked in the module filtering.

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,62 @@ Drupal.behaviors.simpleTestSelectAll = {
+        var $sources = $row.find('.table-filter-text-source');
+        var textMatch = $sources.text().toLowerCase().indexOf(query) !== -1;

I would combine these two lines, personally.

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,62 @@ Drupal.behaviors.simpleTestSelectAll = {
+      function hideTestRow (index, row) {
+        var $row = $(row);
+        var $sources = $row.find('.table-filter-text-source');
+        var textMatch = $sources.text().toLowerCase().indexOf(query) !== -1;
+        $row.closest('tr').toggle(textMatch);

This is unused, and looks the same as showTestRow

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,62 @@ Drupal.behaviors.simpleTestSelectAll = {
+      // Filter if the length of the query is at least 4 characters.
+      if (query.length >= 4) {

I can see why 2 would be too slow, but just testing it, I found having 3 characters do nothing be frustrating. Can we try bumping it down?

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,62 @@ Drupal.behaviors.simpleTestSelectAll = {
+      // @todo Use autofocus attribute when possible.

Is that something happening in D8? Is there an issue?

+++ b/core/modules/simpletest/simpletest.pages.incundefined
@@ -9,6 +9,28 @@
+      'title' => t('Enter a part of the test name or description to filter by.'),

Should this mention the 4 character limitation?

+++ b/core/modules/simpletest/simpletest.pages.incundefined
@@ -149,11 +171,11 @@ function theme_simpletest_test_table($variables) {
+        'data' => '<div class="description">' . t('@description (@class)', array('@description' => $description, '@class' => $test_name)) . '</div>',

This should be format_string, I'm pretty sure.

+++ b/core/modules/simpletest/simpletest.jsundefined
@@ -99,4 +99,53 @@ Drupal.behaviors.simpleTestSelectAll = {
+        // @todo: This does not respect the current state of the group toggle.

no : after the todo
See
http://drupal.org/node/1354#todo
Also, to be consistant with the todo just a few lines after.

That of course is a tiny, nit, so patch attached for it.

But while I was reading it, the wording of the todo seemed strange. It was saying what the code did not do, instead of what the todo was todo. So I reworded the comment with what it sounded like the todo was hoping would be accomplished.

I also tested manually.
Still working great.
Search starts after 3 chars.
There is a tooltip type of popup with the instructions that say to enter at least 3 chars.

Here is what it looks like when there are no matches, as @tim.plunkett mentioned in #6

@tim.plunkett asked about the autofocus todo comment.
Perhaps this is the info looking for. It's a won't fix and but also mentions just using #attributes. I'm not sure what that means for this though.
#1174936-13: Allow FAPI usage of the autofocus attribute

aside form #9 and #10

all the comments from @tim.plunkett have been addressed.

Here are the two follow-ups for the remainder from #7

#1925492: Small optimization for search on modules page

#1925498: Convert t() to format_string that added class to the description, when search added to test list (this one is postponed on this issue)

This seems like a performance improvement that could be mimicked in the module filtering.

This is in fact a mutex, and it should be documented as such. It should be documented how this helps improve performance, because at first glance it looks very odd.

Why is tests in a fieldset?

#8: drupal-Add_a_search_field_to_the_test_overview_page-1919470-8.patch queued for re-testing.

tagging for reroll http://drupal.org/patch/reroll

Rerolled to fix a minor conflict with #1901670: Start using PHPUnit for unit tests.

Missed the tag.

@Bojhan/#14 - that sounds like a separate issue to me, this patch doesn't add or change fieldsets.

tag

#18: 1919470-18.patch queued for re-testing.

This is totally freaking awesome. I tested with and without overlay and it works great. I can't provide a JS code review though--@nod_, @tim.plunkett?

If it looks good codewise I'd just as soon get this in RIGHT NOW so that we all can start using it RIGHT NOW, and then handle the following in followups:

• Not displaying the table header when it's empty.
• Talk about the silly fieldset around the tests. (@Bojhan, as far as I can tell, it's there for the sole purpose of collapsing it so you can get to the "Clean environment" button.)
• Making the test class name and description not so tiny. (It's seriously hard to read.)
• Maybe the search should match the test groups as well? My search for "views plugin" above does not match the 20 items in the Views Plugins test group.

Despite that it's after feature freeze, I think it's fine to put this in by itself and add followups, since this is a developer-facing feature and a very non-intrusive patch.

Things we should do before commit:

• Accessibility testing (see http://drupal.org/core-gates#accessibility).
• Deterimine if there's anything remaining we need to do from the comments in #12 and #13. @Berdir, @Wim Leers?

Also, I'm marking this as a task, because it is going to save every single D8 developer time.

Js is good, based off the module filter code so nothing to worry about.

For those raising the point that we should be using the same JS in the module and simpletest filter, well, we need the same markup first.

Alright then. Thanks @nod_! Let's then do the accessibility testing and file some followups.

Updated issue summary, embedded screenshots in issue summary

adding remaining tasks section

Here is a easy url to use for accessibility testing: http://simplytest.me/project/drupal/8.x?patch[]=http://drupal.org/files/1919470-18.patch

making url work

noting follow-ups that were already made

@xjm That's fine, I guess if you need you can always move the button towards the top.

noting #23 has the list of follow-ups to open

Simplytest.me is having trouble with #1969680: Installation fails with MyISAM - key too long

So, this Contributor Task doc, for manual accessibility testing might help in the mean time: http://drupal.org/node/1853532

#18: 1919470-18.patch queued for re-testing.

#18: 1919470-18.patch queued for re-testing.

#18: 1919470-18.patch queued for re-testing.

Needs a reroll.

Adding a rerolled patch for this.

Regarding the autofocus @todo i don't see a reason to add this because we are executing JS anyway, so i think it's an unecessary overhead to do a feature testing for support of this attribute and then adding it conditionally instead of just using .focus(). Or what would be the reason to do so?

I test the page with the keyboard shortly, and everything is accessible that way. The rest of the accessibility testing seems unnecessary because this page is working in the same way as the module list page.

Thanks @s_leu!

Just wanted to note that #35 is not a straight reroll, here is an interdiff generated by patchutils. The two main changes I saw were that format_string() is used instead of t(), and this:

diff -u b/core/modules/simpletest/simpletest.js b/core/modules/simpletest/simpletest.js
--- b/core/modules/simpletest/simpletest.js
+++ b/core/modules/simpletest/simpletest.js
@@ -135,14 +135,13 @@
         // @todo Make this respect the current state of the group toggle.
         searching = false;
         $rows.hide();
-        $rows.filter('.simpletest-group').show();
+        $rows.filter('.simpletest-group').show().find('div.simpletest-image')
+          .html(settings.simpleTest.images[0]);
       }
     }

Also check the indentation within \Drupal\simpletest\Form\SimpletestTestForm::buildForm(), it's a bit off now.

@s_leu - Next time you reroll and want to make additional changes, I recommend posting two separate patches if possible (first just a direct reroll with no changes) with an interdiff :)

Ok to clean that up, here's a straight reroll, a patch including the improvements and an interdiff containing only the improvements after the straight reroll

Perfect, thanks @s_leu :)

Added another change that takes care about the last remaining todo in the JS, it adds code to respect the opened test actions after the search string gets deleted again.

I did a quick read through and standards wise looks ok to me. (for example the whitespace indent is fixed)

JS needs a little bit of work.

$(this).show(), don't need to put that in the .each(), it can be just before the .each() and will work just as well.
it's drupalSettings now, not Drupal.settings.
not .parents() but .closest(). and the .children()[0] is pretty ugly, no better way to select that?

Added nod_'s suggested changes.

@xjm (or other)
this is tagged with needs followup. Does it need follow-ups that are not already created and listed in the issue summary?
If follow-ups are already created and linked, I think we can remove the needs followup tag.

Ah, I thought the things in #23 are addressed with follow-up issues. Here are the follow-ups.

• #2021065: hide the table header when empty search on the test overview page
• #2021067: remove the field set for tests on the test overview page
• #2021071: too tiny text for description and class on the test overview page
• #2021077: make search also match the test groups on the test overview page

Next: accessibility review
https://drupal.org/contributor-tasks/accessibility-review

added conttributor task doc

Thanks for the reroll s_leu, a few small details if you don't mind.

- I'd prefer we use the global drupalSettings variable instead of settings (the settings parameter will be removed from the behaviors for attach and detached functions).
- can we have .trigger('focus'); instead of .focus() ? there is an issue out there to remove all shorthand version of those things with proper .trigger and .on

Other than that, js is RTBC as far as I'm concerned. Thanks :)

updating remaining tasks

As requested in #52 by @nod_

Firefox was choking on keyup because of all the DOM searching so I added a little timeout delay of 200ms to allow the search a breathing period to cancel the filter function call.

Firefox was choking on keyup because of all the DOM searching so I added a little timeout delay of 200ms to allow the search a breathing period to cancel the filter function call.

Really? I can't reproduce that. But given that Berdir has also indicated performance can be a problem on this page, let's keep this, but instead of using custom code, let's use Drupal.debounce, which exists precisely for this purpose.

In this reroll:

• Fixed a bunch of comments.
• Replaced the custom code in #54 with Drupal.debounce.
• Renamed the searching variable to searched, which is what it really is about. It's *not* a mutex as I said in #12. It's now also documented to prevent future confusion.
• The "select all" checkbox was still visible when the table was filtered, and clicking it would still select ALL the tests, not just the filtered ones. This is very misleading. So, whenever filtering has been applied, the "select all" checkbox is now hidden.

Thanks, that's what debounce is for indeed. Changes are all good on my side.

nice!

the no select all checkbox when filtered,
is consistant with how the search works on the modules (Extend) page too.

@Wim Leers thanks, didn't know of debounce:) I have a couple other things that could make this preform better but that can go in a follow up. RTBC:)

Committed to 8.x. Thanks.

Thank you thank you! Yay!

oh yay!