Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: refactor: useRovingTabindex to utilize IDs #4292

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP: refactor: useRovingTabindex to utilize IDs #4292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions packages/frontend/src/components/chat/ChatListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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}
Expand Down
183 changes: 138 additions & 45 deletions packages/frontend/src/contexts/RovingTabindex.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import React, {
useCallback,
useContext,
useEffect,
useRef,
// useEffect,
useState,
} from 'react'

Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=== null

? 0
: -1

Expand All @@ -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<{
Expand All @@ -92,9 +141,10 @@ type ProviderProps = PropsWithChildren<{
}>

export const RovingTabindexContext = createContext<ContextValue>({
activeElement: null,
activeElementId: null,
onKeydown: () => {},
setActiveElement: () => {},
// unsetAsActiveElement: () => {},
})

/** @see {@link useRovingTabindex} */
Expand All @@ -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
Expand All @@ -174,15 +234,40 @@ 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, which might be focused.
// TODO or should we? In
// https://github.com/deltachat/deltachat-desktop/pull/4376
// and
// https://github.com/deltachat/deltachat-desktop/pull/4294
// we raise a point that videos and audios actually
// utilize arrow keys, and we must not switch focus
// if all the user is trying to do is change volume...
//
// But at least in WhatsApp arrow keys switch between messages
// even when an element inside a message is focused
// (not the message itself).
eligibleEl.contains(document.activeElement)
) {
oldActiveElementInd = i
break
}
}
if (oldActiveElementInd == undefined) {
log.warn(
'Could not find the currently active element in DOM',
activeElement
activeElementId
)
return
}
Expand All @@ -195,22 +280,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}
Expand Down