Fix selected choice was not reliably highlighted when opening the dropdown

[zoglo]

Description

This one was on my list for a while but I had to check where to apply this one as easy as possible.
Basically this highlights the first selected choice in the dropdown on init / rendering of the items

This will fix all of these (as they are duplicates)

• Highlighting and selecting first item from store programatically #586
• Selected item is not the highlighted item when initializing the select #850
• selected Chioce is not highlighted #1290

Types of changes

• Chore (tooling change or documentation change)
• Refactor (non-breaking change which maintains existing functionality)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• My code follows the code style of this project.
• I have added new tests for the bug I fixed/the new feature I added.
• I have modified existing tests for the bug I fixed/the new feature I added.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[zoglo]

Unsure but the tests failing seem to be unrelated (Screenshots differing by a pixel)

[Xon]

_renderChoices already does call _highlightChoice() when it needs to update the selected choice but I think the logic is wrong.

I think a boolean check on when the selectableChoices variable set isn't correct

[zoglo]

_renderChoices already does call _highlightChoice() when it needs to update the selected choice but I think the logic is wrong.
I think a boolean check on when the selectableChoices variable set isn't correct

You are right that it could be improved upon but it's not a boolean check, it has just never been accounted for as explained below:

1. When creating the structure, we already reset highlight position to 0

   Choices/src/scripts/choices.ts

   Line 2287 in d6b4d50

   this._highlightPosition = 0;

2. Due to not passing an element initially here, we run into the next logic whereas the highlight position is 0 (see comment: We never pass an element initially)

   Choices/src/scripts/choices.ts

   Line 1056 in d6b4d50

   this._highlightChoice();

   
   

   Choices/src/scripts/choices.ts

   Lines 2061 to 2071 in d6b4d50

   	if (passedEl) {
   	this._highlightPosition = choices.indexOf(passedEl);
   	} else {
   	// Highlight choice based on last known highlight location
   	if (choices.length > this._highlightPosition) {
   	// If we have an option to highlight
   	passedEl = choices[this._highlightPosition];
   	} else {
   	// Otherwise highlight the option before
   	passedEl = choices[choices.length - 1];
   	}

So my best bet right now is to set the highlightPosition based on the type in _createStructure:

this._highlightPosition = passedElement instanceof WrappedSelect ? passedElement.element.selectedIndex : 0;

I did update the PR c663c6f according to my explanation, the caveat being that this would not work for options that were sorted aftewards or through data-attributes or anything else but that could be a known limitation and the highlighted choice should be updated with own logic after a custom sort.

Mind that the behavior will already account for no items selected due to choices.length > this._highlightPosition

[Xon]

This does look like it will fix the initial highlighted items, but might not handle the setChoiceByValue

I will need to run some tests and doublecheck this, as the logic is on the more complex side. I'll probably setup some new e2e tests to cover this behavior

[Xon]

Yuck, the legacy placeholder (<option value="">This is a placeholder</option>) can break this change :(

[zoglo]

Hmm... how about passing the element from the store within the render method? Or is it the same case here?

[zoglo]

Not using the selectedIndex, we could pass the actual one from the store as initially done in b5019f9 but here:

Choices/src/scripts/choices.ts

Line 1056 in d6b4d50

this._highlightChoice();

With that, if there is no selected choice in the store, it would still pass a null value, leading to the same behavior as before:

this._highlightChoice(this._store.items[0]?.choiceEl);

It would also preselect the custom options and handle all the other cases.

[Xon]

I've tinkered reproducing the linked issues, and I believe this fixes the most cases.

Adding to renderChoices means we can select the first selected item regardless of sorting and other output behavior, and then pass the element to _highlightChoice. The selectableChoices variable can goes away as it is redundant

[Xon]

Refactoring selectableChoices broke something, back to more tinkering :(

[zoglo]

Yeah, latest change will also work and seems way better to me than the store solution, I do however think that 605ca42 will break it again, most likely due to this line
if ((isSearching || !choice.disabled) && !highlightedEl)

Looks like you've already seen that 🙈

[Xon]

This is looking better now, and should address the caveats from the previous solution.

I think this definitely needs some E2E tests to validate the behavior so it stays fixed. I should have some time for that tomorrow-ish, but if you want to take a stab at it that would be good for someone else to contribute to those playwright tests

[zoglo]

I think this definitely needs some E2E tests to validate the behavior so it stays fixed. I should have some time for that tomorrow-ish, but if you want to take a stab at it that would be good for someone else to contribute to those playwright tests

Got some time on my hand today so might look into these but can't promise🤞.
I really appreciate your help, but please don’t feel too much pressure to take this on yourself @Xon, you are already helping me so much.

[zoglo]

The latest commit 305a925 should fix the broken search that was caught by the tests (if e.g. choice 2 was selected before, it would have always selected the second position of the result list...).

So we should only ever have a highlightedEl when we are

• not searching AND if a selected option exists
• (and if we didn't already save a highlighted item) -> See Fix selected choice was not reliably highlighted when opening the dropdown #1339 (comment)

---

Also pushed the current main into the branch as there have already been merge conflicts due to the modified screenshots.

Mind that I updated the demo-page-test as the newly selected option in the dropdown wasn't accounted for before 3e8e5d7:

• before : Starting from option 1, going 2 down, expecting option 3
  

• now : Starting from option 3, going 1 down, expecting option 4
  

With that, everything should work as expected, tests for single-select and multi-select will be added as well.

[zoglo]

Added e2e tests for single-select in 09e0eb6 and multi-select in 7262fa2.

[zoglo]

This is commented out as the current implementation cannot highlight multiple elements right now.

We could remove the check for else if (!highlightedEl), rename it to highlightedElements and either run _highlightChoice multiple times or change it that it would allow an array.