-
Notifications
You must be signed in to change notification settings - Fork 93
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
Set navigation role and aria label on app navigation #1585
Conversation
ariaLabel: { | ||
type: String, | ||
default: '', | ||
}, | ||
/** | ||
* aria-label for the toggle button | ||
*/ | ||
toggleAriaLabel: { | ||
type: String, | ||
default: '', | ||
}, |
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.
Do we really need them?
ariaLabel: { | |
type: String, | |
default: '', | |
}, | |
/** | |
* aria-label for the toggle button | |
*/ | |
toggleAriaLabel: { | |
type: String, | |
default: '', | |
}, | |
ariaLabel: { | |
type: String, | |
default: t('App navigation'), | |
}, | |
/** | |
* aria-label for the toggle button | |
*/ | |
toggleAriaLabel: { | |
type: String, | |
default: t('App navigation toggle'), | |
}, |
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.
Seems it's possible to set the aria-label anyway from outside, so probably not needed as property.
With the toggle I have mixed feelings as it's currently not reachable with keyboard nav, but we might need to do it.
We could also agree to settle on "App navigation" and "App navigation toggle" for these names regardless of the app.
The only thing is that with the talk app it doesn't feel like an app navigation as it's the conversation list, hence the idea of having a custom text there.
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'm fine with having this if you think it's easier, but default to an empty string and fallback in the template is not the way to go :)
props defaults are here for a reason ;)
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.
Seems it's possible to set the aria-label anyway from outside, so probably not needed as property.
Only for the top container though, not for the toggle
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.
ah ok.
anyway, I'll certainly return to this once I look into why we cannot tab to the toggle
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.
See comments
This makes it a region that one can jump to with accessibility tools and also gets announced as such when tabbing into it or exiting it. In general apps should provide an aria label to give such region a name. Signed-off-by: Vincent Petry <vincent@nextcloud.com>
5d8e46c
to
b3c80f5
Compare
Ok, I've decided to go only with the "role=navigation" which should be enough to make this a region to jump to. |
Makes it possible for screen readers to say the text of aria label
followed by the term "navigation" when entering that region.
Also added an optional arial label for the toggle.
In the talk app I've set the aria label to "Conversation list" so when entering the left sidebar (or jumping to any element inside) the screen reader says "Conversation list navigation".