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

Fixed presentation of checked / unchecked state for checkboxes and switches and selected state for radio buttons when used with a screen reader #4047

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pvagner
Copy link

@pvagner pvagner commented Dec 16, 2024

Content

Checkboxes, switches and radio buttons are implemented in a way that icon, title, the interactive control and optional description is encapsulated in a parent composable which receives onClick event. Checkboxes, switches and radio buttons them selves have no click handler. When considering universal user experience it's very nice approach as it increases touch target for these UI controls. However when considering screen reader accessibility screen readers are not receiving role and state information for these controls. Thus when using the app with a screen reader I can't find out if a check box is checked or not, if a switch is turned on or not and which of the radio buttons is selected.
I am trying to address this by adding toggleable and selectable modifiers where appropriate.

Motivation and context

I am using this app daily, I used to experiment and guess the state of these checkboxes, switches and radio buttons, ask my friends until I have found I can try to improve it.

Screenshots / GIFs

I believe appearance is unchanged. Also I have opted not to create and work with screenshots at all since I am unable to.

Tests

Tested on a real device.

  • Pressed both the volume buttons simultaneously to enable talkback screen reader
  • Opened the app
  • Touched any of the rooms on the screen
  • Performed the double tap and hold gesture to bring up the context menu
  • Used single finger flick gestures to navigate to the favorite the selected room.
  • I have repeated the similar procedure in the user settings.

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 14

Checklist

  • Changes have been tested on an Android device or Android emulator with API 23
  • UI change has been tested on both light and dark themes
  • Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
  • Pull request is based on the develop branch
  • Pull request title will be used in the release note, it clearly define what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off
  • You've made a self review of your PR

…itches and selected state for radio buttons when used with a screen reader

Signed-off-by: Peter Vágner <[email protected]>
@pvagner pvagner requested a review from a team as a code owner December 16, 2024 13:34
@pvagner pvagner requested review from bmarty and removed request for a team December 16, 2024 13:34
@CLAassistant
Copy link

CLAassistant commented Dec 16, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:

  • Your branch should be based on origin/develop, at least when it was created.
  • The title of the PR will be used for release notes, so it needs to describe the change visible to the user.
  • The test pass locally running ./gradlew test.
  • The code quality check suite pass locally running ./gradlew runQualityChecks.
  • If you modified anything related to the UI, including previews, you'll have to run the Record screenshots GH action in your forked repo: that will generate compatible new screenshots. However, given Github Actions limitations, it will prevent the CI from running temporarily, until you upload a new commit after that one. To do so, just pull the latest changes and push an empty commit.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement! I have submitted a patch with a proposal, let me know hat you think.

role = Role.Checkbox,
enabled = true,
onValueChange = { onEnabledChanged(!state.isEnabled) }
),
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if all these changes could be handle at a single place: in the ListItem implementation. It will reduce the number of change and will have the advantage of always use toggleable/selectable so that developer will not forget about it.

Here is a patch with a proposal:
ListItemModifier.patch

Note: not tested and may not always work, this patch is just to give some guidance about how it could be implemented. More work is necessary to make this patch an acceptable pull request.

Copy link
Author

Choose a reason for hiding this comment

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

Hello, And thanks for the magic. I think it looks awesome. I am building the app with it applied and I'll test it in a little while.

Copy link
Author

Choose a reason for hiding this comment

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

Hello again,
The patch is working as it should with no modifications from me.
It correctly applies modifiers as expected.
However I still have to go through the places I have modified and verify if the list item is really used in a way this implementation is expecting it.
For example:

  • If there is an onClick handler on the switch or check box it-self it's being presented twice with some screen readers. Having click handler on both list item and switch / checkbox should therefore be avoided if we wish to make the screen reader experience fluent. Currently at least the favourite item in the room list long click menu is implemented like this.
  • Some radio buttons are not working and I need to find out why for example when going to settings -> Advanced, there is a setting that affects app theme, it can be System, Light or Dark. And once the dialog is showing the radio buttons don't currently have the modifier applied as expected.

So if you are okay with the outcome and you do have enough bandwidth for doing it, please consider pushing your patch, in favour of this pull request and we should handle edge cases as follow up PRs. It's great improvement and it enables screen reader users to adjust settings on their own if they wish.

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.

3 participants