From f9c8af2032f77cf17af1642131dfcf875ae8d222 Mon Sep 17 00:00:00 2001
From: WofWca <wofwca@protonmail.com>
Date: Wed, 30 Oct 2024 14:41:59 +0400
Subject: [PATCH] WIP: refactor: `useRovingTabindex` to utilize IDs

This should allow us to more easily `setActiveElement` by its
ID, without having a reference to `HTMLElement`,
e.g. we can more easily set the initial chat list item.
This issue was mentioned in https://github.com/deltachat/deltachat-desktop/pull/4224:
> The initially "active" element is just the first chat item,
> and not the currently selected chat.
---
 .../src/components/chat/ChatListItem.tsx      |  20 +-
 .../frontend/src/contexts/RovingTabindex.tsx  | 172 +++++++++++++-----
 2 files changed, 131 insertions(+), 61 deletions(-)

diff --git a/packages/frontend/src/components/chat/ChatListItem.tsx b/packages/frontend/src/components/chat/ChatListItem.tsx
index 23f37fb7f9..c5fe2a4279 100644
--- a/packages/frontend/src/components/chat/ChatListItem.tsx
+++ b/packages/frontend/src/components/chat/ChatListItem.tsx
@@ -174,17 +174,14 @@ function ChatListItemArchiveLink({
     },
   ])
 
-  const ref = useRef<HTMLButtonElement>(null)
-
   const {
     tabIndex,
     onKeydown: tabindexOnKeydown,
     setAsActiveElement: tabindexSetAsActiveElement,
-  } = useRovingTabindex(ref)
+  } = useRovingTabindex()
 
   return (
     <button
-      ref={ref}
       tabIndex={tabIndex}
       onClick={onClick}
       onKeyDown={tabindexOnKeydown}
@@ -222,17 +219,14 @@ function ChatListItemError({
 }) {
   log.info('Error Loading Chatlistitem ' + chatListItem.id, chatListItem.error)
 
-  const ref = useRef<HTMLButtonElement>(null)
-
   const {
     tabIndex,
     onKeydown: tabindexOnKeydown,
     setAsActiveElement: tabindexSetAsActiveElement,
-  } = useRovingTabindex(ref)
+  } = useRovingTabindex(chatListItem.id)
 
   return (
     <button
-      ref={ref}
       tabIndex={tabIndex}
       onClick={onClick}
       onKeyDown={tabindexOnKeydown}
@@ -284,18 +278,15 @@ function ChatListItemNormal({
   isSelected?: boolean
   hover?: boolean
 }) {
-  const ref = useRef<HTMLButtonElement>(null)
-
   const {
     tabIndex,
     onKeydown: tabindexOnKeydown,
     setAsActiveElement: tabindexSetAsActiveElement,
-  } = useRovingTabindex(ref)
+  } = useRovingTabindex(chatListItem.id)
   // TODO `setAsActiveElement` if `isSelected` and `activeElement === null`
 
   return (
     <button
-      ref={ref}
       tabIndex={tabIndex}
       onClick={onClick}
       onKeyDown={tabindexOnKeydown}
@@ -410,19 +401,16 @@ export const ChatListItemMessageResult = React.memo<{
 }>(props => {
   const { msr, onClick, queryStr } = props
 
-  const ref = useRef<HTMLButtonElement>(null)
-
   const {
     tabIndex,
     onKeydown: tabindexOnKeydown,
     setAsActiveElement: tabindexSetAsActiveElement,
-  } = useRovingTabindex(ref)
+  } = useRovingTabindex()
 
   if (typeof msr === 'undefined') return <PlaceholderChatListItem />
 
   return (
     <button
-      ref={ref}
       tabIndex={tabIndex}
       onClick={onClick}
       onKeyDown={tabindexOnKeydown}
diff --git a/packages/frontend/src/contexts/RovingTabindex.tsx b/packages/frontend/src/contexts/RovingTabindex.tsx
index a68199ae37..a9fef6f75f 100644
--- a/packages/frontend/src/contexts/RovingTabindex.tsx
+++ b/packages/frontend/src/contexts/RovingTabindex.tsx
@@ -6,6 +6,8 @@ import React, {
   useCallback,
   useContext,
   useEffect,
+  useRef,
+  // useEffect,
   useState,
 } from 'react'
 
@@ -30,15 +32,61 @@ import React, {
  *   Then `tabindex` is 0 for all the components,
  *   until `setAsActiveElement` is called explicitly.
  * - lacks "Home", "End" key handling, and some other features.
+ *
+ * @param elementId any value to identify this element among others when using
+ * with `setActiveElement`.
+ * `null` must not be used - it's a special value.
  */
-export function useRovingTabindex(elementRef: RefObject<HTMLElement>) {
+export function useRovingTabindex(elementId?: unknown) {
+  const defaultId = useRef(Symbol())
+  if (elementId === undefined) {
+    elementId = defaultId
+  }
+
   const context = useContext(RovingTabindexContext)
 
+  // const unsetAsActiveElement = context.unsetAsActiveElement
+  // useEffect(() => {
+  //   return () => unsetAsActiveElement(elementId)
+  //   // TODO fix: this is not gonna work because `unsetAsActiveElement`
+  //   // changes every time we set a different active element.
+  // }, [])
+  // const unsetAsActiveElement = context.unsetAsActiveElement
+
+  // useEffect(() => {
+  //   if (context.activeElementId !== elementId) {
+  //     return
+  //   }
+  //   return () => {
+  //     // unsetAsActiveElement(elementId)
+  //     context.setActiveElement(null)
+  //   }
+  // }, [context, context.activeElementId, elementId])
+
+  // const mounted = useRef<boolean>(true)
+  const unsetAsActiveTimeoutId = useRef<number>(-1)
+  useEffect(() => {
+    clearTimeout(unsetAsActiveTimeoutId.current)
+
+    if (context.activeElementId !== elementId) {
+      return
+    }
+
+    // A bit of a hack to unset the element as active when
+    // the component gets unmounted.
+    return () => {
+      unsetAsActiveTimeoutId.current = window.setTimeout(() => {
+        if (context.activeElementId === elementId) {
+          context.setActiveElement(null)
+        }
+      })
+    }
+  }, [context, elementId])
+
   const tabIndex: 0 | -1 =
     // If the active element has not been chosen yet,
     // let' keep the default behavior (tabindex="0")
-    context.activeElement == null ||
-    context.activeElement === elementRef.current
+    context.activeElementId == null || context.activeElementId === elementId
       ? 0
       : -1
 
@@ -64,17 +112,18 @@ export function useRovingTabindex(elementRef: RefObject<HTMLElement>) {
      */
     // `elementRef.current` could be null, I am not sure
     // if this means something to us practically.
-    setAsActiveElement: () => context.setActiveElement(elementRef.current),
-    activeElement: context.activeElement,
+    setAsActiveElement: () => context.setActiveElement(elementId),
+    activeElementId: context.activeElementId,
   }
 }
 
 const log = getLogger('contexts/RovingTabindex')
 
 type ContextValue = {
-  activeElement: HTMLElement | null
+  activeElementId: unknown | null
   onKeydown: (event: React.KeyboardEvent) => void
-  setActiveElement: (element: HTMLElement | null) => void
+  setActiveElement: (elementId: unknown | null) => void
+  // unsetAsActiveElement: (elementId: unknown) => void
 }
 
 type ProviderProps = PropsWithChildren<{
@@ -92,9 +141,10 @@ type ProviderProps = PropsWithChildren<{
 }>
 
 export const RovingTabindexContext = createContext<ContextValue>({
-  activeElement: null,
+  activeElementId: null,
   onKeydown: () => {},
   setActiveElement: () => {},
+  // unsetAsActiveElement: () => {},
 })
 
 /** @see {@link useRovingTabindex} */
@@ -107,47 +157,57 @@ export function RovingTabindexProvider({
     classNameOfTargetElements = 'roving-tabindex'
   }
 
-  const [activeElement, setActiveElement] = useState<HTMLElement | null>(null)
-
-  // Ensure that the "active" element is actually in DOM,
-  // because it's the only one that is keyboard focusable.
-  // Otherwise it's impossible to focus the widget by pressing "Tab".
-  // If it's not in DOM, we need to either set another element
-  // as the active one, or `setActiveElement(null)` to make
-  // all the elements focusable.
-  //
-  // FYI instead of using a `MutationObserver` we could probaby have
-  // utilized React's "unmount" callback (i.e. `useEffect`'s cleanup function),
-  // but I failed at it.
-  // Let's keep things vanilla then I guess.
-  useEffect(() => {
-    if (activeElement == null) {
-      return
-    }
+  const [activeElementId, setActiveElementId] = useState<unknown | null>(null)
 
-    const observer = new MutationObserver(_mutations => {
-      if (!document.body.contains(activeElement)) {
-        // Another option is to select the next/closest element
-        // as the new active one.
-        setActiveElement(null)
-      }
-    })
-    // Maybe we could observe `wrapperElementRef` for performance,
-    // but IDK let's play it safe.
-    observer.observe(document.body, { childList: true, subtree: true })
-    return () => observer.disconnect()
-  }, [activeElement])
+  // // Ensure that the "active" element is actually in DOM,
+  // // because it's the only one that is keyboard focusable.
+  // // Otherwise it's impossible to focus the widget by pressing "Tab".
+  // // If it's not in DOM, we need to either set another element
+  // // as the active one, or `setActiveElement(null)` to make
+  // // all the elements focusable.
+  // //
+  // // FYI instead of using a `MutationObserver` we could probaby have
+  // // utilized React's "unmount" callback (i.e. `useEffect`'s cleanup function),
+  // // but I failed at it.
+  // // Let's keep things vanilla then I guess.
+  // useEffect(() => {
+  //   if (activeElementId == null) {
+  //     return
+  //   }
+
+  //   const observer = new MutationObserver(_mutations => {
+  //     if (!document.body.contains(activeElementId)) {
+  //       // Another option is to select the next/closest element
+  //       // as the new active one.
+  //       setActiveElementId(null)
+  //     }
+  //   })
+  //   // Maybe we could observe `wrapperElementRef` for performance,
+  //   // but IDK let's play it safe.
+  //   observer.observe(document.body, { childList: true, subtree: true })
+  //   return () => observer.disconnect()
+  // }, [activeElementId])
+
+  // const unsetAsActiveElement = (elementId: unknown) => {
+  //   if (activeElementId === elementId) {
+  //     setActiveElementId(null)
+  //   }
+  // }
 
   const onKeydown = useCallback(
     (event: React.KeyboardEvent) => {
       if (!wrapperElementRef.current) {
         log.warn(
           'Received keydown event, but there is no wrapperElement? How?',
-          activeElement
+          activeElementId
         )
         return
       }
-      if (!activeElement) {
+      // TODO now that we utilize `document.activeElement`
+      // (or `event.target`?) to find the currently active element,
+      // do we even need to access `activeElementId`
+      // in `onKeydown` at all?
+      if (activeElementId === null) {
         // This could happen either after the initial render,
         // or if the active element was removed from DOM.
         // Let's just wait for the user to focus another element
@@ -174,7 +234,21 @@ export function RovingTabindexProvider({
       let oldActiveElementInd: number | undefined
       for (let i = 0; i < eligibleElements.length; i++) {
         const eligibleEl = eligibleElements[i]
-        if (eligibleEl === activeElement) {
+
+
+        // TODO maybe utilize `event.target` instead?
+        // When would it be beneficial?
+        // Well, I guess we wouldn't have to check `eligibleEl.contains()`.
+        // Is that all?
+        //
+        // I think `document.activeElement` just makes more sense,
+        // because this entire "library" is about managing focus.
+        if (
+          eligibleEl === document.activeElement ||
+          // also check `contains()` because we must support the active element
+          // having interactive children.
+          eligibleEl.contains(document.activeElement)
+        ) {
           oldActiveElementInd = i
           break
         }
@@ -182,7 +256,7 @@ export function RovingTabindexProvider({
       if (oldActiveElementInd == undefined) {
         log.warn(
           'Could not find the currently active element in DOM',
-          activeElement
+          activeElementId
         )
         return
       }
@@ -195,22 +269,30 @@ export function RovingTabindexProvider({
       // the last or the first.
       if (newActiveElement != undefined) {
         const newActiveElement_ = newActiveElement as HTMLElement
-        setActiveElement(newActiveElement_)
+
+        // Here we could `setActiveElementId()`, but we don't know its ID.
+        // However, things will still work out fine because
+        // after we `newActiveElement_.focus()`,
+        // it will fire the onFocus event,
+        // which should invoke `setAsActiveElement`
+        // (as long as the user attached the event listeners properly).
+
         // It is fine to `.focus()` here without wating for a render
         // or `useEffect`, because elements with `tabindex="-1"`
         // are still programmatically focusable.
         newActiveElement_.focus()
       }
     },
-    [activeElement, classNameOfTargetElements, wrapperElementRef]
+    [activeElementId, classNameOfTargetElements, wrapperElementRef]
   )
 
   return (
     <RovingTabindexContext.Provider
       value={{
-        activeElement,
+        activeElementId,
         onKeydown,
-        setActiveElement,
+        setActiveElement: setActiveElementId,
+        // unsetAsActiveElement,
       }}
     >
       {children}