Fix accessibility issues in SGA by amir-qayyum-khan · Pull Request #87 · mitodl/edx-sga

amir-qayyum-khan · 2015-04-17T13:06:21Z

In this PR I fixed accessibility related issues. Added necessary ARIA tags into code.

@carsongee and @pdpinch please review it

Thanks

pdpinch · 2015-04-28T19:05:04Z

@amir-qayyum-arbisoft it looks like this needs a rebase?

FYI, @bdero is going to put this on sandbox and we'll see if we can get it reviewed by an MIT accessibility specialist.

bdero · 2015-04-30T19:18:31Z

This is on sandxox2o

bdero · 2015-04-30T19:50:24Z

edx_sga/static/js/src/edx_sga.js

A loop would probably be overkill for this, but please store the element before making these calls:

var uploadElement = $(content).find(".upload"); uploadElement.attr(/* params */) // ...etc...

amir-qayyum-khan · 2015-05-04T12:19:49Z

@pdpinch and @bdero done with fix

pdpinch · 2015-05-27T13:16:10Z

@amir-qayyum-khan are there any changes we should consider for this, based on what you heard from edX?

tssheth · 2015-07-23T18:11:50Z

How might I test this?

amir-qayyum-khan · 2015-07-24T07:47:35Z

@jetej 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

tssheth · 2015-07-24T18:56:17Z

Seems to work ok. I was able to use VoiceOver with both the student and instructor UI.

pdpinch · 2015-07-29T12:35:43Z

We'll still need another round of accessibility review after this, but this seems worth merging.

pdpinch · 2015-07-29T12:37:18Z

edx_sga/templates/staff_graded_assignment/show.html

typo: should these be "upload_label"

pdpinch · 2015-07-29T12:38:50Z

Let's merge after the typo fix. This will all need another round of accessibility review afterwards, but this is an improvement.

amir-qayyum-khan · 2015-07-30T11:50:19Z

@pdpinch issue fixed

pdpinch · 2015-08-14T14:40:29Z

edx_sga/templates/staff_graded_assignment/show.html

should this be tabindex="-1" ?

tssheth · 2015-08-14T14:48:50Z

edx_sga/templates/staff_graded_assignment/show.html

tabindex="-1"

amir-qayyum-khan · 2015-08-24T11:53:32Z

Done @pdpinch

There are one more things that needs to be add in dialogs:

Add close icons in each dialog
Trap focus in each dialog
File uploading accessibility is fixed in Made file uploading for student some like staff annotated file upload and added focus to error message #106

pdpinch · 2017-08-04T14:58:59Z

@amir-qayyum-khan not urgent, but could you rebase this sometime? I'm not sure if it's still relevant, but I'd like to clean this up.

pdpinch · 2018-04-09T12:37:26Z

@amir-qayyum-khan do you know if we have an issue for this?

amir-qayyum-khan · 2018-04-09T13:50:36Z

I dont think so @pdpinch

pdpinch · 2018-11-26T15:39:26Z

@amir-qayyum-khan can you address the merge conflicts?

Copilot

Pull request overview

This PR aims to improve accessibility in the Staff Graded Assignment (SGA) module by adding ARIA attributes and making semantic HTML improvements. While the intent is good, several ARIA attributes are misapplied or used incorrectly, which could confuse assistive technology users rather than help them.

Key changes include:

Addition of ARIA attributes (role, aria-live, aria-describedby, aria-label) throughout templates
Improved table semantic structure with proper thead/tbody elements
Enhanced focus management for error messages in JavaScript
Removal of aria-hidden attributes from modal sections

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.

File	Description
edx_sga/templates/staff_graded_assignment/show.html	Added ARIA attributes to various elements, improved table structure with thead/tbody, removed aria-hidden from modals, and wrapped text in span elements
edx_sga/templates/staff_graded_assignment/edit.html	Changed list structure from ul/li to div elements, added aria-describedby to inputs, added role="form" to container, and included aria-label for settings tab
edx_sga/static/js/src/edx_sga.js	Added progressbar ARIA attributes to upload element, implemented focus management for error messages, and added aria-valuenow/aria-valuetext for progress updates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-01T12:00:18Z

edx_sga/static/js/src/edx_sga.js

                    form.find('.error').html('<br/>Grade must be positive.');
