Skip to content
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.

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

V8: Accessibility changes for login screen #5800

Merged
merged 12 commits into from
May 26, 2020
Merged

V8: Accessibility changes for login screen #5800

merged 12 commits into from
May 26, 2020

Conversation

RachBreeze
Copy link
Contributor

@RachBreeze RachBreeze commented Jul 7, 2019

This PR has a number of accessibility changes for the login screen:

  • Separates out the validation of email address and password. Prior to this the error message displayed "username or password", which was confusing as in many cases the prompt is for "email" not "user name"
  • Added aria-required prompt on inputs for screen readers
  • Added the required prompt to inputs as per the inviteUserPasswordForm
  • Includes localisation support for error messages on login

Rachel Breeze added 3 commits July 7, 2019 19:11
@emmaburstow
Copy link
Contributor

Hi @RachBreeze

Thanks for this work! I've assigned the label. We'll take a look and let you know if we need anything more from you,

Em

@RachBreeze
Copy link
Contributor Author

Hi @emmaburstow thank you

@zersiax
Copy link
Contributor

zersiax commented Sep 17, 2019

@RachBreeze I am reasonably certain that adding required as an HTML attribute adds an implicit aria=-required, so you may not need aria-required here. Did you give this a try? :-)

@RachBreeze
Copy link
Contributor Author

@zersiax I suspect not, looking at the order of my PRs I found an issue in Angular which was triggering "invalid" prompt to be played to an NVDA screen reader user when a user tabs into an empty required text box prior to entering any information but this was when the textbox was on an ng-form element not a form one. Once I've finished my current PR I can take another look

Rachel Breeze added 2 commits October 8, 2019 18:01
…to v8/a11y_login

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html
#	src/Umbraco.Web.UI/Umbraco/config/lang/en.xml
#	src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml
… on tab in. The ngaria work in #6561 is pulled into core will help with alerts here
@RachBreeze
Copy link
Contributor Author

RachBreeze commented Oct 8, 2019

I have reviewed the PR and when I use "required" and you are correct the user hears "required" but they also hear "invalid entry" when the user tabs into the textboxes and not when they tab out which is why I used aria required. The work @Matthew-Wise is doing on NGAria should also improve accessibility of the alerts

Copy link
Contributor Author

@RachBreeze RachBreeze left a comment

Choose a reason for hiding this comment

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

Sorry 65161 referenced incorrectly

Rachel Breeze added 2 commits October 21, 2019 16:34
…However aria-required must remain, as required reports an invalid entry on NVDA when a user tabs into the textboxes the first time. Whereas with aria-required invalid entry is only reported after the form is submitted
Copy link
Contributor

@emmaburstow emmaburstow left a comment

Choose a reason for hiding this comment

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

Rachel, this is ace. Lots of work here ad I've done my best to check all over - I'm happy with the approach and I'm happy to approve. 👍

@RachBreeze
Copy link
Contributor Author

Closing as PR has diverged too far from main branch

@RachBreeze RachBreeze closed this Apr 22, 2020
# Conflicts:
#	src/Umbraco.Web.UI.Client/package-lock.json
@nul800sebastiaan nul800sebastiaan changed the base branch from v8/dev to v8/contrib May 26, 2020 10:00
@nul800sebastiaan
Copy link
Member

@RachBreeze I've merged this one with v8/contrib and it went well, there's not any conflicts that I can see. Any other reason you wanted to close this one? I think it can be reviewed as-is now?

@RachBreeze
Copy link
Contributor Author

Hi @nul800sebastiaan
Thank you, that was the only reason for closing it. It would be great to review as is

Copy link
Member

@nul800sebastiaan nul800sebastiaan left a comment

Choose a reason for hiding this comment

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

@RachBreeze I don't understand this change (adding if (formHelper.submitForm({ scope: $scope })) { which was not there before) and the removal of the block:

            if (vm.login && vm.password && vm.login.length > 0 && vm.password.length > 0) {
                vm.loginForm.username.$setValidity('auth', true);
                vm.loginForm.password.$setValidity('auth', true);
            }

Can you explain why these changes were made?

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/common/directives/components/application/umblogin.directive.js
@nul800sebastiaan nul800sebastiaan merged commit 6da69f1 into umbraco:v8/contrib May 26, 2020
@nul800sebastiaan
Copy link
Member

I'm still not entirely sure about formHelper.submitForm but that seems to work just fine in a lot of scenarios I tested to make logins fail etc (incl. causing server errors by breaking the web.config).

I added back the block above

            if (vm.login && vm.password && vm.login.length > 0 && vm.password.length > 0) {
                vm.loginForm.username.$setValidity('auth', true);
                vm.loginForm.password.$setValidity('auth', true);
            }

As you can read from the comments this was there because when server errors happen you can never resubmit the login as filled in because they both need to change first. This doesn't hinder the a11y fixes for marking fields as invalid when they are invalid though, so that new code works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants