Skip to content
Open
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
117 changes: 111 additions & 6 deletions packages/react-aria-components/src/Checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,15 @@ import {HoverEvents} from '@react-types/shared';
import {LabelContext} from './Label';
import {mergeProps} from 'react-aria/mergeProps';
import {mergeRefs} from 'react-aria/mergeRefs';
import React, {createContext, ForwardedRef, forwardRef, Ref, useContext, useMemo} from 'react';
import React, {
createContext,
CSSProperties,
ForwardedRef,
forwardRef,
Ref,
useContext,
useMemo
} from 'react';
import {TextContext} from './Text';
import {useFocusRing} from 'react-aria/useFocusRing';
import {useHover} from 'react-aria/useHover';
Expand Down Expand Up @@ -90,6 +98,29 @@ export interface CheckboxProps
* A ref for the HTML input element.
*/
inputRef?: Ref<HTMLInputElement | null>;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* HTML input element.
*/
inputClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* HTML input element.
*/
inputStyle?: CSSProperties;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* VisuallyHidden wrapper around the HTML input element.
*/
visuallyHiddenClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* VisuallyHidden wrapper around the HTML input element. To make the screen reader focus ring
* match the component, stretch this wrapper to the label (e.g. `{inset: 0, width: 'auto', height: 'auto'}`)
* and set `position: relative` on the label (or a positioned ancestor) so the input resolves
* against it rather than the viewport.
*/
visuallyHiddenStyle?: CSSProperties;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

VisuallyHidden has highly specific styles right now that ensure it's visible to screen readers. If we expose all of these styles, it'd be pretty easy for people to accidentally make it not accessible.

For example, making it full width and height would probably show the native browser chrome for the input. The natural thing to do is then to use visibility hidden. This is not accessible.

Please include a story that you, the person, confirm this works on. Including screen readers. Once we have that maybe we can find some smaller API to expose, as we do not want to expose all of these props if possible.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right that exposing raw control over VisuallyHidden gives people a lot of rope, and the visibility: hidden trap is a real one - it is the natural thing to reach for and it does break screen reader exposure.

Two things I'd like to do:

  1. Add a story and validate it with a real screen reader. I'll add a story for Checkbox and Radio that stretches the hidden input over the component using the documented technique (visuallyHiddenStyle={{inset: 0, width: 'auto', height: 'auto'}} + inputStyle={{position: 'absolute', inset: 0}}), and confirm with VoiceOver that the focus ring tracks the component and the input stays exposed.

  2. Propose a smaller API. Instead of exposing all four props (inputClassName/inputStyle + visuallyHiddenClassName/visuallyHiddenStyle), I'd rather explore a single prop that encodes the safe pattern and keeps VisuallyHidden encapsulated. Something like a focusRing or inputPosition boolean that applies the correct positioning internally (label as containing block, wrapper stretched, input filling it) without handing out arbitrary style/className access. That achieves the fix for the reported issue with a much smaller surface, and it cannot be used to accidentally un-accessible the component.

My instinct is option 2 is the right shape for a library like this - the reported issue is specifically about the screen reader focus ring not tracking the component, and a boolean that encodes "make the hidden input cover the component" solves exactly that without exposing implementation details.

Want me to put together a proposal for the smaller prop (with the story + VoiceOver check) so we can compare it side by side against the current approach?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, let's explore a smaller api

}

export interface CheckboxFieldProps
Expand All @@ -110,6 +141,29 @@ export interface CheckboxFieldProps
* A ref for the HTML input element.
*/
inputRef?: Ref<HTMLInputElement | null>;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* HTML input element.
*/
inputClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* HTML input element.
*/
inputStyle?: CSSProperties;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* VisuallyHidden wrapper around the HTML input element.
*/
visuallyHiddenClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* VisuallyHidden wrapper around the HTML input element. To make the screen reader focus ring
* match the component, stretch this wrapper to the label (e.g. `{inset: 0, width: 'auto', height: 'auto'}`)
* and set `position: relative` on the label (or a positioned ancestor) so the input resolves
* against it rather than the viewport.
*/
visuallyHiddenStyle?: CSSProperties;
}

