Help Modal - Accessibility Fixes by antoviaque · Pull Request #902 · openedx/openedx-platform

antoviaque · 2013-09-09T21:24:52Z

Fixes for LMS-602, LMS-584, LMS-582, LMS-581. This is a set of accessibility fixes, mainly for the help modal.

Cf discussion below for details.

Also adding the email description to the login page and applying the same aria attribute to the correspondin input on that page. From LMS-602: > There is additional explanatory text, marked up in a span element after > the email input. The input elements should be explicitly associated with > this additional text using the aria-describedby attribute. This will > require an ID to be assigned to the span element.

> To make this change explicit to assistive technology, focus should be > moved programmatically to this div after it has been made visible. To > allow the div to be focused via scripting (without adding it to the > regular focus order that keyboard users cycle through), a tabindex=”-1” > should be added. > In addition, the invalid fields should be given a an > aria-invalid="true" attribute.

Added a 'aria-hidden="true"' attribute to the modal initially, then toggle the value of that attribute to "false" when the modal is shown. Also remove the attribute when the modal is closed, whether it's through a click on the close button or on the overlay.

The close button for the modal dialog was marked up as a paragraph, containing a graphical text character, inside two div elements, with an associated click event. This made the button unusable for keyboard users. Replaced <divs> with a link element (<a role="button"), adapted the CSS accordingly, and updated the existing <a> modal close buttons.

Modal dialogs should manage keyboard focus, ensuring that users can't tab out of the modal dialog and back into the page itself (effectively moving “behind” the dimmed modal). On the last focusable element of the dialog, return focus back to the first focusable element. Also move the close button to the start of the dialog's section, making it the first focusable URL, to ensure focus matches the visual representation of the dialog.

Users should be able to close the modal with the "escape" key.

antoviaque · 2013-09-09T21:26:19Z

On Mon, Sep 9, 2013, at 8:26, Brian Talbot wrote:

LMS-602: Would you mind applying this same recommendation to all fields
in this form that have an accompanying element? Also,
when adding id attributes to each element, can you follow the
convention set within this issues description ("id of input/formfield" +
"-tip" - e.g. id="name-tip" for the full name field). Also, it looks like
we've removed the tip for the email address field noted in the issue's
description. Would you mind adding that back to the login view's template
(lms/templates/login)?

This was about the forgot password form I think (lms/templates/forgot_password_modal.html), where the tip for the email address is still present. It makes sense to have this tip on the login too though, so I've added it to the login view, and applied the same recommendation to the new - is that good?

The only other <span class="tip"> I've seen is the "Forgot password?" link on the login page -- I didn't add the aria-describedby to that one, since that doesn't exactly describe the password field - is this ok?

LMS-585: I believe this issue has already been corrected. The template
file that contains this HTML looks to already have a fix in place (made
by these two commits -
edx@214a814
and
edx@7452381).
I don't think there's anything for you to follow-up with on that - I'd
mark it as already resolved, please.

Sounds good, I've left that one alone.

LMS-582: I'd prefer that the close modal control be marked up with a
link element ().

Done. I also assume that you want to use the "title" attribute instead of the hidden-accessible class trick? I saw the <a title="Close"> being used already, and it seemed cleaner, so I went for the first option, but let me know.

I also wasn't sure if you wanted me to replace the class name with action action-modal-close. Since the class modal-close was already referenced from several places, I acted conservatively and kept the existing class name, but let me know.

LMS-584: I'd also like to toggle another HTML attribute along with the
"tabindex" one recommended. Could you please 1) add a
'aria-hidden="true"' attribute to the modal to begin with and 2) then
toggle the value of that attribute to "false" when the modal is shown?

Done. I'm also removing the attribute when the modal is closed, whether it's through a click on the close button or on the overlay.

And also done LMS-581. I only applied it to the help dialog, as I think this bug was specifically about this one.

Thanks again for the help , its very much appreciated. Please let me know
if you have any questions or concerns.

Btw I didn't find tests for the help modal, so I didn't add/update tests with the changes I've made -- do you know if there are tests for this feature somewhere?

Also, do you know why is the code for the help modal inline in the HTML, rather than part of the static assets?

jkarni · 2013-09-10T20:10:15Z

awesome stuff!

adampalay · 2013-09-12T19:56:16Z

This is great, thanks
👍

talbs · 2013-09-13T11:47:01Z

@antoviaque, thanks for all of this great work. I took a spin through and things seem to be in good order.

To answer a few of your questions:

Btw I didn't find tests for the help modal, so I didn't add/update tests with the changes I've made -- do you know if there are tests for this feature somewhere?

I don't think we have any tests written for the help modal, but that is less of my expertise honestly. @jzoldak, are there any tests written/testing dependencies on the help modal at all?

Also, do you know why is the code for the help modal inline in the HTML, rather than part of the static assets?

