feat(radio): support aria-label(ledby) on md-radio #596
Conversation
googlebot
added
the
cla: yes
| @@ -54,6 +54,7 @@ The `md-radio-group` component has no button initially selected. | |||
| | `value` | `any` | The value of this radio button. | | |||
| | `checked` | `boolean` | Whether the radio is checked. | | |||
| | `disabled` | `boolean` | Whether the radio is disabled. | | |||
| | `actionName` | `string` | Used to set the `aria-label` attribute of the underlying input element. | | |||
There was a problem hiding this comment.
This should be aria-label. We just want to forward the attribute.
- As same as in the slide-toggle
There was a problem hiding this comment.
I think it is not a good idea to have a pseudo aria-label attribute on the MdRadio component and at the same time a real one on the underlying input element. It might confuse screen readers and its users. Do you know what I mean? That's why I chose a different name as seen on the Angular 2 documentation for template syntax (see Property Binding / We must use attribute binding when there is no element property to bind).
But while taking a look a the current implementation for MdSlideToggle, I see that the property used there is named ariaLabel, which is different from aria-label. So this subtle difference is probably enough avoid potential issues with screen readers.
So if everyone agrees and doesn't see any more issues, I can change the property name (and add aria-labelledby as well).
There was a problem hiding this comment.
We also use the aria-label input in the checkbox as well. (See here).
Screenreaders shouldn't be confused about that attribute, because the host element is normally not focusable.
Without any attribute binding, you're always using the dash cased attribute
<md-radio-button aria-label="My Label Input">But with an attribute binding, it will be the camel-cased attribute name.
There was a problem hiding this comment.
Thanks for the explanation. I will conform to the pattern used for the checkbox component.
|
Agreed on all of @devversion's comments |
|
I updated the pull request.
Let me know, if this is ok. I will squash all commits then, so it will be ready to be merged. |
| }); | ||
|
|
||
| it('should not add aria-label attribute if not defined', () => { | ||
| expect(fruitRadioNativeInputs[1].getAttribute('aria-label')).toBe(null); |
There was a problem hiding this comment.
I'd use hasAttribute here and in other tests where you're checking for the presence of the attribute.
|
Looks good aside from that one minor comment |
jelbourn
changed the title
jelbourn
changed the title
|
Thanks for the feedback. I updated the two tests to use |
|
LGTM |
|
Thanks! |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds feature #586
radio: adding actionName attribute as label fallback (aria-label)
Added
Please give me feedback, in case something needs to be changed.