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

Remove unnecessary label title #1826

Merged
merged 8 commits into from
Feb 15, 2022

Conversation

PeterYangIO
Copy link
Contributor

@PeterYangIO PeterYangIO commented Jan 29, 2022

Remove unnecessary title attribute on <InputLabel/>. Currently there is a title that says "required field" when the input is marked as required which can end up overriding the accessible name for the input. The first screenshot shows how the a11y labels are parsed when trying to use getByRole('textbox', {name: TEXTINPUTFIELD_LABEL_TEXT}). As you can see, the title overrides the label text which will be particularly confusing on a page with multiple required inputs as the a11y tree will show multiple text inputs all with the "required field" name.

In this PR I simply remove the title attribute because the necessary "required" information is already passed to the <input/> itself with the required attribute.

Screenshots

image

No visual changes (unless you count the lack of a title when hovering as one).

Merge checklist

  • Added/updated tests
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari // I'm on windows
  • Tested in Edge

@PeterYangIO PeterYangIO requested review from a team and pksjce January 29, 2022 08:13
@changeset-bot
Copy link

changeset-bot bot commented Jan 29, 2022

🦋 Changeset detected

Latest commit: 1daaa0e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@PeterYangIO
Copy link
Contributor Author

Bonus side-effect showing how it fixes the issue with Testing Playing (for testing-library):

For required fields with fix applied:
image

Trying to do the same without the fix applied:
image

@PeterYangIO
Copy link
Contributor Author

@pksjce Do you think you could take a look or assign the right person? Thanks!

@rezrah rezrah self-requested a review February 9, 2022 12:27
@rezrah
Copy link
Contributor

rezrah commented Feb 9, 2022

LGTM. Thanks for raising this PR @PeterYangIO.

@alliethu @mperrotti - Do you see any issues if we stop the title attribute automatically being applied to required fields? It also seems to prevent users from applying custom title values in the InputField component.

Screenshot 2022-02-09 at 12 48 41

@PeterYangIO
Copy link
Contributor Author

Hi @rezrah I do not have merge permissions for the repo, if you think it looks good do you think you could merge it?

Thanks!

@rezrah rezrah merged commit 004c462 into primer:main Feb 15, 2022
@primer-css primer-css mentioned this pull request Feb 15, 2022
@PeterYangIO PeterYangIO deleted the users/peteryangio/remove-label-title branch February 16, 2022 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants