Fixed accessibility issues in Forms #161

Wassaf-Shahzad · 2021-09-08T12:59:36Z

Pre-Flight checklist

Testing
- Changes have been manually tested

What are the relevant tickets?

Ticket #153

What's this PR do?

This PR has the following changes

Adds relevant id to input tags.
Adds role="alert" to the FormError Component
Adds property aria-describedby to input tags

How should this be manually tested?

By using light house to test accessibility.

Screenshots (if appropriate)

Light house score for local testing

codecov-commenter · 2021-09-08T13:03:56Z

Codecov Report

Merging #161 (9da70dd) into main (4c6a42e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #161   +/-   ##
=======================================
  Coverage   90.74%   90.74%           
=======================================
  Files         217      217           
  Lines        6301     6302    +1     
  Branches      308      308           
=======================================
+ Hits         5718     5719    +1     
  Misses        496      496           
  Partials       87       87

Impacted Files	Coverage Δ
static/js/components/forms/ChangeEmailForm.js	`100.00% <ø> (ø)`
static/js/components/forms/ChangePasswordForm.js	`100.00% <ø> (ø)`
static/js/components/forms/EmailForm.js	`100.00% <ø> (ø)`
static/js/components/forms/LoginPasswordForm.js	`100.00% <ø> (ø)`
static/js/components/forms/ProfileFormFields.js	`77.41% <ø> (ø)`
static/js/components/forms/RegisterEmailForm.js	`91.66% <ø> (ø)`
static/js/components/forms/ResetPasswordForm.js	`87.50% <ø> (ø)`
static/js/components/forms/elements/FormError.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c6a42e...9da70dd. Read the comment docs.

pdpinch

I had one question, which came up in many, many places.

You should have someone more familiar with react review this as well.

pdpinch · 2021-09-09T12:45:16Z

static/js/components/forms/ChangeEmailForm.js

+              aria-describedby="confirmPasswordError"
+            />
+            <ErrorMessage
+              name="confirmPassword"


Is it necessary to add a name attribute to the error message component? Does this render as a <div>?

(According to my knowledge)
The name attribute is not passed to our custom component and is used by Formik internally for associating the fields name in Formik state.
The name attribute is also required in case for ErrorMessage component.
Reference: https://formik.org/docs/api/errormessage

static/js/components/forms/ChangePasswordForm.js

+              aria-describedby="oldPasswordError"
+            />
+            <ErrorMessage
+              name="oldPassword"


static/js/components/forms/ChangePasswordForm.js

+              aria-describedby="newPasswordError"
+            />
+            <ErrorMessage
+              name="newPassword"


static/js/components/forms/ChangePasswordForm.js

+              aria-describedby="confirmPasswordError"
+            />
+            <ErrorMessage
+              name="confirmPassword"


gsidebo · 2021-09-13T21:24:48Z

static/js/containers/pages/settings/AccountSettingsPage.js

