From b5f363179a0a7d1f19a2281bb13a98cf72869c29 Mon Sep 17 00:00:00 2001 From: ohall-m Date: Wed, 10 Apr 2024 15:35:46 +0000 Subject: [PATCH] Bug 1884929 - Fix Translations Dialog Sheet to Support Pull to Dismiss r=android-reviewers,gl,giorga Before this patch, pulling the translations dialog sheet down did not work except on the top of a very small touch target due to vertical scrolling not being enabled. This patch: * Enables vertical scrolling and reorganizes some of the `Surface` setup. * Changes the settings from `LazyColumn` to `Column` because `verticalScroll` is not compatible with `LazyColumn`. * Sets the dialog handle to close the dialog to be more screen reader compatible by adding a content description and close action. Differential Revision: https://phabricator.services.mozilla.com/D206941 --- .../translations/TranslationOptionsDialog.kt | 60 +++++++++---------- .../translations/TranslationsBottomSheet.kt | 46 ++++++++------ .../TranslationsDialogFragment.kt | 4 +- .../fenix/app/src/main/res/values/strings.xml | 2 + 4 files changed, 62 insertions(+), 50 deletions(-) diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationOptionsDialog.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationOptionsDialog.kt index 608f35224691..8449bf94795a 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationOptionsDialog.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationOptionsDialog.kt @@ -4,6 +4,7 @@ package org.mozilla.fenix.translations +import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.defaultMinSize @@ -11,8 +12,6 @@ import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width -import androidx.compose.foundation.lazy.LazyColumn -import androidx.compose.foundation.lazy.items import androidx.compose.material.Icon import androidx.compose.material.IconButton import androidx.compose.material.Text @@ -50,9 +49,8 @@ fun TranslationOptionsDialog( aboutTranslationClicked: () -> Unit, ) { TranslationOptionsDialogHeader(onBackClicked) - - LazyColumn { - items(translationOptionsList) { item: TranslationSwitchItem -> + translationOptionsList.forEach() { item: TranslationSwitchItem -> + Column { val translationSwitchItem = TranslationSwitchItem( type = item.type, textLabel = item.textLabel, @@ -66,32 +64,32 @@ fun TranslationOptionsDialog( translationSwitchItem = translationSwitchItem, ) } + } - if (showGlobalSettings) { - item { - TextListItem( - label = stringResource(id = R.string.translation_option_bottom_sheet_translation_settings), - modifier = Modifier - .fillMaxWidth() - .padding(start = 56.dp), - onClick = { onTranslationSettingsClicked() }, - ) - } - } - - item { + if (showGlobalSettings) { + Column { TextListItem( - label = stringResource( - id = R.string.translation_option_bottom_sheet_about_translations, - formatArgs = arrayOf(stringResource(R.string.firefox)), - ), + label = stringResource(id = R.string.translation_option_bottom_sheet_translation_settings), modifier = Modifier .fillMaxWidth() .padding(start = 56.dp), - onClick = { aboutTranslationClicked() }, + onClick = { onTranslationSettingsClicked() }, ) } } + + Column { + TextListItem( + label = stringResource( + id = R.string.translation_option_bottom_sheet_about_translations, + formatArgs = arrayOf(stringResource(R.string.firefox)), + ), + modifier = Modifier + .fillMaxWidth() + .padding(start = 56.dp), + onClick = { aboutTranslationClicked() }, + ) + } } @Composable @@ -214,12 +212,14 @@ fun getTranslationOptionsList(): List { @LightDarkPreview private fun TranslationSettingsPreview() { FirefoxTheme { - TranslationOptionsDialog( - translationOptionsList = getTranslationOptionsList(), - showGlobalSettings = true, - onBackClicked = {}, - onTranslationSettingsClicked = {}, - aboutTranslationClicked = {}, - ) + Column { + TranslationOptionsDialog( + translationOptionsList = getTranslationOptionsList(), + showGlobalSettings = true, + onBackClicked = {}, + onTranslationSettingsClicked = {}, + aboutTranslationClicked = {}, + ) + } } } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsBottomSheet.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsBottomSheet.kt index 3ef393de00b9..8d4a74e02c54 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsBottomSheet.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsBottomSheet.kt @@ -12,18 +12,21 @@ import androidx.compose.animation.core.tween import androidx.compose.animation.expandIn import androidx.compose.animation.fadeIn import androidx.compose.animation.slideInHorizontally -import androidx.compose.foundation.background import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.shape.RoundedCornerShape -import androidx.compose.material.Divider +import androidx.compose.foundation.verticalScroll +import androidx.compose.material.Surface import androidx.compose.runtime.Composable import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.input.nestedscroll.nestedScroll import androidx.compose.ui.platform.rememberNestedScrollInteropConnection import androidx.compose.ui.res.stringResource +import androidx.compose.ui.semantics.semantics +import androidx.compose.ui.semantics.traversalIndex import androidx.compose.ui.unit.Density import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.IntSize @@ -31,30 +34,35 @@ import androidx.compose.ui.unit.dp import mozilla.components.concept.engine.translate.Language import mozilla.components.concept.engine.translate.TranslationPageSettings import org.mozilla.fenix.R +import org.mozilla.fenix.compose.BottomSheetHandle import org.mozilla.fenix.theme.FirefoxTheme private const val BOTTOM_SHEET_HANDLE_WIDTH_PERCENT = 0.1f @Composable -internal fun TranslationDialogBottomSheet(content: @Composable () -> Unit) { - Column( - modifier = Modifier - .background( - color = FirefoxTheme.colors.layer1, - shape = RoundedCornerShape(topStart = 8.dp, topEnd = 8.dp), - ) - .nestedScroll(rememberNestedScrollInteropConnection()), +internal fun TranslationDialogBottomSheet( + onRequestDismiss: () -> Unit, + content: @Composable () -> Unit, +) { + Surface( + color = FirefoxTheme.colors.layer1, + shape = RoundedCornerShape(topStart = 8.dp, topEnd = 8.dp), + modifier = Modifier.nestedScroll(rememberNestedScrollInteropConnection()) + .verticalScroll(rememberScrollState()), ) { - Divider( - Modifier - .padding(top = 16.dp) - .fillMaxWidth(BOTTOM_SHEET_HANDLE_WIDTH_PERCENT) - .align(alignment = Alignment.CenterHorizontally), - color = FirefoxTheme.colors.borderInverted, - thickness = 3.dp, - ) + Column { + BottomSheetHandle( + onRequestDismiss = onRequestDismiss, + contentDescription = stringResource(R.string.translation_option_bottom_sheet_close_content_description), + modifier = Modifier + .padding(top = 16.dp) + .fillMaxWidth(BOTTOM_SHEET_HANDLE_WIDTH_PERCENT) + .align(Alignment.CenterHorizontally) + .semantics { traversalIndex = -1f }, + ) - content() + content() + } } } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogFragment.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogFragment.kt index 3b1ad43f5141..7e31dd594dd0 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogFragment.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogFragment.kt @@ -142,7 +142,9 @@ class TranslationsDialogFragment : BottomSheetDialogFragment() { mutableStateOf(false) } - TranslationDialogBottomSheet { + TranslationDialogBottomSheet( + onRequestDismiss = { behavior?.state = BottomSheetBehavior.STATE_HIDDEN }, + ) { TranslationsAnimation( translationsVisibility = translationsVisibility, density = density, diff --git a/mobile/android/fenix/app/src/main/res/values/strings.xml b/mobile/android/fenix/app/src/main/res/values/strings.xml index 1e65802ed977..37bc1b8a0730 100644 --- a/mobile/android/fenix/app/src/main/res/values/strings.xml +++ b/mobile/android/fenix/app/src/main/res/values/strings.xml @@ -2429,6 +2429,8 @@ Translation settings About translations in %1$s + + Close Translations sheet