Fix Select activeRef tracking #6243
Conversation
src/js/components/Select/__tests__/__snapshots__/Select-test.js.snap
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The keyboard behavior seems off.
- Go to 'many options' story.
- Tab focus on input
- Spacebar to open
- Down arrow as far as you can
- Active index gets stuck at Option 33 and then keyboard controls stop working
Alternatively:
- Open drop with mouse
- Scroll to index far down (e.g. 60)
- Try using arrow keys
| background-color: lightgreen; | ||
| color: #7D4CDB; |
There was a problem hiding this comment.
NOTE: This snapshot change is result of restoration of functionality supporting theme.global.hover.background. This was missed in this previous snapshot change L2127.
| @@ -142,6 +142,8 @@ const plainStyle = (props) => css` | |||
|
|
|||
| const activeButtonStyle = (props) => css` | |||
| ${activeStyle} | |||
| ${props.activeIndicator && | |||
There was a problem hiding this comment.
It looks like we're adding activeIndicator to Button? If so, I think we're missing typescript, propTypes, unit tests, etc. Also, I'm wondering why we need this at the Button API level. I would expect that the presence of the active property would be sufficient. In other words, if I indicate that a Button is active, then I would expect the Button to indicate that visually some how.
There was a problem hiding this comment.
Details about isolated use in Select in the code comment here: https://github.com/grommet/grommet/pull/6243/files#r927979426
There was a problem hiding this comment.
open to other ways of achieving the functionality, but the functionality is needed
…into 6208-select-scroll
What does this PR do?
Improves tracking of active item. Previously we were keying off of
activeIndexwhich doesn't always align with the elements index in the select options (because infinite scroll loads options in steps). Instead, we track the active value in a ref and focus that.We only ever want one option to be highlighted at once. I've simplified the
keyboardNavigationlogic to more simply handle the trade off of keyboard and mouse movement.In order for the underlying button to receive the
theme.global.hover.backgroundtreatment correctly for both mouse interactions and keyboard interactions (the styling should be consistent regardless of if user is interacting via mouse or keyboard). Adjusted SelectOption styled component to apply this instead of relying on hoverIndicator (which would only apply on :hover).The snapshot changes are related to the active styling being handled now by
activeStyleas opposed tohoverStyle. The visual experience for the user remains unchanged.Where should the reviewer start?
What testing has been done on this PR?
Tested with voice over and compared new behavior with behavior enhanced in #5934. All behavior is retained.
Previously flagged keyboard errors have been resolved.
How should this be manually tested?
Do Jest tests follow these best practices?
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes #6208
Closes #6238
Closes #6248
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No.
Should this PR be mentioned in the release notes?
Yes.
Is this change backwards compatible or is it a breaking change?
Backwards compatible.