"Create a New Repository" already an repository warning should be an error and announced #16993
Conversation
| @@ -69,14 +69,32 @@ export class AriaLiveContainer extends Component< | |||
| this.onTrackedInputChanged.cancel() | |||
| } | |||
|
|
|||
| /** If the children has a tab focusable element such as a link, then we get | |||
| * hidden tabs on the screen. For screen readers, we only need the text */ | |||
| private getChildrenAsText(children: React.ReactNode): string { | |||
There was a problem hiding this comment.
This stands out as not very idiomatic to me. Iterating over the (react) children is usually a good way to get into trouble as it's operating over the unprocessed JSX tree and not what would eventually get rendered.
It seems to me that what we're wanting to do here is restrict the AriaLive component to only text (which makes sense to me) then we should make it accept a required message prop of type string and stop rendering children instead of trying to poke at the child innards.
There was a problem hiding this comment.
Done. d2deac2 Tho, this does make the components be a lot more verbose than needed most of the time. :/ Not a huge fan of that, but, I think it does make more sense than iterating over the children (which seems discouraged).
I thought of doing a hybrid approach that says if a message is provided, use that otherwise use the children so that we only have to provide a message if the children have tab content... but that seems fraught for someone to make the mistake of having an invisible tab which is not obvious to detect. Plus.. just more code maintenance.
There was a problem hiding this comment.
BTW, I did regression retest along with some known edge case,
- Autocompletion results
- changed list status
- filter list results
- add repositories errors
- tooltipped content thing
- create repository error
- misattributed commit warning.
| <AriaLiveContainer | ||
| message={pathScreenReaderMessage} | ||
| ></AriaLiveContainer> |
There was a problem hiding this comment.
Nit:
| <AriaLiveContainer | |
| message={pathScreenReaderMessage} | |
| ></AriaLiveContainer> | |
| <AriaLiveContainer message={pathScreenReaderMessage} /> |
| <AriaLiveContainer | ||
| message={screenReaderMessage} | ||
| trackedUserInput={this.state.filterValue} | ||
| ></AriaLiveContainer> |
There was a problem hiding this comment.
Nit:
| ></AriaLiveContainer> | |
| /> |
| <AriaLiveContainer | ||
| message={this.props.ariaLiveMessage} | ||
| trackedUserInput={this.props.trackedUserInput} | ||
| ></AriaLiveContainer> |
There was a problem hiding this comment.
Nit:
| ></AriaLiveContainer> | |
| /> |
| > | ||
| {tooltip} | ||
| </AriaLiveContainer> | ||
| ></AriaLiveContainer> |
There was a problem hiding this comment.
Nit:
| ></AriaLiveContainer> | |
| /> |
| @@ -1,7 +1,23 @@ | |||
| import { debounce } from 'lodash' | |||
| import React, { Component } from 'react' | |||
|
|
|||
| export interface IAccessibleMessage { | |||
There was a problem hiding this comment.
Took me a while to understand that this is just some kind of auxiliar structure/interface and nothing actually used by the AriaLiveContainer.
Could we add a comment describing it and maybe even move it to its own file?
My next PR is around screen reader stuff so I will clean these suggestions up in it so I can get this out for accessibility review. |
tidy-dev
dismissed
niik’s stale review
I updated the PR to match his suggestion and he is now OOO.
xref: https://github.com/github/accessibility-audits/issues/4063
Description
This remediates the accessibility issue of the error of a repository already being a repo in the "Create a New Repository" not being announced and styles it as an error instead of a warning. It also make the readme overwrite warning be announced.
While testing this, I found that because the error message had a link if I tabbed through to the next input I would have to double tab because the link in the invisible aria live div. This PR also adds some basic aria live children sanitization that only looks for strings and numbers. I haven't thought of another instance of a child we would want to render, open for scrutiny. :)
Screenshots
CleanShot.2023-06-28.at.07.05.32.mp4
Release notes
Notes: [Improved] The errors and warnings in the "Create a New Repository" dialog are screen reader announced.