veda-consulting-company / uk.co.vedaconsulting.mosaico

Other
39 stars 74 forks source link

Lack of indication that field is required #651

Open seamuslee001 opened 2 months ago

seamuslee001 commented 2 months ago

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 required_mark_not_implemented

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

artfulrobot commented 2 months ago

There's 2 questions here:

  1. should we start using some d9-specific classes instead of core markup. My opinion: No we should not. The more centralised (common) we can get our markup the better in many ways.
  2. is core's required markup accessible and themable? This is a bigger question, some notes below, but I suspect they're out of scope for the desired scope of this issue.

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:

<span crm-ui-visible="crmIsRequired" class="crm-marker" title="This field is required.">*</span>

<span crm-ui-visible="crmIsRequired" class="crm-marker" title="This field is required." style="visibility: inherit;">*</span>

<span class="crm-marker" title="This field is required.">*</span>

I suspect (but have not tested) our current core markup might result in:

star This field is required

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:

<span class="crm-required" ><span class=visually-hidden >(required)</span></span>

<style>
   .crm-required {
       background-image: url(/path/to/asterix.svg);
       width: 1em;
       height: 1em;
       position: relative;
   }
   .crm-required:hover>.visually-hidden {
      clip:none;
      clip-path:none;
      height:auto;
      width: auto; 
      /* some other visual CSS to reveal the text */
   }

  .visually-hidden {
    clip: rect(0 0 0 0);
    clip-path: inset(50%);
    height: 1px;
    overflow: hidden;
    position: absolute;
    white-space: nowrap;
    width: 1px;
  }
</style>

As this looks like it might:

seamuslee001 commented 2 months ago

@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

artfulrobot commented 2 months ago

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

vingle commented 2 months ago

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

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 <input id="email" type="text" required /> (tho this will mean failures for not filling in those inputs are handled more loudly by the browser). But then the red asterix could be pure CSS, targeting input:required (e.g. input:required:before {content: "*"; color: red;}) and we can strip out some spans.

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.

mattwire commented 2 months ago

@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). If we can standardise some of the markup (required and wizard) then that would be a great PR to get merged to improve future maintainability/accessibilitly. @seamuslee001 maybe worth checking if these "bad" patterns have been copied across into the mjml editor that JMA has been working on?

artfulrobot commented 2 months ago

@nicol input:required:before {content: "*"; color: red;} You can't add :before or :after pseudo elements to input elements. AND, pseudo constant content will be read out by screen readers (see linked-to blog post) which is unhelpful "star" to sr users; hence my other ideas.

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!

vingle commented 2 months ago

@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 label:has(~ input:required):before? With background image rather than content as you suggested.

Or just keep the styling - the main point is that <input id="email" type="text" required /> covers the SR needs if the other issue can be resolved.

vingle commented 2 months ago

@mattwire yeh I can open a separate issue. The Mosaico Wizard is similar but different to the other wizards:

image image

I think the ideal would be the same markup for all Civi Wizards with an optional extra utility class like .crm-wizard-icons that adds the green ticks and encircled numbers.

(@seamuslee001 - what theme is your original screengrab using above? Is it WordPress/CAU+Radstock?)

artfulrobot commented 2 months ago

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

Are hidden required inputs that common?

:shrug: they probably exist if required and/or display:none (ng-show?) was used without much thought for/knowledge of a11y.

vingle commented 2 months ago

I feel a themetest snippet coming on for proof of concept!

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.

artfulrobot commented 2 months ago

Are hidden required inputs that common? - @vingle

Funny this should pop up at this time: https://lab.civicrm.org/dev/core/-/issues/5182#note_166241