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

Images with alt attributes cannot be grabbed ByRole #1235

Closed
brtrick opened this issue Jun 14, 2023 · 8 comments · Fixed by #1237
Closed

Images with alt attributes cannot be grabbed ByRole #1235

brtrick opened this issue Jun 14, 2023 · 8 comments · Fixed by #1237

Comments

@brtrick
Copy link

brtrick commented Jun 14, 2023

  • @testing-library/dom version: 9.3.0
  • Testing Framework and version: Jest/React
  • DOM Environment:

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:

function buildElementRoleList(elementRolesMap) {
function makeElementSelector({name, attributes}) {
return `${name}${attributes
.map(({name: attributeName, value, constraints = []}) => {
const shouldNotExist = constraints.indexOf('undefined') !== -1
if (shouldNotExist) {
return `:not([${attributeName}])`
} else if (value) {
return `[${attributeName}="${value}"]`
} else {
return `[${attributeName}]`
}
})
.join('')}`
}

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

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

@brtrick
Copy link
Author

brtrick commented Jun 14, 2023

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

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

brtrick commented Jun 14, 2023

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

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

brtrick commented Jun 14, 2023

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 MatanBobi mentioned this issue Jun 14, 2023
4 tasks
@MatanBobi
Copy link
Member

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

jlp-craigmorten commented Jun 15, 2023

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 A11yance/aria-query#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:

  • combobox role when multiple and size are undefined for the select element
  • combobox role when multiple is undefined and size is 1 for the select element
  • img role when alt is undefined for the img element
  • searchbox role when list is undefined and type is search for the input element
  • textbox role when list is undefined and type is undefined for the input element
  • textbox role when list is undefined and type is email for the input element
  • textbox role when list is undefined and type is tel for the input element
  • textbox role when list is undefined and type is text for the input element
  • textbox role when list is undefined and type is url for the input element

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants