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

[bradzacher]

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]

what’s your eslint config?

[bradzacher]

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

[ljharb]

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]

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.

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.

[ljharb]

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

[bradzacher]

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]

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.

[Parkreiner]

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

[tswaters]

I've hit this problem with the following:

<label>
  <div>
    <input />
  </div>
</label>

Seems like any non-text node children of labels can cause some issues here.

[ljharb]

@tswaters are you sure you're not configured to use both, ie, to require an id/for combo also?

[tswaters]

As far as I'm aware we're using recommended and the rule started giving false positives after an update.

In the code where we hit false positives, id/for are defined for everything. Without the div wrapping the input in the example, it works fine.... think of this as reduced test case 😁

[tswaters]

More details now that I'm back at work. Here's some sample code that raises the lint rule:

<input
    id={`${this.props.id}-time-picker`}
    name={`${this.props.id}-time-picker`}
    type="time"
    placeholder={this.props.placeholder}
    disabled={this.props.disabled || this.props.readOnly}
    className={`form-control time-input ${this.props.disabled ? 'disabled' : ''}`}
    onChange={this.handleTimeChangeFromInput}
    value={this.state.value ? moment(this.state.value).format('HH:mm') : ''}
    onBlur={this.handleBlurEventFromInput}
/>
<div className={`input-group-addon ${this.props.disabled ? 'disabled' : ''}`}>
    <label htmlFor={`${this.props.id}-time-picker`}>
        <span aria-hidden="true" className="rw-i rw-i-clock-o" />
    </label>
</div>

As a test, if I apply this diff, it doesn't raise the lint rule:

diff --git a/eos-web/components/js/common/components/DateTimePicker.jsx b/eos-web/components/js/common/components/DateTimePicker.jsx
index 51f17c8f16..89bbe216c2 100644
--- a/DateTimePicker.jsx
+++ b/DateTimePicker.jsx
@@ -340,7 +340,7 @@ class DateTimePicker extends PureComponent {
                                />
                                <div className={`input-group-addon ${this.props.disabled ? 'disabled' : ''}`}>
                                        <label htmlFor={`${this.props.id}-time-picker`}>
-                                               <span aria-hidden="true" className="rw-i rw-i-clock-o" />
+                                               This is just a text node, surely it won't work?
                                        </label>
                                </div>
                        </div>

So, this component was written in 2017 and it's been like that basically since it was written. We started linting it with a11y in ~2022-09-29 -- and between then and now, both are using v6.6.1 of this module.

I did run an npm update to cause this, maybe something has changed upstream? I looked at the diff of the npm update commit and the only thing that stood out to me was babel modules from 7.22 => 7.24.

eslint 8.24.0 @babel/core 7.24.7 babel-eslint 7.24.7 eslint-plugin-jsx-a11y: 6.6.1

[ljharb]

@tswaters ok so as I try to turn this into a test case, I see that it's erroring, with or without the div around the label.

Indeed, visible text content inside the label is required, otherwise it's not an accessible label. I don't think this is a regression; I think this is a bug fix that unfortunately introduced a lint warning for you.

[bradzacher]

I think what I mentioned here applies to their case as well:

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]

True, thanks for the reminder.

The entire point of this rule is "has accessible label", though - what's the point of a label element if there's no label text in it?

[bradzacher]

I see two separate concerns here: 1) a label is associated with an input 2) a label has accessible content

The two concerns are related to one another in the context of a11y - I.e. "an input must have a label with accessible content" - but ultimately the two concerns are separate and isolated. One can be true without the other and they can be validated independently.

IMO (1) matches with the name of the rule but (2) doesn't. The rule used to only check (1), but as of 6.8.0 it also checks (2). IMO (2) deserves a second rule to enforce such a constraint.

[ljharb]

Separate from organizational purity, what's the benefit of checking only the first? like, why would anyone want a label without accessible content to be present? (and why would the a11y linting plugin have that rule)

[tswaters]

IMO, the rule should be catching "oops! I forgot to add htmlFor to this label" and maybe some very basic structural checks like "this input has a label in parentNodes somewhere"

Correct me if I'm wrong, but when looking at just the AST, it wouldn't be possible to check what the underlying component actually renders?

[ljharb]

@tswaters you're totally correct, and if the children of the label have any custom (non-HTML) components, then we can't know for sure either way what they render (and thus shouldn't warn on them). In the case above, though, it looks like you only have the label for an icon, which is not accessible (because it relies on non-text affordances) - can you add a text label for the icon?

[bradzacher]

Because it is not possible to validate (2) statically. To do so requires cross-file analysis.

For example imagine you have the following:

// mod1.js
export function Custom() {
  return <>Accessible label</>;
}

// mod2.js
import {Custom} from './mod1';

export function Component() {
  return (
    <>
      <label htmlFor="1">
        <Custom />
      </label>
      <input id="1" />
    </>
  );
}

This code is 100% valid and accessible - but the rule errors on it because it cannot determine that <Custom /> renders something accessible.

Such a check may be desirable in some cases - I.e. To enforce that a11y constraints are declared in some statically analysable way - but many codebases would not want this check also and would be okay with assumptions.

It would be possible to add options to configure this behaviour - including turning it off. But at that point it does seem to be better as a separate rule entirely that can be opted-into.

[tswaters]

looks closely Oh shoot, that component isn't accessible at all, is it.... I'll need to double check how it behaves tomorrow when I'm back at work. I think I'm going to need to add "aria-label" to the span element. I'm pretty sure I had legit false positives though, that was just the first one I copied out 😂

[ljharb]

@bradzacher in your example, I agree the linter rule shouldn't be warning on it. There are plenty of cases it can statically know nothing is rendered, but there a great many like your example where it can't know, and thus shouldn't complain.