open-craft / xblock-poll

An XBlock for Polling users and displaying the result
GNU Affero General Public License v3.0
18 stars 66 forks source link

survey a11y improvements #24

Closed arizzitano closed 7 years ago

arizzitano commented 7 years ago

This PR makes two small accessibility changes to the survey template based on feedback from our SSB BART accessibility audit. They are as follows:

@bradenmacdonald or @e-kolpakov, would you please review or assign a reviewer?

@cptvitamin, would you mind reviewing as well? I'll link a sandbox once it finishes building.

Thanks all!

e-kolpakov commented 7 years ago

@arizzitano Hi! Thank you for your contribution!

Unfortunately we likely won't be able to review it this week (as we have certain sprint cycle). @bradenmacdonald should have more authoritative answer, but I wouldn't expect it to differ :)

Meanwhile, please fix the CI build error - there's one failing test. Just in case you're not aware of this feature, if you click on red cross next to commit or on "details" on overview below "All Checks Have Falied" section it'll take you to Travis and display a log of the build - it contains smoe valuable info about what have failed.

arizzitano commented 7 years ago

Cool! I just patched up that test. I tested it out in the xblock sdk but you can also view the new functionality at https://arizzitano.sandbox.edx.org/.

bradenmacdonald commented 7 years ago

Thanks @arizzitano - we'll aim to review this for you next week.

bradenmacdonald commented 7 years ago

@arizzitano Update: We finished sprint planning today and it looks like we probably won't be able to review this until next week (week of March 6). Please let me know if that's a problem.

arizzitano commented 7 years ago

Thanks all. @cptvitamin I added the scope attribute, as requested. @bradenmacdonald, can we still plan for you folks reviewing the change this week?

bradenmacdonald commented 7 years ago

@arizzitano Yes, @mtyaka will review for you this week.

mtyaka commented 7 years ago

@arizzitano Thanks for this! It looks good to me, but aXe complains about an issue that I don't fully understand (see screenshot below). Perhaps we need to add the headers attribute? I don't understand how scope and headers work together.

Maybe @cptvitamin can help?

screen shot 2017-03-08 at 11 43 45

cptvitamin commented 7 years ago

I'm almost positive this is a false positive because we are mixing so many different paradigms here (forms, and tables, and aria role overrides). However, I would like to confirm and help determine what the best course of action to handle the failure should be. @arizzitano can you get the sandbox up and running again?

arizzitano commented 7 years ago

Sandbox rebuilt and is available here https://arizzitano.sandbox.edx.org/courses/course-v1:edX+TEST102+2014_T1/courseware/913546d438554a40abfd549af4f6679f/35e0986a646943efbd20961ff9b0ffa7/.

@cptvitamin, looks like the relevant requirement is H43. I was able to get aXe to pass by inlining the headers attribute to contain the ids of both the col and row <th> elements representing each <td>. Since this worked, I'm going to make this change -- please let me know if I should hold off.

cptvitamin commented 7 years ago

Excellent work. This was as tricky one. Thanks for sticking with it. After extensive screen reader testing, I believe we achieved a solution that works for all screen reader/browser/os combos and satisfies the almighty tools. 👍

arizzitano commented 7 years ago

OK great. Rebased and squashed, all tests passing, no axe violations. @mtyaka, let me know if there's anything else you need in order to merge!

mtyaka commented 7 years ago

It looks great, thanks @arizzitano!

:+1:

mtyaka commented 7 years ago

Merged and tag v1.2.6 created.

arizzitano commented 7 years ago

Thanks @mtyaka! 🙏