Skip to content

fix: Don't recommend ByRole if role is generic #664

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

Merged
merged 2 commits into from
Jun 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/__tests__/element-queries.js
Original file line number Diff line number Diff line change
Expand Up @@ -533,14 +533,14 @@ test('queryAllByRole returns semantic html elements', () => {
expect(queryAllByRole(/listitem/i)).toHaveLength(3)
// TODO: with https://github.com/A11yance/aria-query/pull/42
// the actual value will match `toHaveLength(2)`
expect(queryAllByRole(/textbox/i)).toHaveLength(1)
expect(queryAllByRole(/textbox/i)).toHaveLength(2)
expect(queryAllByRole(/checkbox/i)).toHaveLength(1)
expect(queryAllByRole(/radio/i)).toHaveLength(1)
expect(queryAllByRole('row')).toHaveLength(3)
expect(queryAllByRole(/rowgroup/i)).toHaveLength(2)
// TODO: with https://github.com/A11yance/aria-query/pull/42
// the actual value will match `toHaveLength(3)`
expect(queryAllByRole(/(table)|(textbox)/i)).toHaveLength(2)
expect(queryAllByRole(/(table)|(textbox)/i)).toHaveLength(3)
expect(queryAllByRole(/img/i)).toHaveLength(1)
expect(queryAllByRole('meter')).toHaveLength(1)
expect(queryAllByRole('progressbar')).toHaveLength(0)
Expand Down
30 changes: 18 additions & 12 deletions src/__tests__/suggestions.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ test(`should not suggest if the suggestion would give different results`, () =>
})

test('should suggest by label over title', () => {
renderIntoDocument(`<label><span>bar</span><input title="foo" /></label>`)
renderIntoDocument(
`<label><span>bar</span><input type="password" title="foo" /></label>`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be accurate: The spec related to aria didn't change. This was a bug in earlier versions of aria-query. <input /> with no or an invalid type attribute is always considered <input type="text" /> which is a role="textbox".

)

expect(() => screen.getByTitle('foo')).toThrowError(
/getByLabelText\(\/bar\/i\)/,
Expand Down Expand Up @@ -181,7 +183,7 @@ test('escapes regular expressions in suggestion', () => {

test('should suggest getByLabelText when no role available', () => {
renderIntoDocument(
`<label for="foo">Username</label><input data-testid="foo" id="foo" />`,
`<label for="foo">Username</label><input type="password" data-testid="foo" id="foo" />`,
)
expect(() => screen.getByTestId('foo')).toThrowError(
/getByLabelText\(\/username\/i\)/,
Expand Down Expand Up @@ -232,19 +234,21 @@ test.each([

test(`should suggest label over placeholder text`, () => {
renderIntoDocument(
`<label for="foo">Username</label><input id="foo" data-testid="foo" placeholder="Username" />`,
`<label for="foo">Password</label><input type="password" id="foo" data-testid="foo" placeholder="Password" />`,
)

expect(() => screen.getByPlaceholderText('Username')).toThrowError(
/getByLabelText\(\/username\/i\)/,
expect(() => screen.getByPlaceholderText('Password')).toThrowError(
/getByLabelText\(\/password\/i\)/,
)
})

test(`should suggest getByPlaceholderText`, () => {
renderIntoDocument(`<input data-testid="foo" placeholder="Username" />`)
renderIntoDocument(
`<input type="password" data-testid="foo" placeholder="Password" />`,
)

expect(() => screen.getByTestId('foo')).toThrowError(
/getByPlaceholderText\(\/username\/i\)/,
/getByPlaceholderText\(\/password\/i\)/,
)
})

Expand All @@ -257,25 +261,27 @@ test(`should suggest getByText for simple elements`, () => {
})

test(`should suggest getByDisplayValue`, () => {
renderIntoDocument(`<input id="lastName" data-testid="lastName" />`)
renderIntoDocument(
`<input type="password" id="password" data-testid="password" />`,
)

document.getElementById('lastName').value = 'Prine' // RIP John Prine
document.getElementById('password').value = 'Prine' // RIP John Prine

expect(() => screen.getByTestId('lastName')).toThrowError(
expect(() => screen.getByTestId('password')).toThrowError(
/getByDisplayValue\(\/prine\/i\)/,
)
})

test(`should suggest getByAltText`, () => {
renderIntoDocument(`
<input data-testid="input" alt="last name" />
<input type="password" data-testid="input" alt="password" />
<map name="workmap">
<area data-testid="area" shape="rect" coords="34,44,270,350" alt="Computer">
</map>
`)

expect(() => screen.getByTestId('input')).toThrowError(
/getByAltText\(\/last name\/i\)/,
/getByAltText\(\/password\/i\)/,
)
expect(() => screen.getByTestId('area')).toThrowError(
/getByAltText\(\/computer\/i\)/,
Expand Down
3 changes: 2 additions & 1 deletion src/role-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,9 @@ function getRoles(container, {hidden = false} = {}) {

function prettyRoles(dom, {hidden}) {
const roles = getRoles(dom, {hidden})

//We prefer to skip generic role, we don't reccomand it
return Object.entries(roles)
.filter(([role]) => role !== 'generic')
.map(([role, elements]) => {
const delimiterBar = '-'.repeat(50)
const elementsString = elements
Expand Down
3 changes: 2 additions & 1 deletion src/suggestions.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,10 @@ export function getSuggestedQuery(element, variant = 'get', method) {
return undefined
}

//We prefer to suggest something else if the role is generic
const role =
element.getAttribute('role') ?? getImplicitAriaRoles(element)?.[0]
if (canSuggest('Role', method, role)) {
if (role !== 'generic' && canSuggest('Role', method, role)) {
return makeSuggestion('Role', role, {
variant,
name: computeAccessibleName(element),
Expand Down