Skip to content

Add error recovery and focus trap to thread loading in forum sidebar #1707

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

Merged
gwprice merged 2 commits into from
Nov 20, 2013
Merged

Add error recovery and focus trap to thread loading in forum sidebar #1707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e31ff8
Recover from error loading forum thread list
Nov 14, 2013
9593261
Add focus trap on forum navigation thread loading
Nov 14, 2013
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
5 changes: 5 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ These are notable changes in edx-platform. This is a rolling list of changes,
in roughly chronological order, most recent first. Add your entries at or near
the top. Include a label indicating the component affected.

LMS: Trap focus on the loading element when a user loads more threads
in the forum sidebar to improve accessibility.

LMS: Add error recovery when a user loads more threads in the forum sidebar.

LMS: Add a user-visible alert modal when a forums AJAX request fails.

Blades: Add template for checkboxes response to studio. BLD-193.
Expand Down
6 changes: 3 additions & 3 deletions common/static/coffee/src/discussion/discussion.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ if Backbone?
@add model
model

retrieveAnotherPage: (mode, options={}, sort_options={})->
@current_page += 1
data = { page: @current_page }
retrieveAnotherPage: (mode, options={}, sort_options={}, error=null)->
data = { page: @current_page + 1 }
switch mode
when 'search'
url = DiscussionUtil.urlFor 'search'
Expand Down Expand Up @@ -59,6 +58,7 @@ if Backbone?
@reset new_collection
@pages = response.num_pages
@current_page = response.page
error: error

sortByDate: (thread) ->
#
Expand Down
14 changes: 8 additions & 6 deletions common/static/coffee/src/discussion/utils.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@ class @DiscussionUtil
"notifications_status" : "/notification_prefs/status"
}[name]

@makeFocusTrap: (elem) ->
elem.keydown(
(event) ->
if event.which == 9 # Tab
event.preventDefault()
)

@discussionAlert: (header, body) ->
if $("#discussion-alert").length == 0
alertDiv = $("<div class='modal' role='alertdialog' id='discussion-alert' aria-describedby='discussion-alert-message'/>").css("display", "none")
Expand All @@ -99,12 +106,7 @@ class @DiscussionUtil
" <button class='dismiss'>OK</button>" +
"</div>"
)
# Capture focus
alertDiv.find("button").keydown(
(event) ->
if event.which == 9 # Tab
event.preventDefault()
)
@makeFocusTrap(alertDiv.find("button"))

@marcotuts marcotuts Nov 19, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One note about this approach is that when testing on ChromeVox, a user who is routed to this FocusTrap in the case of a 500 is taken to the button which reads OK, but must then manually navigate out to the alert message and header to understand the cause for the error. Could we .find the entire modal or header item instead?

@gwprice gwprice Nov 19, 2013

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's odd; the non-specific alert dialog reads the whole error message, but this one does not. I'll try to figure out why.

@gwprice gwprice Nov 19, 2013

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Also, you cannot focus on a non-interactive element, so the answer to the question you posed is no.

@gwprice gwprice Nov 19, 2013

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

JAWS doesn't read either alert. According to http://www.w3.org/TR/wai-aria/roles#alertdialog, "authors SHOULD set focus to an active element within the alert dialog, such as a form edit field or an OK button." Also, "Authors SHOULD use aria-describedby on an alertdialog to point to the alert message element in the dialog," which we are doing. I don't know why the alert is not being read. This represents a step in the right direction, so I think we should go ahead with this change and consult with an expert for advice on improving the experience.

alertTrigger = $("<a href='#discussion-alert' id='discussion-alert-trigger'/>").css("display", "none")
alertTrigger.leanModal({closeButton: "#discussion-alert .dismiss", overlay: 1, top: 200})
$("body").append(alertDiv).append(alertTrigger)
Expand Down
11 changes: 9 additions & 2 deletions common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,11 @@ if Backbone?
loadMorePages: (event) ->
if event
event.preventDefault()
@$(".more-pages").html('<div class="loading-animation"><span class="sr">Loading more threads</span></div>')
@$(".more-pages").html('<div class="loading-animation" tabindex=0><span class="sr" role="alert">Loading more threads</span></div>')
@$(".more-pages").addClass("loading")
loadingDiv = @$(".more-pages .loading-animation")
DiscussionUtil.makeFocusTrap(loadingDiv)
loadingDiv.focus()
options = {}
switch @mode
when 'search'
Expand Down Expand Up @@ -156,7 +159,11 @@ if Backbone?
$(".post-list a").first()?.focus()
)

@collection.retrieveAnotherPage(@mode, options, {sort_key: @sortBy})
error = =>
@renderThreads()
DiscussionUtil.discussionAlert("Sorry", "We had some trouble loading more threads. Please try again.")

@collection.retrieveAnotherPage(@mode, options, {sort_key: @sortBy}, error)

renderThread: (thread) =>
content = $(_.template($("#thread-list-item-template").html())(thread.toJSON()))
Expand Down