Skip to content
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

Make radio button accessible #1032

Merged

Conversation

mikoscz
Copy link
Contributor

@mikoscz mikoscz commented Dec 3, 2018

I've added

For paper-radio-button:

  • aria-checked
  • aria-label
  • role

For paper-radio-group:

  • role
  • aria-labelledby

Added

  • paper-radio-group-label

I don't like the solution with aria-labelledby inside the paper-radio-group I was thinking about creating the new component paper-radio-group-label which will be yielded like labelId and it would have passed action that will set the aria-labelledby on the parent radio group component. But I think that would be a bigger change from component API perspective - what do you think about that?

@miguelcobain
Copy link
Collaborator

@mikoscz indeed, yielding a {{group.label}} component would be much better than yielding the label id.

It is just an addition to the existing API and it won't break any existing code people are using, so I think that would be the best thing to do!

Can you update this PR with that?

@mikoscz
Copy link
Contributor Author

mikoscz commented Dec 7, 2018

@miguelcobain updated 😄

@miguelcobain
Copy link
Collaborator

Instead of the "upward" setAriaLabelledBy action to inform the label id, you could easily generate a unique id "downward":

label=(component labelComponent id=labelId)

Where labelId could be defined as

labelId: computed('elementId', function() {
  return `${this.elementId}-label`;
})

@mikoscz
Copy link
Contributor Author

mikoscz commented Dec 7, 2018

That was my initial idea - but then I need to add the logic that checks if the label element is present in the DOM. To not set the aria-labelledby with the id that does not exist. I think it is more clear that when the label is placed in the DOM it tells the group "Hi I just landed now I am describing you". The code is also simpler IMO in the case with action. What do you think about that? If you don't like this approach just give me a sign and I'll add the logic with the passing label from the group to the label component.

@miguelcobain
Copy link
Collaborator

I see the problem now.
The label is optional, so we can't set aria-labelledby attribute without knowing if a label is going to be rendered or not.

This seems good to me, thanks!

@miguelcobain miguelcobain merged commit faa243a into adopted-ember-addons:master Dec 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants