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

Make it easy to generate the error summary with correct href values #929

Closed
adamsilver opened this issue Jul 26, 2018 · 9 comments
Closed
Assignees
Milestone

Comments

@adamsilver
Copy link
Contributor

adamsilver commented Jul 26, 2018

When you submit a form containing groups of fields such as radio buttons, populating the error summary so that focus is moved when the error link is clicked is problematic.

Say you have this:

<a href="#name-of-field">Enter blah</a>

And you have this:

<input type="radio" name="name-of-field" id="name-of-field-1" value="1">
<input type="radio" name="name-of-field" id="name-of-field-2" value="2">

When you click the link focus won't move to the first <input> in the group. For that to work, we'd have to suffix the href with -1 to match the input's id attribute value.

This means the server has to be told that a request that contains name-of-field is in fact a group of radios and therefore when the error summary is rendered for this particular error message, be sure to suffix it with -1.

This is a pain, and an unnecessary burden on the developer. What I've always done, is make sure the first input's id in the group matches the name of the group. Like this:

<input type="radio" name="name-of-field" id="name-of-field" value="1">
<input type="radio" name="name-of-field" id="name-of-field-2" value="2">

This works nicely on the server side and on the client side (should a service implement client-side form validation).

@adamsilver adamsilver changed the title First field in a <fieldset> should not be suffixed with -1 Make it easy to generate the error summary with correct href values Jul 26, 2018
@adamsilver adamsilver changed the title Make it easy to generate the error summary with correct href values Make it easy to generate the error summary with correct href values Jul 26, 2018
@36degrees
Copy link
Contributor

Thanks for raising Adam!

We should go through and check that all of the components work well with the error summary. Am I right in thinking this applies to at least the radios, checkboxes and date input component?

I think there might be some things to explore here, especially whether we should link from the error summary to the first field or to the parent fieldset? Have you done any testing with assistive technologies that shows that linking to the first field is the best thing to do?

@adamsilver
Copy link
Contributor Author

adamsilver commented Jul 26, 2018

No worries.

I think it should do this for the radios and the checkboxes. The date input might be a little different.

With the date input, a problem could be that the user left just one of the three input's empty. So the error might read “Enter the day of your date of birth” (or similar). And so the error should link to the day field (not the fieldset/wrapper). But what if the user leaves two of the three input's blank? Is that two errors or one? Interesting stuff.

I took this approach (with the radios/checkboxes) back in ~2008 after the RNIB audited a site I was working on back then—they were happy with this solution. In terms of functionality, it works and I think this is reasonable behaviour for say a screen reader. It's the group that's erroneous and by focusing the first input, the legend would be read out along with the first radio (in much the same way as they would generally traverse and enter the field).

With that said, I would love to learn more from deeper testing with people who use AT.

@kr8n3r
Copy link

kr8n3r commented Jul 27, 2018

technically the change is simple

{% set id = item.id if item.id else idPrefix + ("-" + loop.index if not loop.first)  %}

but it might be a breaking change

@adamsilver
Copy link
Contributor Author

adamsilver commented Sep 20, 2018

Is there any movement on this one please? If not, we'll have to work around this for every field that is a group by giving it a different id to link to.

@36degrees
Copy link
Contributor

Hi Adam,

To change the default behaviour for this component would have to be a breaking change, as it'd change the IDs for services that already use it.

We try to avoid doing breaking-change releases too frequently, as we want it to be easy for service teams to stay up to date. As we only shipped 2.0 just over a week ago, ideally we wouldn’t ship another for a little while. Of course we try to balance that against the needs of our users – if this is creating a lot of extra work for multiple service teams, then it may be that shipping another breaking release might be the right thing to do.

In this case, we think we've found a short term workaround. You can pass an id to the first checkbox which will prevent the suffix from being added – as long as the id on the first checkbox is the same as the field name, you should end up with the behaviour you're looking for.

For example, this:

  {{ govukRadios({
    fieldset: {
      legend: {
        text: 'Radios'
      }
    },
    name: "radios",
    errorMessage: {
      "text": "Problem with radios"
    },
    items:[
      {
        text: 'One',
        value: 'one',
        id: 'radios'
      },
      {
        text: 'Two',
        value: 'two'
      }
    ]
  }) }}

should generate this HTML:

<div class="govuk-form-group govuk-form-group--error">
  <fieldset class="govuk-fieldset" aria-describedby="radios-error">
    <legend class="govuk-fieldset__legend">Radios</legend>
    <span id="radios-error" class="govuk-error-message">
      Problem with radios
    </span>
    <div class="govuk-radios">
      <div class="govuk-radios__item">
        <input class="govuk-radios__input" id="radios" name="radios" type="radio" value="one">
        <label class="govuk-label govuk-radios__label" for="radios">
          One
        </label>    </div>
      <div class="govuk-radios__item">
        <input class="govuk-radios__input" id="radios-2" name="radios" type="radio" value="two">
        <label class="govuk-label govuk-radios__label" for="radios-2">
          Two
        </label>    </div>
    </div>
  </fieldset>
</div>

Does that solve the immediate problem for you? If so, this will give us time to look into this properly, work out a proper fix and roll the change up into a future release at the right sort of cadence.

@36degrees 36degrees added this to the v3.0.0 milestone Sep 21, 2018
@adamsilver
Copy link
Contributor Author

@36degrees yes, that works a treat.

But, I also want to say there is a problem with my thinking on this. And I have a vague recollection that it was you who told me in a separate slack thread. That is:

We shouldn't focus the form control itself as it might mean the label/legend (any text above the control) is out of the viewport.

@36degrees
Copy link
Contributor

Hey @adamsilver,

We did a lot of research into where the error summary should link to and found that for assistive technologies the best option was to link to the form input.

As you state, this means that the browser would scroll to the form control, leaving the legend or label outside of the viewport. To resolve this, we added JavaScript to the error summary component in #1056 which manually manages the focus and scroll behaviour to get the 'best of both worlds'.

In alphagov/govuk-design-system#634 we also added examples to the Design System to make it clearer where to link to – you can see this behaviour working on the examples on that page.

Hope that helps!

@NickColley
Copy link
Contributor

Also ran into this, setting the id to be the same as the name on the first item was indeed a good way forwards.

@NickColley
Copy link
Contributor

@adamsilver thanks for raising, this'll be resolved in v3.0.0 👍 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

5 participants