-
Notifications
You must be signed in to change notification settings - Fork 2
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
Improve SelectNext arrow key navigation #1290
Conversation
839f9ee
to
a8cc3d5
Compare
38a2240
to
63cd41a
Compare
Hmm, I'm seeing some odd behavior in Safari when testing in http://localhost:4001/input-select-next:
If you press Enter to select an item, the control behaves as if the expected item had been focused given the number of arrow presses, so it looks like this might be an issue with the focus ring. If you interact with the control exclusively using the keyboard, it works as expected. In Chrome and Firefox, it works fine with both methods of interaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did find an issue with the focus highlight when mixing mouse and keyboard input in Safari, and I did note an issue when mixing mouse and keyboard input in all browsers. Nevertheless this is an improvement over the previous behavior for the common case of using exclusively mouse or exclusively keyboard input.
@@ -92,7 +91,7 @@ export function useArrowKeyNavigation( | |||
// Keep track of the element that was last focused by this hook such that | |||
// navigation can be restored if focus moves outside the container and then | |||
// back to/into it. | |||
const lastFocusedItem = useRef<HTMLOrSVGElement | null>(null); | |||
const lastFocusedItem = useRef<HTMLElement | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because getNavigableElements
returns HTMLElement[]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In essence yes, but I could not find a reason for the ref to be typed as HTMLOrSVGElement
.
I'm ok doing an inline as HTMLElement
cast though. We are already doing it in a couple of other places for this hook.
navigableElements, | ||
initialIndex, | ||
containerVisible && autofocus, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To recap: Every time this effect ran, it reset the selected item to the first one in the list. Now it will reset it to lastFocusedItem.current
. This is an improvement over the way it worked before, although lastFocusedItem
doesn't currently get updated if you focus an item a different way, eg using the mouse. In the context of SelectNext
, this means that if you:
- Open the list and select an item using the keyboard
- Re-open the list and select a different item using the mouse
- Re-open the list using the keyboard
Then the focused item after step (3) is the one that was focused in step (1) rather than step (2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to this, the docs for the autofocus
prop say that focusing is applied when the component is mounted, but the current behavior is that auto-focusing is applied when:
- The component is mounted
containerVisible
changes- Any of the general configuration props for the hook change (we expect this will not happen for the majority of uses)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To recap: Every time this effect ran, it reset the selected item to the first one in the list. Now it will reset it to
lastFocusedItem.current
. This is an improvement over the way it worked before, althoughlastFocusedItem
doesn't currently get updated if you focus an item a different way, eg using the mouse. In the context ofSelectNext
, this means that if you:
- Open the list and select an item using the keyboard
- Re-open the list and select a different item using the mouse
- Re-open the list using the keyboard
Then the focused item after step (3) is the one that was focused in step (1) rather than step (2).
Hmm, good catch.
There's another problem still to be solved, also related with combining mouse and keyboard (see last checkpoint in #1285).
I'll try to address those together.
Also, I have checked that the behavior you describe does not happen with other arrow-key-navigable components, like Tabs
. If you focus one tab via arrows, then click another tab, then focus away, then focus back, the tab that gets focused is the one you clicked. So at least the bug is limited to the Select.
Part of #1285
Depends on #1288
This PR's primary focus is to improve arrow key navigation, making sure the active option is the focused one when the listbox is opened, falling back to the first option if none is active.
Additionally, we add explicit
displayName
s forSelectNext
andSelectOption
, which don't have the expected value by default due to theObject.assign(...)
approach to expose the main component and its sub-components.Testing steps
make dev
)In
main
, the first option is the one selected when the list is open, no matter if there's an active option or not.