Forms: Fix forms VoiceOver submission #46707
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Feature] Contact Form
[Package] Forms
[Plugin] Jetpack
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
There was a problem hiding this comment.
Pull request overview
This PR improves accessibility for Jetpack Forms by implementing VoiceOver screen reader announcements when forms are submitted successfully or encounter errors. The changes ensure that users relying on assistive technology receive immediate feedback about form submission outcomes.
Changes:
- Added focus management to announce success messages via the success heading element
- Added callback functions to focus on validation and submission error messages for screen reader announcement
- Enhanced error message containers with proper ARIA attributes (role="alert", tabindex="-1", aria-label)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| projects/plugins/jetpack/changelog/fix-forms-voiceover-submission | Changelog entry for the Jetpack plugin |
| projects/packages/forms/changelog/fix-forms-voiceover-submission | Changelog entry for the Forms package |
| projects/packages/forms/src/modules/form/view.js | Added three callback functions to manage focus for screen reader announcements |
| projects/packages/forms/src/contact-form/class-contact-form.php | Enhanced error and success message HTML with ARIA attributes and unique IDs |
| // Move focus to the success heading for screen reader announcement. | ||
| if ( context.submissionSuccess && ! context.hasClickedBack ) { | ||
| const successHeading = document.getElementById( | ||
| `contact-form-success-header-${ context.formHash }` | ||
| ); | ||
| successHeading?.focus(); | ||
| } |
There was a problem hiding this comment.
The focus management logic added to scrollToWrapper lacks test coverage. Since this file has a corresponding test file at tests/js/modules/form/view.test.js, consider adding tests to verify that the success heading receives focus when submissionSuccess is true and hasClickedBack is false.
projects/packages/forms/changelog/fix-forms-voiceover-submission
Outdated
Show resolved
Hide resolved
| Significance: patch | ||
| Type: fixed | ||
|
|
||
| Announce form submision and errors to VoiceOver |
There was a problem hiding this comment.
The changelog entry should end with a period. According to the project's changelog guidelines, entries should be grammatically correct and end with a period.
| Significance: patch | ||
| Type: bugfix | ||
|
|
||
| Forms: improve VoiceOver for form submissions. |
There was a problem hiding this comment.
The changelog entry should end with a period. According to the project's changelog guidelines, entries should be grammatically correct and end with a period.
| focusOnValidationError() { | ||
| if ( state.showFormErrors ) { | ||
| const { ref } = getElement(); | ||
| ref?.focus(); | ||
| } | ||
| }, | ||
|
|
||
| focusOnSubmissionError() { | ||
| if ( state.showSubmissionError ) { | ||
| const { ref } = getElement(); | ||
| ref?.focus(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The new callback functions focusOnValidationError and focusOnSubmissionError lack test coverage. Since this file has a corresponding test file at tests/js/modules/form/view.test.js, consider adding tests to verify that these callbacks correctly focus elements when the respective error states are true.
Code Coverage SummaryCoverage changed in 3 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
projects/packages/forms/src/contact-form/class-contact-form.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/contact-form/class-contact-form.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| Significance: patch | ||
| Type: fixed | ||
|
|
||
| Announce form submission and errors to VoiceOver |
There was a problem hiding this comment.
The changelog entry should end with a period according to the project's changelog guidelines.
projects/packages/forms/src/contact-form/class-contact-form.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The validation error message was using both role="alert" (which triggers automatic screen reader announcement) and programmatic focus (which also triggers announcement). This caused VoiceOver to read the error twice. Remove role="alert" and rely on focus alone to announce the error message, providing a single clear announcement when validation fails.
The success heading was being focused directly, causing VoiceOver to announce "heading level 4" instead of the content. The role="alert" wasn't working because the element was hidden with display:none initially - alerts only fire when content changes in a visible element. Fix by focusing the wrapper div instead, with aria-labelledby pointing to the heading. This causes VoiceOver to read the heading content without the unhelpful "heading level 4" announcement, while preserving the semantic heading structure for document outline.
The visually-hidden "Warning" text before each field error message doesn't add useful information for screen reader users - the error message itself is sufficient context. Also add aria-hidden="true" to the warning icon for consistency with the form-level error styling.
Replace aria-errormessage with aria-describedby for field error associations. While aria-errormessage is semantically correct for error messages, it has inconsistent support across screen readers, particularly in Safari/VoiceOver. aria-describedby has broad support and ensures error messages are announced when users focus invalid fields.
enejb
added
[Status] Needs Review
projects/packages/forms/src/contact-form/class-contact-form-field.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/contact-form/class-contact-form-field.php
Outdated
Show resolved
Hide resolved
| focusOnSubmissionError() { | ||
| if ( state.showSubmissionError ) { | ||
| const { ref } = getElement(); | ||
| ref?.focus(); |
There was a problem hiding this comment.
focusOnSubmissionError is triggered via data-wp-watch while state.showSubmissionError may stay true until the timeout clears it. If the watch callback runs on subsequent updates, it can repeatedly steal focus back to the submission error element, disrupting attempts to interact with the form (e.g., correcting fields and resubmitting). Consider focusing only once when the submission error first appears (e.g., gate on a context flag / previous value).
| ref?.focus(); | |
| if ( ! ref || ref.__hasFocusedOnSubmissionError ) { | |
| return; | |
| } | |
| ref.__hasFocusedOnSubmissionError = true; | |
| ref.focus(); |
| data-wp-bind--aria-invalid='state.fieldAriaInvalid' | ||
| data-wp-bind--value='state.getFieldValue' | ||
| aria-errormessage='" . esc_attr( $id ) . '-' . esc_attr( $type ) . "-error-message' | ||
| aria-describedby='" . esc_attr( $id ) . '-' . esc_attr( $type ) . "-error-message' | ||
| data-wp-on--input='actions.onFieldChange' |
There was a problem hiding this comment.
aria-describedby is built from the input $type (e.g., ...-tel-error-message). For telephone fields in outlined/animated styles, the inline error markup is suppressed and the error div is rendered later using the field type (e.g., ...-telephone-error-message), which can leave aria-describedby pointing at a non-existent ID. This breaks the input → error message association for assistive tech. Consider decoupling the HTML input type from the error-id suffix (so telephone fields can render type="tel" but still reference/render the ...-telephone-error-message ID consistently).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| get fieldAriaInvalid() { | ||
| // Return 'true' for invalid fields, null to remove the attribute entirely. | ||
| // Using null instead of false prevents VoiceOver from announcing "invalid" for valid fields. | ||
| return state.fieldHasErrors ? 'true' : null; | ||
| }, |
There was a problem hiding this comment.
This change introduces new accessibility-critical state/callback logic (fieldAriaInvalid, success aria-hidden toggling, and new focus callbacks), but the existing Jest coverage for this module focuses on helper functions only (projects/packages/forms/tests/js/modules/form/view.test.js). Adding unit tests for these new behaviors (at least: fieldAriaInvalid returning 'true' vs removing the attribute, and the one-time focus gating) would help prevent regressions across screen readers and keyboard navigation.
|
@claude Please review |
|
Claude finished @enejb's task in 1m 54s —— View job Code Review Complete
SummaryThis PR improves VoiceOver accessibility for Jetpack Forms by announcing success and error messages after form submission. The implementation uses focus management to trigger screen reader announcements. While the approach works functionally, there are several accessibility and code quality concerns that should be addressed. Critical Issues1. Focus Management Pattern May Trap Keyboard Users 🔴Location: The focusOnValidationError() {
if ( state.showFormErrors ) {
const { ref } = getElement();
ref?.focus(); // This can run on every reactive update!
}
},Problem: If Recommendation: Implement one-time focus logic by tracking whether focus has already been moved for the current error state:
This is particularly important for the validation error since users need to interact with form fields while the error state is active. 2. Inconsistent aria-describedby References 🔴Location: The telephone field's aria-describedby="<?php echo esc_attr( $id ); ?>-telephone-error-message"
// But the actual error div has id: {$id}-phone-error-messageImpact: Breaks the ARIA relationship between input and error message for screen readers. Note: I see copilot-swe-agent opened PR #46751 to address this - that's great! However, there's a broader issue here about the 3. Missing Test Coverage for New Accessibility Features 🟡Location: The test file only covers helper functions (
Recommendation: Add tests for these features to prevent regressions: describe('Accessibility features', () => {
test('fieldAriaInvalid returns "true" for invalid fields', () => { ... });
test('fieldAriaInvalid returns null for valid fields', () => { ... });
test('success message is hidden when not successful', () => { ... });
});Quality Issues4. aria-invalid Pattern Inconsistency 🟡The change from The computed property get fieldAriaInvalid() {
// Return 'true' for invalid fields, null to remove the attribute entirely.
// Using null instead of false prevents VoiceOver from announcing "invalid" for valid fields.
return state.fieldHasErrors ? 'true' : null;
},Question: Does binding to 5. Removed "Warning" Screen Reader Text 🟡Location: The PR removes the visually-hidden "Warning" text from error icons: // Before:
<span class="visually-hidden">Warning</span>
// After:
// Removed entirelyThe icon is now marked Question: Is the error message itself clear enough without the "Warning" prefix? For validation errors, this is probably fine since the message is descriptive (e.g., "This field is required"). For submission errors, verify the message text provides sufficient context. 6. Changelog Entry Missing Period 🟡Location: According to the Jetpack changelog guidelines, entries should end with a period: This should be: Wait, it already has a period! The Copilot review was incorrect on this point. ✅ The Jetpack plugin changelog is also correct: Positive Aspects ✅
Recommendations SummaryMust Fix:
Should Fix: Consider: Testing NotesThe PR description says to test with VoiceOver on Mac, which is appropriate. Additional testing recommendations:
Overall, this PR makes meaningful accessibility improvements, but the focus management pattern needs refinement to avoid trapping keyboard users. The telephone field ARIA issue should be resolved in the follow-up PR. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| focusOnValidationError() { | ||
| const context = getContext(); | ||
|
|
||
| if ( state.showFormErrors ) { | ||
| // Only move focus once per error episode to avoid trapping keyboard users. |
There was a problem hiding this comment.
New accessibility behavior is introduced here (aria-invalid/aria-hidden value normalization and focus management via focusOnValidationError/focusOnSubmissionError), but there are no unit tests covering these code paths. Since this package already has Jest tests for interactivity modules, please add tests that mock @wordpress/interactivity (getContext/getElement) to assert the new getters and focus callbacks behave as expected.
Fixes FORMS-550
Screen.Recording.2026-01-23.at.10.51.59.AM.mov
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Visit a form.
Turn on VoiceOver on your mac.
Notice that when you submit the form that you hear that the form was submitted.
Notice that if you have any error while submitting the form that you also hear about them.