-
Notifications
You must be signed in to change notification settings - Fork 563
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
Improve keyboard and screen reader accessibility #2726
Conversation
Elements within `grid` and `rowgroup` require specific roles to be valid for the ARIA spec.
- Announce notes list title when focused on list. - Reduce notes list to one column for easier focus navigation. - Change actionable elements to buttons to improve accessibility. - Improve accuracy of note list labels.
This span appears to be unnecessary and was creating an unnecessary "group" when stepping through UI elements with macOS VoiceOver.
Center the icon within the tap area so that the button is easier to tap with a larger tap area.
Hide internal text input. Describe available action on tag list.
Improve keyboard and screen reader navigation by describing various pieces of the top-level layout and hide internal developer tooling.
Ensure screen reader can accurately describe icon buttons that do not contain text strings.
lib/search-field/index.tsx
Outdated
const placeholder = openedTag ?? 'Search notes and tags'; | ||
|
||
const screenReaderLabel = | ||
'Search ' + (openedTag ? 'notes with tag ' : '') + placeholder; | ||
const description = | ||
'Search ' + | ||
(openedTag ? 'notes with tag ' + openedTag : 'notes and tags'); |
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.
I wanted to draw attention to this change. Previously the placeholder value and the ARIA label were different strings. From reviewing the history of the UI, this at one point made sense because it appears the search field was utilized as the "title" of the list of notes, but also served a search input.
However, several changes were made to this portion of the app — the UI was moved around, the structure of the state, etc. These various changes led to a screen reader description that read "Search Search notes and tags Search notes and tags, search text field." 😂 Needless to say, this description is a bit...confusing.
So, these changes are an attempt to fix this label, but it also involves changing the visual placeholder for the search input. I would appreciate if others would please look carefully at this piece to ensure the current state matches the desired experience.
The SVG icon was cut off on the bottom due to its vertical alignment.
@@ -320,6 +339,7 @@ export class NoteList extends Component<Props> { | |||
rowHeight={heightCache.rowHeight} | |||
rowRenderer={renderNoteRow} | |||
scrollToIndex={selectedIndex} | |||
tabIndex={null} |
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.
what's the difference between tabIndex={-1}
and tabIndex={null}
?
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.
The difference being that tabIndex={null}
unsets a tabIndex="0"
set by default from react-virtualized
and tabIndex={-1}
disables the element from being focused by keyboard tabbing while still allowing programmatic focus.
Given that the contents of our virtualized list are focusable (buttons to pin or edit a note), IMO it doesn't make sense for the entire list to be a focusable element. We also, to my knowledge, have no need to programmatically focus the list, so null
makes more sense than -1
.
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.
interesting. thanks! so the list is no longer focusable: does that have any implications for scrolling the list with the keyboard?
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.
Good question. I had not explicitly tested scrolling the list with the keyboard until just now. It would appear the scrolling the list is still possible and matches the experience as when tabindex="0"
is set. 👍🏻
Address issues introduced from changing elements from div to button for better accessibility.
Avoid unnecessary class method.
Accurately describe that search is scoped to the selected tag. Hide overflow ellipsis.
Replace static "Notes With Selected Tag" title with the tag name itself to provide better context.
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.
This looks good and works well. Seem much better than what we had previously. Thanks for taking the time to do this.
The only confusing part I had while testing with voiceover was when clicking the button to open the left navigation. Trying to get to the items in it once I clicked the button wasn't straight forward, but I think we can look at that in some future enhancements.
@sandymcfadden thanks for taking time to review. #2707 should address all issues with navigating the left navigation with a keyboard or screen reader. So, until both PRs are merged, one has to test those areas independently, unfortunately. Please let me know if I am misunderstanding or there are specific issues you encountered. |
Yes @dcalhoun yeah, of course, you're right. I even looked at and tested the other one. I think I got confused with the two, sorry for that confusion. |
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.
With the caveat that I don't have a lot of domain knowledge here, so in general I trust your expertise. This all looks great!
Fix
Related to #1935 (comment), this addresses several issue that inhibited keyboard
and screen reader users from navigation the application easily.
Summary of changes:
The remaining issues identified by the axe DevTools fall into three categories:
Test
Expected Outcome: No major issues block navigating the application.
Release
Improved accessibility of the application for keyboard and screen reader users.