refactor: Removing aria attributes from Banner component

[williamjstanton]

Summary

The useBanner hook is adding the aria-labelledby and aria-describedby properties to the Banner button element. I removed the call to the hook so the aria attributes are not applied.

The Banner button is using aria-labelledby referencing the ActionText, and aria-describedby referencing the LabelText sub-component. It is difficult to point to measurable "problems" with this, because it does sound nice with screen readers. When isSticky prop is used, the ActionText disappears from the screen, while still being referenced as the main label for the button.

This will break the experience for Dragon Dictation and Voice Control users because the visible LabelText is not being used as the accessible name of the button element.

Release Category

Components

BREAKING CHANGES

We have removed the useBanner hook, the only function of which was to add aria-labelledby and aria-describedby references to the text inside of the Banner. This was not required for accessibility, and browsers can compute the name of the Banner from the text given inside.

---

Checklist

• MDX documentation adheres to Canvas Kit's Documentation Guidelines
• Label ready for review has been added to PR

For the Reviewer

• PR title is short and descriptive
• PR summary describes the change (Fixes/Resolves linked correctly)
• PR Release Notes describes additional information useful to call out in a release message or removed if not applicable
• Breaking Changes provides useful information to upgrade to this code or removed if not applicable

Where Should the Reviewer Start?

Only changed Banner.tsx; observed useBanner hook is returning the object with aria attributes;

Areas for Feedback? (optional)

• Code
• Documentation
• Testing
• Codemods

Testing Manually

1. Navigate to Storybook > Banner examples,
2. Inspect the Banner button element,
3. Validate that both aria-labelledby and aria-describedby are NOT set on the button,
4. Inspect the accessibility tree, and the name computation,
5. Validate the name: computes with the button element inner text strings.
6. Use the isSticky prop,
7. Repeat steps 4 - 5 again to validate the visible text in the button matches the computed name:.

Screenshots or GIFs (if applicable)

No change to the UI; aria does not impact UI.

Thank You Gif (optional)

[cypress]

1 flaky test on run #6134 ↗︎

0

935

2

0

1

Details:

Merge eb535bf into b0ece1d...
Project: canvas-kit	Commit: c470aac59c ℹ️
Status: Passed	Duration: 07:32 💡
Started: Sep 12, 2023 8:39 PM	Ended: Sep 12, 2023 8:47 PM

  cypress/integration/Autocomplete.spec.ts • 1 flaky test

View Output Video

Test		Artifacts
... > when a value is entered > when down arrow key is pressed > when the user presses the enter key > when the use hits the "2" key > should change the filtered results		Output Screenshots Video

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[mannycarrera4]

Hey @bstanton678 thanks for putting this up! I talked to Rusty, and correct me if I'm wrong but don't we want to still have an aria-labelledby when issticky is false? Maybe we can use something like https://github.com/Workday/canvas-kit/blob/master/modules/react/common/lib/styles/accessibleHide.ts on the ActionLabelText when isSticky is true and remove aria-labelledby when is sticky is true

[williamjstanton]

Hey @bstanton678 thanks for putting this up! I talked to Rusty, and correct me if I'm wrong but don't we want to still have an aria-labelledby when issticky is false? Maybe we can use something like https://github.com/Workday/canvas-kit/blob/master/modules/react/common/lib/styles/accessibleHide.ts on the ActionLabelText when isSticky is true and remove aria-labelledby when is sticky is true

We can do it that way if we'd like, but I am having a difficult time seeing the "value" in the complexity. When we don't use any aria at all, we get the sum inner text of the button: "{LabelText} {ActionText}, button"

With the aria: "{ActionText}, button, {LabelText}"

Another consideration: if a consumer would like to provide any text alternative (aria-label) for the Icon sub-component, our hook doesn't account for that. The aria-labelledby will blow that out.

[mannycarrera4]

Change makes sense! I' wondering if this is a breaking change? If not, can see it going into master instead. If it goes in major, we should a bit about the change in our upgrade guide!

[williamjstanton]

Change makes sense! I' wondering if this is a breaking change? If not, can see it going into master instead. If it goes in major, we should a bit about the change in our upgrade guide!

I didn't think that it was a breaking change, but the Lint PR failed and told me to put it in major. So I just did what it told me. I am happy to change the base to master if that is what you recommend. :D

[josh-bagwell]

Hey William,

So I discussed with the team and we decided we should remove the useBanner hook in it's entirety.

So here's what needs to be done moving forward:

• This will stay in prerelease/major
• An examples should be created to show the change and how users would add those attributes without the hook. Also, an example utilizing Tooltip to wrap the Banner component since that will apply the label.
• Update the "Upgrade Guide" for v10 to explain that the useBanner hook has been removed and why. This should also have a link to the updated examples that were created above
• Update "Breaking Change" in the PR summary that has mentioned this hook has been removed and why