Add toBeChecked for <input type="radio" /> and <input type="checkbox" />

[testing-library]

Add toBeChecked for and

[Belco90]

I almost opened the same issue but I found this one, so leaving a comment here. I also found toBeChecked assert really interesting and I'm not sure if .toHaveAttribute('checked', true) covers all the cases.

I rewrote @kentcdodds tests in 2 different ways:

describe('input checkbox', () => {
  it('should handle attribute checked', () => {
    const input = document.createElement('input');
    input.setAttribute('type', 'checkbox');
    input.setAttribute('checked', true);
    expect(input).toHaveAttribute('checked', 'true');
    input.setAttribute('checked', false);
    expect(input).toHaveAttribute('checked', 'false');
  });

  it('should handle property checked', () => {
    const input = document.createElement('input');
    input.setAttribute('type', 'checkbox');
    input.checked = true;
    expect(input).toHaveAttribute('checked', 'true');
    input.checked = false;
    expect(input).toHaveAttribute('checked', 'false');
  });
});

The first one pass but the second one doesn't as it needs to check that input.checked === true. I don't know if toHaveAttribute was covering this case before and it doesn't now, or if I'm missing something else. Would make sense now to add toBeChecked assertion?

Thanks.

[kentcdodds]

Yes I do. Because the code is more communicative and the error message would be more helpful 👍

[gnapse]

Actually, I have thought of an extra functionality that this custom matcher may have that could make it more useful: it could check also for aria-checked.

More concretely, it should check either that

1. The element is a input of type checkbox or radio and in that case it should use the .checked property to determine if it passes or not.
2. The element has a role of checkbox, option, radio or switch, and in that case it should use the aria-checked attribute to decide if it passes or not.

Not sure what should it do if the element is none of the above cases. Maybe also fail with a different message?

[lourenci]

I could handle this. :)

[kentcdodds]

Both 👍 because what you different works there's not enough justification to add to the surface area of the assertion library

[kentcdodds]

Wouldn't need toBeUnchecked() because you can do .not.toBeChecked() 🤷‍♂️

[Belco90]

I agree with that logic. Makes plenty of sense to me

@kentcddods Absolutely, but I think it proves toBeChecked matcher is worth to be added.

@gnapse If you finally decide to implement the matcher, I'm happy to help.

[gnapse]

@Belco90 Like I said before, it's not for me to decide wether we add it or not, but what turns out of the discussion here. It seems we're leaning towards it, what Kent and others have said make sense (although I still think it's kinda a syntactic sugar, but ok). If you mean you're up to providing a PR, please do so.

[kentcdodds]

I definitely see this as an opportunity for us to help people if they decide they want to write a custom checkbox 👍

[gnapse]

I agree, however, with a caveat:

The thing is, that's a bit tricky. Because in not all situations the thing with aria-checked should be focusable. For example, a <div role="option" /> need not be focusable, and be part of a combobox where the input keeps focus, and uses aria-activedescendant to declare what option in the combobox listbox is the one "focused" right now.

All I'm saying is, this, if done, has to be done right and checking out the spec. Not blindly assuming that a div with a role has to have a tabindex.

[kentcdodds]

I suggest that we get a simple implementation released first, then we add the aria support later

[kentcdodds]

I don't see why it would need any changes

[gnapse]

🎉 This issue has been resolved in version 4.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[lourenci]

It would be nice. Will you handle this?

[kentcdodds]

As much as I'd love to do it myself, I don't know if I have the bandwidth at the moment. Just wanted to toss it in as an idea :)

[gnapse]

Hmmm, although I like the descriptiveness of this, this kinda messes up with the additional guiding principle of jest-dom:

this library aims to maintain a minimal but useful set of them, while avoiding bloating itself with merely convenient ones that can be easily achieved with other APIs. In general, the overall criteria for what is considered a useful custom matcher to add to this library, is that doing the equivalent assertion on our own makes the test code more verbose, less clear in its intent, and/or harder to read.

It could be argued that this is easier to read though.

However, wouldn't this work?

expect(input).toHaveAttribute('checked', true)

[kentcdodds]

Fair point 👍 I'm convinced.

[gnapse]

@kentcdodds but because of the guiding principle? Or because the line I suggested works?

[kentcdodds]

I agree with that logic. Makes plenty of sense to me

[gnapse]

Yes, it makes sense, although now I wonder what really makes a checkbox be checked? Is it the attribute or the .checked value? (a quick smoke test in my browser console seems to suggest the .checked property is the one that's taken into account). However, when I set the attribute to true for a new checkbox, the .checked property turned true, but when I set the attribute to false, the .checked property remained true 🤔

And to make matters worse, there's also .indeterminate (ref, ref). Does that mean we should add .toBeIndeterminate()?

I still think that, although a bit less declarative, if we add a .toBeChecked() that is simply a replacement for writing expect(checkboxElement.checked).toBe(true) is not worth it.

However, if we figure out all the needs around checking a checkbox state (the ambiguity between the state of the checked= attribute and the .checked DOM property) and maybe also bring .indeterminate into the mix, and provide a custom matcher that would hide away all the complexity of checking a checkbox state (even if it's not really complex, but at least more than just checking if something is true) I'd me more willing to consider it.

That being said, of course I don't mean to say that mine is the last word. Just my 2 cents. If this is just a replacement for expect(checkbox.checked).toBe(true) for me is not worth it, and would love to hear from others if it is.

[kentcdodds]

Also, I don't think that we need to support things that a tiny fraction of people would use "just for completeness." I hadn't even heard of the .indeterminate state before today. 🙃

[gnapse]

Wouldn't need toBeUnchecked() because you can do .not.toBeChecked()

Yes, I removed that after my initial comment posting because it was not fair. You seem to have been able to see it before I edited my comment to remove it. I agree with you.

You still haven't addressed my main question (on which I am honestly not fully decided): do you think it's worth it to have a custom matcher that simply replaces a check for a value being true? A value that's readily available?

[kentcdodds]

Love that suggestion!

[weyert]

Brilliant!

[afontcu]

I feel this is a nice addition to the library 👍

Maybe also fail with a different message?

What about pointing out that the element check property cannot be asserted due to missing right role and type attributes?

A question that comes to my mind, though, is how far should jest-dom should go in terms of accessibility. For instance, a "fake" checkbox such as <div role="checkbox" aria-checked="false"> might require a tabindex attribute, too. I guess this is not the right issue to discuss that, but it's quite related the minute we add ARIA attributes to the mix.

[Belco90]

If this toBeChecked is finally implemented, should affect others matchers like toHaveFormValues?