Problem/Motivation
Many forms in Views UI have long lists of options which can be filtered by keyword or category (e.g. Add sort criteria dialog). When using these filter controls, the contents of the list change dynamically. The changes are easily noted visually, but are not conveyed to users with screen readers.
Proposed resolution
Use Drupal.announce()
to convey a short message about the number of options now present in the filtered list.
We already do this with the module filter on the admin/modules
page - see system.modules.js
.
Remaining tasks
Devise the announcement string(s): "6 options are available in the modified list."Write a patch.Tests - can we confirm the updated content ofdiv#drupal-live-announce
with JavascriptTestBase?Review changes to javascript to reflect comments by @cottser in #45See #80.
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.
Comment | File | Size | Author |
---|---|---|---|
#80 | 2805197-80.patch | 3.39 KB | Lendude |
|
Comments
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #3
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #4
mgiffordThis is the exact use case why Drupal.announce() was built. We just need to use it more consistently.
Comment #5
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 views 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 #6
Ehud CreditAttribution: Ehud commentedPatch by Matthew Lambert (xiwar) and Ehud Ovadia (Ehud).
Comment #7
mgiffordComment #8
kiwimind CreditAttribution: kiwimind at Chicken commentedRe-rolling to fix indentation issue.
Comment #9
DuaelFrI don't like the way the results are counted. The entire handleFilter function is DOM-agnostic so I think we should keep it this way so, if an admin theme wants to change the way that UI is done, this function will continue working. See the attached patch.
I think that we could test this feature but, as an accessibility improvement, I'd like it to be commited quickly and maybe add tests in a follow-up. @mgifford what would you think about that?
Comment #10
mgiffordI'd like to do that, but not sure that we'll get buy-in. The D8 cycle has had tests first for pretty much everything.
Comment #12
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedI suggest using Drupal.plural to make sure that we don't end up with '1 views are available'.
Comment #13
starshapedWorking on this as part of #sprintweekend2017 #sprintweekendBOS.
Comment #14
starshapedAdded call to Drupal.formatPlural to display '1 view is' or 'n views are' depending on the search results. Patch attached.
Comment #15
starshapedRe-uploading the patch and flipping the ticket to 'needs review' to kick off testing.
Comment #16
left CreditAttribution: left commentedThis patch is based on #9.
I think #14 is incomplete due to not having incorporated #9 and the use of both Drupal.t and formatPlural was not necessary.
I've used only formatPlural (without Drupal.t) and I've renamed the areActive variable to activeViews as I feel it is more descriptive.
Comment #18
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedGreat work left and starshaped. The patch in #16 works, but can be improved just a little.
One thing to improve here: formatPlural already replaces the @count variable out-of-the-box. If you only want to replace the number of items, you don't need to add the extra parameters.
This would suffice:
Comment #19
left CreditAttribution: left commentedHi @BarisW
Thanks for the review and input. I've made a new patch to reflect and changed the status to 'needs review'.
Comment #20
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedAh, but now the patch is missing the logic to count the results. Plus, the Drupal.announce is new. This seems more like a diff against your precious patch than against drupal core?
Comment #21
left CreditAttribution: left commentedOops, you're right. I think I might be missing some details about the patching procedure. It's starting to make more sense now.
Another patch attempt attached :) Thanks for taking the time @BarisW
Comment #22
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedPerfect. When the test turns green this is rtbc according to me.
Comment #23
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThe message in patch #21 says "3 views are available in the modified list." (emph mine), but it isn't a list of views - it's a list of fields (or filters, or sort options, etc.).
The tasks in the issue summary says "6 options are available..." so that's minor change we still need.
Bonus idea: could we get the message to reflect which list we are looking at? Something like "6 fields are available..." vs "6 filter types are available...". If that turns out to be hard, we can leave it for a follow-up.
Comment #24
rachel_norfolkThink I can see how to do this - taking a look now. Give me till the end of the day...
Comment #25
rachel_norfolkWhilst it annoys me greatly that I can't seem to obtain the thing being added, as per #23, we do need to change the text slightly. The text
@count options are available in the modified list.
seems to make sense in all circumstances, at least.
Comment #26
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@rachel_norfolk - Thanks, patch #25 fixes the issue from #23.
The next step is adding functional javascript tests for the announcement message.
Some of the related Drupal.announce() issues already have some tests which are partly written. #2715663: Use Drupal.formatPlural for when announcing module-filter results for screenreader users is a good starting point. If anyone wants to have a crack at those, make sure you understand how to run FunctionalJavascript tests in D8. See Javascript Testing Comes to Drupal 8.
Comment #27
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedRerolled the patch.
Comment #29
benjifisherLooking at the testbot results, I see this:
If you look at what happens with your JS debugger, then it should be pretty easy to see what the problem is.
Comment #30
dmsmidtComment #31
BarisW CreditAttribution: BarisW as a volunteer and at LimoenGroen commentedFor those wanting to write the functional javascript tests, here is an example: #2805205: Provide screen-reader feedback when filtering by block name.
Comment #32
left CreditAttribution: left commentedComment #33
left CreditAttribution: left commentedThe new patch includes announce.js added as a dependency to views_ui.libraries.yml to correct the undefined function error.
And I added a test for the singular case, where one option is available in the list.
I'm adding a patch and inter-diff to reflect.
The plural case, where multiple options are available in the list, still needs a test.
Comment #34
LendudeAdded test coverage for the multiple rows feedback.
Comment #36
LendudeActually including the fix might help :)
Comment #37
loopduplicateWe need to edit views-admin.es6.js and transpile it to views-admin.js. See Drupal core now using ES6 for javascript development , which is also linked to in a comment at the top of views-admin.js. Here's an updated patch.
Comment #38
loopduplicateThis looks pretty good to me. The tests are still passing too. Marking as RTBC.
Comment #41
loopduplicateHiding failing patches.
Comment #42
larowlanHey, can we get someone else to RTBC your patch?
Thanks
Comment #44
rachel_norfolkLatest patch applies cleanly to 8.4.x and I believe we are still allowed to submit this change there, based on the rules. Changing version above to reflect that.
Using Apple VoiceOver and Safari, I am now correctly notified of the number of elements in the list, as desired by the change. No unwanted side-effects seem to be apparent.
Screenshot showing the text being read out correctly by VoiceOver (kinda happy it does that now!)
I think RTBC
Comment #45
star-szrMaybe this would be clearer as:
Couldn't find a standard on this but it made me do a double take because of the type mixing :)
Comment #46
rachel_norfolkI’m inclined to agree. I didn’t understand it and just assumed it was because of my lesser js experience.
I’ve had a search through and can’t find anywhere else in core where we do this, mixing the types. Given it’s a simple fix for better clarity, I think we should do it. I’m going to have a look now how to do this es6 thing...
Comment #47
rachel_norfolkOkay, this is my first time using this fancy-pants yarn thing for es6. Please check I’ve not done anything too bad!
Comment #48
findleys CreditAttribution: findleys commentedWill this ticket cover reporting success/failure messages for the action updates on views as well or just filter changes? I'm trying to determine is a ticket already exists for the actions or if I need to create a new one to report to our 508 compliance organization.
Comment #49
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@findleys This issue is just about the behaviour of the filters. Please file a separate issue (it's best to keep an issue focused on one problem).
Comment #50
rachel_norfolkJust updating Issue Summary remaining tasks
Comment #51
LendudeLooks good now, feedback has been addressed, back to RTBC
Comment #52
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks to everyone who has been working on this. It's working well!
As far as WCAG 2.0 goes, I was unsure if this sort of filtering qualified as a "change of context" under SC 3.2.2 On Input. It's kind-of borderline IMO.
However WCAG 2.1 introduces SC 3.2.7 Change of Content, which clearly matches the situation here. There's a longer explanation at #2864791-33: Implement new Success Criteria from WCAG 2.1.
Since we're now actively pursuing WCAG 2.1, I'm reclassifying this as a bug report. Under WCAG 2.0 the announcement was a nice-to-have, but under WCAG 2.1 it becomes a must-have.
Comment #53
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThe FunctionalJavascript test doesn't try a scenario where zero items are are expected. Is this important? Note we did include a zero-items test in #2805205: Provide screen-reader feedback when filtering by block name..
Comment #54
xjmThanks for the patch and the accessibility feedback! One small thing:
This introduces a regression for our JS coding standards for the
no-var
rule -- see #2914946: JS codestyle: no-var.Comment #55
GrandmaGlassesRopeMan@xjm 👏
Comment #56
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedChanged
var
tolet
in the es6.js file. The .js file remains the same after transpiling.Comment #57
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedDoh. I gave the interdiff a .patch extension instead of .txt
Still OK to review, but expect a spurious test result.
Comment #59
LendudeAll feedback has been addressed, back to RTBC
Comment #60
xjmJust a note that while this is a string addition that normally would be done only in a minor release, I agree with backporting it because the accessibility bug (lack of any feedback) is worse than having an additional untranslated message in this UI on a smaller portion of sites.
That seems like a mighty long timeout. Turns out Session::wait() takes the parameter in ms, whereas Element::waitFor() takes it in seconds. I checked and we do
Session::wait()
a few other places in HEAD with 5000, but this test already uses 10s elsewhere, and JsWebAssert::waitForField() has an explicit default of 10,000 ms. (Also note how inconsistent the methods are, switching whether it's the first or second parameter...)Anyway. So this is okay since it's a timeout rather than a fixed time and HEAD and the API have precedent for 10s. So nothing to change here, but golly, I hope we don't use too much of that time. :)
Rather than doing this dynamically, it would be better to just hardcode the expected result, since the number of expected results is related to the test fixture and counting
tr.filterable-option
isn't really the same as just saying "We expect two rows here!" That also lets us remove a couple of lines from this hunk.Thanks for the quick turnaround on this!
Comment #61
LendudeJust some quick feedback on the feedback:
#60.2 Yeah the idea behind the ridiculously long timeout is indeed that it's never used under normal circumstances and so long we will never have random fails because of it if it occasionally takes a little long to get a response
#60.3 I'd be against this using a hardcoded number, since the expected number ('all available node and global filters') can change when we add/remove any sort of node or global filter to core. And although that doesn't happen that often it would be annoying to have to change a completely unrelated test when you do that.
Comment #63
starshapedFixed #60.3 by hardcoding the expected result.
Comment #64
rachel_norfolkHmm - weirdly, whilst this appears good in Voiceover on Mac etc, I’ve just been watching it using NVDA on Windows 10 and it is reading out the number of items in the list but not the supporting text. LIterally, just “10” not “there are 10 items in the list”
Odd.
Comment #65
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThat's an interesting find, @rachel_norfolk - thanks for showing me this at the Leeds sprint meeting.
When we listened to NVDA with this patch, we heard differences between Firefox+NVDA (whole message was correctly announced) versus Chrome+NVDA (only the number was announced).
I wonder if this about the way
aria-relevant
oraria-atomic
properties work.Drupal.announce()
doesn't try to set these - perhaps the different browsers are making up their own default values? If so, that sounds like something that could be improved in theDrupal.announce()
itself. WhenDrupal.announce()
was first written, I think it was mainly tested with VoiceOver. In any case, all the browsers and screen readers have had new releases since then. It's worth opening an issue to explore how robust we can makeDrupal.announce()
.I think we can stick with the current approach in this issue though.
Update: To clarify, I mean the problem/motivation is still valid. I'm open to alternative solutions though. WCAG 2.0 doesn't really address this, but it does seem to fall under the new WCAG 2.1 success criterion called "Status Updates". In the latest WCAG 2.1 working draft (final, supposedly) this has been narrowed down a lot. This issue may not be a hard requirement any more, but reading around the public W3C comments, it seems to be a borderline case for "Status Updates". I'm still digesting this, and will update our main WCAG 2.1 main plan when I know more.
Comment #66
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #67
GrandmaGlassesRopeManWhile only a warning, the eslint rules try to discourage unary operators.
This getting a bit long, something like this might make it a bit more readable,
Comment #69
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThere's a parent plan about this now.
Comment #78
LendudeReroll, plus #67 feedback, plus rename of the variable since this is used for more than just views filtering
No interdiff because of reroll.
Comment #79
LendudeAttempt at fixing the linting.
Also back to getting the number of expected values dynamically in the test, basically because of what I said in #61, the number can change (and unsurprisingly did in the intervening 5 years) and figuring out the right number is annoying.
Comment #80
LendudeI should really get linting to run at some point....
Comment #81
dwwThis came up for daily random #bugsmash triage, that's why @Lendude helpfully resurrected this and re-rolled to address the feedback. Thanks!
I wish we fixed #3122231: Singular variant for plural string must contain @count so we stopped "doing it wrong" and all new code used
@count
for both the single and plural strings. Probably not worth holding this up to change it, so not setting NW. Close review of the code shows no other problems to complain about.The bot is happy. There's new test coverage. The issue title and summary are clear. I don't believe this needs / wants a CR or release note, but at least tagging that it's a "String change".
RTBC!
Thanks again,
-Derek
Comment #82
dwwRemoving credit for #27, a 1-off re-roll with failing tests that the author never came back to help fix.
Adding credit to @BarisW, @mgifford and myself for helpful reviews.
I think that should be pretty close to accurate, but of course the committer will have the final say.
Thanks again, everyone!
-Derek
Comment #83
mgiffordAdding tag for WCAG SC 3.2.7
Comment #85
dwwSeems like a random fail to me:
Comment #86
bnjmnmA few optimizations to do but this overall looks good.
The unary operator converts boolean to integers 0/1, so this can be a single line
activeItems += found
These two lines can be consolidated by using
waitForElementTextContains()
see WidgetUploadTest in Media LibraryThese two lines can be consolidated by using
waitForElementTextContains()
see WidgetUploadTest in Media Library