Problem/Motivation

The "place block" dialog offers a text input to filter by block name. When using this list of blocks changes dynamically. The change is easily noted visually, but is not conveyed to users with screen readers.

Proposed resolution

Use Drupal.announce() to convey a short message about the number of blocks now present in the filtered list.

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

We already do this with the module filter on the admin/modules page - see system.modules.js.

Remaining tasks

  • DONE - Devise the announcement string(s): "6 blocks are available in the modified list."
  • DONE - Update block.admin.js to use Drupal.announce(), handling singular/plural results.
  • DONE - FunctionalJavascript tests. Confirm the updated content of div#drupal-live-announce with JavascriptTestBase, for singular and plural block counts.

User interface changes

  • No visible changes intended.
  • Add a screen reader announcement to convey updates which are currently only apparent visually.
  • Introduces a new translatable string for Drupal.announce().

API changes

None expected.

Data model changes

None expected.

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

Issue summary: View changes
andrewmacpherson’s picture

Issue summary: View changes
Issue tags: +Dublin2016, +Novice

This would be a good novice task for a mentored sprint at Dublin DrupalCon 2016.

It basically involves a copy/paste, adding a JS behaviour to block module.

We have an accessibility table in the general sprints room but I can come to the mentored room if you need me.

Details of the Drupal.announce() are here:
New accessibility feature: Drupal.announce() (change record)
Accessibility tools for JavaScript in Drupal 8 (handbook page)

Ehud’s picture

Assigned: Unassigned » Ehud

Matthew Lambert and me arecurrently working on that. On Dublin2016

Ehud’s picture

Assigned: Ehud » Unassigned
Status: Active » Needs review
FileSize
691 bytes

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

SteffenR’s picture

Assigned: Unassigned » SteffenR
andrewmacpherson’s picture

Thanks @SteffanR - I did a quick review.

We should use Drupal.formatPlural() instead of Drupal.t() because we may have a single result.

andrewmacpherson’s picture

Status: Needs review » Needs work
SteffenR’s picture

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

droplet’s picture

+++ 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('...')`

lomasr’s picture

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

arunkumark’s picture

I have re-rolled patch based on comment #7

arunkumark’s picture

Status: Needs work » Needs review
andrewmacpherson’s picture

re: #9 - The multiple messages come from calling drupal.announce() too frequently. I looked at how the module filter does it, and it wraps the filtering function call in debounce():

        $input.on({
          keyup: debounce(filterModuleList, 200),

I've done the same thing here when we call filterBlockList and it seems to fix the problem.

re: #10 - I've changed it to $table.find('tr:visible').length - 1

re: #12 - Thanks for the patch. The arguments expected by Drupal.formatPlural() are different to Drupal.t(). I've updated it here (count, singular-message, plural-message).

andrewmacpherson’s picture

Issue summary: View changes
Issue tags: +JavaScriptTest, +Needs tests

This could be tested with JavascriptTestBase. We would want to confirm that the message is correct for single/plural situations. Something like this:

  • Load the block layout page, press a place-block button
  • Focus the block name filter field.
  • Type "recent", wait 100 ms
  • Inspect DOM, find out how many modules are displayed (jQuery) - expecting plural
  • Confirm Drupal.announce() message appears in DOM with correct count.

Repeat for a block name where only one result is expected (e.g. "powered")

A similar test has been written for the module filter in #2715663: Use Drupal.formatPlural for when announcing module-filter results for screenreader users so that can be a starting point.

michielnugter’s picture

Status: Needs review » Needs work
michielnugter’s picture

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

andrewmacpherson’s picture

@michielnugter - Thanks for writing tests! They run successfully on my machine too.

Review of the patch:

+namespace Drupal\Tests\system\FunctionalJavascript;

This namespace should be Drupal\Tests\block\FunctionalJavascript, since we are in the block module.

+class BlockScreenReaderFilterTest extends JavascriptTestBase {

I think I would call it BlockFilterTest as it asserts the basic filter behaviour (i.e. the counts), as well as the new Drupal.announce() message.

michielnugter’s picture

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..

michielnugter’s picture

Status: Needs work » Needs review
michielnugter’s picture

Well that fixed it!

andrewmacpherson’s picture

@michielnugter - thanks for moving this along.

Interdiff seems to generate garbage, sorry about that..

Interdiff isn't so useful in where a file is being renamed, not a problem.

I updated a few things here:

  • Tidying up whitespace.
  • Improve a few comments.
  • Changed the 1-result test string form "Drupal" to "Powered by". Just being cautious, as we could feaibily end up with another block containing the word "Drupal" one day.
  • Otherwise, no changes to the logic of the tests.

The JS condition on wait() bothers me a little bit:

    // 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));

The assertion here is testing that the number of visible rows has changed, rather than the screenreader announcement. So it feels a bit awkward waiting for the live announce region to change, when that's not being tested. I'm not sure what else to suggest, apart from just using wait(1000). What do you think?

michielnugter’s picture

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.

andrewmacpherson’s picture

I'm going to tentatively RTBC this one. (I realize RTBC-ing your own patch isn't quite standard, but it's been passed back and forth between michielnugter and myself, and his last comment was favourable.)

We have a number of similar issue where we are adding Drupal.announce() messages so I'd like to get feedback on the testing approach used. If it's good then we can move the other issues along more quickly.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

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.

michielnugter’s picture

Issue summary: View changes

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.

droplet’s picture

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)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.