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

Accessibility – Remove use of onClick throughout the codebase (except within things like Button/SubmitButton) #68

Closed
mkrause opened this issue Dec 20, 2024 · 1 comment
Assignees
Labels
Milestone

Comments

@mkrause
Copy link
Collaborator

mkrause commented Dec 20, 2024

With a quick grep, we have the following places that do set an onClick event handler:

src/docs/Typography.mdx:- <ButtonAsLink onClick={linkTo(`typography/BodyText`)}>Body text</ButtonAsLink>
src/components/forms/context/SubmitButton/SubmitButton.tsx:    // `onClick` should not be used in most cases, only if the consumer needs low level control over click events.
src/components/forms/context/SubmitButton/SubmitButton.tsx:    props.onClick?.(event); // Call this first, to allow cancellation
src/components/forms/context/SubmitButton/SubmitButton.tsx:  }, [props.onClick, isInteractive, onPress, handlePress]);
src/components/forms/context/SubmitButton/SubmitButton.tsx:      onClick={handleClick}
src/components/forms/context/SubmitButton/SubmitButton.stories.tsx:            <p><SubmitButton variant="tertiary" label="Reset" onClick={resetErrorBoundary}/></p>
src/components/forms/controls/SegmentedControl/SegmentedControl.tsx:            onClick={() => { handleClick(option.value); } }
src/components/forms/controls/Select/Select.tsx:          onClick: () => { selectOption(option); },
src/components/overlays/Tooltip/Tooltip.stories.tsx:        Lorem ipsum dolor sit amet, <a href="/" onClick={event => { event.preventDefault(); }}>consectetur</a> adipiscing elit. Pellentesque eget sem ut neque lobortis pharetra nec vel quam. Etiam sem neque, gravida sed pharetra ut, vehicula quis lectus. Donec ac rhoncus purus. Proin ultricies augue vitae purus feugiat, in ultrices lorem aliquet. Donec eleifend ac dolor a auctor.
src/components/overlays/Tooltip/Tooltip.stories.tsx:        It has a <a href="/" onClick={event => { event.preventDefault(); }}>link</a> you can focus.
src/components/overlays/DropdownMenu/DropdownMenu.tsx:        onClick={() => { onActivate(context); }}
src/components/overlays/DropdownMenu/DropdownMenu.tsx:        onClick={() => { selectOption(option); }}
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.success(args)}>
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.info(args)}>
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.error(args)}>
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.error(args)}>
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.success(args)}>
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.success(args)}>
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.success(args)}>
src/components/overlays/Toast/Toast.stories.tsx:    <Button onClick={() => notify.success(args)}>
src/components/overlays/Toast/Toast.stories.tsx:        <Button onClick={() => notify.success(args)}>
src/components/overlays/Toast/Toast.stories.tsx:        <Button onClick={() => notify.info(args)}>
src/components/overlays/Toast/Toast.stories.tsx:        <Button onClick={() => notify.error(args)}>
src/components/overlays/Toast/Toast.stories.tsx:        <Button onClick={() => notify.success(args)}>
src/components/overlays/Toast/Toast.stories.tsx:        <Button onClick={() => notify.info(args)}>
src/components/overlays/Toast/Toast.stories.tsx:        <Button onClick={() => notify.error(args)}>
src/components/overlays/Toast/Toast.tsx:            onClick={handleCopy}
src/components/overlays/Toast/Toast.tsx:  <Icon icon="cross" className={cx(cl['bk-toast__action-close'])} onClick={closeToast} />
src/components/overlays/Modal/Modal.tsx:      onClick={handleDialogClick}
src/components/overlays/Modal/Modal.tsx:          onClick={close}
src/components/navigations/Tabs/Tabs.tsx:              onClick={() => { onSwitch(tab.props.tabKey); }}
src/components/navigations/Stepper/Stepper.tsx:            onClick={() => { onSwitch(step.stepKey); }}
src/components/actions/Button/Button.stories.tsx:            <p><Button variant="tertiary" label="Reset" onClick={resetErrorBoundary}/></p>
src/components/actions/Button/Button.tsx:    // `onClick` should not be used in most cases, only if the consumer needs low level control over click events.
src/components/actions/Button/Button.tsx:    props.onClick?.(event); // Call this first, to allow cancellation
src/components/actions/Button/Button.tsx:  }, [props.onClick, isInteractive, onPress, handlePress]);
src/components/actions/Button/Button.tsx:      onClick={handleClick}
src/components/actions/Link/Link.stories.tsx:      <a id="anchor" href="/" onClick={evt => { evt.preventDefault(); }}>Anchor</a>
src/components/containers/Dialog/Dialog.tsx:      onClick={handleDialogClick}
src/components/containers/Dialog/Dialog.tsx:        {/* <button autoFocus className="action" onClick={close}>✕</button> */}
src/components/containers/Banner/Banner.stories.tsx:        <Button variant="tertiary" onClick={() => alert('clicked')}>Button</Button>
src/components/containers/Banner/Banner.stories.tsx:        <Button variant="tertiary" onClick={() => alert('clicked')}>Button</Button>
src/components/containers/Banner/Banner.stories.tsx:        <Button variant="tertiary" onClick={() => alert('clicked')}>Button</Button>
src/components/containers/Banner/Banner.stories.tsx:        <Button variant="tertiary" onClick={() => alert('clicked')}>Button</Button>
src/components/containers/Banner/Banner.tsx:          onClick={() => setIsClosed(true)}
src/components/text/Tag/Tag.tsx:        <Icon icon="cross" className={cl['bk-tag__icon']} onClick={onRemove}/>

Most of these should be replaced. For accessibility, we should (1) only have click handlers on interactive elements like buttons and links, and (2) not just use onClick but allow for other means of activation like pressing the Enter key.

We can fix this by:

  • Replacing <Button onClick/> with <Button onPress={} aria-label="My action"/>
  • Note: make sure to add an aria-label as well, since the button needs an accessible name.
  • Making sure <Link/> never uses onClick, only to.
  • For onClick on non-interactive elements, wrap them in a <Button unstyled>...</Button> element.

We should also consider:

  • Adding a lint rule for any use of onClick (note: Biome already does this in many cases, but we're not yet enforcing it)
  • Adding documentation in the README recommending consumers to not use onClick and instead using the patterns mentioned above.
@mkrause
Copy link
Collaborator Author

mkrause commented Jan 22, 2025

This is mostly resolved now as of #113. Some of the work that's left (that will be done as part of other tickets):

  • Fix onClick being applied to an <li> in Tabs.tsx.
  • Triage uses of onClick in MultiSearch.tsx.
  • Investigate if there's a way that we can lint any usage of onClick. (Biome doesn't support custom rules yet unfortunately.)

@mkrause mkrause closed this as completed Jan 22, 2025
@mkrause mkrause added this to the Baklava v1.0 milestone Jan 22, 2025
@mkrause mkrause self-assigned this Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant