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

Control ArrowUp for Dropdown #18324

Merged
merged 5 commits into from
Jan 24, 2025
Merged

Control ArrowUp for Dropdown #18324

merged 5 commits into from
Jan 24, 2025

Conversation

cknabe
Copy link
Contributor

@cknabe cknabe commented Jan 11, 2025

Closes #18277

  • Only allow ArrowUp if the Dropdown is already open
  • Allow all other key presses regardless if open or closed

Testing

  1. Open up the Storybook for this PR
  2. On the left under Components, expand the Dropdown component entry
  3. Select the Default Dropdown from the expanded Dropdown component options in the left nav
  4. Tab to the Dropdown component that appears in the content section on the right
  5. Press the Up arrow key on your keyboard
  6. The Dropdown should not open at all
  7. After confirming that the Dropdown did not open, press the Down arrow key on your keyboard
  8. The Dropdown component should open and you should see the following:
    image
  9. Press the Up arrow key on your keyboard and you should see the Lorem, ipsum dolor sit amet consectetur adipisicing elit. option become the highlighted option.
    image
  10. Press the Enter key on your keyboard, and the Dropdown should close and you should see the following:
    image

@cknabe cknabe requested a review from a team as a code owner January 11, 2025 05:28
@cknabe cknabe requested review from tay1orjones and emyarod January 11, 2025 05:28
Copy link

netlify bot commented Jan 11, 2025

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit ee37955
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/67928c8e6b97da0008e05215
😎 Deploy Preview https://deploy-preview-18324--v11-carbon-web-components.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

netlify bot commented Jan 11, 2025

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit ee37955
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/67928c8e1b32510008ba6194
😎 Deploy Preview https://deploy-preview-18324--carbon-elements.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

netlify bot commented Jan 11, 2025

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

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

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.16%. Comparing base (eee39f8) to head (ee37955).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #18324   +/-   ##
=======================================
  Coverage   84.15%   84.16%           
=======================================
  Files         408      408           
  Lines       14443    14445    +2     
  Branches     4641     4693   +52     
=======================================
+ Hits        12155    12158    +3     
+ Misses       2124     2122    -2     
- Partials      164      165    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@annawen1
Copy link
Member

annawen1 commented Jan 13, 2025

Hi @cknabe, trying to follow the ticket linked in the corresponding bug issue - are the rules listed in your PR description coming from another issue?

Disallows ArrowUp if the first item is selected.
Disallows ArrowDown if the last item is selected.
Allows all key-presses when the Dropdown is open
Allows all key-presses that are not ArrowUp or ArrowDown

@cknabe
Copy link
Contributor Author

cknabe commented Jan 13, 2025

@annawen1 No, #18277 states this:

The closed default & fluid dropdown input should not open when the user presses the Up arrow key, as unlike the combobox, there is no ability to wrap around the options list (i.e., pressing the Up arrow at the top of the open list does nothing).

This PR causes the Dropdown to not open when the user is at the top selection and they press the up arrow key.
When the user is at the bottom selection and presses the down arrow key, it also does not open.

@annawen1
Copy link
Member

annawen1 commented Jan 13, 2025

@annawen1 No, #18277 states this:

The closed default & fluid dropdown input should not open when the user presses the Up arrow key, as unlike the combobox, there is no ability to wrap around the options list (i.e., pressing the Up arrow at the top of the open list does nothing).

This PR causes the Dropdown to not open when the user is at the top selection and they press the up arrow key. When the user is at the bottom selection and presses the down arrow key, it also does not open.

I see, I had understood the issue to mean that the Dropdown should not open at all on arrowUp regardless of which item is selected... @Kritvi-bhatia17 could you clarify? And on the disabling of opening Dropdown on arrowDown when last item is selected?

@guidari
Copy link
Contributor

guidari commented Jan 14, 2025

Thanks for opening that @cknabe
It would be nice to coverage the changes with Jest tests.

@cknabe
Copy link
Contributor Author

cknabe commented Jan 14, 2025

I'll wait for @Kritvi-bhatia17 to comment before implementing the tests. I did make an assumption that the only reason the arrow up should be disabled is if it is already the top selected item, since Dropdown does not wrap. In the same way, I assumed the arrow down should not open if at the last selected item, because Dropdown does not wrap. I concluded that if a middle item is selected, then arrow up/arrow down should work, since you can move up or down in the list from that position.

@tay1orjones
Copy link
Member

@mbgower could you re-clarify here the changes you were wanting to see wrt arrowup/down keypresses on Dropdown?

@Kritvi-bhatia17
Copy link
Contributor

Hi team!
Thanks for opening that, @cknabe.
Based on my understanding of what you've mentioned, @annawen1, it seems correct that the dropdown should not open on arrowUp, regardless of which item is selected. However, I’d still request @mbgower to clarify further, especially regarding the latter part of the question.

I see, I had understood the issue to mean that the Dropdown should not open at all on arrowUp regardless of which item is selected... @Kritvi-bhatia17 could you clarify? And on the disabling of opening Dropdown on arrowDown when last item is selected?

@mbgower
Copy link

mbgower commented Jan 20, 2025

@mbgower could you re-clarify here the changes you were wanting to see wrt arrowup/down keypresses on Dropdown?

@tay1orjones I think this PR wording is making things a bit more complicated than need be. I merely stated that the default Dropdown component should not open on up arrow since the arrow key behaviour when the component is open is that arrow keys do not wrap from bottom to top (or vice versa).

There is NO need to alter any behaviour for down arrow. It is working as expected.

I also do not think the list of 7 test steps is clear or correct.


Since there seems to be some confusion in this PR, I will say for added context that the only one of the variants of Dropdown that wraps with arrow keys is Combobox -- and it also opens on up arrow to match that behaviour. Given combobox can have a LOT of options, that seems reasonable to me (and it's supported by the APG which lists it as optional and uses that behaviour in its demos). We have that documented as such on the a11y tab. So everything for combobox seems to be as expected. Combobox should NOT be changed by this PR.

Further, everything for multi-select seems correct already. It doesn't open on up arrow. The default dropdown behaviour should match this variant.

I'd also say that this should be regarded as a low priority change. It's hard imagining anyone being significantly affected by up arrow opening a dropdown. It's just a tweak to make Carbon components more consistent in behaviour.

@cknabe cknabe changed the title Control ArrowUp and ArrowDown keys for Dropdown Control ArrowUp for Dropdown Jan 22, 2025
Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

Based on the conversations this LGTM 🚀
It just have to fix a small conflict.
Thanks for working on this!

@annawen1 annawen1 added this pull request to the merge queue Jan 24, 2025
Merged via the queue into carbon-design-system:main with commit 5ec40d6 Jan 24, 2025
37 checks passed
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.

[Bug]: Dropdown shouldn't open with Up arrow key press
6 participants