Fix login screen accessibility: add form labels, lang attribute, and update link contrast (#1373) #2252
Conversation
…update link contrast (autolab#1373)
📝 WalkthroughWalkthroughThe pull request includes updates to two files. The Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/views/devise/sessions/new.html.erb (2)
9-12: Consider removing redundant placeholder textThe label implementation looks good and follows accessibility best practices. However, the placeholder text duplicates the label which is redundant. Consider removing the placeholder or using it for format hints instead (e.g., "[email protected]").
- <%= f.email_field :email, autofocus: true, placeholder: "Email Address" %> + <%= f.email_field :email, autofocus: true, placeholder: "[email protected]" %>
13-16: Add password requirements hint for better accessibilityWhile the label implementation is correct, consider adding password requirements using
aria-describedbyto help users understand password constraints before submission.- <%= f.label :password, "Password" %> - <%= f.password_field :password, placeholder: "Password" %> + <%= f.label :password, "Password" %> + <span id="password-requirements" class="sr-only">Password must be at least 8 characters long</span> + <%= f.password_field :password, + "aria-describedby": "password-requirements", + placeholder: "Enter your password" %>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/assets/stylesheets/_variables.scss(1 hunks)app/views/devise/sessions/new.html.erb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/assets/stylesheets/_variables.scss
| <div class="sign-in-panel home" lang="en"> | ||
| <br> | ||
|
|
||
| <%= form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| %> | ||
|
|
||
| <img src="/images/autolab.svg" alt="" class="valign login-logo"> | ||
| <h5>Login to Autolab</h5> | ||
|
|
||
| <div><%= f.email_field :email, autofocus: true, placeholder: "Email Address" %></div> | ||
| <div><%= f.password_field :password, placeholder: "Password" %></div> | ||
| <div> | ||
| <%= f.label :email, "Email Address" %> | ||
| <%= f.email_field :email, autofocus: true, placeholder: "Email Address" %> | ||
| </div> | ||
| <div> | ||
| <%= f.label :password, "Password" %> | ||
| <%= f.password_field :password, placeholder: "Password" %> | ||
| </div> |
There was a problem hiding this comment.
💡 Codebase verification
Heading structure needs improvement for better accessibility
Based on the search results, the login page's h5 heading appears without any preceding higher-level headings, while the rest of the application follows a more structured heading hierarchy (h1-h4). To maintain consistent and accessible heading structure:
- Change
<h5>Login to Autolab</h5>to<h1>Login to Autolab</h1>since it's the main heading of the page - Move the language attribute from the sign-in panel to the
htmlelement - Add
role="main"to the sign-in panel for better landmark navigation
Example:
<div class="sign-in-panel home" role="main">
<br>
<%= form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| %>
<img src="/images/autolab.svg" alt="" class="valign login-logo">
<h1>Login to Autolab</h1>
...🔗 Analysis chain
Additional accessibility improvements to consider
While the current changes improve accessibility, consider these additional enhancements:
- Improve the heading structure - either add higher-level headings or change
h5toh1 - Restructure the remember me checkbox to use proper label association instead of wrapping with
<label> - Add
role="main"to the main content area
Let's check the current heading structure:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for heading elements in the views
rg "<h[1-6]" app/views/
Length of output: 21123
| @@ -1,13 +1,19 @@ | |||
| <div class="sign-in-panel home"> | |||
| <div class="sign-in-panel home" lang="en"> | |||
There was a problem hiding this comment.
💡 Codebase verification
Move lang attribute from div to HTML element in layout files
The lang attribute should be moved from the div to the <html> element in both layout files:
app/views/layouts/application.html.erbapp/views/layouts/doorkeeper/application.html.erb
This ensures proper language identification for the entire document and better compliance with WCAG guidelines. Remove the lang attribute from the div in app/views/devise/sessions/new.html.erb once added to the layout files.
🔗 Analysis chain
Move lang attribute to the HTML element
While adding the lang attribute is a step in the right direction, it should be placed on the <html> element rather than a <div>. This ensures proper language identification for the entire document and better compliance with WCAG guidelines.
Let's verify if the layout file already has a lang attribute:
The output shows that there are two layout files containing <html> tags, and neither of them has the lang attribute. Let me verify if these are the only layout files and their complete paths to provide accurate feedback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the application layout file and check for lang attribute
fd --type f "application.html" --exec cat {} \; | grep -A 5 "<html"
Length of output: 504
Script:
#!/bin/bash
# Get complete paths of layout files
fd --type f "application.html"
Length of output: 119
|
Hi David, it looks like all the checks have passed on your PR. It's great that you are making changes that makes the website more accessible. One thing that might be nice for you to add is to explain how you know the updated color adheres to the AA guidelines. Otherwise, it looks like you've address all three pain points outlined in the issue. The language is specified, you have modified the color to adhere to AA guidelines, and the email and password fields now have labels for voice navigation. |
…update link contrast (#1373)
Description
Added accessibility improvements to the login screen:
Motivation and Context
Fixes #1373
These changes improve accessibility for users relying on screen readers and those with visual impairments by providing proper form labels, language identification, and sufficient color contrast.
How Has This Been Tested?
Tested locally by:
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting