testing-library / jest-dom

:owl: Custom jest matchers to test the state of the DOM
https://testing-library.com/docs/ecosystem-jest-dom
MIT License
4.4k stars 392 forks source link

Support for matching pressed state of toggle buttons #203

Open bfintal opened 4 years ago

bfintal commented 4 years ago

Describe the feature you'd like:

Add a matcher for aria-pressed for checking button toggle states.

Suggested implementation:

A new matcher called toBePressed

Describe alternatives you've considered:

An alternative solution would be to check for the attribute aria-pressed="true" or aria-pressed="mixed"

Teachability, Documentation, Adoption, Migration Strategy:

Docs entry:


toBePressed

toBePressed()

This allows you to check if a form element is currently pressed.

An element is pressed if it is a button having a aria-pressed="true" or aria-pressed="mixed" attribute.

Examples

<button data-testid="pressed-button" aria-pressed="true" />
<input data-testid="pressed-input" type="button" aria-pressed="true" />
<div data-testid="pressed-div" role="button" tabindex="0" aria-pressed="true" />
<button data-testid="pressed-button-mixed" aria-pressed="mixed" />
expect(getByTestId('pressed-button')).toBePressed()
expect(getByTestId('pressed-input')).toBePressed()
expect(getByTestId('pressed-div')).toBePressed()
expect(getByTestId('pressed-button-mixed')).toBePressed()
gnapse commented 4 years ago

To be honest, this seems to be not too justified. The alternative way of checking it using .toHaveAttribute should be enough.

I'm open to hear counter-arguments, in case I'm missing something. But if it's just a replacement for the corresponding .toHaveAttribute check, I'm not sure we'll provide it.

jmvtrinidad commented 3 years ago

adding a matcher that checks the accessibility of an element makes it easier for the dev to adopt and read test that has accessibility on the component.

gnapse commented 3 years ago

@jmvtrinidad I hear you. However, I am torn between what you said and an explicit goal in our own guiding principles stated in the README:

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.

We need to draw the line somewhere. Our matchers need to do something that substitutes a cumbersome way of checking. toHaveAttribute already does that, and seems a perfect fit for this job.

If we go down this path suggested in this issue, then where does it end? If you go and check a list of existing aria attributes, then you'll see that we can soon end up having a myriad of custom matchers that are mere sugary syntax replacement for toHaveAttribute. Imagine having toHaveValueMin, toBeExpanded, toBeSelected, toBeLabelledBy, toBeDescribedBy, etc.

To illustrate the difference even further between what I'd consider good candidates over mere conveniences, consider a hypothetical matcher that I'd like and haven't made the time to propose or implement: toHaveControlOf('elementId'). This matcher would do two things:

  1. check the element's aria-controls to see if it has the given 'elementId'
  2. check that an element with that id actually exist

If this matcher would perform only the step (1) above, then I would not consider it worthy of implementing. If you wanted to check that the attribute is there, toHaveAttribute is enough. Doing the second step actually adds some value and makes a check that would otherwise be cumbersome to do in userland.

Hope this illustrates a bit the difference and what the reasoning is behind the objections to add a matcher such as toBePressed.

jmvtrinidad commented 3 years ago

@gnapse The example you have given totally makes sense, I will stop looking and use the toHaveAttribute. And maybe if someone still wants to do this maybe it can have its own library that focuses on checking the accessibility attributes with the sugary syntax.

gnapse commented 3 years ago

Thanks. I'm going to go ahead and close this, which does not preclude the conversation to continue if someone still objects.

Conclusion: using toHaveAttribute is a good enough substitute for this purpose.

Sawtaytoes commented 2 years ago

I understand wanting to make those library simpler and wanting to draw the line, but I wanna counter your argument with a bit more detail.

Imagine having toHaveValueMin, toBeExpanded, toBeSelected, toBeLabelledBy, toBeDescribedBy, etc.

None of those are necessary because jest-dom already has more meaningful alternatives:

I also understand why we have some of the current helpers:

Why I think toBePressed and toBeSelected make sense:

  1. toBePressed is extremely similar to toBeChecked except that it's only able to be defined with an ARIA attribute. If toBeChecked also covered the button's pressed state, I wouldn't be complaining. Although, checked has a different meaning because it includes "mixed" whereas "pressed" is only true-false.
  2. toHaveValue only covers <select>, it doesn't make sense for things that are role: gridcell, option, row, or tab (a role of "tab" is very common in applications).
  3. "Pressed" and "selected" are common states for a toggle or picker input. Those inputs don't always correspond to an HTML <input> or <select> either. You'll find them as custom components with role button, option, tab, radio, checkbox, and switch. They're either gonna be checked, selected, or pressed.
  4. Doesn't make sense to have toBeChecked when it covers only some of the types of pickers. It's very confusing to the point where I would incorrectly assume toHaveAttribute for the checked state.
  5. I get that this is a special case because it's only supported by ARIA; although, isn't a button also in a pressed state from its CSS :active state? I'm asking; not sure. It's a temporary UI state, but still "pressed".
  6. Pretty sure we're promoting bad coding practices for boolean states:
    .not.toHaveAttribute(
    'aria-pressed',
    'true',
    )
LucianHij commented 7 months ago

I support adding the toBePressed functionality. If there is a need, I can code it and submit a PR @gnapse.

gnapse commented 7 months ago

Thanks for insisting. Indeed, I've changed my mind.

I now see value in this matcher due to the fact that it should also check if the element in question is a button. So the key here is: the custom matcher should respond with false if the element does not have the button role, even if it has the aria-pressed attribute:

// <div data-testid="not-a-button" aria-pressed="true" />
expect(getByTestId('not-a-button')).toBePressed() // should fail

While these other examples should pass:

// <div role="button" aria-pressed="true">Div Button</div>
expect(getByRole('button', { name: 'Div Button' })).toBePressed() // should pass

// <button aria-pressed="true">Real Button</div>
expect(getByRole('button', { name: 'Real Button' })).toBePressed() // should pass

@LucianHij yes, it'd be much appreciated if you contribute this. Thanks!

LucianHij commented 7 months ago

Yes, I will try to do it. Do you have any link to how can I setup the project and do the implementation? @gnapse

gnapse commented 7 months ago

Sure, we do. Check out the "contributing" guide: https://github.com/testing-library/jest-dom/blob/main/CONTRIBUTING.md

And let me know if you need more help or something's not clear. We can always improve that document if something could be better in it.