fix: The displayed error messages are not announced once they appear.#84516
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@rushatgabhane @dannymcclain, I'm a bit confused about the expected behaviour? Should the error message be announced every time we click the submit the form? Currently the error message is announced when it appears for the first time. This PRfix.mp4MainMonosnap.screencast.2026-03-09.18-05-50.mp4 |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@MelvinBot review |
Code ReviewPR Summary: This PR fixes WCAG 4.1.3 compliance by ensuring form validation error messages are announced by screen readers. It introduces a two-phase DOM update pattern (unmount → 100ms delay → remount) for a hidden Overall: The approach is sound. The two-phase DOM update is a well-known technique for forcing screen reader re-announcements, and the separation of the visual error message from the hidden announcement element is clean. The Issue 1: iOS native hook won't re-announce on re-submitThe
This means the re-announcement behavior added by this PR only works on web/Android via the DOM-based alert element, but not on iOS native where Suggestion: Pass function useFormHelpMessageAccessibilityAnnouncement(
message: string | ReactNode,
shouldAnnounceError: boolean,
errorAnnouncementKey: number,
) {
// ...
useEffect(() => {
// ... existing logic, but remove the previousAnnouncedMessageRef dedup
// since errorAnnouncementKey already controls re-trigger timing
}, [message, shouldAnnounceError, errorAnnouncementKey]);
}And at line 98 of useFormHelpMessageAccessibilityAnnouncement(errorAnnouncementText, shouldAnnounceError, errorAnnouncementKey);This also fixes a secondary issue: the hook currently receives Issue 2: Minor —
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Will add the recordings for other platforms tomorrow: Monosnap.screencast.2026-03-13.23-11-36.mp4 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…4866 Made-with: Cursor # Conflicts: # src/components/FormHelpMessage.tsx
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
08d3e8a to
b33d0f5
Compare
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@MelvinBot the current implementation works well with NVDA and JAWS but isn't working with VoiceOver. The error message is not announced when they appear for the first time when the input is blurred. Pressing submit button correctly announces the error message. Please investigate VoiceOver issue. |
VoiceOver Investigation: Error Messages Not Announced on BlurSummaryVoiceOver on macOS doesn't announce input-level error messages when they first appear on blur. This is caused by two interacting issues: (1) the hidden Root Cause AnalysisOn blur — the code path that doesn't work:
On submit — the code path that works:
Why VoiceOver misses the blur announcement: According to accessibility research and MDN documentation, VoiceOver + Safari has documented issues with dynamically injected live regions. The most reliable pattern is:
The current code conditionally renders the hidden alert element, which means it appears in the DOM simultaneously with its text content. VoiceOver may not treat this as a "content change" in a live region it was monitoring, because it wasn't monitoring anything (the element didn't exist). While Additionally, combining Recommended FixOption A: Always-in-DOM pattern (most reliable) Instead of conditionally rendering the hidden alert // In FormHelpMessage, instead of:
{shouldUseSeparateWebLiveAnnouncement && (
<Text role={CONST.ROLE.ALERT} style={styles.hiddenElementOutsideOfWindow}>
{message}
</Text>
)}
// Use:
<Text role={CONST.ROLE.ALERT} style={styles.hiddenElementOutsideOfWindow}>
{shouldUseSeparateWebLiveAnnouncement ? message : ''}
</Text>However, this requires Option B: Key-based fresh DOM node (simpler change) Use the same key-based pattern that makes submit work. Add a local announcement key in const [localAnnouncementKey, setLocalAnnouncementKey] = useState(0);
useEffect(() => {
if (shouldUseSeparateWebLiveAnnouncement && errorAnnouncementText) {
setLocalAnnouncementKey(prev => prev + 1);
}
}, [shouldUseSeparateWebLiveAnnouncement, errorAnnouncementText]);
{shouldUseSeparateWebLiveAnnouncement && localAnnouncementKey > 0 && (
<Text
key={`blur-announce-${localAnnouncementKey}`}
style={styles.hiddenElementOutsideOfWindow}
role={CONST.ROLE.ALERT}
>
{message}
</Text>
)}This forces a new DOM node each time an error appears, which reliably triggers Option C: Remove duplicate ARIA attributes Regardless of which option above is chosen, the hidden element at lines 148-154 should use either Additional Notes
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Please fix conflict |
…4866 Made-with: Cursor # Conflicts: # src/hooks/useAccessibilityAnnouncement/index.ios.ts # src/hooks/useAccessibilityAnnouncement/index.native.ts # src/hooks/useAccessibilityAnnouncement/index.ts
|
@aimane-chnaif conflicts resolved. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari
|
Code Review (Updated)The PR has been significantly reworked since my previous review. Both issues I flagged have been addressed. Here's my review of the current state. All CI checks pass. Summary of the approachThe PR adds three complementary announcement mechanisms:
The What looks good
Minor observations (non-blocking)
Overall this looks solid. The layered approach (web DOM-based + native |
| {isWeb && !!fallbackAnnouncementMessage && errorAnnouncementKey > 0 && ( | ||
| <Text | ||
| key={`fallback-announce-${errorAnnouncementKey}`} | ||
| style={styles.hiddenElementOutsideOfWindow} | ||
| role={CONST.ROLE.ALERT} | ||
| > | ||
| {fallbackAnnouncementMessage} | ||
| </Text> |
There was a problem hiding this comment.
As useAccessibilityAnnouncement hook supports web, can we deprecate hidden Text?
There was a problem hiding this comment.
The web hook doesn't support re-announcement -- it only announces once per message. The keyed hidden Text forces a fresh DOM node on each re-submit which reliably triggers re-announcement. Same as #84516 (comment)
| // eslint-disable-next-line react/no-array-index-key | ||
| key={index} | ||
| style={[StyleUtils.getDotIndicatorTextStyles(isErrorMessage), textStyles]} | ||
| role={isErrorMessage ? CONST.ROLE.ALERT : undefined} |
There was a problem hiding this comment.
Good catch, this is redundant since accessibilityRole already maps to role on web. Removed.
| {isWeb && shouldReannounceOnSubmit && hasError && errorAnnouncementKey > 0 && ( | ||
| <Text | ||
| key={`reannounce-${errorAnnouncementKey}`} | ||
| style={styles.hiddenElementOutsideOfWindow} |
There was a problem hiding this comment.
Same deprecation concern here.
Btw why did we need hidden Text in both places? here in FormHelpMessage and in FormWrapper.
FormWrapper contains FormHelpMessage as child.
FormWrapper > FormAlertWithSubmitButton > FormAlertWrapper > FormHelpMessage
There was a problem hiding this comment.
The web hook can't replace these because it doesn't support announcementKey for re-announcements.
- FormHelpMessage's is for re-announcing the general form alert ('Please fix the errors') on re-submit.
- FormWrapper's is a fallback for forms with only field-level errors (no general alert) -- it announces the first field error on submit.
There was a problem hiding this comment.
The web hook can't replace these because it doesn't support announcementKey for re-announcements.
Why can't introduce announcementKey like you did for native?
There was a problem hiding this comment.
Expected Result
When validation errors occur, the error message should be automatically announced by screen readers and user should receive immediate auditory feedback about the error and its location
- Focus or programmatic notification should help users understand what needs to be corrected
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 announcementKey would require reworking and re-testing everything again, especially since getting this to work consistently across JAWS, NVDA, and VoiceOver was already quite challenging.
@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.
There was a problem hiding this comment.
Could you please check if it’s possible to increase the bounty for this, given that the work has gone beyond the original scope
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
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| const {getErrorAnnouncementKey, getFallbackAnnouncementMessage} = useContext(FormContext); | ||
| const errorAnnouncementKey = getErrorAnnouncementKey(); | ||
| const fallbackAnnouncementMessage = getFallbackAnnouncementMessage(); | ||
| const isWeb = getPlatform() === CONST.PLATFORM.WEB; |
There was a problem hiding this comment.
This is against our cross platform philosophy but we'll fix as followup by deprecating hidden Text in favor of web hook implementation.
|
@Krishna2323 TS error. Seems unrelated though |
| return ''; | ||
| }, [errors]); | ||
|
|
||
| useAccessibilityAnnouncement(firstFieldErrorMessage, !isGeneralAlertVisible && !!firstFieldErrorMessage && errorAnnouncementKey > 0, { |
There was a problem hiding this comment.
Do you have test case where first field error message is announced?
I am seeing general error message for all the cases listed in QA Steps
There was a problem hiding this comment.
Nvm I found cases. i.e. task title input page
| const {translate} = useLocalize(); | ||
| const {isOffline} = useNetwork(); | ||
|
|
||
| const defaultFixErrorsMessage = `${translate('common.please')} ${translate('common.fixTheErrors')} ${translate('common.inTheFormBeforeContinuing')}.`; |
There was a problem hiding this comment.
This is against our translation guideline
https://github.com/Expensify/App/blob/main/contributingGuides/philosophies/INTERNATIONALIZATION.md#--string-concatenation-should-not-be-used-for-translations
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
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #74866 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5652ba6087
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Krishna2323 please merge main. looks like there is some unrelated failing test |
|
@Krishna2323 We have a conflict, could you resolve it? Thanks! |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
🚧 @mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.43-3 🚀
|
Explanation of Change
Fixed Issues
$ #74866
PROPOSAL:
Tests
Same as QA Steps
Verify that no errors appear in the JS console
Offline tests
QA Steps
Other Occurrences:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web_chrome.mp4
web_chrome_jaws.mp4
web_chrome_nvda.mp4