[Checkbox] properly support screen readers #4631

patrickracicot · 2021-11-11T20:50:34Z

WHY are these changes introduced?

The goal of this PR is too properly support screen readers on the Checkbox component. Currently, screen reader users must use a combination of space+enter key to be able to check/uncheck the checkbox.

WHAT is this pull request doing?

The current proposal modifies how the component handles the click and keyboard events. We now avoid hijacking the events on the Choice component. Instead we listen to the events directly on the <input /> element. The reason why we need to do this is due to the fact that screen readers are triggering the events directly on the <input /> element and this triggered event is not capturable by the onClick handler of the wrapping Choice component.

I also removed the onKeyPress handler of the <input />. According to the HTML spec, triggering the space keyboard event will activate (invoke the click event) of an interact-able element. To properly support the focus state, I added the onFocus handler on the <input /> since this was managed inside the onKeyPress previously which is a side effect.

For a deep dive please refer to the following document: Polaris Checkbox (in Draft Orders context)

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

To properly 🎩 these changes, one will need to build the repo using yarn dev.
The checkbox can be tested through the playbook and is actually used in the following examples

Step to 🎩

With a storybook example open
Tab to focus on the checkbox
Hit space to check
With the mouse, left click the box to uncheck
With the mouse, left click the label to check
With a screen reader, use the screen readers shortcut to activate the checkbox
- For mac VoiceOver, the combination is control+option+space

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, ping @ sarahill to update the Polaris UI kit

github-actions · 2021-11-11T20:58:59Z

size-limit report

Path	Size
cjs	166.19 KB (+0.01% 🔺)
esm	96.75 KB (+0.01% 🔺)
esnext	143.3 KB (-0.01% 🔽)
css	34.29 KB (0%)

patrickracicot · 2021-11-12T17:35:20Z

src/components/CheckableButton/tests/CheckableButton.test.tsx

-    it('is called when the CheckableButton pressed with spacebar', () => {
-      const spy = jest.fn();
-      const element = mountWithApp(
-        <CheckableButton {...CheckableButtonProps} onToggleAll={spy} />,
-      );
-
-      element.find(Checkbox)!.find('input')!.trigger('onKeyUp', {
-        keyCode: Key.Space,
-      });
-
-      expect(spy).toHaveBeenCalled();
-    });


Since we no longer manually handler the keyUp events, we no longer need to test this.

Same here? Since KeyUp is back, does this still stand?

Although the onKeyUp handler is back on the Checkbox.Input element, we aren't managing it the same way this test intends. The onKeyUp only manages the focusable state and no longer triggers value change (we let the subsequent click event triggered by the browser do this).

patrickracicot · 2021-11-12T17:38:17Z

src/components/Checkbox/tests/Checkbox.test.tsx

-    it('is not called from keyboard events when disabled', () => {
-      const spy = jest.fn();
-      const checkbox = mountWithApp(
-        <Checkbox label="label" disabled onChange={spy} />,
-      );
-      checkbox.find('input')!.trigger('onKeyUp', {
-        keyCode: Key.Enter,
-      });
-      expect(spy).not.toHaveBeenCalled();
-    });


Since we no longer manually handler the keyUp events, we no longer need to test this.

patrickracicot · 2021-11-12T17:38:24Z

src/components/Checkbox/tests/Checkbox.test.tsx

-    it('is called when space is pressed', () => {
-      const spy = jest.fn();
-      const element = mountWithApp(
-        <Checkbox id="MyCheckbox" label="Checkbox" onChange={spy} />,
-      );
-
-      element.find('input')!.trigger('onKeyUp', {
-        keyCode: Key.Space,
-      });
-
-      expect(spy).toHaveBeenCalledTimes(1);
-    });


Since we no longer manually handler the keyUp events, we no longer need to test this.

you brought back the keyUp, does this still stand?

See #4631 (comment)

patrickracicot · 2021-11-12T17:38:54Z

src/components/Checkbox/tests/Checkbox.test.tsx

-    it('can change values when disabled', () => {
-      const spy = jest.fn();
-      const checkbox = mountWithApp(
-        <Checkbox label="label" disabled onChange={spy} />,
-      );
-
-      checkbox.find('input')!.trigger('onKeyUp', {
-        keyCode: Key.Enter,
-      });
-      checkbox.setProps({checked: true});
-
-      expect(checkbox).toContainReactComponent('input', {
-        checked: true,
-      });
-    });


Since we no longer manually handler the keyUp events, we no longer need to test this.

kyledurand · 2021-11-15T15:33:35Z

I'm down to make this sort of change but I think we ran into issues with handling focus on checkboxes natively back in the day. I think @dleroux has context on why

NathanPJF

I've 🎩 'd on Mac+VoiceOver, JAWS, and NVDA; all works as expected

I think we ran into issues with handling focus on checkboxes natively back in the day

@kyledurand and @dleroux: I went looking through past GitHub issues about checkboxes and focus that involved Dan to see what more to check:

ResourceList #792, I found through tophatting that focus is managed as expected for the ResourceList with multi-select.
Styling in general #2661; and there isn't an issue with the changes in this PR as it still uses the keyFocused state here.

Those were the ones that stood out to me to double-check; happy to hear if there's an issue I missed.

dleroux · 2021-11-17T20:23:36Z

src/components/Checkbox/Checkbox.tsx


    const handleBlur = () => {
      onBlur && onBlur();
      setKeyFocused(false);


The reason we have this keyFocused on checkboxes is 2 folds:

We won't want the styles (focus ring) to be applied on click. Only when tabbing. This may be fixed with :focus-visible having better support now?

Also, since the focus right is on the backdrop, tabbing to the checkbox did not trigger the event to bubble and apply the styles. I believe this still an issue.

I just had a look and yes does show the focus rectangle on click now. so the classname should only be applied when it's a keyboard click.

Alright so I've added back the onKeyUp logic, but made some adjustments to it.

Instead of calling setKeyFocused(true) for every key up event, I only call it for the Space and Tab keyboard events. This will prevent an issue we had in the current implementation where if a user clicked on the checkbox and then type any key other then Tab or Space, it would still apply the focus style.

dleroux · 2021-11-17T20:24:13Z

src/components/Checkbox/Checkbox.tsx


 function noop() {}
-
-function stopPropagation<E>(event: React.MouseEvent<E>) {


IIRC this was also part of the focus issue

Pretty sure this had to do with the bulk actions. I would remove this.

Please refer to other PR comments on the IndexTable component in which I have added a fix to properly support the bulk actions.

TLDR; the root cause of the issue were the same as for the checkbox hence managing events on other DOM nodes then the originating element.

dleroux · 2021-11-17T20:52:57Z

This PR:

Screen.Recording.2021-11-17.at.3.50.54.PM.mov

Core:

Screen.Recording.2021-11-17.at.3.50.20.PM.mov

patrickracicot · 2021-11-18T14:06:46Z

src/components/IndexTable/components/Checkbox/Checkbox.tsx

-          onClick={onInteraction}
-          onKeyUp={onInteraction}
-          onChange={stopPropagation}


We want to avoid doing this as this is event bubbling management and isn't required. Instead, we can simply subscribe to the onChange prop of the PolarisCheckbox

dleroux · 2021-11-18T15:55:50Z

src/components/Checkbox/Checkbox.tsx

+  onChange?(
+    newChecked: boolean,
+    id: string,
+    event?: React.KeyboardEvent | React.MouseEvent<Element, MouseEvent>,


Do we need to pass the event here? There have been ongoing discussions about this and we have always opted to not pass the event. cc @kyledurand

@kyledurand @BPScott there was discussion in this repo https://github.com/orgs/Shopify/teams/polaris/discussions/ but since the team name changed I can't find it.

Fore more context here is the reasons why I'm passing the event inside the onChange handler.

Inside the onInteraction method of the RowContext requires this event as arg. It needs to ensure that we stop the propagation of the event further up in the DOM tree and avoids unwanted side-effects. Although I'm not a fan of always passing event through javascript method args, I feel like in this case it's necessary to avoid the need of further refactoring in the IndexTable.

Also the way the code was written before my proposal, we had a <div> wrapper around the Checkbox component which would listen to the onClick and onKeyUp events that were bubbling up. With this in mind, it's safe to say that passing the event through the method args serves the same purpose.

It was talk from back before the days that Github discussions existed: #521. (and #4111 is a recent-ish reitteration)

It's worth pointing out that exposing dom events as part of our public api plays badly with aspirations around Argo which has no way of moving these events around.

Passing dom events around internally is bearable, but them being part of the public API causes problems. The div wrapper to capture bubbling isn't great but it's better than creating APIs that are incompatible with Argo. That sad, if this is optional then it might not be the end of the world but it might lead to suprising behaviour. You'd need want somebody on the Argo team to give more context on this, as I'm mostly parroting old talking points as I've not been working with polaris-react on a day-to-day basis for a while.

#4111 (comment) - passing through an object containing exactly what is desired rather than tossing through the whole dom event might be an option

Thanks @BPScott for the context.

After reading and gathering context, I went back to the code and tried to see how I could avoid adding the event in our API while still properly supporting the event bubbling.

The fix

On the wrapping div inside the IndexTable/Checkbox, we only now only listen to the bubbling onClick event.

polaris-react/src/components/IndexTable/components/Checkbox/Checkbox.tsx

Lines 36 to 39 in dfb406c

<div

className={wrapperClassName}

onClick={onInteraction}

onKeyUp={noop}

Inside the Polaris/Checkbox component, we stop the propagation of onClick and onKeyUp events emerging from the spin.backdrop DOM element

polaris-react/src/components/Checkbox/Checkbox.tsx

Lines 180 to 184 in dfb406c

<span

className={backdropClassName}

onClick={stopPropagation}

onKeyUp={stopPropagation}

/>

The explanation
To be able to bind on the div.onClick on the wrapper we need to make sure that only the click events coming from the <Input /> control are bubbled up. Also since the activation (space for a checkbox) of a control will trigger the onClick event we need to ignore the onKeyUp events bubbled up to the wrapping div.

dleroux · 2021-11-18T16:24:29Z

Code looks good, so does 🎩 but let hear from others before opening up to passing the event.

UNRELEASED.md

dleroux

Code looks good, 🎩 too! Thanks @patrickracicot

patrickracicot force-pushed the pracicot/checkbox_a11y branch 2 times, most recently from 0372437 to 0b8ef91 Compare November 11, 2021 20:55

patrickracicot force-pushed the pracicot/checkbox_a11y branch 3 times, most recently from 900473b to 0a094f5 Compare November 12, 2021 17:15

patrickracicot self-assigned this Nov 12, 2021

patrickracicot added the Bug Something is broken and not working as intended in the system. label Nov 12, 2021

patrickracicot force-pushed the pracicot/checkbox_a11y branch 2 times, most recently from 265c780 to 9ad0133 Compare November 12, 2021 17:37

patrickracicot commented Nov 12, 2021

View reviewed changes

patrickracicot force-pushed the pracicot/checkbox_a11y branch 2 times, most recently from 2cd2ffc to 05e4131 Compare November 12, 2021 17:49

patrickracicot marked this pull request as ready for review November 12, 2021 18:00

patrickracicot requested review from NathanPJF, chloerice and svinkle November 12, 2021 18:00

kyledurand requested a review from dleroux November 15, 2021 15:33

NathanPJF approved these changes Nov 15, 2021

View reviewed changes

dleroux reviewed Nov 17, 2021

View reviewed changes

patrickracicot commented Nov 18, 2021

View reviewed changes

patrickracicot requested a review from dleroux November 18, 2021 14:07

patrickracicot force-pushed the pracicot/checkbox_a11y branch from 72970f9 to 4f88e86 Compare November 18, 2021 14:13

dleroux reviewed Nov 18, 2021

View reviewed changes

patrickracicot force-pushed the pracicot/checkbox_a11y branch from 7a8086d to 649051d Compare November 22, 2021 17:09

patrickracicot requested a review from BPScott November 22, 2021 18:54

patrickracicot requested a review from dleroux November 22, 2021 18:54

dleroux reviewed Nov 23, 2021

View reviewed changes

UNRELEASED.md Outdated Show resolved Hide resolved

[Checkbox] properly support screen readers

123c986

patrickracicot force-pushed the pracicot/checkbox_a11y branch from dfb406c to 123c986 Compare November 23, 2021 15:54

dleroux self-requested a review November 23, 2021 16:36

dleroux approved these changes Nov 23, 2021

View reviewed changes

patrickracicot merged commit 5572d7f into main Nov 23, 2021

patrickracicot deleted the pracicot/checkbox_a11y branch November 23, 2021 18:24


		function noop() {}

		function stopPropagation<E>(event: React.MouseEvent<E>) {

	<div
	className={wrapperClassName}
	onClick={onInteraction}
	onKeyUp={noop}

	<span
	className={backdropClassName}
	onClick={stopPropagation}
	onKeyUp={stopPropagation}
	/>

[Checkbox] properly support screen readers #4631

[Checkbox] properly support screen readers #4631

Uh oh!

Conversation

patrickracicot commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyledurand commented Nov 15, 2021

Uh oh!

NathanPJF left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux commented Nov 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BPScott Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux commented Nov 18, 2021

Uh oh!

Uh oh!

dleroux left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

patrickracicot commented Nov 11, 2021 •

edited

Loading

github-actions bot commented Nov 11, 2021 •

edited

Loading

dleroux Nov 17, 2021 •

edited

Loading

BPScott Nov 22, 2021 •

edited

Loading