StackExchange / Stacks

Stack Overflowโ€™s Design System
https://stackoverflow.design
MIT License
602 stars 90 forks source link

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

Closed giamir closed 2 months ago

giamir commented 2 months ago

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:

netlify[bot] commented 2 months ago

Deploy Preview for stacks ready!

Name Link
Latest commit ba4a5ee9f706429872e7eac3c42010fa79f67a0f
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.

giamir commented 2 months ago

@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:

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.

CGuindon commented 2 months ago

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 commented 2 months ago

@dancormier when you have a second give me the green flag here and I will merge. Thank you. ๐Ÿ˜Š