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

V8: Accessibility improvements for textbox validation #5664

Merged
merged 4 commits into from
Jun 25, 2019
Merged

V8: Accessibility improvements for textbox validation #5664

merged 4 commits into from
Jun 25, 2019

Conversation

RachBreeze
Copy link
Contributor

A bug in Angular is triggering "invalid" prompt to be played when a user tabs into an empty text box prior to entering any information, this PR removes that prompt.

On reviewing error messages after saving, it's not possible for a screen reader user to determine what the errors are. So this PR also continues to use the sr-only class first introduced in #5585 to allow error messages to be styled for users of both screenreaders and browsers.

Note the sr-only class is deliberately missing from the required error message, as screen readers hear a textbox is required when they tab into it

Rachel Breeze and others added 4 commits June 11, 2019 20:45
…d them. Note the "required" error message has not been changed to be heard by screen readers as they already hear this when in the textbox.
overflow: hidden;
clip: rect(0,0,0,0);
border: 0;
}
Copy link
Contributor

@bjarnef bjarnef Jun 14, 2019

Choose a reason for hiding this comment

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

@RachBreeze it seems this class is more or less identical with the class visually-hidden in this PR https://github.com/umbraco/Umbraco-CMS/pull/5544/files

The A11Y Project mention the visually-hidden.
https://a11yproject.com/posts/how-to-hide-content/

Not sure which one is preferable?
/cc @MMasey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bjarnef sr-only is a naming convention in bootstrap (https://getbootstrap.com/docs/4.0/utilities/screenreaders/).

There are a three pull requests now for a screen reader only class this one and #5585 #5544

It might be worth just doing a PR for screen readonly class and then updating these PRs to reference that class. What do you think @MMasey and @nul800sebastiaan?

Copy link
Contributor Author

@RachBreeze RachBreeze Jun 16, 2019

Choose a reason for hiding this comment

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

Hi @bjarnef and @MMasey
I have a PR pending which will just be CSS amends, the class can either be visually-hidden or sr-only

Indeed almost did the PR with visually-hidden . However bootstrap also has sr-only-focusable class which

will ensure that the link becomes visible once focused (for sighted keyboard users).

Happy to call the class either, the list of issues on #5277 does require a class that supports hidden text for screen readers. It would be good to standardise on one name, and get a PR accepted with just that class in. Let me know which you prefer and I will submit a PR for that.

Cheers

Rachel

@MMasey
Copy link
Contributor

MMasey commented Jun 16, 2019

Hey @RachBreeze, shall we go with visually-hidden as suggested in the A11Y project? It also disassociates from a specific framework. 🙂

@RachBreeze
Copy link
Contributor Author

HI @MMasey

Just a quick update I am very close to submitting a PR with just visually-hidden but am not comfortable with it as I though I would be, sorry.

They key blocker on me submitting this a PR for this is when you consider the use for adding -focusable to the class

  • visually-hidden-focusable as a css classname doesn't work. The name implies the element is hidden, so why would you hide an element but focus on it?

  • sr-only-focusable does as it's a focusable screen reader element.

This is how bootstrap says to use the screen reader classes (https://getbootstrap.com/docs/4.0/utilities/screenreaders/)

Hide an element to all devices except screen readers with .sr-only. Combine .sr-only with .sr-only-focusable to show the element again when it’s focused (e.g. by a keyboard-only user)

I also understand the reason for arguing for dissociating from a specific framework. But various stats sites have the figures for bootstrap usage at somewhere between 53 and 72%.

Cheers

@MMasey
Copy link
Contributor

MMasey commented Jun 21, 2019

Hey @RachBreeze,

I think I managed to miss the part about the focusable class so my bad on that. I’m happy to go with sr-only approach in hindsight, you make a great point 🙂 I’ll update my related PR with the change.

@RachBreeze
Copy link
Contributor Author

Hi @MMasey
I'll submit a PR with just sr-only. I like your .less structure so will continue to use that (and update the other PRs to reflect it to prevent merge conflicts)
cheers :-)

@RachBreeze
Copy link
Contributor Author

Hi @MMasey just to let you know I've submitted the PR for sr-only it's #5705
Have a lovely weekend

@nul800sebastiaan
Copy link
Member

I totally missed this conversation in relation to #5544

I've merged #5544 already but happy to remove the visually-hidden class again (it looks unused).

In the mean time, there's no objection to merge this one as well, is there @RachBreeze ?

@nul800sebastiaan
Copy link
Member

Ah, actually there is 2 usages in 5544 - but they could be updated to sr-only?

@RachBreeze
Copy link
Contributor Author

Hi @nul800sebastiaan no there's no objection to merging this thank you very much

@RachBreeze
Copy link
Contributor Author

RachBreeze commented Jun 25, 2019

And yes I believe @MMasey was happy for the 2 instances in 5544 to be updated to sr-only

@nul800sebastiaan
Copy link
Member

Cool, I'll get that updated!

@nul800sebastiaan nul800sebastiaan merged commit 40311f8 into umbraco:v8/dev Jun 25, 2019
@nul800sebastiaan
Copy link
Member

This all checks out, great job @RachBreeze ! 🎖

@RachBreeze
Copy link
Contributor Author

Cheers for reviewing and merging @nul800sebastiaan

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.

4 participants