From e3b8dd2e8b15063afa469099c1a4017cc5f1ecfe Mon Sep 17 00:00:00 2001 From: Jack Shelton <104264123+thejackshelton@users.noreply.github.com> Date: Thu, 23 Jan 2025 10:29:44 -0600 Subject: [PATCH] Select fixes 0.6.5 (#1042) * fix: select now shows first item instead of placeholder when initial value is set * fix: prevent native scrolling on focus in select * feat: initial scroll handling logic * feat: correct key navigation * fix: infinite scroll * comment out broken ci check * changeset --- .changeset/old-ladybugs-pump.md | 10 ++++ .github/workflows/test.yml | 11 ++-- .../headless/combobox/examples/scrollable.tsx | 25 ++------- .../docs/headless/select/auto-api/api.ts | 55 +++++++++++++++++++ .../select/examples/first-value-selected.tsx | 26 +++++++++ .../headless/select/examples/scrollable.tsx | 24 ++------ .../src/components/combobox/combobox-item.tsx | 2 +- .../src/components/select/select-context.ts | 2 + .../src/components/select/select-item.tsx | 48 ++++++++++------ .../src/components/select/select-popover.tsx | 6 ++ .../src/components/select/select-root.tsx | 18 +++--- .../src/components/select/select-trigger.tsx | 1 + .../src/components/select/select.test.ts | 13 +++++ 13 files changed, 175 insertions(+), 66 deletions(-) create mode 100644 .changeset/old-ladybugs-pump.md create mode 100644 apps/website/src/routes/docs/headless/select/auto-api/api.ts create mode 100644 apps/website/src/routes/docs/headless/select/examples/first-value-selected.tsx diff --git a/.changeset/old-ladybugs-pump.md b/.changeset/old-ladybugs-pump.md new file mode 100644 index 000000000..3c5e8287d --- /dev/null +++ b/.changeset/old-ladybugs-pump.md @@ -0,0 +1,10 @@ +--- +'@qwik-ui/headless': patch +--- + +Select fixes for: + +- [#1001](https://github.com/qwikifiers/qwik-ui/issues/1001) +- [#979](https://github.com/qwikifiers/qwik-ui/issues/979) + +Also fixes for infinite scroll issues when both keyboard and mouse are used to navigate the listbox. diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index da7d2c42f..9189d3d51 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -47,8 +47,9 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # GITHUB_TOKEN is provided automatically in any repository - pagefind: - runs-on: ubuntu-latest - steps: - - name: Generate - run: npx pagefind --site dist/apps/website/client && cp -r dist/apps/website/client/pagefind apps/website/public/pagefind + + # pagefind: + # runs-on: ubuntu-latest + # steps: + # - name: Generate + # run: npx pagefind --site dist/apps/website/client && cp -r dist/apps/website/client/pagefind apps/website/public/pagefind diff --git a/apps/website/src/routes/docs/headless/combobox/examples/scrollable.tsx b/apps/website/src/routes/docs/headless/combobox/examples/scrollable.tsx index 60ba309f6..9f6bf2ac7 100644 --- a/apps/website/src/routes/docs/headless/combobox/examples/scrollable.tsx +++ b/apps/website/src/routes/docs/headless/combobox/examples/scrollable.tsx @@ -4,8 +4,7 @@ import { LuChevronDown } from '@qwikest/icons/lucide'; export default component$(() => { useStyles$(styles); - const activeUsers = ['Tim', 'Ryan', 'Jim', 'Abby']; - const offlineUsers = ['Joey', 'Bob', 'Jack', 'John']; + const items = Array.from({ length: 100 }, (_, i) => (i + 1).toString()); return ( @@ -17,23 +16,11 @@ export default component$(() => { - - Active - {activeUsers.map((user) => ( - - {user} - - ))} - - - - Offline - {offlineUsers.map((user) => ( - - {user} - - ))} - + {items.map((item) => ( + + {item} + + ))} ); diff --git a/apps/website/src/routes/docs/headless/select/auto-api/api.ts b/apps/website/src/routes/docs/headless/select/auto-api/api.ts new file mode 100644 index 000000000..f0f4a49de --- /dev/null +++ b/apps/website/src/routes/docs/headless/select/auto-api/api.ts @@ -0,0 +1,55 @@ +export const api = { + select: [ + { + 'hidden-select-option': [], + }, + { + 'hidden-select': [], + }, + { + 'select-description': [], + }, + { + 'select-display-value': [], + }, + { + 'select-error-message': [], + }, + { + 'select-group-label': [], + }, + { + 'select-group': [], + }, + { + 'select-inline': [], + }, + { + 'select-item-indicator': [], + }, + { + 'select-item-label': [], + }, + { + 'select-item': [], + }, + { + 'select-label': [], + }, + { + 'select-listbox': [], + }, + { + 'select-popover': [], + }, + { + 'select-root': [], + }, + { + 'select-trigger': [], + }, + { + 'use-select': [], + }, + ], +}; diff --git a/apps/website/src/routes/docs/headless/select/examples/first-value-selected.tsx b/apps/website/src/routes/docs/headless/select/examples/first-value-selected.tsx new file mode 100644 index 000000000..dd803b21e --- /dev/null +++ b/apps/website/src/routes/docs/headless/select/examples/first-value-selected.tsx @@ -0,0 +1,26 @@ +import { component$, useStyles$ } from '@builder.io/qwik'; +import { Select } from '@qwik-ui/headless'; + +export default component$(() => { + useStyles$(styles); + const users = ['Tim', 'Ryan', 'Jim', 'Jessie', 'Abby']; + + return ( + + Logged in users + + + + + {users.map((user) => ( + + {user} + + ))} + + + ); +}); + +// internal +import styles from '../snippets/select.css?inline'; diff --git a/apps/website/src/routes/docs/headless/select/examples/scrollable.tsx b/apps/website/src/routes/docs/headless/select/examples/scrollable.tsx index 0f9fa3bbe..38e9fa4f8 100644 --- a/apps/website/src/routes/docs/headless/select/examples/scrollable.tsx +++ b/apps/website/src/routes/docs/headless/select/examples/scrollable.tsx @@ -3,8 +3,7 @@ import { Select } from '@qwik-ui/headless'; export default component$(() => { useStyles$(styles); - const users = ['Tim', 'Ryan', 'Jim', 'Jessie', 'Abby']; - const animals = ['Dog', 'Cat', 'Bird', 'Fish', 'Snake']; + const items = Array.from({ length: 100 }, (_, i) => (i + 1).toString()); return ( @@ -12,22 +11,11 @@ export default component$(() => { - - People - {users.map((user) => ( - - {user} - - ))} - - - Animals - {animals.map((animal) => ( - - {animal} - - ))} - + {items.map((item) => ( + + {item} + + ))} ); diff --git a/packages/kit-headless/src/components/combobox/combobox-item.tsx b/packages/kit-headless/src/components/combobox/combobox-item.tsx index 7c5bad38b..90bfd71d4 100644 --- a/packages/kit-headless/src/components/combobox/combobox-item.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-item.tsx @@ -141,7 +141,7 @@ export const HComboboxItem = component$((props: HComboboxItemProps) => { const observer = new IntersectionObserver(checkVisibility$, { root: context.panelRef.value, - threshold: 1.0, + threshold: 0, }); cleanup(() => observer?.disconnect()); diff --git a/packages/kit-headless/src/components/select/select-context.ts b/packages/kit-headless/src/components/select/select-context.ts index a4187c50b..0310338f7 100644 --- a/packages/kit-headless/src/components/select/select-context.ts +++ b/packages/kit-headless/src/components/select/select-context.ts @@ -21,6 +21,8 @@ export type SelectContext = { selectedIndexSetSig: Signal>; highlightedIndexSig: Signal; currDisplayValueSig: Signal; + isKeyboardFocusSig: Signal; + isMouseOverPopupSig: Signal; isListboxOpenSig: Signal; isDisabledSig: Signal; localId: string; diff --git a/packages/kit-headless/src/components/select/select-item.tsx b/packages/kit-headless/src/components/select/select-item.tsx index ba6dd4b69..b880ae1f6 100644 --- a/packages/kit-headless/src/components/select/select-item.tsx +++ b/packages/kit-headless/src/components/select/select-item.tsx @@ -12,13 +12,13 @@ import { useOnWindow, QRL, } from '@builder.io/qwik'; -import { isServer } from '@builder.io/qwik/build'; import SelectContextId, { SelectItemContext, selectItemContextId, } from './select-context'; import { useSelect } from './use-select'; import { useCombinedRef } from '../../hooks/combined-refs'; +import { isServer } from '@builder.io/qwik/build'; export type SelectItemProps = PropsOf<'div'> & { /** Internal index we get from the inline component. Please see select-inline.tsx */ @@ -39,6 +39,7 @@ export const HSelectItem = component$((props) => { const localIndexSig = useSignal(null); const itemId = `${context.localId}-${_index}`; const typeaheadFnSig = useSignal Promise>>(); + const debounceTimeoutSig = useSignal(); const { selectionManager$, getNextEnabledItemIndex$, getPrevEnabledItemIndex$ } = useSelect(); @@ -60,7 +61,7 @@ export const HSelectItem = component$((props) => { if (disabled) return; if (context.highlightedIndexSig.value === _index) { - itemRef.value?.focus(); + itemRef.value?.focus({ preventScroll: true }); return true; } else { return false; @@ -81,17 +82,20 @@ export const HSelectItem = component$((props) => { const containerRect = context.popoverRef.value?.getBoundingClientRect(); const itemRect = itemRef.value?.getBoundingClientRect(); - if (!containerRect || !itemRect) return; + if (!containerRect || !itemRect || !context.isKeyboardFocusSig.value) return; // Calculates the offset to center the item within the container const offset = itemRect.top - containerRect.top - containerRect.height / 2 + itemRect.height / 2; - context.popoverRef.value?.scrollBy({ top: offset, ...context.scrollOptions }); + context.popoverRef.value?.scrollBy({ + top: document.hasFocus() ? offset : undefined, + ...context.scrollOptions, + }); } }); - useTask$(async function navigationTask({ track, cleanup }) { + useTask$(function handleScrolling({ track, cleanup }) { track(() => context.highlightedIndexSig.value); // update the context with the highlighted item ref @@ -100,25 +104,36 @@ export const HSelectItem = component$((props) => { } if (isServer || !context.popoverRef.value) return; - if (_index !== context.highlightedIndexSig.value) return; const hasScrollbar = context.popoverRef.value.scrollHeight > context.popoverRef.value.clientHeight; - if (!hasScrollbar) { - return; + if (!hasScrollbar) return; + + if (debounceTimeoutSig.value !== undefined) { + clearTimeout(debounceTimeoutSig.value); } - const observer = new IntersectionObserver(checkVisibility$, { - root: context.popoverRef.value, - threshold: 1.0, - }); + debounceTimeoutSig.value = setTimeout(() => { + if (props._index !== context.highlightedIndexSig.value) return; - cleanup(() => observer?.disconnect()); + const observer = new IntersectionObserver(checkVisibility$, { + root: context.popoverRef.value, + threshold: 0, + }); - if (itemRef.value) { - observer.observe(itemRef.value); - } + cleanup(() => observer?.disconnect()); + + if (itemRef.value) { + observer.observe(itemRef.value); + } + }, 100); + + cleanup(() => { + if (debounceTimeoutSig.value !== undefined) { + clearTimeout(debounceTimeoutSig.value); + } + }); }); const handleClick$ = $(async () => { @@ -167,6 +182,7 @@ export const HSelectItem = component$((props) => { const handleKeyDown$ = $(async (e: KeyboardEvent) => { typeaheadFnSig.value?.(e.key); + context.isKeyboardFocusSig.value = true; switch (e.key) { case 'ArrowDown': diff --git a/packages/kit-headless/src/components/select/select-popover.tsx b/packages/kit-headless/src/components/select/select-popover.tsx index 0b8660494..6e1e9a8ba 100644 --- a/packages/kit-headless/src/components/select/select-popover.tsx +++ b/packages/kit-headless/src/components/select/select-popover.tsx @@ -85,6 +85,11 @@ export const HSelectPopover = component$>((props) = initialLoadSig.value = false; }); + const handleMouseEnter$ = $(() => { + context.isKeyboardFocusSig.value = false; + context.isMouseOverPopupSig.value = true; + }); + return ( >((props) = aria-expanded={context.isListboxOpenSig.value ? 'true' : undefined} aria-multiselectable={context.multiple ? 'true' : undefined} aria-labelledby={triggerId} + onMouseEnter$={[handleMouseEnter$, props.onMouseEnter$]} {...rest} > diff --git a/packages/kit-headless/src/components/select/select-root.tsx b/packages/kit-headless/src/components/select/select-root.tsx index 449c9d812..27732df8f 100644 --- a/packages/kit-headless/src/components/select/select-root.tsx +++ b/packages/kit-headless/src/components/select/select-root.tsx @@ -44,13 +44,13 @@ type TMultiValue = type TStringOrArray = | { - multiple?: true; - onChange$?: QRL<(value: string[]) => void>; - } + multiple?: true; + onChange$?: QRL<(value: string[]) => void>; + } | { - multiple?: false; - onChange$?: QRL<(value: string) => void>; - }; + multiple?: false; + onChange$?: QRL<(value: string) => void>; + }; export type SelectProps = Omit< PropsOf<'div'>, @@ -150,7 +150,7 @@ export const HSelectImpl = component$ & InternalSelectProps }); const selectedIndexSetSig = useSignal>( - new Set(givenValuePropIndex ? [givenValuePropIndex] : []), + new Set([givenValuePropIndex ?? []].flat()), ); const highlightedIndexSig = useSignal(givenValuePropIndex ?? null); @@ -168,6 +168,8 @@ export const HSelectImpl = component$ & InternalSelectProps const highlightedItemRef = useSignal(); const isDisabledSig = useSignal(disabled ?? false); const isInvalidSig = useSignal(props.invalid ?? false); + const isKeyboardFocusSig = useSignal(false); + const isMouseOverPopupSig = useSignal(false); useTask$(({ track }) => { /** @@ -192,6 +194,8 @@ export const HSelectImpl = component$ & InternalSelectProps localId, highlightedIndexSig, selectedIndexSetSig, + isKeyboardFocusSig, + isMouseOverPopupSig, isListboxOpenSig, scrollOptions, loop, diff --git a/packages/kit-headless/src/components/select/select-trigger.tsx b/packages/kit-headless/src/components/select/select-trigger.tsx index 55e686591..f58391b43 100644 --- a/packages/kit-headless/src/components/select/select-trigger.tsx +++ b/packages/kit-headless/src/components/select/select-trigger.tsx @@ -56,6 +56,7 @@ export const HSelectTrigger = component$((props) => { const handleKeyDown$ = $(async (e: KeyboardEvent) => { if (!context.itemsMapSig.value) return; + context.isKeyboardFocusSig.value = true; if (!context.isListboxOpenSig.value) { typeahead$(e.key); diff --git a/packages/kit-headless/src/components/select/select.test.ts b/packages/kit-headless/src/components/select/select.test.ts index a56a10e66..deacb32f4 100644 --- a/packages/kit-headless/src/components/select/select.test.ts +++ b/packages/kit-headless/src/components/select/select.test.ts @@ -839,6 +839,19 @@ test.describe('Props', () => { await expect(d.getItemAt(3)).toHaveAttribute('data-highlighted'); await expect(d.getItemAt(3)).toHaveAttribute('aria-selected', 'true'); }); + + test(` + GIVEN a select + WHEN the first option is selected on render + THEN the first option should be displayed`, async ({ page }) => { + const { driver: d } = await setup(page, 'first-value-selected'); + + const expectedValue = await d.getItemAt(0).textContent(); + + await expect(d.getValueElement()).toHaveText(expectedValue!); + await expect(d.getItemAt(0)).toHaveAttribute('data-highlighted'); + await expect(d.getItemAt(0)).toHaveAttribute('aria-selected', 'true'); + }); }); test.describe('controlled', () => {