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.

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

Fix voice over for invalid input #10272

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix voice over for invalid input #10272

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd54637
Set error message for voice over
cttsai-stripe Feb 26, 2025
e6b0336
Add test
cttsai-stripe Feb 26, 2025
c5683ba
Fix lint
cttsai-stripe Feb 26, 2025
144d046
Update errorMessage naming & javadoc
cttsai-stripe Feb 28, 2025
cb0493c
Update tests
cttsai-stripe Feb 28, 2025
b74dfcd
Update naming
cttsai-stripe Feb 28, 2025
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
3 changes: 1 addition & 2 deletions payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CardNumberController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import com.stripe.android.ui.core.elements.events.LocalCardNumberCompletedEventR
import com.stripe.android.uicore.elements.FieldError
import com.stripe.android.uicore.elements.IdentifierSpec
import com.stripe.android.uicore.elements.SectionFieldElement
import com.stripe.android.uicore.elements.SectionFieldErrorController
import com.stripe.android.uicore.elements.TextFieldController
import com.stripe.android.uicore.elements.TextFieldIcon
import com.stripe.android.uicore.elements.TextFieldState
Expand All @@ -50,7 +49,7 @@ import kotlinx.coroutines.flow.drop
import kotlin.coroutines.CoroutineContext
import com.stripe.android.R as PaymentsCoreR

internal sealed class CardNumberController : TextFieldController, SectionFieldErrorController {
internal sealed class CardNumberController : TextFieldController {
abstract val cardBrandFlow: StateFlow<CardBrand>

abstract val selectedCardBrandFlow: StateFlow<CardBrand>
Expand Down
3 changes: 1 addition & 2 deletions payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CvcController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import com.stripe.android.core.strings.resolvableString
import com.stripe.android.model.CardBrand
import com.stripe.android.ui.core.asIndividualDigits
import com.stripe.android.uicore.elements.FieldError
import com.stripe.android.uicore.elements.SectionFieldErrorController
import com.stripe.android.uicore.elements.TextFieldController
import com.stripe.android.uicore.elements.TextFieldIcon
import com.stripe.android.uicore.elements.TextFieldState
Expand All @@ -30,7 +29,7 @@ class CvcController constructor(
cardBrandFlow: StateFlow<CardBrand>,
override val initialValue: String? = null,
override val showOptionalLabel: Boolean = false
) : TextFieldController, SectionFieldErrorController {
) : TextFieldController {
override val capitalization: KeyboardCapitalization = cvcTextFieldConfig.capitalization
override val keyboardType: KeyboardType = cvcTextFieldConfig.keyboard

Expand Down
11 changes: 11 additions & 0 deletions paymentsheet/src/androidTest/java/com/stripe/android/paymentsheet/PaymentSheetPage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ package com.stripe.android.paymentsheet

import android.view.accessibility.AccessibilityNodeInfo
import android.widget.Button
import androidx.compose.ui.semantics.SemanticsProperties
import androidx.compose.ui.test.ExperimentalTestApi
import androidx.compose.ui.test.SemanticsMatcher
import androidx.compose.ui.test.SemanticsNodeInteraction
import androidx.compose.ui.test.assert
import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.assertIsEnabled
import androidx.compose.ui.test.hasContentDescription
Expand Down Expand Up @@ -275,3 +279,10 @@ internal class PaymentSheetPage(
replaceText("Phone (optional)", phone)
}
}

fun SemanticsNodeInteraction.assertHasErrorMessage(expectedMessage: String) =
assert(
SemanticsMatcher("has error '$expectedMessage'") { node ->
node.config[SemanticsProperties.Error] == expectedMessage
}
)
36 changes: 36 additions & 0 deletions paymentsheet/src/androidTest/java/com/stripe/android/paymentsheet/PaymentSheetTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package com.stripe.android.paymentsheet

import androidx.compose.ui.test.assertIsFocused
import androidx.compose.ui.test.hasContentDescription
import androidx.compose.ui.test.onAllNodesWithTag
import androidx.compose.ui.test.onNodeWithText
import com.google.testing.junit.testparameterinjector.TestParameter
import com.google.testing.junit.testparameterinjector.TestParameterInjector
import com.stripe.android.core.utils.urlEncode
Expand Down Expand Up @@ -451,4 +453,38 @@ internal class PaymentSheetTest {

testContext.markTestSucceeded()
}

@Test
fun testCardDetailsErrorMessageAccessibility() = runPaymentSheetTest(
Copy link
Collaborator

Choose a reason for hiding this comment

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

a couple ideas to improve this:

  • could we have separate tests for the card number field and the expiration date field? if the card number field case started failing, the test would stop at that assert and we wouldn't immediately know whether the expiration date field case was also failing
  • wdyt of including these tests in a non-network test test class? These tests take a long time to run and are also more prone to flakiness. We really don't need to test any of the set up (ie the elements session response) to test this case, so I think this might be more appropriate to test in something more similar to a unit test, e.g. VerticalModeFormUITest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VerticalModeFormUITest seems reasonable. I am not sure if this is the best place but I didn't find a better one though.

networkRule = networkRule,
integrationType = integrationType,
resultCallback = ::assertCompleted,
) { testContext ->
networkRule.enqueue(
host("api.stripe.com"),
method("GET"),
path("/v1/elements/sessions"),
) { response ->
response.testBodyFromFile("elements-sessions-requires_payment_method.json")
}

testContext.presentPaymentSheet {
presentWithPaymentIntent(
paymentIntentClientSecret = "pi_example_secret_example",
configuration = defaultConfiguration,
)
}

page.waitForText("Card number")

page.replaceText("Card number", "4242424242424244")
composeTestRule.onNodeWithText("Card number")
.assertHasErrorMessage("Your card's number is invalid.")

page.fillExpirationDate("11/11")
composeTestRule.onNode(hasContentDescription(value = "Expiration date", substring = true))
.assertHasErrorMessage("Your card's expiration year is invalid.")

testContext.markTestSucceeded()
}
}
2 changes: 1 addition & 1 deletion ...pe-ui-core/src/main/java/com/stripe/android/uicore/elements/AddressTextFieldController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class AddressTextFieldController(
private val config: TextFieldConfig,
private val onNavigation: (() -> Unit)? = null,
override val initialValue: String? = null
) : TextFieldController, InputController, SectionFieldErrorController, SectionFieldComposable {
) : TextFieldController, InputController, SectionFieldComposable {

init {
initialValue?.let { onRawValueChange(it) }
Expand Down
4 changes: 2 additions & 2 deletions stripe-ui-core/src/main/java/com/stripe/android/uicore/elements/TextFieldController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import kotlinx.coroutines.flow.asStateFlow

@OptIn(ExperimentalComposeUiApi::class)
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
interface TextFieldController : InputController, SectionFieldComposable {
interface TextFieldController : InputController, SectionFieldComposable, SectionFieldErrorController {
fun onValueChange(displayFormatted: String): TextFieldState?
fun onFocusChange(newHasFocus: Boolean)
fun onDropdownItemClicked(item: TextFieldIcon.Dropdown.Item) {}
Expand Down Expand Up @@ -131,7 +131,7 @@ class SimpleTextFieldController(
private val overrideContentDescriptionProvider: ((fieldValue: String) -> ResolvableString)? = null,
private val shouldAnnounceLabel: Boolean = true,
private val shouldAnnounceFieldValue: Boolean = true
) : TextFieldController, SectionFieldErrorController {
) : TextFieldController {
override val trailingIcon: StateFlow<TextFieldIcon?> = textFieldConfig.trailingIcon
override val capitalization: KeyboardCapitalization = textFieldConfig.capitalization
override val keyboardType: KeyboardType = textFieldConfig.keyboard
Expand Down
13 changes: 13 additions & 0 deletions stripe-ui-core/src/main/java/com/stripe/android/uicore/elements/TextFieldUI.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,16 @@ fun TextField(
val fieldState by textFieldController.fieldState.collectAsState()
val label by textFieldController.label.collectAsState()

val error by textFieldController.error.collectAsState()
val sectionErrorString = error?.let {
it.formatArgs?.let { args ->
stringResource(
it.errorMessage,
*args
)
} ?: stringResource(it.errorMessage)
}

LaunchedEffect(fieldState) {
// When field is in focus and full, move to next field so the user can keep typing
if (fieldState == TextFieldStateConstants.Valid.Full && hasFocus.value) {
Expand Down Expand Up @@ -229,6 +239,7 @@ fun TextField(
placeholder = placeHolder,
trailingIcon = trailingIcon,
shouldShowError = shouldShowError,
errorString = sectionErrorString,
visualTransformation = visualTransformation,
layoutDirection = textFieldController.layoutDirection,
keyboardOptions = KeyboardOptions(
Expand Down Expand Up @@ -257,6 +268,7 @@ internal fun TextFieldUi(
trailingIcon: TextFieldIcon?,
showOptionalLabel: Boolean,
shouldShowError: Boolean,
errorString: String?,
shouldAnnounceLabel: Boolean = true,
modifier: Modifier = Modifier,
visualTransformation: VisualTransformation = VisualTransformation.None,
Expand Down Expand Up @@ -302,6 +314,7 @@ internal fun TextFieldUi(
}
},
isError = shouldShowError,
errorString = errorString,
visualTransformation = visualTransformation,
keyboardOptions = keyboardOptions,
keyboardActions = keyboardActions,
Expand Down
19 changes: 15 additions & 4 deletions stripe-ui-core/src/main/java/com/stripe/android/uicore/elements/compat/CompatTextField.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.composed
import androidx.compose.ui.draw.alpha
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.Shape
Expand Down Expand Up @@ -139,6 +140,7 @@ internal fun CompatTextField(
leadingIcon: @Composable (() -> Unit)? = null,
trailingIcon: @Composable (() -> Unit)? = null,
isError: Boolean = false,
errorString: String?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that typically we call this errorMessage or just error, the String part is implied from the type. Can you update the name here and elsewhere?

Also, on this one can you update the javadoc? We have javadoc for all the other fields so it'd be nice to keep that up

visualTransformation: VisualTransformation = VisualTransformation.None,
keyboardOptions: KeyboardOptions = KeyboardOptions.Default,
keyboardActions: KeyboardActions = KeyboardActions(),
Expand All @@ -160,7 +162,7 @@ internal fun CompatTextField(
value = value,
modifier = modifier
.indicatorLine(enabled, isError, interactionSource, colors)
.defaultErrorSemantics(isError, stringResource(ComposeUiR.string.default_error_message))
.errorSemanticsWithDefault(isError, errorString)
.defaultMinSize(
minWidth = TextFieldDefaults.MinWidth,
minHeight = TextFieldDefaults.MinHeight
Expand Down Expand Up @@ -447,10 +449,19 @@ private object TextFieldTransitionScope {
}
}

private fun Modifier.defaultErrorSemantics(
private fun Modifier.errorSemanticsWithDefault(
isError: Boolean,
defaultErrorMessage: String,
): Modifier = if (isError) semantics { error(defaultErrorMessage) } else this
errorMessage: String?,
): Modifier = composed {
val defaultErrorMessage = stringResource(ComposeUiR.string.default_error_message)
if (isError) {
semantics {
error(errorMessage ?: defaultErrorMessage)
}
} else {
this
}
}

private const val PlaceholderAnimationDuration = 83
private const val PlaceholderAnimationDelayOrDuration = 67
11 changes: 11 additions & 0 deletions stripe-ui-core/src/test/java/com/stripe/android/uicore/elements/TextFieldUiScreenshotTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = null
)
Expand All @@ -50,6 +51,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = null
)
Expand All @@ -66,6 +68,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = true,
errorString = null,
showOptionalLabel = false,
trailingIcon = null
)
Expand All @@ -82,6 +85,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = true,
trailingIcon = null
)
Expand All @@ -98,6 +102,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = TextFieldIcon.Trailing(
idRes = R.drawable.stripe_ic_search,
Expand All @@ -117,6 +122,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = TextFieldIcon.Dropdown(
title = "Select an option".resolvableString,
Expand Down Expand Up @@ -148,6 +154,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = TextFieldIcon.Dropdown(
title = "Select an option".resolvableString,
Expand Down Expand Up @@ -179,6 +186,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = null
)
Expand All @@ -195,6 +203,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = null,
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = null
)
Expand All @@ -211,6 +220,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = "Search for someone...",
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = null
)
Expand All @@ -227,6 +237,7 @@ class TextFieldUiScreenshotTest {
loading = false,
placeholder = "Search for someone...",
shouldShowError = false,
errorString = null,
showOptionalLabel = false,
trailingIcon = null
)
Expand Down
Loading