-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix: The displayed error messages are not announced once they appear. #84516
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
Changes from all commits
d64ba83
79406ee
1a3ab10
abba163
2c83efa
b33d0f5
0204da0
6113c10
93b8030
6b7eb9b
179e514
e866e28
2ecebdd
5652ba6
6b89978
dcb8dcc
ca0f78f
f5b9723
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React, {useImperativeHandle, useRef} from 'react'; | ||
| import React, {useContext, useImperativeHandle, useRef} from 'react'; | ||
| import type {ForwardedRef, RefObject} from 'react'; | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import type {ScrollView as RNScrollView, StyleProp, ViewStyle} from 'react-native'; | ||
|
|
@@ -7,17 +7,20 @@ import FormAlertWithSubmitButton from '@components/FormAlertWithSubmitButton'; | |
| import FormElement from '@components/FormElement'; | ||
| import ScrollView from '@components/ScrollView'; | ||
| import ScrollViewWithContext from '@components/ScrollViewWithContext'; | ||
| import Text from '@components/Text'; | ||
| import useBottomSafeSafeAreaPaddingStyle from '@hooks/useBottomSafeSafeAreaPaddingStyle'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
| import useSafeAreaPaddings from '@hooks/useSafeAreaPaddings'; | ||
| import useThemeStyles from '@hooks/useThemeStyles'; | ||
| import Accessibility from '@libs/Accessibility'; | ||
| import {getLatestErrorMessage} from '@libs/ErrorUtils'; | ||
| import getPlatform from '@libs/getPlatform'; | ||
| import CONST from '@src/CONST'; | ||
| import type {OnyxFormKey} from '@src/ONYXKEYS'; | ||
| import type {Form} from '@src/types/form'; | ||
| import type ChildrenProps from '@src/types/utils/ChildrenProps'; | ||
| import {isEmptyObject} from '@src/types/utils/EmptyObject'; | ||
| import FormContext from './FormContext'; | ||
| import type {FormInputErrors, FormProps, FormWrapperRef, InputRefs} from './types'; | ||
|
|
||
| type FormWrapperProps = ChildrenProps & | ||
|
|
@@ -106,6 +109,10 @@ function FormWrapper({ | |
| const styles = useThemeStyles(); | ||
| const formRef = useRef<RNScrollView>(null); | ||
| const formContentRef = useRef<View>(null); | ||
| const {getErrorAnnouncementKey, getFallbackAnnouncementMessage} = useContext(FormContext); | ||
| const errorAnnouncementKey = getErrorAnnouncementKey(); | ||
| const fallbackAnnouncementMessage = getFallbackAnnouncementMessage(); | ||
| const isWeb = getPlatform() === CONST.PLATFORM.WEB; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is against our cross platform philosophy but we'll fix as followup by deprecating hidden Text in favor of web hook implementation. |
||
|
|
||
| const [formState] = useOnyx<OnyxFormKey, Form>(`${formID}`); | ||
|
|
||
|
|
@@ -224,6 +231,15 @@ function FormWrapper({ | |
| }} | ||
| > | ||
| {children} | ||
| {isWeb && !!fallbackAnnouncementMessage && errorAnnouncementKey > 1 && ( | ||
| <Text | ||
| key={`fallback-announce-${errorAnnouncementKey}`} | ||
| style={styles.hiddenElementOutsideOfWindow} | ||
| role={CONST.ROLE.ALERT} | ||
| > | ||
| {fallbackAnnouncementMessage} | ||
| </Text> | ||
| )} | ||
| {!shouldSubmitButtonStickToBottom && SubmitButton} | ||
| </FormElement> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -50,6 +50,9 @@ function FormAlertWrapper({ | |||||||||||||||||||||
| const {translate} = useLocalize(); | ||||||||||||||||||||||
| const {isOffline} = useNetwork(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const defaultFixErrorsMessage = `${translate('common.please')} ${translate('common.fixTheErrors')} ${translate('common.inTheFormBeforeContinuing')}.`; | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is against our translation guideline Let's not block on this since it's existing on main but should be migrated as followup. App/src/components/FormAlertWrapper.tsx Lines 59 to 68 in 5652ba6
|
||||||||||||||||||||||
| const announcementMessage = message?.length ? message : defaultFixErrorsMessage; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| let content; | ||||||||||||||||||||||
| if (!message?.length) { | ||||||||||||||||||||||
| content = ( | ||||||||||||||||||||||
|
|
@@ -72,8 +75,10 @@ function FormAlertWrapper({ | |||||||||||||||||||||
| <View style={containerStyles}> | ||||||||||||||||||||||
| {isAlertVisible && ( | ||||||||||||||||||||||
| <FormHelpMessage | ||||||||||||||||||||||
| message={message} | ||||||||||||||||||||||
| message={announcementMessage} | ||||||||||||||||||||||
| shouldRenderMessageAsHTML={isMessageHtml} | ||||||||||||||||||||||
| style={[styles.mb3, errorMessageStyle]} | ||||||||||||||||||||||
| shouldReannounceOnSubmit | ||||||||||||||||||||||
| > | ||||||||||||||||||||||
| {content} | ||||||||||||||||||||||
| </FormHelpMessage> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import isEmpty from 'lodash/isEmpty'; | ||
| import React, {useMemo} from 'react'; | ||
| import React, {useContext, useMemo} from 'react'; | ||
| import type {StyleProp, ViewStyle} from 'react-native'; | ||
| import {View} from 'react-native'; | ||
| import useAccessibilityAnnouncement from '@hooks/useAccessibilityAnnouncement'; | ||
|
|
@@ -9,6 +9,7 @@ import useThemeStyles from '@hooks/useThemeStyles'; | |
| import getPlatform from '@libs/getPlatform'; | ||
| import Parser from '@libs/Parser'; | ||
| import CONST from '@src/CONST'; | ||
| import FormContext from './Form/FormContext'; | ||
| import Icon from './Icon'; | ||
| import RenderHTML from './RenderHTML'; | ||
| import Text from './Text'; | ||
|
|
@@ -37,6 +38,11 @@ type FormHelpMessageProps = { | |
|
|
||
| /** Native ID for accessibility association (aria-describedby) */ | ||
| nativeID?: string; | ||
|
|
||
| /** Whether this message should be re-announced on repeated form submissions. | ||
| * All error messages announce on first appearance. This prop controls whether | ||
| * the message also re-announces when the form is re-submitted with the same errors. */ | ||
| shouldReannounceOnSubmit?: boolean; | ||
| }; | ||
|
|
||
| function FormHelpMessage({ | ||
|
|
@@ -48,16 +54,41 @@ function FormHelpMessage({ | |
| shouldRenderMessageAsHTML = false, | ||
| isInfo = false, | ||
| nativeID, | ||
| shouldReannounceOnSubmit = false, | ||
| }: FormHelpMessageProps) { | ||
| const theme = useTheme(); | ||
| const styles = useThemeStyles(); | ||
| const icons = useMemoizedLazyExpensifyIcons(['DotIndicator', 'Exclamation']); | ||
| const {getErrorAnnouncementKey} = useContext(FormContext); | ||
| const errorAnnouncementKey = getErrorAnnouncementKey(); | ||
|
|
||
| const isWeb = getPlatform() === CONST.PLATFORM.WEB; | ||
| const shouldAnnounceError = isError && typeof message === 'string' && !!message && !shouldRenderMessageAsHTML && children == null; | ||
| const shouldUseSeparateWebLiveAnnouncement = isWeb && !!nativeID && shouldAnnounceError; | ||
| const visibleMessageRole = shouldUseSeparateWebLiveAnnouncement || !shouldAnnounceError ? undefined : CONST.ROLE.ALERT; | ||
| const visibleMessageLiveRegion = shouldUseSeparateWebLiveAnnouncement || !shouldAnnounceError ? undefined : 'assertive'; | ||
|
|
||
| const errorAnnouncementText = useMemo(() => { | ||
| if (!isError || typeof message !== 'string') { | ||
| return ''; | ||
| } | ||
| const trimmedMessage = message.trim(); | ||
| if (!trimmedMessage) { | ||
| return ''; | ||
| } | ||
| return shouldRenderMessageAsHTML ? Parser.htmlToText(trimmedMessage) : trimmedMessage; | ||
| }, [isError, message, shouldRenderMessageAsHTML]); | ||
|
|
||
| const hasError = errorAnnouncementText.length > 0; | ||
|
|
||
| useAccessibilityAnnouncement(message, shouldAnnounceError); | ||
|
|
||
| // Re-announce on native via AccessibilityInfo when form is re-submitted | ||
| useAccessibilityAnnouncement(errorAnnouncementText, hasError && shouldReannounceOnSubmit && errorAnnouncementKey > 0, { | ||
| shouldAnnounceOnNative: true, | ||
| announcementKey: errorAnnouncementKey, | ||
| }); | ||
|
|
||
| const HTMLMessage = useMemo(() => { | ||
| if (typeof message !== 'string' || !shouldRenderMessageAsHTML) { | ||
| return ''; | ||
|
|
@@ -72,8 +103,6 @@ function FormHelpMessage({ | |
| return `<muted-text-label>${replacedText}</muted-text-label>`; | ||
| }, [isError, message, shouldRenderMessageAsHTML]); | ||
|
|
||
| useAccessibilityAnnouncement(message, shouldAnnounceError); | ||
|
|
||
| const errorIconLabel = isError && shouldShowRedDotIndicator ? CONST.ACCESSIBILITY_LABELS.ERROR : undefined; | ||
|
|
||
| if (isEmpty(message) && isEmpty(children)) { | ||
|
|
@@ -111,17 +140,13 @@ function FormHelpMessage({ | |
| style={[isError ? styles.formError : styles.formHelp, styles.mb0]} | ||
| nativeID={nativeID} | ||
| role={visibleMessageRole} | ||
| // TalkBack on some Android versions skips role-only alert announcements, | ||
| // so keep native accessibilityRole/live-region as a platform fallback. | ||
| accessibilityRole={!isWeb && shouldAnnounceError ? CONST.ROLE.ALERT : undefined} | ||
| accessibilityLiveRegion={visibleMessageLiveRegion} | ||
| > | ||
| {message} | ||
| </Text> | ||
| ))} | ||
| {shouldUseSeparateWebLiveAnnouncement && ( | ||
| // Keep a separate live region for immediate web announcements without | ||
| // changing the visible described text Safari relies on when refocusing inputs. | ||
| <Text | ||
| style={styles.hiddenElementOutsideOfWindow} | ||
| role={CONST.ROLE.ALERT} | ||
|
|
@@ -130,6 +155,15 @@ function FormHelpMessage({ | |
| {message} | ||
| </Text> | ||
| )} | ||
| {isWeb && shouldReannounceOnSubmit && hasError && errorAnnouncementKey > 0 && ( | ||
| <Text | ||
| key={`reannounce-${errorAnnouncementKey}`} | ||
| style={styles.hiddenElementOutsideOfWindow} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same deprecation concern here. Btw why did we need hidden Text in both places? here in FormHelpMessage and in FormWrapper.
FormWrapper > FormAlertWithSubmitButton > FormAlertWrapper > FormHelpMessage
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The web hook can't replace these because it doesn't support
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why can't introduce
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I’ve already spent a significant amount of time on this PR and gone beyond the original scope. I implemented a re-announcement feature, which turned out to be much more complex than the expected behavior. Introducing @rushatgabhane Could you please check if it’s possible to increase the bounty for this, given that the work has gone beyond the original scope? I can take care of updating the hook if that works.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
an expensify employee would decide that. but yeah if the difficulty is more than expected, you can ask for more and give an explanation when payment is due |
||
| role={CONST.ROLE.ALERT} | ||
| > | ||
| {errorAnnouncementText} | ||
| </Text> | ||
| )} | ||
| </View> | ||
| </View> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| type UseAccessibilityAnnouncementOptions = { | ||
| shouldAnnounceOnNative?: boolean; | ||
| shouldAnnounceOnWeb?: boolean; | ||
| announcementKey?: number; | ||
| }; | ||
|
|
||
| export default UseAccessibilityAnnouncementOptions; |
Uh oh!
There was an error while loading. Please reload this page.