-
Notifications
You must be signed in to change notification settings - Fork 59
Bump aria-query and update to fix tests #448
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
Changes from 15 commits
951feea
95c60cf
762f7e1
2747f2d
d21a25e
281fa95
33fe0b0
985fa01
247eea2
5c98a44
ba52aca
b12cda5
9136431
2fa5eca
b2ed30e
8fb6419
5a66033
aaca700
40c0b2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
const {getProp, getPropValue} = require('jsx-ast-utils') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would appreciate @smockle eyes on this! I extracted the code into this helper, and made updates to fix the bug. |
||
const {elementRoles} = require('aria-query') | ||
const {getElementType} = require('./get-element-type') | ||
const ObjectMap = require('./object-map') | ||
|
||
// Clean-up `elementRoles` from `aria-query` | ||
const elementRolesMap = new ObjectMap() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is constructing a cleaned up version of elementRoles. Below, we'll be constructing a key and grabbing the value from |
||
for (const [key, value] of elementRoles.entries()) { | ||
// - Remove unused `constraints` key | ||
delete key.constraints | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this line still necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooh nice catch! It looks like removing this constraints can result in incorrect mappings in some scenarios... For example,
If we remove the constraints key, we're left with:
Since we cannot always accurately parse out the role of an ancestor in a linter context, I think we'd want to return undefined instead of potentially returning a false positive role. I will follow-up! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I deleted this line in 8fb6419! However, there were two scenarios where it seemed appropriate to assume the role despite the constraints defined in I've added additional tests in aaca700#diff-a6b7fa0d4a2be411ccfcc6e6db5466c135100a1a045599f5c211bb91c37b839dR180-R190. |
||
// - Remove empty `attributes` key | ||
if (!key.attributes || key.attributes?.length === 0) { | ||
delete key.attributes | ||
} | ||
elementRolesMap.set(key, value) | ||
} | ||
// - Remove insufficiently-disambiguated `menuitem` entry | ||
elementRolesMap.delete({name: 'menuitem'}) | ||
// - Disambiguate `menuitem` and `menu` roles by `type` | ||
elementRolesMap.set({name: 'menuitem', attributes: [{name: 'type', value: 'command'}]}, ['menuitem']) | ||
elementRolesMap.set({name: 'menuitem', attributes: [{name: 'type', value: 'radio'}]}, ['menuitemradio']) | ||
elementRolesMap.set({name: 'menuitem', attributes: [{name: 'type', value: 'toolbar'}]}, ['toolbar']) | ||
elementRolesMap.set({name: 'menu', attributes: [{name: 'type', value: 'toolbar'}]}, ['toolbar']) | ||
|
||
/* | ||
Determine role of an element, based on its name and attributes. | ||
*/ | ||
function getRole(context, node) { | ||
// Early return if role is explicitly set | ||
const explicitRole = getPropValue(getProp(node.attributes, 'role')) | ||
if (explicitRole) { | ||
return explicitRole | ||
} | ||
|
||
// Assemble a key for looking-up the element’s role in the `elementRolesMap` | ||
// - Get the element’s name | ||
const key = {name: getElementType(context, node)} | ||
|
||
for (const prop of [ | ||
'aria-label', | ||
'aria-labelledby', | ||
'alt', | ||
'type', | ||
'size', | ||
'role', | ||
'href', | ||
'multiple', | ||
'scope', | ||
'name', | ||
]) { | ||
if ((prop === 'aria-labelledby' || prop === 'aria-label') && !['section', 'aside', 'form'].includes(key.name)) | ||
continue | ||
if (prop === 'name' && key.name !== 'form') continue | ||
if (prop === 'href' && key.name !== 'a' && key.name !== 'area') continue | ||
if (prop === 'alt' && key.name !== 'img') continue | ||
|
||
const propOnNode = getProp(node.attributes, prop) | ||
|
||
if (!('attributes' in key)) { | ||
key.attributes = [] | ||
} | ||
// Disambiguate "undefined" props | ||
if (propOnNode === undefined && prop === 'alt' && key.name === 'img') { | ||
key.attributes.push({name: prop, constraints: ['undefined']}) | ||
continue | ||
} | ||
|
||
const value = getPropValue(propOnNode) | ||
if (value || (value === '' && prop === 'alt')) { | ||
if ( | ||
prop === 'href' || | ||
prop === 'aria-labelledby' || | ||
prop === 'aria-label' || | ||
prop === 'name' || | ||
(prop === 'alt' && value !== '') | ||
) { | ||
key.attributes.push({name: prop, constraints: ['set']}) | ||
} else { | ||
key.attributes.push({name: prop, value}) | ||
} | ||
} | ||
} | ||
|
||
// - Remove empty `attributes` key | ||
if (!key.attributes || key.attributes?.length === 0) { | ||
delete key.attributes | ||
} | ||
|
||
// Get the element’s implicit role | ||
return elementRolesMap.get(key)?.[0] | ||
} | ||
|
||
module.exports = {getRole} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,9 +57,6 @@ ruleTester.run('role-supports-aria-props', rule, { | |
{code: '<a href="#" aria-owns />'}, | ||
{code: '<a href="#" aria-relevant />'}, | ||
|
||
// this will have global | ||
{code: '<a aria-checked />'}, | ||
Comment on lines
-60
to
-61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now treated as |
||
|
||
// AREA TESTS - implicit role is `link` | ||
{code: '<area href="#" aria-expanded />'}, | ||
{code: '<area href="#" aria-atomic />'}, | ||
|
@@ -78,30 +75,6 @@ ruleTester.run('role-supports-aria-props', rule, { | |
{code: '<area href="#" aria-owns />'}, | ||
{code: '<area href="#" aria-relevant />'}, | ||
|
||
// this will have global | ||
{code: '<area aria-checked />'}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now treated as |
||
|
||
// LINK TESTS - implicit role is `link` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{code: '<link href="#" aria-expanded />'}, | ||
{code: '<link href="#" aria-atomic />'}, | ||
{code: '<link href="#" aria-busy />'}, | ||
{code: '<link href="#" aria-controls />'}, | ||
{code: '<link href="#" aria-describedby />'}, | ||
{code: '<link href="#" aria-disabled />'}, | ||
{code: '<link href="#" aria-dropeffect />'}, | ||
{code: '<link href="#" aria-flowto />'}, | ||
{code: '<link href="#" aria-grabbed />'}, | ||
{code: '<link href="#" aria-hidden />'}, | ||
{code: '<link href="#" aria-haspopup />'}, | ||
{code: '<link href="#" aria-label />'}, | ||
{code: '<link href="#" aria-labelledby />'}, | ||
{code: '<link href="#" aria-live />'}, | ||
{code: '<link href="#" aria-owns />'}, | ||
{code: '<link href="#" aria-relevant />'}, | ||
|
||
// this will have global | ||
{code: '<link aria-checked />'}, | ||
|
||
// this will have role of `img` | ||
{code: '<img alt="foobar" aria-busy />'}, | ||
|
||
|
@@ -344,20 +317,25 @@ ruleTester.run('role-supports-aria-props', rule, { | |
{code: '<datalist aria-expanded />'}, | ||
{code: '<div role="heading" aria-level />'}, | ||
{code: '<div role="heading" aria-level="1" />'}, | ||
{code: '<link href="#" aria-expanded />'}, // link maps to nothing | ||
], | ||
|
||
invalid: [ | ||
// implicit basic checks | ||
{ | ||
code: '<a href="#" aria-checked />', | ||
errors: [getErrorMessage('aria-checked', 'link')], | ||
code: '<area aria-checked />', | ||
errors: [getErrorMessage('aria-checked', 'generic')], | ||
}, | ||
{ | ||
code: '<area href="#" aria-checked />', | ||
code: '<a aria-checked />', | ||
errors: [getErrorMessage('aria-checked', 'generic')], | ||
}, | ||
{ | ||
code: '<a href="#" aria-checked />', | ||
errors: [getErrorMessage('aria-checked', 'link')], | ||
}, | ||
{ | ||
code: '<link href="#" aria-checked />', | ||
code: '<area href="#" aria-checked />', | ||
errors: [getErrorMessage('aria-checked', 'link')], | ||
}, | ||
{ | ||
|
@@ -370,7 +348,7 @@ ruleTester.run('role-supports-aria-props', rule, { | |
}, | ||
{ | ||
code: '<aside aria-checked />', | ||
errors: [getErrorMessage('aria-checked', 'complementary')], | ||
errors: [getErrorMessage('aria-checked', 'generic')], | ||
}, | ||
{ | ||
code: '<ul aria-expanded />', | ||
|
@@ -386,15 +364,15 @@ ruleTester.run('role-supports-aria-props', rule, { | |
}, | ||
{ | ||
code: '<aside aria-expanded />', | ||
errors: [getErrorMessage('aria-expanded', 'complementary')], | ||
errors: [getErrorMessage('aria-expanded', 'generic')], | ||
}, | ||
{ | ||
code: '<article aria-expanded />', | ||
errors: [getErrorMessage('aria-expanded', 'article')], | ||
}, | ||
{ | ||
code: '<body aria-expanded />', | ||
errors: [getErrorMessage('aria-expanded', 'document')], | ||
errors: [getErrorMessage('aria-expanded', 'generic')], | ||
}, | ||
{ | ||
code: '<li aria-expanded />', | ||
|
@@ -414,6 +392,10 @@ ruleTester.run('role-supports-aria-props', rule, { | |
}, | ||
{ | ||
code: '<section aria-expanded />', | ||
errors: [getErrorMessage('aria-expanded', 'generic')], | ||
}, | ||
{ | ||
code: '<section aria-label="something" aria-expanded />', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
errors: [getErrorMessage('aria-expanded', 'region')], | ||
}, | ||
{ | ||
|
@@ -480,10 +462,6 @@ ruleTester.run('role-supports-aria-props', rule, { | |
code: '<menu type="toolbar" aria-expanded />', | ||
errors: [getErrorMessage('aria-expanded', 'toolbar')], | ||
}, | ||
{ | ||
code: '<link href="#" aria-invalid />', | ||
errors: [getErrorMessage('aria-invalid', 'link')], | ||
}, | ||
{ | ||
code: '<area href="#" aria-invalid />', | ||
errors: [getErrorMessage('aria-invalid', 'link')], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,11 @@ | ||
const {getElementType} = require('../../lib/utils/get-element-type') | ||
const {mockJSXAttribute, mockJSXOpeningElement} = require('./mocks') | ||
|
||
const mocha = require('mocha') | ||
const describe = mocha.describe | ||
const it = mocha.it | ||
const expect = require('chai').expect | ||
|
||
function mockJSXAttribute(prop, propValue) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm extracting this into a helper. |
||
return { | ||
type: 'JSXAttribute', | ||
name: { | ||
type: 'JSXIdentifier', | ||
name: prop, | ||
}, | ||
value: { | ||
type: 'Literal', | ||
value: propValue, | ||
}, | ||
} | ||
} | ||
|
||
function mockJSXOpeningElement(tagName, attributes = []) { | ||
return { | ||
type: 'JSXOpeningElement', | ||
name: { | ||
type: 'JSXIdentifier', | ||
name: tagName, | ||
}, | ||
attributes, | ||
} | ||
} | ||
|
||
function mockSetting(componentSetting = {}) { | ||
return { | ||
settings: { | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm extracting this entire section a new helper called
getRole
, with tests.