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

fix(action): Add screen reader support for active and indicator props #5875

Merged
merged 12 commits into from
Dec 6, 2022

Conversation

driskull
Copy link
Member

@driskull driskull commented Dec 2, 2022

Related Issue: #4813

Summary

fix(action): Add screen reader support for active and indicator properties. #4813

  • Uses aria-pressed to describe toggle state for active property
  • Uses aria-live region to describe Unsaved changes for indicator property

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Dec 2, 2022
@driskull driskull requested a review from geospatialem December 2, 2022 18:00
Copy link
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

Fantastic improvement, @driskull! 🙌🏻

We're giving context when the indicator is added to the component on-the-fly. It's missing context if an indicator is already present on the component.

It provides instant feedback, which is a great improvement and good starting point for the component.

A few comments below regarding the indicatorMessage prop, which would be awesome to have a default value assigned to it.

@driskull
Copy link
Member Author

driskull commented Dec 2, 2022

It's missing context if an indicator is already present on the component.

Not sure how we can do anything in this case. Do you know?

@geospatialem
Copy link
Member

Not sure how we can do anything in this case. Do you know?

If we have a default text value present in indicatorMessage and add context to the ariaLabel, we can provide context on load.

Very rough example where if present the value is read back with the aria-label, otherwise the empty string will ignore an indicator status:

const {
      active,
      compact,
      disabled,
      loading,
      textEnabled,
      label,
      text,
      indicator,
      indicatorId,
      indicatorMessage,
      intlIndicator,
      buttonId
    } = this;

    const indicatorPresent = indicator ? indicatorMessage ?? intlIndicator : "";

    const ariaLabel = label + indicatorPresent || text + indicatorPresent;

@driskull
Copy link
Member Author

driskull commented Dec 2, 2022

@geospatialem I could use a concatenated aria-label instead of the live region if thats the way we want to go. Let me know @geospatialem. I was concerned that concatenating wouldn't work well in all languages.

@geospatialem
Copy link
Member

geospatialem commented Dec 5, 2022

@geospatialem I could use a concatenated aria-label instead of the live region if thats the way we want to go. Let me know @geospatialem. I was concerned that concatenating wouldn't work well in all languages.

We'd still need the live region for providing context when the indicator is changed, so both would need to be present for context to AT users.

Good call on the concatenation for language support, @driskull what if we used a template literal, such as:

const indicatorStatus = indicator ? indicatorMessage ?? intlIndicator : "";
const ariaLabel = `${label} ${indicatorStatus}` || `${text} ${indicatorStatus}`;

@driskull
Copy link
Member Author

driskull commented Dec 5, 2022

Good call on the concatenation for language support, @driskull what if we used a template literal, such as:

I can do that if we don't think it will be an issue to combine the two strings.

@driskull
Copy link
Member Author

driskull commented Dec 5, 2022

@geospatialem can you test again?

@geospatialem
Copy link
Member

@geospatialem can you test again?

@driskull Looking great! 😎

JAWS transcript below, with a similar experience in NVDA, with indicatorMessage set as "My indicator message":

If the indicator is present on load:

hello world (My indicator message) �Toggle button� �Pressed� 
To toggle the state press spacebar.

after the indicator is removed:

hello world

and after the indicator is added:

hello world (My indicator message)

@driskull driskull marked this pull request as ready for review December 6, 2022 00:00
@driskull driskull requested a review from a team as a code owner December 6, 2022 00:00
@driskull driskull changed the title fix(action): Add screen reader support for active and indicator properties. #4813 fix(action): Add screen reader support for active and indicator properties Dec 6, 2022
@driskull driskull changed the title fix(action): Add screen reader support for active and indicator properties fix(action): Add screen reader support for active and indicator props Dec 6, 2022
@driskull
Copy link
Member Author

driskull commented Dec 6, 2022

Ready for review.

intlIndicator
} = this;

const ariaLabel = `${label || text}${indicator ? ` (${intlIndicator})` : ""}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

to support built-in localization for aira-label , t9nComponent interface should be implemented here.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm. it will be covered 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.

Yep. Can you approve?

Copy link
Contributor

@anveshmekala anveshmekala left a comment

Choose a reason for hiding this comment

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

🚀

@driskull driskull merged commit b6bcfa0 into master Dec 6, 2022
@driskull driskull deleted the dris0000/action-a11y-fixes branch December 6, 2022 21:24
@github-actions github-actions bot added this to the 2023 January Priorities milestone Dec 6, 2022
geospatialem added a commit that referenced this pull request Dec 8, 2022
…5938)

**Related Issue:** #4813  

## Summary
Updates the action component's `indicator` and `intlIndicator` prop
descriptions.

cc: #5875, #5895

Co-authored-by: Ben Elan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants