Skip to content

Commit

Permalink
fix(select a11y): handle focussed option index separately when filtering
Browse files Browse the repository at this point in the history
  • Loading branch information
@Mohammer5
Mohammer5 committed Nov 4, 2024
1 parent 78a120f commit 3126037
Show file tree
Hide file tree
Showing 9 changed files with 127 additions and 80 deletions.
2 changes: 0 additions & 2 deletions components/select/src/single-select-a11y/__stories__/InfiniteLoading.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,13 @@ export const InfiniteLoading = () => {

const loadNextOptions = useCallback(() => {
const nextPage = curLoadedPage + 1
console.log('>', { nextPage, loading })

if (
// We're already loading a page
loading ||
// No need to load anything when already loaded everything
nextPage >= optionChunks.length
) {
console.log('> do nothing')
return
}

Expand Down
7 changes: 1 addition & 6 deletions components/select/src/single-select-a11y/features/keyboard-interactions.test.e2e.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ describe('<SingleSelectA11y/>', () => {

cy.findByRole('option', { selected: true })
.invoke('parent') // listbox
.invoke('parent') // .listbox-container
.invoke('parent') // scrollable div
.then((listBoxParent) => {
listBoxParent.get(0).scrollTop = optionOffset
Expand Down Expand Up @@ -71,7 +70,6 @@ describe('<SingleSelectA11y/>', () => {

cy.findByRole('option', { selected: true })
.invoke('parent') // listbox
.invoke('parent') // .listbox-container
.invoke('parent') // scrollable div
.then((listBoxParent) => {
listBoxParent.get(0).scrollTop = optionOffset
Expand Down Expand Up @@ -183,7 +181,6 @@ describe('<SingleSelectA11y/>', () => {

cy.findByRole('option', { selected: true })
.invoke('parent') // listbox
.invoke('parent') // .listbox-container
.invoke('parent') // scrollable div
.then((listBoxParent) => {
listBoxParent.get(0).scrollTop = optionOffset
Expand Down Expand Up @@ -228,7 +225,6 @@ describe('<SingleSelectA11y/>', () => {

cy.findByRole('option', { selected: true })
.invoke('parent') // listbox
.invoke('parent') // .listbox-container
.invoke('parent') // scrollable div
.then((listBoxParent) => {
listBoxParent.get(0).scrollTop = optionOffset
Expand Down Expand Up @@ -408,7 +404,6 @@ describe('<SingleSelectA11y/>', () => {
cy
.findByRole('option', { selected: true })
.invoke('parent')
.invoke('parent') // .listbox-container
.invoke('parent')
).then(([nextTopOption, listBoxParent]) => {
const { offsetTop } = nextTopOption.get(0)
Expand Down Expand Up @@ -528,7 +523,7 @@ describe('<SingleSelectA11y/>', () => {
)
cy.all(
() => cy.findAllByRole('option').eq(89),
() => cy.findByRole('listbox').invoke('parent').invoke('parent') // scrollable div
() => cy.findByRole('listbox').invoke('parent') // scrollable div
).then(([nextTopOption, listBoxParent]) => {
const { offsetTop } = nextTopOption.get(0)
listBoxParent.get(0).scrollTop = offsetTop
Expand Down
4 changes: 4 additions & 0 deletions components/select/src/single-select-a11y/menu/menu-filter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import React from 'react'
import i18n from '../../locales/index.js'

export function MenuFilter({
idPrefix,
value,
onChange,
dataTest,
Expand All @@ -16,6 +17,8 @@ export function MenuFilter({
<div data-test={dataTest}>
<Input
dense
ariaControls={`${idPrefix}-listbox`}
ariaHaspopup="listbox"
ariaLabel={label || i18n.t('Search options')}
dataTest={`${dataTest}-input`}
value={value}
Expand Down Expand Up @@ -43,6 +46,7 @@ export function MenuFilter({
}

MenuFilter.propTypes = {
idPrefix: PropTypes.string.isRequired,
value: PropTypes.string.isRequired,
onChange: PropTypes.func.isRequired,
dataTest: PropTypes.string,
Expand Down
26 changes: 16 additions & 10 deletions components/select/src/single-select-a11y/menu/menu.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export function Menu({
>
{filterable && (
<MenuFilter
idPrefix={idPrefix}
dataTest={`${dataTestPrefix}-filter`}
value={filterValue}
onChange={onFilterChange}
Expand Down Expand Up @@ -114,35 +115,40 @@ export function Menu({
onChange={onChange}
onEndReached={onEndReached}
/>

{loading && (
<div className="menu-loading-container">
<MenuLoading message={loadingText} />
</div>
)}
</div>

{loading && (
<div className="menu-loading-container">
<MenuLoading message={loadingText} />
</div>
)}

<style jsx>{`
.menu {
display: flex;
flex-direction: column;
height: auto;
overflow: auto;
background: ${colors.white};
border: 1px solid ${colors.grey200};
border-radius: 3px;
box-shadow: ${elevations.e300};
/* We want the provided height to be exact */
/* We want the provided height to be exact, otherwise
the consumer would have to know about the border's width */
box-sizing: content-box;
}
.listbox-wrapper {
.listbox-container {
position: relative;
overflow: auto;
height: 100%;
flex-grow: 1;
}
.menu-loading-container {
position: absolute;
left: 0;
top: 0;
bottom: 0;
width: 100%;
height: 100%;
}
Expand Down
2 changes: 1 addition & 1 deletion components/select/src/single-select-a11y/menu/option.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export function Option({

useEffect(() => {
if (onBecameVisible) {
const scrollableContainer = listBoxRef.current.parentNode.parentNode
const scrollableContainer = listBoxRef.current.parentNode
const intersectionOptions = {
root: scrollableContainer,
rootMargin: '0px',
Expand Down
51 changes: 47 additions & 4 deletions components/select/src/single-select-a11y/single-select-a11y.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { requiredIf } from '@dhis2/prop-types'
import { sharedPropTypes } from '@dhis2/ui-constants'
import cx from 'classnames'
import PropTypes from 'prop-types'
import React, { useCallback, useRef, useState } from 'react'
import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'
import { Menu } from './menu/index.js'
import { SelectedValue } from './selected-value/index.js'
import { optionProp } from './shared-prop-types.js'
Expand All @@ -11,6 +11,44 @@ import {
useHandleKeyPressOnFilterInput,
} from './use-handle-key-press/index.js'

function useFocussedOptionIndex({ filterable, filterValue, options }) {
const [defaultFocussedOptionIndex, setDefaultFocussedOptionIndex] =
useState(0)
const [searchFocussedOptionIndex, setSearchFocussedOptionIndex] =
useState(0)

// We want to reset the focussed option index when searching when the
// options change
//
// @TODO: We could think about making this smarter, e.g. by looking whether
// the previously highlighted option is still present in the options list.
// At this point I think that optimizations might be overkill or even cause
// bad UX, so I'm keeping it simple
const initialized = useRef(false)
useEffect(() => {
// Ignore first call to prevent unnecessary re-render
if (!initialized.current) {
initialized.current = true
} else if (filterable) {
setSearchFocussedOptionIndex(0)
}
}, [options, filterable])

const focussedOptionIndex = useMemo(() => {
return filterValue
? searchFocussedOptionIndex
: defaultFocussedOptionIndex
}, [defaultFocussedOptionIndex, searchFocussedOptionIndex, filterValue])

const setFocussedOptionIndex = useMemo(() => {
return filterValue
? setSearchFocussedOptionIndex
: setDefaultFocussedOptionIndex
}, [filterValue])

return [focussedOptionIndex, setFocussedOptionIndex]
}

export function SingleSelectA11y({
options,
idPrefix,
Expand Down Expand Up @@ -66,10 +104,15 @@ export function SingleSelectA11y({
)
}

// Using `useState` here so components get notified when the value changes (from null -> div)
const comboBoxRef = useRef()
const listBoxRef = useRef()
const [focussedOptionIndex, setFocussedOptionIndex] = useState(0)
const [focussedOptionIndex, setFocussedOptionIndex] =
useFocussedOptionIndex({
filterable,
filterValue,
options,
})

const [selectRef, setSelectRef] = useState()
const [expanded, setExpanded] = useState(false)
const closeMenu = useCallback(() => setExpanded(false), [])
Expand All @@ -82,7 +125,7 @@ export function SingleSelectA11y({
}

setExpanded(true)
}, [options, value, focussedOptionIndex])
}, [options, value, focussedOptionIndex, setFocussedOptionIndex])
const toggleMenu = useCallback(() => {
if (expanded) {
closeMenu()
Expand Down
14 changes: 9 additions & 5 deletions components/select/src/single-select-a11y/single-select-a11y.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ describe('<SingleSelectA11y />', () => {
fireEvent.click(screen.getByRole('combobox'))

const listbox = screen.getByRole('listbox')
const listboxContainer = listbox.parentNode.parentNode
expect(listboxContainer.style.maxHeight).toBe('100px')
const menu = listbox.parentNode.parentNode
expect(menu.style.maxHeight).toBe('100px')
})

it('should accept a placeholder', () => {
Expand Down Expand Up @@ -327,7 +327,7 @@ describe('<SingleSelectA11y />', () => {
it('should have an empty-text in the menu', () => {
render(
<SingleSelectA11y
empty={<p>Empty</p>}
empty="Empty"
idPrefix="a11y"
value=""
onChange={jest.fn()}
Expand All @@ -336,7 +336,7 @@ describe('<SingleSelectA11y />', () => {
)

const emptyTextBeforeOpen = screen.queryByText('Empty')
expect(emptyTextBeforeOpen).not.toBeVisible()
expect(emptyTextBeforeOpen).toBeNull()

fireEvent.click(screen.getByRole('combobox'))

Expand All @@ -359,7 +359,7 @@ describe('<SingleSelectA11y />', () => {
/>
)

expect(screen.queryByLabelText('Search options')).not.toBeVisible()
expect(screen.queryByLabelText('Search options')).toBeNull()

fireEvent.click(screen.getByRole('combobox'))

Expand Down Expand Up @@ -393,6 +393,8 @@ describe('<SingleSelectA11y />', () => {
/>
)

fireEvent.click(screen.getByRole('combobox'))

expect(screen.getByLabelText('Custom filter label')).not.toBeNull()
})

Expand Down Expand Up @@ -421,6 +423,8 @@ describe('<SingleSelectA11y />', () => {
/>
)

fireEvent.click(screen.getByRole('combobox'))

// @TODO: For some reason this is called three times
// Is this because of unnecessary re-renders?
expect(consoleError).toHaveBeenNthCalledWith(
Expand Down
Loading

0 comments on commit 3126037

Please sign in to comment.