Multiple HeaderFields in UserForm creates WCAG Duplicate ID issue

[holyavocad0]

When a UserForm uses more than one HeaderField in a single Form then the frontend will render multiple HeaderFields with the same HTML "id" attribute. This scenario creates an accessibility and html validation issue, see image below.

The forms ID is used to create a small amount of uniqueness for the html "id" attribute, it seems like the though process here is that a HeaderField would only really get used once per form. Adding the fields ID might be a possible solution.

Module version(s) affected

silverstripe/userforms 6.0.4

Description

https://github.com/silverstripe/silverstripe-framework/blob/f60e1bc236cf5c238351d8f88e729b76d812334f/src/Forms/HeaderField.php#L61

The HeaderField class has no template to override and the getAttributes() method is not extendable (updateAttributes)as a DataExtension so there is no way to customise this behaviour at the moment.

How to reproduce

class HeaderField extends DatalessField {

  public function getAttributes()
      {
          return array_merge(
              parent::getAttributes(),
              [
                  'id' => $this->ID(),
                  'class' => $this->extraClass(),
                  'type' => null,
                  'name' => null
              ]
          );
      }
}

This method returns:

HeaderField.php:68 - SilverStripe\Forms\HeaderField::getAttributes() - 
array (
  'type' => NULL,
  'name' => NULL,
  'value' => NULL,
  'class' => ' FormHeading hide',
  'id' => 'UserForm_Form_24_userforms-header',
  'disabled' => false,
  'readonly' => false,
  'autofocus' => false,
  'data-id' => 'EditableTextField_ac0fd',
)

In cases where a form uses multiple HeaderFields this ID is repeated throughout the page where unique ID is the ID of the form itself:

'id' => 'UserForm_Form_24_userforms-header'

To reproduce:

1. Create a UserFrom.
2. Add multiple HeaderFields to the form.
3. Validate the rendered HTML https://validator.w3.org/

Possible Solution

Update the EditbaleFormHeading class getFormField method to append the fields ID.

public function getFormField()
    {
        $labelField = HeaderField::create('userforms-header-' . $this->ID , $this->Title ?: false)
            ->setHeadingLevel($this->Level);
        $labelField->addExtraClass('FormHeading');
        $labelField->setAttribute('data-id', $this->Name);
        $this->doUpdateFormField($labelField);
        return $labelField;
    }

Additional Context

Validations

• [X] Check that there isn't already an issue that reports the same bug
• [X] Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

• https://github.com/silverstripe/silverstripe-userforms/pull/1312

[GuySartorelli]

The title of this issue indicates a clear bug - but there's nothing about it in the "Description" section of the actual issue.... can you please update the issue so that it talks about the actual bug which is hinted at in the title?

[holyavocad0]

@GuySartorelli updated. I hope that more sense now. TY

[NightJar]

It seems the issue is that the name for the generated/output form field is hard-coded. https://github.com/silverstripe/silverstripe-userforms/blob/82b137a76bd7e33768aabf1689fd19e9f4067aa8/code/Model/EditableFormField/EditableFormHeading.php#L73

Adding the ID to this could resolve the problem for example.

@holyavocad0 in the meantime you should be able to use the afterUpdateFormField hook to get around this via the FormField API directly (->setName($nonConflictingId)) https://github.com/silverstripe/silverstripe-userforms/blob/82b137a76bd7e33768aabf1689fd19e9f4067aa8/code/Model/EditableFormField.php#L736

Would you like to submit a pull request if the change identified above (the first one; adding the ID to the name string) solves the problem?

[holyavocad0]

@NightJar just creating a pull request for it now. Which branch should I target?

[NightJar]

@holyavocad0 it depends on which branch you're based on. Generally the advice is to fix the issue when it occurred, and it will be merged up... however I expect this has been a (very) long standing problem. It is probably best to target the least "in support" version.

Personally I would rebase onto and target 5.15 - but @GuySartorelli might have another opinion as an actual maintainer ;)

[GuySartorelli]

CMS 4 branches shouldn't be targetted, as we aren't releasing new patches for CMS 4 outside of security releases.

If the PR has no API changes you can target 6.2, otherwise target 6. See https://docs.silverstripe.org/en/contributing/code/#picking-the-right-version for more detailed advice.

[NightJar]

If the fix is as I suggested then 6.2 should be OK then. However it is probably best to check that there is also no impact on the defult bundled JS solutions (that hide/show fields, etc.) with this change @holyavocad0, for a bug-free solution :)

[holyavocad0]

@GuySartorelli PR here: https://github.com/silverstripe/silverstripe-userforms/pull/1312

[GuySartorelli]

PR merged. This will be automatically tagged by GitHub Actions