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.

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

feat(Form fields): improve accesibility of errors #1246

Merged
merged 13 commits into from
Sep 26, 2024

Conversation

atabel
Copy link
Collaborator

@atabel atabel commented Sep 18, 2024

WEB-1958

  • Added an icon to error messages (this makes the error message more accesible over inverse and for color-blind users)
  • Added an aria-live region to announce all the fields with error when the form is submitted and there is more than one field with error
  • Make sure all mandatory form fields are validated
  • Form fields are validated onBlur. Previously, we only validated fields with a special format, like email.
  • Fixed an error making VoiceOver read fields label twice

Copy link

github-actions bot commented Sep 18, 2024

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

Copy link

github-actions bot commented Sep 18, 2024

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-5rc2kpewp-flows-projects-65bb050e.vercel.app

Built with commit 4e07e1a.
This pull request is being automatically deployed with vercel-action

Copy link

github-actions bot commented Sep 18, 2024

Size stats

master this branch diff
Total JS 12.2 MB 12.2 MB +1.65 kB
JS without icons 2.01 MB 2.01 MB +1.65 kB
Lib overhead 68.2 kB 68.2 kB 0 B
Lib overhead (gzip) 16.6 kB 16.6 kB 0 B

Copy link

github-actions bot commented Sep 18, 2024

Screenshot tests report

✔️ All passing

Comment on lines -115 to -117
if (!rawValue) {
return optional ? '' : texts.formFieldErrorIsMandatory || t(tokens.formFieldErrorIsMandatory);
}
Copy link
Collaborator Author

@atabel atabel Sep 19, 2024

Choose a reason for hiding this comment

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

I've found this code repeated in most input fields, I've moved it inside useFieldProps

onBlur?.(e);
},
inputRef: (input: HTMLInputElement | null) => register(name, {input, validator: validate}),
inputRef: (input: HTMLInputElement | null) => register(name, {input, validator: validate, label}),
Copy link
Collaborator Author

@atabel atabel Sep 19, 2024

Choose a reason for hiding this comment

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

I need the field label to be in the registrations map, to be able to use it when rendering the global form error: "Revisa los errores en los siguientes campos: nombre, email". See changes in form.tsx

Comment on lines +339 to +350
{label && (
<Label
style={labelStyle}
error={error}
forId={id}
inputState={inputState}
shrinkLabel={shrinkLabel}
optional={!rest.required}
>
{label}
</Label>
)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the label before the input. This avoids a bug (I think it's a bug) in Safari/VoiceOver which made the label being read twice by screen reader (when focusing the input, and then as an independent element). This doesn't happen if the label appears before the input in the html.

@@ -42,7 +42,6 @@ const TooltipContent = ({acceptedCards}: {acceptedCards: CardOptions}) => {
<Text2>
{texts.formCreditCardCvvTooltipAmex || t(tokens.formCreditCardCvvTooltipAmex)}
</Text2>
)
Copy link
Collaborator Author

@atabel atabel Sep 19, 2024

Choose a reason for hiding this comment

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

image

@atabel atabel marked this pull request as ready for review September 19, 2024 12:57
src/text-field-components.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

in desktop, the text is not aligned in the same way as mobile, @aweell is this expected?

is it possible to align the text line to the middle of the icon? (compare with mobile screenshot to see the difference)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Not expected, but also not correctly documented in the specs

the size of the icon could be 1lh to match the line height in mobile (16) but also in desktop (20) so even if the font-size increases appears aligned to the first line of text? this approach seems correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed size to 1lh (fallback to 1rem for old browsers not supporting lh unit)

Copy link
Contributor

@aweell aweell left a comment

Choose a reason for hiding this comment

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

LGTM!

@atabel atabel added this pull request to the merge queue Sep 26, 2024
Merged via the queue into master with commit e35a99e Sep 26, 2024
11 checks passed
@atabel atabel deleted the atoledano-form-errors-a11y branch September 26, 2024 16:35
tuentisre pushed a commit that referenced this pull request Sep 27, 2024
# [16.1.0](v16.0.0...v16.1.0) (2024-09-27)

### Features

* **BrandLoadingScreen:** improve lottie animations ([#1235](#1235)) ([e7dc87f](e7dc87f)), closes [/github.com/airbnb/lottie-web/issues/1184#issuecomment-411586909](https://github.com//github.com/airbnb/lottie-web/issues/1184/issues/issuecomment-411586909)
* **Form fields:** allow blocking copy/cut ([#1251](#1251)) ([8fd2838](8fd2838))
* **Form fields:** improve accesibility of errors ([#1246](#1246)) ([e35a99e](e35a99e))
* **PhoneNumberField:** Custom formatter support + lazy load libphonenumber on demand ([#1244](#1244)) ([2ee88e9](2ee88e9))
* **PosterCard, DisplayMediaCard:** allow using srcSet for backgroundImage ([#1253](#1253)) ([3b3d85f](3b3d85f))
* **Rating, InfoRating:** new components ([#1196](#1196)) ([02c91f6](02c91f6))
* **SearchField, TextField:** support inputMode prop ([#1249](#1249)) ([fe31eca](fe31eca))
* **Sheet:** lazy load sheet implementations ([#1250](#1250)) ([40fecdd](40fecdd))
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 16.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants