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

optgroup does not work when some options are nested and some are not #615

Closed
EvanLovely opened this issue Jul 16, 2019 · 4 comments
Closed

Comments

@EvanLovely
Copy link

EvanLovely commented Jul 16, 2019

The library used works if every <option> is in a <optgroup> but has a bug when some are nested and some are not.

This will work:

  <select>
    <optgroup label="UK">
      <option value="London">London</option>
      <option value="Manchester">Manchester</option>
    </optgroup>
    <optgroup label="FR">
      <option value="Paris">Paris</option>
      <option value="Marseille">Marseille</option>
    </optgroup>
  </select>

This will not:

  <select>
    <option value="NYC">New York</option>
    <optgroup label="UK">
      <option value="London">London</option>
      <option value="Manchester">Manchester</option>
    </optgroup>
    <optgroup label="FR">
      <option value="Paris">Paris</option>
      <option value="Marseille">Marseille</option>
    </optgroup>
  </select>

Related: #253

@Geolim4
Copy link

Geolim4 commented Aug 31, 2022

Hello @mtriff, can you take a look at this bug eventually please ? 😭

voidus added a commit to voidus/Choices that referenced this issue Mar 31, 2023
@voidus
Copy link
Contributor

voidus commented Mar 31, 2023

I have started to look into this, but didn't get very far yet. I do have a failing cypress test though, if anyone wants to support: https://github.com/voidus/Choices/tree/fix-mixed-optgroups

Next step: Find the relevant place in the code

voidus added a commit to voidus/Choices that referenced this issue Mar 31, 2023
@voidus
Copy link
Contributor

voidus commented Mar 31, 2023

Okay I've oriented myself a bit in the code and found an approach.

Right now, afaict, the parts that process the options deal with either Choice objects or the html options, and my approach is to convert the html elements to Choice objects first. This should enable us to simplify the processing code.

TBH, I was expecting the change on the branch to already have an effect, but it doesn't look like it.
If anybody wants to continue, please don't hesitate to grab the code!

@mtriff would you be generally open to merge something like this? Otherwise we should probably discuss potential approaches. (Tagging you because you seem to be maintaining the repo)

voidus added a commit to voidus/Choices that referenced this issue Apr 4, 2023
voidus added a commit to voidus/Choices that referenced this issue Apr 4, 2023
Closes Choices-js#615.

This pushes the conversion of OPTION/OPTGROUP elements to Choice objects
into the WrappedSelect class and unifies the code paths a little between
groups-present and groups-not-present.

Some work towards possibly fixing Choices-js#615
voidus added a commit to voidus/Choices that referenced this issue Apr 5, 2023
Closes Choices-js#615.

This pushes the conversion of OPTION/OPTGROUP elements to Choice objects
into the WrappedSelect class and unifies the code paths a little between
groups-present and groups-not-present.

Some work towards possibly fixing Choices-js#615
@voidus voidus mentioned this issue Apr 5, 2023
10 tasks
@voidus
Copy link
Contributor

voidus commented Apr 5, 2023

Yeah folks I think I got it. 🕺
Let's see if this is taking the code in a direction that @mtriff is happy with :)

@Xon Xon mentioned this issue Aug 6, 2024
9 tasks
@Xon Xon closed this as completed Aug 22, 2024
ChrisBAshton added a commit to alphagov/whitehall that referenced this issue Oct 4, 2024
Visiting https://whitehall-admin.publishing.service.gov.uk/government/admin/editions
There is an "All organisations" dropdown, an "All types" documents
dropdown, etc. These options are represented by an `<optgroup>`
with a single `<option>` inside, that option having a blank `""`
value.

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.

From a quick look, it seems any option with a blank value is
dropped.

We need to investigate what changes are needed on our end to have
Choices.js render the "All *" options - made a bit tricky by the
multiple layers of helpers etc that sit between the options list
array and what actually gets passed to Choices.js.
ChrisBAshton added a commit to alphagov/whitehall that referenced this issue Oct 7, 2024
Visiting https://whitehall-admin.publishing.service.gov.uk/government/admin/editions
There is an "All organisations" dropdown, an "All types" documents
dropdown, etc. These options are represented by an `<optgroup>`
with a single `<option>` inside, that option having a blank `""`
value.

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.

From a quick look, it seems any option with a blank value is
dropped.

We need to investigate what changes are needed on our end to have
Choices.js render the "All *" options - made a bit tricky by the
multiple layers of helpers etc that sit between the options list
array and what actually gets passed to Choices.js.
ChrisBAshton added a commit to alphagov/whitehall that referenced this issue Oct 9, 2024
Visiting https://whitehall-admin.publishing.service.gov.uk/government/admin/editions
There is an "All organisations" dropdown, an "All types" documents
dropdown, etc. These options are represented by an `<optgroup>`
with a single `<option>` inside, that option having a blank `""`
value.

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.

From a quick look, it seems any option with a blank value is
dropped.

We need to investigate what changes are needed on our end to have
Choices.js render the "All *" options - made a bit tricky by the
multiple layers of helpers etc that sit between the options list
array and what actually gets passed to Choices.js.

The select component behaves differently depending on whether or
not `grouped_options` or `options` is passed. First step will be
to decide on a common data structure to pass around, and
consolidate onto one `options` parameter - let the component
dynamically infer whether or not an `<optgroup>` is needed.
ChrisBAshton added a commit to alphagov/whitehall that referenced this issue Oct 16, 2024
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.
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 a pull request may close this issue.

4 participants