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

BaseControl should require an id or create one if missing #6989

Closed
tofumatt opened this issue May 28, 2018 · 3 comments · Fixed by #14151
Closed

BaseControl should require an id or create one if missing #6989

tofumatt opened this issue May 28, 2018 · 3 comments · Fixed by #14151
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality

Comments

@tofumatt
Copy link
Member

Is your feature request related to a problem? Please describe.
As mentioned in #6922, it's possible to create a BaseControl element that outputs a <label> not associated with any element. This causes issues like #6918. This kind of disassociated label markup is frustrating and a personal pet-peeve of mine online. We should strive never to make a label that is unassociated 😆

Describe the solution you'd like
Make id a required prop of BaseControl such that omitting it causes a linting error. And/or create an internal ID automatically if an explicit one isn't supplied, so the label of a BaseControl component gets linked to its single child element (if it only has one).

Describe alternatives you've considered
My review in #6922 mentions that education here is not enough, as this is easily omitted. (Especially when the label prop is supplied without an iD).

It might be that a default ID being created if none is supplied would (also) work.

@tofumatt tofumatt added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality labels May 28, 2018
@afercia
Copy link
Contributor

afercia commented May 28, 2018

Worth reminding an explicit label / field association (using for/id attributes and not wrapping the form field) is required for any new code landing in WordPress and it's part of the Accessibility standards: https://make.wordpress.org/core/handbook/best-practices/coding-standards/accessibility-coding-standards/#labeling

@tofumatt
Copy link
Member Author

Seems like #6098 is also related.

@afercia
Copy link
Contributor

afercia commented Feb 26, 2019

There's now a proposal by @aduth that seems to be the most appropriate one, see #9802 (comment)

It would be fairly trivial to create an ESLint rule which forbids a BaseControl that includes label prop but omits id.

Then, special cases could be addressed just passing other content as child, as outlined in the proposal (please read all the comment there).

Agreed it would greatly help to avoid the scenario of unlabelled inputs in the future, see a recent case in #13894. At this point, it probably just needs a PR 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
3 participants