Improving accessibility on the new user registration page #3272

Andrea-Guevara · 2024-08-27T20:23:34Z

References

Fixes #The new user registration form is not accessible to users using a screen reader #3271

Description

Use of the attributes “attr.aria-label”, “aria-describedby” and “attr.aria-invalid” to improve the user experience when browsing DSpace using a screen reader.

Instructions for reviewers

Use of the “attr.aria-label” attribute which provides a clear description of what the user needs to do in the email field. Use of the “aria-describedby” attribute which describes error messages and use of the “attr.aria-invalid” attribute which informs when a field is invalid.

List of changes in this PR:

In the html the attributes attr.aria-label, aria-describedby and attr.aria-invalid have been added.
Creation of ids in the spans that announce error messages to be used in the aria-describedby attribute.

Note: This PR does not deal with the accessibility of the “Register” button as there is already a PR dealing with this issue.
#3268

To reproduce:

Using a screen reader, go to the login area.
click on “register new user”.
Navigate through the page and notice that there is a better accessibility experience.

tdonohue

@Andrea-Guevara : Thanks for this PR. We reviewed this PR as a group in today's Developers Meeting. I've captured the feedback from the team in inline comments below.
Overall it looks good, but there's some code cleanup that is needed.

This PR also needs to be tested by someone (which I can do after the code cleanup is done), but the code itself looks like it should work.

tdonohue · 2024-08-29T15:16:34Z

.github/workflows/build.yml

@@ -11,7 +11,7 @@ permissions:

 jobs:
  tests:
-    runs-on: ubuntu-latest
+    runs-on: [self-hosted, acervos]


All the changes to the .github/workflows/ files in this PR will need to be removed before this can be merged. So, there are 6 files you've accidentally changed under that folder which need to be reverted.

tdonohue · 2024-08-29T15:17:56Z

src/app/register-email-form/register-email-form.component.html

@@ -14,13 +14,16 @@ <h1>{{MESSAGE_PREFIX + '.header'|translate}}</h1>
                    <label class="font-weight-bold"
                           for="email">{{MESSAGE_PREFIX + '.email' | translate}}</label>
                    <input [className]="(email.invalid) && (email.dirty || email.touched) ? 'form-control is-invalid' :'form-control'"
-                           type="text" id="email" formControlName="email"/>
+                           type="text" id="email" formControlName="email"
+                           [attr.aria-label]="'register-email.aria.label'|translate"


On this line, we'd recommend changing the label to be: MESSAGE_PREFIX + '.aria.label' | translate because the MESSAGE_PREFIX is used for other labels in this same file. See above on line 15.

tdonohue · 2024-08-29T15:18:53Z

src/app/register-email-form/register-email-form.component.html

+                           type="text" id="email" formControlName="email"
+                           [attr.aria-label]="'register-email.aria.label'|translate"
+                           aria-describedby="email-errors-required email-error-not-valid"
+                           [attr.aria-invalid]="form.get('email')?.invalid"/>


You can likely simplify this to be [attr.aria-invalid]="email.invalid", similar to the next line (which also uses email.invalid). It's best to use the same check just for consistency.

Andrea-Guevara · 2024-08-29T15:30:57Z

Good afternoon, thank you very much for your feedback @tdonohue!

I was wondering if you would prefer me to make a new PR available with the proposed changes? that way the code will be cleaner

tdonohue · 2024-08-29T15:47:40Z

@Andrea-Guevara : There is no need to make a new PR. If you feel comfortable doing so, you should be able to use git rebase to remove the last commit (the one that changed the .github/workflow/ files) and then simply add one new commit to cleanup your code.

It is also possible to "squash" multiple commits into a single commit using git rebase. You are welcome to use git rebase as much as you want on your branch, and this PR should update appropriately. That is often easier than creating a new PR...and it makes it easier on reviewers so that they can see the prior feedback in the existing PR, rather than having to compare the new PR to the old PR.

Andrea-Guevara · 2024-08-29T18:40:27Z

Good afternoon @tdonohue!

The changes that have been made to the .github/workflows/ files are internal company orders so I won't be able to undo them.

I'll be back soon with a solution to this inconvenience. I'll possibly make a new PR available.

tdonohue · 2024-08-29T18:52:26Z

