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

Fixed accessibility issues in Schedule Tab in CCX #9114

Merged
merged 1 commit into from
Aug 14, 2015

Conversation

amir-qayyum-khan
Copy link
Contributor

Background

(copied verbatim from Mark Sadecki's CCX Accessibility audit - July 17, 2015)

Unfortunately, there are some significant issues with the way the Schedule Table is implemented. In summary, there is nothing that programmatically indicates the relationship between the nested table rows. They only appear related visually by their padding-left. Also, the controls to expand/collapse have no accessible text and are not associated with the Section/Subsection/Unit name. All of these objects are conveyed as equal table rows.

Schedule Tab tree

  • Keep the table structure
  • Add screen reader text[4] that follows each “title” in the Unit column that identifies it as a “Section”, “Subsection”, or “Unit”
  • Add screen reader text for each toggle control that indicates what clicking it will do (either “Expand” or - - “Collapse” this will toggle according to its current state)
  • Add controls before the table to “Expand All”/“Collapse All”

Date picker

The accessibility of the date and time pickers associated with type=“date” and type=“time” input fields is the responsibility of the user agent. Some UAs will allow the user to manually type a date. For this to work though, it has to be in the right format. Suggest adding help text to these form fields which indicates what the acceptable format is e.g. yyyy-mm-dd or 2015–07–17. The current placeholder indicates that yyyy/mm/dd is acceptable. However Firefox and Safari accept the yyyy-mm-dd format. This makes the current placeholder misleading.

Save Changes

After adding a new unit to the schedule, the Save Changes block is added to the DOM. A screen reader user would not know this has happened. Since an action is required (“Save Changes”) it is appropriate to move focus to this new content. Suggest adding a tabindex=“–1” to this div and moving focus() to it. Saving Changes moves the focus back tot he Schedule a Unit div, and I think that is an acceptable workflow.

Other issues

  • The “Set date” and “Set time” links should really be buttons.
  • When you click them, focus[2] must be moved to the modal dialog that opens.
  • The date and time inputs in the dialog box suffer from the same label issues as in “Schedule a Unit” above
  • Focus must remain trapped in the dialog box until it is closed.
  • “Remove” links are all identical and provide no context. It would be helpful to add screen reader text that includes the name of the Unit that would be removed.
  • All of the toggle controls need accessible text.
  • All icon fonts ) need aria-hidden=“true” added to them.

Studio Updates: None.

LMS Updates: CCX schedule tab is now fix to meet edX Accessibility Guidelines.

What is done in this PR

  • Added Expand all button
  • Added Collapse all button
  • Added expand/collapse status to tree
  • Added labels to remove link, expand/collapse links
  • Made set date links, buttons in tree
  • Added focus to set date dialog
  • Added placeholder alternatives for screen readers in set date dialog
  • Trapped focus to set date until it is open
  • Added tab index to alerts like error message, save changes, all unit saved etc
  • Added labels and roles to alerts line error message, save changes, all unit saved etc
  • On any change made to schedule put focus to save changes alert.
  • Fixed date picker issues where it was showing 2 date picker on chrome and was accepting format mm/dd/yy where in other browsers it was taking format yyyy-mm-dd
  • Fixed time picker issue on chrome.
  • Added screen reader text that follows each “title” in the Unit column that identifies it as a “Section”, “Subsection”, or “Unit”

Testing

You can use screen reading tools like chromeVox (A chrome browser plugin) or command+F5 on mac to verify this PR.
Also read http://edx-partner-course-staff.readthedocs.org/en/latest/getting_started/accessibility.html

Issue mitocw#22
issue jazkarta#120
issue jazkarta#63
issue jazkarta#30
issue jazkarta#121
@pdpinch @pwilkins @clrux

screen shot 2015-08-10 at 9 38 37 pm 2

@openedx-webhooks
Copy link