@@ -123,7 +123,7 @@ export class AccountSettingsPage extends React.Component<Props> {
      >
        <div className="container auth-page account-settings-page">
          <div className="auth-header">
-            <h1>User Settings</h1>


I don't think this change is necessary, and I have an open PR that updates this page layout anyway (#185). I think you can revert this change

gsidebo · 2021-09-13T21:33:03Z

static/js/components/forms/ProfileFormFields.js

        className="form-control"
        autoComplete="given-name"
+        aria-describedby="legal_address.first_nameError"


Our original mistake here was naming these form fields in a way that matches the API results, instead of giving them camel case names like our other forms and mapping the API results to those fields. This field should have name="firstName" and the error should have id="firstNameError". That issue existed long before this PR though, so you definitely don't need to change all of these values here. In the meantime, this mixture of snake case and camel case is awkward. Can you change the naming scheme so that this value is legal_address.first_name.error or legal_address.first_name_error, whichever one you prefer? After that, please apply the same naming scheme to these other attributes that were added in this PR.

pdpinch

I saw that we didn't get 100 on the profile and user settings forms. Can we improve those scores?

Wassaf-Shahzad · 2021-09-14T12:57:18Z

I saw that we didn't get 100 on the profile and user settings forms. Can we improve those scores?

Regarding the User settings, I believe @gsidebo PR #185 will improve the score
and Incase of Profile form @arslanashraf7 PR #168 will improve the score.

Issues in Profile Settings

ARIA ID's are not unique

This issue is handled by Arslan's header PR,

Issue in User Settings

ARIA ID's are not unique
Heading Elements are not in sequential order

This will be handled in Gavin's PR

gsidebo · 2021-09-17T16:12:04Z

static/js/components/forms/ChangeEmailForm.js

              name="confirmPassword"
              className="form-control"
              component={PasswordInput}
+              aria-describedby="confirm_password_error"


Sorry, I don't think I was clear enough in my last review. I was suggesting that only the fields that already have the anti-pattern snake case attribute values (like name="profile.job_title") should have ids that are also snake case. I know it seems weird to have different patterns for the same attributes in different files, but eventually we're going to refactor those snake case attribute values (#186) and the id's/aria attributes can be updated as well. If you notice here, confirm_password_error introduces three different conventions for attribute values in a single component. We have camel case for id and name, lowercase + dashes for className, and now snake case for aria-describedby. Dashes in CSS class names are already an understood convention that is specific to CSS. For other attributes, we should try to stick to camel case. So for example:

~~aria-describedby="confirm_password_error"~~ --> aria-describedby="confirmPasswordError"

~~aria-describedby="email_error"~~ --> aria-describedby="emailError"

Please apply that logic to all id and aria-describedby attributes in this PR except for the ones that are associated with a field that already has snake case attribute values (like legal_address.first_name)

This was my first thought as well However please apply the same naming scheme to these other attributes that were added in this PR. this line threw me off a bit and I assumed I had to apply the style to all attributes to maintain consistency.
I should have consulted you before hand, that was an oversight on my end and as requested above I have completed the required change.

gsidebo

Looks great! 👍

Wassaf-Shahzad added the Work in progress label Sep 8, 2021

Wassaf-Shahzad force-pushed the wassaf/153-accessibility-issues-form branch from ec2a59c to 4852095 Compare September 9, 2021 10:41

Wassaf-Shahzad changed the title ~~added id's to input fields~~ Fixed accessibility issues in Forms Sep 9, 2021

Wassaf-Shahzad added Needs Review and removed Work in progress labels Sep 9, 2021

Wassaf-Shahzad requested review from arslanashraf7, gsidebo and pdpinch September 9, 2021 11:48

pdpinch reviewed Sep 9, 2021

View reviewed changes

Wassaf-Shahzad added Work in progress and removed Needs Review labels Sep 9, 2021

Wassaf-Shahzad linked an issue Sep 10, 2021 that may be closed by this pull request

Fix accessibility issues in the forms #153

Closed

4 tasks

Wassaf-Shahzad added Needs Review and removed Work in progress labels Sep 10, 2021

Wassaf-Shahzad requested a review from pdpinch September 10, 2021 12:02

gsidebo self-assigned this Sep 10, 2021

gsidebo suggested changes Sep 13, 2021

View reviewed changes

gsidebo mentioned this pull request Sep 13, 2021

Rename snake case form fields #186

Open

gsidebo added Waiting on Author and removed Needs Review labels Sep 13, 2021

Wassaf-Shahzad force-pushed the wassaf/153-accessibility-issues-form branch from eae88a7 to e743806 Compare September 14, 2021 08:43

Wassaf-Shahzad requested a review from gsidebo September 14, 2021 09:28

Wassaf-Shahzad added Needs Review and removed Waiting on Author labels Sep 14, 2021

pdpinch reviewed Sep 14, 2021

View reviewed changes

gsidebo suggested changes Sep 17, 2021

View reviewed changes

gsidebo added Waiting on Author and removed Needs Review labels Sep 17, 2021

Wassaf-Shahzad force-pushed the wassaf/153-accessibility-issues-form branch 2 times, most recently from 9da70dd to ae47141 Compare September 20, 2021 08:33

Wassaf-Shahzad requested a review from gsidebo September 20, 2021 08:42

Wassaf-Shahzad added Needs Review and removed Waiting on Author labels Sep 20, 2021

gsidebo approved these changes Sep 20, 2021

View reviewed changes

Wassaf-Shahzad force-pushed the wassaf/153-accessibility-issues-form branch from ae47141 to d31363e Compare September 20, 2021 13:15

gsidebo added Waiting on Author and removed Needs Review labels Sep 20, 2021

Wassaf-Shahzad force-pushed the wassaf/153-accessibility-issues-form branch from d31363e to 9b5e049 Compare September 21, 2021 06:53

Fixed accessibility issues in forms

14fc69b

Wassaf-Shahzad force-pushed the wassaf/153-accessibility-issues-form branch from 9b5e049 to 14fc69b Compare September 21, 2021 06:55

Wassaf-Shahzad merged commit 7c1eade into main Sep 21, 2021

Wassaf-Shahzad mentioned this pull request Sep 24, 2021

Fixed Heading font sizes #220

Merged

4 tasks

rhysyngsun deleted the wassaf/153-accessibility-issues-form branch July 20, 2022 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed accessibility issues in Forms #161

Fixed accessibility issues in Forms #161

Wassaf-Shahzad commented Sep 8, 2021 •

edited

Loading

codecov-commenter commented Sep 8, 2021 •

edited

Loading

pdpinch left a comment

pdpinch Sep 9, 2021

Wassaf-Shahzad Sep 9, 2021

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

gsidebo Sep 13, 2021

gsidebo Sep 13, 2021

pdpinch left a comment

Wassaf-Shahzad commented Sep 14, 2021 •

edited

Loading

gsidebo Sep 17, 2021

Wassaf-Shahzad Sep 20, 2021

gsidebo left a comment

Fixed accessibility issues in Forms #161

Fixed accessibility issues in Forms #161

Conversation

Wassaf-Shahzad commented Sep 8, 2021 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Screenshots (if appropriate)

codecov-commenter commented Sep 8, 2021 • edited Loading

Codecov Report

pdpinch left a comment

Choose a reason for hiding this comment

pdpinch Sep 9, 2021

Choose a reason for hiding this comment

Wassaf-Shahzad Sep 9, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

gsidebo Sep 13, 2021

Choose a reason for hiding this comment

gsidebo Sep 13, 2021

Choose a reason for hiding this comment

pdpinch left a comment

Choose a reason for hiding this comment

Wassaf-Shahzad commented Sep 14, 2021 • edited Loading

gsidebo Sep 17, 2021

Choose a reason for hiding this comment

Wassaf-Shahzad Sep 20, 2021

Choose a reason for hiding this comment

gsidebo left a comment

Choose a reason for hiding this comment

Wassaf-Shahzad commented Sep 8, 2021 •

edited

Loading

codecov-commenter commented Sep 8, 2021 •

edited

Loading

Wassaf-Shahzad commented Sep 14, 2021 •

edited

Loading