Stepper accessibility fixes #4571
Open
+89
−18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook staging is available at https://kiwicom-orbit-sarka-stepper-a11y.surge.sh |
|
Size Change: +52 B (+0.01%) Total Size: 459 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
dbc4826
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://543061dd.orbit.pages.dev |
| Branch Preview URL: | https://sarka-stepper-a11y.orbit.pages.dev |
sarkaaa
commented
Jan 9, 2025
| | titleDecrement | `string \| (any => string)` | | Specifies `title` property on decrement `Button`. | | ||
| | titleIncrement | `string \| (any => string)` | | Specifies `title` property on increment `Button`. | | ||
|
|
||
| ### size |
There was a problem hiding this comment.
It looks like size is not used, so I removed this part from the README file.
sarkaaa
commented
Jan 9, 2025
| @@ -103,6 +105,8 @@ const StepperStateless = ({ | |||
| }} | |||
| onBlur={onBlur} | |||
| onFocus={onFocus} | |||
| aria-label={ariaLabel} | |||
| aria-live={ariaLive} | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
sarkaaa
commented
Jan 9, 2025
| icons={iconStyles} | ||
| title={titleIncrement} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
2 times, most recently
from
aa7965f to
b925e95
Compare
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
2 times, most recently
from
6640b0d to
3f6ed58
Compare
There was a problem hiding this comment.
I didn't leave comments for the aria-live related stuff as I think we shouldn't be adding it, but let's discuss that in the comment I posted.
Overall, by looking at the component, I am thinking if maybe we should connect the individual subcomponents so it's clear that they are connected. Looking at the usage, I found this example:
I guess we could add aria-labelledby to the Stepper so it's possible to connect these categories to the individual Steppers? You added it to the documentation, but you didn't added it to the component.
Also, maybe we could add aria-controls internally to the buttons to connect them with the displayed text? You again added it to the documentation and not the component, but I think this one could be added only internally to buttons/input and not as a prop to Stepper component. Maybe aria-controls is not the best solution for this, maybe you have more information and suggestions.
WDYT?
| @@ -103,6 +105,8 @@ const StepperStateless = ({ | |||
| }} | |||
| onBlur={onBlur} | |||
| onFocus={onFocus} | |||
| aria-label={ariaLabel} | |||
| aria-live={ariaLive} | |||
There was a problem hiding this comment.
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.
| icons={iconStyles} | ||
| title={titleIncrement} |
There was a problem hiding this comment.
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.
Comment on lines
130
to
145
|
|
||
| ## 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. |
There was a problem hiding this comment.
I think this whole section is very overwhelming.
- First of all, you didn't add
ariaDescribedBy,ariaControlsnotariaLabellerByto the component, no? - 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.
- I think this contains duplicate information with the info in the table above. E.g.
TheariaLabelprop allows you to specify anaria-labelattribute for the Stepper input component.vsOptional prop for aria-label value.. Also, you are talking aboutStepper input componentbut that's talking about the internals of the component, I think this could be worded better. - 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. - Same for
titleDecrement/titleDecrement, they are already explained in the table.
There was a problem hiding this comment.
- yeah, added accidentally. gp
- I agree, we can have only
Accessibilitytab for a11y descriptions. - 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.
- 👍
- 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?
|
|
||
| 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. |
There was a problem hiding this comment.
Why didn't you add the same table as in the Seat Accessibility tab?
There was a problem hiding this comment.
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.
|
|
||
| #### Example | ||
|
|
||
| The content of children component is text, so it is read by screen reader. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
leftover from another accessibility doc.
| @@ -23,10 +23,17 @@ export const Default: Story = { | |||
| disable: true, | |||
| }, | |||
| }, | |||
|
|
|||
| args: { | |||
| ariaLabel: "Passengers number", | |||
There was a problem hiding this comment.
Maybe "Number of passengers" would be better name 🤔 (Same for the example in the docs)
| /> | ||
| ``` | ||
|
|
||
| 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. |
There was a problem hiding this comment.
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"?
| The content of children component is text, so it is read by screen reader. | ||
|
|
||
| ```jsx | ||
| <Stepper |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
I added an example when using Stepper with a connected title (ariaLabelledBy prop is used).
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
2 times, most recently
from
ae01432 to
afe75c2
Compare
Adding |
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
from
afe75c2 to
24f1960
Compare
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
from
24f1960 to
dbc4826
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.tsCloses https://kiwicom.atlassian.net/browse/FEPLT-2195
This PR fixes these a11y violations:
✨
Description by Callstackai
This PR introduces accessibility improvements to the Stepper component by adding props for better screen reader support, including
ariaLabelandariaLabelledByattributes.Diagrams of code changes
sequenceDiagram participant User participant Screen Reader participant Stepper participant DecrementButton participant Input participant IncrementButton User->>Stepper: Interacts with component alt Using aria-label Screen Reader->>Input: Reads ariaLabel ("Number of passengers") else Using aria-labelledby Screen Reader->>Input: Reads referenced element text end User->>DecrementButton: Focuses Screen Reader->>DecrementButton: Reads titleDecrement ("Remove a passenger") User->>Input: Focuses Screen Reader->>Input: Reads current value User->>IncrementButton: Focuses Screen Reader->>IncrementButton: Reads titleIncrement ("Add a passenger")Files Changed
This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.mdx,.md. See list of supported languages.