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

keyboard navigation : feature/kb open dropdowns #595

Merged
merged 14 commits into from
Nov 13, 2024

Conversation

interim17
Copy link
Contributor

@interim17 interim17 commented Oct 21, 2024

Time estimate or Size

medium

Problem

Closes #593, advances #347

Opening and closing dropdown menus with the keyboard.

Solution

Does not address all aspects of keyboard navigating dropdowns, just the most basic interactions.
Load Model or Help are focused:

  • enter/spacebar/down arrow open the dropdown menu
  • escape closes the menu and returns focus to button

tab and shift+tab while the menu is open, and navigating within the menu, are not in scope.

NavButton gets wrapped in forwardRef to make sure that the <button> can be targeted with focus() when needed.

Type of change

  • New feature (non-breaking change which adds functionality)

Steps to Verify:

  1. Tab over to the Load model and Help buttons.
  2. Try opening menus with enter/space/downarrow, then closing with escape.

styles[buttonType],
{ [styles.disabled]: isDisabled }
);
const NavButton = forwardRef<HTMLButtonElement, NavButtonProps>(
Copy link
Contributor Author

@interim17 interim17 Oct 21, 2024

Choose a reason for hiding this comment

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

The only real changes in this file are here, 26, and 38 where we are forwarding the ref to the <button>

@interim17 interim17 changed the title Feature/kb open dropdowns keyboard navigation: : feature/kb open dropdowns Oct 23, 2024
@interim17 interim17 changed the title keyboard navigation: : feature/kb open dropdowns keyboard navigation : feature/kb open dropdowns Oct 23, 2024
Base automatically changed from feature/focus-states to main October 29, 2024 23:40
Copy link

github-actions bot commented Nov 1, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
66.92% (+0.16% 🔼)
696/1040
🟡 Branches
67.1% (+0.43% 🔼)
104/155
🔴 Functions
35.5% (+0.25% 🔼)
93/262
🟡 Lines
65.3% (+0.18% 🔼)
619/948

Test suite run success

129 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from e34e4c7

@interim17 interim17 marked this pull request as ready for review November 1, 2024 16:26
@interim17 interim17 requested a review from a team as a code owner November 1, 2024 16:26
@interim17 interim17 requested review from toloudis, tyler-foster and ShrimpCryptid and removed request for a team and toloudis November 1, 2024 16:26
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

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

Left some comments, all non-blocking!

Comment on lines 77 to 78
event.preventDefault();
setDropdownState(DropdownState.FORCED_OPEN); // Opened by keyboard
Copy link
Contributor

Choose a reason for hiding this comment

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

May need to also clear closeTimeoutRef here. I think it's possible to mouse over the menu quickly while it's closed, which would start a close timer, and then use the keyboard to click on the menu to get it into the FORCED_OPEN state. Sincere there's no check for FORCED_OPEN when the timeout fires, it would suddenly close the menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is true, and also made me realize that focus was being captured in that scenario because of autoFocus.

This implies some things to deal with in the next PR, but for now removing autoFocus, clearing the timeout here, also running this anytime the state is not FORCED_OPEN is doing the trick.

};

const handleMouseLeaveWithDelay = () => {
if (dropdownState !== DropdownState.FORCED_OPEN) {
Copy link
Contributor

Choose a reason for hiding this comment

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

or dropdownState === DropdownState.OPEN

const buttonClickHandler = () => {
setDropdownState(
dropdownState === DropdownState.CLOSED
? DropdownState.OPEN
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be FORCED_OPEN, or is that only for keyboard-based clicking?

Copy link
Contributor Author

@interim17 interim17 Nov 6, 2024

Choose a reason for hiding this comment

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

Yes, I'm only using FORCED_OPEN only for keyboard based clicking at this point

@interim17 interim17 merged commit 88eb4d6 into main Nov 13, 2024
6 checks passed
@interim17 interim17 deleted the feature/kb-open-dropdowns branch November 13, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open and close dropdown menus with keyboard
3 participants