springernature / frontend-toolkits

Frontend Component Toolkits for the Elements Design System
MIT License
12 stars 3 forks source link

`globalFormRadios` Component Fieldset Accessibility Error #864

Open danielclubb opened 1 year ago

danielclubb commented 1 year ago

Hey team!

We think we may have found a bug in the implementation of global forms. We realised after we implemented Pa11y tests in our project which includes the CSAT widget.

The globalFormRadios component has a <fieldset> element inside of it, which uses the globalFormAttributes component to set data/aria attributes on the fieldset element dependent on the context.

However, inside the globalFormAttributes component there is the following line: {{unless optional}}required aria-required="true"{{/unless}}

In our use case, Pa11y is throwing the following accessibility error: "Elements must only use allowed ARIA attributes (https://dequeuniversity.com/rules/axe/4.2/aria-allowed-attr?application=axeAPI)" because global forms is adding an aria-required attribute to a group <fieldset> element which it shouldn't be, it should only add this aria attribute to the individual input elements inside the fieldset.

This is happening because the unless logic on the globalFormAttributes component will always set this attribute unless optional is explicitly defined.

We feel the logic will need to be updated so that this attribute won't be set when globalFormAttributes is used on a group level element.

Thanks!

CC: Andreas Koscinski andreas.koscinski@springernature.com, Daniel Clubb daniel.clubb@springernature.com, Morgan Cugerone morgan.cugerone@springernature.com

Heydon commented 1 year ago

Hi @danielclubb. Some points:

The issue arises from applying common attributes to any field where present in the data, like {{> inputCommonAttributes}}. Otherwise we'd have a significant amount of code duplication. For a fieldset, you simply would not add attributes only suitable for inputs. However, required is set by default so has to be removed manually using the workaround above.

If handlebars was a more expressive templating language, we'd be able to write {{#if template !== 'globalFormRadios'}} or similar and use that logic. The only way to use expressions with Handlebars is with Handlebars helpers. Unfortunately, these would have to be maintained in Java and JavaScript and frontend-toolkits is not a Java repository/environment. 🤷‍♂️🤷‍♂️🤷‍♂️. Sorry for the inconvenience!

morgaan commented 1 year ago

Hi @heydon, thank you for your elaborate answer 🤗

Far from us to go down the more expressive templating language way, using helpers... as this may encourage sticking more logic in the view... hence the Java/Javascript overhead it brings...

After some processing about what you meant by:

the workaround is to add "optional": true to the globalFormRadios field in question. We did just that and then got rid of the pa11y issue 🙌. It feels a bit counter intuitive, so maybe that would be worth a hint into the documentation 🤷? I'll leave this issue open just in case you want to use it as a reference for adding the hint, and if not, please go ahead and close it 🙏.

That fix made our day anyway so thanks again 🤗.

Cheers!