@Andrea-Guevara : Ok, just to be aware, we are unable to merge any PR which includes changes to .github/workflow/* files. So, please make sure that no other PRs from your company include those changes, or else we cannot merge them.

In case it isn't clear, merging those changes would cause all DSpace PRs (from every institution in the world) to use your company settings. That would not be desirable for DSpace or for your company.

Andrea-Guevara · 2024-08-29T18:56:19Z

I understand! I'll make sure that doesn't happen

tdonohue · 2024-08-29T18:59:54Z

@Andrea-Guevara : I was just looking around at your other recent PRs, and it looks like they all include this new change to .github/workflow/* files. So, this unfortunately means that you may need to recreate them all, if you cannot remove that commit.

I wanted to make sure you are aware that this seems to have been pushed to all PRs from you.

Andrea-Guevara · 2024-08-29T19:24:20Z

Thanks for the comment! It'll work out hehe

…gotten password forms

Andrea-Guevara · 2024-09-05T13:28:40Z

Good morning @tdonohue, I hope you're well!

After internal discussions, we have reached a consensus and reverted the commit that contained the aforementioned files.

We've also done the code refactoring that was requested.

Looking forward to your feedback!

tdonohue

@Andrea-Guevara : Apologies for the delay in getting back to this. The basics of this PR work, but I'm not sure we should hardcode the aria-describedby on the email field. See my inline comment below.

tdonohue · 2024-10-30T18:36:48Z

src/app/register-email-form/register-email-form.component.html

-                           type="text" id="email" formControlName="email"/>
+                           type="text" id="email" formControlName="email"
+                           [attr.aria-label]="MESSAGE_PREFIX + '.aria.label' | translate"
+                           aria-describedby="email-errors-required email-error-not-valid"


I'm not sure this aria-describedby should be hardcoded. If there are no errors on the form, then both "email-errors-required" and "email-error-not-valid" will not exist. In that scenario, won't this aria-describedby be incorrect?

It seems like the aria-describedby field should be more dynamic. It should say email-errors-required if email.errors is true, but it should say email-error-not-valid if email.invalid is true. If no errors exist, then it seems like aria-describedby should also not exist.

Hello, good afternoon @tdonohue!

Certainly if there is no error in the form the “aria-describedby” is not necessary, but in this case, its purpose is to announce to users who make use of assistive technology that there are errors in the form and therefore it is not possible to continue. Like sometimes, we type our email in the wrong way and don't realize it hehe.

Do you think this isn't necessary, or do you know of a better way to address this issue?

I welcome your suggestions. Looking forward to hearing from you!

Hi @Andrea-Guevara : The problem that I see is that when there are no errors, then aria-describedby="email-errors-required email-error-not-valid" will point at fields that do not exist in the page. In other words, the email-errors-required field ONLY exists if the email is empty. And, the email-error-not-valid field ONLY exists if an invalid email is entered.

So, it seems like the aria-describedby should ONLY point at fields which exist. If an error field is visible, then it should be listed in the aria-describedby. But, if there are no errors, then the aria-describedby should be empty (or not exist).

If it's not clear, I agree that we need the aria-describedby attribute to notify users of errors in the form. But, I feel that attribute should not exist when there are no errors in the form (otherwise, it is notifying users of something that doesn't exist). The aria-describedby also should only ever have one value (at most), because either the email-errors-required error will exist, or the email-error-no-valid error will exist (but not both at the same time).

Hello @tdonohue

Thank you very much for your feedback. I understand your comment! I've provided a possible fix

I look forward to hearing from you

tdonohue · 2024-10-31T17:29:39Z

src/app/register-email-form/register-email-form.component.html

-                           type="text" id="email" formControlName="email"/>
+                           type="text" id="email" formControlName="email"
+                           [attr.aria-label]="MESSAGE_PREFIX + '.aria.label' | translate"
+                           [attr.aria-describedby]="ariaDescribedby"


@Andrea-Guevara : I think you can make this dynamic in a much easier way.

Something like this should work:
[attr.aria-describedby]="(!email.errors) ? '' : (email.errors.required ? 'email-errors-required' : 'email-error-not-valid')"

That logic is just an if/else structure. If there are no errors (!email.errors) have an empty string in aria-describedby. Otherwise, put in the correct error field ID based on whether email.errors.required is true or false.

I think this logic would be a lot easier to understand. It'd also allow you to remove all of your recent changes to the register-email-form.component.ts class.

(You'll also see similar if/else structures in other HTML attributes in this same file, so this is more similar to the current code.)

@tdonohue Your code suggestion works perfectly. Thank you very much :)

tdonohue

👍 Thanks @Andrea-Guevara This now looks good. I tested it and it seems to be working well now.

dspace-bot · 2024-11-01T14:37:52Z

Successfully created backport PR for dspace-7_x:

[Port dspace-7_x] Improving accessibility on the new user registration page #3592

dspace-bot · 2024-11-01T14:37:56Z

Successfully created backport PR for dspace-8_x:

[Port dspace-8_x] Improving accessibility on the new user registration page #3593

Improving accessibility on the new user registration page

0eb2d5c

github-actions bot assigned Andrea-Guevara Aug 27, 2024

Andrea-Guevara mentioned this pull request Aug 27, 2024

The new user registration form is not accessible to users using a screen reader #3271

Open

tdonohue added bug authentication: general general authentication issues authentication: password related to built in password authentication 1 APPROVAL pull request only requires a single approval to merge accessibility labels Aug 27, 2024

tdonohue requested changes Aug 29, 2024

View reviewed changes

IgorBaptist4 force-pushed the AccessibilityInNewUserRegistration branch from 7578001 to 0eb2d5c Compare September 4, 2024 20:29

Code refactoring - Accessibility on the new user registration and for…

f00eae6

…gotten password forms

tdonohue self-requested a review September 6, 2024 14:16

tdonohue reviewed Oct 30, 2024

View reviewed changes

Andrea-Guevara added 2 commits October 31, 2024 11:27

Dynamic aria-describedby attribute

e629d9e

Resolving a wrongly declared variable error

7b72a5f

tdonohue reviewed Oct 31, 2024

View reviewed changes

Andrea-Guevara added 2 commits October 31, 2024 15:01

Simplifying the implementation of dynamic aria-describedby

fa6e85d

Adjusting spaces in ts

009da08

tdonohue approved these changes Nov 1, 2024

View reviewed changes

tdonohue added port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Nov 1, 2024

tdonohue added this to the 9.0 milestone Nov 1, 2024

tdonohue merged commit 4fd3df4 into DSpace:main Nov 1, 2024
11 checks passed

dspace-bot mentioned this pull request Nov 1, 2024

[Port dspace-7_x] Improving accessibility on the new user registration page #3592

Merged

dspace-bot mentioned this pull request Nov 1, 2024

[Port dspace-8_x] Improving accessibility on the new user registration page #3593

Merged

tdonohue removed port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving accessibility on the new user registration page #3272

Improving accessibility on the new user registration page #3272

Andrea-Guevara commented Aug 27, 2024

tdonohue left a comment •

edited

Loading

tdonohue Aug 29, 2024

tdonohue Aug 29, 2024

tdonohue Aug 29, 2024

Andrea-Guevara commented Aug 29, 2024

tdonohue commented Aug 29, 2024

Andrea-Guevara commented Aug 29, 2024

tdonohue commented Aug 29, 2024

Andrea-Guevara commented Aug 29, 2024

tdonohue commented Aug 29, 2024

Andrea-Guevara commented Aug 29, 2024

Andrea-Guevara commented Sep 5, 2024

tdonohue left a comment

tdonohue Oct 30, 2024 •

edited

Loading

Andrea-Guevara Oct 30, 2024

tdonohue Oct 30, 2024

tdonohue Oct 30, 2024

Andrea-Guevara Oct 31, 2024

tdonohue Oct 31, 2024

Andrea-Guevara Oct 31, 2024

tdonohue left a comment

dspace-bot commented Nov 1, 2024

dspace-bot commented Nov 1, 2024

Improving accessibility on the new user registration page #3272

Improving accessibility on the new user registration page #3272

Conversation

Andrea-Guevara commented Aug 27, 2024

References

Description

Instructions for reviewers

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andrea-Guevara commented Aug 29, 2024

tdonohue commented Aug 29, 2024

Andrea-Guevara commented Aug 29, 2024

tdonohue commented Aug 29, 2024

Andrea-Guevara commented Aug 29, 2024

tdonohue commented Aug 29, 2024

Andrea-Guevara commented Aug 29, 2024

Andrea-Guevara commented Sep 5, 2024

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

dspace-bot commented Nov 1, 2024

dspace-bot commented Nov 1, 2024

tdonohue left a comment •

edited

Loading

tdonohue Oct 30, 2024 •

edited

Loading