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 useDrupal.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
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #3
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThis 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)
Comment #4
Ehud CreditAttribution: Ehud commentedMatthew Lambert and me arecurrently working on that. On Dublin2016
Comment #5
Ehud CreditAttribution: Ehud commentedPatch by Matthew Lambert (xiwar) and Ehud Ovadia (Ehud).
Comment #6
SteffenRComment #7
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks @SteffanR - I did a quick review.
We should use Drupal.formatPlural() instead of Drupal.t() because we may have a single result.
Comment #8
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #9
SteffenRThe 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
Comment #10
droplet CreditAttribution: droplet commentedlet's don't hard-coded it. `$table.find('...')`
Comment #11
lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedApplied the patch in #5. It worked cleanly. Please see the screenshot.
Comment #12
arunkumarkI have re-rolled patch based on comment #7
Comment #13
arunkumarkComment #14
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedre: #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 indebounce()
: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 toDrupal.t()
. I've updated it here (count, singular-message, plural-message).Comment #15
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThis could be tested with JavascriptTestBase. We would want to confirm that the message is correct for single/plural situations. Something like this:
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.
Comment #16
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI added a test based on the mentioned issue (which I also updated).
Comment #18
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRequeueing test, it runs fine on my machine with the latest 8.3.x, don't really understand the reported error..
Comment #20
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@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 newDrupal.announce()
message.Comment #23
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedIf 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..
Comment #24
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #25
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedWell that fixed it!
Comment #26
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@michielnugter - thanks for moving this along.
Interdiff isn't so useful in where a file is being renamed, not a problem.
I updated a few things here:
The JS condition on
wait()
bothers me a little bit: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?Comment #27
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedGood 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.
Comment #28
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI'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.
Comment #29
alexpottNot 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.
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?
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.
Comment #30
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI 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.
Comment #31
droplet CreditAttribution: droplet commentedNo way to just debounce announces?
unnecessary
Can we drop a @todo to add a test to the HEADER (thead) row. (Ensure it doesn't hidden)
Comment #33
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRe rolled.
Fixed 3 of #29. Didn't understand 2 of #29.
Fixed 1 of #31.
Comment #34
mgifford@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.
Comment #35
rogierbom CreditAttribution: rogierbom at Synetic commentedComment #36
lauriekap CreditAttribution: lauriekap at LimoenGroen commentedI will do a code style review for the latest patch.
Comment #37
lauriekap CreditAttribution: lauriekap at LimoenGroen commentedError: no new line at end of file
Error: no new line at end of file
Error: no new line at end of file
Missing parameter comments.
Add space between closing parentheses and opening curly bracket.
Comment #38
lauriekap CreditAttribution: lauriekap at LimoenGroen commentedSuccessful test for applying patch announce-feedback-filtering-block-name-2805205-33.patch.
Comment #39
yoroy CreditAttribution: yoroy at Roy Scholten commentedunasigning alexpott, he's on a long vacation :)
Setting to needs work for the review in #37
Comment #40
lauriekap CreditAttribution: lauriekap at LimoenGroen commentedHere are the modifications suggested for patch in #33 and the relevant inderdiff.
Comment #41
lauriekap CreditAttribution: lauriekap at LimoenGroen commentedForgot to include the original changed. Here's a new patch and interdiff.
Comment #43
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedThanks Laurie! I'll have a look on the failing tests.
Comment #44
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedThe test should be fixed by adding the
use
statement.While on it, I changed a few things:
self::assertEquals
is changed to$this->assertEquals
assertWaitOnAjaxRequest()
withwaitForElement()
Comment #45
LendudeCouple of nits:
needs the new array notation
need spaces around >
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.
Comment #46
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedThanks for the nits. Here we go.
Comment #47
LendudeAll 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.
Comment #49
pk188 CreditAttribution: pk188 at OpenSense Labs commentedAny update?
Comment #50
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedQueued another test, to see if it needs a re-roll after #2880007: Auto-fix ESLint errors and warnings was committed today.
Comment #52
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThese fail to apply now
Comment #53
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedRe-rolled patch. #2880007: Auto-fix ESLint errors and warnings removed the
'use strict;'
parts form the JS files.Sorry, no interdiff file. I got errors when trying to make one, relating to the same chunk. I manually compared patches #46 and #53 prior to uploading it here.
Comment #54
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRerolled successfully. Again changing it to RTBC.
Comment #55
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedforgot to remove tag when I made the last patch
Comment #57
lauriiiUpdated the issue credit.
Comment #58
lauriiiComment #59
lauriiiComment #61
lauriiiAnd 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!
Comment #62
Wim LeersAwesome work here! I didn't know about this issue — but very glad to see more things use
Drupal.announce()
:)Comment #63
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedCool, thanks. And now the same with #2805499: Provide screen reader feedback when Views List is filtered by name or description!
Comment #65
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThis use of Drupal.announce() helps us to address one of the new WCAG 2.1 Success Criterion: 3.2.7 Change of Content. So I'm retrospectively folding this into the parent plan, to help us if we want to report on our WCAG 2.1 progress later on.
Longer explanation at #2864791-33: Implement new Success Criteria from WCAG 2.1.
Comment #66
xiwar CreditAttribution: xiwar at manifesto commented