Jest-dom: Throw if non-unique id for label `for` attribute

[skovy]

Describe the feature you'd like:

Similar to getBy* queries throwing if there is more than one element that matches, throw if more than one input control id exists for a given label's for attribute.

This is a fairly specific feature request but I could imagine this being more generally done on render, validating the entire document has only unique ids. This would remove the need to do this within the ByLabelText query and would catch many more issues (not only with labels). However, the rest of this is focused on the label example.

Basic Example

Today, a document like this would return only the first input element for getByLabelText("1st").

    <div>
      <label for="not-a-unique-id">1st</label>
      <input id="not-a-unique-id" />
      <input id="not-a-unique-id" />
    </div>

Realistic Example

The above example is contrived, a more realistic example would be forgetting to unique-ify the id in a list.

import React from 'react'
import {render} from '@testing-library/react'

const Example = () => {
  return (
    <>
      {[0, 1, 2].map(i => (
        <React.Fragment key={i}>
          <label htmlFor="input">#{i}</label>
          <input id="input" /> // same id is used 3 times, probably should interpolate `i`
        </React.Fragment>
      ))}
    </>
  )
}

test('is fine today', () => {
  const {getByLabelText} = render(<Example />)

  // Returns the first input, but there are 2 other inputs with the same label
  const input = getByLabelText('#1')
})

Suggested implementation:

At an initial glance, I was thinking this may belong in the queryAllByLabelText branch for the for attribute. If changing querySelector to querySelectorAll and validating the length is 1 before returning this could help validate exactly one input matches the label's for.

As a proof of concept of this approach, I've created a branch: https://github.com/testing-library/dom-testing-library/compare/master...skovy:skovy/unique-label-for-id. Note, the label.control is commented out because it only returns the first input.

Describe alternatives you've considered:

Based on the fact that jsdom support label.control it seems like jsdom may be the better place for this to live base on the label.control logic.

Support for label.control in jsdom was added in https://github.com/jsdom/jsdom/pull/2217. The logic for label.control behavior currently exists here.

Going back to the more general solution of erroring on any duplicate id may also be better done in jsdom? It looks like v0.2.11 added support for allow non-unique element ids in https://github.com/jsdom/jsdom/commit/ac0fe28a939bb61af69ee79fe5735da3d4a7ee28.

Teachability, Documentation, Adoption, Migration Strategy:

• Similar to other error messages, this should help guide users to ensure they have only unique id when testing. It doesn't guarantee only unique ids _(if you only test one element in the example)_.
• It could be noted in the document, but a good error message seems sufficient to only provide information when needed.

[eps1lon]

If we decide to validate the tree I would rather throw if any duplicate id is found. This affects any attribute that relies on IDREFs (aria-activedescendant, list etc).

[kentcdodds]

I think that DOM validation is one thing that would be good to have (like accessibility validation as well), but is probably not something we can or should bake into the library at this point.

I would rather have some external library that does some automatic checking of the DOM as changes are made and people can install that and just add it to their setup-env.js file.

I don't think that this is something we're going to do here. But I am open to hearing other suggestions.

In any case, I think this would be a breaking change, and I don't think I'm prepared to include it in the one we're doing now.

[alexkrolick]

I was thinking at some point expect(dom).toBeValidDOM() & expect(dom).toBeValidAOM() (or something like that) would be good to add to jest-dom.

[kentcdodds]

That would be great!

I would love it even more if it just happened automatically via Mutation Observer, but I tried that and it was not easy, so having an assertion would be a good first step 👍👍

[skovy]

Thanks for all the responses! I agree that a more general approach to validating the DOM would be better than the very narrow feature request originally proposed in this issue.

Is it possible to move this issue to the jest-dom repo to continue the discussion or should I close/re-open a new issue there?

[gnapse]

Hi everyone. Given that this landed from another repo, is there an actionable here? The original proposal no longer applies, since it proposes addressing the issue from the DOM Testing Library POV. What would the actionable be in terms of custom assertions?

[alexkrolick]

• toBeValidDOM general DOM validation rules
• toBeValidAOM/toBeAccessible something like that, if not included in the DOM validation automatically

I'd say you could do these automatically in afterEach, but teardown time is probably too late to be useful

[gnapse]

Sill too vague (though sorry if I'm missing something):

1. What exactly is toBeValidDOM supposed to validate? I get the IDREFs part, but what else? And even in the IDREFs side of thing, would be nice to have a list of all the cases.
2. What exactly is toBeAccessible supposed to validate? I wonder if this (and toBeValidDOM) are scoped too generally, or their name to vague. For instance, there are plenty of misuses of aria- attributes that do not follow the specs thoroughly, and components end up being not really accessible. Do we really want to name a custom matcher like this, implying that if it passes, we developed accessible components and code?

Also relevant is the discussion in #55, in particular this comment https://github.com/testing-library/jest-dom/issues/55#issuecomment-418485227 where I was proposing something like it, and in that discussion jest-axe was mentioned as a much more comprehensive and already existing tool for that.

Which makes me think that that it could be worth checking if jest-axe checks the thing you were originally reporting @skovy. Check out out https://github.com/nickcolley/jest-axe

[skovy]

Thanks for sharing @gnapse! It looks like they do have several rules around duplicative ids:

• https://github.com/dequelabs/axe-core/blob/develop/doc/rule-descriptions.md
• This rule links to https://dequeuniversity.com/rules/axe/3.5/duplicate-id?application=RuleDescription

I think this solves my need. The only disadvantage is that it took opening an issue, discussion with several folks until I came across this (maybe due to my poor googling). I was thinking it would be nice to have something like this "just work" to continue encouraging folks to create accessible applications but I don't have any good ideas on how to approach that. Maybe adding to the Testing Lib docs as a "hey, this thing exists"?

Given there's no clear next step I'm fine with this issue being closed 👍 Thanks for all the assistance everyone!

[gnapse]

Good points. Would you be up to add a note about jest-axe in the README. It could be mentioned in the "Other solutions" part of the README as something that's not overlapping with jest-dom but can complement it for more in-depth accessibility checking.

If not that's ok, I can do it myself. Let's leave this issue open to keep track of having that done.

[gnapse]

@skovy I think we can close this after #226. Thanks!