Allow resetting the focus on SelectPanel

[alondahari]

We are updating the labels picker in issues to not have a loading state, but instead just add more items from the server as they are loaded. This is causing the previously focused item to still be highlighted even if there are newer, more relevant options:

https://github.com/user-attachments/assets/1a527bf3-7430-4d07-8856-996cd41deea6

I understand from @broccolinisoup that the primer_react_select_panel_with_modern_action_list would solve it, but it's still a bit of a ways out before it's rolled out. She suggested I opened an issue to add an API to update / reset the focus on the list.

[broccolinisoup]

Thanks @alondahari for the issue.

Just additional notes for the triage session.

primer_react_select_panel_with_modern_action_list would solve it,

The flag inherently solves this just because it focuses on the first item not the previous one. I have to mention that though I am not sure if this is intentional. I'll confirm this behaviour with the accessibility team and provide an update here.

She suggested I opened an issue to add an API to update / reset the focus on the list.

We need to first check this with accessibility to ensure this is an accessible pattern then we can triage this.

[siddharthkp]

Added to the SelectPanel feature improvements epic: https://github.com/github/primer/issues/3399, can you help me with the prioritisation. Does the epic have other features that you would like to see ship before this one?

[alondahari]

@siddharthkp thank you for looping us in!

• For prioritising this feature, I would probably say it's top priority for us right now since it's blocking us from improving the label picker experience, and we received a lot of negative feedback on it from our users. Either this or rolling out primer_react_select_panel_with_modern_action_list
• implementing the loading state would also be great, but not a blocker to the above.
• We'd love the create new item support, but we have a (not very good) workaround for now.
• We are also interested in https://github.com/github/primer/issues/4027 for our pickers in the new issue dialog, but we have a workaround of giving the modal a fixed height (not ideal but works for us for now).

This would be also order of priority for us atm.

[siddharthkp]

Thanks for that! I've moved this issue to Priority 1, the exact position is still TBD because some of the others are already work in progress

[broccolinisoup]

We need to first check this with accessibility to ensure this is an accessible pattern

Update: I checked this with accessibility and they recommend resetting the option to the first item. (Slack thread)

Rather than making where to focus optional (first vs previous) we might need to fix the code and make sure it always resets to the first item. Should we mark this as "bug" @siddharthkp ?

[alondahari]

Just one comment, that we should make it so after more items have loaded the focus remains on the same "index" it was before. Unlike the workaround I just shipped, if you already used the keyboard to move down I'd expect the same position after we add more items. Here is how it currently behaves (behind a FF):

https://github.com/user-attachments/assets/301f1086-250f-436c-8884-6657d79cb554

[broccolinisoup]

@alondahari I am probably missing a great deal of context here so I appreciate any information 🙏🏻 - just wanted to ask if we have checked the original behaviour with accessibility before? Regardless where the focus is set, I am curious if loading the options dynamically and updating the list after is any chance considered distracting for both visual and screen reader users 🤔

[siddharthkp]

👋

There are 2 scenarios to consider:

1. Changing all the items when searching for items

• Should show an inline loader (in the input)
• Focus should reset to the first item

2. Loading more items in the background below the previous items

• Focus should stay where it is

Need to verify if this is how it behaves with the feature flag on. If not, that's a bug

---

@alondahari, looking at your video from https://github.com/primer/react/issues/5261#issuecomment-2478285880, it looks like the feature flag is not turned on, is that intentional?

[alondahari]

@siddharthkp sorry my comment above is pretty misleading. The video is with my hacky workaround, just to illustrate why it's not an ideal solution.

• Focus should stay where it is

By "where it is" meaning the same index, not the same item, right? so if more items are added and before that the focus is on the 4th item down, after the items are added I would expect it to still be on the 4th item down, even if it's now a different item.

[broccolinisoup]

@alondahari I'm curious to understand the reasoning behind your expectation that the focus remains on the same index as before. 🤔

