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

fix(aria-input-field-name): skip combobox popups #3886

Merged
merged 7 commits into from
Jan 31, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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 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';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found a typo


// 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} optionsw
WilcoFiers marked this conversation as resolved.
Show resolved Hide resolved
* @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');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think we needed additional virtualNode tests, but let me know if I missed anything.

}
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) {
Copy link
Contributor Author

@WilcoFiers WilcoFiers Jan 25, 2023

Choose a reason for hiding this comment

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

We were using closest before, but this is more accurate and faster. It's not perfect though. We should really have a method that can tell you the owner. I spent more time on this PR than I expected to already, and didn't want to bloat this further. I opened a tech debt ticket: #3888

while ((vNode = vNode.parent)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
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, ['listbox'])) {
WilcoFiers marked this conversation as resolved.
Show resolved Hide resolved
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