-
Notifications
You must be signed in to change notification settings - Fork 75
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
Lack of indication that field is required #651
Comments
There's 2 questions here:
There's a great blog post on why there's no definitive good way to achieve the visual look you wanted and make it good for a11y. Our current markup patterns, seem to be like:
I suspect (but have not tested) our current core markup might result in:
for Voice Over, Narrator, NVDA and ChromeVox. which is pretty crap. Also note that NDVA will still read elements with aria-hidden=true if it is focusable - form controls are focusable. I would love to know the results of using "required" attribute on html elements where it's required (this may be more complex than it seems at first) and then in the label:
As this looks like it might:
|
@artfulrobot I was just using the D9 themes as examples as I found them but also it seems shoreditch had implemented this class as well so i suspect maybe it dates from when you couldn't use Mosacio without shoreditch. I appreciate the information in regards to the themability / accessibility point |
@seamuslee001 yes, there's allsorts like that. e.g. messages - I never know which of the classes to use and when writing an extension often just pick something that "works for me", which I confess is bad behaviour! I still think we should use the same markup as core though. |
Yes - that's my understanding too @seamuslee001. It was only testing Mosaico with RiverLea I realised that the asterixes were invisible, but not for Shoreditch/Island. So in reply to the original issue: yes, this should use Civi-standard classes for required. Re accessibility, at risk of piggybacking a much bigger subject on a smaller issue, my understanding is it's best to indicate it with an attribute While piggybacking, somewhat related I'd love the Mosaico Wizard bar in the screengrab above to use the same markup as Civi's Wizard bars used in CiviMail, and all the import screens. It uses some but not all of it, which means it breaks with other themes than Shoreditch. |
@seamuslee001 @artfulrobot @vingle There has been some effort in the past to clean up the Mosaico markup (mostly from @artfulrobot and @mlutfy if I remember correctly). |
@nicol The required attribute is good but there's a slight challenge in that there may be situations where a block of stuff is removed with display:none - the input:required still exists and will still fail to submit the form yet you won't see the error bc display:none. For example, "Unsubscribe group" - if that's just hidden/shown then required won't work. If it's actually removed from the DOM when not needed, it will be fine to be declared with required attr. @mattwire yes, standardisation is great! |
@artfulrobot hmm. Are hidden required inputs that common? There are some suggestions for changing the attribute on show/hide. Perhaps the extra complexity of that is justified by the markup improvement? But regarding psuedo elements, ah yes. Maybe something more like Or just keep the styling - the main point is that |
@mattwire yeh I can open a separate issue. The Mosaico Wizard is similar but different to the other wizards: I think the ideal would be the same markup for all Civi Wizards with an optional extra utility class like (@seamuslee001 - what theme is your original screengrab using above? Is it WordPress/CAU+Radstock?) |
@vingle I feel a themetest snippet coming on for proof of concept! From a11y POV, the current styling is an issue because we don't expect visual users to put up with *Required field on every field (we hide the words, and visually it's easy to ignore a star), so we shouldn't expect SR users to have to have "star required field" read out every time. But that's a core issue, so I'll park that one.
🤷 they probably exist if required and/or display:none (ng-show?) was used without much thought for/knowledge of a11y. |
For both the wizard and the input… There's a big PoC needed around how inputs sit with labels and descriptions - which maybe should be its own ThemeTest section as there's many variations from tables, to div's floating 17% to the right, to inline. |
Funny this should pop up at this time: https://lab.civicrm.org/dev/core/-/issues/5182#note_166241 |
On I would say a decent number of themes there is a lack of a visible indication that a field is required (usually because of an asterisk) as per the attached screenshot
This is seemly caused by the change to use the "required-mark" css class instead of using the way core does it which is in a template like this https://github.com/civicrm/civicrm-core/blob/master/ang/crmUi.js#L253
It isn't clear to me what the intention was when changing to the class but I notice Olivero and Claro in D9 use this class.
it could be that it was intentional? It seems like the class was defined in shoreditch?
thoughts @mattwire @vingle @totten @artfulrobot
The text was updated successfully, but these errors were encountered: