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

fix(Multiselect): remove focus when MultiSelect is open #16259

Merged

Conversation

tw15egan
Copy link
Member

Closes #16252
Closes #16257

Removes focus from MultiSelect when the dropdown menu is opened. The focus should return to the input when it is closed.

This also adds a distinct FilterableMultiselect story under FluidMultiSelect for consistency with the non-fluid Multiselect story

Changelog

New

  • Filterable story under Fluid components --> FluidMultiselect

Changed

  • Only adds focus to the Multiselect button when the input is closed

Testing / Reviewing

Open the Multiselect; there should be no focus border around the input, only the item. Ensure the Filterable variant of FluidMultiselect is discoverable

@tw15egan tw15egan requested a review from a team as a code owner April 24, 2024 16:34
Copy link

netlify bot commented Apr 24, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 04c771d
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6630a2af6f36a40008821bc4
😎 Deploy Preview https://deploy-preview-16259--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@Kritvi-bhatia17 Kritvi-bhatia17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DAMN, @tw15egan, that was quick! 🔥

  1. Sorry, I might not have been clear about the focus state explanation (both for default and fluid multi-select).

image

  1. What was the initial thought behind "With Initial Selected Item" showing this upfront?

  2. Should we also introduce Filterable With Layer under fluid multi-select as well?

image

@tw15egan
Copy link
Member Author

tw15egan commented Apr 24, 2024

@Kritvi-bhatia17 We keep the focus on FilterableMultiselect and Combobox when an item is selected because a user can still type in the input. What affordance does the added input focus give a user when an item is selected vs not? It gains "focus" but doesn't gain any new functionality

#2 is just an example of using the initialSelectedItems prop / ensures it is working properly

#3 Sure I can add that 👍🏻

@Kritvi-bhatia17
Copy link
Contributor

@Kritvi-bhatia17 We keep the focus on FilterableMultiselect and Combobox when an item is selected because a user can still type in the input. What affordance does the added input focus give a user when an item is selected vs not? It gains "focus" but doesn't gain any new functionality

#2 is just an example of using the initialSelectedItems prop / ensures it is working properly

#3 Sure I can add that 👍🏻

#1 In the selected multi-select, as a user can clicks on the 2 X tag (actionable), that's why a focus state is applied to the parent element.

@tw15egan
Copy link
Member Author

@Kritvi-bhatia17 Focus states are usually added as an affordance for keyboard users, not mouse users, but I can add this if you would like. We don't have any of these "double" focus states outside the ListBox components, so we are setting precedence here that may need to be expanded to other components in the repo. When the menu is closed, a keyboard user will clear the selection by pressing the escape key when the focus is on the input. Focus used to go to the x icon directly, but that was removed in #13173

@tw15egan
Copy link
Member Author

@Kritvi-bhatia17 updated 👍

@Kritvi-bhatia17
Copy link
Contributor

@Kritvi-bhatia17 Focus states are usually added as an affordance for keyboard users, not mouse users, but I can add this if you would like. We don't have any of these "double" focus states outside the ListBox components, so we are setting precedence here that may need to be expanded to other components in the repo. When the menu is closed, a keyboard user will clear the selection by pressing the escape key when the focus is on the input. Focus used to go to the x icon directly, but that was removed in #13173

Hi @tw15egan! Thanks for sharing this with me. I'm not entirely sure about this, so I'll reach out to Lauren to get a little more context.

Copy link
Contributor

@Kritvi-bhatia17 Kritvi-bhatia17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Great work @tw15egan!! 🔥

@Kritvi-bhatia17
Copy link
Contributor

@Kritvi-bhatia17 Focus states are usually added as an affordance for keyboard users, not mouse users, but I can add this if you would like. We don't have any of these "double" focus states outside the ListBox components, so we are setting precedence here that may need to be expanded to other components in the repo. When the menu is closed, a keyboard user will clear the selection by pressing the escape key when the focus is on the input. Focus used to go to the x icon directly, but that was removed in #13173

Hi @tw15egan, I connected with Lauren to confirm about this and she said that:
While the point you're making is valid, but we we are adding this because of a11y reasons. We show focus on the first item in the menu for Menu, and same with Combo button and Overflow menu, and those were a11y decisions. So it seems like we are kind of doing this across multiple components in our system already.

@alisonjoseph alisonjoseph added this pull request to the merge queue May 1, 2024
Merged via the queue into carbon-design-system:main with commit b348878 May 1, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants