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

Jump to bottom

ARIA activedescendant, label #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ARIA activedescendant, label #78

wants to merge 1 commit into from

Conversation

Lewiscowles1986
Copy link
Contributor

Purpose (TL;DR)

To enable screen reader software and assistive ARIA aware technologies to announce each item as highlighted prior to selection.
Fixes #77

Background (Problem in detail)

Detailed in #77

Solution

Uses additional ARIA attributes to announce choices prior to selection, differentiating from selection announcement.
Supporting DOM changes. (likely "choice" constant will need to be configurable).

Other solutions considered

Please see ticket.

  • ARIA live region (did not announce on selection).

How to verify - mandatory

  1. Check out this branch
  2. npm ci
  3. npm run start
  4. use http://localhost:8080/
  5. download browser extension for Google Chrome (or any screenreader) link

Contributor declaration

  • I've read and followed the contributing document.
  • The code I am submitting is mine to submit and is done so under the license of this repository.
  • Where features have been changed, there are tests and documentation to accompany the entire contribution.
  • All tests remained passing as the behavior of these attributes is not owned by plete.
  • Documentation should not be necessary as this is a transparent change.
  • Please let me know. I expect the first-cut is an improvement, but may need further improvement.

Lewiscowles1986
Lewiscowles1986 commented Jan 22, 2021
View reviewed changes
lib/render-item.js Outdated
@@ -8,6 +8,7 @@ function renderItem(state, value) {
const attrValue = valueIsString ? value : value.id;
const label = valueIsString ? value : value.label;

item.setAttribute("aria-label", `Choice: ${label}.`);
Copy link
Contributor Author

@Lewiscowles1986 Lewiscowles1986 Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Stopped short of changing this as I felt you may have ideas, opinions, insights into the best place to make Choice: localizable.

Begs the question, should there be an optional localization parameter, config value, that can be stored in state, so server-side request components forward a locale from the initialization of the component?

It feels mildly out of scope for this, but perhaps having that machinery is more important than serving a particularly rigid feature under the guise of accessibility?

Copy link
Owner

Choose a reason for hiding this comment

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

Localisation does feel a bit out of scope here ... perhaps we can tackle it in a separate issue, so we can get this improvement over the finish line?

@Lewiscowles1986
Copy link
Contributor Author

😂 because of the CDN usage when deploying, the changes are not actually visible.
https://youtu.be/WKKFbsL9vao

mroderick
mroderick requested changes Jan 23, 2021
View reviewed changes
Copy link
Owner

@mroderick mroderick left a comment

Choose a reason for hiding this comment

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

Would you mind adding some test coverage that asserts that the correct ARIA label is being set?

lib/render-item.js Outdated
@@ -8,6 +8,7 @@ function renderItem(state, value) {
const attrValue = valueIsString ? value : value.id;
const label = valueIsString ? value : value.label;

item.setAttribute("aria-label", `Choice: ${label}.`);
Copy link
Owner

Choose a reason for hiding this comment

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

Localisation does feel a bit out of scope here ... perhaps we can tackle it in a separate issue, so we can get this improvement over the finish line?

@Lewiscowles1986 Lewiscowles1986 mentioned this pull request Jan 23, 2021
Open
2 tasks
@Lewiscowles1986 Lewiscowles1986 force-pushed the feature/aria-announce-options branch 2 times, most recently from 3692cd7 to 38d95b4 Compare January 23, 2021 19:29
@codecov-io
Copy link

codecov-io commented Jan 24, 2021 •
edited
Loading

Codecov Report

Merging #78 (f19cc50) into master (de63b80) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #78   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines          239       242    +3     
=========================================
+ Hits           239       242    +3
Flag Coverage Δ
unit 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/clear-suggestions.js 100.00% <100.00%> (ø)
lib/highlight.js 100.00% <100.00%> (ø)
lib/render.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de63b80...f19cc50. Read the comment docs.

@Lewiscowles1986
Copy link
Contributor Author

Hmm.. Says 100% coverage. Did I miss something the automated checkers missed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroderick mroderick Awaiting requested review from mroderick

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aria live regions
3 participants
@Lewiscowles1986 @codecov-io @mroderick