Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for matching pressed state of toggle buttons #203

Open
bfintal opened this issue Feb 11, 2020 · 10 comments · May be fixed by #658
Open

Support for matching pressed state of toggle buttons #203

bfintal opened this issue Feb 11, 2020 · 10 comments · May be fixed by #658
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@bfintal
Copy link

bfintal commented Feb 11, 2020

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
Copy link
Member

gnapse commented Mar 15, 2020

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.

@gnapse gnapse added the wontfix This will not be worked on label Mar 15, 2020
@jmvtrinidad
Copy link

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
Copy link
Member

gnapse commented Jan 4, 2021

@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
Copy link

@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
Copy link
Member

gnapse commented Jan 7, 2021

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.

@gnapse gnapse closed this as completed Jan 7, 2021
@Sawtaytoes
Copy link

Sawtaytoes commented May 26, 2022

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:

  • toBeExpanded is handled by toBeVisible.
  • toBeLabelledBy and toBeDescribedBy are handled by Testing Library's queryBy. You can't even select the element if the values are wrong or missing (this can happen when they have a hidden attribute I found out).
  • toBeSelected can also be handled by toHaveValue, but only for <select>. More on this below.
  • toHaveMinValue should be handled by a UI test or some sort of validation checking.

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

  • toBeVisible is checking something different than aria-hidden such as hidden and display: none.
  • There are two ways for something to be checked: checked and aria-checked, so it makes sense to have a toBeChecked. This is also separate from toHaveValue as the value on a checkbox can be static while the checked property might not be.
  • There are 2 ways to know if an option is selected: selected and aria-selected, but as I stated earlier, this doesn't warrant a separate expectation because you can use toHaveValue instead so long as it's a <select>.

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
Copy link

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

@gnapse
Copy link
Member

gnapse commented Feb 13, 2024

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!

@gnapse gnapse reopened this Feb 13, 2024
@gnapse gnapse added enhancement New feature or request good first issue Good for newcomers and removed wontfix This will not be worked on labels Feb 13, 2024
@LucianHij
Copy link

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
Copy link
Member

gnapse commented Feb 14, 2024

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.

kretajak added a commit to kretajak/jest-dom that referenced this issue Dec 21, 2024
@kretajak kretajak linked a pull request Dec 21, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants