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

docs(forms): switch to aria-describedby #38592

Merged
merged 4 commits into from
Jun 1, 2023
Merged

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented May 12, 2023

Description

aria-labelledby is wrongly used here since it overrides existing label: either we add label's id as the first value in aria-labelledby, or we switch to aria-describedby as done in this PR.

In both cases, I think the callout before should mention this difference between the two attributes.

Friendly ping @patrickhlauke as you may have another recommendation. :)

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, agree, that's wrong. any other places where we might have the same oversight?

@ffoodd
Copy link
Member Author

ffoodd commented May 12, 2023

I checked the forms page without luck but didn't check examples or cheatsheet.

@patrickhlauke
Copy link
Member

ah, now i remember where that came from... #37587

I was actually not convinced by that PR myself, but it was merged anyway. I would still say that the assertion at the basis of that PR is...debatable. I would suggest just wholesale reverting that PR, to be honest.

@patrickhlauke
Copy link
Member

i.e. also remove the paragraph that says authors should use aria-labelledby ...

@julien-deramond
Copy link
Member

Friendly ping @hannahiss and @Aniort. Might be interested in the topic.

@hannahiss
Copy link
Contributor

Yes, you're right, #37587 might have been merged a little bit too quickly...
I would propose to add label's id as the first value, since using aria-labelledby for mandatory information could be a good idea...? Would like to have @Aniort opinion on this.
The callout should be adapted anyway...

Please don't revert totally #37587 because the first part (in input-group) is still needed.

@patrickhlauke
Copy link
Member

I would propose to add label's id as the first value, since using aria-labelledby for mandatory information could be a good idea...?

While I understand the "could be a good idea" part, I personally think this ends up being massively overengineered for what it actually is/does (particularly for a generic example in our documentation). It's been generally accepted practice to use aria-describedby to link additional info and instructions for a form field. The nuance of "if this is mandatory, it should be in the accessible label, not in the accessible description" is one that I'd say should be left up to authors to decide. Any more information on the original assertion that aria-labelledby is better supported than aria-describedby?

@patrickhlauke
Copy link
Member

Please don't revert totally #37587 because the first part (in input-group) is still needed.

ah yes, good point. agree, that part is non-controversial and correct

@ffoodd
Copy link
Member Author

ffoodd commented May 15, 2023

Agreed, if it's that much important it should probably be in <label>, not passed as a potentially wrong label override.

@ffoodd
Copy link
Member Author

ffoodd commented May 16, 2023

I just reverted the callout changes from #37587, and also improved labels in select sizing examples—which previously mentioned class names…

@Aniort
Copy link

Aniort commented May 16, 2023

I think using aria-describedby instead of rewriting <label> via aria-labelledby is the good choice when infos are not mandatory and inserted in the <label>if not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants