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

Improve accessibility of Umbraco Forms #1038

Closed
rickdoesburg opened this issue Jun 19, 2023 · 4 comments
Closed

Improve accessibility of Umbraco Forms #1038

rickdoesburg opened this issue Jun 19, 2023 · 4 comments
Labels
release/10.5.0 release/12.1.0 state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks type/feature

Comments

@rickdoesburg
Copy link

rickdoesburg commented Jun 19, 2023

I was tasked with styling some forms in an Umbraco website we're building, and I've noticed a couple of accessibility issues. We were able to work around some of the issues, but it would be better to have them fixed in Umbraco Forms itself.

  1. The field validation span (@Html.ValidationMessage) that's present on page load doesn't notify screen readers that the contents have been updated after an unsuccessful submit. The validation-element should contain role="alert". WCAG.3.3.1. Fields should also get an aria-invalid attribute if they're invalid after submitting.
  2. The default rendering of the CheckBoxList and RadioButtonList could be improved. Currently these elements are rendered using the same loop as the other fields. Resulting in:
<div class="">
  <label for="fieldID">Field label</label>
  <div class="umbraco-forms-field-wrapper">
    <span class="checkboxlist" id="fieldID">
      <input id="fieldID_1" type="checkbox">
      <label for="fieldID_1">Checkbox Label</label>
      <input id="fieldID_1" type="checkbox">
      <label for="fieldID_1">Checkbox Label</label>
    </span>
  </div>
</div>

While according to w3 it should be:

<fieldset class="">
  <legend>Field label</legend>
  <div class="umbraco-forms-field-wrapper">
    <span class="checkboxlist">
      <input id="fieldID_1" type="checkbox"> 
      <label for="fieldID_1">Checkbox Label</label>
      <input id="fieldID_1" type="checkbox">
      <label for="fieldID_1">Checkbox Label</label>
    </span>
  </div>
</fieldset>

Radio button groups should always be grouped using <fieldset>. The legend for a group of controls can also highlight common attributes of all controls, for example, to advise that all fields in the group are required.

  1. Same goes for the single checkbox. Because it's rendered using the default loop it renders the label using the above view but without any text (label) next to the checkbox. The single checkbox field would also be best with the improved view above. Example: Customers would use the single checkbox for opting in to a newsletter. Resulting in a label (above the checkbox) with a single checkbox without a label below it.
  2. Required form fields will have the *, but that will be read as 'star'. required or aria-required should be added to indicate required fields.
  3. The field-hint could be linked to the input using aria-describedby. See example 2.

This item has been added to our backlog AB#30809

@Ambertvu
Copy link

Tried incorperating this with @rickdoesburg in our custom views, but it was a bit of an if/else fest, not very neat and not upgrade friendly :-).
I reckon a method that either return the div with a label or a fieldset with a legend is easily enough to add and can help making Forms more accessible.

@rickdoesburg rickdoesburg changed the title Improve accessibility of Umbraco Froms Improve accessibility of Umbraco Forms Jun 20, 2023
@AndyButland AndyButland added type/feature state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks labels Jun 27, 2023
@AndyButland
Copy link

AndyButland commented Jun 27, 2023

Thanks for these suggestions. I've been having a look this morning to see what we can reasonably apply. 1, 4 and 5 seems straightforward.

3 seems to be a bit tricky to be able to sensibly wrangle the generalised mark-up rendering to support this.
I'm not particularly an accessibility expert - but perhaps this isn't too important? We do have a label and an input that are correctly related via the for and id attributes, it's just that they aren't siblings in the mark-up.

For 2 I'm looking at adding a new property on the definition of a FieldType, called IsHtmlFieldSet. This will be true for the radio button and checkbox list (and any custom field types people create and want to set this value), otherwise false by default.

Then we can amend the mark-up to something like this:

@foreach (FieldViewModel f in c.Fields)
{
    bool hideFieldWhenRendering = f.HasCondition && f.ConditionActionType == FieldConditionActionType.Show;

    @if (f.FieldType != null && f.FieldType.IsHtmlFieldSet)
    {
        <fieldset class="@Html.GetFormFieldWrapperClass(f.FieldTypeName) @f.CssClass" @{ if (hideFieldWhenRendering) { <text> style="display: none" </text>  } }>

            @if (!f.HideLabel)
            {
                <legend class="umbraco-forms-legend">
                    @{
                        RenderCaption(f);
                    }
                </legend>
            }

            @{
                await RenderField(f);
            }

        </fieldset>                                  
    }
    else
    {
        <div class="@Html.GetFormFieldWrapperClass(f.FieldTypeName) @f.CssClass" @{ if (hideFieldWhenRendering) { <text> style="display: none" </text>  } }>

            @if (!f.HideLabel)
            {
                <label for="@f.Id" class="umbraco-forms-label">
                    @{
                        RenderCaption(f);
                    }
                </label>
            }

            @{
                await RenderField(f);
            }

        </div>                                
    }
}

I'm aware that this could be presentationally breaking (e.g. if someone has used and styled the default theme, but their CSS doesn't target the new elements). But currently considering that would be OK to do so long as we make it clear in the release notes and blog post that accompany the next minor release.

Please let me know if you have any further thoughts.

@rickdoesburg
Copy link
Author

Thanks for the response, Andy. Changing the implementation of point 2 would already be a great improvement, and I think your example markup is good. The markup of the single checkbox is indeed just fine, and they are correctly 'linked,' so you could just leave it as it is. My example about it being used as a 'consent' option wasn't correct because users could simply use the 'consent' field, which does have the extra label.

It's great to hear that my suggestions are being considered for implementation!

@AndyButland
Copy link

These updates as discussed above will be part of the next releases.

We've taken a call that to avoid the potential presentationally breaking change mentioned, that for point 2 - the use of fieldset/legend - it will be necessary to switch on the changed rendering in configuration. That way anyone relying on existing CSS won't experience any change when they upgrade, but anyone still can opt into the this more semantic rendering of radio and check box lists if they want to. For the next major, we'll remove the configuration and make the newer and improved mark-up the default.

The following will be added to the documentation and shared theme on release, but just to also include here, the setting will be:

  "Umbraco": {
    "Forms": {
      "Options": {
        "UseSemanticFieldsetRendering": true|false

We'll then use that to populate a field on the FormViewModel and use it in the default theme as follows.

A property RenderInputType with a value of Single, Multiple or Custom has been added to field types, so each field type can be defined according to how it renders input types. All the Forms field types have the appropriate value.

@foreach (FieldViewModel f in c.Fields)
{
    bool hideFieldWhenRendering = f.HasCondition && f.ConditionActionType == FieldConditionActionType.Show;

    if (Model.UseSemanticFieldsetRendering)
    {
        switch (f.FieldType?.RenderInputType ?? RenderInputType.Single)
        {
            case RenderInputType.Single:
                <div class="@Html.GetFormFieldWrapperClass(f.FieldTypeName) @f.CssClass" @{ if (hideFieldWhenRendering) { <text> style="display: none" </text>  } }>

                    @if (!f.HideLabel)
                    {
                        <label for="@f.Id" class="umbraco-forms-label">
                            @{
                                RenderCaption(f);
                            }
                        </label>
                    }

                    @{
                        await RenderField(f);
                    }

                </div>
                break;

            case RenderInputType.Multiple:
                <fieldset class="@Html.GetFormFieldWrapperClass(f.FieldTypeName) @f.CssClass" @{ if (hideFieldWhenRendering) { <text> style="display: none" </text>  } }>

                    @if (!f.HideLabel)
                    {
                        <legend class="umbraco-forms-legend">
                            @{
                                RenderCaption(f);
                            }
                        </legend>
                    }

                    @{
                        await RenderField(f);
                    }

                </fieldset>
                break;

            case RenderInputType.Custom:
                @await Html.PartialAsync(FormThemeResolver.GetFieldView(Model, f), f)
                break;
        }
    }
    else
    {
        <div class="@Html.GetFormFieldWrapperClass(f.FieldTypeName) @f.CssClass" @{ if (hideFieldWhenRendering) { <text> style="display: none" </text>  } }>

            @if (!f.HideLabel)
            {
                <label for="@f.Id" class="umbraco-forms-label">
                    @{
                        RenderCaption(f);
                    }
                </label>
            }

            @{
                await RenderField(f);
            }

        </div>                                
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/10.5.0 release/12.1.0 state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks type/feature
Projects
None yet
Development

No branches or pull requests

3 participants