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 fails for identical string values of id and htmlFor #511

Open
kevindice opened this issue Dec 14, 2018 · 15 comments

Comments

@kevindice
Copy link

The following code fails on jsx-a11y/label-has-associated-control. Changing id and htmlFor to string literals results in a pass.

const DotfilesCheckBox = (props: Props) => {
  const { id, showDotfiles, toggleDotfiles } = props;

  return (
    <div
      className="checkbox"
      style={{
        marginLeft: '1.0em',
      }}
    >
      <input
        checked={showDotfiles}
        id={`DotfilesCheckbox${id}`}
        type="checkbox"
        onChange={toggleDotfiles}
      />

      <label htmlFor={`DotfilesCheckbox${id}`}>Show Dotfiles</label>
    </div>
  );
};

I could see this failing by design for mutable identifiers, but since these are defined as const, it seems safe to conclude that they will evaluate to identical strings if they are in the same block scope. I also realize there are limitations to what can be done statically.

@ljharb
Copy link
Member

ljharb commented Dec 14, 2018

It's not really feasible for the linter rule to try to follow every variable in scope and try to figure out if it's the same or not.

What happens if you put the ID in a variable, and use the same variable in both places?

@kevindice
Copy link
Author

kevindice commented Dec 14, 2018

Thought that might fix it, but it still fails:

const DotfilesCheckBox = (props: Props) => {
  const { id, showDotfiles, toggleDotfiles } = props;
  const inputId = `DotfilesCheckbox${id}`;

  return (
    <div
      className="checkbox"
      style={{
        marginLeft: '1.0em',
      }}
    >
      <input
        checked={showDotfiles}
        id={inputId}
        type="checkbox"
        onChange={toggleDotfiles}
      />

      <label htmlFor={inputId}>Show Dotfiles</label>
    </div>
  );
};

@pjchender
Copy link

I have the same issue with the code below:

const Foo = () => {
  const domId = 'foo';

  return (
    <div>
      <label htmlFor={domId}>Foo</label>
      <input type="text" id={domId} />
    </div>
  );
};

@himynameisdave
Copy link

Yep, same issue. Contrary to @ljharb's suggestion, I actually don't think it's that unfeasible for the linter to be able to handle this, since it's a somewhat common use-case. I will start on a PR.

@ljharb
Copy link
Member

ljharb commented Jan 29, 2019

@himynameisdave note i said “every” :-) a PR that can catch more cases would be very appreciated!

@kevindice kevindice changed the title label-has-associated-control doesn't match identical string template literals label-has-associated-control fails for identical string values of id and htmlFor Jan 30, 2019
@jessebeach jessebeach added this to To do in Deque aXe Hackathon at CSUN via automation Feb 11, 2019
@JESii
Copy link

JESii commented Aug 22, 2019

And I have an even simpler case where linting fails:

     <PopupContent>
        <label htmlFor="popup" style={{ margintop : '0px', marginbottom : '0px' }}>log out</label>
        <button type="button" id="popup" onClick={this.logUserOut} />
      </PopupContent>

Errors:

84:9  error  A form label must be associated with a control  jsx-a11y/label-has-associated-control
 85:9  error  A control must be associated with a text label  jsx-a11y/control-has-associated-label

@ljharb
Copy link
Member

ljharb commented Aug 22, 2019

Nest the input in the label.

@JESii
Copy link

JESii commented Aug 22, 2019

Nope; doesn't solve the problem.

@ljharb
Copy link
Member

ljharb commented Aug 23, 2019

ah, maybe because it's a button. buttons don't typically have labels because they contain their own CTA text, but maybe they should be considered? cc @jessebeach

@jeroenpeeters
Copy link

I have a very similar issue. With version 6.2.3 of the plugin the following code reports an error;

   <form>
      <label htmlFor="id1">Postalcode</label>
      <input
        id="id1"
        value={zipcode}
      />
    </form>

40:7 error A form label must be associated with a control jsx-a11y/label-has-associated-control

Please don't respond that I need to wrap the input in the label. I don't want this. In fact, my actual code is more complex, but even this simple example doesn't work

@ljharb
Copy link
Member

ljharb commented Sep 24, 2019

@jeroenpeeters is your rule configured to allow the more lax arrangement of not properly wrapping the input in the label? By default i believe it requires both wrapping and linking.

@jeroenpeeters
Copy link

jeroenpeeters commented Sep 24, 2019

@ljharb That would be strange, because the documentation (https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/label-has-associated-control.md) clearly states that;

  • This rule checks that any label tag either wraps an input element
  • or has an htmlFor attribute and that the label tag has text content.

The rule is not explicitly configured, so it must be defaults then.

@ljharb
Copy link
Member

ljharb commented Sep 24, 2019

Are you extending any shared configs, like airbnb, that might be setting it?

@nth-chile
Copy link

Having the same issue with the following code. I didn't think it was an HTML requirement to wrap inputs with labels -- more like an option? So maybe it should be an option in this linter rule as well

<label htmlFor="name">Name</label>
<input
  className="form-control"
  id="name"
  type="text"
  onChange={(e) => setName(e.target.value)}
  value={name}
 />

@ljharb
Copy link
Member

ljharb commented Nov 14, 2019

@nth-chile it's a requirement if you want to support 100% of assistive devices.

It is an option for this linter rule, but the default is to do both. The second-best option is to nest, since the majority of devices support that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

7 participants