Skip to content

Commit

Permalink
fix(aria-input-field-name): skip combobox popups (#3886)
Browse files Browse the repository at this point in the history
* fix(aria-input-field-name): skip combobox popups

* skip popups on no-naming-method-matches

* Test isComboboxPopup

* Improve to run better on virtual trees

* modals require a name though

* Resolve feedback
  • Loading branch information
WilcoFiers committed Jan 31, 2023
1 parent da19946 commit 3dcdd42
Show file tree
Hide file tree
Showing 8 changed files with 266 additions and 81 deletions.
4 changes: 2 additions & 2 deletions lib/commons/aria/get-role.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import getImplicitRole from './implicit-role';
import getGlobalAriaAttrs from '../standards/get-global-aria-attrs';
import isFocusable from '../dom/is-focusable';
import { getNodeFromTree } from '../../core/utils';
import AbstractVirtuaNode from '../../core/base/virtual-node/abstract-virtual-node';
import AbstractVirtualNode from '../../core/base/virtual-node/abstract-virtual-node';

// when an element inherits the presentational role from a parent
// is not defined in the spec, but through testing it seems to be
Expand Down Expand Up @@ -126,7 +126,7 @@ function hasConflictResolution(vNode) {
*/
function resolveRole(node, { noImplicit, ...roleOptions } = {}) {
const vNode =
node instanceof AbstractVirtuaNode ? node : getNodeFromTree(node);
node instanceof AbstractVirtualNode ? node : getNodeFromTree(node);
if (vNode.props.nodeType !== 1) {
return null;
}
Expand Down
1 change: 1 addition & 0 deletions lib/commons/aria/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export { default as implicitNodes } from './implicit-nodes';
export { default as implicitRole } from './implicit-role';
export { default as isAccessibleRef } from './is-accessible-ref';
export { default as isAriaRoleAllowedOnElement } from './is-aria-role-allowed-on-element';
export { default as isComboboxPopup } from './is-combobox-popup';
export { default as isUnsupportedRole } from './is-unsupported-role';
export { default as isValidRole } from './is-valid-role';
export { default as labelVirtual } from './label-virtual';
Expand Down
55 changes: 55 additions & 0 deletions lib/commons/aria/is-combobox-popup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import getRole from './get-role';
import ariaAttrs from '../../standards/aria-attrs';
import { getRootNode } from '../../core/utils';

/**
* Whether an element is the popup for a combobox
* @method isComboboxPopup
* @memberof axe.commons.aria
* @instance
* @param {VirtualNode} virtualNode
* @param {Object} options
* @property {String[]} popupRoles Overrides which roles can be popup. Defaults to aria-haspopup values
* @returns {boolean}
*/
export default function isComboboxPopup(virtualNode, { popupRoles } = {}) {
const role = getRole(virtualNode);
popupRoles ??= ariaAttrs['aria-haspopup'].values;
if (!popupRoles.includes(role)) {
return false;
}

// in ARIA 1.1 the container has role=combobox
const vParent = nearestParentWithRole(virtualNode);
if (isCombobox(vParent)) {
return true;
}

const { id } = virtualNode.props;
if (!id) {
return false;
}

if (!virtualNode.actualNode) {
throw new Error('Unable to determine combobox popup without an actualNode');
}
const root = getRootNode(virtualNode.actualNode);
const ownedCombobox = root.querySelectorAll(
// aria-owns was from ARIA 1.0, aria-controls was from ARIA 1.2
`[aria-owns~="${id}"][role~="combobox"]:not(select),
[aria-controls~="${id}"][role~="combobox"]:not(select)`
);

return Array.from(ownedCombobox).some(isCombobox);
}

const isCombobox = node => node && getRole(node) === 'combobox';

function nearestParentWithRole(vNode) {
while ((vNode = vNode.parent)) {
if (getRole(vNode, { noPresentational: true }) !== null) {
return vNode;
}
}
return null;
}
11 changes: 8 additions & 3 deletions lib/rules/no-naming-method-matches.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getExplicitRole } from '../commons/aria';
import { querySelectorAll } from '../core/utils';
import getExplicitRole from '../commons/aria/get-explicit-role';
import isComboboxPopup from '../commons/aria/is-combobox-popup';
import getElementSpec from '../commons/standards/get-element-spec';
import querySelectorAll from '../core/utils/query-selector-all';

/**
* Filter out elements that have a naming method (i.e. img[alt], table > caption, etc.)
Expand All @@ -10,14 +11,18 @@ function noNamingMethodMatches(node, virtualNode) {
if (namingMethods && namingMethods.length !== 0) {
return false;
}

// Additionally, ignore combobox that get their name from a descendant input:
if (
getExplicitRole(virtualNode) === 'combobox' &&
querySelectorAll(virtualNode, 'input:not([type="hidden"])').length
) {
return false;
}
// Ignore listboxes that are referenced by a combobox
// Other roles don't require a name at all, or require one anyway
if (isComboboxPopup(virtualNode, { popupRoles: ['listbox'] })) {
return false;
}
return true;
}

Expand Down
90 changes: 18 additions & 72 deletions lib/rules/scrollable-region-focusable-matches.js
Original file line number Diff line number Diff line change
@@ -1,75 +1,21 @@
import { hasContentVirtual } from '../commons/dom';
import { getExplicitRole } from '../commons/aria';
import {
querySelectorAll,
getScroll,
closest,
getRootNode,
tokenList
} from '../core/utils';
import ariaAttrs from '../standards/aria-attrs';

function scrollableRegionFocusableMatches(node, virtualNode) {
/**
* Note:
* `excludeHidden=true` for this rule, thus considering only elements in the accessibility tree.
*/

/**
* if not scrollable -> `return`
*/
if (!!getScroll(node, 13) === false) {
return false;
}

/**
* ignore scrollable regions owned by combobox. limit to roles
* ownable by combobox so we don't keep calling closest for every
* node (which would be slow)
* @see https://github.com/dequelabs/axe-core/issues/1763
*/
const role = getExplicitRole(virtualNode);
if (ariaAttrs['aria-haspopup'].values.includes(role)) {
// in ARIA 1.1 the container has role=combobox
if (closest(virtualNode, '[role~="combobox"]')) {
return false;
}

// in ARIA 1.0 and 1.2 the combobox owns (1.0) or controls (1.2)
// the listbox
const id = virtualNode.attr('id');
if (id) {
const doc = getRootNode(node);
const owned = Array.from(
doc.querySelectorAll(`[aria-owns~="${id}"], [aria-controls~="${id}"]`)
);
const comboboxOwned = owned.some(el => {
const roles = tokenList(el.getAttribute('role'));
return roles.includes('combobox');
});

if (comboboxOwned) {
return false;
}
}
}

/**
* check if node has visible contents
*/
const nodeAndDescendents = querySelectorAll(virtualNode, '*');
const hasVisibleChildren = nodeAndDescendents.some(elm =>
hasContentVirtual(
elm,
true, // noRecursion
true // ignoreAria
)
import hasContentVirtual from '../commons/dom/has-content-virtual';
import isComboboxPopup from '../commons/aria/is-combobox-popup';
import { querySelectorAll, getScroll } from '../core/utils';

export default function scrollableRegionFocusableMatches(node, virtualNode) {
return (
// The element scrolls
getScroll(node, 13) !== undefined &&
// It's not a combobox popup, which commonly has keyboard focus added
isComboboxPopup(virtualNode) === false &&
// And there's something actually worth scrolling to
isNoneEmptyElement(virtualNode)
);
if (!hasVisibleChildren) {
return false;
}

return true;
}

export default scrollableRegionFocusableMatches;
function isNoneEmptyElement(vNode) {
return querySelectorAll(vNode, '*').some(elm =>
// (elm, noRecursion, ignoreAria)
hasContentVirtual(elm, true, true)
);
}
145 changes: 145 additions & 0 deletions test/commons/aria/is-combobox-popup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
describe('isComboboxPopup', () => {
const { isComboboxPopup } = axe.commons.aria;
const { queryFixture } = axe.testUtils;

it('does not match non-popup roles', () => {
const roles = ['main', 'combobox', 'textbox', 'button'];
for (const role of roles) {
const vNode = queryFixture(
`<div role="combobox" aria-controls="target"></div>
<div role="${role}" id="target"></div>`
);
assert.isFalse(isComboboxPopup(vNode));
}
});

for (const role of ['menu', 'listbox', 'tree', 'grid', 'dialog']) {
describe(role, () => {
it('is false when not related to the combobox', () => {
const vNode = queryFixture(
`<div role="combobox"></div>
<div role="${role}" id="target"></div>`
);
assert.isFalse(isComboboxPopup(vNode));
});

describe('using aria-controls (ARIA 1.2 pattern)', () => {
it('is true when referenced', () => {
const vNode = queryFixture(
`<div role="combobox" aria-controls="target"></div>
<div role="${role}" id="target"></div>`
);
assert.isTrue(isComboboxPopup(vNode));
});

it('is false when controlled by a select element', () => {
const vNode = queryFixture(
`<select role="combobox" aria-controls="target"></select>
<div role="${role}" id="target"></div>`
);
assert.isFalse(isComboboxPopup(vNode));
});

it('is false when not controlled by a combobox', () => {
const vNode = queryFixture(
`<div role="button combobox" aria-controls="target"></div>
<div role="${role}" id="target"></div>`
);
assert.isFalse(isComboboxPopup(vNode));
});
});

describe('using parent owned (ARIA 1.1 pattern)', () => {
it('is true when its a child of the combobox', () => {
const vNode = queryFixture(
`<div role="combobox">
<div role="${role}" id="target"></div>
</div>`
);
assert.isTrue(isComboboxPopup(vNode));
});

it('is false when its not a child of a real combobox', () => {
const vNode = queryFixture(
`<div role="button combobox">
<div role="${role}" id="target"></div>
</div>`
);
assert.isFalse(isComboboxPopup(vNode));
});

it('is false when its nearest parent with a role is not a combobox', () => {
const vNode = queryFixture(
`<div role="combobox">
<div role="region">
<div role="${role}" id="target"></div>
</div>
</div>`
);
assert.isFalse(isComboboxPopup(vNode));
});

it('is true when its nearest parent with a role is not a combobox', () => {
const vNode = queryFixture(
`<div role="combobox">
<div>
<div role="none">
<div role="presentation">
<div role="${role}" id="target"></div>
</div>
</div>
</div>
</div>`
);
assert.isTrue(isComboboxPopup(vNode));
});
});

describe('when using aria-owns (ARIA 1.0 pattern)', () => {
it('is true when referenced', () => {
const vNode = queryFixture(
`<div role="combobox" aria-owns="target"></div>
<div role="${role}" id="target"></div>`
);
assert.isTrue(isComboboxPopup(vNode));
});

it('is false when owned by a select element', () => {
const vNode = queryFixture(
`<select role="combobox" aria-owns="target"></select>
<div role="${role}" id="target"></div>`
);
assert.isFalse(isComboboxPopup(vNode));
});

it('is false when not owned by a combobox', () => {
const vNode = queryFixture(
`<div role="button combobox" aria-owns="target"></div>
<div role="${role}" id="target"></div>`
);
assert.isFalse(isComboboxPopup(vNode));
});
});
});
}

describe('options.popupRoles', () => {
it('allows custom popup roles', () => {
const vNode = queryFixture(
`<div role="combobox" aria-controls="target"></div>
<div role="button" id="target"></div>`
);
assert.isFalse(isComboboxPopup(vNode));
assert.isTrue(isComboboxPopup(vNode, { popupRoles: ['button'] }));
});

it('overrides the default popup roles', () => {
const vNode = queryFixture(
`<div role="combobox" aria-controls="target"></div>
<div role="listbox" id="target"></div>`
);
assert.isTrue(isComboboxPopup(vNode));
assert.isFalse(isComboboxPopup(vNode, { popupRoles: ['button'] }));
});
});
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
<!-- PASS -->
<!-- combobox -->
<div id="pass1" aria-label="country" role="combobox">England</div>
<div
id="pass1"
aria-label="country"
role="combobox"
aria-expanded="true"
aria-controls="inapplicable1"
>
England
</div>

<!-- Controlled by combobox: -->
<ul role="listbox" id="inapplicable1">
<li role="option">Zebra</li>
<li role="option" id="selected_option">Zoom</li>
</ul>

<!-- listbox -->
<p id="pass2Label">Select a color:</p>
<div id="pass2" role="listbox" aria-labelledby="pass2Label">
Expand Down Expand Up @@ -87,13 +102,13 @@
</label>

<!-- INAPPLICABLE -->
<input id="inapplicable1" />
<select id="inapplicable2">
<input id="inapplicable2" />
<select id="inapplicable3">
<option value="volvo">Volvo</option>
<option value="saab">Saab</option>
<option value="opel">Opel</option>
</select>
<textarea id="inapplicable3" title="Label"></textarea>
<textarea id="inapplicable4" title="Label"></textarea>

<!-- INCOMPLETE -->
<!-- Implicit label -->
Expand Down
Loading

0 comments on commit 3dcdd42

Please sign in to comment.