Skip to content

Commit

Permalink
Change options_html to return <option> for 1-item groups
Browse files Browse the repository at this point in the history
Previous versions of Choices.js happily accepted HTML like this:

```html
<optgroup label="">
  <option value="">All organisations</option>
</optgroup>
```

However, in v11+ of Choices.js, that's no longer allowed -
presumably since this change:
Choices-js/Choices#615

The option is now silently dropped from rendering. Choices.js now
seems to expect such HTML to be passed outside of an `<optgroup>`,
i.e. `<option value="">All organisations</option>`.

NB, it is perfectly fine to mix top-level `<option>`s with
`<optgroup>`s:
https://www.w3.org/TR/WCAG20-TECHS/H85.html#:~:text=The%20optgroup%20element%20should%20be,desired%20intent%20when%20using%20this.

This work was made a bit tricky by us having separate `options`
and `grouped_options` parameters, meaning we have to maintain
two separate methods, making behaviour change more difficult.
I had hoped to consolidate onto one single `options` parameter
and have the helper be clever enough to dynamically return
options or optgroups, but in my attempt in 2b643a8
there were so many test failures that this wider rearchitecture
was not worth the time.
  • Loading branch information
ChrisBAshton committed Oct 16, 2024
1 parent a9caa7f commit 1e282b9
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,7 @@ def css_classes
def options_html
if @grouped_options.present?
blank_option_if_include_blank +
grouped_options_for_select(
transform_grouped_options(@grouped_options),
selected_option,
)
grouped_and_ungrouped_options_for_select(@grouped_options)
elsif @options.present?
blank_option_if_include_blank +
options_for_select(
Expand All @@ -54,6 +51,24 @@ def data_attributes
}.compact
end

def grouped_and_ungrouped_options_for_select(unsorted_options)
# Filter out the 'single option' options and treat them as simply `<option>`
# The remainder should be treated as true 'grouped options', i.e. `<optgroup>`
single_options = []
grouped_options = []
unsorted_options.each_with_index do |(group, options), _index|
if group == ""
single_options << options
else
grouped_options << [group, options]
end
end
single_options.flatten!

options_for_select(transform_options(single_options), selected_option) +
grouped_options_for_select(transform_grouped_options(grouped_options), selected_option)
end

private

def transform_options(options)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,8 @@ class SelectWithSearchHelperPresenterTest < PresenterTestCase
)

expected = <<~HTML.strip
<optgroup label="">
<option selected="selected" value="">All organisations</option>
</optgroup>
<optgroup label="">
<option value="foo">All foo</option>
</optgroup>
<optgroup label="Live organisations">
<option value="foo">foo</option>
<option value="bar">bar</option>
Expand Down

0 comments on commit 1e282b9

Please sign in to comment.