From 31260375b7d44254ff48dc1a7b38b362b3d0cce2 Mon Sep 17 00:00:00 2001
From: Mohammer5 <jan-gerke@dhis2.org>
Date: Mon, 4 Nov 2024 15:43:08 +0800
Subject: [PATCH] fix(select a11y): handle focussed option index separately
 when filtering

---
 .../__stories__/InfiniteLoading.js            |  2 -
 .../keyboard-interactions.test.e2e.js         |  7 +-
 .../single-select-a11y/menu/menu-filter.js    |  4 +
 .../src/single-select-a11y/menu/menu.js       | 26 +++--
 .../src/single-select-a11y/menu/option.js     |  2 +-
 .../single-select-a11y/single-select-a11y.js  | 51 +++++++++-
 .../single-select-a11y.test.js                | 14 ++-
 .../use-highlight-last-option-on-next-page.js | 99 +++++++++----------
 .../use-handle-key-press/use-page-down.js     |  2 +-
 9 files changed, 127 insertions(+), 80 deletions(-)

diff --git a/components/select/src/single-select-a11y/__stories__/InfiniteLoading.js b/components/select/src/single-select-a11y/__stories__/InfiniteLoading.js
index 6b3750097..6744436fa 100644
--- a/components/select/src/single-select-a11y/__stories__/InfiniteLoading.js
+++ b/components/select/src/single-select-a11y/__stories__/InfiniteLoading.js
@@ -33,7 +33,6 @@ export const InfiniteLoading = () => {
 
     const loadNextOptions = useCallback(() => {
         const nextPage = curLoadedPage + 1
-        console.log('>', { nextPage, loading })
 
         if (
             // We're already loading a page
@@ -41,7 +40,6 @@ export const InfiniteLoading = () => {
             // No need to load anything when already loaded everything
             nextPage >= optionChunks.length
         ) {
-            console.log('> do nothing')
             return
         }
 
diff --git a/components/select/src/single-select-a11y/features/keyboard-interactions.test.e2e.js b/components/select/src/single-select-a11y/features/keyboard-interactions.test.e2e.js
index 4df11efd4..bfc6b396b 100644
--- a/components/select/src/single-select-a11y/features/keyboard-interactions.test.e2e.js
+++ b/components/select/src/single-select-a11y/features/keyboard-interactions.test.e2e.js
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
@@ -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
diff --git a/components/select/src/single-select-a11y/menu/menu-filter.js b/components/select/src/single-select-a11y/menu/menu-filter.js
index c90ec1cec..8d6aee173 100644
--- a/components/select/src/single-select-a11y/menu/menu-filter.js
+++ b/components/select/src/single-select-a11y/menu/menu-filter.js
@@ -5,6 +5,7 @@ import React from 'react'
 import i18n from '../../locales/index.js'
 
 export function MenuFilter({
+    idPrefix,
     value,
     onChange,
     dataTest,
@@ -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}
@@ -43,6 +46,7 @@ export function MenuFilter({
 }
 
 MenuFilter.propTypes = {
+    idPrefix: PropTypes.string.isRequired,
     value: PropTypes.string.isRequired,
     onChange: PropTypes.func.isRequired,
     dataTest: PropTypes.string,
diff --git a/components/select/src/single-select-a11y/menu/menu.js b/components/select/src/single-select-a11y/menu/menu.js
index 87b3cbcfd..775e554ed 100644
--- a/components/select/src/single-select-a11y/menu/menu.js
+++ b/components/select/src/single-select-a11y/menu/menu.js
@@ -81,6 +81,7 @@ export function Menu({
                 >
                     {filterable && (
                         <MenuFilter
+                            idPrefix={idPrefix}
                             dataTest={`${dataTestPrefix}-filter`}
                             value={filterValue}
                             onChange={onFilterChange}
@@ -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%;
                         }
diff --git a/components/select/src/single-select-a11y/menu/option.js b/components/select/src/single-select-a11y/menu/option.js
index 46f4f515e..a473f6a98 100644
--- a/components/select/src/single-select-a11y/menu/option.js
+++ b/components/select/src/single-select-a11y/menu/option.js
@@ -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',
diff --git a/components/select/src/single-select-a11y/single-select-a11y.js b/components/select/src/single-select-a11y/single-select-a11y.js
index 48231cdb1..bc25ed0b3 100644
--- a/components/select/src/single-select-a11y/single-select-a11y.js
+++ b/components/select/src/single-select-a11y/single-select-a11y.js
@@ -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'
@@ -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,
@@ -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), [])
@@ -82,7 +125,7 @@ export function SingleSelectA11y({
         }
 
         setExpanded(true)
-    }, [options, value, focussedOptionIndex])
+    }, [options, value, focussedOptionIndex, setFocussedOptionIndex])
     const toggleMenu = useCallback(() => {
         if (expanded) {
             closeMenu()
diff --git a/components/select/src/single-select-a11y/single-select-a11y.test.js b/components/select/src/single-select-a11y/single-select-a11y.test.js
index 89e7640e8..8b07a5818 100644
--- a/components/select/src/single-select-a11y/single-select-a11y.test.js
+++ b/components/select/src/single-select-a11y/single-select-a11y.test.js
@@ -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', () => {
@@ -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()}
@@ -336,7 +336,7 @@ describe('<SingleSelectA11y />', () => {
         )
 
         const emptyTextBeforeOpen = screen.queryByText('Empty')
-        expect(emptyTextBeforeOpen).not.toBeVisible()
+        expect(emptyTextBeforeOpen).toBeNull()
 
         fireEvent.click(screen.getByRole('combobox'))
 
@@ -359,7 +359,7 @@ describe('<SingleSelectA11y />', () => {
             />
         )
 
-        expect(screen.queryByLabelText('Search options')).not.toBeVisible()
+        expect(screen.queryByLabelText('Search options')).toBeNull()
 
         fireEvent.click(screen.getByRole('combobox'))
 
@@ -393,6 +393,8 @@ describe('<SingleSelectA11y />', () => {
             />
         )
 
+        fireEvent.click(screen.getByRole('combobox'))
+
         expect(screen.getByLabelText('Custom filter label')).not.toBeNull()
     })
 
@@ -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(
diff --git a/components/select/src/single-select-a11y/use-handle-key-press/use-highlight-last-option-on-next-page.js b/components/select/src/single-select-a11y/use-handle-key-press/use-highlight-last-option-on-next-page.js
index a53372a49..8bc418aff 100644
--- a/components/select/src/single-select-a11y/use-handle-key-press/use-highlight-last-option-on-next-page.js
+++ b/components/select/src/single-select-a11y/use-handle-key-press/use-highlight-last-option-on-next-page.js
@@ -7,58 +7,55 @@ export function useHighlightLastOptionOnNextPage({
     setFocussedOptionIndex,
     listBoxRef,
 }) {
-    return useCallback(
-        (listBoxParent) => {
-            const optionElements = Array.from(listBoxRef.current.childNodes)
-            const visibleOptionsAmount = options.filter(
-                (_, index) =>
-                    !isOptionHidden(optionElements[index], listBoxParent)
-            ).length
-
-            const nextHighlightedOptionIndex = Math.min(
-                options.length - 1,
-                focussedOptionIndex + visibleOptionsAmount
-            )
-
-            // If there's no next option and we already have the last option in the list highlighted
-            if (!options[nextHighlightedOptionIndex]) {
-                return
-            }
-
-            if (!options[nextHighlightedOptionIndex].disabled) {
-                // This will be the first option in the list
-                const { offsetTop: scrollPosition } =
-                    optionElements[
-                        nextHighlightedOptionIndex - visibleOptionsAmount + 1
-                    ]
-
-                listBoxParent.scrollTop = scrollPosition
-                setFocussedOptionIndex(nextHighlightedOptionIndex)
-                return
-            }
-
-            const followingEnabledOptionIndex =
-                nextHighlightedOptionIndex +
-                options
-                    .slice(nextHighlightedOptionIndex)
-                    .findIndex((option) => !option.disabled)
-
-            // There is no enabled option after the disabled option that's at the end of the next page
-            // So we stay where we are
-            if (followingEnabledOptionIndex === -1) {
-                return
-            }
-
-            // There is an enabled option after the disabled option that's at the end of the next page
-            // So that'll be the new highlighted option and the bottom of the next displayed page
-            const { offsetTop: adjustedScrollPosition } =
+    return useCallback(() => {
+        const listBoxParent = listBoxRef.current.parentNode
+        const optionElements = Array.from(listBoxRef.current.childNodes)
+        const visibleOptionsAmount = options.filter(
+            (_, index) => !isOptionHidden(optionElements[index], listBoxParent)
+        ).length
+
+        const nextHighlightedOptionIndex = Math.min(
+            options.length - 1,
+            focussedOptionIndex + visibleOptionsAmount
+        )
+
+        // If there's no next option and we already have the last option in the list highlighted
+        if (!options[nextHighlightedOptionIndex]) {
+            return
+        }
+
+        if (!options[nextHighlightedOptionIndex].disabled) {
+            // This will be the first option in the list
+            const { offsetTop: scrollPosition } =
                 optionElements[
-                    followingEnabledOptionIndex - visibleOptionsAmount + 1
+                    nextHighlightedOptionIndex - visibleOptionsAmount + 1
                 ]
 
-            listBoxParent.scrollTop = adjustedScrollPosition
-            setFocussedOptionIndex(followingEnabledOptionIndex)
-        },
-        [options, focussedOptionIndex, setFocussedOptionIndex, listBoxRef]
-    )
+            listBoxParent.scrollTop = scrollPosition
+            setFocussedOptionIndex(nextHighlightedOptionIndex)
+            return
+        }
+
+        const followingEnabledOptionIndex =
+            nextHighlightedOptionIndex +
+            options
+                .slice(nextHighlightedOptionIndex)
+                .findIndex((option) => !option.disabled)
+
+        // There is no enabled option after the disabled option that's at the end of the next page
+        // So we stay where we are
+        if (followingEnabledOptionIndex === -1) {
+            return
+        }
+
+        // There is an enabled option after the disabled option that's at the end of the next page
+        // So that'll be the new highlighted option and the bottom of the next displayed page
+        const { offsetTop: adjustedScrollPosition } =
+            optionElements[
+                followingEnabledOptionIndex - visibleOptionsAmount + 1
+            ]
+
+        listBoxParent.scrollTop = adjustedScrollPosition
+        setFocussedOptionIndex(followingEnabledOptionIndex)
+    }, [options, focussedOptionIndex, setFocussedOptionIndex, listBoxRef])
 }
diff --git a/components/select/src/single-select-a11y/use-handle-key-press/use-page-down.js b/components/select/src/single-select-a11y/use-handle-key-press/use-page-down.js
index 8e373b29b..b7764cc42 100644
--- a/components/select/src/single-select-a11y/use-handle-key-press/use-page-down.js
+++ b/components/select/src/single-select-a11y/use-handle-key-press/use-page-down.js
@@ -35,7 +35,7 @@ export function usePageDown({
         }
 
         if (highestVisibleIndex > -1) {
-            highlightLastOptionOnNextPage(listBoxParent)
+            highlightLastOptionOnNextPage()
             return
         }