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

Refactor ariaLabel props to aria-label #12802

Open
9 of 13 tasks
ItsMarcoMSF opened this issue Dec 6, 2022 · 15 comments · Fixed by #13364 · May be fixed by #16332
Open
9 of 13 tasks

Refactor ariaLabel props to aria-label #12802

ItsMarcoMSF opened this issue Dec 6, 2022 · 15 comments · Fixed by #13364 · May be fixed by #16332
Labels
afrohacks See https://ibm.biz/afrohacks-hackathon good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. planning: umbrella Umbrella issues, surfaced in Projects views type: a11y ♿ type: infrastructure 🤖 Issues relating to devops, tech debt, etc.

Comments

@ItsMarcoMSF
Copy link

ItsMarcoMSF commented Dec 6, 2022

Click to expand details

Package

carbon-components, carbon-components-react

Browser

Chrome

Operating System

MacOS

Package version

[email protected] & [email protected]

React version

v16.14.0

Automated testing tool and ruleset

IBM Equal Access Accessibility Checker

Assistive technology

No response

Description

According to @carbon/[email protected] storybook, the Modal component accepts a prop called aria-label.

Screenshot 2022-12-06 at 11 06 59 AM

However, in most other Carbon components in react, the prop ariaLabel is accepted

Screenshot 2022-12-06 at 11 29 13 AM

Can there be at least consistency between components in this area? A deeper dive into the source can also show that a number of components still accepts aria-label as a prop while the storybook says ariaLabel. And then there are other components that only accept one of ariaLabel or aria-label and not both.

According to React's official document on WAI-ARIA: https://reactjs.org/docs/accessibility.html#wai-aria:

Note that all aria-* HTML attributes are fully supported in JSX. Whereas most DOM properties and attributes in React are camelCased, these attributes should be hyphen-cased (also known as kebab-case, lisp-case, etc) as they are in plain HTML

So perhaps, the best approach is to support aria-label in all components?

WCAG 2.1 Violation

No response

Reproduction/example

Not applicable.

Steps to reproduce

Not applicable.

Code of Conduct


Plan of action

As described below, Components with the prop ariaLabel need to be refactored to aria-label. We wanted to ship this as part of v11, #9535, but it fell out of scope. There are currently 13 usages that need to be refactored: https://carbon-react-explorer.vercel.app/props/ariaLabel

Components to update

Steps to complete for each component

  • Deprecate ariaLabel by updating to use deprecate()
  • Add aria-label prop -- e.g. ['aria-label']: PropTypes.string,
  • Refactor usages of ariaLabel to prefer aria-label if it's provided. Both props need to work for the time being to remain backwards compatibility
@tay1orjones
Copy link
Member

tay1orjones commented Dec 9, 2022

Thanks for bringing this up @ItsMarcoMSF - yes, usages of ariaLabel should be converted to be aria-label instead.

We intended to ship this as part of v11, #9535, but it fell out of scope. There are currently 13 usages that need to be refactored: https://carbon-react-explorer.vercel.app/props/ariaLabel

@tay1orjones tay1orjones added type: infrastructure 🤖 Issues relating to devops, tech debt, etc. proposal: accepted This request has gone through triaging and we are accepting PR's against it. planning: umbrella Umbrella issues, surfaced in Projects views and removed status: needs triage 🕵️‍♀️ proposal: accepted This request has gone through triaging and we are accepting PR's against it. labels Dec 9, 2022
@tay1orjones tay1orjones changed the title [a11y]: ariaLabel vs aria-label Naming Convention Mismatch Refactor ariaLabel props to aria-label Dec 9, 2022
@tay1orjones
Copy link
Member

I just updated the issue title and body with a new "Plan of action" section outlining the work that needs done.

This issue is a lower priority item for our team, but we'll happily accept PRs if you're open to contributing!

@tay1orjones tay1orjones added needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. hacktoberfest See https://hacktoberfest.com/ good first issue 👋 Used by GitHub to elevate contribution opportunities and removed status: waiting for author's response 💬 labels Dec 9, 2022
@ItsMarcoMSF
Copy link
Author

@tay1orjones Thanks for replying to the issue. I'm open to contribute, let me know if you have a timeline in mind!

@tay1orjones
Copy link
Member

@ItsMarcoMSF Whatever timeline works for you! We won't be able to pick this up for quite some time unfortunately.

It's all in the contributing.md, but at a high level just fork the repo into your personal account, make a branch, make the changes, commit/push up, and open a PR. As this is a change to props on a component you may need to regenerate the public API snapshot via yarn test -u at the root after making your changes, but we can triage that in the pull request reviews.

Let me know if I can help get you up and running. You could start with a single component and just ref this issue #

@jsehull
Copy link
Contributor

jsehull commented Feb 24, 2023

Hey team - good find with the aria inconsistencies. I'm going to start working on some fixes for this.

@jsehull
Copy link
Contributor

jsehull commented Feb 27, 2023

@tay1orjones this is a rather large body of work for all of this. I could break it down into multiple PRs for merging to get some things out there door sooner. How would you prefer something like this be handled?

@jsehull
Copy link
Contributor

jsehull commented Feb 27, 2023

@tay1orjones Can you clarify what to do with the following regarding aria-label?

  1. FilterableMultiSelect: on the list, but does not have it
  2. NumberInput: on the list, but does not have it
  3. Notifications
    1. ActionableNotification has aria-label but is not on Issue list above. Update or remove?
    1. ToastNotification has the propType listed, but it is not currently used. Update or remove?
  1. InlineCheckbox is not active in Storybook nor is it listed as an subitem under Checkbox. Ignore? Remove?
  • Note: there is a current request for checkboxes in rows. Is this similar enough that it needs to be kept and could be used towards a solution here instead of trying to pass an "inline prop" to the current Checkbox?
  • Issue: [Feature]: Checkboxes improperly format as a row#11803

@tay1orjones
Copy link
Member

@jsehull Yeah, I would sequence it out into separate PRs per component to get it out the door sooner.

The list of components to refactor was generated by searching the codebase for ariaLabel: PropTypes. Every usage listed there needs to be refactored to use aria-label unless the prop is not used (and not spread onto a child via ...other or similar)

InlineCheckbox is used in a few spots - TableSelectRow, TableSelectAllRow, etc. and can be viewed in those DataTable stories using those components.

Also I think we can just ignore any feature requests for now and focus on updating the existing API in the state it's in.

@jsehull
Copy link
Contributor

jsehull commented Feb 28, 2023

Below are some of my progress updates in case others are monitoring.

Items removed:

  1. NumberInput

Not started:

In process:
3. all 4 table related items (half done)

  • for some I get the new aria-label working but am having trouble keeping the deprecated ariaLabel functional

Completed code

Done (PR created)

  1. StructuredListWrapper
  2. Dropdown
  3. ComboBox
  4. CodeSnippet
  5. FilterableMultiSelect
  6. OverflowMenu
  7. Notification
  8. InlineCheckbox

@jsehull
Copy link
Contributor

jsehull commented Mar 3, 2023

8 of 12 completed and pending PR approvals.

4 of 12 remaining:

  • TableSelectRow
  • TableSelectAll
  • TableExpandRow
  • TableExpandHeader

The table section is proving more challenging, though the fix should be nearly the same. I'm having varying degrees of success getting those working.

At best, for some, I get the new aria-label working but am having trouble keeping the deprecated ariaLabel functional


For now I'm moving on to other Issues. Anyone is welcome to hop in and try these out!

Whoever works on this next can reference recent fixes as the updates should be similar.


Once a solution is found and properly tested, I run the following to ensure that snapshots are updated. It's common to have to fix tests afterwards. DataTable has many tests!

yarn && yarn build && yarn test -u

@tw15egan
Copy link
Member

Still a few left to complete

@tw15egan tw15egan reopened this Mar 22, 2023
@TannerS
Copy link
Contributor

TannerS commented Mar 28, 2023

@tay1orjones @tw15egan do yall mind if i help out with this?

@tw15egan
Copy link
Member

Go for it @TannerS 🎉

@fitrahmunir
Copy link

@tay1orjones @tw15egan May I help by working on this?

@tw15egan
Copy link
Member

Sure thing @fitrahfm 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
afrohacks See https://ibm.biz/afrohacks-hackathon good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. planning: umbrella Umbrella issues, surfaced in Projects views type: a11y ♿ type: infrastructure 🤖 Issues relating to devops, tech debt, etc.
Projects
Status: Ready for community implementation
Status: In Progress
6 participants