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

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

Open
RichardBradley opened this issue Feb 27, 2024 · 6 comments · Fixed by #4899
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation) nunjucks small story textarea

Comments

@RichardBradley
Copy link

RichardBradley commented Feb 27, 2024

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)

  • Operating system: all
  • Browser: all
  • Browser version: all
  • GOV.UK Frontend Version: v5.1.0, v5.2.0

Other notes

I have also commented on this at #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 RichardBradley added awaiting triage Needs triaging by team 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) labels Feb 27, 2024
@RichardBradley
Copy link
Author

@colinrotherham
Copy link
Contributor

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:

{{- govukAttributes(params.attributes) }}>{{ params.value }}</textarea>

I'll mention it to the team

@RichardBradley
Copy link
Author

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.

@36degrees 36degrees added this to the [next] milestone Mar 11, 2024
@querkmachine querkmachine modified the milestones: v5.3.0, [next] Mar 21, 2024
@36degrees 36degrees moved this from Backlog 🏃🏼‍♀️ to Needs review 🔍 in GOV.UK Design System cycle board Apr 3, 2024
@36degrees 36degrees moved this from Needs review 🔍 to Ready to release 🚀 in GOV.UK Design System cycle board Apr 4, 2024
@owenatgov owenatgov moved this from Ready to release 🚀 to Done 🏁 in GOV.UK Design System cycle board Apr 19, 2024
@devkokov
Copy link

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

@RichardBradley
Copy link
Author

RichardBradley commented Oct 18, 2024

@devkokov , I agree, that looks like the same bug.

I don't have permission to re-open this issue. I suggest you create a new issue referencing this one if that is causing you problems. I have the fix I need for my work, so I don't plan to do any more on this.

If it were up to me, I would remove all the "indent" ops from the runtime code. As I commented above, I don't see why there's any need for the runtime emitted HTML to be pretty-printed, and it can cause bugs which are very hard to detect (as well as a small runtime compute cost that is wasteful).

@querkmachine querkmachine reopened this Oct 18, 2024
@querkmachine querkmachine removed this from the 5.3.1 milestone Oct 18, 2024
@querkmachine querkmachine moved this from Done 🏁 to Backlog 🏃🏼‍♀️ in GOV.UK Design System cycle board Oct 18, 2024
@querkmachine
Copy link
Member

I've reopened this for the time being, just in case a new issue doesn't arise.

@owenatgov owenatgov removed their assignment Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation) nunjucks small story textarea
Projects
Status: Backlog 🏃🏼‍♀️
Development

Successfully merging a pull request may close this issue.

7 participants