Skip to content

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

Merged
mdo merged 4 commits into
mainfrom
main-fod-docs-forms-accessibility
Jun 1, 2023
Merged

docs(forms): switch to aria-describedby#38592
mdo merged 4 commits into
mainfrom
main-fod-docs-forms-accessibility

Conversation

@ffoodd

@ffoodd ffoodd commented May 12, 2023

Copy link
Copy Markdown
Contributor

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

@patrickhlauke patrickhlauke left a comment

Copy link
Copy Markdown
Member

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

ffoodd commented May 12, 2023

Copy link
Copy Markdown
Contributor Author

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

@patrickhlauke

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@julien-deramond

Copy link
Copy Markdown
Member

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

@hannahiss

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

ffoodd commented May 15, 2023

Copy link
Copy Markdown
Contributor Author

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

@ffoodd

ffoodd commented May 16, 2023

Copy link
Copy Markdown
Contributor Author

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

@Aniort

Aniort commented May 16, 2023

Copy link
Copy Markdown

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