Skip to content

Commit

Permalink
Internal refactor: use flushSync() instead of d.nextFrame() (#3263)
Browse files Browse the repository at this point in the history 
* use `flushSync` instead of `d.nextFrame`

This guarantees that after the `flushSync` call the DOM is updated. This
means that we don't have to guess and delay by a double
`requestAnimationFrame` (`nextFrame`) and _hope_ that the DOM was
updated already.

* inline disposables call

Each function in the `disposables()` object returns a cleanup function
which means we can return this directly.

* inline if-statements

Small one, but consistent with `<Menu />` and `<Listbox />` components.

* inline `flushSync()` callbacks
  • Loading branch information
@RobinMalfait
RobinMalfait authored Jun 3, 2024
1 parent 479853d commit 2d3ec80
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 73 deletions.
50 changes: 19 additions & 31 deletions packages/@headlessui-react/src/components/combobox/combobox.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import React, {
type MouseEvent as ReactMouseEvent,
type Ref,
} from 'react'
import { flushSync } from 'react-dom'
import { useActivePress } from '../../hooks/use-active-press'
import { useByComparator, type ByComparator } from '../../hooks/use-by-comparator'
import { useControllable } from '../../hooks/use-controllable'
Expand Down Expand Up @@ -1189,12 +1190,8 @@ function InputFn<
return match(data.comboboxState, {
[ComboboxState.Open]: () => actions.goToOption(Focus.Previous),
[ComboboxState.Closed]: () => {
actions.openCombobox()
d.nextFrame(() => {
if (!data.value) {
actions.goToOption(Focus.Last)
}
})
flushSync(() => actions.openCombobox())
if (!data.value) actions.goToOption(Focus.Last)
},
})

Expand Down Expand Up @@ -1320,14 +1317,12 @@ function InputFn<
if (!data.immediate) return
if (data.comboboxState === ComboboxState.Open) return

actions.openCombobox()
flushSync(() => actions.openCombobox())

// We need to make sure that tabbing through a form doesn't result in incorrectly setting the
// value of the combobox. We will set the activation trigger to `Focus`, and we will ignore
// selecting the active option when the user tabs away.
d.nextFrame(() => {
actions.setActivationTrigger(ActivationTrigger.Focus)
})
actions.setActivationTrigger(ActivationTrigger.Focus)
})

let labelledBy = useLabelledBy()
Expand Down Expand Up @@ -1439,7 +1434,6 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
autoFocus = false,
...theirProps
} = props
let d = useDisposables()

let refocusInput = useRefocusableInput(data.inputRef)

Expand All @@ -1452,46 +1446,40 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
event.preventDefault()
event.stopPropagation()
if (data.comboboxState === ComboboxState.Closed) {
actions.openCombobox()
flushSync(() => actions.openCombobox())
}

return d.nextFrame(() => refocusInput())
refocusInput()
return

case Keys.ArrowDown:
event.preventDefault()
event.stopPropagation()
if (data.comboboxState === ComboboxState.Closed) {
actions.openCombobox()
d.nextFrame(() => {
if (!data.value) {
actions.goToOption(Focus.First)
}
})
flushSync(() => actions.openCombobox())
if (!data.value) actions.goToOption(Focus.First)
}

return d.nextFrame(() => refocusInput())
refocusInput()
return

case Keys.ArrowUp:
event.preventDefault()
event.stopPropagation()
if (data.comboboxState === ComboboxState.Closed) {
actions.openCombobox()
d.nextFrame(() => {
if (!data.value) {
actions.goToOption(Focus.Last)
}
})
flushSync(() => actions.openCombobox())
if (!data.value) actions.goToOption(Focus.Last)
}
return d.nextFrame(() => refocusInput())
refocusInput()
return

case Keys.Escape:
if (data.comboboxState !== ComboboxState.Open) return
event.preventDefault()
if (data.optionsRef.current && !data.optionsPropsRef.current.static) {
event.stopPropagation()
}
actions.closeCombobox()
return d.nextFrame(() => refocusInput())
flushSync(() => actions.closeCombobox())
refocusInput()
return

default:
return
Expand Down
37 changes: 15 additions & 22 deletions packages/@headlessui-react/src/components/listbox/listbox.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import React, {
type MouseEvent as ReactMouseEvent,
type Ref,
} from 'react'
import { flushSync } from 'react-dom'
import { useActivePress } from '../../hooks/use-active-press'
import { useByComparator, type ByComparator } from '../../hooks/use-by-comparator'
import { useComputed } from '../../hooks/use-computed'
Expand Down Expand Up @@ -755,8 +756,6 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
let buttonRef = useSyncRefs(data.buttonRef, ref, useFloatingReference())
let getFloatingReferenceProps = useFloatingReferenceProps()

let d = useDisposables()

let handleKeyDown = useEvent((event: ReactKeyboardEvent<HTMLButtonElement>) => {
switch (event.key) {
// Ref: https://www.w3.org/WAI/ARIA/apg/patterns/menubutton/#keyboard-interaction-13
Expand All @@ -768,18 +767,14 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
case Keys.Space:
case Keys.ArrowDown:
event.preventDefault()
actions.openListbox()
d.nextFrame(() => {
if (!data.value) actions.goToOption(Focus.First)
})
flushSync(() => actions.openListbox())
if (!data.value) actions.goToOption(Focus.First)
break

case Keys.ArrowUp:
event.preventDefault()
actions.openListbox()
d.nextFrame(() => {
if (!data.value) actions.goToOption(Focus.Last)
})
flushSync(() => actions.openListbox())
if (!data.value) actions.goToOption(Focus.Last)
break
}
})
Expand All @@ -798,8 +793,8 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
let handleClick = useEvent((event: ReactMouseEvent) => {
if (isDisabledReactIssue7711(event.currentTarget)) return event.preventDefault()
if (data.listboxState === ListboxStates.Open) {
actions.closeListbox()
d.nextFrame(() => data.buttonRef.current?.focus({ preventScroll: true }))
flushSync(() => actions.closeListbox())
data.buttonRef.current?.focus({ preventScroll: true })
} else {
event.preventDefault()
actions.openListbox()
Expand Down Expand Up @@ -995,7 +990,6 @@ function OptionsFn<TTag extends ElementType = typeof DEFAULT_OPTIONS_TAG>(
let getFloatingPanelProps = useFloatingPanelProps()
let optionsRef = useSyncRefs(data.optionsRef, ref, anchor ? floatingRef : null)

let d = useDisposables()
let searchDisposables = useDisposables()

useEffect(() => {
Expand Down Expand Up @@ -1030,8 +1024,8 @@ function OptionsFn<TTag extends ElementType = typeof DEFAULT_OPTIONS_TAG>(
actions.onChange(dataRef.current.value)
}
if (data.mode === ValueMode.Single) {
actions.closeListbox()
disposables().nextFrame(() => data.buttonRef.current?.focus({ preventScroll: true }))
flushSync(() => actions.closeListbox())
data.buttonRef.current?.focus({ preventScroll: true })
}
break

Expand Down Expand Up @@ -1060,8 +1054,9 @@ function OptionsFn<TTag extends ElementType = typeof DEFAULT_OPTIONS_TAG>(
case Keys.Escape:
event.preventDefault()
event.stopPropagation()
actions.closeListbox()
return d.nextFrame(() => data.buttonRef.current?.focus({ preventScroll: true }))
flushSync(() => actions.closeListbox())
data.buttonRef.current?.focus({ preventScroll: true })
return

case Keys.Tab:
event.preventDefault()
Expand Down Expand Up @@ -1205,11 +1200,9 @@ function OptionFn<
if (data.listboxState !== ListboxStates.Open) return
if (!active) return
if (data.activationTrigger === ActivationTrigger.Pointer) return
let d = disposables()
d.requestAnimationFrame(() => {
return disposables().requestAnimationFrame(() => {
internalOptionRef.current?.scrollIntoView?.({ block: 'nearest' })
})
return d.dispose
}, [
internalOptionRef,
active,
Expand All @@ -1228,8 +1221,8 @@ function OptionFn<
if (disabled) return event.preventDefault()
actions.onChange(value)
if (data.mode === ValueMode.Single) {
actions.closeListbox()
disposables().nextFrame(() => data.buttonRef.current?.focus({ preventScroll: true }))
flushSync(() => actions.closeListbox())
data.buttonRef.current?.focus({ preventScroll: true })
}
})

Expand Down
35 changes: 15 additions & 20 deletions packages/@headlessui-react/src/components/menu/menu.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import React, {
type MouseEvent as ReactMouseEvent,
type Ref,
} from 'react'
import { flushSync } from 'react-dom'
import { useActivePress } from '../../hooks/use-active-press'
import { useDidElementMove } from '../../hooks/use-did-element-move'
import { useDisposables } from '../../hooks/use-disposables'
Expand Down Expand Up @@ -469,8 +470,6 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
let getFloatingReferenceProps = useFloatingReferenceProps()
let buttonRef = useSyncRefs(state.buttonRef, ref, useFloatingReference())

let d = useDisposables()

let handleKeyDown = useEvent((event: ReactKeyboardEvent<HTMLButtonElement>) => {
switch (event.key) {
// Ref: https://www.w3.org/WAI/ARIA/apg/patterns/menubutton/#keyboard-interaction-13
Expand All @@ -480,15 +479,15 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
case Keys.ArrowDown:
event.preventDefault()
event.stopPropagation()
dispatch({ type: ActionTypes.OpenMenu })
d.nextFrame(() => dispatch({ type: ActionTypes.GoToItem, focus: Focus.First }))
flushSync(() => dispatch({ type: ActionTypes.OpenMenu }))
dispatch({ type: ActionTypes.GoToItem, focus: Focus.First })
break

case Keys.ArrowUp:
event.preventDefault()
event.stopPropagation()
dispatch({ type: ActionTypes.OpenMenu })
d.nextFrame(() => dispatch({ type: ActionTypes.GoToItem, focus: Focus.Last }))
flushSync(() => dispatch({ type: ActionTypes.OpenMenu }))
dispatch({ type: ActionTypes.GoToItem, focus: Focus.Last })
break
}
})
Expand All @@ -508,8 +507,8 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
if (isDisabledReactIssue7711(event.currentTarget)) return event.preventDefault()
if (disabled) return
if (state.menuState === MenuStates.Open) {
dispatch({ type: ActionTypes.CloseMenu })
d.nextFrame(() => state.buttonRef.current?.focus({ preventScroll: true }))
flushSync(() => dispatch({ type: ActionTypes.CloseMenu }))
state.buttonRef.current?.focus({ preventScroll: true })
} else {
event.preventDefault()
dispatch({ type: ActionTypes.OpenMenu })
Expand Down Expand Up @@ -722,20 +721,18 @@ function ItemsFn<TTag extends ElementType = typeof DEFAULT_ITEMS_TAG>(
case Keys.Escape:
event.preventDefault()
event.stopPropagation()
dispatch({ type: ActionTypes.CloseMenu })
disposables().nextFrame(() => state.buttonRef.current?.focus({ preventScroll: true }))
flushSync(() => dispatch({ type: ActionTypes.CloseMenu }))
state.buttonRef.current?.focus({ preventScroll: true })
break

case Keys.Tab:
event.preventDefault()
event.stopPropagation()
dispatch({ type: ActionTypes.CloseMenu })
disposables().microTask(() => {
focusFrom(
state.buttonRef.current!,
event.shiftKey ? FocusManagementFocus.Previous : FocusManagementFocus.Next
)
})
flushSync(() => dispatch({ type: ActionTypes.CloseMenu }))
focusFrom(
state.buttonRef.current!,
event.shiftKey ? FocusManagementFocus.Previous : FocusManagementFocus.Next
)
break

default:
Expand Down Expand Up @@ -837,11 +834,9 @@ function ItemFn<TTag extends ElementType = typeof DEFAULT_ITEM_TAG>(
if (state.menuState !== MenuStates.Open) return
if (!active) return
if (state.activationTrigger === ActivationTrigger.Pointer) return
let d = disposables()
d.requestAnimationFrame(() => {
return disposables().requestAnimationFrame(() => {
internalItemRef.current?.scrollIntoView?.({ block: 'nearest' })
})
return d.dispose
}, [
state.__demoMode,
internalItemRef,
Expand Down

0 comments on commit 2d3ec80

Please sign in to comment.