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 UrlInput combobox to use the ARIA 1.0 pattern. #47148

Merged
merged 4 commits into from
Feb 1, 2023
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
2 changes: 2 additions & 0 deletions changelog.txt
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@
#### Block Library
- Lodash: Remove `_.pickBy()` from latest posts block. ([46974](https://github.com/WordPress/gutenberg/pull/46974))

### Accessibility
- Block Editor: Revert `aria-controls` to `aria-owns` in `URLInput` to use the more broadly supported ARIA 1.0 combobox pattern. ([47148](https://github.com/WordPress/gutenberg/pull/47148))

### Experiments

Expand Down
121 changes: 118 additions & 3 deletions packages/block-editor/src/components/link-control/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,122 @@ describe( 'Basic rendering', () => {
// Search Input UI.
const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

expect( searchInput ).toBeInTheDocument();
expect( searchInput ).toBeVisible();
} );

it( 'should have aria-owns attribute to follow the ARIA 1.0 pattern', () => {
render( <LinkControl /> );

// Search Input UI.
const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

expect( searchInput ).toBeVisible();
// Make sure we use the ARIA 1.0 pattern with aria-owns.
// See https://github.com/WordPress/gutenberg/issues/47147
expect( searchInput ).not.toHaveAttribute( 'aria-controls' );
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a good reason to have this assertion in place. I think we can remove it.

Suggested change
expect( searchInput ).not.toHaveAttribute( 'aria-controls' );

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'd like to make sure aria-selected is set only on the highlighted item and omitted from all the other items. The current usage is wrong and I'd like to to avoid future misuse. I'd agree it's best to move this to a separate test though.

Copy link
Member

Choose a reason for hiding this comment

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

I understand. thanks for clarifying. I guess it's worth adding some context as a comment to the test then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I see now you mentioned the assertion for aria-controls, not the one for aria-selected. Will add comments to clarify both.

expect( searchInput ).toHaveAttribute( 'aria-owns' );
} );

it( 'should have aria-selected attribute only on the highlighted item', async () => {
const user = userEvent.setup();

let resolver;
mockFetchSearchSuggestions.mockImplementation(
() =>
new Promise( ( resolve ) => {
resolver = resolve;
} )
);

render( <LinkControl /> );

// Search Input UI.
const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

// Simulate searching for a term.
await user.type( searchInput, 'Hello' );

// Wait for the spinner SVG icon to be rendered.
expect( await screen.findByRole( 'presentation' ) ).toBeVisible();
// Check the suggestions list is not rendered yet.
expect( screen.queryByRole( 'listbox' ) ).not.toBeInTheDocument();

// Make the search suggestions fetch return a response.
resolver( fauxEntitySuggestions );

const resultsList = await screen.findByRole( 'listbox', {
name: 'Search results for "Hello"',
} );

// Check the suggestions list is rendered.
expect( resultsList ).toBeVisible();
// Check the spinner SVG icon is not rendered any longer.
expect( screen.queryByRole( 'presentation' ) ).not.toBeInTheDocument();

const searchResultElements =
within( resultsList ).getAllByRole( 'option' );

expect( searchResultElements ).toHaveLength(
// The fauxEntitySuggestions length plus the 'Press ENTER to add this link' button.
fauxEntitySuggestions.length + 1
);

// Step down into the search results, highlighting the first result item.
triggerArrowDown( searchInput );

const firstSearchSuggestion = searchResultElements[ 0 ];
const secondSearchSuggestion = searchResultElements[ 1 ];

let selectedSearchResultElement = screen.getByRole( 'option', {
selected: true,
} );

// We should have highlighted the first item using the keyboard.
expect( selectedSearchResultElement ).toEqual( firstSearchSuggestion );

// Check the aria-selected attribute is set only on the highlighted item.
expect( firstSearchSuggestion ).toHaveAttribute(
'aria-selected',
'true'
);
// Check the aria-selected attribute is omitted on the non-highlighted items.
expect( secondSearchSuggestion ).not.toHaveAttribute( 'aria-selected' );

// Step down into the search results, highlighting the second result item.
triggerArrowDown( searchInput );

selectedSearchResultElement = screen.getByRole( 'option', {
selected: true,
} );

// We should have highlighted the first item using the keyboard.
expect( selectedSearchResultElement ).toEqual( secondSearchSuggestion );

// Check the aria-selected attribute is omitted on non-highlighted items.
expect( firstSearchSuggestion ).not.toHaveAttribute( 'aria-selected' );
// Check the aria-selected attribute is set only on the highlighted item.
expect( secondSearchSuggestion ).toHaveAttribute(
'aria-selected',
'true'
);

// Step up into the search results, highlighting the first result item.
triggerArrowUp( searchInput );

selectedSearchResultElement = screen.getByRole( 'option', {
selected: true,
} );

// We should be back to highlighting the first search result again.
expect( selectedSearchResultElement ).toEqual( firstSearchSuggestion );

// Check the aria-selected attribute is set only on the highlighted item.
expect( firstSearchSuggestion ).toHaveAttribute(
'aria-selected',
'true'
);
// Check the aria-selected attribute is omitted on non-highlighted items.
expect( secondSearchSuggestion ).not.toHaveAttribute( 'aria-selected' );
} );

it( 'should not render protocol in links', async () => {
Expand Down Expand Up @@ -559,7 +674,7 @@ describe( 'Manual link entry', () => {
} );

// Verify the UI hasn't allowed submission.
expect( searchInput ).toBeInTheDocument();
expect( searchInput ).toBeVisible();
expect( submitButton ).toBeDisabled();
expect( submitButton ).toBeVisible();
}
Expand Down Expand Up @@ -601,7 +716,7 @@ describe( 'Manual link entry', () => {
} );

// Verify the UI hasn't allowed submission.
expect( searchInput ).toBeInTheDocument();
expect( searchInput ).toBeVisible();
expect( submitButton ).toBeDisabled();
expect( submitButton ).toBeVisible();
}
Expand Down
5 changes: 3 additions & 2 deletions packages/block-editor/src/components/url-input/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ class URLInput extends Component {
'aria-label': label ? undefined : __( 'URL' ), // Ensure input always has an accessible label
'aria-expanded': showSuggestions,
'aria-autocomplete': 'list',
'aria-controls': suggestionsListboxId,
'aria-owns': suggestionsListboxId,
'aria-activedescendant':
selectedSuggestion !== null
? `${ suggestionOptionIdPrefix }-${ selectedSuggestion }`
Expand Down Expand Up @@ -531,7 +531,8 @@ class URLInput extends Component {
tabIndex: '-1',
id: `${ suggestionOptionIdPrefix }-${ index }`,
ref: this.bindSuggestionNode( index ),
'aria-selected': index === selectedSuggestion,
'aria-selected':
index === selectedSuggestion ? true : undefined,
};
};

Expand Down