+                    form.find('.error').focus();
                } else if (score > max_score) {
                    form.find('.error').html('<br/>Maximum score is ' + max_score);


Inconsistent focus management: the error message is missing a .focus() call here, while all other validation error cases (lines 209, 212, 215) properly set focus to the error element for accessibility. Add form.find('.error').focus(); after line 217 to maintain consistency and ensure screen reader users are notified of this validation error.

Suggested change

form.find('.error').html('<br/>Maximum score is ' + max_score);

form.find('.error').html('<br/>Maximum score is ' + max_score);

form.find('.error').focus();

Copilot · 2025-12-01T12:00:18Z

edx_sga/templates/staff_graded_assignment/show.html


 <div class="sga-block" data-state="{{ student_state }}" data-max-size="{{ max_file_size }}"
-     data-staff="{{ is_course_staff }}">
+     data-staff="{{ is_course_staff }}" role="article" aria-describedby="title">


The aria-describedby="title" reference on line 4 points to an element with id="title" on line 7, but this element is inside a script template and may not exist in the DOM when the outer div is rendered. The referenced element is rendered dynamically via JavaScript template, so it won't be available when the page initially loads. This creates a broken ARIA reference. Consider moving the role and aria-describedby to the dynamically rendered content inside the template, or ensure the title element exists before the aria-describedby reference.

Copilot · 2025-12-01T12:00:19Z

edx_sga/templates/staff_graded_assignment/show.html

-          </tr>
+          <thead>
+            <tr>
+              <th>{% trans "Module Fields" %}</th>


The table has only one column header but two columns of data (name and field value). The <thead> should have a <th> for each column to properly describe the table structure for screen readers. Add a second <th> for the field value column, or consider restructuring as a definition list if this is meant to be a list of name-value pairs.

Suggested change

<th>{% trans "Module Fields" %}</th>

<th>{% trans "Field Name" %}</th>

<th>{% trans "Field Value" %}</th>

Copilot · 2025-12-01T12:00:19Z

edx_sga/templates/staff_graded_assignment/show.html

          </td>
          <td>
-            <div class="upload">
+            <div class="upload" aria-live="assertive" role="alert" tabindex="-1">


Similar issue as line 36: role="alert", aria-live="assertive", and tabindex="-1" are incorrectly applied to this static upload container. These ARIA attributes should only be used for dynamic announcements and status messages, not for the container element itself. Remove these attributes from this div.

Suggested change

<div class="upload" aria-live="assertive" role="alert" tabindex="-1">

<div class="upload">

Copilot · 2025-12-01T12:00:19Z

edx_sga/templates/staff_graded_assignment/show.html

           style="height: 80%" id="{{ id }}-debug">
    <div class="inner-wrapper" style="color: black">
-      <header><h2>{% trans "Staff Debug" %}</h2></header>
+      <header role="banner"><h2>{% trans "Staff Debug" %}</h2></header>


The role="banner" attribute is incorrectly used here. The banner role should be used for the main header of the page/site, not for modal headers. Each page should typically have only one banner landmark. This header is within a modal dialog and should not use the banner role. Consider removing this role attribute.

Suggested change

<header role="banner"><h2>{% trans "Staff Debug" %}</h2></header>

<header><h2>{% trans "Staff Debug" %}</h2></header>

Copilot · 2025-12-01T12:00:20Z

edx_sga/templates/staff_graded_assignment/show.html

  </section>

-  <section aria-hidden="true" class="modal grade-modal" id="{{ id }}-enter-grade">
+  <section  class="modal grade-modal" id="{{ id }}-enter-grade">


[nitpick] Extra whitespace before the class attribute. Should be <section class="modal grade-modal" instead of <section class="modal grade-modal" (note the double space).

Suggested change

<section class="modal grade-modal" id="{{ id }}-enter-grade">

<section class="modal grade-modal" id="{{ id }}-enter-grade">

Copilot · 2025-12-01T12:00:20Z

edx_sga/templates/staff_graded_assignment/show.html

+      <label for="file_uploaded"><b>File uploaded<span class="sr">&nbsp;download link</span></b></label>
+        <a href="<%= downloadUrl %>" id="file_uploaded"><%= uploaded.filename %></a>


The <label> element is being used incorrectly here. Labels should be associated with form controls (input, select, textarea), not with links. The uploaded file link is not a form input. Consider using a <span> or removing the label wrapper entirely, and rely on the semantic meaning of the link text itself. The screen reader text addition is good, but the label wrapper is semantically incorrect.

Suggested change

<label for="file_uploaded"><b>File uploaded<span class="sr"> download link</span></b></label>

<a href="<%= downloadUrl %>" id="file_uploaded"><%= uploaded.filename %></a>

<span><b>File uploaded<span class="sr"> download link</span></b></span>

<a href="<%= downloadUrl %>" id="file_uploaded" aria-label="Download uploaded file: <%= uploaded.filename %>"><%= uploaded.filename %></a>

Copilot · 2025-12-01T12:00:21Z

edx_sga/templates/staff_graded_assignment/show.html

          <td>
            <% if (assignment.may_grade) { %>
-              <a class="enter-grade-button button" href="#{{ id }}-enter-grade">
+              <a class="enter-grade-button button" href="#{{ id }}-enter-grade" role="button">


Links should not have role="button". This creates confusion for assistive technology users as the element is semantically a link (using an anchor tag with href). If this needs to behave like a button, consider using an actual <button> element instead. Otherwise, keep it as a link without the role attribute.

Suggested change

<a class="enter-grade-button button" href="#{{ id }}-enter-grade" role="button">

<a class="enter-grade-button button" href="#{{ id }}-enter-grade">

Copilot · 2025-12-01T12:00:21Z

edx_sga/templates/staff_graded_assignment/show.html

+        <span id="upload_label">{% trans "Upload your assignment" %}</span>
      <% } %>
-      <div class="upload">
+      <div class="upload" aria-describedby="upload_label" role="alert" aria-live="assertive" tabindex="-1">


The role="alert", aria-live="assertive", and tabindex="-1" attributes are misapplied to this upload container. The role="alert" is meant for important, time-sensitive messages that should interrupt the user, not for static UI containers. The upload div doesn't receive programmatic focus in the JavaScript code (only error elements do), making tabindex="-1" unnecessary. Remove these ARIA attributes from this div - they should only be applied to actual dynamic status/error message elements.

Suggested change

<div class="upload" aria-describedby="upload_label" role="alert" aria-live="assertive" tabindex="-1">

<div class="upload" aria-describedby="upload_label">

Copilot · 2025-12-01T12:00:21Z

edx_sga/templates/staff_graded_assignment/edit.html

+  <div class="list-input settings-list">
    {% for field, value, validator in fields %}
-    <li class="field comp-setting-entry metadata_entry">
+    <div class="field comp-setting-entry metadata_entry">
      <div class="wrapper-comp-setting">
-        <label class="label setting-label" 
+        <label class="label setting-label"
          for="{{ field.name }}_input">{{ field.display_name }}</label>
-        <input class="input setting-input" type="text" 
-               id="{{ field.name }}_input" name="{{ field.name}}" 
+        <input class="input setting-input" type="text" aria-describedby="{{ field.name }}_hint"
+               id="{{ field.name }}_input" name="{{ field.name}}"
               value="{{ value }}" data-validator="{{ validator }}"/>
      </div>
-      <span class="tip setting-help">{{ field.help }}</span>
-    </li>
+      <span id="{{ field.name }}_hint" class="tip setting-help">{{ field.help }}</span>
+    </div>


Changing the semantic structure from <ul> and <li> to <div> elements reduces the semantic meaning and accessibility of the list. The original structure communicated that these were a list of settings to screen readers. Consider keeping the <ul> and <li> structure for better semantics, or if a div structure is required, add role="list" and role="listitem" to maintain the semantic information.

pdpinch assigned bdero Apr 28, 2015

amir-qayyum-khan force-pushed the accessibility_fix branch from a77b6a2 to cfa465b Compare April 29, 2015 10:04

bdero reviewed Apr 30, 2015
View reviewed changes

amir-qayyum-khan force-pushed the accessibility_fix branch from 618f49b to c818401 Compare May 4, 2015 11:37

amir-qayyum-khan force-pushed the accessibility_fix branch 3 times, most recently from c2119c4 to d659fee Compare June 9, 2015 12:55

pdpinch reviewed Jul 29, 2015
View reviewed changes

edx_sga/templates/staff_graded_assignment/show.html Outdated

Copy link

Member

pdpinch Jul 29, 2015

Choose a reason for hiding this comment

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

typo: should these be "upload_label"

amir-qayyum-khan force-pushed the accessibility_fix branch 2 times, most recently from 31673b1 to b02665d Compare July 30, 2015 11:48

amir-qayyum-khan added the Needs Review label Jul 30, 2015

amir-qayyum-khan changed the title ~~fix accessibility issues in code~~ Fix accessibility issues in SGA Jul 30, 2015

amir-qayyum-khan force-pushed the accessibility_fix branch from 68bb1b8 to 003d1d9 Compare July 30, 2015 12:13

pdpinch reviewed Aug 14, 2015
View reviewed changes

edx_sga/templates/staff_graded_assignment/show.html Outdated

Copy link

Member

pdpinch Aug 14, 2015

Choose a reason for hiding this comment

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

should this be tabindex="-1" ?

tssheth reviewed Aug 14, 2015
View reviewed changes

edx_sga/templates/staff_graded_assignment/show.html Outdated

Copy link

tssheth Aug 14, 2015

Choose a reason for hiding this comment

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

tabindex="-1"

pdpinch assigned pdpinch and unassigned bdero Aug 14, 2015

pdpinch added waiting on author and removed Needs Review labels Aug 14, 2015

pdpinch assigned amir-qayyum-khan and unassigned pdpinch Aug 14, 2015

amir-qayyum-khan force-pushed the accessibility_fix branch 2 times, most recently from 2d731a0 to a138ad5 Compare August 24, 2015 11:28

fix accessibility issues in code

664d138

amir-qayyum-khan force-pushed the accessibility_fix branch from 433c817 to 664d138 Compare August 24, 2015 11:33

amir-qayyum-khan added on hold and removed waiting on author labels Apr 11, 2018

pdpinch assigned pdpinch and unassigned amir-qayyum-khan Dec 3, 2018

pdpinch requested a review from Copilot December 1, 2025 11:55

Copilot started reviewing on behalf of pdpinch December 1, 2025 11:55 View session

Copilot finished reviewing on behalf of pdpinch December 1, 2025 11:58

Copilot AI reviewed Dec 1, 2025

View reviewed changes

	form.find('.error').html('<br/>Maximum score is ' + max_score);
	form.find('.error').html('<br/>Maximum score is ' + max_score);
	form.find('.error').focus();

	<th>{% trans "Module Fields" %}</th>
	<th>{% trans "Field Name" %}</th>
	<th>{% trans "Field Value" %}</th>

	<div class="upload" aria-live="assertive" role="alert" tabindex="-1">
	<div class="upload">

	<header role="banner"><h2>{% trans "Staff Debug" %}</h2></header>
	<header><h2>{% trans "Staff Debug" %}</h2></header>

	<section class="modal grade-modal" id="{{ id }}-enter-grade">
	<section class="modal grade-modal" id="{{ id }}-enter-grade">

		<label for="file_uploaded"><b>File uploaded<span class="sr"> download link</span></b></label>
		<a href="<%= downloadUrl %>" id="file_uploaded"><%= uploaded.filename %></a>

	<a class="enter-grade-button button" href="#{{ id }}-enter-grade" role="button">
	<a class="enter-grade-button button" href="#{{ id }}-enter-grade">

	<div class="upload" aria-describedby="upload_label" role="alert" aria-live="assertive" tabindex="-1">
	<div class="upload" aria-describedby="upload_label">

Conversation

amir-qayyum-khan commented Apr 17, 2015

Uh oh!

pdpinch commented Apr 28, 2015

Uh oh!

bdero commented Apr 30, 2015

Uh oh!

bdero Apr 30, 2015

Choose a reason for hiding this comment

Uh oh!

amir-qayyum-khan commented May 4, 2015

Uh oh!

pdpinch commented May 27, 2015

Uh oh!

tssheth commented Jul 23, 2015

Uh oh!

amir-qayyum-khan commented Jul 24, 2015

Uh oh!

tssheth commented Jul 24, 2015

Uh oh!

pdpinch commented Jul 29, 2015

Uh oh!

pdpinch Jul 29, 2015

Choose a reason for hiding this comment

Uh oh!

pdpinch commented Jul 29, 2015

Uh oh!

amir-qayyum-khan commented Jul 30, 2015

Uh oh!

pdpinch Aug 14, 2015

Choose a reason for hiding this comment

Uh oh!

tssheth Aug 14, 2015

Choose a reason for hiding this comment

Uh oh!

amir-qayyum-khan commented Aug 24, 2015

Uh oh!

pdpinch commented Aug 4, 2017

Uh oh!

pdpinch commented Apr 9, 2018

Uh oh!

amir-qayyum-khan commented Apr 9, 2018

Uh oh!

pdpinch commented Nov 26, 2018

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!