Skip to content

Conversation

@knoxfighter
Copy link
Contributor

This contains a lot of changes and fixes for the Select component to make disabling options properly work (see #169).

  • I had to write custom focus_prev, focus_last, focus_next, focus_first, etc. functions that are independent of the ones in FocusState. This also means that i removed FocusState from the SelectContext component.
  • The options Vec is now a BTreeMap, to make it easy to sort via tabindex.
  • Small aria adjustments
  • Additional tests

I hope this code is up to your standards and the idea is something you are willing to use.

@github-actions
Copy link

@knoxfighter
Copy link
Contributor Author

I forgot to run clippy..
There is just one question: What should i do with FocusState::item_count()? It is not used anymore, but it might be interesting for other components.

@ealmloff
Copy link
Member

What part of the select focus management needs to be different in select vs other components? This PR fixes the issues for select but if we can integrate those fixes into the hook it would be better so it works for the other components

@knoxfighter
Copy link
Contributor Author

knoxfighter commented Dec 29, 2025

Since I decided to move the focus functions directly into the SelectContext, it now uses more data than is theoretically necessary.

The options Vec (now a Map) is used with iterators to find the next element. This could be changed back to a Vec when using the original FocusState, by using its index for access, provided the vector is kept sorted.

Each option in the options map has a disabled field indicating whether it is disabled. This information is required but is not available when using FocusState. This is especially important because I plan to extend this field to also reflect whether an option is filtered based on user input (for a future ComboBox implementation).

I am not sure how this information could be made available in FocusState. One idea would be to add an optional Vec<Signal<bool>> (though that reuires the sorted array index to be synced and each option to know its index) to FocusState. What do you think?

@ealmloff
Copy link
Member

There is a variant of the existing focus hook here that only adds the item to the focus state's list if it isn't disabled. I imagine we could do something similar for select?

@knoxfighter
Copy link
Contributor Author

The FocusState does only hold the amount of possible elements. When that value is lowered by a disabled option, then the last option is not focusable, not the one that is disabled (use_focus_entry_disabled is the function for that),
It might be possible to step over disabled elements in use_focus_control_disabled that runs next/prev again, but that needs an additional field to know the current direction.
And this doesn't ensure that the tab_index is correctly used (aka. the tab index has to start with 0/1 and has to be consecutive).

@ealmloff
Copy link
Member

Feel free to make changes to FocusState, if it has the wrong behavior. The behavior in this PR seems right if we can get it merged into the main hook

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.

2 participants