From fdd4dd1b016b9bc26027dd4facd51ecbf9a5679e Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 9 Mar 2022 17:34:56 +0100 Subject: [PATCH] Remove `focus()` from Listbox Option (#1218) * cleanup auto-scrolling We keep the actual container focused, so we don't require the invidiual option to be focused as well. We do want to scroll it into view but that's part of another piece of code. Also cleaned up some manual `document.querySelector` calls now that we keep track of a `ref`. * update changelog --- CHANGELOG.md | 2 ++ .../src/components/listbox/listbox.tsx | 30 +++++++++---------- .../src/components/listbox/listbox.ts | 9 +++--- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c85d81..3cc279e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Adjust active {item,option} index ([#1184](https://github.com/tailwindlabs/headlessui/pull/1184)) - Only activate the `Tab` on mouseup ([#1192](https://github.com/tailwindlabs/headlessui/pull/1192)) - Ignore "outside click" on removed elements ([#1193](https://github.com/tailwindlabs/headlessui/pull/1193)) +- Remove `focus()` from Listbox Option ([#1218](https://github.com/tailwindlabs/headlessui/pull/1218)) ### Added @@ -44,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix `Dialog` cycling ([#553](https://github.com/tailwindlabs/headlessui/pull/553)) - Only activate the `Tab` on mouseup ([#1192](https://github.com/tailwindlabs/headlessui/pull/1192)) - Ignore "outside click" on removed elements ([#1193](https://github.com/tailwindlabs/headlessui/pull/1193)) +- Remove `focus()` from Listbox Option ([#1218](https://github.com/tailwindlabs/headlessui/pull/1218)) ### Added diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index 58b239b..f4540e3 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -15,6 +15,7 @@ import React, { MouseEvent as ReactMouseEvent, MutableRefObject, Ref, + useEffect, } from 'react' import { useDisposables } from '../../hooks/use-disposables' @@ -554,7 +555,7 @@ let Options = forwardRefWithAs(function Options< return state.listboxState === ListboxStates.Open })() - useIsoMorphicEffect(() => { + useEffect(() => { let container = state.optionsRef.current if (!container) return if (state.listboxState !== ListboxStates.Open) return @@ -706,6 +707,17 @@ let Option = forwardRefWithAs(function Option< let internalOptionRef = useRef(null) let optionRef = useSyncRefs(ref, internalOptionRef) + useIsoMorphicEffect(() => { + if (state.listboxState !== ListboxStates.Open) return + if (!active) return + if (state.activationTrigger === ActivationTrigger.Pointer) return + let d = disposables() + d.requestAnimationFrame(() => { + internalOptionRef.current?.scrollIntoView?.({ block: 'nearest' }) + }) + return d.dispose + }, [internalOptionRef, active, state.listboxState, state.activationTrigger, /* We also want to trigger this when the position of the active item changes so that we can re-trigger the scrollIntoView */ state.activeOptionIndex]) + let bag = useRef({ disabled, value, domRef: internalOptionRef }) useIsoMorphicEffect(() => { @@ -715,8 +727,8 @@ let Option = forwardRefWithAs(function Option< bag.current.value = value }, [bag, value]) useIsoMorphicEffect(() => { - bag.current.textValue = document.getElementById(id)?.textContent?.toLowerCase() - }, [bag, id]) + bag.current.textValue = internalOptionRef.current?.textContent?.toLowerCase() + }, [bag, internalOptionRef]) let select = useCallback(() => state.propsRef.current.onChange(value), [state.propsRef, value]) @@ -729,20 +741,8 @@ let Option = forwardRefWithAs(function Option< if (state.listboxState !== ListboxStates.Open) return if (!selected) return dispatch({ type: ActionTypes.GoToOption, focus: Focus.Specific, id }) - document.getElementById(id)?.focus?.() }, [state.listboxState]) - useIsoMorphicEffect(() => { - if (state.listboxState !== ListboxStates.Open) return - if (!active) return - if (state.activationTrigger === ActivationTrigger.Pointer) return - let d = disposables() - d.requestAnimationFrame(() => { - document.getElementById(id)?.scrollIntoView?.({ block: 'nearest' }) - }) - return d.dispose - }, [id, active, state.listboxState, state.activationTrigger, /* We also want to trigger this when the position of the active item changes so that we can re-trigger the scrollIntoView */ state.activeOptionIndex]) - let handleClick = useCallback( (event: { preventDefault: Function }) => { if (disabled) return event.preventDefault() diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.ts b/packages/@headlessui-vue/src/components/listbox/listbox.ts index 78f5233..bf4e832 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.ts +++ b/packages/@headlessui-vue/src/components/listbox/listbox.ts @@ -473,8 +473,8 @@ export let ListboxOptions = defineComponent({ event.preventDefault() event.stopPropagation() if (api.activeOptionIndex.value !== null) { - let { dataRef } = api.options.value[api.activeOptionIndex.value] - api.select(dataRef.value) + let activeOption = api.options.value[api.activeOptionIndex.value] + api.select(activeOption.dataRef.value) } api.closeListbox() nextTick(() => dom(api.buttonRef)?.focus({ preventScroll: true })) @@ -592,7 +592,7 @@ export let ListboxOption = defineComponent({ domRef: internalOptionRef, })) onMounted(() => { - let textValue = document.getElementById(id)?.textContent?.toLowerCase().trim() + let textValue = dom(internalOptionRef)?.textContent?.toLowerCase().trim() if (textValue !== undefined) dataRef.value.textValue = textValue }) @@ -606,7 +606,6 @@ export let ListboxOption = defineComponent({ if (api.listboxState.value !== ListboxStates.Open) return if (!selected.value) return api.goToOption(Focus.Specific, id) - document.getElementById(id)?.focus?.() }, { immediate: true } ) @@ -616,7 +615,7 @@ export let ListboxOption = defineComponent({ if (api.listboxState.value !== ListboxStates.Open) return if (!active.value) return if (api.activationTrigger.value === ActivationTrigger.Pointer) return - nextTick(() => document.getElementById(id)?.scrollIntoView?.({ block: 'nearest' })) + nextTick(() => dom(internalOptionRef)?.scrollIntoView?.({ block: 'nearest' })) }) function handleClick(event: MouseEvent) {