export interface CheckboxButtonProps
Expand All @@ -125,6 +179,29 @@ export interface CheckboxButtonProps
* @default 'react-aria-CheckboxButton'
*/
className?: ClassNameOrFunction<CheckboxButtonRenderProps>;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* HTML input element.
*/
inputClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* HTML input element.
*/
inputStyle?: CSSProperties;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* VisuallyHidden wrapper around the HTML input element.
*/
visuallyHiddenClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* VisuallyHidden wrapper around the HTML input element. To make the screen reader focus ring
* match the component, stretch this wrapper to the label (e.g. `{inset: 0, width: 'auto', height: 'auto'}`)
* and set `position: relative` on the label (or a positioned ancestor) so the input resolves
* against it rather than the viewport.
*/
visuallyHiddenStyle?: CSSProperties;
}

export interface CheckboxGroupRenderProps {
Expand Down Expand Up @@ -343,6 +420,10 @@ interface InternalCheckboxContextValue extends CheckboxAria {
defaultClassName: string;
isIndeterminate?: boolean;
isRequired?: boolean;
inputClassName?: string;
inputStyle?: CSSProperties;
visuallyHiddenClassName?: string;
visuallyHiddenStyle?: CSSProperties;
}

const InternalCheckboxContext = createContext<InternalCheckboxContextValue | null>(null);
Expand Down Expand Up @@ -406,7 +487,11 @@ export const CheckboxField = /*#__PURE__*/ (forwardRef as forwardRefType)(functi
inputRef,
defaultClassName: 'react-aria-CheckboxButton',
isIndeterminate: props.isIndeterminate,
isRequired: props.isRequired
isRequired: props.isRequired,
inputClassName: props.inputClassName,
inputStyle: props.inputStyle,
visuallyHiddenClassName: props.visuallyHiddenClassName,
visuallyHiddenStyle: props.visuallyHiddenStyle
}
],
[
Expand Down Expand Up @@ -476,7 +561,11 @@ export const Checkbox = /*#__PURE__*/ (forwardRef as forwardRefType)(function Ch
inputRef,
defaultClassName: 'react-aria-Checkbox',
isIndeterminate: props.isIndeterminate,
isRequired: props.isRequired
isRequired: props.isRequired,
inputClassName: props.inputClassName,
inputStyle: props.inputStyle,
visuallyHiddenClassName: props.visuallyHiddenClassName,
visuallyHiddenStyle: props.visuallyHiddenStyle
}}>
<CheckboxButton {...props} ref={ref} />
</InternalCheckboxContext.Provider>
Expand All @@ -501,11 +590,22 @@ export const CheckboxButton = /*#__PURE__*/ (forwardRef as forwardRefType)(funct
inputRef,
defaultClassName,
isIndeterminate,
isRequired
isRequired,
inputClassName,
inputStyle,
visuallyHiddenClassName,
visuallyHiddenStyle
} = useContext(InternalCheckboxContext)!;
let {isFocused, isFocusVisible, focusProps} = useFocusRing();
let isInteractionDisabled = isDisabled || isReadOnly;

// Allow inputClassName/inputStyle to be passed directly to CheckboxButton,
// taking precedence over values inherited from a wrapping Checkbox/CheckboxField.
inputClassName = props.inputClassName ?? inputClassName;
inputStyle = props.inputStyle ?? inputStyle;
visuallyHiddenClassName = props.visuallyHiddenClassName ?? visuallyHiddenClassName;
visuallyHiddenStyle = props.visuallyHiddenStyle ?? visuallyHiddenStyle;

