Skip to content

fix: add a11y configuration for checking through control and input component #7774

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.

Sign up for GitHub

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

Jump to bottom
Closed
tanhauhau wants to merge 5 commits into from
Closed

fix: add a11y configuration for checking through control and input component #7774

tanhauhau wants to merge 5 commits into from

Conversation

@tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Aug 9, 2022

Fixes #6469

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

baseballyama
baseballyama reviewed Aug 14, 2022
View reviewed changes
Copy link
Member

@baseballyama baseballyama left a comment •
edited
Loading

Choose a reason for hiding this comment

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

How about adding a test for <svelte:component> as well?

@tanhauhau tanhauhau force-pushed the tanlh/gh-6469 branch 2 times, most recently from cdb1266 to 80e7556 Compare October 16, 2022 03:04
dummdidumm
dummdidumm approved these changes Feb 23, 2023
View reviewed changes
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Code looks good to me and I would be in favor of merging, though I want to wait on the opinion of other maintainers here since it's a first / gives precedence for other such rule options.

@vercel
Copy link

vercel bot commented Feb 23, 2023

@dummdidumm is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

dummdidumm
dummdidumm reviewed Feb 23, 2023
View reviewed changes
site/content/docs/06-accessibility-warnings.md Outdated

- `labelComponents` is a list of Svelte component names that should be checked for an associated control.
- `controlComponents` is a list of Svelte component names that will output an input element.
- `depth` (default 3, max 25) is an integer that determines how deep within the label element the rule should look for an element to determine if the label element has associated control.
Copy link
Member

@dummdidumm dummdidumm Feb 23, 2023

Choose a reason for hiding this comment

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

previously the depth was unlimited. Are we confident with 3 being a right number which doesn't turn up false positives for people previously having no warning?

Copy link
Member Author

@tanhauhau tanhauhau Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i would say it's a safe enough number, or what number would u suggest?

Copy link
Member

@dummdidumm dummdidumm Feb 27, 2023

Choose a reason for hiding this comment

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

If I were to design this from scratch I'd also take 3, it's more about people not seing a warning now which they previously didn't. But having such a deeply nested input is probably super rare. Maybe I'd go with 5.

Copy link
Member Author

@tanhauhau tanhauhau Feb 27, 2023

Choose a reason for hiding this comment

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

sure, i dont have a strong opinion on any number

@dummdidumm dummdidumm added this to the one day milestone Mar 1, 2023
@dummdidumm
Copy link
Member

dummdidumm commented Mar 1, 2023

Not merging for v3 or v4 to give us room to think about the whole thing more deeply for v5

@benmccann benmccann changed the title [fix] add a11y configuration for checking through control and input component fix: add a11y configuration for checking through control and input component Mar 14, 2023
@ByteAtATime
Copy link

ByteAtATime commented Apr 12, 2024

Hey, is there a reason this PR isn't merged yet? I feel that it would be very useful in situations where inputs need custom input validation or functionality.

@Rich-Harris
Copy link
Member

Rich-Harris commented Dec 3, 2024

In Svelte 5, we skip the warning if a label contains a snippet or a component, since they could contain a control. As such I'll close this

@Rich-Harris Rich-Harris closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dummdidumm dummdidumm dummdidumm approved these changes

@baseballyama baseballyama baseballyama left review comments

Assignees

No one assigned

Labels

a11y

Projects

None yet

Milestone

one day

Development

Successfully merging this pull request may close these issues.

a11y-label-has-associated-control error for control inside of component

5 participants

@tanhauhau @dummdidumm @ByteAtATime @Rich-Harris @baseballyama