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

[label-has-associated-control] regression - rule errors when a label does not directly have text, even if it has htmlFor #966

Open
bradzacher opened this issue Nov 14, 2023 · 8 comments

Comments

@bradzacher
Copy link

bradzacher commented Nov 14, 2023

function Foo() {
  return (
    <>
      <label className={styles.linkLabel} htmlFor="1">
        <CustomText />
      </label>
      <input id="1" />
    </>
  );
}

In 6.7.1 this code was fine and did not error.
In 6.8.0 this code results in an error.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2023

what’s your eslint config?

@bradzacher
Copy link
Author

Sorry forgot to mention - using the default config for the rule with no settings.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2023

then in that case, I'd assume CustomInput isn't recognized as an input, which means the label does not have an associated control, and the new error is a bug fix.

If you change your settings so that CustomInput is treated as an input, I'd expect the rule to no longer warn.

@bradzacher bradzacher changed the title jsx-a11y/label-has-associated-control error with a variable htmlFor jsx-a11y/label-has-associated-control regression - rule errors when a label has a custom child Nov 14, 2023
@bradzacher
Copy link
Author

Sorry, I just revisited to do an isolated repro outside our repo and have updated the example in the OP to match the correct repro case.

screenshot of example in OP within VSCode

The change in behaviour occurs when you specifically have a custom component as the child for the label. In that case the rule (as of 6.8.0) ignores the htmlFor.

@bradzacher bradzacher changed the title jsx-a11y/label-has-associated-control regression - rule errors when a label has a custom child [label-has-associated-control] regression - rule errors when a label has a custom child, even if it has htmlFor Nov 14, 2023
@ljharb
Copy link
Member

ljharb commented Nov 14, 2023

In that case, you have two conflicting mechanisms - nesting and for/ID. What do browsers do in that case?

@bradzacher
Copy link
Author

bradzacher commented Nov 15, 2023

The code will only produce invalid HTML if you assume CustomText renders an input element. In all other cases it's technically valid.
EG the rendered HTML that browser sees could easily be something like <label htmlFor="1"><span>text</span></label><input id="1" /> - which is valid.

The rule also works fine if the CustomText element accepts a string child, eg this code does not error.

function Foo() {
  return (
    <>
      <label className={styles.linkLabel} htmlFor="1">
        <CustomText>Text</CustomText>
      </label>
      <input id="1" />
    </>
  );
}

So the rule does make some assumptions (i.e. there's no requirement that a component renders children).

From what i can tell - there's also no way around this error either. There's no config option to tell the rule "hey this component renders text it's okay" - the only way to satisfy the rule is to satisfy this utility function's checks - i.e. nested no more than depth layers deep you must have either:

  • a JSXExpressionContainer as a child,
  • JSXText as a child, or
  • a configured labelling prop on a child.

To summarise the problem:

  1. as of 6.8.0 the rule has started enforcing that the above constraint strictly where it didn't before
  2. the error message when this constraint is violated reads as A form label must be associated with a control. - which is very misleading.
  3. there is no way to opt-out of this new behaviour at all to configure an allowlist of "components that render valid label children" or similar.

I would propose that the whole "has accessible label" check be moved to its own rule entirely - it seems very weird that a rule entitled "label has associated control" also enforces that the label has valid text too.

@ljharb
Copy link
Member

ljharb commented Nov 15, 2023

Hmm, interesting problem.

I agree that since a custom component may not render its children, that a custom element with a text child can't be statically assumed to render text - that's a bug in that utility, and it should be changed to not have that false assumption.

I also agree that "label has associated control" should only be concerned with a label and a control, not whether either of those things is accessible.

I'd appreciate PRs for either or both of those issues.

@bradzacher bradzacher changed the title [label-has-associated-control] regression - rule errors when a label has a custom child, even if it has htmlFor [label-has-associated-control] regression - rule errors when a label does not directly have text, even if it has htmlFor Nov 15, 2023
@Parkreiner
Copy link

Sorry for pinging this issue! We're just encountering issues with our CI/CD, and I wanted to link a similar issue for documentation. Still ruling out whether the issue is on our end

lipemat added a commit to lipemat/eslint-config that referenced this issue Apr 10, 2024
Version 6.8.0 has false positives

jsx-eslint/eslint-plugin-jsx-a11y#966

A form label must be associated w ith a control jsx-a11y/label-has-associated-cont rol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants