Fix top bar challenges notifications accessibility issues #12803
Conversation
These two toggle buttons used an <a> element despite lacking an href attribute. This makes screen readers and using tab to navigate the site skip over these elements since they appear non-interactive. (A link with href can't be navigated to). To make it more semantically correct, and indeed picked up by navigating this way, this replaces them with <button>. This does require a CSS change (to remove user agent default background and border), and a small JS changes, so the code looking for a.toggle in the top bar now looks for button.toggle instead.
top-bar Notifications, challenges: include number of unreads in the title attribute, so that a screenreader can read out "Challenges: 7", for example.
When opening the challenges at the top, automatically focus on the "Accept" button of the first incoming challenge. If the challenges have not yet loaded, this will try again up to 5 times on an interval of 100ms. I figure any longer than that could be jarring for an automatic focus change. An additional change here is to make the accept button "described by" the challenge details. This ensures a screen reader doesn't just read out "Accept- button" without informing the user about any of the challenge details. The other challenge controls are not described by this, since I figure that moving to the decline button, the user already knows the challenge they're dealing with. That said, it's trivial to make the description apply to the other controls like it does to the accept button.
Analogous to how it's done in challenges in the previous commit.
Guess I made a typo in my `sed` replacement command. Thankfully I have `sed` to fix it.
Thanks! So is the procedure to only make change to the source/.xml files and leave the rest alone? Shall I revert the changes to dest/.xml files? |
Yes, about reversing changes it's better but can do if you want |
This reverts changes to translation destination files, as those will be supplied by crowdin. I did run trans-clean to resolve the errors that left in trans-lint. Also properly capitalizes X in new translation keys where a variable is indicated.
|
No worries, I've got no trouble doing that revert. :) I also ran trans-clean as trans-lint had a bunch of complaints about the now orphaned "challenges" key in all the dest files. |
|
When hovering over the notifications the PR introduce a weird blue focus that stay even after moving the cursor away. I assume it's due to switching to button but haven't found where it comes from |
|
Hm, well, the notification does get focused by these changes. Might be a default browser style for Perhaps it should be something that only happens when in accessibility mode instead of always. In firefox I'm not seeing this effect; what browser is this in? |
|
Safari. If it only affects Safari it's ok, a workaround can be found later |
|
Added a commit which ensures the auto-focusing of the first item upon opening challenges/notification only happens when blind mode is enabled. Well I'd rather not introduce visual defects if I can help it. :-) |
|
I reverted the changes to the trans destinations. Crowdin will automatically delete the unused keys and doing it manually will just lead to massive merge conflicts. It probably wasn't a good idea to add the clean script to the repo in the first place. Though it maybe would make sense to stop the lint from complaining about this. |
|
Makes sense, thanks @benediktwerner. Do you think it'd be good to add some notes about translations and how to modify them in the repo? I'm thinking either in contributing.md or a new readme file in translations/. It's out of scope for this PR but I'd be happy to submit another small one, just to help save some time in future. :) |
|
We have How translations work, and working on translations wiki pages which should cover it pretty much (feel free to fix things if you see! It's open to edit for all), which are linked at the beginning of the development guide but I agree you can skip them easily. |
|
I'll admit I must've skipped over those when getting started, oops 😅 |
|
It doesn't look like they are linked in the dev guide. I added a link to the wiki homepage but definitely still hard to find everything. Maybe would make sense to at least link the wiki in the contributing.md as well. |
|
Is there anything further I should do for this one at the moment? I could merge the latest from the |
* master: (414 commits) setup rating: final checks and simplification fix more duplicated code fix duplicated code, fix bug when ratingMap[perf] doesn't exist remove superfluous check, the type says it always returns a Perf Revert "fewer dom text nodes and more css tricks" only feature swiss tournaments where players actually play scala tweaks scala tweaks New Crowdin updates (lichess-org#12949) tweak audio context handling voice mic selector prevent lichess.storage race condition on init moveCtrl is available remove ui/voice `let moveCtrl` global move voice toggle code out of the event listener mic.ts: if the audio context is suspended, wait for user interaction use OpaqueInt+Int=OpaqueInt Use fullMoveNumber when writing played moves Revert RendererActor visibility make RoundSocket round robin listen to 16 channels ...
ui/site/src/component/top-bar.ts
Outdated
| $countSpan | ||
| .attr('title', siteTrans('challengesX', nb)) | ||
| .attr('aria-label', siteTrans('challengesX', nb)) | ||
| .data('count', nb); | ||
| const newTitle = $countSpan.attr('title')!.replace(/\d+/, nb.toString()); | ||
| $countSpan.data('count', nb).attr('title', newTitle).attr('aria-label', newTitle); |
There was a problem hiding this comment.
I considered something like this before learning how to use the clientside portion of the translations system, but didn't like it because it felt a bit too uncertain whether any of the translations might include something matching /\d+/. I suppose that's very unlikely, still feels pretty hacky though.
Does the addition of translation keys in the main layout make much of an impact, or is it more that adding them there is a bit of a hacky workaround in itself?
There was a problem hiding this comment.
adding the translation keys in the layout is the right thing to do. But it does add some weight and parsing time to every homepage load. So here I decided that we could use a little hack instead.
There was a problem hiding this comment.
Makes sense, the weight on every load outweighs the limited applicability of the keys (when notifications/challenges update).
too dirty for my taste, with these setTimeout dom manipulation retries it should probably be done in the relevant ui/apps instead, with snabbdom in general it's better to stick to a single feature per pull request, to help streamline the review process
|
Thanks for the PR and sorry for the delay! Please see my last commit comment about the |
|
Cheers! Saw it, makes sense to me. :) |
Fixes #12305.
Summary of changes
Change
<a>to<button>for the top bar togglesAn anchor element without a
hrefattribute is not considered an interactive element, so the browser skips over it when moving between controls. Since there is nothing to link to, a<button>seemed like an appropriate replacement, compared to using something like<a href="#">.Include count in labels
The label (
titleattribute) for the challenges toggle now includes the number of challenges.This is duplicated in the
aria-labelattribute, because when a number was displayed (data-count > 0), the screen reader would read that number and skip the contents of thetitleattribute. Thearia-labelattribute being present overrides this behaviour.This change is implemented both in the layout in scala, as well as in the code handling updates through web socket, ensuring the data in there remains accurate.
The same change was made to notifications.
All translations are updated for the updated "Challenges: X" and "Notifications: X" form.
Auto-focus first item
When opening the challenges/notifications dropdown, automatically focus on the first item. For challenges this is the first incoming challenge, for notifications, the first navigable notification.
For challenges, the focus is shifted to the "accept" button. Since the accept button on its own provides no information about the challenge, it is now described by the contents of the challenge, with an
aria-describedbyattribute pointing at the HTML element containing that. Screen readers first read out "Accept, button", then read the contents of challenge.Side note: when the properties of the challenge (color, time control, rated/unrated) is spoken, the separating character is also pronounced. A good further optimization would be to convert this list of things to some markup that gets styled instead, to reduce the screen reader noise. I've not included that in these changes though.
The reject button and reason
<select>are not "described by" anything—I figure that the user will have the context from when it was read out for the accept button.A little hack
When first opening challenges/notifications after a page load, the "application" is not yet loaded, so the first incoming challenge is not necessarily available yet, so there's nothing to send focus to. To work around that, I've added a function that polls every 100ms for its presence, and gives up after 5 attempts. The 500ms was chosen because I believe it would be jarring for focus to shift automatically longer than half a second after the last actual interaction. In my testing this was plenty.
I also considered using
pubsubbut this seemed like a more self-contained approach.