Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions .../kotlin/io/element/android/features/analytics/api/preferences/AnalyticsPreferencesView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
package io.element.android.features.analytics.api.preferences

import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.selection.toggleable
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import androidx.compose.ui.tooling.preview.PreviewParameter
import io.element.android.features.analytics.api.AnalyticsOptInEvents
import io.element.android.features.analytics.api.R
Expand Down Expand Up @@ -43,6 +45,13 @@ fun AnalyticsPreferencesView(
)
Column(modifier) {
ListItem(
modifier = Modifier
.toggleable(
value = state.isEnabled,
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.

Copy link
Author

Choose a reason for hiding this comment

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

Dear @bmarty
I have noticed you are leading efforts on improving accessibility these days.
I have found out what you are proposing for trailingContent is working fine however we should add the same thing for leadingContent on the ListItems to fix radio button presentation for screen reader users.
Can you please include this or would you like me to take your patch and edit this PR accordingly?

I'd appreciate if this got fixed in the mean time.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @pvagner , I have created a replacement PR #4626

Let me check what's need to be done for leadingContent.

headlineContent = {
Text(stringResource(id = R.string.screen_analytics_settings_share_data))
},
Expand All @@ -52,10 +61,7 @@ fun AnalyticsPreferencesView(
leadingContent = null,
trailingContent = ListItemContent.Switch(
checked = state.isEnabled,
),
onClick = {
onEnabledChanged(!state.isEnabled)
}
)
)
ListSupportingText(annotatedString = linkText)
}
Expand Down
21 changes: 18 additions & 3 deletions ...ain/kotlin/io/element/android/features/createroom/impl/configureroom/ConfigureRoomView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.imePadding
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.selection.selectable
import androidx.compose.foundation.selection.selectableGroup
import androidx.compose.foundation.text.KeyboardOptions
import androidx.compose.foundation.verticalScroll
Expand All @@ -31,6 +32,7 @@ import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.LocalFocusManager
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import androidx.compose.ui.text.input.KeyboardCapitalization
import androidx.compose.ui.tooling.preview.PreviewParameter
import androidx.compose.ui.unit.dp
Expand Down Expand Up @@ -281,6 +283,13 @@ private fun RoomVisibilityOptions(
RoomVisibilityItem.entries.forEach { item ->
val isSelected = item == selected
ListItem(
modifier = Modifier
.selectable(
selected = isSelected,
role = Role.RadioButton,
enabled = true,
onClick = { onOptionClick(item) }
),
leadingContent = ListItemContent.Custom {
RoundedIconAtom(
size = RoundedIconAtomSize.Big,
Expand All @@ -291,7 +300,6 @@ private fun RoomVisibilityOptions(
headlineContent = { Text(text = stringResource(item.title)) },
supportingContent = { Text(text = stringResource(item.description)) },
trailingContent = ListItemContent.RadioButton(selected = isSelected),
onClick = { onOptionClick(item) },
)
}
}
Expand All @@ -308,11 +316,18 @@ private fun RoomAccessOptions(
modifier = modifier,
) {
RoomAccessItem.entries.forEach { item ->
val isSelected = item == selected
ListItem(
modifier = Modifier
.selectable(
selected = isSelected,
role = Role.RadioButton,
enabled = true,
onClick = { onOptionClick(item) }
),
headlineContent = { Text(text = stringResource(item.title)) },
supportingContent = { Text(text = stringResource(item.description)) },
trailingContent = ListItemContent.RadioButton(selected = item == selected),
onClick = { onOptionClick(item) },
trailingContent = ListItemContent.RadioButton(selected = isSelected),
)
}
}
Expand Down
10 changes: 9 additions & 1 deletion ...mpl/src/main/kotlin/io/element/android/features/messages/impl/report/ReportMessageView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import androidx.compose.foundation.layout.heightIn
import androidx.compose.foundation.layout.imePadding
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.selection.toggleable
import androidx.compose.foundation.verticalScroll
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.MaterialTheme
Expand All @@ -27,6 +28,7 @@ import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.LocalFocusManager
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import androidx.compose.ui.tooling.preview.PreviewParameter
import androidx.compose.ui.unit.dp
import io.element.android.compound.theme.ElementTheme
Expand Down Expand Up @@ -102,6 +104,12 @@ fun ReportMessageView(

Row(
modifier = Modifier
.toggleable(
value = state.blockUser,
role = Role.Checkbox,
enabled = !isSending,
onValueChange = { state.eventSink(ReportMessageEvents.ToggleBlockUser) }
)
.fillMaxWidth()
.padding(vertical = 12.dp)
) {
Expand All @@ -119,7 +127,7 @@ fun ReportMessageView(
Switch(
enabled = !isSending,
checked = state.blockUser,
onCheckedChange = { state.eventSink(ReportMessageEvents.ToggleBlockUser) },
onCheckedChange = null,
)
}

Expand Down
13 changes: 11 additions & 2 deletions .../poll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.itemsIndexed
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.foundation.selection.toggleable
import androidx.compose.foundation.text.KeyboardOptions
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.filled.Add
Expand All @@ -30,6 +31,7 @@ import androidx.compose.ui.Modifier
import androidx.compose.ui.focus.FocusRequester
import androidx.compose.ui.focus.focusRequester
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import androidx.compose.ui.text.input.ImeAction
import androidx.compose.ui.text.input.KeyboardCapitalization
import androidx.compose.ui.tooling.preview.PreviewParameter
Expand Down Expand Up @@ -145,7 +147,7 @@ fun CreatePollView(
trailingContent = ListItemContent.Custom {
Icon(
imageVector = CompoundIcons.Delete(),
contentDescription = null,
contentDescription = stringResource(id = R.string.screen_create_poll_remove_option_btn),
modifier = Modifier.clickable(answer.canDelete) {
state.eventSink(CreatePollEvents.RemoveAnswer(index))
},
Expand Down Expand Up @@ -176,11 +178,18 @@ fun CreatePollView(
Column {
HorizontalDivider()
ListItem(
modifier = Modifier
.toggleable(
value = state.pollKind == PollKind.Undisclosed,
role = Role.Checkbox,
enabled = true,
onValueChange = { state.eventSink(CreatePollEvents.SetPollKind(if (it) PollKind.Undisclosed else PollKind.Disclosed)) }
),
headlineContent = { Text(text = stringResource(id = R.string.screen_create_poll_anonymous_headline)) },
supportingContent = { Text(text = stringResource(id = R.string.screen_create_poll_anonymous_desc)) },
trailingContent = ListItemContent.Switch(
checked = state.pollKind == PollKind.Undisclosed,
onChange = { state.eventSink(CreatePollEvents.SetPollKind(if (it) PollKind.Undisclosed else PollKind.Disclosed)) },
onChange = null,
),
)
if (state.canDelete) {
Expand Down
1 change: 1 addition & 0 deletions features/poll/impl/src/main/res/values/localazy.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:xliff="urn:oasis:names:tc:xliff:document:1.2">
<string name="screen_create_poll_add_option_btn">"Add option"</string>
<string name="screen_create_poll_remove_option_btn">"Remove option"</string>
<string name="screen_create_poll_anonymous_desc">"Show results only after poll ends"</string>
<string name="screen_create_poll_anonymous_headline">"Hide votes"</string>
<string name="screen_create_poll_answer_hint">"Option %1$d"</string>
Expand Down
50 changes: 35 additions & 15 deletions ...main/kotlin/io/element/android/features/preferences/impl/advanced/AdvancedSettingsView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@

package io.element.android.features.preferences.impl.advanced

import androidx.compose.foundation.selection.toggleable
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import androidx.compose.ui.tooling.preview.PreviewParameter
import im.vector.app.features.analytics.plan.Interaction
import io.element.android.compound.theme.Theme
Expand Down Expand Up @@ -53,6 +55,13 @@ fun AdvancedSettingsView(
}
)
ListItem(
modifier = Modifier
.toggleable(
value = state.isDeveloperModeEnabled,
role = Role.Checkbox,
enabled = true,
onValueChange = { state.eventSink(AdvancedSettingsEvents.SetDeveloperModeEnabled(!state.isDeveloperModeEnabled)) }
),
headlineContent = {
Text(text = stringResource(id = CommonStrings.action_view_source))
},
Expand All @@ -61,10 +70,16 @@ fun AdvancedSettingsView(
},
trailingContent = ListItemContent.Switch(
checked = state.isDeveloperModeEnabled,
),
onClick = { state.eventSink(AdvancedSettingsEvents.SetDeveloperModeEnabled(!state.isDeveloperModeEnabled)) }
)
)
ListItem(
modifier = Modifier
.toggleable(
value = state.isSharePresenceEnabled,
role = Role.Checkbox,
enabled = true,
onValueChange = { state.eventSink(AdvancedSettingsEvents.SetSharePresenceEnabled(!state.isSharePresenceEnabled)) }
),
headlineContent = {
Text(text = stringResource(id = R.string.screen_advanced_settings_share_presence))
},
Expand All @@ -73,10 +88,26 @@ fun AdvancedSettingsView(
},
trailingContent = ListItemContent.Switch(
checked = state.isSharePresenceEnabled,
),
onClick = { state.eventSink(AdvancedSettingsEvents.SetSharePresenceEnabled(!state.isSharePresenceEnabled)) }
)
)
ListItem(
modifier = Modifier
.toggleable(
value = state.doesCompressMedia,
role = Role.Checkbox,
enabled = true,
onValueChange = {
val newValue = !state.doesCompressMedia
analyticsService.captureInteraction(
if (newValue) {
Interaction.Name.MobileSettingsOptimizeMediaUploadsEnabled
} else {
Interaction.Name.MobileSettingsOptimizeMediaUploadsDisabled
}
)
state.eventSink(AdvancedSettingsEvents.SetCompressMedia(newValue))
}
),
headlineContent = {
Text(text = stringResource(id = R.string.screen_advanced_settings_media_compression_title))
},
Expand All @@ -86,17 +117,6 @@ fun AdvancedSettingsView(
trailingContent = ListItemContent.Switch(
checked = state.doesCompressMedia,
),
onClick = {
val newValue = !state.doesCompressMedia
analyticsService.captureInteraction(
if (newValue) {
Interaction.Name.MobileSettingsOptimizeMediaUploadsEnabled
} else {
Interaction.Name.MobileSettingsOptimizeMediaUploadsDisabled
}
)
state.eventSink(AdvancedSettingsEvents.SetCompressMedia(newValue))
}
)
}

Expand Down
11 changes: 9 additions & 2 deletions .../android/features/preferences/impl/notifications/edit/DefaultNotificationSettingOption.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@

package io.element.android.features.preferences.impl.notifications.edit
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.selection.selectable
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import io.element.android.features.preferences.impl.R
import io.element.android.libraries.designsystem.components.list.ListItemContent
import io.element.android.libraries.designsystem.preview.ElementPreview
Expand Down Expand Up @@ -38,11 +40,16 @@ fun DefaultNotificationSettingOption(
else -> null
}
ListItem(
modifier = modifier,
modifier = modifier
.selectable(
selected = isSelected,
role = Role.RadioButton,
enabled = true,
onClick = { onSelectOption(mode) }
),
headlineContent = { Text(title) },
supportingContent = subtitle?.let { { Text(it) } },
trailingContent = ListItemContent.RadioButton(selected = isSelected),
onClick = { onSelectOption(mode) },
)
}

Expand Down
11 changes: 9 additions & 2 deletions .../android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsOption.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
package io.element.android.features.roomdetails.impl.notificationsettings

import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.selection.selectable
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import io.element.android.features.roomdetails.impl.R
import io.element.android.libraries.designsystem.components.list.ListItemContent
import io.element.android.libraries.designsystem.preview.ElementPreview
Expand All @@ -37,12 +39,17 @@ fun RoomNotificationSettingsOption(
else -> null
}
ListItem(
modifier = modifier,
modifier = modifier
.selectable(
selected = isSelected,
role = Role.RadioButton,
enabled = enabled,
onClick = { onSelectOption(roomNotificationSettingsItem) }
),
enabled = enabled,
headlineContent = { Text(title) },
supportingContent = subtitle?.let { { Text(it) } },
trailingContent = ListItemContent.RadioButton(selected = isSelected),
onClick = { onSelectOption(roomNotificationSettingsItem) },
)
}

Expand Down
16 changes: 10 additions & 6 deletions ...ist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListContextMenu.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ package io.element.android.features.roomlist.impl
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.selection.toggleable
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.MaterialTheme
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.Role
import androidx.compose.ui.text.font.FontStyle
import androidx.compose.ui.tooling.preview.PreviewParameter
import io.element.android.compound.theme.ElementTheme
Expand Down Expand Up @@ -123,6 +125,13 @@ private fun RoomListModalBottomSheetContent(
}
}
ListItem(
modifier = Modifier
.toggleable(
value = contextMenu.isFavorite,
role = Role.Checkbox,
enabled = true,
onValueChange = { onFavoriteChange(!contextMenu.isFavorite) }
),
headlineContent = {
Text(
text = stringResource(id = CommonStrings.common_favourite),
Expand All @@ -137,13 +146,8 @@ private fun RoomListModalBottomSheetContent(
),
trailingContent = ListItemContent.Switch(
checked = contextMenu.isFavorite,
onChange = { isFavorite ->
onFavoriteChange(isFavorite)
},
onChange = null,
),
onClick = {
onFavoriteChange(!contextMenu.isFavorite)
},
style = ListItemStyle.Primary,
)
ListItem(
Expand Down
Loading
Loading