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

Omit the value attribute from select options with no value #3773

Merged
merged 3 commits into from
Jun 9, 2023

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented Jun 9, 2023

In HTML, if you omit the value attribute from an <option> the value defaults to the text contained inside the element.

However if you omit the value from an option when using the govukSelect macro, we currently include an empty value attribute on the <option> element. This would mean that when submitting a form using a select with options with no value set, the value submitted would be the empty string.

Instead, omit the value attribute if no value option has been provided. We then mimic the default HTML behaviour, as the option value will then naturally fall back to the text content.

We still need to ensure that the value attribute is included when the value option is passed but has a falsey value (like an empty string, 0, or boolean false).

Also allow the top-level value option on a govukSelect to work with options that have no explicit value set by comparing it to the text content instead.

Fixes #3440.

In HTML, if you omit the `value` attribute from an `<option>`the value defaults to the text contained inside the element [1].

However if you omit the value from an option when using the `govukSelect` macro, we currently include an empty value attribute on the `<option>` element. This would mean that when submitting a form using a select with options with no value set, the value submitted would be the empty string.

Instead, omit the `value` attribute if no `value` option has been provided. We then mimic the default HTML behaviour, as the option value will then naturally fall back to the text content.

We still need to ensure that the `value` attribute is included when the value option is passed but has a falsey value (like an empty string, 0, or boolean false).

Fixes #3440.

[1]: https://html.spec.whatwg.org/multipage/form-elements.html#the-option-element:~:text=The%20value%20attribute%20provides%20a%20value%20for%20element.%20The%20value%20of%20an%20option%20element%20is%20the%20value%20of%20the%20value%20content%20attribute%2C%20if%20there%20is%20one%2C%20or%2C%20if%20there%20is%20not%2C%20the%20value%20of%20the%20element%27s%20text%20IDL%20attribute.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3773 June 9, 2023 10:29 Inactive

const $firstItem = $('.govuk-select option:first-child')
// Ideally we'd test for $firstItem.attr('value') == undefined but it's
// broken in Cheerio – https://github.com/cheeriojs/cheerio/issues/3237
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

it('selects options with implicit value using selected value', () => {
const $ = render('select', examples['without values with selected value'])

const $selectedItem = $('option:contains(\'Green\')')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint should be happy with double quotes when they're intentionally nesting single quotes if you like

Suggested change
const $selectedItem = $('option:contains(\'Green\')')
const $selectedItem = $("option:contains('Green')")

@colinrotherham
Copy link
Contributor

@36degrees Do we need a CHANGELOG entry for this should anyone rely on the old behaviour?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3773 June 9, 2023 12:17 Inactive
Allow the top-level `value` option on a `govukSelect` to work with options that have no explicit value set.

The value of an option element when there is no `value` attribute is the text content of the element, so use that instead.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3773 June 9, 2023 12:19 Inactive
@36degrees
Copy link
Contributor Author

Do we need a CHANGELOG entry for this should anyone rely on the old behaviour?

D'oh! I'd added it but not pushed. Have applied your suggestion and updated.

Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good still 😊

@36degrees 36degrees merged commit e75fc42 into main Jun 9, 2023
21 checks passed
@36degrees 36degrees deleted the select-without-value branch June 9, 2023 15:53
colinrotherham pushed a commit that referenced this pull request Jul 19, 2023
Omit the value attribute from select options with no value
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibly unexpected behaviour if you provide text but no value for select items
3 participants