Add accessibility tags #288
Conversation
WalkthroughThe changes in this pull request focus on enhancing accessibility across various UI components in the application. A new composable function Changes
Possibly related PRs
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: 4
🧹 Outside diff range and nitpick comments (8)
app/src/main/res/layout/group_editor_fragment.xml (1)
31-33: Remove unnecessary design-time attributesThe added
tools:layout_editor_absoluteX="0dp"andtools:layout_editor_absoluteY="0dp"attributes are only used for layout preview in Android Studio and don't affect runtime behavior or accessibility. Consider removing them to keep the layout file clean.app:screenState="@{viewModel.screenState}" - app:screenStateHandler="@{viewModel.screenStateHandler}" - tools:layout_editor_absoluteX="0dp" - tools:layout_editor_absoluteY="0dp" /> + app:screenStateHandler="@{viewModel.screenStateHandler}" />app/src/main/res/layout/dialog_change_password.xml (3)
38-38: Consider enhancing password field accessibilityWhile adding a description is good, password fields benefit from additional accessibility attributes:
app:description="@string/password" +android:importantForAccessibility="yes" +android:contentDescription="@string/password_input_field" +android:accessibilityLiveRegion="polite"Also, consider creating a more descriptive string resource like "Enter your current password" instead of just "password" to provide better context for screen reader users.
56-56: Improve new password field distinction for screen readersThe current description might not provide enough context to distinguish this field from the current password field.
Consider creating a more descriptive string resource that clearly indicates this is for the new password, for example:
-app:description="@string/new_password" +app:description="@string/new_password_input_description" +android:importantForAccessibility="yes" +android:accessibilityLiveRegion="polite"Add to strings.xml:
<string name="new_password_input_description">Enter your desired new password</string>
Line range hint
14-17: Improve dialog-level accessibilityThe dialog container should have proper accessibility attributes to ensure users understand they're in a password change dialog.
Add these attributes to the ConstraintLayout:
android:layout_width="match_parent" android:layout_height="wrap_content" android:background="?attr/kpDialogBackgroundColor" android:minWidth="@dimen/min_dialog_width" +android:contentDescription="@string/change_password_dialog_description" +android:accessibilityTraversalBefore="@id/password" +android:focusable="true" +android:focusableContainer="true"Add to strings.xml:
<string name="change_password_dialog_description">Change password dialog. Contains current password, new password, and confirmation fields.</string>app/src/main/res/layout/new_database_fragment.xml (1)
Line range hint
154-191: Consider implementing a reusable accessibility wrapper.The accessibility improvements are well implemented. To maintain consistency across the app, consider:
- Creating a custom MaterialEditText style that includes these accessibility attributes
- Implementing a base accessibility component that can be reused across different input fields
- Documenting these accessibility patterns in your team's style guide
Example base style:
<style name="AccessibleMaterialEditText" parent="@style/Material.EditText"> <item name="android:importantForAccessibility">yes</item> <item name="android:focusable">true</item> <item name="android:focusableInTouchMode">true</item> </style>app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Accessibility.kt (1)
10-16: Add nullability handling and documentationThe implementation looks good, but consider these improvements:
- Add KDoc documentation explaining the purpose and usage
- Consider handling null descriptions
Here's a suggested improvement:
+/** + * Creates a Modifier that adds a content description to a composable for accessibility. + * + * @param description The description to be read by screen readers. If null or empty, + * no content description will be added. + * @return A Modifier with the semantic content description property + */ @Composable -fun contentDescription(description: String): Modifier { +fun contentDescription(description: String?): Modifier { return remember { + if (description.isNullOrEmpty()) { + Modifier + } else { Modifier.semantics { contentDescription = description } + } } }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginScreen.kt (1)
230-230: Consider adding role descriptions for input fieldsWhile the current implementation is good, consider enhancing it further by specifying the input field roles.
Example enhancement:
- .then(contentDescription(stringResource(R.string.url))) + .then(contentDescription(stringResource(R.string.url_input_field)))This would make it clearer to screen reader users that these are input fields.
Also applies to: 230-230, 308-308, 308-308, 339-339, 339-339
app/src/main/res/values/strings.xml (1)
47-51: LGTM: Well-organized accessibility string resourcesThe accessibility descriptions are properly organized in a dedicated section and follow Android accessibility best practices.
Consider more descriptive strings for better accessibility
While the current strings are functional, they could be more descriptive for better screen reader experience.
Consider these enhancements:
- <string name="url">URL</string> + <string name="url">Website or server URL</string> - <string name="unlock_button">Unlock button</string> + <string name="unlock_button">Unlock database button</string> - <string name="eye_icon">Eye icon</string> + <string name="eye_icon">Toggle password visibility</string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Accessibility.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/MaterialEditText.kt(6 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginScreen.kt(5 hunks)app/src/main/res/layout/dialog_change_password.xml(3 hunks)app/src/main/res/layout/group_editor_fragment.xml(2 hunks)app/src/main/res/layout/groups_fragment.xml(1 hunks)app/src/main/res/layout/new_database_fragment.xml(4 hunks)app/src/main/res/layout/password_generator_fragment.xml(1 hunks)app/src/main/res/layout/widget_material_edit_text.xml(2 hunks)app/src/main/res/layout/widget_unlock_view.xml(2 hunks)app/src/main/res/values/attrs.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/layout/password_generator_fragment.xml
👮 Files not reviewed due to content moderation or server errors (3)
- app/src/main/res/layout/widget_unlock_view.xml
- app/src/main/res/layout/groups_fragment.xml
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/MaterialEditText.kt
🔇 Additional comments (10)
app/src/main/res/layout/group_editor_fragment.xml (1)
44-44: Verify if duplicate text is needed for accessibility
The added description attribute app:description="@string/title" improves accessibility, which is great! However, since it uses the same text as the hint (@string/title), verify if this duplication is necessary for screen readers or if the hint text is already being properly announced.
✅ Verification successful
Duplicate text for accessibility is consistent with app patterns
Based on the search results, this is a consistent pattern throughout the application where MaterialEditText components use the same text for both description and hint attributes. This can be seen in multiple files:
password_generator_fragment.xml: uses same text for description and hintwidget_unlock_view.xml: uses same text for description and hintgroups_fragment.xml: uses same text for description and hintdialog_change_password.xml: uses same text for description and hintnew_database_fragment.xml: uses same text for description and hint (except for confirm password)
The duplication appears to be an intentional design choice for accessibility in this application, making the current implementation correct and consistent with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other MaterialEditText components in the codebase use different text for hint and description
rg -U "MaterialEditText[^>]*hint=\"([^\"]+)\"[^>]*description=\"(?!\1[^\"])" --type xml
Length of output: 404
Script:
#!/bin/bash
# Search for MaterialEditText components with both hint and description attributes
rg -U "MaterialEditText.*?(hint|description).*?(hint|description)" --type xml -A 2
Length of output: 84
Script:
#!/bin/bash
# Search for string resource title to understand its context
rg "title" app/src/main/res/values/strings.xml
Length of output: 1216
Script:
#!/bin/bash
# Look for accessibility-related attributes in layout files
rg -U "android:contentDescription|app:description|android:hint" --type xml -A 1
Length of output: 4917
app/src/main/res/layout/dialog_change_password.xml (1)
38-38: Verify MaterialEditText accessibility implementation
Ensure that the custom MaterialEditText widget properly implements the description attribute for accessibility.
Also applies to: 56-56, 73-73
✅ Verification successful
MaterialEditText properly implements accessibility description
The implementation correctly handles the description attribute by:
- Defining a custom attribute
descriptioninattrs.xml - Setting the
contentDescriptionof the underlying EditText inreadAttributes()method:
val description = params.getString(R.styleable.MaterialEditText_description)
if (description != null) {
binding.textInput.contentDescription = description
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if MaterialEditText properly implements the description attribute
ast-grep --pattern 'class MaterialEditText {
$$$
description($$$) {
$$$
}
$$$
}'
# Check for accessibility-related implementations
rg -l "AccessibilityDelegate|contentDescription|AccessibilityEvent" --type kotlin
Length of output: 794
Script:
#!/bin/bash
# Check MaterialEditText implementation for description handling
cat app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/MaterialEditText.kt
# Check if there are any custom attributes defined for MaterialEditText
fd attrs.xml --exec cat {}
Length of output: 16127
app/src/main/res/layout/new_database_fragment.xml (4)
154-154: LGTM! Good accessibility improvement.
Adding the description attribute enhances screen reader support by providing a clear semantic description of the input field's purpose.
169-169: LGTM! Good accessibility improvement.
Adding the description attribute enhances screen reader support by providing a clear semantic description of the password field's purpose.
133-133: Verify if the tint attribute change is intentional.
This change appears unrelated to the PR's accessibility focus. While the change is valid, it might belong in a separate styling-focused PR.
✅ Verification successful
The tint attribute is consistent with app-wide styling
The app:tint="?attr/kpIconPrimaryColor" is consistently used across multiple layout files for styling icons, including similar info/visibility icons. This is a standard styling pattern in the app, making this change appropriate even in an accessibility-focused PR as it maintains visual consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this is part of a broader styling update
rg -l "kpIconPrimaryColor" --type xml
Length of output: 637
Script:
#!/bin/bash
# Check the usage pattern of tint attribute with kpIconPrimaryColor
rg "app:tint=\"\?attr/kpIconPrimaryColor\"" --type xml -A 2
Length of output: 3954
Line range hint 184-191: Consider validation before database creation.
Good accessibility improvements with the description attribute. The onDoneAction improves keyboard navigation, but please ensure that:
- All form validations are performed before creating the database
- Users are warned about any validation failures
- The UI provides clear feedback during the creation process
✅ Verification successful
Let me check the validation function to ensure it's comprehensive.
Let me try a different pattern to find the validation function.
Let me get the rest of the validation function to see the complete implementation.
Let me get the final part of the validation function to see how it handles password confirmation.
Validation is properly implemented before database creation
The code implements comprehensive validation that:
- Checks for empty fields (filename, password, confirmation)
- Verifies password confirmation matches
- Shows appropriate error messages to users
- Only proceeds with database creation when all validations pass
- Provides clear UI feedback during the process through ScreenState updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if validation is performed in the ViewModel
ast-grep --pattern 'fun createNewDatabaseFile() {
$$$
}'
Length of output: 6771
Script:
#!/bin/bash
# Check the validation function implementation
ast-grep --pattern 'fun isFieldsValid($_, $_, $_) {
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Search for isFieldsValid function with a more flexible pattern
ast-grep --pattern 'private fun isFieldsValid'
# Also search for any validation-related code
rg "isFieldsValid" -A 10
Length of output: 6106
Script:
#!/bin/bash
# Get more context for the validation function
rg "private fun isFieldsValid" -A 30 "app/src/main/kotlin/com/ivanovsky/passnotes/presentation/newdb/NewDatabaseViewModel.kt"
Length of output: 1037
Script:
#!/bin/bash
# Get the complete validation function
rg "private fun isFieldsValid" -A 40 "app/src/main/kotlin/com/ivanovsky/passnotes/presentation/newdb/NewDatabaseViewModel.kt"
Length of output: 1371
app/src/main/res/values/attrs.xml (1)
11-11: Consider adding attribute documentation and verify usage
The new description attribute is a good addition for accessibility. Consider adding documentation in the form of a comment to help other developers understand its purpose.
Here's a suggested improvement:
+ <!-- Content description for accessibility services -->
<attr name="description" format="string" />✅ Verification successful
I'm requesting these additional checks to:
- See all MaterialEditText instances with their attributes to ensure none are missing descriptions
- Verify that all the referenced string resources exist in the strings.xml file
This will help us make a complete assessment of the description attribute usage and its consistency.
Add documentation and consider making description required for MaterialEditText
The description attribute is being used consistently across all MaterialEditText instances in the codebase, with proper string resource references. However, two improvements are recommended:
- Add documentation comment to clarify the attribute's purpose for accessibility:
+ <!-- Description used by accessibility services to describe the input field's purpose -->
<attr name="description" format="string" />- Consider making this attribute required since it's crucial for accessibility and is already being used consistently across all MaterialEditText instances.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the description attribute is being used consistently
echo "Checking usage of description attribute in layout files..."
rg -t xml 'app:description="' app/src/main/res/layout
# Verify if there are any MaterialEditText instances missing the description
echo "Checking for MaterialEditText instances potentially missing descriptions..."
rg -t xml '<.*MaterialEditText[^>]*>' app/src/main/res/layout
Length of output: 1426
Script:
#!/bin/bash
# Let's check for all MaterialEditText instances to ensure we're not missing any
rg -t xml -A 2 'MaterialEditText' app/src/main/res/layout
# Also check the string resources to verify the description strings exist
echo "Checking string resources..."
rg -t xml '<string name="(password|generated_password|file_name|confirm_password|title|query)"' app/src/main/res/values/
Length of output: 4679
app/src/main/res/layout/widget_material_edit_text.xml (1)
Line range hint 15-23: Ensure TextInputEditText has proper accessibility support
The TextInputEditText should have appropriate accessibility support.
✅ Verification successful
Let me gather more context about the TextInputEditText usage and its parent container to better understand the accessibility setup.
TextInputEditText already has proper accessibility support through TextInputLayout
The TextInputEditText is properly wrapped in a TextInputLayout, which is the recommended Material Design pattern for accessibility. The TextInputLayout handles accessibility automatically by:
- Managing the hint/label behavior
- Providing proper content labeling for screen readers
- Supporting error states and helper text
The layout follows Material Design best practices where:
- TextInputLayout has a
hintattribute (shown via tools:hint in the layout) - TextInputEditText is properly nested within TextInputLayout
- The component uses the correct Material Design style (
Widget.MaterialComponents.TextInputLayout.OutlinedBox)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if TextInputEditText instances have proper accessibility attributes
echo "Checking TextInputEditText accessibility attributes..."
rg -t xml '<com\.google\.android\.material\.textfield\.TextInputEditText[^>]*>' app/src/main/res/layout
# Check if there are any accessibility-related attributes already set
echo "Checking existing accessibility attributes..."
rg -t xml 'android:contentDescription|android:hint|android:labelFor' app/src/main/res/layout
Length of output: 1795
Script:
#!/bin/bash
# Check the complete widget_material_edit_text.xml content
echo "Checking complete widget_material_edit_text.xml content..."
cat app/src/main/res/layout/widget_material_edit_text.xml
# Check if there are any accessibility-related strings defined
echo -e "\nChecking accessibility-related strings..."
rg -t xml "name=\".*hint.*\"" app/src/main/res/values/strings.xml
Length of output: 2428
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginScreen.kt (2)
17-17: LGTM: Appropriate imports for accessibility support
The imports for accessibility-related functionality are correctly added.
Also applies to: 31-31
230-230: Well-implemented accessibility descriptions for input fields
The contentDescription modifiers are appropriately added to all text input fields, providing clear semantic meaning for screen readers. The implementation is consistent across URL, username, and password fields.
Also applies to: 230-230, 308-308, 308-308, 339-339, 339-339
| @@ -68,6 +70,7 @@ | |||
| android:layout_width="0dp" | |||
| android:layout_height="wrap_content" | |||
| android:layout_marginTop="@dimen/element_margin" | |||
| app:description="@string/confirm_password" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance password confirmation accessibility and error announcements
The confirmation field needs clear distinction and proper error state handling for accessibility.
- Enhance the description:
-app:description="@string/confirm_password"
+app:description="@string/confirm_password_input_description"
+android:importantForAccessibility="yes"
+android:accessibilityLiveRegion="polite"- Add to strings.xml:
<string name="confirm_password_input_description">Re-enter your new password for confirmation</string>- Ensure error states are properly announced:
+android:accessibilityLiveRegion="assertive"
+app:errorAccessibilityAnnouncement="@{viewModel.confirmationError}"This will ensure that any password mismatch errors are properly announced to screen reader users.
| package com.ivanovsky.passnotes.presentation.core.compose | ||
|
|
||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.runtime.remember | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.semantics.contentDescription | ||
| import androidx.compose.ui.semantics.semantics | ||
|
|
||
| @Composable | ||
| fun contentDescription(description: String): Modifier { | ||
| return remember { | ||
| Modifier.semantics { | ||
| contentDescription = description | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add content descriptions to interactive UI elements
Several Compose UI components lack proper content descriptions for accessibility:
ImageButtoninButtons.ktexplicitly setscontentDescription = nullImagecomponents inHistoryHeaderCell.ktandInfoCell.kthave null content descriptions- Icons in
TextField.kthave null content descriptions
The newly added contentDescription() modifier function is not being utilized yet. These missing descriptions could impact accessibility for users relying on screen readers.
🔗 Analysis chain
Verify usage patterns across the codebase
Let's ensure this new accessibility function is being used consistently across the codebase.
Let me search more specifically for UI components that might need accessibility descriptions and check if the new contentDescription function is being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential UI components that might need content descriptions
echo "Searching for Compose UI components that might need content descriptions..."
rg -t kotlin "Icon|Button|Image|TextField" app/src/main/kotlin
# Check current usage of the new contentDescription function
echo "Checking usage of contentDescription function..."
rg -t kotlin "contentDescription\(" app/src/main/kotlin
Length of output: 68267
Script:
#!/bin/bash
# Search for Compose UI components that commonly need content descriptions
echo "Searching for Compose UI components that need content descriptions..."
rg -t kotlin "Icon\(|Button\(|Image\(|IconButton\(" app/src/main/kotlin
# Check current usage of contentDescription modifier
echo "Checking usage of contentDescription modifier..."
rg -t kotlin "contentDescription\s*=" app/src/main/kotlin
Length of output: 5458
| @@ -30,6 +30,7 @@ | |||
| android:layout_width="wrap_content" | |||
| android:layout_height="wrap_content" | |||
| android:layout_marginTop="@dimen/small_margin" | |||
| android:contentDescription="@string/eye_icon" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider dynamic content description for visibility toggle
The static content description "@string/eye_icon" might not be descriptive enough. Consider making it dynamic based on the visibility state (e.g., "Show password" / "Hide password").
This should be handled in the corresponding Kotlin code rather than the layout XML.
| android:focusable="false" | ||
| android:focusableInTouchMode="false" |
There was a problem hiding this comment.
Review focusability settings for accessibility
Setting focusable="false" and focusableInTouchMode="false" on the ImageButton might prevent screen reader users from accessing the visibility toggle functionality.
Consider removing these attributes to ensure the button is accessible via screen readers and keyboard navigation.
- android:focusable="false"
- android:focusableInTouchMode="false"
+ android:focusable="true"
+ android:importantForAccessibility="yes"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| android:focusable="false" | |
| android:focusableInTouchMode="false" | |
| android:focusable="true" | |
| android:importantForAccessibility="yes" |
Summary by CodeRabbit
New Features
contentDescriptionfunction to improve accessibility for UI elements.descriptionattributes to variousMaterialEditTextcomponents to enhance user guidance.Bug Fixes
Documentation