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(ListBox): update focus states for listbox components #15980

Merged

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Mar 15, 2024

Closes #15698

Updates focus states for Listbox components, as weel as setting focus to the first item in MultiSelect and FilterableMultiSelect

Changelog

New

Changed

  • Focus state when ListBox component is open is now just 1px
  • Focus on first element when menu is opened for MultiSelect and FilterableMultiSelect
  • Updated some tests to account for new behavior

Testing / Reviewing

Ensure focus border on ListBox components goes to 1px when the menu is open, and that the first item is focused when the menu opens with keyboard

Copy link

netlify bot commented Mar 15, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit f20a261
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6605c1d4b172b50007f8029d
😎 Deploy Preview https://deploy-preview-15980--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.

Hi @tw15egan, have you modified the focus state for dropdowns and comboboxes? 🤔
I'm not certain if it's a bug, but I'm unable to view the updated focus state in the multi-select and filterable multi-select options...

@tw15egan
Copy link
Collaborator Author

@Kritvi-bhatia17 Yes, they have all been updated and I am able to see the changes when looking at the latest deploy preview:

@Kritvi-bhatia17
Copy link
Contributor

Kritvi-bhatia17 commented Mar 25, 2024

Hi @tw15egan,

@Kritvi-bhatia17
Copy link
Contributor

Heyy @tw15egan! Could you please add Michael and Lauren as reviewers once you've updated everything?

@tw15egan tw15egan requested review from mbgower and laurenmrice March 28, 2024 18:02
@tw15egan
Copy link
Collaborator Author

@Kritvi-bhatia17 changes have been made and the deploy preview should be updated in a few minutes 👍🏻

@mbgower
Copy link

mbgower commented Mar 28, 2024

The focus handling on dropdown and combobox looks good.

This may not have been introduced here, but I was a little surprised to see the list in combobox wrapping around (i.e., when I got to the bottom item, it wrapped to the top item). This isn't a menu, so I was a little surprised to see that happen. I would call that atypical for this kind of function. Like I said, if that wasn't introduced in this change, this is probably a new issue to explore.

@mbgower
Copy link

mbgower commented Apr 1, 2024

I had a look at the pre-existing version of this component and that 'wraparound' keyboard is doing it there too, so it’s not related to the changes here.
I think I would treat this an a new issue. I don’t think this is necessarily a failure, but I do think it warrants some discussion: when should and shouldn’t a dropdown list have wrap-around keyboard interaction from the bottom to the top item in a dropdown list? My sense is that this is possibly another defining behaviour difference between select-like elements and menu-like elements (where select doesn't wraps and menu does).

@Kritvi-bhatia17
Copy link
Contributor

Damn, nice point @mbgower! I totally agree with you and will create a separate issue for that.

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.

Awesome work, @tw15egan! You nailed those changes super quickly. ⚡️

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

looks good to merge!

@andreancardona andreancardona added this pull request to the merge queue Apr 3, 2024
Merged via the queue into carbon-design-system:main with commit 801dd7b Apr 3, 2024
20 checks passed
preetibansalui pushed a commit to tay1orjones/carbon that referenced this pull request Apr 24, 2024
…gn-system#15980)

* fix(ListBox): update focus states for listbox components

* fix(ListBox): update fluid listbox components

* test(Multiselect): update keybaord nav tests

* fix(Multiselect): remove console.log

* test(FluidMultiselect): update tests

* fix(ComboBox): adjust mouse interactions

* fix(Dropdown): adjust focus state on mouse leave

* fix(MultiSelect): match mouse focus to keyboard,  home/end interaction

* fix(FilterableMultiselect): align keyboard, mouse focus interaction

* style(ListBox): adjust outline offset when outline is only 1px

* chore(console): remove console.log

* chore(Multiselect): revert testing default state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dropdown: Combobox & Multiselect]: Focus state dev implementation
6 participants