Eslint-plugin-jsx-a11y: Make default for anchor-is-valid both id and nesting

[evcohen]

To avoid breaking changes, we left it to default to just check htmlFor, but ideally it would default to both htmlFor and nesting. When the first necessary breaking change comes in, we should go back and update this.

[AlmeroSteyn]

@evcohen Should this be for label-has-for instead?

[evcohen]

yep- my bad was getting caught up on all of the changes and misread haha. thanks @AlmeroSteyn

[jessebeach]

That time is now! Let's get this in for a v6 release.

[jessebeach]

@AlmeroSteyn whomever of us gets to it first :)

[AlmeroSteyn]

@jessebeach Going to try and be the first, but my agenda for the coming two weeks may have other opinions about that hehe.

But, and this is probably a discussion that has already happened elsewhere, are we happy with the label-has-for rule?

Currently, it only checks if a label has a filled htmlFor prop or if a label contains any JSX element as content.

So the following will pass (I just added form to illustrate that the dangling labels pass by themselves):

<form>
   <label htmlFor='id-does-nog-exist' />
</form>

and

<form>
   <label>
       <div />
   </label>
</form>

It certainly increases the chance that labels are associated with form controls, however, it does not check whether form controls have an accessible name. Also, it does not check if the label contains any text at all.

What I do find worrying is that the following fails:

<label>
   {children>
</label>

That would certainly be a way I would want to use the nesting functionality combined with React components.

Now, with props spreading, componentization and such I can imagine that all these checks could be quite difficult to check properly without creating blocking scenarios. But I was wondering if this is something you would like to invest time in?

To switch on the nesting by default is no huge issue but, with it not catering for children, I don't think we can simply do that.

Opinions?

[jessebeach]

Now, with props spreading, componentization and such I can imagine that all these checks could be quite difficult to check properly without creating blocking scenarios.

I'm pretty convinced that we have a hard-ceiling with linting and I'm OK with at. It's not meant to be wholistic solution; it's a chapter in a quality story. There's only so much we can provide a go/no-go evaluation for at this level. My team's next project is to provide quality signal at the composition level.

<label>
   {children>
</label>

You're right that this shouldn't be failing. Logging a bug.

[AlmeroSteyn]

Yeah, also some wishful thinking on my part. First time actually writing linter rules instead of just using them.

The composition level check sounds awesome.

For the bug fix for children and just switching the default on I certainly do want to look at it. Will get something together as soon as I can.

[evcohen]

My team's next project is to provide quality signal at the composition level.

That's what I'm hoping accessibility-webpack-plugin can solve FWIW 🤷‍♂️

[jessebeach]

That's what I'm hoping accessibility-webpack-plugin can solve FWIW

Nice. We've not using Webpack unforch. I'll need to focus my efforts in our toolchain. Probably Jest.

[AlmeroSteyn]

See the bug is already picked up. Awesome!

Will revisit this issue when that is merged :-)

[AlmeroSteyn]

Still on my radar to do before the next major release. Should I continue to wait for #259 before doing this?

[evcohen]

yes, that sounds good or @mjaltamirano can loop that into his PR

[michaeljaltamirano]

@AlmeroSteyn have you started working on this?

I'm happy to tackle this as well (especially since I'm more comfortable with the rule).

Changing the required rule from const required = options.required || 'id'; to: const required = options.required || { every: ['nesting', 'id'] };, which is what I believe this change would entail, results in 16 tests failing.

[jessebeach]

@mjaltamirano please do go ahead and address this if you have time.

[michaeljaltamirano]

@jessebeach I'll take care of this early this afternoon. Everything looks fine but {children} is now throwing an error so I might have to make another slight change to that existing code.

[jessebeach]

but {children} is now throwing an error so I might have to make another slight change to that existing code.

Awesome, thank you!

[michaeljaltamirano]

Link to commit, updated to reflect using the object-assign package in the test file as suggested below in #259

Of note is the change in the docs (the Succeed case) as well as using objectAssign to test cases where we want to test for components like <Label> and <Descriptor> along with another rule. All tests until now had tested one rule at a time. Updating the default rule to be { "every": ["nesting", "id"] } broke the custom tag tests that listed array as the only option.

[ljharb]

you can use the object.assign package in tests or implementation if you need to.

[evcohen]

Fixed in #259 thank you @mjaltamirano !!!