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

Conform toHaveErrorMessage to Spec and Rename to toHaveAccessibleErrorMessage #503

Merged
merged 2 commits into from Jul 18, 2023

Conversation

ITenthusiasm
Copy link
Contributor

@ITenthusiasm ITenthusiasm commented Jul 17, 2023

What

Primarily, this PR is intended to be a fix that conforms the original toHaveErrorMessage method more closely to the WAI ARIA spec. This PR also renames toHaveErrorMessage to toHaveAccessibleErrorMessage.

There was also a slight change to how the assertions are exported from matchers.js.

Why

Spec Concerns:

Currently, there are some ways in which toHaveErrorMessage doesn't conform to the specifications:

  • aria-errormessage is expected to be a single id, not a list of ids. Supplying multiple ids is illegal (because it would constitute an invalid singular id). I didn't recognize this until reading through the spec more closely.
  • The cases in which an element technically "does not have an accessible error message" could be improved for the sake of developer experience and expectations.

Rename:

The other a11y-related methods explicitly call out the fact that they're intended for accessibility-related assertions. I figured it would be helpful to keep things consistent.

Exports Update:

This is completely optional. But since it was simple, I thought it was worth trying and running by the maintainers. The new approach involves less lines of code, which will hopefully be more readable and less prone to error when it's updated.

How

These changes were implemented with a new toHaveAccessibleErrorMessage method that largely built on top of the previous toHaveErrorMessage method. I tested the aria-errormessage attribute with my iPhone to make sure the test cases that I wrote were consistent with reality.

Technically speaking, it's debatable whether or not the toHaveAccessibleErrorMessage assertion should have early error cases that guide the user to success with intermediate checks (e.g., checks on aria-errormessage and aria-invalid). But since the aria-errormessage attribute is a little more complicated than the others, it could be helpful to keep the checks/warnings around. I'm not tied to down to either view, but I continued with the pattern that was already established (i.e., the pattern of helping the developer correctly give an error message to an element).

Checklist

I forgot the typedefs and docs. 😭 I'll look into this. Feel free to add any comments in the meantime.

Resolves #256
Improves #370

@ITenthusiasm
Copy link
Contributor Author

ITenthusiasm commented Jul 18, 2023

So I actually don't know where the type definitions are. 😳 Some assistance would be helpful. 😅

Is the documentation automatically generated from the type definitions/JSDocs?


Edit: I'm assuming that the docs and the type definitions are separate -- interestingly enough. Feel free to correct me if I'm wrong. But if this is indeed the case, then I'll update the docs here and then update the types in Definitely Typed.

Is there a plan for this library to own its types in the future?

Included Changes:
- According to the WAI-ARIA spec, passing an invalid
  `id` to `aria-errormessage` now fails assertion.
  This means that any empty spaces inside
  `aria-errormessage` will now cause test failures.

- According to the WAI-ARIA spec, developers can now
  assert that an accessible error message is missing
  if `aria-invalid` is `false` (or if the
  `aria-errormessage` attribute is invalid).

- Updated the error message and test cases surrounding
  the requirement for `aria-invalid`. They are now
  more detailed/accurate.

- Renamed `toHaveErrorMessage` to
  `toHaveAccessibleErrorMessage` to be consistent with
  the other a11y-related methods (`toHaveAccessibleName`
  and `toHaveAccessibleDescription`).
   - Note: This deprecates the previous
   `toHaveErrorMessage` method.

- Updated documentation. Similar to the
  `toHaveAccessibleDescription` method, this description
  is much more lean, as the reader can simply read the
  WAI ARIA spec for additional details/requirements.
This makes the code easier to maintain as more
exports are added.
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #503 (fbe6feb) into main (948d90f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #503   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines          630       664   +34     
  Branches       236       251   +15     
=========================================
+ Hits           630       664   +34     
Impacted Files Coverage Δ
src/to-have-accessible-errormessage.js 100.00% <100.00%> (ø)
src/to-have-errormessage.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gnapse gnapse merged commit d717c66 into testing-library:main Jul 18, 2023
8 checks passed
@ITenthusiasm ITenthusiasm deleted the pr/fix/to-have-errormessage branch July 18, 2023 12:41
@github-actions
Copy link

🎉 This PR is included in version 5.17.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ITenthusiasm added a commit to ITenthusiasm/DefinitelyTyped that referenced this pull request Jul 18, 2023
typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 24, 2023
…rary/jest-dom` Types by @ITenthusiasm

Created in response to (and thus depends on):
- testing-library/jest-dom#503
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toHaveErrorMessage
2 participants