
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.
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff_49-54.txt | 1.13 KB | ravi.shankar |
#54 | 2715663-54.patch | 4.34 KB | ravi.shankar |
#49 | interdiff_48-49.txt | 1.16 KB | ravi.shankar |
#49 | 2715663-49.patch | 4.08 KB | ravi.shankar |
#48 | interdiff_45-48.txt | 1.5 KB | ravi.shankar |
Comments
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #3
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedFirst 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 ?
Comment #5
scuba_flyApplied #3 ( 2715663-2.patch )on 8.2.x-dev and can confirm it is now saying "1 module is available in the modified list."
Comment #6
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks @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:
Feel free to take a crack at writing a test - it's something I hope to get round to later this weekend.
Comment #7
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedOMG! 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: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.
Comment #8
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #9
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented#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().
Comment #10
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI 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.
Comment #11
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks @michielnugter - these improvements make sense to me.
Comment #14
emcoward CreditAttribution: emcoward at Investis Digital commentedHi, I tried to apply this patch to 8.4.x, but it no longer applies. Going to attempt a re-roll.
Comment #15
opdaviesI'm mentoring Em with this at DrupalCon Vienna.
Comment #16
emcoward CreditAttribution: emcoward at Investis Digital commentedRe-rolled the patch and tested with Voiceover.
Comment #17
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks 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.
Comment #18
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedManual review is good, patch in #16 does what this patch expects: "2 modules", "1 module", or "0 modules".
Comment #19
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedA 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:
Comment #20
BarisW CreditAttribution: BarisW as a volunteer and at ronder commentedOne 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.
Comment #21
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI'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.
Comment #22
waako CreditAttribution: waako at Annertech commentedUpdated patch:
$assertSession->statusCodeEquals(200);
from test file.system.modules.js
changes tosystem.modules.es6.js
and regeneratedsystem.modules.js
from that.Comment #23
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedthanks Tom.
changing the status to wake the test bots
Comment #26
waako CreditAttribution: waako at Annertech commentedUpdating
system.module.es6.js
so it applies to 8.9.xUpdating test file to replace
JavascriptTestBase
withWebDriverTestBase
, also add missing$module
and documentationComment #27
waako CreditAttribution: waako at Annertech commentedUpdated with some coding standard fixes to ModuleFilterTest.php
Comment #31
longwaveFound 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.
Comment #32
ankithashettyRerolled the patch in #27, thanks!
Comment #33
longwaveThanks! There are some spelling and JS style issues picked up by the bot, see https://www.drupal.org/pift-ci-job/2207441 for details.
Comment #34
ankithashettyFixed custom command failure errors in #32, thanks!
Comment #35
longwaveThanks again - one more trailing comma to add then this is good to go, I think.
Comment #36
yogeshmpawarThis will fix the custom commands failure & added an interdiff.
Comment #38
yogeshmpawarHope this updated patch will resolve test failures. Added an interdiff as well.
Changing
public static $modules
toprotected static $modules
as per this change record -$modules property on BrowserTestBase and KernelTestBase is protected
- https://www.drupal.org/node/2833984Comment #40
karishmaamin CreditAttribution: karishmaamin at Specbee commentedTried to fix build fail in #38. Attached interdiff file
Comment #44
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis 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.
Comment #45
longwaveRerolled.
Comment #46
smustgrave CreditAttribution: smustgrave at Mobomo commentedTest 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.
Comment #47
alexpottI think this is more meaningful if
self::assertGreaterThan(count($visible_rows), count($module_rows));
I think we should do
$this->assertGreaterThan(1, count($visible_rows));
just so this test fails in the right place if this is 0.filterVisibleElements(array $elements): array {
Comment #48
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedTried to address comment #47, please review.
Comment #49
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAddressed PHPCS issue of patch #48.
Comment #50
smustgrave CreditAttribution: smustgrave at Mobomo commentedConfirmed points #47 were addressed.
Retested test file without the fix and failed
With the fix it's all green as we can see.
Comment #52
smustgrave CreditAttribution: smustgrave at Mobomo commentedRandom ckeditor error
Comment #53
alexpottThis should not have been removed.
Mixing
$this->assert
andself::assert
is odd... let's change$this->
toself::
to be consistent with everything else in this file.Comment #54
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedMade changes as per comment #53, please review.
Comment #55
smustgrave CreditAttribution: smustgrave at Mobomo commentedappears the changes from 53 have been made.
Comment #56
lauriiiCommitted 78a7b47 and pushed to 10.1.x. Thanks!
Not backporting this because of the string change.
Comment #57
xjm