let {hoverProps, isHovered} = useHover({
...props,
isDisabled: isInteractionDisabled
Expand Down Expand Up @@ -547,8 +647,13 @@ export const CheckboxButton = /*#__PURE__*/ (forwardRef as forwardRefType)(funct
data-readonly={isReadOnly || undefined}
data-invalid={isInvalid || undefined}
data-required={isRequired || undefined}>
<VisuallyHidden elementType="span">
<input {...mergeProps(inputProps, focusProps)} ref={inputRef} />
<VisuallyHidden elementType="span" className={visuallyHiddenClassName} style={visuallyHiddenStyle}>
<input
{...mergeProps(inputProps, focusProps)}
ref={inputRef}
className={inputClassName}
style={inputStyle}
/>
</VisuallyHidden>
{renderProps.children}
</dom.label>
Expand Down
130 changes: 123 additions & 7 deletions packages/react-aria-components/src/RadioGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,15 @@ import {LabelContext} from './Label';
import {mergeProps} from 'react-aria/mergeProps';
import {mergeRefs} from 'react-aria/mergeRefs';
import {RadioGroupState, useRadioGroupState} from 'react-stately/useRadioGroupState';
import React, {createContext, ForwardedRef, forwardRef, Ref, useContext, useMemo} from 'react';
import React, {
createContext,
CSSProperties,
ForwardedRef,
forwardRef,
Ref,
useContext,
useMemo
} from 'react';
import {SelectionIndicatorContext} from './SelectionIndicator';
import {SharedElementTransition} from './SharedElementTransition';
import {TextContext} from './Text';
Expand Down Expand Up @@ -90,6 +98,29 @@ export interface RadioProps
* A ref for the HTML input element.
*/
inputRef?: Ref<HTMLInputElement | null>;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* HTML input element.
*/
inputClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* HTML input element.
*/
inputStyle?: CSSProperties;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* VisuallyHidden wrapper around the HTML input element.
*/
visuallyHiddenClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* VisuallyHidden wrapper around the HTML input element. To make the screen reader focus ring
* match the component, stretch this wrapper to the label (e.g. `{inset: 0, width: 'auto', height: 'auto'}`)
* and set `position: relative` on the label (or a positioned ancestor) so the input resolves
* against it rather than the viewport.
*/
visuallyHiddenStyle?: CSSProperties;
}

export interface RadioFieldProps
Expand All @@ -109,6 +140,29 @@ export interface RadioFieldProps
* A ref for the HTML input element.
*/
inputRef?: Ref<HTMLInputElement | null>;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* HTML input element.
*/
inputClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* HTML input element.
*/
inputStyle?: CSSProperties;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* VisuallyHidden wrapper around the HTML input element.
*/
visuallyHiddenClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* VisuallyHidden wrapper around the HTML input element. To make the screen reader focus ring
* match the component, stretch this wrapper to the label (e.g. `{inset: 0, width: 'auto', height: 'auto'}`)
* and set `position: relative` on the label (or a positioned ancestor) so the input resolves
* against it rather than the viewport.
*/
visuallyHiddenStyle?: CSSProperties;
}

export interface RadioButtonProps
Expand All @@ -124,6 +178,29 @@ export interface RadioButtonProps
* @default 'react-aria-RadioButton'
*/
className?: ClassNameOrFunction<RadioButtonRenderProps>;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* HTML input element.
*/
inputClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* HTML input element.
*/
inputStyle?: CSSProperties;
/**
* The CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the
* VisuallyHidden wrapper around the HTML input element.
*/
visuallyHiddenClassName?: string;
/**
* The inline [style](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style) for the
* VisuallyHidden wrapper around the HTML input element. To make the screen reader focus ring
* match the component, stretch this wrapper to the label (e.g. `{inset: 0, width: 'auto', height: 'auto'}`)
* and set `position: relative` on the label (or a positioned ancestor) so the input resolves
* against it rather than the viewport.
*/
visuallyHiddenStyle?: CSSProperties;
}

export interface RadioGroupRenderProps {
Expand Down Expand Up @@ -364,7 +441,15 @@ export const Radio = /*#__PURE__*/ (forwardRef as forwardRefType)(function Radio

return (
<InternalRadioContext.Provider
value={{...aria, inputRef, defaultClassName: 'react-aria-Radio'}}>
value={{
...aria,
inputRef,
defaultClassName: 'react-aria-Radio',
inputClassName: props.inputClassName,
inputStyle: props.inputStyle,
visuallyHiddenClassName: props.visuallyHiddenClassName,
visuallyHiddenStyle: props.visuallyHiddenStyle
}}>
<RadioButton {...props} ref={ref} />
</InternalRadioContext.Provider>
);
Expand All @@ -373,6 +458,10 @@ export const Radio = /*#__PURE__*/ (forwardRef as forwardRefType)(function Radio
interface InternalRadioContextValue extends RadioAria {
inputRef: RefObject<HTMLInputElement | null>;
defaultClassName: string;
inputClassName?: string;
inputStyle?: CSSProperties;
visuallyHiddenClassName?: string;
visuallyHiddenStyle?: CSSProperties;
}

const InternalRadioContext = createContext<InternalRadioContextValue | null>(null);
Expand Down Expand Up @@ -438,7 +527,11 @@ export const RadioField = /*#__PURE__*/ (forwardRef as forwardRefType)(function
{
...aria,
inputRef,
defaultClassName: 'react-aria-RadioButton'
defaultClassName: 'react-aria-RadioButton',
inputClassName: props.inputClassName,
inputStyle: props.inputStyle,
visuallyHiddenClassName: props.visuallyHiddenClassName,
visuallyHiddenStyle: props.visuallyHiddenStyle
}
],
[
Expand All @@ -463,12 +556,30 @@ export const RadioButton = /*#__PURE__*/ (forwardRef as forwardRefType)(function
props: RadioButtonProps,
ref: ForwardedRef<HTMLLabelElement>
) {
let {labelProps, inputProps, isSelected, isDisabled, isPressed, defaultClassName, inputRef} =
useContext(InternalRadioContext)!;
let {
labelProps,
inputProps,
isSelected,
isDisabled,
isPressed,
defaultClassName,
inputRef,
inputClassName,
inputStyle,
visuallyHiddenClassName,
visuallyHiddenStyle
} = useContext(InternalRadioContext)!;
let state = React.useContext(RadioGroupStateContext)!;
let {isFocused, isFocusVisible, focusProps} = useFocusRing();
let interactionDisabled = isDisabled || state.isReadOnly;

// Allow inputClassName/inputStyle to be passed directly to RadioButton,
// taking precedence over values inherited from a wrapping Radio/RadioField.
inputClassName = props.inputClassName ?? inputClassName;
inputStyle = props.inputStyle ?? inputStyle;
visuallyHiddenClassName = props.visuallyHiddenClassName ?? visuallyHiddenClassName;
visuallyHiddenStyle = props.visuallyHiddenStyle ?? visuallyHiddenStyle;

let {hoverProps, isHovered} = useHover({
...props,
isDisabled: interactionDisabled
Expand Down Expand Up @@ -507,8 +618,13 @@ export const RadioButton = /*#__PURE__*/ (forwardRef as forwardRefType)(function
data-readonly={state.isReadOnly || undefined}
data-invalid={state.isInvalid || undefined}
data-required={state.isRequired || undefined}>
<VisuallyHidden elementType="span">
<input {...mergeProps(inputProps, focusProps)} ref={inputRef} />
<VisuallyHidden elementType="span" className={visuallyHiddenClassName} style={visuallyHiddenStyle}>
<input
{...mergeProps(inputProps, focusProps)}
ref={inputRef}
className={inputClassName}
style={inputStyle}
/>
</VisuallyHidden>
{renderProps.children}
</dom.label>
Expand Down
Loading