Implement .toBeLabelled custom matcher

[hiwelo]

What:

This PR is proposing an implementation for the custom matcher .toBeLabelled.

This custom matcher is checking if an element is matching at least one of these requirements:

• the element is an image having a not empty alt attribute;
• the element is a SVG element having a not empty title element;
• the element is having an aria-label attribute;
• the element is referencing a labelling element via aria-labelledby;
• the element is a form input having a title attribute;
• the element is an image associated to a short content within a button or a link;
• the element is a link, a button or an object having a text content.

Why:

This PR is following the feature requested discussed with the issue #111.
This custom matcher would replace a series of tests to know if an element is labelled.

How:

This custom matcher is checking the markup of the current test target and applying a series of a tests to know if this element is labelled correctly for screen readers.

Checklist:

• Documentation
• Tests
• Updated Type Definitions
• Ready to be merged
• Added myself to contributors table N/A

This is a proposed implementation, but as usual please let me know if I should update something.
I tried to document also each check directly within the codebase to improve the readability of the tests.

If merged, this would close #111.

[gnapse]

Hi @hiwelo, nice work here.

I left a few comments. I realize one of them may add significant complexity to this, but I'd be willing to help more actively to make it work.

Also, I share @alexkrolick's concerns expressed in #111 about the potential confusion of mixing checking for labels and checking for images alt attributes. In particular the potential for having an image considered labeled because it has a aria-label, even when it lacks thr alt attribute. It is my understanding that that is not how to properly label an image. What do you think?

[gnapse]

What do you think about this checking if the referred element really exist?

[hiwelo]

I though about that a bit but I do not know if we should do that all the time.
I am thinking about my own usage for example where I am doing a series of check on a UI library's components. This UI library is using the atomic design philosophy and for example, I want to check that my atoms can be labelled, even if the label is not existing in the tested DOM.
In the case of my atoms & molecules components, it could complexify the test scenarii.

But at the same time, aria-labelledby is only a valid way to label an element if the target is existing. So I would agree with you that the test cannot really returns true if we are not sure the target is existing.

Should this check be an option?

[gnapse]

I feel that this should indeed do the additional check. If users merely wanted to check for the presence of the aria-labelledby attribute, they can easily do so with .toHaveAttribute('aria-labelledby'). The real value proposition of this matcher is to actually check the label actually exists. And as I mentioned in another comment, to eventually even allow to make assertions on the label text as well. It aligns much better with the overall value proposition of the testing libraries ecosystem to promote testing the end result that users see.

If a UI library that you're testing has components that receive the aria-labelledby but they do not render the target element themselves, you can always in you tests render an outer div that includes the target element with a meaningful label text, alongside your component. Then when you test it, it will actually have a label. For example in react it would be something like this:

render(
  <div>
    <h1 id="my-component-title">Hello world</h1>
    <MyComponent aria-labelledby="my-component-title" />
  </div>
)

[alexkrolick]

The real value proposition of this matcher is to actually check the label actually exists

This would be really useful - you can add this before doing 'label' queries so that you can tell what is going wrong - the label or the reference. Also ensures the aria relationships don't get stale or broken over time.

[hiwelo]

Also, I share @alexkrolick's concerns expressed in #111 about the potential confusion of mixing checking for labels and checking for images alt attributes. In particular the potential for having an image considered labeled because it has a aria-label, even when it lacks thr alt attribute. It is my understanding that that is not how to properly label an image. What do you think?

I answered in the issue #111 directly. 🙂

[hiwelo]

Sorry for the time elapsed, I was on vacation and then a bit busy. I resolved some of the discussions, I just have two last points open where I may need help or answers @gnapse :)

[gnapse]

Looking good.

Just requesting a small change in the README and in the aria-labelledby check.

Also, I'm not sure what you referred to in your last comment about you still awaiting or needing seom help from me. What is it?

[weyert]

How to progress this?

[gnapse]

How to progress this?

😢

Indeed, my apologies for having let this fall into oblivion. @hiwelo let me know if you have some bandwidth to resume this and finish it. If you ca'nt that's ok. I may be able to make some time to take this over if needed.

I just have two last points open where I may need help or answers @gnapse :)

Let me know if you recall what these were. I'll also perform a more recent in-depth review to recall myself where this was left off.

[weyert]

Yes, I am happy to assist, if I can help in anyway. Just a bit unclear to me what the current state of things is.

[eps1lon]

This should be implemented using dom-accessibility-api which covers all edge cases of aria-label, aria-labelledby, alt etc.

[hiwelo]

Aww sorry I just realised this PR was still open. It was completely out of my memory, I'm deeply sorry for the lack of answer. 🙀

I can't recall what was exactly the status. But I definitely agree with @eps1lon that using the dom-accessibility-api would now be the cleanest way to implement this custom matcher. 🙂

I don't have the runway this week to work on this, but could make some space if needed in the near future. So if someone wants to give it a try in the meantime, please feel free to do so :)

[eps1lon]

For Material-UI we used accessibleName instead of labelled. In jest this would mean calling it toHaveAccessibleName.
Since it relies on computed styles (which is not fully implemented in jsdom and slower than the browser implementation) we use a fake implementation of getComputedStyle that worked for us.

We use chai so the matcher extension API looks a bit different. I hope this helps regardless: https://github.com/mui-org/material-ui/blob/ac5c1c1d05987f020e2f31ec73eed8bbd35b6032/test/utils/initMatchers.js#L67-L137.

I wouldn't use the custom getComputedStyle but rather make this configurable in jest-dom. The default could look like the one we used though. When we increased usage of getComputedStyle in dom-testing-library we got reports of tests that run twice as long so you might want to be cautious.

[gnapse]

I'm closing this in favour of #377, which follows the implementation approach recommended in the feedback to this PR.

[gnapse]

Thanks to @hiwelo for the effort put into this anyway.