Provide screen-reader feedback when filtering by block name.

Matthew Lambert and me arecurrently working on that. On Dublin2016

Patch by Matthew Lambert (xiwar) and Ehud Ovadia (Ehud).

The patch is not working for me.
Sometimes i get the correct block count - in other cases i get more that one message in the drupal-live-announce div.
Looks like we should put the Drupal.announce function to another place in block.admin.js.

Attached you also find some screenshots showing the wrong behavior - the correct behavior can be seen on the modules page.

You can reproduce it by typing several titles in the block name field / remove letters / repeat and look at the drupal-live-announce div in the chrome dev tools. Looks like messages are appended via Drupal.announce.

SteffenR

+++ b/core/modules/block/js/block.admin.js
@@ -56,6 +56,12 @@
+                  {'!blocks': $('table.block-add-table tr:visible').length - 1}

let's don't hard-coded it. `$table.find('...')`

Applied the patch in #5. It worked cleanly. Please see the screenshot.

I have re-rolled patch based on comment #7

I added a test based on the mentioned issue (which I also updated).

Requeueing test, it runs fine on my machine with the latest 8.3.x, don't really understand the reported error..

If I run my test locally with run-tests.sh I also get it to fail.

I debugged some more, the tests fails on drupalLogin. This method visits the /user page which doesn't exist (I created a screenshot to check). I didn't get it to work yet but then again, no patch currently works for me this way. It does make sense to add the user as a dependency so the new patch does that (and fixes the above mentioned issues).

I hope it now does pass the test..

Interdiff seems to generate garbage, sorry about that..

Well that fixed it!

Good changes, cleans everything up a bit.

As for the wait condition, I agree it's a bit odd and tried to look for a better solution but there are no reliable css selectors to check with. Tough if the wait condition fails the test will just wait ~1000ms and then it will probably also fail. As it's only a wait condition and thus only used to shorten the wait time I think it's acceptable.

Not wild about self-rtbcing patches - that just makes maintainers feel bad - if we go forward with the patch then we break the process if we set it back to needs work for just that we delay the issue.

Anyhow there is a reason to set it back to needs work.

1. The issue summary needs updating to detail the full extent of the fix - ie. why debounce is added

2. As it's only a wait condition and thus only used to shorten the wait time I think it's acceptable.

   needs addressing from #27. I agree that 1 sec is excessive here because the JS does not need to load anything to do the filtering. How was the 1000ms reached?

3. +++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,90 @@
   +    // Test block filter reduces the number of visible rows.
   +    $filter->setValue('ad');
   +    $session->wait(1000, 'jQuery("#drupal-live-announce").html().indexOf("blocks are available") > -1');
   +    $visible_rows = $this->filterVisibleElements($block_rows);
   +    self::assertNotEquals(count($block_rows), count($visible_rows));
   +
   +    // Test Drupal.announce() message when multiple matches are expected.
   +    $expected_message = count($visible_rows) . ' blocks are available in the modified list.';
   +    $assertSession->elementTextContains('css', '#drupal-live-announce', $expected_message);
   

   This could potentially be the same as the Pan-Galactic Gargle Blaster test below ie. this will pass if the filter results in 0 blocks - we should assert that the count is greater than 1 too.

I updated the IS to include a statement on debounce.

The 1000 ms is a minimum I feel comfortable with when waiting for a timeout of 200 ms in Javascript. The actual times for 200 ms can vary greatly (I've seen double the amount once). In theory 500ms should do but I don't see an advantage of decreasing it, 99% of the times the wait will pass within 300ms and the test continues.

Testing with a count of greater than 0 wont work because that's always true for an unfiltered list, we have to check for it this way to make sure it has changed.

As for RTBC'ing, I'm reluctant to do this as this test uses assertWaitOnAjaxRequest() and in #2837676: Provide a better way to validate all javascript activity is completed there is a discussion on the proper way of waiting for this kind of stuff. To me, that issue is a blocker for all JTB tests getting in.

A debounce() is required to prevent too many updates and decrease the amount of announces to a minimum.

No way to just debounce announces?

1. +++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,90 @@
   +    $assertSession->statusCodeEquals(200);
   

   unnecessary

Can we drop a @todo to add a test to the HEADER (thead) row. (Ensure it doesn't hidden)

Re rolled.
Fixed 3 of #29. Didn't understand 2 of #29.
Fixed 1 of #31.

@alexpott can you get back to @pk188 about 2 of #29? You ask:
"I agree that 1 sec is excessive here because the JS does not need to load anything to do the filtering. How was the 1000ms reached?"

@michielnugter answered your question in #30 I think.

I don't have any experience with how long this should be.

I will do a code style review for the latest patch.

1. +++ b/core/modules/block/js/block.admin.js
   @@ -54,4 +61,4 @@
   \ No newline at end of file
   

   Error: no new line at end of file

2. +++ b/core/modules/block/js/block.admin.js
   @@ -54,4 +61,4 @@
   \ No newline at end of file
   

   Error: no new line at end of file

3. +++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,91 @@
   \ No newline at end of file
   

   Error: no new line at end of file

+++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
@@ -0,0 +1,91 @@
+   * @param array $elements

Missing parameter comments.

+++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
@@ -0,0 +1,91 @@
+    if(count($block_rows)>0){

Add space between closing parentheses and opening curly bracket.

Successful test for applying patch announce-feedback-filtering-block-name-2805205-33.patch.

unasigning alexpott, he's on a long vacation :)
Setting to needs work for the review in #37

Here are the modifications suggested for patch in #33 and the relevant inderdiff.

Forgot to include the original changed. Here's a new patch and interdiff.

Thanks Laurie! I'll have a look on the failing tests.

The test should be fixed by adding the use statement.
While on it, I changed a few things:

• self::assertEquals is changed to $this->assertEquals
• I made the changes in the es6.js files and compiled to .js files according to #2815083: Drupal core now using ES6 for JavaScript development
• I upped the wait from 1s to 10s. This is the default, according to lendude. This is the maximum wait time. If the second argument is true within these 10 seconds, the test continues anyway.
• I replaced assertWaitOnAjaxRequest() with waitForElement()

Couple of nits:

1. +++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,93 @@
   +  public static $modules = array('user', 'block');
   

   needs the new array notation

2. +++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,93 @@
   +    if(count($block_rows)>0) {
   

   need spaces around >

3. +++ b/core/modules/block/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,93 @@
   +    $session->wait(1000, 'jQuery("#drupal-live-announce").html().indexOf("block is available") > -1');
   ...
   +    $session->wait(1000, 'jQuery("#drupal-live-announce").html().indexOf("0 blocks are available") > -1');
   

   I think we need to bump these to 10000 too, it's the default wait time for all 'wait' things, and if it passes it doesn't prolong the test run time anyway, and why risk random fails for this.

Thanks for the nits. Here we go.

All feedback has been addressed, we have test coverage for the new feature plus some basic coverage for the block filter functionality which is a nice bonus. IS was updated in #30 and still looks up to date.

Any update?

Rerolled successfully. Again changing it to RTBC.

Updated the issue credit.

And finally a commit. Thank you everyone! This is a great accessibility improvement. Thank you also for adding test coverage for this.

Committed 847b214 and pushed to 8.4.x. Thanks!

Awesome work here! I didn't know about this issue — but very glad to see more things use Drupal.announce() :)

Cool, thanks. And now the same with #2805499: Provide screen reader feedback when Views List is filtered by name or description!