I would personally think that it should be the same item (I'll double check with @siddharthkp to clarify his wording) after loading more items in the background. Mainly because after user navigates to a certain element, they might be about to make a selection and if we load new items and move focus to another item, even though the same index, this could be a distracting behaviour.

Also, I wonder if loading new items above the current list is an accessible practise - as I mentioned in my previous comment.

[alondahari]

If we are loading more items above the focused item, we should not keep it focused since it's no longer the most relevant result and probably not close to any relevant result. I might see a possible approach of loading more and keeping only the focused item on top as the first result, but I have a feeling that would lead to some more jarring experience.

@ohiosveryown wdyt should be the behaviour here?

[siddharthkp]

Focus should stay where it is By "where it is" meaning the same index, not the same item, right?

For the scenario i'm mentioning here of loading more items below the current items, it should not matter.

so if more items are added and before that the focus is on the 4th item down, after the items are added I would expect it to still be on the 4th item down, even if it's now a different item.

Sorry, this feels a bit off. Adding items that change the position of items seems a bit rough for user experience 😅 and probably not accessible?

If we are loading more items above the focused item, we should not keep it focused since it's no longer the most relevant result and probably not close to any relevant result.

This sounds like the 1st scenario of filtering items, and in my opinion, we should reset the focus to the first item. This also aligns with the screen reader announcements built into SelectPanel. (See https://github.com/primer/react/pull/4968 for more info)

Might be helpful to get some design and accessibility eyes on this as well

[alondahari]

VSCode just reported this issue as well.

[ohiosveryown]

@ohiosveryown wdyt should be the behaviour here?

...we should reset the focus to the first item. This also aligns with the screen reader announcements built into SelectPanel. (See https://github.com/primer/react/pull/4968 for more info)

This makes the most sense in my mind as well.

I might see a possible approach of loading more and keeping only the focused item on top as the first result, but I have a feeling that would lead to some more jarring experience.

I agree, on the surface this might seem like a feasible solution, but it feels like it might be a little confusing / jarring.

[alondahari]

I talked to @broccolinisoup and we decided to open a bug fix for primer to support the behaviour of always resetting the focus to the first item when we change the items on the list. She will add some pointers / notes to this issue and we'll pick it up as a part of our current initiative.

[github-actions[bot]]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

[alondahari]

@broccolinisoup do you know if this is still planned to any degree?

[broccolinisoup]

@alondahari I don't know. I see it is removed from board but not sure if that was intentional.

@lesliecdubs is it possible to add this to the board again? Thank you 😊

[lesliecdubs]

Hi! @broccolinisoup I see this one is assigned to you as per this comment; is this still a change you were planning to contribute?

I'm also preemptively tagging in the Primer PM @hellojanehere to help coordinate/triage further while I'm OOO.

[broccolinisoup]

Thanks @lesliecdubs 🙏🏻

I didn't notice that it was assigned to me 🙈 Just skimming through the related issue again - I noticed that in the description , enabling primer_react_select_panel_with_modern_action_list solves our problem and now seeing that this flag is staff shipped, I wonder if we would still need this fix. 🤔 @alondahari also mentioned that the behaviour we requested seems to be a bit jarring. I think it is best to re-visit this behaivour.

@alondahari Could you review the current behaviour and report back please if we are happy with this or will still need a further fix?

[siddharthkp]

Hey 👋

Just checking in if this was solved, no rush!

[alondahari]

Hey, so what we did was wrap our component with a FF provider, so now we're just using the modern action list in the ItemPicker. So even though the primer_react_select_panel_with_modern_action_list is staff shipped, we just use it for all users in the context of our pickers.

While this is not the optimal long term solution, it fully alleviated our issues. I will leave it up to you if you want to fix / update the behaviour of the SelectPanel or not.

[siddharthkp]

@alondahari Sounds perfect! It's also shipped to all users now, so we can safely remove the wrapper.

@francinelucca I think we need to clean up the flag on dev portal!

[alondahari]

Sorry what is shipped to all users? which wrapper can we remove?

[siddharthkp]

Sorry! primer_react_select_panel_with_modern_action_list is shipped for everyone (https://github.com/primer/react/pull/6339). We don't have to wrap ItemPicker with <FeatureFlag> anymore :)

@francinelucca Correct me if i'm wrong

[alondahari]

Oh great! @broccolinisoup do you want to remove our wrapper?

[francinelucca]

Sorry! primer_react_select_panel_with_modern_action_list is shipped for everyone (https://github.com/primer/react/pull/6339). We don't have to wrap ItemPicker with <FeatureFlag> anymore :)

That's correct! I believe I already removed the wrapper from ItemPicker whenever we fully shipped the flag. once we remove the existing reference in dotcom, we can safely delete, I'll get on that!

[siddharthkp]

I believe I already removed the wrapper from ItemPicker whenever we fully shipped the flag.

🙃 You already removed it for ItemPicker, that's my bad for not checking main

[francinelucca]

@siddharthkp I removed it and looks like the flag has been deleted from DevPortal, can we close this now?

[siddharthkp]

yep, let's close it. Thank you!