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

CustomSelectControl V2: fix labelling with a visually hidden label #63137

Merged
merged 4 commits into from
Jul 8, 2024
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 @@ -35,6 +35,7 @@
- `CustomSelectControlV2`: fix select popover content overflow. ([#62844](https://github.com/WordPress/gutenberg/pull/62844))
- `CustomSelectControlV2`: keep legacy arrow down behavior only for legacy wrapper. ([#62919](https://github.com/WordPress/gutenberg/pull/62919))
- `CustomSelectControlV2`: fix trigger button font size. ([#63131](https://github.com/WordPress/gutenberg/pull/63131))
- `CustomSelectControlV2`: fix labelling with a visually hidden label. ([#63137](https://github.com/WordPress/gutenberg/pull/63137))
- Extract `TimeInput` component from `TimePicker` ([#60613](https://github.com/WordPress/gutenberg/pull/60613)).
- `TimeInput`: Add `label` prop ([#63106](https://github.com/WordPress/gutenberg/pull/63106)).
- Method style type signatures have been changed to function style ([#62718](https://github.com/WordPress/gutenberg/pull/62718)).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type * as Ariakit from '@ariakit/react';
import * as Ariakit from '@ariakit/react';

/**
* WordPress dependencies
Expand All @@ -25,6 +24,7 @@ import type {
} from './types';
import InputBase from '../input-control/input-base';
import SelectControlChevronDown from '../select-control/chevron-down';
import BaseControl from '../base-control';

export const CustomSelectContext =
createContext< CustomSelectContextType >( undefined );
Expand Down Expand Up @@ -113,13 +113,20 @@ function _CustomSelect(
return (
// Where should `restProps` be forwarded to?
<div className={ className }>
{ hideLabelFromVision ? ( // TODO: Replace with BaseControl
<VisuallyHidden as="label">{ label }</VisuallyHidden>
) : (
<Styled.SelectLabel store={ store }>
{ label }
</Styled.SelectLabel>
) }
<Ariakit.SelectLabel
store={ store }
render={
hideLabelFromVision ? (
// @ts-expect-error `children` are passed via the render prop
<VisuallyHidden />
) : (
// @ts-expect-error `children` are passed via the render prop
<BaseControl.VisualLabel as="div" />
Comment on lines +120 to +124
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the label HTML element is not correct in this case, as also confirmed by the ariakit docs:

Since it's not a native select element, we can't use the native label element.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting! I played around and confirmed that although a native label will work as expected in Chrome, it behaves differently in Firefox and Safari. Clicking on the label not only focuses but clicks the "select button", expanding the dropdown. So yeah, I guess it does need special handling 🫤

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's part of what Ariakit.SelectLabel does under the hood

)
}
>
{ label }
</Ariakit.SelectLabel>
<InputBase
__next40pxDefaultSize
size={ size }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,16 @@ describe.each( [
);
} );

it( 'Should label the component correctly even when the label is not visible', () => {
render( <Component { ...legacyProps } hideLabelFromVision /> );

expect(
screen.getByRole( 'combobox', {
name: legacyProps.label,
} )
).toBeVisible();
} );

describe( 'Keyboard behavior and accessibility', () => {
it( 'Captures the keypress event and does not let it propagate', async () => {
const onKeyDown = jest.fn();
Expand Down
8 changes: 0 additions & 8 deletions packages/components/src/custom-select-control-v2/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,6 @@ const getSelectItemSize = (
return sizes[ size ] || sizes.default;
};

export const SelectLabel = styled( Ariakit.SelectLabel )`
font-size: 11px;
font-weight: 500;
line-height: ${ CONFIG.fontLineHeightBase };
text-transform: uppercase;
margin-bottom: ${ space( 2 ) };
`;

export const Select = styled( Ariakit.Select, {
// Do not forward `hasCustomRenderProp` to the underlying Ariakit.Select component
shouldForwardProp: ( prop ) => prop !== 'hasCustomRenderProp',
Expand Down
10 changes: 10 additions & 0 deletions packages/components/src/custom-select-control-v2/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -436,4 +436,14 @@ describe.each( [
} )
).toBeVisible();
} );

it( 'Should label the component correctly even when the label is not visible', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding a test 🙌

render( <Component { ...defaultProps } hideLabelFromVision /> );

expect(
screen.getByRole( 'combobox', {
name: defaultProps.label,
} )
).toBeVisible();
} );
} );
10 changes: 10 additions & 0 deletions packages/components/src/custom-select-control/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,16 @@ describe.each( [
);
} );

it( 'Should label the component correctly even when the label is not visible', () => {
render( <Component { ...props } hideLabelFromVision /> );

expect(
screen.getByRole( 'button', {
name: props.label,
} )
).toBeVisible();
} );

describe( 'Keyboard behavior and accessibility', () => {
it( 'Captures the keypress event and does not let it propagate', async () => {
const user = userEvent.setup();
Expand Down
Loading