search

testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.25k stars 463 forks source link

Images with alt attributes cannot be grabbed ByRole #1235

Closed brtrick closed 1 year ago

brtrick commented 1 year ago

The Issue

If an img tag has an alt attribute, its role shows up as presentation instead of as img. So, e.g., this test:

import { render, screen } from '@testing-library/react';

test("Displays an img", () => {
  render (<img src="https://http.cat/418" alt="404" />);
  screen.getByRole('img')
})

produces this result:

ImgError

ARIA Background

According to the ARIA docs an img should only default to the role of presentation if it has an alt attribute equal to "":

AriaImg

Source of the Problem and Proposed Solution

The problem arises when the selector is being made in role-helpers.js:

https://github.com/testing-library/dom-testing-library/blob/d09b3c25b07486c17603317f4c5a0320c188cbed/src/role-helpers.js#L80-L94

Because a value of "" resolves to false in line 87, the selector for a presentation img ends up returning [alt] instead of [alt=""] as its characteristic attribute, meaning that any img with an alt attribute will be marked with a presentation role. Simply adding an explicit check for value === "" on line 87 fixes the issue. (The check could be made more specific if that is needed.)

I will submit a PR with the proposed fix and updated tests.

MatanBobi commented 1 year ago

Hi @brtrick, thanks for the detailed report :) We're here if you need any help.

brtrick commented 1 year ago

Thanks. I finished the pr, but the codebase had 15 failing tests before I did anything (including a couple related to other issues in role-helpers.js), and now it doesn't want to let me commit because of the failed tests. Should I bypass verification and submit the pr anyway? Not sure how best to proceed...

MatanBobi commented 1 year ago

Thanks. I finished the pr, but the codebase had 15 failing tests before I did anything (including a couple related to other issues in role-helpers.js), and now it doesn't want to let me commit because of the failed tests. Should I bypass verification and submit the pr anyway? Not sure how best to proceed...

Hmm, the codebase shouldn't have failing tests, as you can see in the commits, the latest commits passed ci. Which version of node are you using? You can skip the verification to see if it passed in ci :)

brtrick commented 1 year ago

Using Node 16.17 locally, but I get the same failures in all ci Node versions that I got locally. Tried cloning the repo directly for a fresh install; gives me the same 15 failures out of the box.

MatanBobi commented 1 year ago

@brtrick This might be related to the new aria-query release, can you please see what's the version of aria-query you have installed? @eps1lon Heads up about this, it looks like the aria-query release broke a few use cases for us.

brtrick commented 1 year ago

Confirmed. I was using the new aria-query 5.2.1 that was released yesterday. When I reverted to 5.1.3, everything passed.

MatanBobi commented 1 year ago

@brtrick After consulting with @eps1lon, we're pinning the version for now so we'll be able to work on it properly because it looks like these changes might be big.

jlp-craigmorten commented 1 year ago

Sleuthing through the aria-query changes, although the suggested change works around the issue it looks like there are potentially wider considerations may be needed.

In https://github.com/A11yance/aria-query/pull/447 it appears all constraints associated with attributes were removed. E.g. see https://github.com/A11yance/aria-query/pull/447/files#diff-10f57900798ab6f135687081ccc9acafbb0a469d71355baddd128ba1b2b45399L3377 where the constraint that the alt must either be set or not set at all (for the two cases) in order the the img tag to have the img role were removed.

Consequently the check for "undefined" in the constraints array in the role-helpers.js will now never be truthy. See https://github.com/testing-library/dom-testing-library/blob/main/src/role-helpers.js#L84

A naive search suggests there are potentially 9 scenarios where this constraint is no longer present in the latest version(s) of aria-query, impacting the following scenarios: