-
Notifications
You must be signed in to change notification settings - Fork 47
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
fix(a11y): Add keyboard support to highlight comment flow #690
Conversation
}; | ||
|
||
useOutsideEvent('keydown', buttonRef, handleKeydownOutside); |
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.
Is the approach of setting focus to the button on mount viable?
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 would rather avoid messing with focus, if possible, since I would need to set it back to the originating element if the popup is closed.
useOutsideEvent('mousedown', buttonRef, onCancel); | ||
|
||
return ( | ||
<PopupBase className="ba-PopupHighlight" options={options} reference={reference}> | ||
<PopupBase | ||
aria-live="polite" |
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.
Does aria-live
and role="dialog"
jive together? Seems like the docs indicate that a dialog should draw focus to the button whereas aaria-live
instructs a live region to be read out when update a list of search results on the fly, or to display a discreet alert or notification which does not require user interaction
.
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.
It seems to work well. I found that omitting aria-live
made it so that VoiceOver doesn't "see" the popup when it's added to the DOM. I think role="dialog"
should be okay, from the docs linked:
The dialog role is used to mark up an HTML based application dialog or window that separates content or UI from the rest of the web application or page. Dialogs are generally placed on top of the rest of the page content using an overlay. Dialogs can be either non-modal (it's still possible to interact with content outside of the dialog) or modal (only the content in the dialog can be interacted with).
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.
Should we have some aria-label
for the dialog to instruct about pressing 'enter' to add the highlight? I'm thinking since it's not a standard a11y pattern, they might not know what to do even if the screen reader reads out the promoter
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 don't think so. It's a button, so it should be the same interaction as any other interactive element.
No description provided.