Good question, honestly it may have come down to speed and time constraints when initially implementing it. Abstracting the JS into its own method may be good things to do in the near future. Thanks for the heads up.

This looks good from my perspective. 👍 Thanks again!
@jtauber, are there any other steps needed before we merge/close on this good work?

antoviaque · 2013-09-13T12:06:54Z

@talbs @jkarni @adampalay Thanks for the kind words, I'm glad this can be helpful. And thanks to you in the first place, for the great work on edX. It's a pleasure to work with the project's code!

wedaly · 2013-09-13T12:21:38Z

@talbs We have acceptance-level tests for the help modal. I ran the LMS acceptance tests on this branch, and everything looks good.

talbs · 2013-09-13T13:53:15Z

@wedaly, thx for checking and that's great news. This is ready to bring to "mergetown" from my perspective.

Help Modal - Accessibility Fixes

…urn-only-username Return only username on OpenID provider requests.

* Merge pull request openedx#937 from nttks/gacco/cypress.biz-2nd (openedx#943) * Merge pull request openedx#937 from nttks/gacco/cypress.biz-2nd gacco/cypress.biz 2nd (cherry picked from commit 4bc3bc2) Conflicts: biz/djangoapps/ga_achievement/management/commands/tests/test_update_biz_score_status.py lms/djangoapps/ccx/tests/test_field_override_performance.py lms/djangoapps/instructor/tests/test_api.py lms/envs/test.py setup.py * Merge pull request openedx#941 from kawaguchi-ks/develop/cypress/biz-fix-timezone-problem Fix timezone problem on boundary test. (cherry picked from commit 20e1b0c) * Fix transaction, autocommit(commit_on_success) -> atomic. * Define class Meta. * Change AUTHENTICATION_BACKENDS for disable RateLimitBackend within lo… (openedx#902) * Change AUTHENTICATION_BACKENDS for disable RateLimitBackend within login authentication(openedx#540) (openedx#883) (cherry picked from commit 1e8b68a) Conflicts: cms/djangoapps/course_creators/tests/test_admin.py * Fix test, "RedirectCycleError: Redirect loop detected".

Master to Staging

Commits: Add marketing opt-in checkbox to registration form

We missed a copy edit from the previous PR made for this ticket. Also changing the id to something more relevant to the message.

…hn_mfe fix: don't enable Authn MFE by default

antoviaque added 6 commits September 9, 2013 14:07

accessibility: Allow to close help modal with "escape" key (LMS-581)

8d2297e

Users should be able to close the modal with the "escape" key.

jtauber added a commit that referenced this pull request Sep 13, 2013

Merge pull request #902 from antoviaque/accessibility

c6dfd97

Help Modal - Accessibility Fixes

jtauber merged commit c6dfd97 into openedx:master Sep 13, 2013

adampalay mentioned this pull request Sep 19, 2013

manage focus to/from/within password reset modal appropriately #1065

Merged

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014

Merge pull request openedx#902 from MITx/hotfix/rocha/openid-sreg-ret…

e920ffb

…urn-only-username Return only username on OpenID provider requests.

ericfab179 pushed a commit to eduNEXT/edx-platform that referenced this pull request Sep 14, 2018

Merge pull request openedx#902 from proversity-org/master

164af07

Master to Staging

caesar2164 pushed a commit to caesar2164/edx-platform that referenced this pull request Sep 17, 2019

Merge PR openedx#902 stv/feature/registration-optin-marketing

f9412ce

Commits: Add marketing opt-in checkbox to registration form

Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022

chore: [MICROBA-1780] missing copy edit (openedx#902)

8b32e58

We missed a copy edit from the previous PR made for this ticket. Also changing the id to something more relevant to the message.

DanielVZ96 pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 31, 2024

Merge pull request openedx#902 from openedx/zamir/disable_default_aut…

8ea53b8

…hn_mfe fix: don't enable Authn MFE by default

Navigation Menu

Uh oh!

Help Modal - Accessibility Fixes - #902

Help Modal - Accessibility Fixes#902
jtauber merged 6 commits into
openedx:masterfrom
antoviaque:accessibility

antoviaque commented Sep 9, 2013

Uh oh!

antoviaque commented Sep 9, 2013

Uh oh!

jkarni commented Sep 10, 2013

Uh oh!

adampalay commented Sep 12, 2013

Uh oh!

talbs commented Sep 13, 2013

Uh oh!

antoviaque commented Sep 13, 2013

Uh oh!

wedaly commented Sep 13, 2013

Uh oh!

talbs commented Sep 13, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

antoviaque commented Sep 9, 2013

Uh oh!

antoviaque commented Sep 9, 2013

Uh oh!

jkarni commented Sep 10, 2013

Uh oh!

adampalay commented Sep 12, 2013

Uh oh!

talbs commented Sep 13, 2013

Uh oh!

antoviaque commented Sep 13, 2013

Uh oh!

wedaly commented Sep 13, 2013

Uh oh!

talbs commented Sep 13, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants