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.

Sign up for GitHub

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

Jump to bottom

Fix SelectNext focus inconsistencies when mixing mouse and keyboard interaction #1292

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Fix SelectNext focus inconsistencies when mixing mouse and keyboard interaction #1292

merged 1 commit into from
Oct 5, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Oct 4, 2023 •
edited
Loading

Closes #1285

This PR addresses the two problems left in SelectNext, both related with option focusing.

  • Now, the focus ring is visible for the first option in the listbox when open. The option was previously focused already, but the style was applied to :focus-visible and not :focus, which prevents the UA from applying the styles. See first example in https://developer.mozilla.org/en-US/docs/Web/CSS/:focus-visible#examples
  • When an option is clicked, we now make sure it is focused just before closing the listbox, becoming the last focused element. This ensures it is focused when the listbox is opened again, regardless of how the last option was selected (via mouse or keyboard).

In order to fix the first issue, options are no longer Button components, but simple li where we have to override less styles and we can apply focus rings to :focus instead of :focus-visible.

This also means the listbox is now a ul instead of a div.

@acelaya acelaya requested a review from robertknight October 4, 2023 14:43
acelaya
acelaya commented Oct 4, 2023
View reviewed changes
src/hooks/use-arrow-key-navigation.ts
@@ -94,6 +94,10 @@ export function useArrowKeyNavigation(
const lastFocusedItem = useRef<HTMLElement | null>(null);

useEffect(() => {
if (!containerVisible) {
return () => {};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not strictly related with the other changes on this PR, but I realized we can safely skip any action when containerVisible is false, making the rest of the logic easier to reason about.

acelaya
acelaya commented Oct 4, 2023
View reviewed changes
src/hooks/use-arrow-key-navigation.ts
const elements = getNavigableElements();
const targetIndex = elements.indexOf(event.target as HTMLElement);
if (targetIndex >= 0) {
updateTabIndexes(elements, targetIndex);
updateTabIndexes(elements, targetIndex, autofocus);
Copy link
Contributor Author

@acelaya acelaya Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is what was causing the options to not be set as lastFocusedItem when being clicked instead of selected with the keyboard.

@acelaya acelaya mentioned this pull request Oct 4, 2023
Closed
7 tasks
@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch from 1468463 to be01f16 Compare October 5, 2023 07:50
@codecov
Copy link

codecov bot commented Oct 5, 2023 •
edited
Loading

Codecov Report

Merging #1292 (164a063) into main (f3ac2d1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1292   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           62        62           
  Lines          911       916    +5     
  Branches       350       351    +1     
=========================================
+ Hits           911       916    +5
Files Coverage Δ
src/components/input/SelectNext.tsx 100.00% <100.00%> (ø)
src/hooks/use-arrow-key-navigation.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch from be01f16 to e4e7fa8 Compare October 5, 2023 08:41
@acelaya
Copy link
Contributor Author

acelaya commented Oct 5, 2023

There's a line not covered, which should be when containerVisible changes and the effect is re-triggered. However, the test is not changing this and I'm trying to figure out why.

I'll put this in draft state for now, just in case this ends up having deeper implications.

@acelaya acelaya marked this pull request as draft October 5, 2023 08:49
@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch 2 times, most recently from 80ca5e9 to c03227d Compare October 5, 2023 09:08
@acelaya
Copy link
Contributor Author

acelaya commented Oct 5, 2023

There's a line not covered, which should be when containerVisible changes and the effect is re-triggered. However, the test is not changing this and I'm trying to figure out why.

I'll put this in draft state for now, just in case this ends up having deeper implications.

Ok, I was missing some act calls to make sure side effects were triggered when clicking a button.

@acelaya acelaya marked this pull request as ready for review October 5, 2023 09:09
@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch from c03227d to 9c2adf6 Compare October 5, 2023 09:15
@acelaya acelaya mentioned this pull request Oct 5, 2023
Merged
@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch 2 times, most recently from 0e01213 to 1b974d1 Compare October 5, 2023 13:19
acelaya
acelaya commented Oct 5, 2023
View reviewed changes
src/components/input/SelectNext.tsx
if (e.key === 'Enter') {
selectValue(value);
}
}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was transparently handled by Buttons, but needs to be explicitly added for lis.

@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch from 1b974d1 to e8ab7bd Compare October 5, 2023 13:24
robertknight
robertknight reviewed Oct 5, 2023
View reviewed changes
Copy link
Member

@robertknight robertknight left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just two minor things I spotted with the change to <li>:

  • Space should select the item as well as Enter (I'm not sure if there is a more general way of getting the keys which have activation behavior)
  • The cursor should be set to pointer

Otherwise this looks good to me.

src/components/input/SelectNext.tsx Outdated
{ 'hover:bg-grey-1': !disabled },
classes,
)}
onClick={() => selectValue(value)}
onKeyPress={e => {
if (e.key === 'Enter') {
Copy link
Member

Choose a reason for hiding this comment

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

Also the Space key should select the item.

@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch from e8ab7bd to 98bc0d0 Compare October 5, 2023 15:29
@acelaya acelaya requested a review from robertknight October 5, 2023 15:31
@acelaya acelaya force-pushed the select-next-mouse-keyboard-mix branch from 98bc0d0 to 164a063 Compare October 5, 2023 15:32
@acelaya acelaya merged commit c7dc992 into main Oct 5, 2023
4 checks passed
@acelaya acelaya deleted the select-next-mouse-keyboard-mix branch October 5, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertknight robertknight robertknight approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix issues found after using SelectNext in the open
2 participants
@acelaya @robertknight