add toBeReadonly matcher

[bastibuck]

What:

This adds two new matchers toBeReadonly and toBeWritable

Why:

While adding tests to a component lately I was expecting jest-dom to have a matcher similar to toBeDisabled for readonly form fields. Unfortunately there was none.

How:

I added two new matchers toBeReadonly and toBeWritable based on the very similar toBeDisabled and toBeEnabled matchers. Copy-paste pretty much 😉

Checklist:

• Documentation
• Tests
• Updated Type Definitions
• Ready to be merged
  

I was wondering why there was nothing like this before.
Has no one thought about this yet? Is it easy enough to check with toHaveAttribute? If so, why is there something like toBeDisabled which could just be as simply checked?

Also the Type Definitions are not maintained within this repo (anymore), are they? Will submit a PR to DefinatelyTyped after this is merged.

[codecov]

Codecov Report

Merging #204 into master will not change coverage by %.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #204   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           19        20    +1     
  Lines          240       252   +12     
  Branches        58        61    +3     
=========================================
+ Hits           240       252   +12     

Impacted Files	Coverage Δ
src/to-be-readonly.js	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b98d4d...3e90b21. Read the comment docs.

[lourenci]

Has no one thought about this yet? Is it easy enough to check with toHaveAttribute? If so, why is there something like toBeDisabled which could just be as simply checked?

Hi, @bastibuck.

The toBeDisabled is wider. An ancestor (like <fieldset />) can disable an input field, thus making the specs verbose to check for disabled. readonly is only for the form field.

[gnapse]

@bastibuck thanks for the contribution.

I was wondering why there was nothing like this before. Has no one thought about this yet?

Because no one has suggested it before, or needed it. You seem to be the first, at least the first to want it enough to bother. Thanks!

Is it easy enough to check with toHaveAttribute? If so, why is there something like toBeDisabled which could just be as simply checked?

I'd say that the "readonly" case is less compelling than the "disabled" case. In the "disabled" case, in addition to the attribute presence, an element could be disabled because it was contained inside a fieldset that was also disabled. And actually it was not even that simple. There's a relatively complex set of rules around that.

Can you make a case for why .toHaveAttribute('readonly') is not enough?

[bastibuck]

Hey,

thanks for the quick responses and addressing my questions.

I totally get the point of what you're saying with the wider use-case of toBeDisabled(). Thanks for clarifying :-)

@gnapse Initially I couldn't make up a case other than that I wanted to check if a given field has the readonly attribute set. I s awit more like a nicer way of writing the tests for these cases though.

Looking at the code for toBeRequired() and toBeChecked() I think I could add aria-readonly checks which does bring up a use-case as I don't want to manually check both possibilities.

PS: This brings up the question and maybe another PR: Shouldn't toBeDisabled() also check the aria-disabled attribute? 🤔

[gnapse]

This brings up the question and maybe another PR: Shouldn't toBeDisabled() also check the aria-disabled attribute? 🤔

This was already brought up, and although it seemed logical at first, it's not that simple. See the discussion in the proposal #144 and the attempted-but-never-merged pull request #146. TL;DR: aria-disabled is semantically softer than disabled. It does not enforce by itself any behavior about the disabled element not being interactive. Whereas disabled has more behavioral implications. If we conflated the two attributes in a single matcher, it could lead to assertions passing on elements that are really not disabled in the stronger sense of the word.

Looking at the code for toBeRequired() and toBeChecked() I think I could add aria-readonly checks which does bring up a use-case as I don't want to manually check both possibilities.

Not sure what you're saying here? Are you acknowledging that you can get by with simply checking for the presence of the readonly attribute? What are these "both possibilities" you refer to having to check manually?

[bastibuck]

Not sure what you're saying here? Are you acknowledging that you can get by with simply checking for the presence of the readonly attribute? What are these "both possibilities" you refer to having to check manually?

Sorry, my bad.

I was trying to say that after looking at the other two matchers I found out that something like aria-readonly exists which my code doesn't check for yet.

https://www.w3.org/TR/html-aria/#att-readonly makes it look like <input /> and <textarea> are the only elements that are allowed to have the "real" readonly attribute - which implicitly has aria-readonly set and has an impact on how the user can interact with the elements.

If I understand correctly only all other elements can have the aria-readonly attribute set which by itself doesn't result in a change of behaviour for the user. Therefor I'm not sure if it makes sense to check for aria-readonly. Just like outlined in the discussions you linked.

TL;DR: aria-disabled is semantically softer than disabled. It does not enforce by itself any behavior about the disabled element not being interactive. Whereas disabled has more behavioral implications. If we conflated the two attributes in a single matcher, it could lead to assertions passing on elements that are really not disabled in the stronger sense of the word.

This seems just as true for aria-readonly.

---

Which brings us back to the initial question if a matcher like the proposed makes sense at all ;-)

IMO it can make for a nicer way of writing tests but this might lead to an unnecessary increase of matchers as there are way more matchers that could be handled with a toHaveAttribute() matcher.

[gnapse]

Indeed. I'm leaning towards this being a very small syntactic sugar on top of .toHaveAttribute('readonly'). And since it does not make sense to check for both readonly and aria-readonly under the same custom matcher, because of their semantic differences, the benefits do not outweight the cost of having our API surface expanded.

Thanks for understanding. I'm sorry you devoted time to this. That's why I always encourage people to propose new matchers first in an issue, instead of jumping into the PR. Hopefully this is not the last time we'll see you around contributing.

[bastibuck]

Agreed!

Don't worry, will contribute if I find or need something. But I will start a discussion before creating a PR the next time 😉 No wasted time though as I learned something.