-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 Management screen-reader accessibility. #11601
Improve Management screen-reader accessibility. #11601
Conversation
- Improve description of image generated by an Image URL scripted field. - Remove unnecessary title attribute from Create Index Pattern ISO week date link. - Add aria-label for Edit Index Pattern tabs. - Add aria-label for Edit Saved Objects tabs.
@@ -76,7 +76,6 @@ | |||
class="kuiLink" | |||
href="https://en.wikipedia.org/wiki/ISO_week_date" | |||
target="_blank" | |||
title="Wikipedia: ISO Week Date" |
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.
We should never use title
, and this element contains text so it's already accessible.
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.
Out of curiosity: why is using title
bad? Not as part of this PR but it might be good to include this guideline (with rationale) in our HTML style guide.
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.
Never mind, I found your link in the issue for this PR: https://www.paciellogroup.com/blog/2012/01/html5-accessibility-chops-title-attribute-use-and-abuse/. I'm do think we should add this guideline to our HTML style guide (as part of a separate PR, if you prefer).
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 noticed that the article is from January 2012, over 5 years ago. The main reason it suggests not using title
is that not all browsers support it. I checked https://caniuse.com/#search=title and that seems to no longer be the case. Thoughts?
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.
Never mind (again!) 😛. I misunderstood the article (and the W3C recommendation it links to). It's not so much that browsers don't support title
, it's that its not very friendly on devices that don't have a keyboard or mouse (screenreaders, etc.). +1 for not using it and documenting it in our HTML style guide.
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.
Haha ok I'll open a PR. Thanks man.
<span data-test-subj="tab-count-{{ editSection.index }}"> | ||
<span | ||
data-test-subj="tab-count-{{ editSection.index }}" | ||
aria-label="{{:: editSection.count + ' ' + editSection.title}}" |
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 tried to make this label a little more informative by including both the count and the title.
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.
Silly question, but how do I get the aria-label to display?
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's only accessible to screen-readers, so you'll have to use a screen reader to read it aloud to you. I'm on OS X so I'm just using the Voice Over app which ships with it. I think for Windows, NVDA is really popular (and it's free). Here's a guide on how to use it.
ng-click="changeTab(service)" | ||
> | ||
{{ service.title }} | ||
<small> | ||
({{service.data.length}}<span ng-show="service.total > service.data.length"> of {{service.total}}</span>) | ||
<small aria-label="{{:: service.data.length + ' of ' + service.total + ' ' + service.title }}"> |
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.
To make this fully make sense, I combined the visible number of services, the total, and the kind of service.
src/ui/public/stringify/types/url.js
Outdated
@@ -93,7 +93,7 @@ export function stringifyUrl(Private) { | |||
|
|||
switch (this.param('type')) { | |||
case 'img': | |||
return '<img src="' + url + '" alt="' + label + '" title="' + label + '">'; | |||
return `<img src="${url}" alt="A dynamically-specified image located at ${label}">`; |
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 original title was just the URL, which doesn't describe the image contents at all. Unfortunately, because this image is dynamically defined and AFAIK we don't support description metadata, the best we can do is to just tell screen readers about where this image came from.
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.
alt="A dynamically-specified image located at ${label}"
Are you sure label
is the field that you want to display here? If I add a field formatter to the field, and specify a label, then all of the alt
values are going to be identical. Did you want this to be url
?
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 catch! Thanks man.
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.
LGTM
src/ui/public/stringify/types/url.js
Outdated
@@ -93,7 +93,7 @@ export function stringifyUrl(Private) { | |||
|
|||
switch (this.param('type')) { | |||
case 'img': | |||
return '<img src="' + url + '" alt="' + label + '" title="' + label + '">'; | |||
return `<img src="${url}" alt="A dynamically-specified image located at ${label}">`; |
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.
alt="A dynamically-specified image located at ${label}"
Are you sure label
is the field that you want to display here? If I add a field formatter to the field, and specify a label, then all of the alt
values are going to be identical. Did you want this to be url
?
@BigFunger Could you take another look and let me know if this makes sense? |
LGTM |
* Improve Management screen-reader accessibility: - Improve description of image generated by an Image URL scripted field. Allow formatted label to be presented in lieu of default alt attribute. - Remove unnecessary title attribute from Create Index Pattern ISO week date link. - Add aria-label for Edit Index Pattern tabs. - Add aria-label for Edit Saved Objects tabs.
* Improve Management screen-reader accessibility: - Improve description of image generated by an Image URL scripted field. Allow formatted label to be presented in lieu of default alt attribute. - Remove unnecessary title attribute from Create Index Pattern ISO week date link. - Add aria-label for Edit Index Pattern tabs. - Add aria-label for Edit Saved Objects tabs.
* Improve Management screen-reader accessibility: - Improve description of image generated by an Image URL scripted field. Allow formatted label to be presented in lieu of default alt attribute. - Remove unnecessary title attribute from Create Index Pattern ISO week date link. - Add aria-label for Edit Index Pattern tabs. - Add aria-label for Edit Saved Objects tabs.
Partially addresses #11520. Extracts some work from #11548.