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

Adds selection state of elements for screenreader #19360

Merged
merged 4 commits into from
Dec 19, 2018

Conversation

adeelkhan
Copy link
Contributor

@adeelkhan adeelkhan commented Dec 3, 2018

Dashboard header was missing aria-current and
aria-selected attributes causing no information
exposed to screen reader about selection.This
would enable such info to be passed to screen
reader

LEARNER-6611

@adeelkhan adeelkhan changed the title Adds selection state to elements for screenreader Adds selection state of elements for screenreader Dec 3, 2018
@adeelkhan adeelkhan force-pushed the adeel/learner_6611_exposing_selection_state branch from 5644e02 to ae7136f Compare December 3, 2018 20:43
Copy link
Contributor

@MichaelRoytman MichaelRoytman left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'm going to hold off on approving it until the failing accessibility tests are passing.

@wittjeff
Copy link

wittjeff commented Dec 4, 2018

jenkins run a11y

@wittjeff
Copy link

wittjeff commented Dec 4, 2018

The problem is a role/state mismatch. See https://www.stefanjudis.com/blog/aria-selected-and-when-to-use-it/.
I'll look into a suggestion for a fix.

@wittjeff
Copy link

wittjeff commented Dec 4, 2018

I think this will work if we just use aria-current="page" and omit aria-selected (which was an older method before aria-current came in with ARIA1.1).

@adeelkhan adeelkhan force-pushed the adeel/learner_6611_exposing_selection_state branch from ae7136f to 468abe2 Compare December 5, 2018 11:14
@wittjeff
Copy link

wittjeff commented Dec 5, 2018

jenkins run a11y

1 similar comment
@adeelkhan
Copy link
Contributor Author

jenkins run a11y

@fysheets
Copy link
Contributor

fysheets commented Dec 6, 2018

Hey @adeelkhan can we spin up a sandbox for this? Based on the notes from this URL the default for aria-current should be false. Based on this line of code: https://github.com/edx/edx-platform/pull/19360/files#diff-9e702e24e0c4050c5f6038511aa82b29R34 I would expect that by setting else '' it would default false and work as expected. But if that is not happening I think actually seeing the code in action would be useful for determining why. Also, is there a reason why we can't just call out else 'false' here instead of empty string?

@adeelkhan adeelkhan force-pushed the adeel/learner_6611_exposing_selection_state branch 2 times, most recently from 7340164 to 5fff6c7 Compare December 6, 2018 20:36
@wittjeff
Copy link

wittjeff commented Dec 6, 2018

Noting for the record that the Axe tests were failing due to the package (v1.1) not recognizing aria-current. Aria-current rules were added with v2.2. Adeel is testing a bump to current version (v3.1.2). There's also this issue regarding aria-current="". dequelabs/axe-core#994

@MichaelRoytman
Copy link
Contributor

I tried bumping the version of axe-core in edx/edx-custom-a11y-rules and installed from my branch into edx-platform to see if that was the culprit, but it looks like it's still failing. Either my branch was not installed correctly into edx-platform, or the version of axe-core in edx/edx-custom-a11y-rules is not relevant for these tests.

https://github.com/edx/edx-custom-a11y-rules/tree/mroytman/update-axe-core

@adeelkhan adeelkhan force-pushed the adeel/learner_6611_exposing_selection_state branch from ff24ce7 to c8b340c Compare December 8, 2018 13:14
@adeelkhan
Copy link
Contributor Author

jenkins run all

@adeelkhan adeelkhan force-pushed the adeel/learner_6611_exposing_selection_state branch from c8b340c to 6f42e31 Compare December 9, 2018 12:50
@adeelkhan
Copy link
Contributor Author

adeelkhan commented Dec 10, 2018

@fysheets Here is the sandbox you asked. https://learner-6611-exposing-selection-state.sandbox.edx.org
It has the changes appearing correctly, please do review.

@fysheets
Copy link
Contributor

@MichaelRoytman Looking at the link you sent, I'm not sure that your changes were released? Wouldn't we need to do a release of your changes in that repo then update the version in the platform package.json?

@MichaelRoytman
Copy link
Contributor

@fysheets I was trying to npm install my changes from the branch in edx-custom-a11y-rules. Do you suspect that that's not working?

@fysheets
Copy link
Contributor

Ah - Perhaps? If you try to install it that way locally, do you see the expected version installed? I think we had some issues with branch installations recently, didn't we?

@adeelkhan adeelkhan force-pushed the adeel/learner_6611_exposing_selection_state branch from 6f42e31 to 79370bf Compare December 12, 2018 20:40
adeelkhan and others added 3 commits December 19, 2018 08:24
Dashboard header was missing aria-current and
aria-selected attributes causing no information
exposed to screen reader about selection.This
would enable such info to be passed to screen
reader

LEARNER-6611
@fysheets fysheets force-pushed the adeel/learner_6611_exposing_selection_state branch from fe33ce0 to 3d2aea8 Compare December 19, 2018 13:25
@adeelkhan
Copy link
Contributor Author

jenkins run lettuce
jenkins run quality

@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

@wittjeff wittjeff merged commit 7c74ecb into master Dec 19, 2018
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, December 20, 2018.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@fysheets fysheets deleted the adeel/learner_6611_exposing_selection_state branch April 17, 2020 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants