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

fix(a11y): indicate required form controls using an asterisk #1723

Merged
merged 5 commits into from May 7, 2024

Conversation

giamir
Copy link
Contributor

@giamir giamir commented Apr 29, 2024

STACKS-582

@CGuindon @dancormier Shall we have a section where we document the new class a bit more formally like we do for validations classes?

Screenshot 2024-04-29 at 17 02 23

@dancormier I would like to get your feedback on the class naming s-required-symbol and location label.less.
I did not prefix it with s-label since the class could be used elsewhere (e.g. in the legend explaining the abbreviation)

TODOs:

  • Make a decision about the name of the class (currently s-required-symbol)
  • Add visual and a11y test variant
  • Test screen reader behavior with NVDA in Windows

Copy link

netlify bot commented Apr 29, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit ba4a5ee
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/6634a6640fe5740008e82937
😎 Deploy Preview https://deploy-preview-1723--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@dancormier dancormier left a comment

Choose a reason for hiding this comment

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

Great work @giamir. I think this PR shows an elegant and uncomplicated way to handle denoting required inputs. I have a few minor suggestions but nothing too intense.

docs/product/components/labels.html Show resolved Hide resolved
lib/components/label/label.less Outdated Show resolved Hide resolved
docs/product/components/inputs.html Outdated Show resolved Hide resolved
giamir and others added 3 commits April 30, 2024 10:44
@giamir
Copy link
Contributor Author

giamir commented May 3, 2024

@dancormier I have added the visual/a11y tests and took the occasion to refactor a bit. You can find the diff here.
I would say that this PR is now ready to be merged after a final look from your end.

I have tested the example I put together in the inputs page in NVDA and this is what the narrator announces when I move from section to section with the arrow keys:

  • Heading level 1 ask a question
  • Required fields star
  • Title star
  • Edit type a title
  • Body star
  • Edit multiline type a question
  • Ask team members
  • Edit type a name

As you can see by default NVDA doesn't read the title attribute of the abbr tag. This seems to be expected behavior from the quick research I have done. There is a settings that users can change to have the narrator announcing the title attribute too.
From a personal perspective I don't find the text announced optimal but the technique we are using is one recommended by WCAG so we should be good. Especially the Required fields star is a bit robotic. I can see how the text they use in the WCAG example would deliver a better experience for users using SRs: Required fields are marked with an asterisk star.
Anyway I think what we have is an acceptable tradeoff that makes us WCAG compliant. Unless I get a red flag either from you or @CGuindon I am happy to proceed with the merge and integrate similar solution for the pages we know about in Core.

@giamir giamir marked this pull request as ready for review May 3, 2024 10:35
@CGuindon
Copy link
Collaborator

CGuindon commented May 3, 2024

I'm less familiar with screen reader norms but I trust you both and if this passes WCAG I think we can proceed. No red flags from me.

@giamir
Copy link
Contributor Author

giamir commented May 6, 2024

@dancormier when you have a second give me the green flag here and I will merge. Thank you. 😊

@giamir giamir merged commit 457404b into develop May 7, 2024
11 checks passed
@giamir giamir deleted the STACKS-582/required-fields branch May 7, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants