Problem/Motivation

The module-filter search field on the admin/modules page announces the number of results to screen reader users. However it doesn't handle singular/plural correctly, so when only ONE module is available, it announces "1 modules are available...".

To replicate: install the Standard profile, then search for "tel" while a screen reader is running. The only match is the telephone field module.

Proposed resolution

Use Drupal.formatPlural() for the message which is passed to Drupal.announce() in system.modules.js

Remaining tasks

  • Patch system.modules.js - DONE.
  • FunctionalJavascript tests for module filter behaviour, asserting correct drupal.announce() messages - DONE.

User interface changes

No visible changes.
Improves the announcement made to a screenreader user when there is only one result in the filtered module list.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#54 interdiff_49-54.txt1.13 KBravi.shankar
#54 2715663-54.patch4.34 KBravi.shankar
#49 interdiff_48-49.txt1.16 KBravi.shankar
#49 2715663-49.patch4.08 KBravi.shankar
#48 interdiff_45-48.txt1.5 KBravi.shankar
#48 2715663-48.patch4.05 KBravi.shankar
#45 2715663-45.patch4.24 KBlongwave
#40 interdiff_2715663_38-40.txt503 byteskarishmaamin
#40 2715663-40.patch5.2 KBkarishmaamin
#38 interdiff-2715663-36-38.txt657 bytesyogeshmpawar
#38 2715663-38.patch5.21 KByogeshmpawar
#36 Screen Shot 2021-10-18 at 2.43.06 PM.png308.95 KByogeshmpawar
#36 interdiff-2715663-34-36.txt621 bytesyogeshmpawar
#36 2715663-36.patch5.13 KByogeshmpawar
#34 interdiff_2715663_32-34.txt1.24 KBankithashetty
#34 2715663-34.patch5.13 KBankithashetty
#32 reroll_diff_2715663_27-32.txt1.36 KBankithashetty
#32 2715663-32.patch5.13 KBankithashetty
#27 interdiff.2715663.26-27.txt936 byteswaako
#27 2715663-27.drupal.Use-DrupalformatPlural-for-when-announcing-modulefilter-results-for-screenreader-users.patch4.94 KBwaako
#26 2715663-26.drupal.Use-DrupalformatPlural-for-when-announcing-modulefilter-results-for-screenreader-users.patch4.85 KBwaako
#22 use_drupal_formatplural-2715663-22.patch4.62 KBwaako
#17 interdiff-2715663-10-16.txt1.38 KBandrewmacpherson
#16 use_drupal_formatplural-2715663-16.patch4.01 KBemcoward
#10 use_drupal_formatplural-2715663-10.patch3.9 KBmichielnugter
#10 interdiff-9-10.txt2.77 KBmichielnugter
#9 interdiff-2715663-7-9.txt784 bytesandrewmacpherson
#9 use_drupal_formatplural-2715663-9.patch3.68 KBandrewmacpherson
#7 use_drupal_formatplural-2715663-7.patch3.89 KBandrewmacpherson
#3 2715663-2.patch774 bytesandrewmacpherson

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

Title: Use Drupal.formatPlural for when announcing » Use Drupal.formatPlural for when announcing module-filter results for screenreader users
andrewmacpherson’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new774 bytes

First patch. This fixes the announcements in system.modules.js.

I suppose we can write tests for this with the new JS test framework in 8.1.x ?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

scuba_fly’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Dublin2016

Applied #3 ( 2715663-2.patch )on 8.2.x-dev and can confirm it is now saying "1 module is available in the modified list."

andrewmacpherson’s picture

Issue tags: +Needs tests

Thanks @scuba_fly!

I think we might be able to write a Functional Javascript test for Drupal.announce() uses like this.

In pseudo-code this would be:

  • Load admin/modules page
  • Focus module filter field.
  • Type "field", wait 100 ms
  • Inspect DOM, find out how many modules are displayed (jQuery)
  • Confirm Drupal.announce() message appears in DOM with correct count.

Feel free to take a crack at writing a test - it's something I hope to get round to later this weekend.

andrewmacpherson’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests +JavaScriptTest
StatusFileSize
new3.89 KB

OMG! I think is the first Functional Javascript test we have for a screen reader feature!

core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.php tests the following:

  • The filter reduces the number of visible modules
  • The Drupal.announce() message div contains a correct plural message when multiple modules are visible
  • The Drupal.announce() message div contains a correct plural message when one module is visible
  • The Drupal.announce() message div contains a correct plural message when zero modules are visible

For instructions for running Functional Javascript tests locally, see Javascript Testing Comes to Drupal 8

Note that this test enables the toolbar module (and permission). We have an RTBC patch over at #2702969: Module filter does not announce number of results for screenreader when Toolbar module is disabled to fix a missing JS library dependency declaration. After that is fixed, we can should update this test.

andrewmacpherson’s picture

andrewmacpherson’s picture

Issue summary: View changes
StatusFileSize
new3.68 KB
new784 bytes

#2702969: Module filter does not announce number of results for screenreader when Toolbar module is disabled is fixed, so the test no longer needs to enable toolbar module to get drupal.announce().

michielnugter’s picture

StatusFileSize
new2.77 KB
new3.9 KB

I updated the patch a little:

- Assigned getSession and getAssertSession() to variables so they don't have to be looked up each time
- Added asserts for the number of results.
- Added a condition for the wait() statements to improve tests performance.

andrewmacpherson’s picture

Thanks @michielnugter - these improvements make sense to me.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

emcoward’s picture

Status: Needs review » Needs work
Issue tags: +Vienna2017

Hi, I tried to apply this patch to 8.4.x, but it no longer applies. Going to attempt a re-roll.

opdavies’s picture

I'm mentoring Em with this at DrupalCon Vienna.

emcoward’s picture

Status: Needs work » Needs review
StatusFileSize
new4.01 KB

Re-rolled the patch and tested with Voiceover.

andrewmacpherson’s picture

StatusFileSize
new1.38 KB

Thanks for working on this @emcoward!

I've created an interdiff, which helps other contributors understand how a patch is evolving.

Doing a manual review next.

andrewmacpherson’s picture

Manual review is good, patch in #16 does what this patch expects: "2 modules", "1 module", or "0 modules".

andrewmacpherson’s picture

A useful next step would be to look at the test, and check how closely it follows the test we committed in #2805205-60: Provide screen-reader feedback when filtering by block name.. That issue was the first of these Drupal.announce() FunctionalJavascript tests to be committed, and it makes sense to keep them all consistent.

For instance, I think it was decided that we don't need:

+    $assertSession->statusCodeEquals(200);
BarisW’s picture

Status: Needs review » Needs work

One more thing; we don't update js files anymore directly.

Can you create a new patch with the changes applied to the es6.js file and then regenerate the js file? See issue 2815083: Drupal core now using ES6 for javascript development.

andrewmacpherson’s picture

I'm bringing these filter-list announcement issues under the plan for WCAG 2.1

This relates to the new "Change of Content" success criterion. Under WCAG 2.0 the announcement was a nice-to-have, but under WCAG 2.1 it becomes a must-have. For a longer explanation, see #2864791-33: Implement new Success Criteria from WCAG 2.1.

waako’s picture

StatusFileSize
new4.62 KB

Updated patch:

  • Removed $assertSession->statusCodeEquals(200); from test file.
  • Applied system.modules.js changes to system.modules.es6.js and regenerated system.modules.js from that.
andrewmacpherson’s picture

Status: Needs work » Needs review

thanks Tom.

changing the status to wake the test bots

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

waako’s picture

Updating system.module.es6.js so it applies to 8.9.x
Updating test file to replace JavascriptTestBase with WebDriverTestBase, also add missing $module and documentation

waako’s picture

Updated with some coding standard fixes to ModuleFilterTest.php

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative, +Needs reroll

Found via the Bug Smash Initiative random issue button.

This looks like a straightforward fix and should be ready to go in, just needs a reroll.

ankithashetty’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new5.13 KB
new1.36 KB

Rerolled the patch in #27, thanks!

longwave’s picture

Status: Needs review » Needs work

Thanks! There are some spelling and JS style issues picked up by the bot, see https://www.drupal.org/pift-ci-job/2207441 for details.

ankithashetty’s picture

Status: Needs work » Needs review
StatusFileSize
new5.13 KB
new1.24 KB

Fixed custom command failure errors in #32, thanks!

longwave’s picture

Status: Needs review » Needs work

Thanks again - one more trailing comma to add then this is good to go, I think.

yogeshmpawar’s picture

Status: Needs work » Needs review
StatusFileSize
new5.13 KB
new621 bytes
new308.95 KB

This will fix the custom commands failure & added an interdiff.

Status: Needs review » Needs work

The last submitted patch, 36: 2715663-36.patch, failed testing. View results

yogeshmpawar’s picture

Status: Needs work » Needs review
StatusFileSize
new5.21 KB
new657 bytes

Hope this updated patch will resolve test failures. Added an interdiff as well.

Changing public static $modules to protected static $modules as per this change record - $modules property on BrowserTestBase and KernelTestBase is protected - https://www.drupal.org/node/2833984

Status: Needs review » Needs work

The last submitted patch, 38: 2715663-38.patch, failed testing. View results

karishmaamin’s picture

Status: Needs work » Needs review
StatusFileSize
new5.2 KB
new503 bytes

Tried to fix build fail in #38. Attached interdiff file

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new4.24 KB

Rerolled.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Test this out on D10.1

When 1 option confirmed it's using singular module language.
Test fails without the fix. Passes with it.

Good to mark this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. This issue is looking good - just a few minor nits
  2. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.php
    @@ -0,0 +1,96 @@
    +    self::assertNotEquals(count($module_rows), count($visible_rows));
    

    I think this is more meaningful if self::assertGreaterThan(count($visible_rows), count($module_rows));

  3. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.php
    @@ -0,0 +1,96 @@
    +    // Test Drupal.announce() message when multiple matches are expected.
    +    $expected_message = count($visible_rows) . ' modules are available in the modified list.';
    

    I think we should do $this->assertGreaterThan(1, count($visible_rows)); just so this test fails in the right place if this is 0.

  4. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.php
    @@ -0,0 +1,96 @@
    +  protected function filterVisibleElements($elements) {
    

    filterVisibleElements(array $elements): array {

ravi.shankar’s picture

Status: Needs work » Needs review
StatusFileSize
new4.05 KB
new1.5 KB

Tried to address comment #47, please review.

ravi.shankar’s picture

StatusFileSize
new4.08 KB
new1.16 KB

Addressed PHPCS issue of patch #48.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed points #47 were addressed.

Retested test file without the fix and failed

PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing /var/www/html/web/core/modules/system/tests/src/FunctionalJavascript

Behat\Mink\Exception\ElementTextException : The text "1 module is available in the modified list." was not found in the text of the element matching css "#drupal-live-announce".

With the fix it's all green as we can see.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: 2715663-49.patch, failed testing. View results

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

Random ckeditor error

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.php
    @@ -53,11 +53,8 @@ public function testModuleFilter() {
    -    // Test Drupal.announce() message when multiple matches are expected.
    -    $expected_message = count($visible_rows) . ' modules are available in the modified list.';
    -    $assertSession->elementTextContains('css', '#drupal-live-announce', $expected_message);
    

    This should not have been removed.

  2. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.php
    @@ -53,11 +53,8 @@ public function testModuleFilter() {
    +    self::assertGreaterThan(count($visible_rows), count($module_rows));
    +    $this->assertGreaterThan(1,  count($visible_rows));
    

    Mixing $this->assert and self::assert is odd... let's change $this-> to self:: to be consistent with everything else in this file.

ravi.shankar’s picture

StatusFileSize
new4.34 KB
new1.13 KB

Made changes as per comment #53, please review.

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

appears the changes from 53 have been made.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +String change

Committed 78a7b47 and pushed to 10.1.x. Thanks!

Not backporting this because of the string change.

xjm’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.