From b29c25bee289e0d8375410e14c3e8cd8381682d2 Mon Sep 17 00:00:00 2001 From: Michael Cousins Date: Mon, 17 Jun 2024 15:08:01 -0400 Subject: [PATCH] APP-4651: do not render `` list when collapsed (#532) --- packages/core/package.json | 2 +- .../__tests__/searchable-select.spec.ts | 203 +++++++++--------- .../src/lib/select/searchable-select.svelte | 198 +++++++++-------- .../core/src/lib/select/select-input.svelte | 2 +- packages/core/src/lib/use-next-frame.ts | 27 +++ 5 files changed, 237 insertions(+), 195 deletions(-) create mode 100644 packages/core/src/lib/use-next-frame.ts diff --git a/packages/core/package.json b/packages/core/package.json index 35f84f61..fdf7972b 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@viamrobotics/prime-core", - "version": "0.0.123", + "version": "0.0.124", "publishConfig": { "access": "public" }, diff --git a/packages/core/src/lib/select/__tests__/searchable-select.spec.ts b/packages/core/src/lib/select/__tests__/searchable-select.spec.ts index fe844575..b2b6ec1a 100644 --- a/packages/core/src/lib/select/__tests__/searchable-select.spec.ts +++ b/packages/core/src/lib/select/__tests__/searchable-select.spec.ts @@ -39,13 +39,13 @@ const renderSubject = (props: Partial> = {}) => { const getResults = (): { search: HTMLElement; button: HTMLElement; - list: HTMLElement; + list: HTMLElement | null; options: HTMLElement[]; } => { const search = screen.getByRole('combobox'); const button = screen.getByRole('button'); - const list = screen.getByRole('listbox'); - const options = within(list).queryAllByRole('option'); + const list = screen.queryByRole('listbox'); + const options = list ? within(list).queryAllByRole('option') : []; return { search, button, list, options }; }; @@ -54,11 +54,8 @@ describe('SearchableSelect', () => { it('is a combobox that controls a listbox', () => { renderSubject(); - const { search, button, list } = getResults(); + const { search } = getResults(); - expect(list).toHaveAttribute('id', expect.any(String)); - expect(button).toHaveAttribute('aria-controls', list.id); - expect(search).toHaveAttribute('aria-controls', list.id); expect(search).toHaveAttribute('aria-autocomplete', 'list'); expect(search).not.toHaveAttribute('aria-multiselectable'); }); @@ -118,15 +115,19 @@ describe('SearchableSelect', () => { const { search, button, list } = getResults(); - expect(list).toHaveClass('hidden'); + expect(list).not.toBeInTheDocument(); expect(search).toHaveAttribute('aria-expanded', 'false'); expect(button).toHaveAttribute('aria-expanded', 'false'); await user.keyboard('{Tab}'); + const { list: expandedList } = getResults(); + expect(onFocus).toHaveBeenCalledOnce(); expect(search).toHaveFocus(); - expect(list).not.toHaveClass('hidden'); + expect(expandedList).toHaveAttribute('id', expect.any(String)); + expect(button).toHaveAttribute('aria-controls', expandedList?.id); + expect(search).toHaveAttribute('aria-controls', expandedList?.id); expect(button).toHaveAttribute('aria-expanded', 'true'); expect(search).toHaveAttribute('aria-expanded', 'true'); }); @@ -212,10 +213,13 @@ describe('SearchableSelect', () => { expect(onChange).not.toHaveBeenCalled(); }); - it('has options', () => { + it('has options', async () => { + const user = userEvent.setup(); renderSubject(); - const { search, options } = getResults(); + const { search } = getResults(); + await user.click(search); + const { options } = getResults(); expect(options).toHaveLength(2); expect(options[0]).toHaveAccessibleName('hello from'); @@ -225,12 +229,15 @@ describe('SearchableSelect', () => { expect(search).not.toHaveAttribute('aria-activedescendant'); }); - it('has options with labels descriptions and icons', () => { + it('has options with labels descriptions and icons', async () => { + const user = userEvent.setup(); renderSubject({ options: detailedOptions, }); - const { search, options } = getResults(); + const { search } = getResults(); + await user.click(search); + const { options } = getResults(); expect(options).toHaveLength(2); const firstOption = screen.getByRole('option', { name: /gale/iu }); @@ -252,9 +259,10 @@ describe('SearchableSelect', () => { const user = userEvent.setup(); renderSubject(); - const { search, options } = getResults(); - + const { search } = getResults(); await user.click(search); + const { options } = getResults(); + // TODO(mc, 2024-02-03): replace .click with userEvent // https://github.com/testing-library/user-event/issues/1119 await act(() => options[0]?.click()); @@ -286,32 +294,32 @@ describe('SearchableSelect', () => { const user = userEvent.setup(); renderSubject({ options: detailedOptions }); - const { search, options } = getResults(); - + const { search } = getResults(); await user.click(search); + const { options } = getResults(); + // TODO(mc, 2024-02-03): replace .click with userEvent // https://github.com/testing-library/user-event/issues/1119 await act(() => options[0]?.click()); + const searchIcon = screen.getByTestId('icon-viam-process'); + expect(search).toHaveFocus(); expect(onChange).toHaveBeenCalledWith('opt-1'); expect(search).toHaveValue('Gale'); expect(search).toHaveAttribute('aria-expanded', 'false'); expect(search).not.toHaveAttribute('aria-activedescendant'); - // test that both icons render for option + svg in the input - expect(screen.getAllByTestId('icon-viam-process')).toHaveLength(2); + expect(searchIcon).toBeInTheDocument(); }); it('renders the initial input value', () => { - renderSubject({ - options: detailedOptions, - value: 'opt-2', - }); + renderSubject({ options: detailedOptions, value: 'opt-2' }); const { search } = getResults(); + const searchIcon = screen.getByTestId('icon-language-cpp'); + expect(search).toHaveValue('Karlach'); - // test that both icons render for option + svg in the input - expect(screen.getAllByTestId('icon-language-cpp')).toHaveLength(2); + expect(searchIcon).toBeInTheDocument(); }); it('auto-selects search result on Enter', async () => { @@ -335,18 +343,18 @@ describe('SearchableSelect', () => { expect(onChange).toHaveBeenCalledWith('the other side'); expect(search).toHaveValue('the other side'); expect(search).toHaveAttribute('aria-expanded', 'false'); - expect(options[0]).toHaveAttribute('aria-selected', 'false'); - expect(options[1]).toHaveAttribute('aria-selected', 'false'); expect(search).not.toHaveAttribute('aria-activedescendant'); }); it('updates the rendered options when the options input field changes', async () => { - const { component } = renderSubject({ - options: detailedOptions, - }); + const user = userEvent.setup(); + const { component } = renderSubject({ options: detailedOptions }); + + const { search } = getResults(); + await user.click(search); // Verify initial options - let { options } = getResults(); + const { options } = getResults(); expect(options).toHaveLength(stringOptions.length); expect(options[0]).toHaveAccessibleName(detailedOptions[0]?.label); expect(options[1]).toHaveAccessibleName(detailedOptions[1]?.label); @@ -365,11 +373,11 @@ describe('SearchableSelect', () => { }); // Verify updated options - ({ options } = getResults()); - expect(options).toHaveLength(newOptions.length); - expect(options[0]).toHaveAccessibleName('New Option 1'); - expect(options[1]).toHaveAccessibleName('New Option 2'); - expect(options[2]).toHaveAccessibleName('New Option 3'); + const { options: nextOptions } = getResults(); + expect(nextOptions).toHaveLength(newOptions.length); + expect(nextOptions[0]).toHaveAccessibleName('New Option 1'); + expect(nextOptions[1]).toHaveAccessibleName('New Option 2'); + expect(nextOptions[2]).toHaveAccessibleName('New Option 3'); expect(screen.getByTestId('icon-apple')).toBeInTheDocument(); }); @@ -512,9 +520,12 @@ describe('SearchableSelect', () => { expect(search).toHaveAttribute('aria-activedescendant', options[2]?.id); }); - it('has no "other" option when value empty', () => { + it('has no "other" option when value empty', async () => { + const user = userEvent.setup(); renderSubject(); + const { search } = getResults(); + await user.click(search); const { options } = getResults(); expect(options).toHaveLength(2); @@ -592,23 +603,26 @@ describe('SearchableSelect', () => { const user = userEvent.setup(); renderSubject(); - const { search, options } = getResults(); + const { search } = getResults(); await user.type(search, 'the other'); await user.keyboard('{Escape} side'); + const { options } = getResults(); + expect(search).toHaveAttribute('aria-expanded', 'true'); - expect(search).toHaveAttribute('aria-activedescendant', options[1]?.id); - expect(options[0]).toHaveAttribute('aria-selected', 'false'); - expect(options[1]).toHaveAttribute('aria-selected', 'true'); + expect(search).toHaveAttribute('aria-activedescendant', options[0]?.id); + expect(options[0]).toHaveAttribute('aria-selected', 'true'); + expect(options[1]).toHaveAttribute('aria-selected', 'false'); }); it('moves visual focus to options on arrow keys', async () => { const user = userEvent.setup(); renderSubject(); - const { search, options } = getResults(); + const { search } = getResults(); await user.keyboard('{Tab}'); + const { options } = getResults(); await user.keyboard('{ArrowDown}'); expect(search).toHaveAttribute('aria-activedescendant', options[0]?.id); @@ -673,8 +687,9 @@ describe('SearchableSelect', () => { const user = userEvent.setup(); renderSubject(); - const { search, options } = getResults(); + const { search } = getResults(); await user.keyboard('{Tab}{Alt>}{ArrowDown}{/Alt}'); + const { options } = getResults(); expect(search).toHaveAttribute('aria-expanded', 'true'); expect(options[0]).toHaveAttribute('aria-selected', 'false'); @@ -687,9 +702,10 @@ describe('SearchableSelect', () => { const user = userEvent.setup(); renderSubject(); - const { search, options } = getResults(); + const { search } = getResults(); await user.type(search, 'hello'); await user.keyboard(`{ArrowDown}${key}`); + const { options } = getResults(); expect(options[0]).toHaveAttribute('aria-selected', 'true'); } @@ -699,27 +715,58 @@ describe('SearchableSelect', () => { const user = userEvent.setup(); renderSubject(); - const { search, options } = getResults(); + const { search } = getResults(); await user.type(search, 'hello'); await user.keyboard('{Escape}{ArrowDown}'); + const { options } = getResults(); expect(options[0]).toHaveAttribute('aria-selected', 'true'); }); + it('renders the icon and does not change the contents of the select when Enter is pressed twice', async () => { + const user = userEvent.setup(); + renderSubject({ options: detailedOptions }); + + const { search } = getResults(); + await user.type(search, 'Gale{Enter}'); + const searchIcon = screen.getByTestId('icon-viam-process'); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith('opt-1'); + expect(search).toHaveValue('Gale'); + expect(searchIcon).toBeInTheDocument(); + + await user.keyboard('{Enter}'); + + // Verify that onChange is not called again + expect(onChange).toHaveBeenCalledTimes(1); + expect(search).toHaveValue('Gale'); + expect(searchIcon).toBeInTheDocument(); + }); + + it('closes menu on blur', async () => { + const user = userEvent.setup(); + renderSubject({ multiple: true }); + + const { search } = getResults(); + await user.click(search); + await user.keyboard('{Tab}'); + + expect(search).toHaveAttribute('aria-expanded', 'false'); + }); + describe('multiple mode', () => { it('can select multiple options without closing', async () => { const user = userEvent.setup(); renderSubject({ multiple: true }); - const { search, options } = getResults(); + const { search } = getResults(); expect(search).toHaveAttribute('aria-multiselectable', 'true'); - await user.click(search); + await user.type(search, 'hello{Enter}'); + const { options } = getResults(); - // TODO(mc, 2024-02-03): replace .click with userEvent - // https://github.com/testing-library/user-event/issues/1119 - await act(() => options[0]?.click()); expect(onMultiChange).toHaveBeenCalledWith(['hello from']); expect(search).toHaveFocus(); expect(search).toHaveValue(''); @@ -727,7 +774,8 @@ describe('SearchableSelect', () => { expect(options[0]).toHaveAttribute('aria-checked', 'true'); expect(options[1]).toHaveAttribute('aria-checked', 'false'); - await act(() => options[1]?.click()); + await user.type(search, 'other{Enter}'); + expect(onMultiChange).toHaveBeenCalledWith([ 'hello from', 'the other side', @@ -739,13 +787,13 @@ describe('SearchableSelect', () => { expect(options[1]).toHaveAttribute('aria-checked', 'true'); }); - it('can select unselect with the mouse', async () => { + it('can select and unselect with the mouse', async () => { const user = userEvent.setup(); renderSubject({ multiple: true }); - const { search, options } = getResults(); - + const { search } = getResults(); await user.click(search); + const { options } = getResults(); // TODO(mc, 2024-02-03): replace .click with userEvent // https://github.com/testing-library/user-event/issues/1119 @@ -754,52 +802,5 @@ describe('SearchableSelect', () => { await act(() => options[0]?.click()); expect(onMultiChange).toHaveBeenCalledWith([]); }); - - it('resets search input on select', async () => { - const user = userEvent.setup(); - renderSubject({ multiple: true }); - - const { search } = getResults(); - await user.click(search); - - await user.type(search, 'hello{Enter}'); - - expect(onMultiChange).toHaveBeenCalledWith(['hello from']); - expect(search).toHaveValue(''); - }); - - it('renders the icon and does not change the contents of the select when Enter is pressed twice', async () => { - const user = userEvent.setup(); - renderSubject({ options: detailedOptions }); - - const { search } = getResults(); - await user.click(search); - await user.type(search, 'Gale'); - await user.keyboard('{Enter}'); - - expect(onChange).toHaveBeenCalledWith('opt-1'); - expect(search).toHaveValue('Gale'); - expect(screen.getAllByTestId('icon-viam-process')).toHaveLength(2); - - onChange.mockReset(); - - await user.keyboard('{Enter}'); - - // Verify that onChange is not called again - expect(onChange).not.toHaveBeenCalled(); - expect(search).toHaveValue('Gale'); - expect(screen.getAllByTestId('icon-viam-process')).toHaveLength(2); - }); - - it('closes menu on blur', async () => { - const user = userEvent.setup(); - renderSubject({ multiple: true }); - - const { search } = getResults(); - await user.click(search); - await user.keyboard('{Tab}'); - - expect(search).toHaveAttribute('aria-expanded', 'false'); - }); }); }); diff --git a/packages/core/src/lib/select/searchable-select.svelte b/packages/core/src/lib/select/searchable-select.svelte index 24fca9b6..905b8aa0 100644 --- a/packages/core/src/lib/select/searchable-select.svelte +++ b/packages/core/src/lib/select/searchable-select.svelte @@ -19,6 +19,7 @@ import { Icon } from '$lib/icon'; import { InputStates, type InputState } from '$lib/input'; import { createHandleKey } from '$lib/keyboard'; import { uniqueId } from '$lib/unique-id'; +import { useNextFrame } from '$lib/use-next-frame'; import { SortOptions, @@ -28,6 +29,7 @@ import { type SortOption, type DetailedOption, } from './search'; + import SelectInput from './select-input.svelte'; /** The options the user should be allowed to search and select from. */ @@ -97,6 +99,7 @@ const SELECTED_ID = uniqueId('combobox-list-selected-item'); const CLOSED = 'closed'; const FOCUS_SEARCH = 'focus-search'; const FOCUS_ITEM = 'focus-item'; +const onNextFrame = useNextFrame(); type MenuState = typeof CLOSED | typeof FOCUS_SEARCH | typeof FOCUS_ITEM; @@ -127,7 +130,7 @@ const resetSearchValue = ( }; $: resetSearchValue(value, detailedOptionsMap); -// selectedSeachOption represents the value that was last selected (or the initial value) +// selectedSearchOption represents the value that was last selected (or the initial value) $: selectedSearchOption = detailedOptionsMap[value]; $: searchResults = getSearchResults(detailedOptions, searchValue, sort); @@ -155,8 +158,8 @@ $: if (menuState === undefined || menuState === FOCUS_SEARCH) { ); // if we don't find any options with a non negative priority, // we set the nextAutoSelectIndex to the option that matches - // the current seach value. This is used so that when we blur - // with a valid seachValue, we autoSelect the correct option + // the current search value. This is used so that when we blur + // with a valid searchValue, we autoSelect the correct option if (nextAutoSelectIndex === -1) { nextAutoSelectIndex = allOptions.findIndex( ({ option }) => optionDisplayValue(option) === searchValue @@ -175,9 +178,17 @@ $: activeElement = activeOption ? optionElements[activeOption.option.value] : undefined; -$: if (typeof activeElement?.scrollIntoView === 'function') { - activeElement.scrollIntoView({ block: 'nearest' }); -} +const scrollIntoView = (element: HTMLElement | undefined): void => { + if (typeof element?.scrollIntoView === 'function') { + // NOTE(mc, 2024-06-17): do not scroll right away, or else we can accidentally + // scroll the whole document instead of the element's containing `
    ` + onNextFrame(() => { + element.scrollIntoView({ block: 'nearest', inline: 'nearest' }); + }); + } +}; + +$: scrollIntoView(activeElement); const handleSingleSelect = (selectedOption: DetailedOption | undefined) => { // if we are exclusive && it is not in the search, we should fallback to an empty value option @@ -229,7 +240,7 @@ const handleFocus = (event: FocusEvent) => { }; const handleBlur = (event: FocusEvent) => { - // blur can still be triggered if the input is diabled or the menu is closed, + // blur can still be triggered if the input is disabled or the menu is closed, // but we shouldn't select anything if so if (!disabled && menuState !== CLOSED) { handleSelect(autoSelectOption?.option); @@ -325,94 +336,97 @@ const handleKeydown = createHandleKey({ bind:inputElement value={searchValue} /> - -
      - {#each allOptions as { option, highlight } (option)} - {@const isActive = activeOption?.option === option} - {@const isSelected = multiple ? false : isActive} - {@const isChecked = multiple ? values.includes(option.value) : undefined} - {@const isOther = otherOption?.option === option} - {@const descriptionID = uniqueId('combobox-list-item-description')} - - {#if isOther && allOptions.length > 1} -
    • - {/if} - -
    • handleSelect(option)} - bind:this={optionElements[option.value]} - > -
      - - - {#if multiple} - - {/if} - {#if option.icon} - - {/if} -
      -

      - {#if highlight !== undefined} - {@const [prefix, match, suffix] = highlight} - {prefix}{match}{suffix} - {:else if isOther && otherOptionPrefix} - {otherOptionPrefix} {optionDisplayValue(option)} - {:else} - {optionDisplayValue(option)} - {/if} -

      - {#if option.description} -

      - {option.description} -

      +
    • handleSelect(option)} + bind:this={optionElements[option.value]} + > +
      + + + {#if multiple} + {/if} + {#if option.icon} + + {/if} +
      +

      + {#if highlight !== undefined} + {@const [prefix, match, suffix] = highlight} + {prefix}{match}{suffix} + {:else if isOther && otherOptionPrefix} + {otherOptionPrefix} {optionDisplayValue(option)} + {:else} + {optionDisplayValue(option)} + {/if} +

      + {#if option.description} +

      + {option.description} +

      + {/if} +
      - -
      • - {/each} - -
    -     + + {/each} + +
+ +{/if} diff --git a/packages/core/src/lib/select/select-input.svelte b/packages/core/src/lib/select/select-input.svelte index ed26c480..c162a4d5 100644 --- a/packages/core/src/lib/select/select-input.svelte +++ b/packages/core/src/lib/select/select-input.svelte @@ -74,7 +74,7 @@ $: errorClasses = type="text" class={cx( 'h-7.5 w-full grow appearance-none border py-1.5 pl-2 pr-1 text-xs leading-tight outline-none', - // We want the native select to include the icon so we need abolute positioning and this padding + // We want the native select to include the icon so we need absolute positioning and this padding { 'pl-8': Boolean(icon) }, defaultClasses, disabledClasses, diff --git a/packages/core/src/lib/use-next-frame.ts b/packages/core/src/lib/use-next-frame.ts new file mode 100644 index 00000000..1e177cc8 --- /dev/null +++ b/packages/core/src/lib/use-next-frame.ts @@ -0,0 +1,27 @@ +/** + * Add a callback to `requestAnimationFrame`, with cleanup. + */ +import { onDestroy } from 'svelte'; + +export type OnNextFrame = (callback: FrameRequestCallback) => void; + +export const useNextFrame = (): OnNextFrame => { + const frameIDs = new Set(); + + const onNextFrame = (callback: FrameRequestCallback) => { + const frameID = requestAnimationFrame((time) => { + callback(time); + frameIDs.delete(frameID); + }); + + frameIDs.add(frameID); + }; + + onDestroy(() => { + for (const frameID of frameIDs) { + cancelAnimationFrame(frameID); + } + }); + + return onNextFrame; +};