Skip to content

Fix accessibility issues in SGA #87

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

Open
amir-qayyum-khan wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix accessibility issues in SGA #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions edx_sga/static/js/src/edx_sga.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,19 @@ function StaffGradedAssignmentXBlock(runtime, element) {
return;
}
}
var uploadElement = $(content).find(".upload");
uploadElement.attr("role", "progressbar");
uploadElement.attr("aria-valuemax", "100");
uploadElement.attr("aria-valuemin", "0");
Comment on lines +51 to +54
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The progressbar role is being set on the upload element, but this element's purpose changes during the upload lifecycle. Initially it's a container for the file input and button, then it becomes a progress indicator. Dynamically changing an element's role can confuse assistive technologies. Consider creating a separate element specifically for the progress indicator with the progressbar role, or ensure this role is only set when actually showing progress and removed afterwards.

Copilot uses AI. Check for mistakes.
data.submit();
});
},
progressall: function(e, data) {
var percent = parseInt(data.loaded / data.total * 100, 10);
$(content).find('.upload').text(
'Uploading... ' + percent + '%');
var uploadElement = $(content).find(".upload");
uploadElement.text("Uploading... " + percent + "%");
uploadElement.attr("aria-valuenow" , percent);
uploadElement.attr("aria-valuetext" , "Uploading... " + percent + "%");
},
fail: function(e, data) {
/**
Expand Down Expand Up @@ -100,8 +106,10 @@ function StaffGradedAssignmentXBlock(runtime, element) {
}
}
});

updateChangeEvent(fileUpload);
if (!_.isUndefined(state.error)) {
$(content).find('p.error').focus();
}
}

function renderStaffGrading(data) {
Expand Down Expand Up @@ -198,10 +206,13 @@ function StaffGradedAssignmentXBlock(runtime, element) {
event.preventDefault();
if (isNaN(score)) {
form.find('.error').html('<br/>Grade must be a number.');
form.find('.error').focus();
} else if (score !== parseInt(score)) {
form.find('.error').html('<br/>Grade must be an integer.');
form.find('.error').focus();
} else if (score < 0) {
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);
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
} else {
Expand Down
21 changes: 11 additions & 10 deletions edx_sga/templates/staff_graded_assignment/edit.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
{% load i18n %}

<div id="staff-graded-assignment-edit">
<ul class="list-input settings-list">
<div id="staff-graded-assignment-edit" role="form">
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The role="form" is redundant and potentially incorrect here. This div is not a form element - forms should use the <form> tag. If this is meant to contain form fields, wrap them in a proper <form> element instead of adding role="form" to a div. The role attribute should be removed.

Copilot uses AI. Check for mistakes.
<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>
Comment on lines +4 to +15
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
{% endfor %}
</ul>
</div>
</div>

<script type="application/javascript">
$("ul.action-modes").html(' <li class="view-button sga_editor_tab">{% trans "Settings" %}</li>')
$("ul.action-modes").html(' <li class="view-button sga_editor_tab" aria-label="settings">{% trans "Settings" %}</li>')
$("h2.modal-window-title").attr("tabindex","0");
</script>
70 changes: 39 additions & 31 deletions edx_sga/templates/staff_graded_assignment/show.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
{% load i18n %}

<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">
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
<script type="text/template" id="sga-tmpl">
<% if (display_name) { %>
<b><%= display_name %></b>
<b id="title"><%= display_name %></b>
<% } %>
<% if (uploaded) { %>
<p><b>File uploaded</b>
<a href="<%= downloadUrl %>"><%= uploaded.filename %></a></p>
<br/>
<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>
Comment on lines +11 to +12
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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">&nbsp;download link</span></b></label>
<a href="<%= downloadUrl %>" id="file_uploaded"><%= uploaded.filename %></a>
<span><b>File uploaded<span class="sr">&nbsp;download link</span></b></span>
<a href="<%= downloadUrl %>" id="file_uploaded" aria-label="Download uploaded file: <%= uploaded.filename %>"><%= uploaded.filename %></a>

Copilot uses AI. Check for mistakes.
<% } else { %>
<p>No file has been uploaded.</p>
<% } %>
Expand All @@ -18,28 +19,28 @@
<b>Instructor comment</b> <%= graded.comment %>
<% } %>
<% if (annotated) { %><br/>
<b>Annotated file from instructor</b>
<a href="<%= annotatedUrl %>"><%= annotated.filename %></a>
<b id="annotated_file">Annotated file from instructor</b>
<a href="<%= annotatedUrl %>" aria-describedby="annotated_file"><%= annotated.filename %></a>
<% } %>
</p>
<% } else if (uploaded) { %>
{% trans "This assignment has not yet been graded." %}
<p>{% trans "This assignment has not yet been graded." %}</p>
<% } %>
<% if (upload_allowed) { %>
<p>
<% if (uploaded) { %>
{% trans "Upload a different file" %}
<span id="upload_label">{% trans "Upload a different file" %}</span>
<% } else { %>
{% trans "Upload your assignment" %}
<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">
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
<input class="fileupload" type="file" name="assignment"/>
<button>Select a file</button>
</div>
</p>
<% } %>
<% if (error) { %>
<p class="error"><%= error %></p>
<p class="error" role="alert" aria-live="assertive" tabindex="-1"><%= error %></p>
<% } %>
</script>

Expand Down Expand Up @@ -93,7 +94,7 @@
</td>
<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">
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
<% if (assignment.needs_approval) { %>
{% trans "Approve grade" %}
<% } else { %>
Expand All @@ -103,7 +104,7 @@
<% } %>
</td>
<td>
<div class="upload">
<div class="upload" aria-live="assertive" role="alert" tabindex="-1">
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
<input class="fileupload" type="file" name="annotated"/>
<button>Upload annotated file</button>
</div>
Expand All @@ -114,14 +115,14 @@
</table>
</script>

<div aria-hidden="true" class="wrap-instructor-info">
<div class="wrap-instructor-info">
<a class="instructor-info-action" id="grade-submissions-button"
href="#{{ id }}-grade">{% trans "Grade Submissions" %}</a>
<a class="instructor-info-action" id="staff-debug-info-button"
href="#{{ id }}-debug">{% trans "Staff Debug Info" %}</a>
</div>

<section aria-hidden="true" class="modal staff-modal" id="{{ id }}-grade" style="height: 75%">
<section class="modal staff-modal" id="{{ id }}-grade" style="height: 75%">
<div class="inner-wrapper" style="color: black; overflow: auto;">
<header><h2><span class="display_name">{{ display_name }}</span> - {% trans "Staff Graded Assignment" %}</h2></header>
<br/>
Expand All @@ -131,32 +132,39 @@
</div>
</section>

<section aria-hidden="true" class="modal staff-modal"
<section class="modal staff-modal"
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>
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
<br/>
<div class="staff_info" style="display: block; white-space: normal">
is_released = {{ is_released }}<br/>
location = {{ location }}<br/>
<span>is_released = {{ is_released }}</span> <br/>
<span>location = {{ location }}</span><br/>
<br/>
<table summary="${_('Module Fields')}">
<tr><th>{% trans "Module Fields" %}</th></tr>
{% for name, field in fields %}
<tr>
<td>{{name}}</td>
<td>
<pre style="display:inline-block; margin: 0;">{{field}}</pre>
</td>
</tr>
<thead>
<tr>
<th>{% trans "Module Fields" %}</th>
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
</tr>
</thead>
<tbody>
{% for name, field in fields %}
<tr>
<td>{{name}}</td>
<td>
<pre style="display:inline-block; margin: 0;">{{field}}</pre>
</td>
</tr>
{% endfor %}
</table><br/>
category = {{category}}
</tbody>
</table>
<br/>
<span>category = {{category}}</span>
</div>
</div>
</section>

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

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

[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 uses AI. Check for mistakes.
<div class="inner-wrapper" style="color: black">
<header><h2>
{% trans "Enter Grade" %}
Expand All @@ -168,7 +176,7 @@
<input id="submission_id-input" type="hidden" name="submission_id"/>
<div>Grade for <span id="student-name"/></div>
<div>Grade: <input id="grade-input" name="grade"/>
<span class="error"></span></div>
<span class="error" role="alert" aria-live="assertive" tabindex="-1"></span></div>
<div>Comment: <textarea id="comment-input" name="comment" rows="4"></textarea></div>
<div>
<button type="submit">{% trans "Submit" %}</button>
Expand Down