Thanks for the pull request, @amir-qayyum-khan! I've created OSPR-726 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ('this must be merged by XX date', and why that is)
  • partner information ('this is a course on edx.org')
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Jul 29, 2015
@amir-qayyum-khan amir-qayyum-khan changed the title Fixed accessibility issues in Schedule Tab Fixed accessibility issues in Schedule Tab in CCX Jul 29, 2015
@sarina
Copy link
Contributor

sarina commented Jul 29, 2015

@amir-qayyum-khan can you please post a test course and instructions of how reviewers can see this screen? Alternatively, a sandbox and links to the specific page(s) would work as well.

@bdero
Copy link
Contributor

bdero commented Aug 3, 2015

Could you please rebase this when you get the chance @amir-qayyum-khan ? There's an unrelated problem that prevents proper deployment which has been fixed in the latest edx/edx-platform/master.

@tssheth
Copy link

tssheth commented Aug 3, 2015

These changes seem to work great on sandbox2o.
I noticed a few small issues with adding a date to new content in the schedule tab. When I selected a "start date" and "end date" with content and clicked "add unit" the content did not appear with the dates. Instead the content appeared with the "set date" button (A). Also if the date has been previously set and then removed (as mentioned below), selecting a new date will not override the old date (B). I was able to use the "set date" button to add dates to previously added content.

(A)
screen shot 2015-08-03 at 3 36 41 pm

screen shot 2015-08-03 at 3 36 59 pm

(B)
screen shot 2015-08-04 at 11 04 24 am

screen shot 2015-08-04 at 11 04 51 am

When each date is set for previously added content, the schedule tab tree collapses. I'm not sure if this is desired but it may be easier for the user for the units to stay expanded.

Also, start and end dates for content are remembered after content has been removed from the "schedule panel". These dates will reappear when that content is added again. I'm not sure if this is desired either.

@amir-qayyum-khan
Copy link
Contributor Author

@bdero done with rebase

@@ -1,3 +1,8 @@
<div align="right">
<button id="ccx_expand_all_btn" aria-expanded="true">Expand All</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

@amir-qayyum-khan aria-expanded is inappropriate here. aria-expanded is used to indicate the current state of a region or a toggle control that affects the state of that region. Since these are two buttons (not a single toggle) that affect multiple regions, we don't want aria-expanded here at all.

@amir-qayyum-khan amir-qayyum-khan force-pushed the fix/aq/ccx_schedule branch 2 times, most recently from 082d9d4 to c314805 Compare August 11, 2015 12:23
@openedx-webhooks openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Aug 11, 2015
@@ -203,7 +214,7 @@ var edx = edx || {};
self.schedule_collection.set(self.schedule);
var button = $('#dirty-schedule #save-changes');
button.prop('disabled', true).text(gettext("Saving")+'...');
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is outside of your changes but could you remove the '...' from this string? It is not compatible with internationalization (or I believe accessibility) and we've worked to remove these from strings. Obviously this one is concatenated so I missed it.

@amir-qayyum-khan
Copy link
Contributor Author

Done @sarina

@sarina
Copy link
Contributor

sarina commented Aug 11, 2015

👍 from me (be sure you squash commits before you're done here)

@cptvitamin can you please merge when you're satisfied?

@amir-qayyum-khan amir-qayyum-khan force-pushed the fix/aq/ccx_schedule branch 2 times, most recently from 10c7332 to 480a2e1 Compare August 11, 2015 15:27
@pdpinch
Copy link
Contributor

pdpinch commented Aug 13, 2015

Commits have been squashed.

@cptvitamin @clrux is there anything more we need to do here?

@downzer0
Copy link
Contributor

@amir-qayyum-khan @pdpinch Looking very nice! 👍

@sarina
Copy link
Contributor

sarina commented Aug 13, 2015

@clrux when tests pass can you merge? Or does Mark need to give thumbs up too?

@downzer0
Copy link
Contributor

@sarina I can, but I don't think it'll hurt to have @cptvitamin take another pass too.

@pdpinch
Copy link
Contributor

pdpinch commented Aug 14, 2015

FYI this PR is currently on sandbox2o.mitx.mit.edu. Ping me or @pwilkins if you need an account.

@cptvitamin
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering review open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants