hmrc / play-frontend-hmrc

Apache License 2.0
13 stars 11 forks source link

SelectItem with empty text produces invalid HTML5 #223

Open pangiole-hmrc opened 1 year ago

pangiole-hmrc commented 1 year ago

Symptoms

The following example of usage:

import uk.gov.hmrc.govukfrontend.views.viewmodels.select.{Select, SelectItem}

val select = Select(
  id = "example",
  items = Seq(
    Select(text = "")
    Select(value="1", text = "hello")
  )
)

would produce the following HTML fragment:

<select id="example">
  <option value=""></option>
  <option value="1">hello</option>
</select>

which, when fed into an accessibilty tester, would report the following HTML5 validatation error

Element “option” without attribute “label” must not be empty.

Diagnose

By Googling around, I understood that each <option> element must have not only a value but also a display name. That name can be specified either as text content (such as hello) or via the label attribute.

The above HTML5 fragment is invalid because of the first <option> element. Accoding to the HTML5 specs, there would be no way to derive the display name because: (1) the text content is given as empty and (2) at the same time there's no additional label attribute .

Quick and dirty solution

Just pass the &nbsp; value as text content in Scala code:

import uk.gov.hmrc.govukfrontend.views.viewmodels.select.{Select, SelectItem}

val select = Select(
  id = "example",
  items = Seq(
    Select(value="1", text = "hello"),
    Select(text = "&nbsp;")
  )
)

that seems to trick the validator into believing the HTML code is valid. Also, it does not seem to break the selection widget on screen.

Clean solution

???

oscarduignan commented 1 year ago

Hi @pangiole-hmrc, apologies, totally missed this! We have a few places where we've added some @require() assertions on parameters to prevent invalid states (like stopping people from marking two radio buttons as checked at the same time.) This might be a good place for something like that - I will put it on our tech debt board internally and flag it with our product owner

We also have some helper methods for building up the parameters required by components, so I could imagine paired with an assertion to prevent the invalid state, we could have a helper method that was something like Select().withAnEmptyOptionAtStart() or something like that, to save people having to remember what a valid empty option looks like

I'm not sure what the impact to users would be in terms of accessibility though, even if it is invalid html, it might be that browsers are mostly able to accomodate this - so I can't say how quickly it will be prioritised amongst other work. We have to be careful with additions like this that narrow the scope of allowable parameters because they are technically breaking changes that might be missed on big services and lead to worse user experience / accessibility than what we're trying to avoid