Skip to content
Open
Show file tree
Hide file tree
Changes from 5 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
11 changes: 6 additions & 5 deletions packages/clerk-js/src/ui/elements/FormControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function useFormTextAnimation() {
transitionTimingFunction: t.transitionTiming.$common,
});
},
[prefersReducedMotion],
[prefersReducedMotion, appearanceAnimations],
);

return {
Expand Down Expand Up @@ -143,10 +143,9 @@ export const FormFeedback = (props: FormFeedbackProps) => {
return {
elementDescriptor: descriptor,
elementId: id ? descriptor?.setId?.(id) : undefined,
// We only want the id applied when the feedback type is an error
// to avoid having multiple elements in the dom with the same id attribute.
// We also only have aria-describedby applied to the input when it is an error.
id: type === 'error' ? errorMessageId : undefined,
// Generate unique IDs for all feedback types to enable aria-describedby usage.
// Use errorMessageId for errors to maintain existing behavior, otherwise generate a unique ID.
id: type === 'error' ? errorMessageId : `${id}-${type}-feedback`,
};
};

Expand Down Expand Up @@ -182,6 +181,7 @@ export const FormFeedback = (props: FormFeedbackProps) => {
getFormTextAnimation(!!feedbacks.a?.shouldEnter, { inDelay: true }),
]}
localizationKey={titleize(feedbacks.a?.feedback)}
aria-live={feedbacks.a?.shouldEnter ? 'polite' : 'off'}
/>
<InfoComponentB
{...getElementProps(feedbacks.b?.feedbackType)}
Expand All @@ -193,6 +193,7 @@ export const FormFeedback = (props: FormFeedbackProps) => {
getFormTextAnimation(!!feedbacks.b?.shouldEnter, { inDelay: true }),
]}
localizationKey={titleize(feedbacks.b?.feedback)}
aria-live={feedbacks.b?.shouldEnter ? 'polite' : 'off'}
/>
</Flex>
);
Expand Down
109 changes: 104 additions & 5 deletions packages/clerk-js/src/ui/elements/__tests__/PlainInput.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fireEvent, render } from '@testing-library/react';
import { fireEvent, render, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { describe, expect, it } from 'vitest';

Expand All @@ -21,6 +21,9 @@ const createField = (...params: Parameters<typeof useFormControl>) => {
{...props}
/>
<button onClick={() => field.setError('some error')}>set error</button>
<button onClick={() => field.setSuccess('some success')}>set success</button>
<button onClick={() => field.setWarning('some warning')}>set warning</button>
<button onClick={() => field.setInfo('some info')}>set info</button>
</>
);
});
Expand Down Expand Up @@ -132,15 +135,21 @@ describe('PlainInput', () => {
placeholder: 'some placeholder',
});

const { getByRole, getByLabelText, findByText } = render(<Field />, { wrapper });
const { getByRole, getByLabelText, findByText, container } = render(<Field />, { wrapper });

await userEvent.click(getByRole('button', { name: /set error/i }));

expect(await findByText(/Some Error/i)).toBeInTheDocument();

const label = getByLabelText(/some label/i);
expect(label).toHaveAttribute('aria-invalid', 'true');
expect(label).toHaveAttribute('aria-describedby', 'error-firstname');
const input = getByLabelText(/some label/i);
expect(input).toHaveAttribute('aria-invalid', 'true');
// The input should have both error IDs in aria-describedby
expect(input).toHaveAttribute('aria-describedby', 'error-firstname firstname-error-feedback');

// Verify the error message element has the correct ID
const errorElement = container.querySelector('#error-firstname');
expect(errorElement).toBeInTheDocument();
expect(errorElement).toHaveTextContent(/Some Error/i);
});

