alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.17k stars 320 forks source link

"conditional.html" value on radio item has whitespace inserted into it, breaking <textarea> elements #4807

Closed RichardBradley closed 6 months ago

RichardBradley commented 7 months ago

Description of the issue

In our public facing app, we have some radio items which conditionally reveal a text area, for users to input their overseas address, only if they answer the radio question a certain way.

(We are aware that this isn't ideal from an accessibility point of view, and we have recently been unpacking some of these into separate pages, but I don't think that invalidates this bug.)

Since v5.2.0 / commit 6955db326a2e560c9862d29b28e39bd8f4020829, whitespace is added to the html in here by govuk-frontend, which changes the value inside a textarea, corrupting user input.

Steps to reproduce the issue

Here is an extract from the njk template on our page that has caught this bug. I have marked the key line with "HERE". The localisedCharacterCount macro is our wrapper around https://design-system.service.gov.uk/components/character-count/ and contains a <textarea>

{% set bfpoAddressHtml %}
  {% call govukFieldset({
    classes: "govuk-!-width-two-thirds"
  }) %}
    {{ localisedCharacterCount({
      errorMessage: errors['bfpo-address'],
      name: "bfpo-address",
      id: "bfpo-address",
      classes: 'address-textarea',
      value: (form['bfpo-address'] or session.contactAddress.address) if bfpoInitiallySelected,
      label: {
        text: 'Address'
      },
      autocomplete: "street-address",
      hint: {
        text: 'Maximum 5 lines'
      },
      maxlength: 1200,
      threshold: 75,
      isWelshLocale: isWelshLocale
    }) }}

    {{ govukInput({
      label: {
        text: "BFPO"
      },
      id: "bfpo",
      name: "bfpo",
      classes: "govuk-input--width-10",
      value: (form['bfpo'] or session.contactAddress.postcode) if bfpoInitiallySelected,
      errorMessage: errors['bfpo']
    }) }}
  {% endcall %}
{% endset %}

...

{% block articleContent %}
  <form method="post" id="contact-address-form">
    {{ govukRadios({
      idPrefix: "type",
      name: "type",
      fieldset: {
        legend: {
          text: "Where should we write to you about your registration?",
          isPageHeading: true,
          classes: "govuk-fieldset__legend--xl"
        }
      },
      errorMessage: errors["type"],
      items: [
        {
          value: "uk",
          text: ukAddressLine,
          checked: form['type']==='uk' or session.contactAddress.type === 'uk'
        } if showUK,
        {
          value: "bfpo",
          text: 'British Forces Post Office (BFPO) address',
          checked: bfpoInitiallySelected,
          conditional: {
            html: bfpoAddressHtml   /// HERE -- whitespace is added by govuk-frontend
          }
        },
        {
          value: "other",
          text: 'Other address',
          checked: otherInitiallySelected,
          conditional: {
            html: otherAddressHtml
          }
        }
      ] | removeEmpty
    }) }}

    {{ govukButton({
      text: "Continue",
      type: "submit",
      id: "submit-button"
    }) }}
  </form>
{% endblock %}

Actual vs expected behaviour

I would expect the app provided value in conditional.html to be emitted to the page unchanged. In fact, whitespace is added at the start of each line.

Environment (where applicable)

Other notes

I have also commented on this at https://github.com/alphagov/govuk-frontend/pull/4676

It may be the case that other app-supplied param values are being indented and shouldn't be, to avoid similar issues. I haven't checked the rest of the components

RichardBradley commented 7 months ago

This bug is also present in v5.1.0 due to the "indent" on this line:

https://github.com/alphagov/govuk-frontend/blob/v5.1.0/packages/govuk-frontend/src/govuk/components/checkboxes/template.njk#L114

colinrotherham commented 7 months ago

Thanks for raising this @RichardBradley

We should review all HTML placeholders that could contain Textarea components

Textarea content in params.value was left unformatted here for the same reason:

https://github.com/alphagov/govuk-frontend/blob/e92f66ca479bf7c4247cb6ad483f7008e132ac5d/packages/govuk-frontend/src/govuk/components/textarea/template.njk#L50

I'll mention it to the team

RichardBradley commented 7 months ago

I wonder if the recently added/updated approach of properly indenting the emitted HTML should be reviewed entirely?

While it's nice and beneficial for the govuk-frontend source code files themselves to be nicely indented at coding-time, is there really any justification for the templating framework to be re-indenting things at runtime? There will be a small but significant runtime cost of the compute resources spent by millions and millions of web requests to reformat HTML to essentially zero benefit.

devkokov commented 2 weeks ago

This is still an issue when the radios/checkboxes have a fieldset. The indent(2) here affects conditional textareas.