Eslint-plugin-jsx-a11y: `label-has-for` false positive when using `allowChildren`

[amannn]

When using this rule:

"jsx-a11y/label-has-for": [2, {
  "required": {"every": ["nesting"]},
  "allowChildren": true
}]

This code passes the linter check:

<label>Firstname</label>
<input type="text" name="firstname" />

Currently it passes due to case 'Literal': return Boolean(child.value); in hasAccessibleChild I think.

Or am I missing something?

[ljharb]

This should definitely fail. I assume if you set allowChildren to false, it fails as expected?

[amannn]

Yep, that's correct.

[ljharb]

This was added in #259 by @mjaltamirano - do you have any thoughts on how to fix this?

[michaeljaltamirano]

Hey y'all, sorry for the inconvenience. I'll check this first thing tomorrow morning. I suspect it is what @amannn pointed out, in hasAccessibleChild. When I initially wrote #259 I didn't use it and checked specifically for a JSXExpressionContainer. That change might fix it, and I'll add additional test cases to check for that, too.

[evcohen]

@mjaltamirano agreed- that's my bad for pointing you in this direction. we should be explicitly checking for a child element that's not Literal. sorry about that 😞 and thanks for fixing!

[amannn]

No worries, thanks for your help! I'm wondering though what the right heuristic might be.

By looking at hasAccessibleChild, I think the following currently pass despite not being accessible (not tested though):

// This is the one already mentioned and should be easy to identify
<label>Firstname</label>
<input type="text" name="firstname" />

// Maybe can be prevented by deep traversing the nodes and
// looking if there's an interactive element. If not and there
// is no `htmlFor` prop on the label this is probably an error.
<label><span>Firstname</span></label>
<input type="text" name="firstname" />

// This is pretty hard to identify.
const labels= {firstname: 'Firstname'};
<label>{labels.firstname}</label>
<input type="text" name="firstname" />

// This is also hard to identify
<label><SomeComponent /></label>
<input type="text" name="firstname" />

I think the latter two are hard to identify, since they shouldn't lead to false negatives like:

<label>{this.renderInput()}</label>
<label>{this.props.children}</label>
<label><Input /></label>

I'm wondering if there's a good way to identify some of these patterns.

[michaeljaltamirano]

Please see #299 for a potential fix.

Using a new function, validateChild, in place of hasAccessibleChildren should fix some of the concerns in the test cases @amannn noted. It checks specifically for a JSXExpressionContainer that read {children} or {this.props.children}. Since it checks specifically for a 'JSXExpressionContainer', JSXElements like fail the property check.

Would love a quick review if anyone sees anything that looks incorrect!

[jessebeach]

I'd like to address this issue with a new rule that would replace label-has-for: #326

[ljharb]

I would still want to enforce what label-has-for checks, as-is - i like the new rule, but it wouldn't be a replacement imo unless i could configure it that way.

[jessebeach]

@ljharb, label-has-for is checking a technical detail (personal opinion alert), rather than an intent, which is that a label is associated with an input. It's impossible(-ish) to check the inverse because of the one-way relationship of htmlFor.

unless i could configure it that way

@ljharb, this might be compromise here. What behavior are you referring to? I can probably update label-has-associated-control to include this configuration option.

[ljharb]

label-has-for has "nesting", "id", and allow for both "some" and "every" semantics. I explicitly want a linter rule that has a hardcoded label (or configurable custom alternative) and a hardcoded input (or configurable custom alternative) placed inside that label and/or linked by htmlFor/id (as configured). I don't want to lightly check for "a11y potential", I want to strictly allow labels and inputs in only this tiny subset of use cases.

[jessebeach]

label-has-for has "nesting", "id", and allow for both "some" and "every" semantics.

For me, the "some" and "every" semantics are completely inscrutable. Are they necessary? It seems like the necessary config is 'id', 'nesting' or 'both'.

[ljharb]

Yes, it's critical to allow either id or nesting. For one, every version of Dragon (a11y software) supports only for/ID-linking. Separately, if someone has two inputs inside a label, it's necessary to be able to disambiguate which input the label points to (bad practice aside); also, if someone has two labels pointing to the same input, the non-wrapping one needs to be for/ID-linked, and it shouldn't run afoul of the rule.

[jessebeach]

if someone has two inputs inside a label

I need to check for this in label-has-associated-control. I'll update it.

if someone has two labels pointing to the same input

Easy to check for, I'll add it.

Seems like adding a config for htmlFor, id, both would satisfy the use case of Dragon needing htmlFor.

And I think we can drop the every and some semantics.

[ljharb]

I still want the some/every configurability; if someone supports Dragon, they need "id" but might still want to enforce "nesting", if someone doesn't support Dragon, they might want to allow for either.

In other words, "id", "nesting", "both", or "either" are required; the current some/every mechanics allow for future extensibility if a third thing is added.

[jessebeach]

the current some/every mechanics allow for future extensibility if a third thing is added.

@ljharb Aha, I see what you mean. I'd like to push back a little:

1. A "third-thing" would require an update to how HTML works -- either a new attribute or method of associating a label to an input. An event like that would warrant a large breaking change to a rule like this. I may not be aware of some mechanism that you have in mind.
2. I'd like to eliminate the two independently varying options that affect each other. every means nothing unless both id and nesting are also specified. The interdependence is confusing.
3. ["id", "nesting", "both", or "either"] seem like a complete set to achieve the behavior to satisfy the use case you describe.
4. I'd like to have one rule for input labels, that's why I'm pushing so hard on this. Two rules with slightly varied behavior will be confusing for end users of the plugin. I think the feedback here is helping me construct a better rule, but it still needs some tweaking. If you agree with point number 1 above, then I think we can move forward. I don't disagree with your intent, just the mechanics of the config to achieve it with simple user-facing options.

[ljharb]

I agree with point number 1 :-) the choice of all 4 options (perhaps with better names: [id, nesting, id-and-nesting, id-or-nesting]?) would suffice for me (provided that the allowChildren option could be disabled, whatever that means for the new rule's configuration)

[jessebeach]

provided that the allowChildren option could be disabled, whatever that means for the new rule's configuration

@ljharb understood, I'll make it happen and add the test cases.

[jessebeach]

Please use label-has-associated-control instead of label-has-for.