it('with info', async () => {
Expand All @@ -157,4 +166,94 @@ describe('PlainInput', () => {
fireEvent.focus(await findByLabelText(/some label/i));
expect(await findByText(/some info/i)).toBeInTheDocument();
});

it('with success feedback and aria-describedby', async () => {
const { wrapper } = await createFixtures();
const { Field } = createField('firstname', 'init value', {
type: 'text',
label: 'some label',
placeholder: 'some placeholder',
});

const { getByRole, getByLabelText, findByText, container } = render(<Field />, { wrapper });

await userEvent.click(getByRole('button', { name: /set success/i }));

expect(await findByText(/Some Success/i)).toBeInTheDocument();

const input = getByLabelText(/some label/i);
expect(input).toHaveAttribute('aria-invalid', 'false');
expect(input).toHaveAttribute('aria-describedby', 'firstname-success-feedback');

// Verify the success message element has the correct ID
const successElement = container.querySelector('#firstname-success-feedback');
expect(successElement).toBeInTheDocument();
expect(successElement).toHaveTextContent(/Some Success/i);
});

it('transitions between error and success feedback types', async () => {
const { wrapper } = await createFixtures();
const { Field } = createField('firstname', 'init value', {
type: 'text',
label: 'some label',
placeholder: 'some placeholder',
});

const { getByRole, getByLabelText, findByText, container } = render(<Field />, { wrapper });

// Start with error
await userEvent.click(getByRole('button', { name: /set error/i }));
expect(await findByText(/Some Error/i)).toBeInTheDocument();

let input = getByLabelText(/some label/i);
expect(input).toHaveAttribute('aria-invalid', 'true');
// Error includes both IDs for backwards compatibility and new behavior
expect(input).toHaveAttribute('aria-describedby', 'error-firstname firstname-error-feedback');

// Transition to success
await userEvent.click(getByRole('button', { name: /set success/i }));
expect(await findByText(/Some Success/i)).toBeInTheDocument();

input = getByLabelText(/some label/i);
expect(input).toHaveAttribute('aria-invalid', 'false');
expect(input).toHaveAttribute('aria-describedby', 'firstname-success-feedback');

// Verify success element exists with proper ID
const successElement = container.querySelector('#firstname-success-feedback');
expect(successElement).toBeInTheDocument();
expect(successElement).toHaveTextContent(/Some Success/i);
});

it('aria-live attribute is correctly applied', async () => {
const { wrapper } = await createFixtures();
const { Field } = createField('firstname', 'init value', {
type: 'text',
label: 'some label',
placeholder: 'some placeholder',
});

const { getByRole, findByText, container } = render(<Field />, { wrapper });

// Set error feedback
await userEvent.click(getByRole('button', { name: /set error/i }));
expect(await findByText(/Some Error/i)).toBeInTheDocument();

// Verify the visible error message has aria-live="polite"
const errorElement = container.querySelector('#error-firstname');
expect(errorElement).toHaveAttribute('aria-live', 'polite');

// Transition to success
await userEvent.click(getByRole('button', { name: /set success/i }));
expect(await findByText(/Some Success/i)).toBeInTheDocument();

// Verify the visible success message has aria-live="polite"
const successElement = container.querySelector('#firstname-success-feedback');
expect(successElement).toHaveAttribute('aria-live', 'polite');

// The previous error message should now have aria-live="off" (though it might still exist in DOM but hidden)
// We can verify the currently visible element has aria-live="polite"
const allAriaLivePolite = container.querySelectorAll('[aria-live="polite"]');
// At least the success message should have aria-live="polite"
expect(allAriaLivePolite.length).toBeGreaterThanOrEqual(1);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that me strictly equal to one ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, good catch! 651f11b

});
});
1 change: 0 additions & 1 deletion packages/clerk-js/src/ui/primitives/FormErrorText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ export const FormErrorText = forwardRef<HTMLElement, FormErrorTextProps>((props,
<Text
ref={ref}
colorScheme='danger'
aria-live='polite'
id={errorMessageId}
{...rest}
css={applyVariants(props)}
Expand Down
1 change: 0 additions & 1 deletion packages/clerk-js/src/ui/primitives/FormInfoText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ export const FormInfoText = forwardRef<HTMLElement, FormTextProps>((props, ref)
<Text
ref={ref}
colorScheme='secondary'
aria-live='polite'
id={errorMessageId}
{...props}
css={applyVariants(props)}
Expand Down
1 change: 0 additions & 1 deletion packages/clerk-js/src/ui/primitives/FormSuccessText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export const FormSuccessText = forwardRef<HTMLElement, FormTextProps>((props, re
<Text
ref={ref}
colorScheme='secondary'
aria-live='polite'
{...rest}
css={applyVariants(props) as any}
>
Expand Down
1 change: 0 additions & 1 deletion packages/clerk-js/src/ui/primitives/FormWarningText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export const FormWarningText = forwardRef<HTMLElement, FormTextProps>((props, re
<Text
ref={ref}
colorScheme='secondary'
aria-live='polite'
{...rest}
css={applyVariants(props) as any}
>
Expand Down
10 changes: 4 additions & 6 deletions packages/clerk-js/src/ui/primitives/Input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,9 @@ export type InputProps = PrimitiveProps<'input'> & StyleVariants<typeof applyVar

export const Input = React.forwardRef<HTMLInputElement, InputProps>((props, ref) => {
const fieldControl = useFormField() || {};
// @ts-expect-error Typescript is complaining that `errorMessageId` does not exist. We are clearly passing them from above.
const { errorMessageId, ignorePasswordManager, feedbackType, ...fieldControlProps } = sanitizeInputProps(
fieldControl,
['errorMessageId', 'ignorePasswordManager', 'feedbackType'],
);
// @ts-expect-error Typescript is complaining that `errorMessageId` and `feedbackMessageId` do not exist. We are clearly passing them from above.
const { errorMessageId, feedbackMessageId, ignorePasswordManager, feedbackType, ...fieldControlProps } =
sanitizeInputProps(fieldControl, ['errorMessageId', 'feedbackMessageId', 'ignorePasswordManager', 'feedbackType']);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Fix type definitions to eliminate ts-expect-error.

The ts-expect-error comment indicates that errorMessageId and feedbackMessageId are not properly typed in the return value of sanitizeInputProps. This bypasses type safety and could mask future type errors.

Update the return type of sanitizeInputProps in useFormField.tsx to explicitly include these properties when they're in the keep array:

export const sanitizeInputProps = <K extends keyof ReturnType<typeof useFormField>>(
  obj: ReturnType<typeof useFormField>,
  keep?: K[],
): Omit<ReturnType<typeof useFormField>, 
  'radioOptions' | 'validatePassword' | 'hasPassedComplexity' | 'isFocused' | 
  'feedback' | 'feedbackType' | 'setHasPassedComplexity' | 'setWarning' | 
  'setSuccess' | 'setError' | 'setInfo' | 'errorMessageId' | 'feedbackMessageId' | 
  'fieldId' | 'label' | 'clearFeedback' | 'infoText' | 'debouncedFeedback' | 
  'ignorePasswordManager' | 'transformer'
> & Pick<ReturnType<typeof useFormField>, K extends keyof ReturnType<typeof useFormField> ? K : never> => {
  // implementation
}

Alternatively, consider using a simpler approach with proper type assertions where the dynamic behavior is actually needed.


const propsWithoutVariants = filterProps({
...props,
Expand Down Expand Up @@ -106,7 +104,7 @@ export const Input = React.forwardRef<HTMLInputElement, InputProps>((props, ref)
required={_required}
id={props.id || fieldControlProps.id}
aria-invalid={_hasError}
aria-describedby={errorMessageId ? errorMessageId : undefined}
aria-describedby={[errorMessageId, feedbackMessageId].filter(Boolean).join(' ') || undefined}
aria-required={_required}
aria-disabled={_disabled}
data-feedback={feedbackType}
Expand Down
9 changes: 7 additions & 2 deletions packages/clerk-js/src/ui/primitives/hooks/useFormField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ type FormFieldProviderProps = ReturnType<typeof useFormControlUtil<FieldId>>['pr

type FormFieldContextValue = Omit<FormFieldProviderProps, 'id'> & {
errorMessageId?: string;
feedbackMessageId?: string;
id?: string;
fieldId?: FieldId;
hasError: boolean;
Expand Down Expand Up @@ -49,9 +50,10 @@ export const FormFieldContextProvider = (props: React.PropsWithChildren<FormFiel
// Both html attributes (e.g. data-invalid) and css styles depend on hasError being debounced
const hasError = debounced.feedbackType === 'error';

// Track whether the `FormErrorText` has been rendered.
// Track whether any feedback message has been rendered.
// We use this to append its id the `aria-describedby` of the `input`.
const errorMessageId = hasError ? `error-${propsId}` : '';
const feedbackMessageId = debounced.feedback ? `${propsId}-${debounced.feedbackType}-feedback` : '';
const value = React.useMemo(
() => ({
isRequired,
Expand All @@ -60,6 +62,7 @@ export const FormFieldContextProvider = (props: React.PropsWithChildren<FormFiel
id,
fieldId: propsId,
errorMessageId,
feedbackMessageId,
setError,
setSuccess,
setWarning,
Expand All @@ -76,6 +79,7 @@ export const FormFieldContextProvider = (props: React.PropsWithChildren<FormFiel
id,
propsId,
errorMessageId,
feedbackMessageId,
isDisabled,
setError,
setSuccess,
Expand Down Expand Up @@ -129,6 +133,7 @@ export const sanitizeInputProps = (
setError,
setInfo,
errorMessageId,
feedbackMessageId,
fieldId,
label,
clearFeedback,
Expand All @@ -143,7 +148,7 @@ export const sanitizeInputProps = (
/**
* Ignore error for the index type as we have defined it explicitly above
*/
// @ts-ignore
// @ts-ignore - Dynamic property access for form field props
inputProps[key] = obj[key];
});

Expand Down