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

TokenInput field: try alternative approach to fix screen reader focus issue #44526

Merged
merged 6 commits into from
Sep 29, 2022
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- `Popover`: fix limitShift logic by adding iframe offset correctly [#42950](https://github.com/WordPress/gutenberg/pull/42950)).
- `Popover`: refine position-to-placement conversion logic, add tests ([#44377](https://github.com/WordPress/gutenberg/pull/44377)).
- `TokenInput`: improve logic around the `aria-activedescendant` attribute, which was causing unintended focus behavior for some screen readers ([#44526](https://github.com/WordPress/gutenberg/pull/44526)).

### Internal

Expand Down
5 changes: 0 additions & 5 deletions packages/components/src/combobox-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,6 @@ function ComboboxControl( {
instanceId={ instanceId }
ref={ inputContainer }
value={ isExpanded ? inputValue : currentLabel }
aria-label={
currentLabel
? `${ currentLabel }, ${ label }`
: null
}
Comment on lines -250 to -254
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I decided to delete those lines like in the original PR. See #44347 (comment) and #44347 (comment) for additional context and motivations for this change

(cc @tellthemachines )

onFocus={ onFocus }
onBlur={ onBlur }
isExpanded={ isExpanded }
Expand Down
23 changes: 22 additions & 1 deletion packages/components/src/form-token-field/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2057,7 +2057,12 @@ describe( 'FormTokenField', () => {

const suggestions = [ 'Pine', 'Pistachio', 'Sage' ];

render( <FormTokenFieldWithState suggestions={ suggestions } /> );
render(
<>
<FormTokenFieldWithState suggestions={ suggestions } />
<button>Click me</button>
</>
);

// No suggestions visible
const input = screen.getByRole( 'combobox' );
Expand Down Expand Up @@ -2093,6 +2098,22 @@ describe( 'FormTokenField', () => {
pineSuggestion.id
);

// Blur the input and make sure that the `aria-activedescendant`
// is removed
const button = screen.getByRole( 'button', { name: 'Click me' } );

await user.click( button );

expect( input ).not.toHaveAttribute( 'aria-activedescendant' );

// Focus the input again, `aria-activedescendant` should be added back.
await user.click( input );

expect( input ).toHaveAttribute(
'aria-activedescendant',
pineSuggestion.id
);

// Add the suggestion, which hides the list
await user.keyboard( '[Enter]' );

Expand Down
28 changes: 25 additions & 3 deletions packages/components/src/form-token-field/token-input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
* External dependencies
*/
import classnames from 'classnames';
import type { ChangeEvent, ForwardedRef } from 'react';
import type { ChangeEvent, ForwardedRef, FocusEventHandler } from 'react';

/**
* WordPress dependencies
*/
import { forwardRef } from '@wordpress/element';
import { forwardRef, useState } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -26,9 +26,13 @@ export function UnForwardedTokenInput(
selectedSuggestionIndex,
className,
onChange,
onFocus,
onBlur,
...restProps
} = props;

const [ hasFocus, setHasFocus ] = useState( false );

const size = value ? value.length + 1 : 0;

const onChangeHandler = ( event: ChangeEvent< HTMLInputElement > ) => {
Expand All @@ -39,6 +43,18 @@ export function UnForwardedTokenInput(
}
};

const onFocusHandler: FocusEventHandler< HTMLInputElement > = ( e ) => {
setHasFocus( true );
onFocus?.( e );
};

const onBlurHandler: React.FocusEventHandler< HTMLInputElement > = (
e
) => {
setHasFocus( false );
onBlur?.( e );
};

return (
<input
ref={ ref }
Expand All @@ -47,6 +63,8 @@ export function UnForwardedTokenInput(
{ ...restProps }
value={ value || '' }
onChange={ onChangeHandler }
onFocus={ onFocusHandler }
onBlur={ onBlurHandler }
size={ size }
className={ classnames(
className,
Expand All @@ -62,7 +80,11 @@ export function UnForwardedTokenInput(
: undefined
}
aria-activedescendant={
selectedSuggestionIndex !== -1
// Only add the `aria-activedescendant` attribute when:
// - the user is actively interacting with the input (`hasFocus`)
// - there is a selected suggestion (`selectedSuggestionIndex !== -1`)
// - the list of suggestions are rendered in the DOM (`isExpanded`)
hasFocus && selectedSuggestionIndex !== -1 && isExpanded
? `components-form-token-suggestions-${ instanceId }-${ selectedSuggestionIndex }`
: undefined
}
Expand Down