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.

Sign up for GitHub

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

Stepper accessibility fixes #4571

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Stepper accessibility fixes #4571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e189ec
feat(StepperStateless): add ariaLabel, ariaLive props
sarkaaa Jan 8, 2025
4eae2b6
feat(Stepper): add ariaLabel prop
sarkaaa Jan 8, 2025
3f6ed58
docs: add accessibility docs for Stepper(StepperStateless) component
sarkaaa Jan 9, 2025
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
34 changes: 34 additions & 0 deletions docs/src/documentation/03-components/07-interaction/stepper/03-accessibility.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
---
title: Accessibility
redirect_from:
- /components/stepper/accessibility/
---

## Accessibility

### Stepper

The Stepper component has been designed with accessibility in mind. It can be used with keyboard navigation and includes properties that enhance the experience for users of assistive technologies.

Although the `ariaLabel` and `ariaLive` props are optional for the Stepper (StepperStateless) component itself, it is recommended to fill them in to ensure that the component can be perceived by assistive technologies. This ensures that the Stepper (StepperStateless) component is accessible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't you add the same table as in the Seat Accessibility tab?

Copy link
Member Author

@sarkaaa sarkaaa Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

there's no specific reason why I added it as a section (as it's in some other components). but I agree we can have accessibility things only in accessibility tags.


Also, it is highly recommended to fill the `titleDecrement` and `titleIncrement` props in to provide a description of the decrement and increment buttons for screen readers.

#### Example

The content of children component is text, so it is read by screen reader.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is children component? I assume you are referring to StepperStateless or Button/Input/Button inside StepperStateless but those are again internals so I would choose different wording here.

Copy link
Member Author

Choose a reason for hiding this comment

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

leftover from another accessibility doc.


```jsx
<Stepper
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe add more elements to this example, as it probably wouldn't be placed in isolation but with some label? (This is based on the comment I put to the review with the image example, I just wanted to post it also here in case we add all these other props.)

step={1}
minValue={0}
minValue={10}
ariaLabel="Passengers number"
titleDecrement="Remove a passenger"
titleIncrement="Add a passenger"
ariaLive="assertive"
/>
```

The screen reader will announce stepper title (`Passengers number`) and buttons title (`Add a passenger`, `Remove a passenger`) once they are focused by screen reader.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it stepper title? I thought it's title of the input? From "stepper title" I would assume it's the title for the whole component, including the buttons and displayed text.

Will the screen reader first announce "Passengers number" and only then "Remove a passenger" and "Add a passenger"? It won't be "Remove a passenger", then "Passengers number" and then "Add a passenger"?

In case of changing the stepper value, the screen reader will announce the new value (`ariaLive` is set to `assertive` value).
68 changes: 40 additions & 28 deletions packages/orbit-components/src/Stepper/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,9 @@ Table below contains all types of the props available in Stepper component.
| onChange | `number => void \| Promise` | | Function for handling onClick event. |
| onFocus | `event => void \| Promise` | | Function for handling onFocus event. |
| step | `number` | `1` | Specifies the value of step to increment and decrement. |
| titleDecrement | `string \| (any => string)` | | Specifies `title` property on decrement `Button`. |
| titleIncrement | `string \| (any => string)` | | Specifies `title` property on increment `Button`. |

### size
Copy link
Member Author

@sarkaaa sarkaaa Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like size is not used, so I removed this part from the README file.


