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

Allow resetting the focus on SelectPanel #5261

Open
alondahari opened this issue Nov 11, 2024 · 9 comments
Open

Allow resetting the focus on SelectPanel #5261

alondahari opened this issue Nov 11, 2024 · 9 comments

Comments

@alondahari
Copy link
Contributor

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:

cap.mov

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
Copy link
Member

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
Copy link
Member

siddharthkp commented Nov 11, 2024

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
Copy link
Contributor Author

alondahari commented Nov 12, 2024

@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
Copy link
Member

siddharthkp commented Nov 12, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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):

te.mov

@broccolinisoup
Copy link
Member

@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
Copy link
Member

siddharthkp commented Nov 20, 2024

👋

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
  1. 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 #5261 (comment), it looks like the feature flag is not turned on, is that intentional?

@alondahari
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants