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

[Autocomplete] Fix aria-controls and aria-activedescendant #18142

Merged
merged 4 commits into from
Nov 3, 2019
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
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"version": "4.5.2",
"private": true,
"scripts": {
"postinstall": "patch-package",
"proptypes": "ts-node --skip-project ./scripts/generateProptypes.ts",
"deduplicate": "node scripts/deduplicate.js",
"argos": "argos upload test/regressions/screenshots/chrome --token $ARGOS_TOKEN",
Expand Down Expand Up @@ -111,6 +112,8 @@
"lerna": "^3.16.4",
"mocha": "^6.2.0",
"nyc": "^14.1.1",
"patch-package": "^6.2.0",
"postinstall-postinstall": "^2.0.0",
"prettier": "1.17.0",
"pretty-bytes": "^5.3.0",
"prop-types": "^15.7.2",
Expand Down
197 changes: 189 additions & 8 deletions packages/material-ui-lab/src/Autocomplete/Autocomplete.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,22 @@ import React from 'react';
import { expect } from 'chai';
import { createMount, getClasses } from '@material-ui/core/test-utils';
import describeConformance from '@material-ui/core/test-utils/describeConformance';
import { spy } from 'sinon';
import { createClientRender, fireEvent } from 'test/utils/createClientRender';
import TextField from '@material-ui/core/TextField';
import Autocomplete from './Autocomplete';
import TextField from '@material-ui/core/TextField';

describe('<Autocomplete />', () => {
let mount;
let classes;
const render = createClientRender({ strict: true });
const defaultProps = {
renderInput: params => <TextField {...params} label="defaultProps" />,
};

before(() => {
classes = getClasses(<Autocomplete {...defaultProps} />);
classes = getClasses(<Autocomplete renderInput={() => null} />);
mount = createMount({ strict: true });
});

describeConformance(<Autocomplete {...defaultProps} />, () => ({
describeConformance(<Autocomplete renderInput={() => null} />, () => ({
classes,
inheritComponent: 'div',
mount,
Expand All @@ -30,7 +28,9 @@ describe('<Autocomplete />', () => {

describe('combobox', () => {
it('should clear the input when blur', () => {
const { container } = render(<Autocomplete {...defaultProps} />);
const { container } = render(
<Autocomplete renderInput={params => <TextField {...params} />} />,
);
const input = container.querySelector('input');
input.focus();
fireEvent.change(input, { target: { value: 'a' } });
Expand All @@ -42,11 +42,192 @@ describe('<Autocomplete />', () => {

describe('multiple', () => {
it('should not crash', () => {
const { container } = render(<Autocomplete {...defaultProps} multiple />);
const { container } = render(
<Autocomplete renderInput={params => <TextField {...params} />} multiple />,
);
const input = container.querySelector('input');
input.focus();
document.activeElement.blur();
input.focus();
});
});

describe('WAI-ARIA conforming markup', () => {
specify('when closed', () => {
const { getAllByRole, getByRole, queryByRole } = render(
<Autocomplete renderInput={params => <TextField {...params} />} />,
);

const combobox = getByRole('combobox');
expect(combobox).to.have.attribute('aria-expanded', 'false');
// reflected aria-haspopup is `listbox`
// this assertion can fail if the value is `listbox`
expect(combobox).not.to.have.attribute('aria-haspopup');

const textbox = getByRole('textbox');
expect(combobox).to.contain(textbox);
// reflected aria-multiline has to be false i.e. not present or false
expect(textbox).not.to.have.attribute('aria-multiline');
expect(textbox).to.have.attribute('aria-autocomplete', 'list');
expect(textbox, 'no option is focused when openened').not.to.have.attribute(
'aria-activedescendant',
);

// popup is not only inaccessible but not in the DOM
const popup = queryByRole('listbox', { hidden: true });
expect(popup).to.be.null;

const buttons = getAllByRole('button');
expect(buttons).to.have.length(2);
// TODO: computeAccessibleName
expect(buttons[0]).to.have.attribute('title', 'Clear');
// TODO: computeAccessibleName
expect(buttons[1]).to.have.attribute('title', 'Open popup');
buttons.forEach(button => {
expect(button, 'button is not in tab order').to.have.property('tabIndex', -1);
});
});

specify('when open', () => {
const { getAllByRole, getByRole } = render(
<Autocomplete
open
options={['one', 'two ']}
renderInput={params => <TextField {...params} />}
/>,
);

const combobox = getByRole('combobox');
expect(combobox).to.have.attribute('aria-expanded', 'true');

const textbox = getByRole('textbox');

const popup = getByRole('listbox');
expect(combobox, 'combobox owns listbox').to.have.attribute(
'aria-owns',
popup.getAttribute('id'),
);
expect(textbox).to.have.attribute('aria-controls', popup.getAttribute('id'));
expect(textbox, 'no option is focused when openened').not.to.have.attribute(
'aria-activedescendant',
);

const options = getAllByRole('option');
expect(options).to.have.length(2);
options.forEach(option => {
expect(popup).to.contain(option);
});

const buttons = getAllByRole('button');
expect(buttons).to.have.length(2);
// TODO: computeAccessibleName
expect(buttons[0]).to.have.attribute('title', 'Clear');
// TODO: computeAccessibleName
expect(buttons[1]).to.have.attribute('title', 'Close popup');
buttons.forEach(button => {
expect(button, 'button is not in tab order').to.have.property('tabIndex', -1);
});
});
});

describe('when popup closed', () => {
it('opens when the textbox is focused', () => {
const handleOpen = spy();
render(
<Autocomplete
onOpen={handleOpen}
renderInput={params => <TextField autoFocus {...params} />}
/>,
);

expect(handleOpen.callCount).to.equal(1);
});

['ArrowDown', 'ArrowUp'].forEach(key => {
it(`opens on ${key} when focus is on the textbox without moving focus`, () => {
const handleOpen = spy();
const { getByRole } = render(
<Autocomplete
open={false}
onOpen={handleOpen}
renderInput={params => <TextField autoFocus {...params} />}
/>,
);

fireEvent.keyDown(document.activeElement, { key });

// first from focus
expect(handleOpen.callCount).to.equal(2);
expect(getByRole('textbox')).not.to.have.attribute('aria-activedescendant');
});
});

it('does not clear the textbox on Escape', () => {
const handleChange = spy();
render(
<Autocomplete
onChange={handleChange}
open={false}
options={['one', 'two']}
value="one"
renderInput={params => <TextField autoFocus {...params} />}
/>,
);

fireEvent.keyDown(document.activeElement, { key: 'Escape' });

expect(handleChange.callCount).to.equal(0);
});
});

describe('when popup open', () => {
it('closes the popup if Escape is pressed ', () => {
const handleClose = spy();
render(
<Autocomplete
onClose={handleClose}
open
options={['one', 'two ']}
renderInput={params => <TextField autoFocus {...params} />}
/>,
);

fireEvent.keyDown(document.activeElement, { key: 'Escape' });

expect(handleClose.callCount).to.equal(1);
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be called twice with jsdom actual because the implementation includes a .focus() noop that causes jsdom to fire a blur event which also calls onClose. In the browser this will work correctly.

});

it('moves focus to the first option on ArrowDown', () => {
const { getAllByRole, getByRole } = render(
<Autocomplete
options={['one', 'two ']}
renderInput={params => <TextField autoFocus {...params} />}
/>,
);

fireEvent.keyDown(document.activeElement, { key: 'ArrowDown' });

expect(getByRole('textbox')).to.have.attribute(
'aria-activedescendant',
getAllByRole('option')[0].getAttribute('id'),
);
});

it('moves focus to the last option on ArrowUp', () => {
const { getAllByRole, getByRole } = render(
<Autocomplete
options={['one', 'two ']}
renderInput={params => <TextField autoFocus {...params} />}
/>,
);

fireEvent.keyDown(document.activeElement, { key: 'ArrowUp' });

const options = getAllByRole('option');
expect(getByRole('textbox')).to.have.attribute(
'aria-activedescendant',
options[options.length - 1].getAttribute('id'),
);
});
});
});
13 changes: 9 additions & 4 deletions packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,10 @@ export default function useAutocomplete(props) {

function setHighlightedIndex(index, mouse = false) {
highlightedIndexRef.current = index;
inputRef.current.setAttribute('aria-activedescendant', `${id}-option-${index}`);
// does the index exist?
if (index !== -1) {
inputRef.current.setAttribute('aria-activedescendant', `${id}-option-${index}`);
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
}

if (!listboxRef.current) {
return;
Expand Down Expand Up @@ -650,8 +653,11 @@ export default function useAutocomplete(props) {
onBlur: handleBlur,
onFocus: handleFocus,
onChange: handleInputChange,
// if open then this is handled imperativeley so don't let react override
// only have an opinion about this when closed
'aria-activedescendant': popupOpen ? undefined : null,
'aria-autocomplete': autoComplete ? 'both' : 'list',
'aria-controls': `${id}-listbox`,
'aria-controls': `${id}-popup`,
// autoComplete: 'off', // Disable browser's suggestion that might overlap with the popup.
autoComplete: 'disabled', // disable autocomplete and autofill
ref: inputRef,
Expand All @@ -678,11 +684,10 @@ export default function useAutocomplete(props) {
}),
getPopupProps: () => ({
role: 'presentation',
id: `${id}-popup`,
}),
getListboxProps: () => ({
role: 'listbox',
id: `${id}-listbox`,
id: `${id}-popup`,
Copy link
Member

Choose a reason for hiding this comment

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

If the role of this element is listbox, should we suffix the id with -listbox?

Copy link
Member Author

Choose a reason for hiding this comment

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

The popup is the generic term for the "list" of possible items. It can also be a grid or tree. It makes more sense if you look at the aria-controls which doesn't care if you have a listbox or grid or tree. Just that the element in question is the popup with the possible items.

Copy link
Member

Choose a reason for hiding this comment

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

This seems correct, oops 🙃. What do you think of killing the getPopupProps() method and renaming PopupComponent to PopperComponent? (for future changes)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why Popup to Popper?

Copy link
Member

Choose a reason for hiding this comment

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

I thought that the popup was a different element to the listbox. As you raised it out, it's not. Because the default value for PopupComponent is a Popper, the change could make the API more intuitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that makes sense.

'aria-labelledby': `${id}-label`,
ref: handleListboxRef,
onMouseDown: event => {
Expand Down
15 changes: 15 additions & 0 deletions patches/jsdom+15.2.0.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
diff --git a/node_modules/jsdom/lib/jsdom/living/nodes/HTMLOrSVGElement-impl.js b/node_modules/jsdom/lib/jsdom/living/nodes/HTMLOrSVGElement-impl.js
index 468f263..2ef0288 100644
--- a/node_modules/jsdom/lib/jsdom/living/nodes/HTMLOrSVGElement-impl.js
+++ b/node_modules/jsdom/lib/jsdom/living/nodes/HTMLOrSVGElement-impl.js
@@ -45,6 +45,10 @@ class HTMLOrSVGElementImpl {

const previous = this._ownerDocument._lastFocusedElement;

+ if (previous === this) {
+ return;
+ }
+
focusing.fireFocusEventWithTargetAdjustment("blur", previous, this);
this._ownerDocument._lastFocusedElement = this;
focusing.fireFocusEventWithTargetAdjustment("focus", this, previous);
Loading