Skip to content

Add description attributes to all app texts for sight impaired a11y support#649

Open
prince-0408 wants to merge 7 commits into
scribe-org:mainfrom
prince-0408:feature/a11y-alt-texts-641
Open

Add description attributes to all app texts for sight impaired a11y support#649
prince-0408 wants to merge 7 commits into
scribe-org:mainfrom
prince-0408:feature/a11y-alt-texts-641

Conversation

@prince-0408

Copy link
Copy Markdown
Contributor

Description

Resolves #641

This PR introduces foundational accessibility support for sight-impaired users by adding contentDescription attributes to the prioritized elements across the app.

Following the initial guidance in the issue, this PR prioritizes the menu options, tab bar, and the texts in the about tab submenus.

Changes Made

Component / Area Description of Changes
ScribeItem Models Updated the ScribeItem base class and its sub-classes (ClickableItem, SwitchableItem, ExternalLinkItem, CustomItem) to accept an optional altText: Int? string resource.
Menu Composables Updated ClickableItemComp, SwitchableItemComp, and AboutPageItemComp to apply Modifier.semantics(mergeDescendants = true) { contentDescription = altText } when an altText string is provided, ensuring screen readers treat the entire row as a single actionable item.
Bottom Navigation Bar Modified BottomBarScreen and ScribeBottomBar to resolve and use string resources for their tab labels and alt texts (labelRes and altTextRes).
String Resources Created corresponding _alt_text labels in string.xml for all tabs and all menu items within the Settings and About screens.
Menu Configurations Applied the new _alt_text string resources to all instantiated ScribeItem configurations inside SettingsScreen.kt, LanguageSettingsScreen.kt, and AboutUtil.kt.

Question for @andrewtavis

While doing a deep dive to verify the accessibility tags across the app, I noticed there are still quite a few hardcoded English strings being used for contentDescription on icons across other screens (e.g., "Back", "Right Arrow", "Search", "Clear" in InstallationScreen, ConjugateScreen, and Tutorial screens).

I wanted to ask: Would you like me to tackle replacing these remaining hardcoded contentDescription tags in this PR, or should we leave them for a follow-up issue?

@github-actions

Copy link
Copy Markdown

Thank you for the pull request! 💙🩵

The Scribe-Android team will do our best to address your contribution as soon as we can. The following are some important points:

  • Those interested in developing their skills and expanding their role in the community should read the mentorship and growth section of the contribution guide
  • If you're not already a member of our public Matrix community, please consider joining!
    • We'd suggest that you use the Element client as well as Element X for a mobile app
    • Join the General and Android rooms once you're in
  • Also consider attending our bi-weekly Saturday developer syncs!
    • Details are shared in the General room on Matrix each Wednesday before the sync
    • It would be great to meet you 😊

Note

Scribe uses Conventional Comments in reviews to make sure that communication is as clear as possible.

@github-actions

Copy link
Copy Markdown

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • Tests for changes have been written and the unit test, linting and formatting workflows within the PR checks do not indicate new errors in the files changed

    • Tests may need to be reran as they're at times not deterministic
  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis

Copy link
Copy Markdown
Member

@prince-0408, would you be able to fix the CI errors that are occurring on this PR?

@DeleMike DeleMike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @prince-0408 , thanks for the PR! 🌟

All looks good to me! And I think it is ready bar the merge conflict.

Could you please look into the merge conflicts? Thanks.

@DeleMike DeleMike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think all is good from my end!

Thanks for resolving the merge conflicts. Thanks for the great work @prince-0408 🚀

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice

@andrewtavis

Copy link
Copy Markdown
Member

Looking into this a bit more and responding the the question for me above:

While doing a deep dive to verify the accessibility tags across the app, I noticed there are still quite a few hardcoded English strings being used for contentDescription on icons across other screens (e.g., "Back", "Right Arrow", "Search", "Clear" in InstallationScreen, ConjugateScreen, and Tutorial screens).

For the tutorial screen, let's not worry about it as @ashb155 should have those covered in scribe-org/Scribe-i18n#117 that's being brought over to #618. Any other strings would ideally be localized and not hard coded. We should also move them over to Scribe-i18n and then bring them in over here.

Ideally app/src/main/res/values/strings.xml would be empty except for any placeholders that we need that the user isn't seeing. We should also make sure that these _alt_text values aren't too similar to their labels, or if the label is descriptive enough for an alt text, then maybe we can just use the label and not have the same strings more than once in the Weblate localization 😊

@prince-0408, do you want to open a PR up in Scribe-i18n with the strings that we need for sight impaired users and any other strings that are needed based on what's on the current main of Scribe-i18n, and then we can merge that and bring it over here?

@prince-0408

Copy link
Copy Markdown
Contributor Author

Looking into this a bit more and responding the the question for me above:

While doing a deep dive to verify the accessibility tags across the app, I noticed there are still quite a few hardcoded English strings being used for contentDescription on icons across other screens (e.g., "Back", "Right Arrow", "Search", "Clear" in InstallationScreen, ConjugateScreen, and Tutorial screens).

For the tutorial screen, let's not worry about it as @ashb155 should have those covered in scribe-org/Scribe-i18n#117 that's being brought over to #618. Any other strings would ideally be localized and not hard coded. We should also move them over to Scribe-i18n and then bring them in over here.

Ideally app/src/main/res/values/strings.xml would be empty except for any placeholders that we need that the user isn't seeing. We should also make sure that these _alt_text values aren't too similar to their labels, or if the label is descriptive enough for an alt text, then maybe we can just use the label and not have the same strings more than once in the Weblate localization 😊

@prince-0408, do you want to open a PR up in Scribe-i18n with the strings that we need for sight impaired users and any other strings that are needed based on what's on the current main of Scribe-i18n, and then we can merge that and bring it over here?

Hi @andrewtavis! Thanks for the thorough review and the deep dive. That makes total sense. I completely agree with keeping strings.xml clean and reusing the descriptive labels for alt texts to avoid duplicating work in Weblate.

I'd be happy to open a PR in the Scribe-i18n repository to add these accessibility strings and move the hardcoded ones over. I'll get that opened up and link it here once it's ready, and then we can pull the submodule updates into this PR afterwards!

@andrewtavis

Copy link
Copy Markdown
Member

Thanks so much for your efforts, @prince-0408!

@prince-0408

Copy link
Copy Markdown
Contributor Author

Thanks so much for your efforts, @prince-0408!

Hi @andrewtavis! Everything is now ready for your review:

  • Submodule Updated: Updated assets/i18n to include the merged changes from PR Add missing accessibility strings and alt_texts Scribe-i18n#118.
  • i18n Keys Assigned: Removed all hardcoded general UI and accessibility labels (Back, Search, Clear, Play button, Expand tenses, Copy conjugation, Right Arrow, Select Keyboard) and mapped them to their new localized keys.
  • Cleaned strings.xml: Kept only the non-user-facing application defaults.
  • Tests & CI Green: Fixed a couple of layout issues and updated the fallback assertion in SettingsUtilTest so that both unit and instrumentation test suites are passing.

Let me know if this looks good to merge!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add description attributes to all app texts for sight impared a11y support

3 participants