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.

Sign up for GitHub

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

Jump to bottom

Fix UrlInput combobox to use the ARIA 1.0 pattern. #47148

Merged
merged 4 commits into from
Feb 1, 2023
Merged

Fix UrlInput combobox to use the ARIA 1.0 pattern. #47148

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2b06d0
Fix UrlInput combobox to use the ARIA 1.0 pattern.
afercia Jan 13, 2023
228b4bf
Update changelog.
afercia Jan 13, 2023
e32866a
Improve testing for correct ARIA combobox pattern.
afercia Jan 16, 2023
aaa647d
Add and clarify test comments.
afercia Feb 1, 2023
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
View file
Open in desktop
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
38 changes: 38 additions & 0 deletions packages/block-editor/src/components/link-control/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,17 @@ describe( 'Basic rendering', () => {
expect( searchInput ).toBeInTheDocument();
} );

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 ).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to change this assertion to use toBeVisible()? I've found this to be a bit more specific query, since toBeInTheDocument() will be truthy if the element is not accessible to the user at all.

Suggested change
expect( searchInput ).toBeInTheDocument();
expect( searchInput ).toBeVisible();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. There are a few more occurrences of toBeInTheDocument(). Some of them use not and they actually test for some elements to not be in the DOM. I guess the other ones should be changed to toBeVisible().

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 not render protocol in links', async () => {
const user = userEvent.setup();
mockFetchSearchSuggestions.mockImplementation( () =>
Expand Down Expand Up @@ -1375,6 +1386,15 @@ describe( 'Selecting links', () => {
firstSearchSuggestion
);

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

// Check we can go down again using the down arrow.
triggerArrowDown( searchInput );

Expand All @@ -1387,6 +1407,15 @@ describe( 'Selecting links', () => {
secondSearchSuggestion
);

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

// Check we can go back up via up arrow.
triggerArrowUp( searchInput );

Expand All @@ -1399,6 +1428,15 @@ describe( 'Selecting links', () => {
firstSearchSuggestion
);

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

expect( mockFetchSearchSuggestions ).toHaveBeenCalledTimes( 1 );
} );
} );
Expand Down
5 changes: 3 additions & 2 deletions packages/block-editor/src/components/url-input/index.js
View file
Open in desktop
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