Improve some internal code (#1221)

* remove raw `document.getElementById` calls When we introduced the `forwardRef` for all components, we also made sure that internal `ref`s were used to keep track of the actual DOM node. This code prefers the `internalXXRef` refs in favor of the `document.getElementById` calls. This is way more React-ish, and also fixes a few issues: - Potential performance improvements (no need to re-query the DOM, since we already have a reference to the DOM node). Note: this is a *guess*, I didn't measure this. - It could be that the element is rendered in another `document`, the correct would involve something like `someDOMNode.ownerDocument.getElementById(...)` but that should not be necessary anymore now. * make Disclosure implementation between React & Vue consistent * use a similar convention for DOM refs to other components * update changelog
tailwindlabs · Mar 9, 2022 · 07c3a61 · 07c3a61
1 parent fdd4dd1
commit 07c3a61
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - 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))
+- Improve some internal code ([#1221](https://github.com/tailwindlabs/headlessui/pull/1221))
 
 ### Added
 
@@ -46,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - 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))
+- Improve some internal code ([#1221](https://github.com/tailwindlabs/headlessui/pull/1221))
 
 ### Added
 

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx
@@ -895,8 +895,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(() => actions.selectOption(id), [actions, id])
 
@@ -928,10 +928,10 @@ let Option = forwardRefWithAs(function Option<
     if (state.activationTrigger === ActivationTrigger.Pointer) return
     let d = disposables()
     d.requestAnimationFrame(() => {
-      document.getElementById(id)?.scrollIntoView?.({ block: 'nearest' })
+      internalOptionRef.current?.scrollIntoView?.({ block: 'nearest' })
     })
     return d.dispose
-  }, [id, active, state.comboboxState, 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])
+  }, [internalOptionRef, active, state.comboboxState, 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 }) => {

diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.tsx
@@ -39,6 +39,9 @@ interface StateDefinition {
 
   linkedPanel: boolean
 
+  buttonRef: MutableRefObject<HTMLButtonElement | null>
+  panelRef: MutableRefObject<HTMLDivElement | null>
+
   buttonId: string
   panelId: string
 }
@@ -157,9 +160,14 @@ let DisclosureRoot = forwardRefWithAs(function Disclosure<
   let panelId = `headlessui-disclosure-panel-${useId()}`
   let disclosureRef = useSyncRefs(ref)
 
+  let panelRef = useRef<StateDefinition['panelRef']['current']>(null)
+  let buttonRef = useRef<StateDefinition['buttonRef']['current']>(null)
+
   let reducerBag = useReducer(stateReducer, {
     disclosureState: defaultOpen ? DisclosureStates.Open : DisclosureStates.Closed,
     linkedPanel: false,
+    buttonRef,
+    panelRef,
     buttonId,
     panelId,
   } as StateDefinition)
@@ -232,12 +240,12 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
   ref: Ref<HTMLButtonElement>
 ) {
   let [state, dispatch] = useDisclosureContext('Disclosure.Button')
-  let internalButtonRef = useRef<HTMLButtonElement | null>(null)
-  let buttonRef = useSyncRefs(internalButtonRef, ref)
-
   let panelContext = useDisclosurePanelContext()
   let isWithinPanel = panelContext === null ? false : panelContext === state.panelId
 
+  let internalButtonRef = useRef<HTMLButtonElement | null>(null)
+  let buttonRef = useSyncRefs(internalButtonRef, ref, !isWithinPanel ? state.buttonRef : null)
+
   let handleKeyDown = useCallback(
     (event: ReactKeyboardEvent<HTMLButtonElement>) => {
       if (isWithinPanel) {
@@ -249,7 +257,7 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
             event.preventDefault()
             event.stopPropagation()
             dispatch({ type: ActionTypes.ToggleDisclosure })
-            document.getElementById(state.buttonId)?.focus()
+            state.buttonRef.current?.focus()
             break
         }
       } else {
@@ -263,7 +271,7 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
         }
       }
     },
-    [dispatch, isWithinPanel, state.disclosureState, state.buttonId]
+    [dispatch, isWithinPanel, state.disclosureState, state.buttonRef]
   )
 
   let handleKeyUp = useCallback((event: ReactKeyboardEvent<HTMLButtonElement>) => {
@@ -284,12 +292,12 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
 
       if (isWithinPanel) {
         dispatch({ type: ActionTypes.ToggleDisclosure })
-        document.getElementById(state.buttonId)?.focus()
+        state.buttonRef.current?.focus()
       } else {
         dispatch({ type: ActionTypes.ToggleDisclosure })
       }
     },
-    [dispatch, props.disabled, state.buttonId, isWithinPanel]
+    [dispatch, props.disabled, state.buttonRef, isWithinPanel]
   )
 
   let slot = useMemo<ButtonRenderPropArg>(
@@ -341,7 +349,7 @@ let Panel = forwardRefWithAs(function Panel<TTag extends ElementType = typeof DE
   let [state, dispatch] = useDisclosureContext('Disclosure.Panel')
   let { close } = useDisclosureAPIContext('Disclosure.Panel')
 
-  let panelRef = useSyncRefs(ref, () => {
+  let panelRef = useSyncRefs(ref, state.panelRef, () => {
     if (state.linkedPanel) return
     dispatch({ type: ActionTypes.LinkPanel })
   })

diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx
@@ -453,8 +453,8 @@ let Items = forwardRefWithAs(function Items<TTag extends ElementType = typeof DE
           event.stopPropagation()
           dispatch({ type: ActionTypes.CloseMenu })
           if (state.activeItemIndex !== null) {
-            let { id } = state.items[state.activeItemIndex]
-            document.getElementById(id)?.click()
+            let { dataRef } = state.items[state.activeItemIndex]
+            dataRef.current?.domRef.current?.click()
           }
           disposables().nextFrame(() => state.buttonRef.current?.focus({ preventScroll: true }))
           break
@@ -580,20 +580,19 @@ let Item = forwardRefWithAs(function Item<TTag extends ElementType = typeof DEFA
     if (state.activationTrigger === ActivationTrigger.Pointer) return
     let d = disposables()
     d.requestAnimationFrame(() => {
-      document.getElementById(id)?.scrollIntoView?.({ block: 'nearest' })
+      internalItemRef.current?.scrollIntoView?.({ block: 'nearest' })
     })
     return d.dispose
-  }, [id, active, state.menuState, 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.activeItemIndex])
+  }, [internalItemRef, active, state.menuState, 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.activeItemIndex])
 
   let bag = useRef<MenuItemDataRef['current']>({ disabled, domRef: internalItemRef })
 
   useIsoMorphicEffect(() => {
     bag.current.disabled = disabled
   }, [bag, disabled])
-
   useIsoMorphicEffect(() => {
-    bag.current.textValue = document.getElementById(id)?.textContent?.toLowerCase()
-  }, [bag, id])
+    bag.current.textValue = internalItemRef.current?.textContent?.toLowerCase()
+  }, [bag, internalItemRef])
 
   useIsoMorphicEffect(() => {
     dispatch({ type: ActionTypes.RegisterItem, id, dataRef: bag })

diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts
@@ -708,7 +708,7 @@ export let ComboboxOption = defineComponent({
       if (api.comboboxState.value !== ComboboxStates.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) {

diff --git a/packages/@headlessui-vue/src/components/disclosure/disclosure.ts b/packages/@headlessui-vue/src/components/disclosure/disclosure.ts
@@ -139,17 +139,17 @@ export let DisclosureButton = defineComponent({
     let panelContext = useDisclosurePanelContext()
     let isWithinPanel = panelContext === null ? false : panelContext === api.panelId
 
-    let elementRef = ref(null)
+    let internalButtonRef = ref(null)
 
     if (!isWithinPanel) {
       watchEffect(() => {
-        api.button.value = elementRef.value
+        api.button.value = internalButtonRef.value
       })
     }
 
     let type = useResolveButtonType(
       computed(() => ({ as: props.as, type: attrs.type })),
-      elementRef
+      internalButtonRef
     )
 
     function handleClick() {
@@ -201,14 +201,14 @@ export let DisclosureButton = defineComponent({
       let slot = { open: api.disclosureState.value === DisclosureStates.Open }
       let propsWeControl = isWithinPanel
         ? {
-            ref: elementRef,
+            ref: internalButtonRef,
             type: type.value,
             onClick: handleClick,
             onKeydown: handleKeyDown,
           }
         : {
             id: api.buttonId,
-            ref: elementRef,
+            ref: internalButtonRef,
             type: type.value,
             'aria-expanded': props.disabled
               ? undefined

diff --git a/packages/@headlessui-vue/src/components/menu/menu.ts b/packages/@headlessui-vue/src/components/menu/menu.ts
@@ -366,8 +366,9 @@ export let MenuItems = defineComponent({
           event.preventDefault()
           event.stopPropagation()
           if (api.activeItemIndex.value !== null) {
-            let { id } = api.items.value[api.activeItemIndex.value]
-            document.getElementById(id)?.click()
+            let activeItem = api.items.value[api.activeItemIndex.value]
+            let _activeItem = activeItem as unknown as UnwrapNestedRefs<typeof activeItem>
+            dom(_activeItem.dataRef.domRef)?.click()
           }
           api.closeMenu()
           nextTick(() => dom(api.buttonRef)?.focus({ preventScroll: true }))
@@ -490,7 +491,7 @@ export let MenuItem = defineComponent({
       domRef: internalItemRef,
     }))
     onMounted(() => {
-      let textValue = document.getElementById(id)?.textContent?.toLowerCase().trim()
+      let textValue = dom(internalItemRef)?.textContent?.toLowerCase().trim()
       if (textValue !== undefined) dataRef.value.textValue = textValue
     })
 
@@ -501,7 +502,7 @@ export let MenuItem = defineComponent({
       if (api.menuState.value !== MenuStates.Open) return
       if (!active.value) return
       if (api.activationTrigger.value === ActivationTrigger.Pointer) return
-      nextTick(() => document.getElementById(id)?.scrollIntoView?.({ block: 'nearest' }))
+      nextTick(() => dom(internalItemRef)?.scrollIntoView?.({ block: 'nearest' }))
     })
 
     function handleClick(event: MouseEvent) {