Add validation examples for single-question, single-field form pages

[paulwaitehomeoffice]

What problem does the pull request solve?

Developers currently have no examples of how to implement error-state HTML for single-question form pages where the page’s <h1> is also the label for the question’s single field/fieldset (this pattern was added to the documentation in https://github.com/alphagov/govuk_elements/pull/507).

This is intended to fix https://github.com/alphagov/govuk_elements/issues/543.

However, the HTML and appearance of these examples might not be optimal:

1. I’ve used <span class="heading-large"> within the <label> so that the heading’s bottom margin appears between the heading text and any hint or error text. However, it is not display:block by default. I’ve fixed that in the example by adding an inline style (<span class="heading-large" style="display:block;">), but that seems sub-optimal.

2. When we include the <h1> inside the <legend>, because <fieldset> elements prevent the margins of their child elements from collapsing, we get more space above the h1 compared to single-text-field pages. This is clearly visible when the error border is applied.

   I can’t find any way of changing this fieldset behaviour (display:contents may fix this one day, but not today). We could create a new class to be applied to the fieldset or the parent form-group that adds the top margin, there, and removes it from child headings - I'm not sure if that's over-thinking this.

3. Our interaction designers pointed out that with just one question on the page, there might not be any need for the red left border — in multi-question forms it indicates which question is in error, but as there’s only one question, it’s perhaps a bit superfluous here. Thoughts?

4. We also wondered whether we needed the error summary box at the top, given that again, there was only one error to summarise.

   We presumed that the role="alert" markup, and the fact that it's the first thing on the page, helps to clearly indicate to screen readers that there is an error; we could imagine that without it, the screen reader user wouldn’t learn about the error unless and until they navigated all the way to the field label.

Any advice/discussion would be welcome.

Screenshots:

How has this been tested?

• Validated, using validator.w3.org. Validation errors were raised for <span> elements being used within <legend> elements, even though the spec seems to say that both headings and phrasing content (which includes spans) are allowed. The errors about the spans went away when I removed the <h1> from the legend, so perhaps this is a bug in the validator?
• Visually inspected for consistency in:
  • macOS 10.12
    • Safari 10.1.2
    • Chrome 61 (desktop width, and simulated portrait smartphone width)
    • Firefox 55
  • Windows 10, Edge 38
  • Windows 8, Internet Explorer 11
  • Windows 7
    • Internet Explorer 9, 10, 11
    • Chromium 38, 43
  • Windows XP, Internet Explorer 8

What type of change is it?

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Has the documentation been updated?

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.

[craigmorrison]

It looks like there is a bit of inconsistency between the W3C and WHATWG specs when it comes to the valid content of a legend. The mention of headings is only present in HTML 5.2, not in 5.1 or the WHATWG spec. I think that explains why the validator isn't very reliable here.

On the student finance service we've previously used a span with role="heading" inside the legend to tag content as both. I couldn't find any real world issues with using actual heading elements and it's certainly clearer than the ARIA attributes. It's a bit of a grey area but I'm encouraged to see it clarified in the W3C spec now.

[edwardhorsford]

On passports we nested the left border beneath the h1. Throwing it out as an option.

[paulwaitehomeoffice]

@edwardhorsford we thought that would look better too. Is there a public URL where I can see the HTML you used?

[edwardhorsford]

@pauldwaite Start here, then go down UK renewal route...

Edit: I don't know how current the markup is with latest Elements.

[paulwaitehomeoffice]

@edwardhorsford Marvellous, thank you.

Yeah so the main difference with that passport example is the <h1> isn’t in the <fieldset>’s <legend> — instead, its content gets repeated and visually hidden in the fieldset legend. So the left border naturally gets nested under the h1.

When the h1 is in the legend (along with the hint and error message), it’s tricker to get it sitting outside of the border. I did get an example working, but it was a bit fiddly.

We did wonder whether the red left border was actually needed for single-field single-page questions — it felt like its purpose was to clearly indicate which field had the error, and when there’s just one field, that’s hopefully obvious.

[edwardhorsford]

@pauldwaite It will be markup from before we started recommending putting the h1 inside of the legend.

I personally like the left border - for me it's a good visual change that doesn't rely on colour alone to indicate a problem.

[selfthinker]

Generally, good stuff! Those examples are really needed in Elements.

I’ve used <span class="heading-large"> within the <label> so that the heading’s bottom margin appears between the heading text and any hint or error text. However, it is not display:block by default. I’ve fixed that in the example by adding an inline style (<span class="heading-large" style="display:block;">)

I think it should be fine to add display: block; to all the heading CSS (i.e. within the CSS not inline). There is a very slight chance that it could break something but I would expect all kinds of headings to be block elements.

When we include the <h1> inside the <legend>, because <fieldset> elements prevent the margins of their child elements from collapsing, we get more space above the h1 compared to single-text-field pages.

We should usually not get any space, right? Why not just set the top margin to zero in this case (.form-group-error legend h1)?

We presumed that the role="alert" markup, and the fact that it's the first thing on the page, helps to clearly indicate to screen readers that there is an error; we could imagine that without it, the screen reader user wouldn’t learn about the error unless and until they navigated all the way to the field label.

You can read more about the reason for the alert role in #511. There are many mechanisms in place to alert screen reader users to the errors on the page. They are starting the title with the word "error", focussing on the error summary box and giving it the alert role. The latter is only really for fixing a bug in iOS and Android.

Validation errors were raised for <span> elements being used within <legend> elements

We intentionally decided to be ahead of times and implement it even though it is in a future spec, but it is very well supported. You can read more about the reasoning in #507.

[paulwaitehomeoffice]

@craigmorrison that makes sense; as @selfthinker added, it looks like it’s the same issue from #507 .

[paulwaitehomeoffice]

@selfthinker Marvellous, thank you for the feedback.

Why not just set the top margin to zero in this case (.form-group-error legend h1)?

We could do, but I think the heading’s top margin is required to define the space above it. If there’s an error summary there, that matters less; but if there’s not, you end up with less space between the heading and, say, the breadcrumb:

I think it should be fine to add display: block; to all the heading CSS

Sure, makes sense. Will do.

There are many mechanisms in place to alert screen reader users to the errors on the page

Right, gotcha — very helpful summary, thank you. I’ll see what our interaction designer thinks.

We intentionally decided to be ahead of times and implement it even though it is in a future spec, but it is very well supported

Gotcha, sounds like this is the same thing from #507 .

[selfthinker]

Why not just set the top margin to zero in this case (.form-group-error legend h1)?

We could do, but I think the heading’s top margin is required to define the space above it. If there’s an error summary there, that matters less; but if there’s not, you end up with less space between the heading and, say, the breadcrumb

I assumed whenever there is an error, there will always be the error summary. At least that's what the guidance currently says. But I guess people might ignore that if there is just one error. That's a tricky one. I would usually only give things a bottom margin, very rarely a top margin, then you wouldn't have this problem. But changing the general spacing within Elements is obviously out of scope here.

[paulwaitehomeoffice]

@selfthinker ah yes I see — so the error summary is expected, even for single-question pages.

If the red left border is changed to not appear next to heading labels (as @edwardhorsford suggested), that makes the extra space less obvious:

If we do want headings in fieldsets to have the same effective top margin styles as other headings, I think it requires removing the top border from headings in legends like you suggested:

legend {
  overflow: hidden;

  // Remove top margins from headings in legends, because fieldsets block margin collapsing
  .heading-xlarge,
  .heading-large,
  .heading-medium,
  .heading-small {
    margin-top: 0;
  }
}

and then adding an additional class to fieldsets, perhaps like this?

fieldset {
  @extend %contain-floats;
  width: 100%;

  &.with-heading-xlarge {
    margin-top: em(15, 19);

    @include media(tablet) {
      margin-top: em(30, 19);
    }
  }

  &.with-heading-large {
    margin-top: em(25, 19);

    @include media(tablet) {
      margin-top: em(45, 19);
    }
  }

  &.with-heading-medium {
    margin-top: em(25, 20);

    @include media(tablet) {
      margin-top: em(45, 24);
    }
  }

  &.with-heading-small {
    margin-top: em(10, 16);

    @include media(tablet) {
      margin-top: em(20, 19);
    }
  }
}

Used like this:

<div class="form-group">
  <fieldset class="with-heading-large">

    <legend {% if error %} id="example-personal-details"{% endif %}>
      <h1 class="heading-large">
        Are your personal details correct and up-to-date?
      </h1>

Quite a lot of complexity introduced, just to maintain vertical space consistency. There may well a simpler or better-designed way to do it, but I can’t think of it at the moment.

[paulwaitehomeoffice]

@edwardhorsford Moving form-group-error to separate divs is a way to get the desired border styling:

<div class="form-group">
  <fieldset>
    <legend id="example-personal-details">
      <h1 class="heading-large">Are your personal details correct and up-to-date?</h1>

      <div class="form-group-error">
        <span class="form-hint">Look at your name, signature and other details.</span>

        <span class="error-message">Error message about personal details goes here</span>
      </div>
    </legend>

    <div class="form-group-error">
      <div class="multiple-choice">
        <input id="personal_details_yes" type="radio" name="personalDetails" value="Yes">
        <label for="personal_details_yes">Yes, my personal details are correct</label>
      </div>

      <div class="multiple-choice">
        <input id="personal_details_no" type="radio" name="personalDetails" value="No">
        <label for="personal_details_no">No, some details are wrong or have changed</label>
      </div>
    </div>

  </fieldset>
</div>

Although for it to work for radios and checkboxes, @extend %contain-floats; has to be added to .form-group-error.

[timpaul]

Just adding my tuppence-worth. I agree with Ed that the red border is more effective when it runs below the H1 rather than alongside it.

[selfthinker]

I have an idea how to remove some of the complexity from the CSS and HTML... The main reason for the complexity is the consistency, right?

You could subtract the error summary bottom margin from a different element which surrounds the heading. Now that you've moved the form-group-error further down, without adding a new class, it can only be targeted via e.g. .error-summary + .form-group. So, if you undo 51a3b06, you could use

.error-summary + .form-group {
  margin-top: -30px;
}

(If the .form-group-error was still around the whole section, that could be used instead, making it even simpler.)

Disadvantages are: Uncertainty about changes developers might make to the code (which could be fixed by adding a new class instead), "magic numbers" are never great (should at least add a comment explaining where the -30px are coming from) and that it's easy to forget mobile adjustments which would otherwise come for free.

[paulwaitehomeoffice]

@selfthinker That could work, although we’d need to limit it to fieldsets — .error-summary + .form-group would also apply to single-question pages without fieldsets too, which don’t have the same problem:

So we’d probably want something like:

.error-summary + fieldset,
.error-summary + .form-group > fieldset:first-of-type {
  margin-top: -30px;
}

That would work for the example HTML I’ve added, although the selector is more wide-ranging, and so could have less predictable results.

[selfthinker]

I'd personally be happy with .error-summary + fieldset etc. but knowing my colleagues, they would probably much prefer a specific class for this.

[paulwaitehomeoffice]

@selfthinker Right right. Selfishly, our project has to support IE 8, which doesn’t support .error-summary + .form-group > fieldset:first-of-type, so I’m good with a class instead.

[selfthinker]

Apart from the CSS change, I would also rename the links to the example pages. They should reflect more what people are looking for and not what code is in there. (Unfortunately the difference between legend and label handling and the h1 is only due to code. But I wouldn't explicitly name it like that.) I'd suggest (including changing the order):

• form validation - single question with radio buttons
• form validation - single question with text field
• form validation - single question with more text
• form validation - multiple questions

[paulwaitehomeoffice]

@selfthinker right right, yes that is better.

Is “single question with radio buttons” the one where the <legend> is in the <h1>, and “single question with more text” is the one with radio buttons where the <legend> and the <h1> are separate?

[selfthinker]

Is “single question with radio buttons” the one where the <legend> is in the <h1>, and “single question with more text” is the one with radio buttons where the <legend> and the <h1> are separate?

Yes, that's what I meant. So, current form 1 would be form 3. It should get much more text according to #542, but that's obviously not in this PR.

[paulwaitehomeoffice]

@selfthinker oh yes I see. Maybe #542 would be “... with even more text”.

[selfthinker]

The problem with the current "single question" example is that the additional text is not necessary. It could easily be changed to make h1 and legend the same. ("Are your personal details correct and up-to-date?" as h1/legend with no intro.) That's why there should ideally be an example which makes the additional text necessary.

[edwardhorsford]

I might have a better example with more text - let me have a think.

FWIW I might call it form validation - single question with additional guidance or something similar that indicates it's about significant extra text, not hint text.

[edwardhorsford]

Images moved to #542.

[selfthinker]

@edwardhorsford, can you post those examples in #542? I would keep it separate from this PR.

[edwardhorsford]

@selfthinker I've copied them over. Would you like me to delete from this ticket?

[selfthinker]

@edwardhorsford, I think that would make sense. Instead of deleting the whole comment you could just refer to your new comment on #542.

[paulwaitehomeoffice]

I’ve given the new example pages more sensible names. I have also added an example like the one requested in #542 , although I can of course get rid of that if we’d rather not have it in this PR.

[selfthinker]

Great, although I feel it should ideally be in a separate PR, I'm also happy to have it in this one. I can have look at it today or tomorrow.

There are still the CSS changes open which we discussed. Do you need any help with that?

[paulwaitehomeoffice]

@selfthinker Marvellous, cheers cheers. Ah yes, the CSS changes are already in there. (You can see the new .with-heading class in _forms.scss.)

[selfthinker]

I was thinking of the CSS changes I suggested to remove the complexity. Right now we have 4 different classes (with-heading-xlarge, with-heading-large, etc) which are not necessary and the one class we need only needs one line (margin-top: -30px), well, and another for the mobile case. I also find the naming confusing, with-heading-* is too generic and people might use it elsewhere. The name should ideally reflect its purpose. What about fieldset.after-error-summary?

[paulwaitehomeoffice]

@self-thinker: ah yes I see your point — if the other heading sizes aren’t used in this context, then they’re not needed here.

with-heading-* is very generic. fieldset.after-error-summary is better; the only potential confusion I can imagine with that is if someone thinks it means the class should only be used when the error summary is shown; when it should probably be used all the time (for consistent margin collapsing).

[selfthinker]

I think you might have misunderstood me. You can still have all the other heading sizes with my code. They are irrelevant. The point is, you can rewrite the CSS to be less complex so you don't need any dependency on the different heading sizes.

Maybe just try it locally to see what I mean:

1. Remove everything you added to _forms.scss
2. Add &.after-error-summary { margin-top: -30px; @include media(mobile) { margin-top: -15px; } }
3. Change with-heading-large in form-error-radio.html to after-error-summary
4. Play around with different heading sizes and they will all have their normal spacing

Yes, the name after-error-summary is not great, either. It somehow needs to reflect the case when it is used. So far that is whenever a fieldset comes after an error summary and has a heading inside. No idea how best to describe that in a class name. It's annoying that it's a single case and not consistent. Otherwise we could use a more generic name and just always add it. (That makes me wish .error-summary + .form-group etc back.)

[cjforms]

@paulwaitehomeoffice @selfthinker "We also wondered whether we needed the error summary box at the top, given that again, there was only one error to summarise."

A bit of historical context. When @gemmaleigh was sorting out this Element around Easter 2015, there was some urgency because the previous version was not accessible to screenreader users. We were working with Leonie Watson, and there was a lot of discussion about how to make sure it was accessible and usable by a wide range of users. As I recall, the process had stretched into many days if not weeks, and making something live and accessible was a priority. (I recall the dates relatively accurately because part of the work was interrupted by the closure of Aviation House due to the Holborn fire at that time).

At the time, @gemmaleigh correctly said that having a 'if a single error, do this; if multiple errors, do that' would be a good idea in theory but would take even longer to test (both for accessibility and with people who use the Elements). So we opted for the current 'always have a summary box' guidance because it worked for both the single and multiple examples, albeit probably somewhat overkill for the single question on a page, single error example.

Since then, the Form Structure pattern has been changed and simplified to put more focus on understanding why you're asking every question, designing for common circumstances first, and (perhaps most relevant to this discussion) starting with one thing per page. This means that the single question on a page has become a key feature of the design of many services, and it seems like an excellent idea to me to have a matching way of handling a single error that reflects its simplicity - with the 'multiple question/multiple error' case now being seen as the exception rather than typical.

[paulwaitehomeoffice]

@selfthinker Oh yes I see — sorry, I’d lost track of the thread.

Right yes of course, that solves it eh. I was thinking it might lead to inconsistency because the heading’s top margin won’t collapse with any margins above it within the same grid column, but page headings don’t seem to have anything else above them within the same grid column (apart from the error summary).

One tiny last query — should it be:

&.after-error-summary {
    margin-top: -$gutter-half;

    @include media(tablet) {
      margin-top: -$gutter;
    }
  }

to match the .error-summary definition in _form-validation.scss more closely?

[paulwaitehomeoffice]

@cjforms gotcha, really helpful to know the context around it. So we could perhaps think about not having an error summary for single-question pages.

[selfthinker]

@paulwaitehomeoffice, yes, using $gutter makes absolute sense. (Sorry, I was not always looking at the code but sometimes only at the browser output.) Except, the values should be the other way around and using mobile not tablet:

&.after-error-summary {
    margin-top: -$gutter;

    @include media(mobile) {
        margin-top: -$gutter-half;
    }
}

[paulwaitehomeoffice]

@selfthinker naw that‘s cool. Ah right — in _form-validation.scss, the margins are defined the other way round:

.error-summary {
  ...
  margin-top: $gutter-half;
  ...
  @include media(tablet) {
    ...
    margin-top: $gutter;

Do we now prefer larger first, then smaller?

[selfthinker]

You need to look at what the bottom margin does, because that's the equivalent of the top margin of what's underneath it. But I can see that it's also the other way around. Sorry, I didn't think of mobile first, that's why. I guess it makes sense to stick to that. In that case the tablet makes more sense again. Weird that my example worked when I tested it. I cannot test until tomorrow but in theory your previous example looks good. Sorry for the noise.

[paulwaitehomeoffice]

@selfthinker Oh dear, yes sorry I deleted the wrong margin — the error summary’s bottom margin happens to be the same.

.error-summary {
  ..
  margin-bottom: $gutter-half;
  ...
  @include media(tablet) {
    ...
    margin-bottom: $gutter;

Gotcha, it did seem like mobile first was the convention. I think both produce the same effect anyway (like media(tablet) means wider than mobile, and media(mobile) means narrower than mobile?).

Great, I’ll make those changes and clean up the commit history.

[paulwaitehomeoffice]

There we go, the commit history is a bit cleaner now. Thanks so much for your patient help on this.

[selfthinker]

I'm generally happy with this. Great work!

There are still two smaller issues and one bigger issue:

• There shouldn't be any changes in the packages/ folder. That only gets updated with every release, see #467 for more info.
• Minor: &.after-error-summary could do with some comment in the CSS, especially as we couldn't come up with a better name so far. You had a comment in an earlier version, I thought that one was good.
• Minor: You could follow our git styleguide for writing the commit messages. We've merged PRs by external people who don't follow them before. So, I'd be happy to merge without that change. But it's a good practice.

[paulwaitehomeoffice]

@selfthinker Marvellous, thanks so much for all your guidance.

Ah yes I must have changed something in packages by accident, I’ll get that fixed. A comment for &.after-error-summary sounds like a good idea, I’ll put something back in. The commit message style guide sounds like a good idea too, I’ll get those changed.

[paulwaitehomeoffice]

@selfthinker Alrighty, I’ve got rid of the mistaken changes in /packages, added class for .after-error-summary, and tweaked the commit messages to hopefully match the git style guide a little better.

[paulwaitehomeoffice]

Fabulous, thanks, so much for all your patient help. Great to have an official solution for this for our project.