| size |
| :--------- |
| `"small"` |
| `"normal"` |
| titleDecrement | `string \| (any => string)` | | Specifies `ariaLabel` property on decrement `Button`. |
| titleIncrement | `string \| (any => string)` | | Specifies `ariaLabel` property on increment `Button`. |
| ariaLabel | `string` | | Optional prop for `aria-label` value. [See Accessibility](#accessibility). |

## Functional specs

Expand All @@ -61,25 +55,27 @@ import StepperStateless from "@kiwicom/orbit-components/lib/Stepper/StepperState

Table below contains all types of the props available in `StepperStateless` component.

| Name | Type | Default | Description |
| :---------------- | :-------------------------- | :------ | :------------------------------------------------------ |
| dataTest | `string` | | Optional prop for testing purposes. |
| disabled | `boolean` | `false` | If `true`, the Stepper will be disabled. |
| disabledIncrement | `boolean` | | If `true`, the increment `Button` will be disabled. |
| disabledDecrement | `boolean` | | If `true`, the decrement `Button` will be disabled. |
| maxValue | `number` | `∞` | Specifies the maximum value for the Stepper. |
| minValue | `number` | `-∞` | Specifies the minimum value for the Stepper. |
| name | `string` | | The name for the Stepper. |
| onBlur | `event => void \| Promise` | | Function for handling onBlur event. |
| onChange | `number => void \| Promise` | | Function for handling onClick event. |
| onDecrement | `event => void \| Promise` | | Function for handling decrement event.l |
| onFocus | `event => void \| Promise` | | Function for handling onFocus event. |
| onIncrement | `event => void \| Promise` | | Function for handling increment event. |
| onKeyDown | `event => void \| Promise` | | Function for handling onKeyDown event present on input. |
| step | `number` | `1` | Specifies the value of step to increment and decrement. |
| titleDecrement | `string \| (any => string)` | | Specifies `title` property on decrement `Button`. |
| titleIncrement | `string \| (any => string)` | | Specifies `title` property on increment `Button`. |
| value | `number \| string` | | Specifies the value of the StepperStateless. |
| Name | Type | Default | Description |
| :---------------- | :--------------------------------- | :------ | :------------------------------------------------------------------------- |
| dataTest | `string` | | Optional prop for testing purposes. |
| disabled | `boolean` | `false` | If `true`, the Stepper will be disabled. |
| disabledIncrement | `boolean` | | If `true`, the increment `Button` will be disabled. |
| disabledDecrement | `boolean` | | If `true`, the decrement `Button` will be disabled. |
| maxValue | `number` | `∞` | Specifies the maximum value for the Stepper. |
| minValue | `number` | `-∞` | Specifies the minimum value for the Stepper. |
| name | `string` | | The name for the Stepper. |
| onBlur | `event => void \| Promise` | | Function for handling onBlur event. |
| onChange | `number => void \| Promise` | | Function for handling onClick event. |
| onDecrement | `event => void \| Promise` | | Function for handling decrement event. |
| onFocus | `event => void \| Promise` | | Function for handling onFocus event. |
| onIncrement | `event => void \| Promise` | | Function for handling increment event. |
| onKeyDown | `event => void \| Promise` | | Function for handling onKeyDown event present on input. |
| step | `number` | `1` | Specifies the value of step to increment and decrement. |
| titleDecrement | `string \| (any => string)` | | Specifies `ariaLabel` property on decrement `Button`. |
| titleIncrement | `string \| (any => string)` | | Specifies `ariaLabel` property on increment `Button`. |
| value | `number \| string` | | Specifies the value of the StepperStateless. |
| ariaLabel | `string` | | Optional prop for `aria-label` value. [See Accessibility](#accessibility). |
| ariaLive | `"off" \| "assertive" \| "polite"` | | Optional prop for `aria-live` value. [See Accessibility](#accessibility). |

### Usage:

Expand Down Expand Up @@ -131,3 +127,19 @@ Helper function for validating decrement. Can be used with Stateless Stepper to
```js
validateDecrement({ value, minValue, step });
```

## Accessibility

- The `ariaLabel` prop allows you to specify an `aria-label` attribute for the Stepper input component. This attribute provides additional information to screen readers, enhancing the accessibility of the component. By using `ariaLabel`, you can ensure that users who rely on assistive technologies receive the necessary context and information about the component's purpose and functionality.

- The `titleDecrement` prop allows you to specify an `aria-label` attribute for the decrement icon button in the Stepper (StepperStateless) component. This attribute helps screen readers to announce the purpose of the decrement button, making it clear that it decreases the value.

- The `titleIncrement` prop allows you to specify an `aria-label` attribute for the increment icon button in the Stepper (StepperStateless) component. This attribute helps screen readers to announce the purpose of the increment button, making it clear that it increases the value.

- The `ariaLive` prop allows you to specify an `aria-live` attribute for the Stepper component. This attribute is used to inform assistive technologies of updates to the content.

- The `ariaDescribedBy` prop allows you to specify an `aria-describedby` attribute for the Stepper component. This attribute references the IDs of elements that describe the Stepper, providing additional context to screen readers.

- The `ariaControls` prop allows you to specify an `aria-controls` attribute for the Stepper component. This attribute references the ID of the element that the Stepper controls, helping users understand the relationship between the Stepper and the controlled element.

- The `ariaLabelledBy` prop allows you to specify an `aria-labelledby` attribute for the Stepper component. This attribute references the ID of the element that labels the Stepper, ensuring that screen readers announce the label correctly.
Comment on lines +130 to +145
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole section is very overwhelming.

  1. First of all, you didn't add ariaDescribedBy, ariaControls not ariaLabellerBy to the component, no?
  2. What is the difference between this section in the React tab and the Accessibility tab? I saw it both ways and I am not sure I understand the difference.
  3. I think this contains duplicate information with the info in the table above. E.g. The ariaLabelprop allows you to specify anaria-label attribute for the Stepper input component. vs Optional prop for aria-label value.. Also, you are talking about Stepper input component but that's talking about the internals of the component, I think this could be worded better.
  4. This part seems too wordy to me "This attribute provides additional information to screen readers, enhancing the accessibility of the component. By using ariaLabel, you can ensure that users who rely on assistive technologies receive the necessary context and information about the component's purpose and functionality.", I think this applies to all of these props and could be stated at the beginning/end.
  5. Same for titleDecrement/titleDecrement, they are already explained in the table.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. yeah, added accidentally. gp
  2. I agree, we can have only Accessibility tab for a11y descriptions.
  3. We have a similar approach for other components - a prop/aria-attribute is mentioned in the table (short description) and in the accessibility sections (enhanced description) as well. AFAIK we haven't set the approach we will always follow.
  4. 👍
  5. We have a similar approach for other components - a prop/aria-attribute is mentioned in the table (short description) and in the accessibility sections (enhanced description) as well. AFAIK we haven't set the approach we will always follow.

Regarding the duplication of the words, I can have it in a following form:

The following attributes provide additional information to screen readers, enhancing the accessibility of the respective component. By using them, you can ensure that users who rely on assistive technologies receive the necessary context and information about the component's purpose and functionality.

- The `ariaLabel` prop allows you to specify an `aria-label` attribute of the component.

- The `titleDecrement` prop allows you to specify an `aria-label` attribute for the decrement icon button in the Stepper (StepperStateless) component.

- The `titleIncrement` prop allows you to specify an `aria-label` attribute for the increment icon button in the Stepper (StepperStateless) component.

WDYT?

17 changes: 13 additions & 4 deletions packages/orbit-components/src/Stepper/Stepper.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,17 @@ export const Default: Story = {
disable: true,
},
},

args: {
ariaLabel: "Passengers number",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "Number of passengers" would be better name 🤔 (Same for the example in the docs)

titleIncrement: "Add a passenger",
titleDecrement: "Remove a passenger",
},
};

export const Playground: Story = {
args: {
...Default.args,
id: "stepper-ID",
name: "Passengers number",
step: 1,
Expand All @@ -36,8 +43,6 @@ export const Playground: Story = {
active: false,
disabled: false,
maxWidth: 120,
titleIncrement: "Add a passenger",
titleDecrement: "Remove a passenger",
onChange: action("onChange"),
onFocus: action("onFocus"),
onBlur: action("onBlur"),
Expand Down Expand Up @@ -83,9 +88,9 @@ export const Stateless: Story & StoryObj<typeof StatelessStepper> = {
};

export const Rtl: Story = {
render: () => (
render: args => (
<RenderInRtl>
<Stepper />
<Stepper {...args} />
</RenderInRtl>
),

Expand All @@ -95,4 +100,8 @@ export const Rtl: Story = {
disable: true,
},
},

args: {
...Default.args,
},
};
8 changes: 6 additions & 2 deletions packages/orbit-components/src/Stepper/StepperStateless/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ const StepperStateless = ({
titleDecrement,
disabledIncrement,
disabledDecrement,
ariaLabel,
ariaLive,
}: Props) => {
const theme = useTheme();

Expand Down Expand Up @@ -77,8 +79,8 @@ const StepperStateless = ({
onDecrement(ev);
}
}}
title={titleDecrement}
icons={iconStyles}
title={titleDecrement}
/>
<input
className={cx(
Expand All @@ -103,6 +105,8 @@ const StepperStateless = ({
}}
onBlur={onBlur}
onFocus={onFocus}
aria-label={ariaLabel}
aria-live={ariaLive}
Copy link
Member Author

Choose a reason for hiding this comment

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

tbh, I am not sure if it's the best idea to include aria-level attribute, but I included it as the devs might want to set up this attribute to ensure the user that the value in the input component was de/increased on key press (?). WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

What made you add aria-live here, did you see it recommended somewhere?

I was looking at documentation and found this:

Live regions are perceivable regions of a web page that are typically updated as a result of an external event when user focus may be elsewhere. These regions are not always updated as a result of a user interaction. Examples of live regions include a chat log, stock ticker, or a sport scoring section that updates periodically to reflect game statistics. Since these asynchronous areas are expected to update outside the user's area of focus, assistive technologies such as screen readers have either been unaware of their existence or unable to process them for the user.

If user is using Stepper component, his focus is on this component so I don't think we should be adding this here.

readOnly
/>
<ButtonPrimitive
Expand All @@ -115,8 +119,8 @@ const StepperStateless = ({
onIncrement(ev);
}
}}
title={titleIncrement}
icons={iconStyles}
title={titleIncrement}
Copy link
Member Author

@sarkaaa sarkaaa Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking of changing this prop name from title to ariaLabel in ButtonPrimitive and making it more specific, however, this would be (probably) BC. I searched where this prop is used and I've found it's used here.

I'm not sure if it makes sense to change this prop name, if it could be included within this PR or in a separate one.

Copy link
Contributor

@domihustinova domihustinova Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So your suggestion is to keep prop names in Stepper as titleIncrement and titleDecrement and change the prop on the ButtonPrimitive from title to ariaLabel so the usage would be ariaLabel={titleIncrement}?

But anyway, I think it's out of scope and this should be tackled when making Button/ButtonPrimitive accessible.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that wouldn't make sense. :D My suggestion was to rename aria attribute in ButtonPrimitive (probably in a different PR) and also change titleIncrement (titleDecrement). I kept titleDecrement (titleIncrement) because of attribute naming in ButtonPrimitive.

As you said, I also think it's out of the scope of this PR, that's the reason why I didn't touch ButtonPrimitive at all.

/>
</div>
);
Expand Down
4 changes: 4 additions & 0 deletions packages/orbit-components/src/Stepper/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ const Stepper = ({ onChange, defaultValue = 0, maxWidth = 108, ...props }: Props
titleIncrement,
titleDecrement,
active,
ariaLabel,
ariaLive,
} = props;
return (
<StepperStateless
Expand All @@ -63,6 +65,8 @@ const Stepper = ({ onChange, defaultValue = 0, maxWidth = 108, ...props }: Props
id={id}
value={value}
name={name}
ariaLabel={ariaLabel}
ariaLive={ariaLive}
titleIncrement={titleIncrement}
titleDecrement={titleDecrement}
/>
Expand Down
2 changes: 2 additions & 0 deletions packages/orbit-components/src/Stepper/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export interface SharedProps extends Common.Globals {
readonly maxWidth?: string | number;
readonly maxValue?: number;
readonly minValue?: number;
readonly ariaLabel?: string;
readonly ariaLive?: "off" | "assertive" | "polite";
// Deviation from other stepper properties
readonly titleIncrement?: string;
readonly titleDecrement?: string;
Expand Down
Loading