-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Desktop: Resolves #11621: Accessibility: Add status update after update #11634
base: dev
Are you sure you want to change the base?
Conversation
The accessibility tester is failing in CI — it looks like the Error: expect(received).toEqual(expected) // deep equality
- Expected - 1
+ Received + 40
- Array []
+ Array [
+ Object {
+ "description": "Ensure all ARIA attributes have valid values",
+ "help": "ARIA attributes must conform to valid values",
+ "helpUrl": "https://dequeuniversity.com/rules/axe/4.10/aria-valid-attr-value?application=playwright",
+ "id": "aria-valid-attr-value",
+ "impact": "critical",
+ "nodes": Array [
+ Object {
+ "all": Array [
+ Object {
+ "data": Array [
+ "aria-controls=\"search-results\"",
+ ],
+ "id": "aria-valid-attr-value",
+ "impact": "critical",
+ "message": "Invalid ARIA attribute value: aria-controls=\"search-results\"",
+ "relatedNodes": Array [],
+ },
+ ],
+ "any": Array [],
+ "failureSummary": "Fix all of the following:
+ Invalid ARIA attribute value: aria-controls=\"search-results\"",
+ "html": "<input type=\"text\" placeholder=\"Search for plugins...\" spellcheck=\"false\" aria-controls=\"search-results\" class=\"sc-eeDRCY sc-dtBdUo bKptan Qsarb\" value=\"\">",
+ "impact": "critical",
+ "none": Array [],
+ "target": Array [
+ ".sc-eeDRCY",
+ ],
+ },
+ ],
+ "tags": Array [
+ "cat.aria",
+ "wcag2a",
+ "wcag412",
+ "EN-301-549",
+ "EN-9.4.1.2",
+ ],
+ },
+ ]
26 | await waitForAnimationsToEnd(page);
27 | const results = await createScanner(page).analyze();
> 28 | expect(results.violations).toEqual([]);
| ^
29 | }; Note that the automated accessibility tests can be run locally with Thank you for working on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part, this looks good to me! I've left two (minor) comments.
Thank you for working on this!
@@ -302,6 +302,7 @@ const NoteList = (props: Props) => { | |||
onKeyUp={onKeyUp} | |||
onDrop={onDrop} | |||
onContextMenu={onContainerContextMenu} | |||
id='search-results' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id='search-results' | |
id='notes-list' |
Since this <div>
is also used for displaying the notes list (when not searching), an ID of notes-list
might make more sense.
@@ -198,7 +198,7 @@ const NoteList = (props: Props) => { | |||
|
|||
const renderEmptyList = () => { | |||
if (props.notes.length) return null; | |||
return <div className="emptylist">{getEmptyFolderMessage(props.folders, props.selectedFolderId)}</div>; | |||
return <div className="emptylist" role="status">{getEmptyFolderMessage(props.folders, props.selectedFolderId)}</div>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional) Consider leaving a comment that explains why role='status'
is used here.
Resolves #11621
Summary
While searching notes on Desktop there was no feedback on screenreaders on how many resulting notes were available, if any.
This PR addresses this behaviour by adding an aria-controls tag to the list of notes and a role "status" to the message of an empty note list.
Testing