From 6fd9bb2034609ae518e216898c402584665d6a9c Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 12:39:04 -0700 Subject: [PATCH 01/53] Fix Virtualizer in react strict mode --- .../@react-aria/virtualizer/src/Virtualizer.tsx | 15 +++++++++++---- .../@react-stately/virtualizer/src/Virtualizer.ts | 11 ++++++----- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index dfbc69b7944..9288b71ea9e 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -76,24 +76,31 @@ function Virtualizer(props: VirtualizerProps, ref: Re let {virtualizerProps} = useVirtualizer(props, state, ref); // Handle scrolling, and call onLoadMore when nearing the bottom. + let isLoadingRef = useRef(isLoading); + useLayoutEffect(() => { + isLoadingRef.current = isLoading; + }, [isLoading]); + let onVisibleRectChange = useCallback((rect: Rect) => { state.setVisibleRect(rect); - if (!isLoading && onLoadMore) { + if (!isLoadingRef.current && onLoadMore) { let scrollOffset = state.virtualizer.contentSize.height - rect.height * 2; if (rect.y > scrollOffset) { + isLoadingRef.current = true; onLoadMore(); } } - }, [isLoading, onLoadMore, state]); + }, [onLoadMore, state]); useLayoutEffect(() => { - if (!isLoading && onLoadMore && !state.isAnimating) { + if (!isLoadingRef.current && onLoadMore && !state.isAnimating) { if (state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) { + isLoadingRef.current = true; onLoadMore(); } } - }, [state.contentSize, state.isAnimating, state.virtualizer, onLoadMore, isLoading]); + }, [state.contentSize, state.isAnimating, state.virtualizer, onLoadMore]); return ( { } this._invalidationContext = context; - this._relayoutRaf = requestAnimationFrame(() => { - this._relayoutRaf = null; - this.relayoutNow(); - }); } /** @@ -814,6 +810,12 @@ export class Virtualizer { } afterRender() { + if (this._transactionQueue.length > 0) { + this._processTransactionQueue(); + } else if (this._invalidationContext) { + this.relayoutNow(); + } + if (this.shouldOverscan) { this._overscanManager.collectMetrics(); } @@ -1112,7 +1114,6 @@ export class Virtualizer { this._transactionQueue.push(this._nextTransaction); this._nextTransaction = null; - this._processTransactionQueue(); return true; } From 927f85fba8c4499e24ad38b235cc2749af1d7c8f Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 12:39:13 -0700 Subject: [PATCH 02/53] ListBox 157 --- packages/@react-spectrum/listbox/src/ListBoxBase.tsx | 11 ++++++++--- packages/@react-spectrum/listbox/test/ListBox.test.js | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx index ff257f7fc5c..c6399eda76c 100644 --- a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx +++ b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx @@ -21,7 +21,7 @@ import {ListBoxOption} from './ListBoxOption'; import {ListBoxSection} from './ListBoxSection'; import {ListLayout} from '@react-stately/layout'; import {ListState} from '@react-stately/list'; -import {mergeProps} from '@react-aria/utils'; +import {mergeProps, useLayoutEffect} from '@react-aria/utils'; import {ProgressCircle} from '@react-spectrum/progress'; import React, {HTMLAttributes, ReactElement, ReactNode, RefObject, useMemo} from 'react'; import {ReusableView} from '@react-stately/virtualizer'; @@ -78,8 +78,13 @@ function ListBoxBase(props: ListBoxBaseProps, ref: RefObject { + // Sync loading state into the layout. + if (layout.isLoading !== props.isLoading) { + layout.isLoading = props.isLoading; + layout.virtualizer.relayoutNow(); + } + }, [layout, props.isLoading]); // This overrides collection view's renderWrapper to support heirarchy of items in sections. // The header is extracted from the children so it can receive ARIA labeling properties. diff --git a/packages/@react-spectrum/listbox/test/ListBox.test.js b/packages/@react-spectrum/listbox/test/ListBox.test.js index aa841fb7442..78e1f4fbd77 100644 --- a/packages/@react-spectrum/listbox/test/ListBox.test.js +++ b/packages/@react-spectrum/listbox/test/ListBox.test.js @@ -882,7 +882,7 @@ describe('ListBox', function () { expect(options.length).toBe(5); // onLoadMore called twice from onVisibleRectChange due to ListBox sizeToFit // onLoadMore called twice from useLayoutEffect in React < 18 - expect(onLoadMore).toHaveBeenCalledTimes(parseInt(React.version, 10) >= 18 ? 3 : 4); + expect(onLoadMore).toHaveBeenCalledTimes(parseInt(React.version, 10) >= 18 ? 2 : 4); }); }); From 584207528d047ac01f09499d54255280fc203ed6 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 12:43:50 -0700 Subject: [PATCH 03/53] TagGroup 155 --- packages/@react-spectrum/tag/src/TagGroup.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-spectrum/tag/src/TagGroup.tsx b/packages/@react-spectrum/tag/src/TagGroup.tsx index 374d6082b0e..45388a6ce89 100644 --- a/packages/@react-spectrum/tag/src/TagGroup.tsx +++ b/packages/@react-spectrum/tag/src/TagGroup.tsx @@ -94,7 +94,7 @@ function TagGroup(props: SpectrumTagGroupProps, ref: DOMRef let currContainerRef: HTMLDivElement | null = containerRef.current; let currTagsRef: HTMLDivElement | null = tagsRef.current; let currActionsRef: HTMLDivElement | null = actionsRef.current; - if (!currContainerRef || !currTagsRef || state.collection.size === 0) { + if (!currContainerRef || !currTagsRef || !currActionsRef || state.collection.size === 0) { return { visibleTagCount: 0, showCollapseButton: false From 908f913113cfc10545c6726dc191973a2e846997 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 13:02:38 -0700 Subject: [PATCH 04/53] Fix Virtualizer auto scroll 142 --- packages/@react-aria/virtualizer/src/Virtualizer.tsx | 11 +++++++---- packages/@react-spectrum/listbox/src/ListBoxBase.tsx | 1 + 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 9288b71ea9e..83923ed18ef 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -35,7 +35,8 @@ interface VirtualizerProps extends HTMLAttributes void, shouldUseVirtualFocus?: boolean, - scrollToItem?: (key: Key) => void + scrollToItem?: (key: Key) => void, + autoFocus?: boolean } function Virtualizer(props: VirtualizerProps, ref: RefObject) { @@ -122,7 +123,8 @@ interface VirtualizerOptions { tabIndex?: number, focusedKey?: Key, scrollToItem?: (key: Key) => void, - shouldUseVirtualFocus?: boolean + shouldUseVirtualFocus?: boolean, + autoFocus?: boolean } export function useVirtualizer(props: VirtualizerOptions, state: VirtualizerState, ref: RefObject) { @@ -133,15 +135,16 @@ export function useVirtualizer(props: VirtualizerOptions // to all of the item DOM nodes. let lastFocusedKey = useRef(null); let isFocusWithin = useRef(false); + let autoFocus = useRef(props.autoFocus); useEffect(() => { if (virtualizer.visibleRect.height === 0) { return; } // Only scroll the focusedKey into view if the modality is not pointer to avoid jumps in position when clicking/pressing tall items. - // Exception made if focus isn't within the virtualizer (e.g. opening a picker via click should scroll the selected item into view) let modality = getInteractionModality(); - if (focusedKey !== lastFocusedKey.current && (modality !== 'pointer' || !isFocusWithin.current)) { + if (focusedKey !== lastFocusedKey.current && (modality !== 'pointer' || autoFocus.current)) { + autoFocus.current = false; if (scrollToItem) { // If user provides scrolltoitem, then it is their responsibility to call scrollIntoViewport if desired // since we don't know if their scrollToItem may take some time to actually bring the active element into the virtualizer's visible rect. diff --git a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx index c6399eda76c..88f190c18d4 100644 --- a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx +++ b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx @@ -117,6 +117,7 @@ function ListBoxBase(props: ListBoxBaseProps, ref: RefObject Date: Sat, 20 May 2023 13:33:37 -0700 Subject: [PATCH 05/53] Fix useUpdateEffect --- packages/@react-aria/utils/src/useUpdateEffect.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/@react-aria/utils/src/useUpdateEffect.ts b/packages/@react-aria/utils/src/useUpdateEffect.ts index f480311c22d..b70d66d42ad 100644 --- a/packages/@react-aria/utils/src/useUpdateEffect.ts +++ b/packages/@react-aria/utils/src/useUpdateEffect.ts @@ -15,13 +15,15 @@ import {EffectCallback, useEffect, useRef} from 'react'; // Like useEffect, but only called for updates after the initial render. export function useUpdateEffect(effect: EffectCallback, dependencies: any[]) { const isInitialMount = useRef(true); + const lastDeps = useRef(null); useEffect(() => { if (isInitialMount.current) { isInitialMount.current = false; - } else { + } else if (!lastDeps.current || dependencies.some((dep, i) => dep !== lastDeps[i])) { effect(); } + lastDeps.current = dependencies; // eslint-disable-next-line react-hooks/exhaustive-deps }, dependencies); } From f07556b05a0ca1a389c87b7c123f411ca5151da0 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 13:47:34 -0700 Subject: [PATCH 06/53] Fix VirtualizerItem ReusableView can be mutated, so we need to pass the layoutInfo/content at the time we render to the JSX element rather than reading directly 122 --- .../virtualizer/src/Virtualizer.tsx | 7 ++++-- .../virtualizer/src/VirtualizerItem.tsx | 23 ++++++++++--------- packages/@react-aria/virtualizer/src/index.ts | 2 +- .../virtualizer/src/useVirtualizerItem.ts | 17 +++++++++----- .../@react-spectrum/card/src/CardView.tsx | 7 ++++-- .../listbox/src/ListBoxBase.tsx | 13 +++++++---- .../listbox/src/ListBoxSection.tsx | 20 ++++++++-------- .../@react-spectrum/sidenav/src/SideNav.tsx | 7 ++++-- .../@react-spectrum/table/src/TableView.tsx | 9 +++++--- 9 files changed, 64 insertions(+), 41 deletions(-) diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 83923ed18ef..3daa607689d 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -233,7 +233,10 @@ function defaultRenderWrapper( return ( + layoutInfo={reusableView.layoutInfo} + virtualizer={reusableView.virtualizer} + parent={parent?.layoutInfo}> + {reusableView.rendered} + ); } diff --git a/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx b/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx index 0137b88c993..8dbc228a4e6 100644 --- a/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx +++ b/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx @@ -12,28 +12,29 @@ import {Direction} from '@react-types/shared'; import {LayoutInfo, ReusableView} from '@react-stately/virtualizer'; -import React, {CSSProperties, useRef} from 'react'; +import React, {CSSProperties, ReactNode, useRef} from 'react'; import {useLocale} from '@react-aria/i18n'; -import {useVirtualizerItem} from './useVirtualizerItem'; +import {useVirtualizerItem, VirtualizerItemOptions} from './useVirtualizerItem'; -interface VirtualizerItemProps { - reusableView: ReusableView, - parent?: ReusableView, - className?: string +interface VirtualizerItemProps extends Omit { + parent?: LayoutInfo, + className?: string, + children: ReactNode } -export function VirtualizerItem(props: VirtualizerItemProps) { - let {className, reusableView, parent} = props; +export function VirtualizerItem(props: VirtualizerItemProps) { + let {className, layoutInfo, virtualizer, parent, children} = props; let {direction} = useLocale(); let ref = useRef(); useVirtualizerItem({ - reusableView, + layoutInfo, + virtualizer, ref }); return ( -
- {reusableView.rendered} +
+ {children}
); } diff --git a/packages/@react-aria/virtualizer/src/index.ts b/packages/@react-aria/virtualizer/src/index.ts index f02be4c1492..1910c2785e1 100644 --- a/packages/@react-aria/virtualizer/src/index.ts +++ b/packages/@react-aria/virtualizer/src/index.ts @@ -12,7 +12,7 @@ export type {RTLOffsetType} from './utils'; export {useVirtualizer, Virtualizer} from './Virtualizer'; -export {useVirtualizerItem} from './useVirtualizerItem'; +export {useVirtualizerItem, VirtualizerItemOptions} from './useVirtualizerItem'; export {VirtualizerItem, layoutInfoToStyle} from './VirtualizerItem'; export {ScrollView} from './ScrollView'; export {getRTLOffsetType, getScrollLeft, setScrollLeft} from './utils'; diff --git a/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts b/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts index a3fbd69e253..c4132ef3bdc 100644 --- a/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts +++ b/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts @@ -10,17 +10,22 @@ * governing permissions and limitations under the License. */ -import {RefObject, useCallback} from 'react'; -import {ReusableView, Size} from '@react-stately/virtualizer'; +import {Key, RefObject, useCallback} from 'react'; +import {LayoutInfo, ReusableView, Size} from '@react-stately/virtualizer'; import {useLayoutEffect} from '@react-aria/utils'; -interface VirtualizerItemOptions { - reusableView: ReusableView, +interface IVirtualizer { + updateItemSize(key: Key, size: Size): void +} + +export interface VirtualizerItemOptions { + layoutInfo: LayoutInfo, + virtualizer: IVirtualizer ref: RefObject } -export function useVirtualizerItem(options: VirtualizerItemOptions) { - let {reusableView: {layoutInfo, virtualizer}, ref} = options; +export function useVirtualizerItem(options: VirtualizerItemOptions) { + let {layoutInfo, virtualizer, ref} = options; let updateSize = useCallback(() => { let size = getSize(ref.current); diff --git a/packages/@react-spectrum/card/src/CardView.tsx b/packages/@react-spectrum/card/src/CardView.tsx index a6f38198224..f0edf871fc2 100644 --- a/packages/@react-spectrum/card/src/CardView.tsx +++ b/packages/@react-spectrum/card/src/CardView.tsx @@ -92,8 +92,11 @@ function CardView(props: SpectrumCardViewProps, ref: DOMRef let renderWrapper = (parent: View, reusableView: View) => ( + layoutInfo={reusableView.layoutInfo} + virtualizer={reusableView.virtualizer} + parent={parent?.layoutInfo}> + {reusableView.rendered} + ); let focusedKey = state.selectionManager.focusedKey; diff --git a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx index 88f190c18d4..5069148c6ef 100644 --- a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx +++ b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx @@ -94,8 +94,10 @@ function ListBoxBase(props: ListBoxBaseProps, ref: RefObject c.viewType === 'header')}> + item={reusableView.content} + layoutInfo={reusableView.layoutInfo} + virtualizer={reusableView.virtualizer} + headerLayoutInfo={children.find(c => c.viewType === 'header').layoutInfo}> {renderChildren(children.filter(c => c.viewType === 'item'))} ); @@ -104,8 +106,11 @@ function ListBoxBase(props: ListBoxBaseProps, ref: RefObject + layoutInfo={reusableView.layoutInfo} + virtualizer={reusableView.virtualizer} + parent={parent?.layoutInfo}> + {reusableView.rendered} + ); }; diff --git a/packages/@react-spectrum/listbox/src/ListBoxSection.tsx b/packages/@react-spectrum/listbox/src/ListBoxSection.tsx index fbb75c5d937..8b49e16faad 100644 --- a/packages/@react-spectrum/listbox/src/ListBoxSection.tsx +++ b/packages/@react-spectrum/listbox/src/ListBoxSection.tsx @@ -11,26 +11,25 @@ */ import {classNames} from '@react-spectrum/utils'; -import {layoutInfoToStyle, useVirtualizerItem} from '@react-aria/virtualizer'; +import {LayoutInfo} from '@react-stately/virtualizer'; +import {layoutInfoToStyle, useVirtualizerItem, VirtualizerItemOptions} from '@react-aria/virtualizer'; import {ListBoxContext} from './ListBoxContext'; import {Node} from '@react-types/shared'; import React, {Fragment, ReactNode, useContext, useRef} from 'react'; -import {ReusableView} from '@react-stately/virtualizer'; import styles from '@adobe/spectrum-css-temp/components/menu/vars.css'; import {useListBoxSection} from '@react-aria/listbox'; import {useLocale} from '@react-aria/i18n'; import {useSeparator} from '@react-aria/separator'; -interface ListBoxSectionProps { - reusableView: ReusableView, unknown>, - header: ReusableView, unknown>, +interface ListBoxSectionProps extends Omit { + headerLayoutInfo: LayoutInfo, + item: Node, children?: ReactNode } /** @private */ export function ListBoxSection(props: ListBoxSectionProps) { - let {children, reusableView, header} = props; - let item = reusableView.content; + let {children, layoutInfo, headerLayoutInfo, virtualizer, item} = props; let {headingProps, groupProps} = useListBoxSection({ heading: item.rendered, 'aria-label': item['aria-label'] @@ -42,7 +41,8 @@ export function ListBoxSection(props: ListBoxSectionProps) { let headerRef = useRef(); useVirtualizerItem({ - reusableView: header, + layoutInfo: headerLayoutInfo, + virtualizer, ref: headerRef }); @@ -51,7 +51,7 @@ export function ListBoxSection(props: ListBoxSectionProps) { return ( -
+
{item.key !== state.collection.getFirstKey() &&
(props: ListBoxSectionProps) {
(props: SpectrumSideNavProps) { return ( + layoutInfo={reusableView.layoutInfo} + virtualizer={reusableView.virtualizer} + parent={parent?.layoutInfo}> + {reusableView.rendered} + ); }; diff --git a/packages/@react-spectrum/table/src/TableView.tsx b/packages/@react-spectrum/table/src/TableView.tsx index 1e5e49dbd57..26b59e03773 100644 --- a/packages/@react-spectrum/table/src/TableView.tsx +++ b/packages/@react-spectrum/table/src/TableView.tsx @@ -387,8 +387,9 @@ function TableView(props: SpectrumTableProps, ref: DOMRef(props: SpectrumTableProps, ref: DOMRef + }> + {reusableView.rendered} + ); }; From f02b0873e88bb8a44087862fefba733837109daa Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 13:47:43 -0700 Subject: [PATCH 07/53] Fix TableView loading state --- .../@react-spectrum/table/src/TableView.tsx | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/@react-spectrum/table/src/TableView.tsx b/packages/@react-spectrum/table/src/TableView.tsx index 26b59e03773..96786311a1e 100644 --- a/packages/@react-spectrum/table/src/TableView.tsx +++ b/packages/@react-spectrum/table/src/TableView.tsx @@ -657,25 +657,32 @@ function TableVirtualizer({layout, collection, focusedKey, renderView, renderWra headerRef.current.scrollLeft = bodyRef.current.scrollLeft; }, [bodyRef, headerRef]); + let isLoadingRef = useRef(isLoading); + useLayoutEffect(() => { + isLoadingRef.current = isLoading; + }, [isLoading]); + let onVisibleRectChange = useCallback((rect: Rect) => { state.setVisibleRect(rect); - if (!isLoading && onLoadMore) { + if (!isLoadingRef.current && onLoadMore) { let scrollOffset = state.virtualizer.contentSize.height - rect.height * 2; if (rect.y > scrollOffset) { + isLoadingRef.current = true; onLoadMore(); } } // eslint-disable-next-line react-hooks/exhaustive-deps - }, [onLoadMore, isLoading, state.setVisibleRect, state.virtualizer]); + }, [onLoadMore, state.setVisibleRect, state.virtualizer]); useLayoutEffect(() => { - if (!isLoading && onLoadMore && !state.isAnimating) { + if (!isLoadingRef.current && onLoadMore && !state.isAnimating) { if (state.contentSize.height <= state.virtualizer.visibleRect.height) { + isLoadingRef.current = true; onLoadMore(); } } - }, [state.contentSize, state.virtualizer, state.isAnimating, onLoadMore, isLoading]); + }, [state.contentSize, state.virtualizer, state.isAnimating, onLoadMore]); let resizerPosition = layout.getResizerPosition() - 2; @@ -1122,7 +1129,7 @@ function TableDragHeaderCell({column}) {
Date: Sat, 20 May 2023 16:53:38 -0700 Subject: [PATCH 08/53] Remove old Virtualizer focus behavior We used to focus the collection itself when the focused item went out of view, but we don't need this anymore because we use persistedKeys to ensure the focused item is always in the DOM. In strict mode this was causing some weird behavior due to ordering of the useEffects. We also need to ensure that when a DOM node is reused for a different item that we do actually move focus. --- packages/@react-aria/grid/src/useGridCell.ts | 12 ++++++++++-- .../@react-aria/gridlist/src/useGridListItem.ts | 12 ++++++++++-- .../@react-aria/virtualizer/src/Virtualizer.tsx | 15 +++------------ 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index 3cd0a42997b..5efe934992e 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -17,7 +17,7 @@ import {GridCollection, GridNode} from '@react-types/grid'; import {gridMap} from './utils'; import {GridState} from '@react-stately/grid'; import {isFocusVisible} from '@react-aria/interactions'; -import {KeyboardEvent as ReactKeyboardEvent, RefObject} from 'react'; +import {KeyboardEvent as ReactKeyboardEvent, RefObject, useRef} from 'react'; import {useLocale} from '@react-aria/i18n'; import {useSelectableItem} from '@react-aria/selection'; @@ -62,6 +62,10 @@ export function useGridCell>(props: GridCellProps let {direction} = useLocale(); let {keyboardDelegate, actions: {onCellAction}} = gridMap.get(state); + // We need to track the key of the item at the time it was last focused so that we force + // focus to go to the item when the DOM node is reused for a different item in a virtualizer. + let keyWhenFocused = useRef(null); + // Handles focusing the cell. If there is a focusable child, // it is focused, otherwise the cell itself is focused. let focus = () => { @@ -81,7 +85,10 @@ export function useGridCell>(props: GridCellProps } } - if (!ref.current.contains(document.activeElement)) { + if ( + (keyWhenFocused.current != null && node.key !== keyWhenFocused.current) || + !ref.current.contains(document.activeElement) + ) { focusSafely(ref.current); } }; @@ -208,6 +215,7 @@ export function useGridCell>(props: GridCellProps // Grid cells can have focusable elements inside them. In this case, focus should // be marshalled to that element rather than focusing the cell itself. let onFocus = (e) => { + keyWhenFocused.current = node.key; if (e.target !== ref.current) { // useSelectableItem only handles setting the focused key when // the focused element is the gridcell itself. We also want to diff --git a/packages/@react-aria/gridlist/src/useGridListItem.ts b/packages/@react-aria/gridlist/src/useGridListItem.ts index a344583f58a..d133fe98c36 100644 --- a/packages/@react-aria/gridlist/src/useGridListItem.ts +++ b/packages/@react-aria/gridlist/src/useGridListItem.ts @@ -16,7 +16,7 @@ import {getRowId, listMap} from './utils'; import {getScrollParent, mergeProps, scrollIntoViewport, useSlotId} from '@react-aria/utils'; import {isFocusVisible} from '@react-aria/interactions'; import type {ListState} from '@react-stately/list'; -import {KeyboardEvent as ReactKeyboardEvent, RefObject} from 'react'; +import {KeyboardEvent as ReactKeyboardEvent, RefObject, useRef} from 'react'; import {SelectableItemStates, useSelectableItem} from '@react-aria/selection'; import {useLocale} from '@react-aria/i18n'; @@ -55,10 +55,17 @@ export function useGridListItem(props: AriaGridListItemOptions, state: ListSt let {direction} = useLocale(); let {onAction} = listMap.get(state); let descriptionId = useSlotId(); + + // We need to track the key of the item at the time it was last focused so that we force + // focus to go to the item when the DOM node is reused for a different item in a virtualizer. + let keyWhenFocused = useRef(null); let focus = () => { // Don't shift focus to the row if the active element is a element within the row already // (e.g. clicking on a row button) - if (!ref.current.contains(document.activeElement)) { + if ( + (keyWhenFocused.current != null && node.key !== keyWhenFocused.current) || + !ref.current.contains(document.activeElement) + ) { focusSafely(ref.current); } }; @@ -155,6 +162,7 @@ export function useGridListItem(props: AriaGridListItemOptions, state: ListSt }; let onFocus = (e) => { + keyWhenFocused.current = node.key; if (e.target !== ref.current) { // useSelectableItem only handles setting the focused key when // the focused element is the row itself. We also want to diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 3daa607689d..d14826e4b15 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -185,25 +185,16 @@ export function useVirtualizer(props: VirtualizerOptions isFocusWithin.current = ref.current.contains(e.relatedTarget as Element); }, [ref]); - // When the focused item is scrolled out of view and is removed from the DOM, - // move focus to the collection view as a whole if focus was within before. - let focusedView = virtualizer.getView(focusedKey); - useEffect(() => { - if (focusedKey && !focusedView && isFocusWithin.current && document.activeElement !== ref.current) { - focusWithoutScrolling(ref.current); - } - }); - - // Set tabIndex to -1 if the focused view is in the DOM, otherwise 0 so that the collection + // Set tabIndex to -1 if there is a focused key, otherwise 0 so that the collection // itself is tabbable. When the collection receives focus, we scroll the focused item back into // view, which will allow it to be properly focused. If using virtual focus, don't set a // tabIndex at all so that VoiceOver on iOS 14 doesn't try to move real DOM focus to the element anyway. let tabIndex: number; if (!shouldUseVirtualFocus) { - // When there is no focusedView the default tabIndex is 0. We include logic for empty collections too. + // When there is no focusedKey the default tabIndex is 0. We include logic for empty collections too. // For collections that are empty, but have a link in the empty children we want to skip focusing this // and let focus move to the link similar to link moving to children. - tabIndex = focusedView ? -1 : 0; + tabIndex = focusedKey != null ? -1 : 0; // If the collection is empty, we want the tabIndex provided from props (if any) // so that we handle when tabbable items are added to the empty state. From cf617f3de90fabc90a4e36a74217ccc1d5db100b Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 17:22:20 -0700 Subject: [PATCH 09/53] Fix CardView tests 118 --- packages/@react-spectrum/card/test/CardView.test.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/@react-spectrum/card/test/CardView.test.js b/packages/@react-spectrum/card/test/CardView.test.js index 5fa5c3c107c..0e087707eab 100644 --- a/packages/@react-spectrum/card/test/CardView.test.js +++ b/packages/@react-spectrum/card/test/CardView.test.js @@ -720,7 +720,7 @@ describe('CardView', function () { }); } - expect(document.activeElement).toEqual(pageDownElement); + expect(document.activeElement).toHaveTextContent(pageDownElement.textContent); }); }); }); @@ -1197,12 +1197,16 @@ describe('CardView', function () { fireEvent.keyDown(document.activeElement, {key: 'End', code: 35, charCode: 35}); fireEvent.keyUp(document.activeElement, {key: 'End', code: 35, charCode: 35}); + act(() => { + grid.scrollTop += 100; + fireEvent.scroll(grid); + }); + act(() => { jest.runAllTimers(); }); expect(within(grid).getByText('Title 12')).toBeTruthy(); - let spinner = within(grid).getByRole('progressbar'); expect(spinner).toHaveAttribute('aria-label', 'Loading moreโ€ฆ'); expect(getCardStyles(spinner.parentNode).height).toBe('60px'); From 57344bccc119a4e45f9929940c15066d23065f39 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 19:13:29 -0700 Subject: [PATCH 10/53] Fix DragManager 84 --- packages/@react-aria/dnd/src/DragManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-aria/dnd/src/DragManager.ts b/packages/@react-aria/dnd/src/DragManager.ts index e65f5a60161..f73afe533ff 100644 --- a/packages/@react-aria/dnd/src/DragManager.ts +++ b/packages/@react-aria/dnd/src/DragManager.ts @@ -498,6 +498,7 @@ class DragSession { end() { this.teardown(); + endDragging(); if (typeof this.dragTarget.onDragEnd === 'function') { let target = this.currentDropTarget && this.dropOperation !== 'cancel' ? this.currentDropTarget : this.dragTarget; @@ -526,7 +527,6 @@ class DragSession { } this.setCurrentDropTarget(null); - endDragging(); } cancel() { From 3768a9373274586b13459e2112b1227d56e8fadf Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 19:18:48 -0700 Subject: [PATCH 11/53] Fix ListView findDOMNode deprecation 80 --- packages/@react-spectrum/list/src/ListViewItem.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/@react-spectrum/list/src/ListViewItem.tsx b/packages/@react-spectrum/list/src/ListViewItem.tsx index 2e99cb29c99..42ffd209058 100644 --- a/packages/@react-spectrum/list/src/ListViewItem.tsx +++ b/packages/@react-spectrum/list/src/ListViewItem.tsx @@ -54,6 +54,7 @@ export function ListViewItem(props: ListViewItemProps) { } = useContext(ListViewContext); let {direction} = useLocale(); let rowRef = useRef(); + let checkboxWrapperRef = useRef(); let { isFocusVisible: isFocusVisibleWithin, focusProps: focusWithinProps @@ -249,8 +250,9 @@ export function ListViewItem(props: ListViewItemProps) { exit: listStyles['react-spectrum-ListViewItem-checkbox--exit'], exitActive: listStyles['react-spectrum-ListViewItem-checkbox--exitActive'] }} - timeout={160} > -
+ timeout={160} + nodeRef={checkboxWrapperRef} > +
Date: Sat, 20 May 2023 20:11:16 -0700 Subject: [PATCH 12/53] Make DragManager cancelation behavior consistent Escape should take you back to the original drag target, and should not affect the focusedKey of the target list. --- packages/@react-aria/dnd/src/DragManager.ts | 1 + packages/@react-spectrum/list/test/ListViewDnd.test.js | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/@react-aria/dnd/src/DragManager.ts b/packages/@react-aria/dnd/src/DragManager.ts index f73afe533ff..f841f9e8965 100644 --- a/packages/@react-aria/dnd/src/DragManager.ts +++ b/packages/@react-aria/dnd/src/DragManager.ts @@ -530,6 +530,7 @@ class DragSession { } cancel() { + this.setCurrentDropTarget(null); this.end(); if (!this.dragTarget.element.closest('[aria-hidden="true"]')) { this.dragTarget.element.focus(); diff --git a/packages/@react-spectrum/list/test/ListViewDnd.test.js b/packages/@react-spectrum/list/test/ListViewDnd.test.js index 8fd9e5d2a01..030e04533f4 100644 --- a/packages/@react-spectrum/list/test/ListViewDnd.test.js +++ b/packages/@react-spectrum/list/test/ListViewDnd.test.js @@ -2377,10 +2377,10 @@ describe('ListView', function () { beginDrag(tree); userEvent.tab(); // Should automatically jump to the folder target since we didn't provide onRootDrop and onInsert - expect(document.activeElement).toHaveAttribute('aria-label', 'Drop on Apps'); + expect(document.activeElement).toHaveAttribute('aria-label', 'Drop on Pictures'); fireEvent.keyDown(document.activeElement, {key: 'ArrowDown'}); fireEvent.keyUp(document.activeElement, {key: 'ArrowDown'}); - expect(document.activeElement).toHaveAttribute('aria-label', 'Drop on Pictures'); + expect(document.activeElement).toHaveAttribute('aria-label', 'Drop on Apps'); }); it('should allow the user to override the util handlers via onDrop and getDropOperations', function () { From b80d11bf59058fb4a049b4fe1fd521be732db79e Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 21:13:24 -0700 Subject: [PATCH 13/53] Improve virtualizer onLoadMore some more 75 --- .../@react-aria/virtualizer/src/Virtualizer.tsx | 14 +++++++++++--- .../@react-spectrum/combobox/test/ComboBox.test.js | 6 +----- .../@react-spectrum/listbox/test/ListBox.test.js | 6 ++---- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index d14826e4b15..cf480fe9980 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -78,9 +78,15 @@ function Virtualizer(props: VirtualizerProps, ref: Re // Handle scrolling, and call onLoadMore when nearing the bottom. let isLoadingRef = useRef(isLoading); + let prevProps = useRef(props); useLayoutEffect(() => { - isLoadingRef.current = isLoading; - }, [isLoading]); + // Only update isLoadingRef if props object actually changed, + // not if a local state change occurred. + if (props !== prevProps.current) { + isLoadingRef.current = isLoading; + prevProps.current = props; + } + }, [isLoading, props]); let onVisibleRectChange = useCallback((rect: Rect) => { state.setVisibleRect(rect); @@ -94,13 +100,15 @@ function Virtualizer(props: VirtualizerProps, ref: Re } }, [onLoadMore, state]); + let lastContentSize = useRef(0); useLayoutEffect(() => { if (!isLoadingRef.current && onLoadMore && !state.isAnimating) { - if (state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) { + if (state.contentSize.height !== lastContentSize.current && state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) { isLoadingRef.current = true; onLoadMore(); } } + lastContentSize.current = state.contentSize.height; }, [state.contentSize, state.isAnimating, state.virtualizer, onLoadMore]); return ( diff --git a/packages/@react-spectrum/combobox/test/ComboBox.test.js b/packages/@react-spectrum/combobox/test/ComboBox.test.js index 09d6d0b01a3..6fc46c3b4ba 100644 --- a/packages/@react-spectrum/combobox/test/ComboBox.test.js +++ b/packages/@react-spectrum/combobox/test/ComboBox.test.js @@ -2153,11 +2153,7 @@ describe('ComboBox', function () { expect(onOpenChange).toHaveBeenCalledTimes(3); expect(onOpenChange).toHaveBeenLastCalledWith(true, 'manual'); - // Please test this in storybook. - // In virtualizer.tsx the onVisibleRectChange() causes this to be called twice - // because the browser limits the popover height via calculatePosition(), - // while the test doesn't, causing virtualizer to try to load more - expect(onLoadMore).toHaveBeenCalledTimes(2); + expect(onLoadMore).toHaveBeenCalledTimes(1); expect(load).toHaveBeenCalledTimes(1); // close menu diff --git a/packages/@react-spectrum/listbox/test/ListBox.test.js b/packages/@react-spectrum/listbox/test/ListBox.test.js index 78e1f4fbd77..366dc4d5bfe 100644 --- a/packages/@react-spectrum/listbox/test/ListBox.test.js +++ b/packages/@react-spectrum/listbox/test/ListBox.test.js @@ -853,7 +853,7 @@ describe('ListBox', function () { fireEvent.scroll(listbox); act(() => jest.runAllTimers()); - expect(onLoadMore).toHaveBeenCalledTimes(2); + expect(onLoadMore).toHaveBeenCalledTimes(1); }); it('should fire onLoadMore if there aren\'t enough items to fill the ListBox ', async function () { @@ -880,9 +880,7 @@ describe('ListBox', function () { let listbox = getByRole('listbox'); let options = within(listbox).getAllByRole('option'); expect(options.length).toBe(5); - // onLoadMore called twice from onVisibleRectChange due to ListBox sizeToFit - // onLoadMore called twice from useLayoutEffect in React < 18 - expect(onLoadMore).toHaveBeenCalledTimes(parseInt(React.version, 10) >= 18 ? 2 : 4); + expect(onLoadMore).toHaveBeenCalledTimes(1); }); }); From bffe05cc236accf2938ecb5228a166da2b81ea82 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 21:13:42 -0700 Subject: [PATCH 14/53] Only dispatch initial fetch once in useAsyncList --- packages/@react-stately/data/src/useAsyncList.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/@react-stately/data/src/useAsyncList.ts b/packages/@react-stately/data/src/useAsyncList.ts index bd18c1a5589..9cc3bf253ec 100644 --- a/packages/@react-stately/data/src/useAsyncList.ts +++ b/packages/@react-stately/data/src/useAsyncList.ts @@ -11,7 +11,7 @@ */ import {createListActions, ListData, ListState} from './useListData'; -import {Key, Reducer, useEffect, useReducer} from 'react'; +import {Key, Reducer, useEffect, useReducer, useRef} from 'react'; import {LoadingState, Selection, SortDescriptor} from '@react-types/shared'; export interface AsyncListOptions { @@ -313,8 +313,12 @@ export function useAsyncList(options: AsyncListOptions): As } }; + let didDispatchInitialFetch = useRef(false); useEffect(() => { - dispatchFetch({type: 'loading'}, load); + if (!didDispatchInitialFetch.current) { + dispatchFetch({type: 'loading'}, load); + didDispatchInitialFetch.current = true; + } // eslint-disable-next-line react-hooks/exhaustive-deps }, []); From eec9f87e6a84d0364db386f1ccea2449a2c411a9 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 21:35:25 -0700 Subject: [PATCH 15/53] Fix breadcrumbs 20 --- packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx b/packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx index 5aab36a3dee..a478c6c1e40 100644 --- a/packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx +++ b/packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx @@ -134,8 +134,13 @@ function Breadcrumbs(props: SpectrumBreadcrumbsProps, ref: DOMRef) { useResizeObserver({ref: domRef, onResize: updateOverflow}); - // eslint-disable-next-line react-hooks/exhaustive-deps - useLayoutEffect(updateOverflow, [children]); + let lastChildren = useRef(null); + useLayoutEffect(() => { + if (children !== lastChildren.current) { + lastChildren.current = children; + updateOverflow(); + } + }); let contents = childArray; if (childArray.length > visibleItems) { From 5d2ed38a7c364b83ef8af26fa58a119521e7e530 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 22:31:17 -0700 Subject: [PATCH 16/53] Make sure we relayout when virtualizer changes size 18 --- packages/@react-spectrum/list/test/ListView.test.js | 6 +++--- packages/@react-spectrum/table/test/Table.test.js | 2 +- packages/@react-stately/virtualizer/src/Virtualizer.ts | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index 8ba6ea7d029..5bb7b0db720 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -1423,10 +1423,10 @@ describe('ListView', function () { }); expect(grid.scrollTop).toBe(0); - focusRow(tree, 'Item 10'); - expect(document.activeElement).toBe(getRow(tree, 'Item 10')); + focusRow(tree, 'Item 1'); + expect(document.activeElement).toBe(getRow(tree, 'Item 1')); - expect(grid.scrollTop).toBe(380); + expect(grid.scrollTop).toBe(20); }); it('should scroll to a row when it is focused off screen', function () { diff --git a/packages/@react-spectrum/table/test/Table.test.js b/packages/@react-spectrum/table/test/Table.test.js index 8bb14037646..e0932b17640 100644 --- a/packages/@react-spectrum/table/test/Table.test.js +++ b/packages/@react-spectrum/table/test/Table.test.js @@ -4052,7 +4052,7 @@ describe('TableView', function () { fireEvent.scroll(scrollView); act(() => {jest.runAllTimers();}); - expect(onLoadMore).toHaveBeenCalledTimes(3); + expect(onLoadMore).toHaveBeenCalledTimes(2); }); it('should automatically fire onLoadMore if there aren\'t enough items to fill the Table', function () { diff --git a/packages/@react-stately/virtualizer/src/Virtualizer.ts b/packages/@react-stately/virtualizer/src/Virtualizer.ts index 46675101da1..d4371a55965 100644 --- a/packages/@react-stately/virtualizer/src/Virtualizer.ts +++ b/packages/@react-stately/virtualizer/src/Virtualizer.ts @@ -185,7 +185,8 @@ export class Virtualizer { this._visibleRect = rect; if (shouldInvalidate) { - this.relayout({ + // We are already in a layout effect when this method is called, so relayoutNow is appropriate. + this.relayoutNow({ offsetChanged: !rect.pointEquals(current), sizeChanged: !rect.sizeEquals(current) }); From 94f04915946f8937f98f3bd65205b1bbe1c91e69 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 23:04:17 -0700 Subject: [PATCH 17/53] Fix toast 2 --- .../@react-spectrum/toast/src/ToastContainer.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/@react-spectrum/toast/src/ToastContainer.tsx b/packages/@react-spectrum/toast/src/ToastContainer.tsx index 4f4c348e5ac..97d2596ffb5 100644 --- a/packages/@react-spectrum/toast/src/ToastContainer.tsx +++ b/packages/@react-spectrum/toast/src/ToastContainer.tsx @@ -55,6 +55,12 @@ function subscribe(fn: () => void) { return () => subscriptions.delete(fn); } +function triggerSubscriptions() { + for (let fn of subscriptions) { + fn(); + } +} + function getActiveToastContainer() { return toastProviders.values().next().value; } @@ -73,10 +79,12 @@ export function ToastContainer(props: SpectrumToastContainerProps): ReactElement // We use a ref to do this, since it will have a stable identity // over the lifetime of the component. let ref = useRef(); - toastProviders.add(ref); // eslint-disable-next-line arrow-body-style useEffect(() => { + toastProviders.add(ref); + triggerSubscriptions(); + return () => { // When this toast provider unmounts, reset all animations so that // when the new toast provider renders, it is seamless. @@ -88,9 +96,7 @@ export function ToastContainer(props: SpectrumToastContainerProps): ReactElement // This will cause all other instances to re-render, // and the first one to become the new active toast provider. toastProviders.delete(ref); - for (let fn of subscriptions) { - fn(); - } + triggerSubscriptions(); }; }, []); From 34137a99c8312e200a6338be01502b8bc3b356d8 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 23:04:33 -0700 Subject: [PATCH 18/53] Only emit PressResponder warning once 1 --- packages/@react-aria/interactions/src/PressResponder.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@react-aria/interactions/src/PressResponder.tsx b/packages/@react-aria/interactions/src/PressResponder.tsx index d630b9e4d2d..eef0de84a83 100644 --- a/packages/@react-aria/interactions/src/PressResponder.tsx +++ b/packages/@react-aria/interactions/src/PressResponder.tsx @@ -42,6 +42,7 @@ export const PressResponder = React.forwardRef(({children, ...props}: PressRespo 'A PressResponder was rendered without a pressable child. ' + 'Either call the usePress hook, or wrap your DOM node with component.' ); + isRegistered.current = true; // only warn once in strict mode. } }, []); From 29a44373deea9ae4bd5d05bd94f4663109fc65c4 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 20 May 2023 23:39:44 -0700 Subject: [PATCH 19/53] skip bad useId merging test --- packages/@react-spectrum/utils/test/Slots.test.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/@react-spectrum/utils/test/Slots.test.js b/packages/@react-spectrum/utils/test/Slots.test.js index afee605924a..7ced4ba4732 100644 --- a/packages/@react-spectrum/utils/test/Slots.test.js +++ b/packages/@react-spectrum/utils/test/Slots.test.js @@ -96,7 +96,16 @@ describe('Slots', function () { expect(onPressUser).toHaveBeenCalledTimes(1); }); - it('lets users set their own id', function () { + it.skip('lets users set their own id', function () { + // This test does not work in strict mode because useId merges ids. + // Only the id that goes through useId can be updated (in this case "bar"), + // so the non-useId generated id ends up winning. Outside strict mode, this + // appears to work because the id is not registered until after the mergeProps + // so it is not updated. In strict mode we render twice so the id is updated + // during the second render. This would also be broken outside strict mode if + // the component _ever_ re-rendered, however. In the real world, all of the ids + // we would pass through slots are generated by useId (tested below) so this + // isn't a big problem. let slots = { slotname: {id: 'foo'} }; From bb3811b40e629f84549af0ba0e5751dba9ec12de Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 00:04:45 -0700 Subject: [PATCH 20/53] Centralize onLoadMore logic --- .../virtualizer/src/Virtualizer.tsx | 86 ++++++++++--------- packages/@react-aria/virtualizer/src/index.ts | 3 +- .../@react-spectrum/table/src/TableView.tsx | 42 +++------ .../@react-spectrum/table/test/Table.test.js | 5 +- 4 files changed, 59 insertions(+), 77 deletions(-) diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index cf480fe9980..92bbaed2475 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -11,9 +11,9 @@ */ import {Collection} from '@react-types/shared'; -import {focusWithoutScrolling, mergeProps, scrollIntoViewport, useLayoutEffect} from '@react-aria/utils'; import {getInteractionModality} from '@react-aria/interactions'; import {Layout, Rect, ReusableView, useVirtualizerState, VirtualizerState} from '@react-stately/virtualizer'; +import {mergeProps, scrollIntoViewport, useLayoutEffect} from '@react-aria/utils'; import React, {FocusEvent, HTMLAttributes, Key, ReactElement, RefObject, useCallback, useEffect, useMemo, useRef} from 'react'; import {ScrollView} from './ScrollView'; import {VirtualizerItem} from './VirtualizerItem'; @@ -74,50 +74,14 @@ function Virtualizer(props: VirtualizerProps, ref: Re } }); - let {virtualizerProps} = useVirtualizer(props, state, ref); - - // Handle scrolling, and call onLoadMore when nearing the bottom. - let isLoadingRef = useRef(isLoading); - let prevProps = useRef(props); - useLayoutEffect(() => { - // Only update isLoadingRef if props object actually changed, - // not if a local state change occurred. - if (props !== prevProps.current) { - isLoadingRef.current = isLoading; - prevProps.current = props; - } - }, [isLoading, props]); - - let onVisibleRectChange = useCallback((rect: Rect) => { - state.setVisibleRect(rect); - - if (!isLoadingRef.current && onLoadMore) { - let scrollOffset = state.virtualizer.contentSize.height - rect.height * 2; - if (rect.y > scrollOffset) { - isLoadingRef.current = true; - onLoadMore(); - } - } - }, [onLoadMore, state]); - - let lastContentSize = useRef(0); - useLayoutEffect(() => { - if (!isLoadingRef.current && onLoadMore && !state.isAnimating) { - if (state.contentSize.height !== lastContentSize.current && state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) { - isLoadingRef.current = true; - onLoadMore(); - } - } - lastContentSize.current = state.contentSize.height; - }, [state.contentSize, state.isAnimating, state.virtualizer, onLoadMore]); + let {virtualizerProps, scrollViewProps} = useVirtualizer(props, state, ref); return ( void, shouldUseVirtualFocus?: boolean, - autoFocus?: boolean + autoFocus?: boolean, + isLoading?: boolean, + onLoadMore?: () => void, } export function useVirtualizer(props: VirtualizerOptions, state: VirtualizerState, ref: RefObject) { - let {focusedKey, scrollToItem, shouldUseVirtualFocus} = props; + let {focusedKey, scrollToItem, shouldUseVirtualFocus, isLoading, onLoadMore} = props; let {virtualizer} = state; // Scroll to the focusedKey when it changes. Actually focusing the focusedKey // is up to the implementation using Virtualizer since we don't have refs @@ -211,11 +177,49 @@ export function useVirtualizer(props: VirtualizerOptions } } + // Handle scrolling, and call onLoadMore when nearing the bottom. + let isLoadingRef = useRef(isLoading); + let prevProps = useRef(props); + useLayoutEffect(() => { + // Only update isLoadingRef if props object actually changed, + // not if a local state change occurred. + if (props !== prevProps.current) { + isLoadingRef.current = isLoading; + prevProps.current = props; + } + }, [isLoading, props]); + + let onVisibleRectChange = useCallback((rect: Rect) => { + state.setVisibleRect(rect); + + if (!isLoadingRef.current && onLoadMore) { + let scrollOffset = state.virtualizer.contentSize.height - rect.height * 2; + if (rect.y > scrollOffset) { + isLoadingRef.current = true; + onLoadMore(); + } + } + }, [onLoadMore, state]); + + let lastContentSize = useRef(0); + useLayoutEffect(() => { + if (!isLoadingRef.current && onLoadMore && !state.isAnimating) { + if (state.contentSize.height !== lastContentSize.current && state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) { + isLoadingRef.current = true; + onLoadMore(); + } + } + lastContentSize.current = state.contentSize.height; + }, [state.contentSize, state.isAnimating, state.virtualizer, onLoadMore]); + return { virtualizerProps: { tabIndex, onFocus, onBlur + }, + scrollViewProps: { + onVisibleRectChange } }; } diff --git a/packages/@react-aria/virtualizer/src/index.ts b/packages/@react-aria/virtualizer/src/index.ts index 1910c2785e1..78713b4a795 100644 --- a/packages/@react-aria/virtualizer/src/index.ts +++ b/packages/@react-aria/virtualizer/src/index.ts @@ -11,8 +11,9 @@ */ export type {RTLOffsetType} from './utils'; +export type {VirtualizerItemOptions} from './useVirtualizerItem'; export {useVirtualizer, Virtualizer} from './Virtualizer'; -export {useVirtualizerItem, VirtualizerItemOptions} from './useVirtualizerItem'; +export {useVirtualizerItem} from './useVirtualizerItem'; export {VirtualizerItem, layoutInfoToStyle} from './VirtualizerItem'; export {ScrollView} from './ScrollView'; export {getRTLOffsetType, getScrollLeft, setScrollLeft} from './utils'; diff --git a/packages/@react-spectrum/table/src/TableView.tsx b/packages/@react-spectrum/table/src/TableView.tsx index 96786311a1e..d61d046d790 100644 --- a/packages/@react-spectrum/table/src/TableView.tsx +++ b/packages/@react-spectrum/table/src/TableView.tsx @@ -12,7 +12,7 @@ import {AriaLabelingProps, DOMProps, DOMRef, DropTarget, FocusableElement, FocusableRef, SpectrumSelectionProps, StyleProps} from '@react-types/shared'; import ArrowDownSmall from '@spectrum-icons/ui/ArrowDownSmall'; -import {chain, mergeProps, scrollIntoView, scrollIntoViewport, useLayoutEffect} from '@react-aria/utils'; +import {chain, mergeProps, scrollIntoView, scrollIntoViewport} from '@react-aria/utils'; import {Checkbox} from '@react-spectrum/checkbox'; import ChevronDownMedium from '@spectrum-icons/ui/ChevronDownMedium'; import { @@ -581,7 +581,8 @@ function TableView(props: SpectrumTableProps, ref: DOMRef ({ tabIndex: otherProps.tabIndex, focusedKey, - scrollToItem - }, state, domRef); + scrollToItem, + isLoading, + onLoadMore + }), [otherProps.tabIndex, focusedKey, scrollToItem, isLoading, onLoadMore]); + + let {virtualizerProps, scrollViewProps: {onVisibleRectChange}} = useVirtualizer(memoedVirtualizerProps, state, domRef); // this effect runs whenever the contentSize changes, it doesn't matter what the content size is // only that it changes in a resize, and when that happens, we want to sync the body to the @@ -657,33 +662,6 @@ function TableVirtualizer({layout, collection, focusedKey, renderView, renderWra headerRef.current.scrollLeft = bodyRef.current.scrollLeft; }, [bodyRef, headerRef]); - let isLoadingRef = useRef(isLoading); - useLayoutEffect(() => { - isLoadingRef.current = isLoading; - }, [isLoading]); - - let onVisibleRectChange = useCallback((rect: Rect) => { - state.setVisibleRect(rect); - - if (!isLoadingRef.current && onLoadMore) { - let scrollOffset = state.virtualizer.contentSize.height - rect.height * 2; - if (rect.y > scrollOffset) { - isLoadingRef.current = true; - onLoadMore(); - } - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [onLoadMore, state.setVisibleRect, state.virtualizer]); - - useLayoutEffect(() => { - if (!isLoadingRef.current && onLoadMore && !state.isAnimating) { - if (state.contentSize.height <= state.virtualizer.visibleRect.height) { - isLoadingRef.current = true; - onLoadMore(); - } - } - }, [state.contentSize, state.virtualizer, state.isAnimating, onLoadMore]); - let resizerPosition = layout.getResizerPosition() - 2; let resizerAtEdge = resizerPosition > Math.max(state.virtualizer.contentSize.width, state.virtualizer.visibleRect.width) - 3; diff --git a/packages/@react-spectrum/table/test/Table.test.js b/packages/@react-spectrum/table/test/Table.test.js index e0932b17640..94021715952 100644 --- a/packages/@react-spectrum/table/test/Table.test.js +++ b/packages/@react-spectrum/table/test/Table.test.js @@ -4052,7 +4052,7 @@ describe('TableView', function () { fireEvent.scroll(scrollView); act(() => {jest.runAllTimers();}); - expect(onLoadMore).toHaveBeenCalledTimes(2); + expect(onLoadMore).toHaveBeenCalledTimes(1); }); it('should automatically fire onLoadMore if there aren\'t enough items to fill the Table', function () { @@ -4077,8 +4077,7 @@ describe('TableView', function () { render(); act(() => jest.runAllTimers()); - // first loadMore triggered by onVisibleRectChange, other 2 by useLayoutEffect - expect(onLoadMoreSpy).toHaveBeenCalledTimes(3); + expect(onLoadMoreSpy).toHaveBeenCalledTimes(1); }); }); From bc703231aabbb0a2de8190d2d8cc6d8d492d598e Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 00:47:01 -0700 Subject: [PATCH 21/53] Fix NumberField and ColorField Simplified versions of #4005 with minimal changes. Uses state instead of refs to store previous values and avoid flicker due to effect. Removes parsed ref, which has been unnecessary since 10e57d449dde2ae6bd095a63a8339e1d4c61cf59. --- .../spinbutton/src/useSpinButton.ts | 16 ++++------- .../color/src/useColorFieldState.ts | 19 ++++++------- .../numberfield/src/useNumberFieldState.ts | 28 ++++++++----------- 3 files changed, 25 insertions(+), 38 deletions(-) diff --git a/packages/@react-aria/spinbutton/src/useSpinButton.ts b/packages/@react-aria/spinbutton/src/useSpinButton.ts index e02d4f99dc9..955b4a9b0c2 100644 --- a/packages/@react-aria/spinbutton/src/useSpinButton.ts +++ b/packages/@react-aria/spinbutton/src/useSpinButton.ts @@ -16,7 +16,7 @@ import {DOMAttributes, InputBase, RangeInputBase, Validation, ValueBase} from '@ // @ts-ignore import intlMessages from '../intl/*.json'; import {useCallback, useEffect, useRef} from 'react'; -import {useGlobalListeners} from '@react-aria/utils'; +import {useEffectEvent, useGlobalListeners} from '@react-aria/utils'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; @@ -56,8 +56,6 @@ export function useSpinButton( onIncrementToMax } = props; const stringFormatter = useLocalizedStringFormatter(intlMessages); - const propsRef = useRef(props); - propsRef.current = props; const clearAsync = () => clearTimeout(_async.current); @@ -137,10 +135,10 @@ export function useSpinButton( } }, [textValue]); - const onIncrementPressStart = useCallback( + const onIncrementPressStart = useEffectEvent( (initialStepDelay: number) => { clearAsync(); - propsRef.current.onIncrement(); + onIncrement(); // Start spinning after initial delay _async.current = window.setTimeout( () => { @@ -151,14 +149,12 @@ export function useSpinButton( initialStepDelay ); }, - // eslint-disable-next-line react-hooks/exhaustive-deps - [onIncrement, maxValue, value] ); - const onDecrementPressStart = useCallback( + const onDecrementPressStart = useEffectEvent( (initialStepDelay: number) => { clearAsync(); - propsRef.current.onDecrement(); + onDecrement(); // Start spinning after initial delay _async.current = window.setTimeout( () => { @@ -169,8 +165,6 @@ export function useSpinButton( initialStepDelay ); }, - // eslint-disable-next-line react-hooks/exhaustive-deps - [onDecrement, minValue, value] ); let cancelContextMenu = (e) => { diff --git a/packages/@react-stately/color/src/useColorFieldState.ts b/packages/@react-stately/color/src/useColorFieldState.ts index e02ce7f1ec7..7aea1520d13 100644 --- a/packages/@react-stately/color/src/useColorFieldState.ts +++ b/packages/@react-stately/color/src/useColorFieldState.ts @@ -14,7 +14,7 @@ import {Color, ColorFieldProps} from '@react-types/color'; import {parseColor} from './Color'; import {useColor} from './useColor'; import {useControlledState} from '@react-stately/utils'; -import {useMemo, useRef, useState} from 'react'; +import {useMemo, useState} from 'react'; export interface ColorFieldState { /** @@ -85,13 +85,12 @@ export function useColorFieldState( } }; - let prevValue = useRef(colorValue); - if (prevValue.current !== colorValue) { + let [prevValue, setPrevValue] = useState(colorValue); + if (prevValue !== colorValue) { setInputValue(colorValue ? colorValue.toString('hex') : ''); - prevValue.current = colorValue; + setPrevValue(colorValue); } - let parsedValue = useMemo(() => { let color; try { @@ -101,8 +100,6 @@ export function useColorFieldState( } return color; }, [inputValue]); - let parsed = useRef(null); - parsed.current = parsedValue; let commit = () => { // Set to empty state if input value is empty @@ -113,12 +110,12 @@ export function useColorFieldState( } // if it failed to parse, then reset input to formatted version of current number - if (parsed.current == null) { + if (parsedValue == null) { setInputValue(colorValue ? colorValue.toString('hex') : ''); return; } - safelySetColorValue(parsed.current); + safelySetColorValue(parsedValue); // in a controlled state, the numberValue won't change, so we won't go back to our old input without help let newColorValue = ''; if (colorValue) { @@ -128,7 +125,7 @@ export function useColorFieldState( }; let increment = () => { - let newValue = addColorValue(parsed.current, step); + let newValue = addColorValue(parsedValue, step); // if we've arrived at the same value that was previously in the state, the // input value should be updated to match // ex type 4, press increment, highlight the number in the input, type 4 again, press increment @@ -139,7 +136,7 @@ export function useColorFieldState( safelySetColorValue(newValue); }; let decrement = () => { - let newValue = addColorValue(parsed.current, -step); + let newValue = addColorValue(parsedValue, -step); // if we've arrived at the same value that was previously in the state, the // input value should be updated to match // ex type 4, press increment, highlight the number in the input, type 4 again, press increment diff --git a/packages/@react-stately/numberfield/src/useNumberFieldState.ts b/packages/@react-stately/numberfield/src/useNumberFieldState.ts index c75ba28610d..6dd6cc2cb16 100644 --- a/packages/@react-stately/numberfield/src/useNumberFieldState.ts +++ b/packages/@react-stately/numberfield/src/useNumberFieldState.ts @@ -13,7 +13,7 @@ import {clamp, snapValueToStep, useControlledState} from '@react-stately/utils'; import {NumberFieldProps} from '@react-types/numberfield'; import {NumberFormatter, NumberParser} from '@internationalized/number'; -import {useCallback, useMemo, useRef, useState} from 'react'; +import {useCallback, useMemo, useState} from 'react'; export interface NumberFieldState { /** @@ -104,21 +104,17 @@ export function useNumberFieldState( // Update the input value when the number value or format options change. This is done // in a useEffect so that the controlled behavior is correct and we only update the // textfield after prop changes. - let prevValue = useRef(numberValue); - let prevLocale = useRef(locale); - let prevFormatOptions = useRef(formatOptions); - if (!Object.is(numberValue, prevValue.current) || locale !== prevLocale.current || formatOptions !== prevFormatOptions.current) { + let [prevValue, setPrevValue] = useState(numberValue); + let [prevLocale, setPrevLocale] = useState(locale); + let [prevFormatOptions, setPrevFormatOptions] = useState(formatOptions); + if (!Object.is(numberValue, prevValue) || locale !== prevLocale || formatOptions !== prevFormatOptions) { setInputValue(format(numberValue)); - prevValue.current = numberValue; - prevLocale.current = locale; - prevFormatOptions.current = formatOptions; + setPrevValue(numberValue); + setPrevLocale(locale); + setPrevFormatOptions(formatOptions); } - // Store last parsed value in a ref so it can be used by increment/decrement below let parsedValue = useMemo(() => numberParser.parse(inputValue), [numberParser, inputValue]); - let parsed = useRef(0); - parsed.current = parsedValue; - let commit = () => { // Set to empty state if input value is empty if (!inputValue.length) { @@ -128,7 +124,7 @@ export function useNumberFieldState( } // if it failed to parse, then reset input to formatted version of current number - if (isNaN(parsed.current)) { + if (isNaN(parsedValue)) { setInputValue(format(numberValue)); return; } @@ -136,9 +132,9 @@ export function useNumberFieldState( // Clamp to min and max, round to the nearest step, and round to specified number of digits let clampedValue: number; if (isNaN(step)) { - clampedValue = clamp(parsed.current, minValue, maxValue); + clampedValue = clamp(parsedValue, minValue, maxValue); } else { - clampedValue = snapValueToStep(parsed.current, minValue, maxValue, step); + clampedValue = snapValueToStep(parsedValue, minValue, maxValue, step); } clampedValue = numberParser.parse(format(clampedValue)); @@ -149,7 +145,7 @@ export function useNumberFieldState( }; let safeNextStep = (operation: '+' | '-', minMax: number) => { - let prev = parsed.current; + let prev = parsedValue; if (isNaN(prev)) { // if the input is empty, start from the min/max value when incrementing/decrementing, From b60944645c86c473a6137b4c30387064b01d3e46 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 01:21:38 -0700 Subject: [PATCH 22/53] Make useMove use useEffectEvent Fixes sliders and color components (alternative to #4012). --- .../@react-aria/color/src/useColorArea.ts | 26 +++--- .../@react-aria/color/src/useColorWheel.ts | 21 ++--- .../@react-aria/interactions/src/useMove.ts | 84 ++++++++++--------- packages/@react-aria/slider/src/useSlider.ts | 8 +- .../@react-aria/slider/src/useSliderThumb.ts | 12 ++- .../slider/test/RangeSlider.test.tsx | 13 +-- .../slider/test/Slider.test.tsx | 13 +-- .../slider/src/useSliderState.ts | 1 + .../utils/test/useControlledState.test.js | 8 +- 9 files changed, 95 insertions(+), 91 deletions(-) diff --git a/packages/@react-aria/color/src/useColorArea.ts b/packages/@react-aria/color/src/useColorArea.ts index e3c08e21d5e..2f0cf672c32 100644 --- a/packages/@react-aria/color/src/useColorArea.ts +++ b/packages/@react-aria/color/src/useColorArea.ts @@ -70,11 +70,9 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) } }, [inputXRef]); - let stateRef = useRef(null); - stateRef.current = state; - let {xChannel, yChannel, zChannel} = stateRef.current.channels; - let xChannelStep = stateRef.current.xChannelStep; - let yChannelStep = stateRef.current.yChannelStep; + let {xChannel, yChannel, zChannel} = state.channels; + let xChannelStep = state.xChannelStep; + let yChannelStep = state.yChannelStep; let currentPosition = useRef<{x: number, y: number}>(null); @@ -88,27 +86,27 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) // same handling as useMove, don't need to stop propagation, useKeyboard will do that for us e.preventDefault(); // remember to set this and unset it so that onChangeEnd is fired - stateRef.current.setDragging(true); + state.setDragging(true); valueChangedViaKeyboard.current = true; switch (e.key) { case 'PageUp': - stateRef.current.incrementY(stateRef.current.yChannelPageStep); + state.incrementY(state.yChannelPageStep); focusedInputRef.current = inputYRef.current; break; case 'PageDown': - stateRef.current.decrementY(stateRef.current.yChannelPageStep); + state.decrementY(state.yChannelPageStep); focusedInputRef.current = inputYRef.current; break; case 'Home': - direction === 'rtl' ? stateRef.current.incrementX(stateRef.current.xChannelPageStep) : stateRef.current.decrementX(stateRef.current.xChannelPageStep); + direction === 'rtl' ? state.incrementX(state.xChannelPageStep) : state.decrementX(state.xChannelPageStep); focusedInputRef.current = inputXRef.current; break; case 'End': - direction === 'rtl' ? stateRef.current.decrementX(stateRef.current.xChannelPageStep) : stateRef.current.incrementX(stateRef.current.xChannelPageStep); + direction === 'rtl' ? state.decrementX(state.xChannelPageStep) : state.incrementX(state.xChannelPageStep); focusedInputRef.current = inputXRef.current; break; } - stateRef.current.setDragging(false); + state.setDragging(false); if (focusedInputRef.current) { focusInput(focusedInputRef.current ? focusedInputRef : inputXRef); } @@ -118,7 +116,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) let moveHandler = { onMoveStart() { currentPosition.current = null; - stateRef.current.setDragging(true); + state.setDragging(true); }, onMove({deltaX, deltaY, pointerType, shiftKey}) { let { @@ -132,7 +130,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) yChannelStep, getThumbPosition, setColorFromPoint - } = stateRef.current; + } = state; if (currentPosition.current == null) { currentPosition.current = getThumbPosition(); } @@ -161,7 +159,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) }, onMoveEnd() { isOnColorArea.current = undefined; - stateRef.current.setDragging(false); + state.setDragging(false); focusInput(focusedInputRef.current ? focusedInputRef : inputXRef); } }; diff --git a/packages/@react-aria/color/src/useColorWheel.ts b/packages/@react-aria/color/src/useColorWheel.ts index 202c726df58..b1229b36cae 100644 --- a/packages/@react-aria/color/src/useColorWheel.ts +++ b/packages/@react-aria/color/src/useColorWheel.ts @@ -56,9 +56,6 @@ export function useColorWheel(props: AriaColorWheelOptions, state: ColorWheelSta } }, [inputRef]); - let stateRef = useRef(null); - stateRef.current = state; - let currentPosition = useRef<{x: number, y: number}>(null); let {keyboardProps} = useKeyboard({ @@ -71,18 +68,18 @@ export function useColorWheel(props: AriaColorWheelOptions, state: ColorWheelSta // same handling as useMove, don't need to stop propagation, useKeyboard will do that for us e.preventDefault(); // remember to set this and unset it so that onChangeEnd is fired - stateRef.current.setDragging(true); + state.setDragging(true); switch (e.key) { case 'PageUp': e.preventDefault(); - state.increment(stateRef.current.pageStep); + state.increment(state.pageStep); break; case 'PageDown': e.preventDefault(); - state.decrement(stateRef.current.pageStep); + state.decrement(state.pageStep); break; } - stateRef.current.setDragging(false); + state.setDragging(false); } }); @@ -93,18 +90,18 @@ export function useColorWheel(props: AriaColorWheelOptions, state: ColorWheelSta }, onMove({deltaX, deltaY, pointerType, shiftKey}) { if (currentPosition.current == null) { - currentPosition.current = stateRef.current.getThumbPosition(thumbRadius); + currentPosition.current = state.getThumbPosition(thumbRadius); } currentPosition.current.x += deltaX; currentPosition.current.y += deltaY; if (pointerType === 'keyboard') { if (deltaX > 0 || deltaY < 0) { - state.increment(shiftKey ? stateRef.current.pageStep : stateRef.current.step); + state.increment(shiftKey ? state.pageStep : state.step); } else if (deltaX < 0 || deltaY > 0) { - state.decrement(shiftKey ? stateRef.current.pageStep : stateRef.current.step); + state.decrement(shiftKey ? state.pageStep : state.step); } } else { - stateRef.current.setHueFromPoint(currentPosition.current.x, currentPosition.current.y, thumbRadius); + state.setHueFromPoint(currentPosition.current.x, currentPosition.current.y, thumbRadius); } }, onMoveEnd() { @@ -175,7 +172,7 @@ export function useColorWheel(props: AriaColorWheelOptions, state: ColorWheelSta if (innerRadius < radius && radius < outerRadius && !state.isDragging && currentPointer.current === undefined) { isOnTrack.current = true; currentPointer.current = id; - stateRef.current.setHueFromPoint(x, y, radius); + state.setHueFromPoint(x, y, radius); focusInput(); state.setDragging(true); diff --git a/packages/@react-aria/interactions/src/useMove.ts b/packages/@react-aria/interactions/src/useMove.ts index 5f42caf5ac4..658ca237a61 100644 --- a/packages/@react-aria/interactions/src/useMove.ts +++ b/packages/@react-aria/interactions/src/useMove.ts @@ -13,7 +13,7 @@ import {disableTextSelection, restoreTextSelection} from './textSelection'; import {DOMAttributes, MoveEvents, PointerType} from '@react-types/shared'; import React, {useMemo, useRef} from 'react'; -import {useGlobalListeners} from '@react-aria/utils'; +import {useEffectEvent, useGlobalListeners} from '@react-aria/utils'; export interface MoveResult { /** Props to spread on the target element. */ @@ -43,52 +43,54 @@ export function useMove(props: MoveEvents): MoveResult { let {addGlobalListener, removeGlobalListener} = useGlobalListeners(); - let moveProps = useMemo(() => { - let moveProps: DOMAttributes = {}; - - let start = () => { - disableTextSelection(); - state.current.didMove = false; - }; - let move = (originalEvent: EventBase, pointerType: PointerType, deltaX: number, deltaY: number) => { - if (deltaX === 0 && deltaY === 0) { - return; - } + let move = useEffectEvent((originalEvent: EventBase, pointerType: PointerType, deltaX: number, deltaY: number) => { + if (deltaX === 0 && deltaY === 0) { + return; + } - if (!state.current.didMove) { - state.current.didMove = true; - onMoveStart?.({ - type: 'movestart', - pointerType, - shiftKey: originalEvent.shiftKey, - metaKey: originalEvent.metaKey, - ctrlKey: originalEvent.ctrlKey, - altKey: originalEvent.altKey - }); - } - onMove({ - type: 'move', + if (!state.current.didMove) { + state.current.didMove = true; + onMoveStart?.({ + type: 'movestart', pointerType, - deltaX: deltaX, - deltaY: deltaY, shiftKey: originalEvent.shiftKey, metaKey: originalEvent.metaKey, ctrlKey: originalEvent.ctrlKey, altKey: originalEvent.altKey }); - }; - let end = (originalEvent: EventBase, pointerType: PointerType) => { - restoreTextSelection(); - if (state.current.didMove) { - onMoveEnd?.({ - type: 'moveend', - pointerType, - shiftKey: originalEvent.shiftKey, - metaKey: originalEvent.metaKey, - ctrlKey: originalEvent.ctrlKey, - altKey: originalEvent.altKey - }); - } + } + onMove({ + type: 'move', + pointerType, + deltaX: deltaX, + deltaY: deltaY, + shiftKey: originalEvent.shiftKey, + metaKey: originalEvent.metaKey, + ctrlKey: originalEvent.ctrlKey, + altKey: originalEvent.altKey + }); + }); + + let end = useEffectEvent((originalEvent: EventBase, pointerType: PointerType) => { + restoreTextSelection(); + if (state.current.didMove) { + onMoveEnd?.({ + type: 'moveend', + pointerType, + shiftKey: originalEvent.shiftKey, + metaKey: originalEvent.metaKey, + ctrlKey: originalEvent.ctrlKey, + altKey: originalEvent.altKey + }); + } + }); + + let moveProps = useMemo(() => { + let moveProps: DOMAttributes = {}; + + let start = () => { + disableTextSelection(); + state.current.didMove = false; }; if (typeof PointerEvent === 'undefined') { @@ -223,7 +225,7 @@ export function useMove(props: MoveEvents): MoveResult { }; return moveProps; - }, [state, onMoveStart, onMove, onMoveEnd, addGlobalListener, removeGlobalListener]); + }, [state, addGlobalListener, removeGlobalListener, move, end]); return {moveProps}; } diff --git a/packages/@react-aria/slider/src/useSlider.ts b/packages/@react-aria/slider/src/useSlider.ts index 668d40822d2..ee87307dc7e 100644 --- a/packages/@react-aria/slider/src/useSlider.ts +++ b/packages/@react-aria/slider/src/useSlider.ts @@ -66,8 +66,6 @@ export function useSlider( // It is set onMouseDown/onTouchDown; see trackProps below. const realTimeTrackDraggingIndex = useRef(null); - const stateRef = useRef(null); - stateRef.current = state; const reverseX = direction === 'rtl'; const currentPosition = useRef(null); const {moveProps} = useMove({ @@ -79,7 +77,7 @@ export function useSlider( let size = isVertical ? height : width; if (currentPosition.current == null) { - currentPosition.current = stateRef.current.getThumbPercent(realTimeTrackDraggingIndex.current) * size; + currentPosition.current = state.getThumbPercent(realTimeTrackDraggingIndex.current) * size; } let delta = isVertical ? deltaY : deltaX; @@ -91,12 +89,12 @@ export function useSlider( if (realTimeTrackDraggingIndex.current != null && trackRef.current) { const percent = clamp(currentPosition.current / size, 0, 1); - stateRef.current.setThumbPercent(realTimeTrackDraggingIndex.current, percent); + state.setThumbPercent(realTimeTrackDraggingIndex.current, percent); } }, onMoveEnd() { if (realTimeTrackDraggingIndex.current != null) { - stateRef.current.setThumbDragging(realTimeTrackDraggingIndex.current, false); + state.setThumbDragging(realTimeTrackDraggingIndex.current, false); realTimeTrackDraggingIndex.current = null; } } diff --git a/packages/@react-aria/slider/src/useSliderThumb.ts b/packages/@react-aria/slider/src/useSliderThumb.ts index 249097a1947..59e16f699ec 100644 --- a/packages/@react-aria/slider/src/useSliderThumb.ts +++ b/packages/@react-aria/slider/src/useSliderThumb.ts @@ -82,8 +82,6 @@ export function useSliderThumb( } }, [isFocused, focusInput]); - const stateRef = useRef(null); - stateRef.current = state; let reverseX = direction === 'rtl'; let currentPosition = useRef(null); @@ -97,7 +95,7 @@ export function useSliderThumb( setThumbValue, setThumbDragging, pageSize - } = stateRef.current; + } = state; // these are the cases that useMove or useSlider don't handle if (!/^(PageUp|PageDown|Home|End)$/.test(e.key)) { e.continuePropagation(); @@ -128,7 +126,7 @@ export function useSliderThumb( let {moveProps} = useMove({ onMoveStart() { currentPosition.current = null; - stateRef.current.setThumbDragging(index, true); + state.setThumbDragging(index, true); }, onMove({deltaX, deltaY, pointerType, shiftKey}) { const { @@ -138,7 +136,7 @@ export function useSliderThumb( incrementThumb, step, pageSize - } = stateRef.current; + } = state; let {width, height} = trackRef.current.getBoundingClientRect(); let size = isVertical ? height : width; @@ -162,7 +160,7 @@ export function useSliderThumb( } }, onMoveEnd() { - stateRef.current.setThumbDragging(index, false); + state.setThumbDragging(index, false); } }); @@ -244,7 +242,7 @@ export function useSliderThumb( 'aria-invalid': validationState === 'invalid' || undefined, 'aria-errormessage': opts['aria-errormessage'], onChange: (e: ChangeEvent) => { - stateRef.current.setThumbValue(index, parseFloat(e.target.value)); + state.setThumbValue(index, parseFloat(e.target.value)); } }), thumbProps: { diff --git a/packages/@react-spectrum/slider/test/RangeSlider.test.tsx b/packages/@react-spectrum/slider/test/RangeSlider.test.tsx index c2952bb2363..7ff0f2bb68c 100644 --- a/packages/@react-spectrum/slider/test/RangeSlider.test.tsx +++ b/packages/@react-spectrum/slider/test/RangeSlider.test.tsx @@ -14,7 +14,7 @@ import {fireEvent, render} from '@react-spectrum/test-utils'; import {press, testKeypresses} from './utils'; import {Provider} from '@adobe/react-spectrum'; import {RangeSlider} from '../'; -import React, {useState} from 'react'; +import React, {useCallback, useState} from 'react'; import {theme} from '@react-spectrum/theme-default'; import userEvent from '@testing-library/user-event'; @@ -126,11 +126,14 @@ describe('RangeSlider', function () { }); it('can be controlled', function () { - let renders = []; + let setValues = []; function Test() { - let [value, setValue] = useState({start: 20, end: 40}); - renders.push(value); + let [value, _setValue] = useState({start: 20, end: 40}); + let setValue = useCallback((val) => { + setValues.push(val); + _setValue(val); + }, [_setValue]); return (); } @@ -154,7 +157,7 @@ describe('RangeSlider', function () { expect(sliderRight).toHaveAttribute('aria-valuetext', '50'); expect(output).toHaveTextContent('30 โ€“ 50'); - expect(renders).toStrictEqual([{start: 20, end: 40}, {start: 30, end: 40}, {start: 30, end: 50}]); + expect(setValues).toStrictEqual([{start: 30, end: 40}, {start: 30, end: 50}]); }); it('supports a custom valueLabel', function () { diff --git a/packages/@react-spectrum/slider/test/Slider.test.tsx b/packages/@react-spectrum/slider/test/Slider.test.tsx index c28974c8920..4acd318497e 100644 --- a/packages/@react-spectrum/slider/test/Slider.test.tsx +++ b/packages/@react-spectrum/slider/test/Slider.test.tsx @@ -13,7 +13,7 @@ import {act, fireEvent, installMouseEvent, render} from '@react-spectrum/test-utils'; import {press, testKeypresses} from './utils'; import {Provider} from '@adobe/react-spectrum'; -import React, {useState} from 'react'; +import React, {useCallback, useState} from 'react'; import {Slider} from '../'; import {theme} from '@react-spectrum/theme-default'; import userEvent from '@testing-library/user-event'; @@ -119,11 +119,14 @@ describe('Slider', function () { }); it('can be controlled', function () { - let renders = []; + let setValues = []; function Test() { - let [value, setValue] = useState(50); - renders.push(value); + let [value, _setValue] = useState(50); + let setValue = useCallback((val) => { + setValues.push(val); + _setValue(val); + }, [_setValue]); return (); } @@ -141,7 +144,7 @@ describe('Slider', function () { expect(slider).toHaveAttribute('aria-valuetext', '55'); expect(output).toHaveTextContent('55'); - expect(renders).toStrictEqual([50, 55]); + expect(setValues).toStrictEqual([55]); }); it('supports a custom getValueLabel', function () { diff --git a/packages/@react-stately/slider/src/useSliderState.ts b/packages/@react-stately/slider/src/useSliderState.ts index 2132000bd45..d23e4c9407a 100644 --- a/packages/@react-stately/slider/src/useSliderState.ts +++ b/packages/@react-stately/slider/src/useSliderState.ts @@ -191,6 +191,7 @@ export function useSliderState(props: SliderStateOp const isEditablesRef = useRef(new Array(values.length).fill(true)); const [focusedIndex, setFocusedIndex] = useState(undefined); + // These refs should be ok as long as we never read them during render. const valuesRef = useRef(null); valuesRef.current = values; const isDraggingsRef = useRef(null); diff --git a/packages/@react-stately/utils/test/useControlledState.test.js b/packages/@react-stately/utils/test/useControlledState.test.js index 1a6875c0528..e904a397c9b 100644 --- a/packages/@react-stately/utils/test/useControlledState.test.js +++ b/packages/@react-stately/utils/test/useControlledState.test.js @@ -99,12 +99,16 @@ describe('useControlledState tests', function () { let {getByRole, getByTestId} = render(); let button = getByRole('button'); getByTestId('5'); - expect(renderSpy).toBeCalledTimes(1); + if (!process.env.STRICT_MODE) { + expect(renderSpy).toBeCalledTimes(1); + } actDOM(() => userEvent.click(button) ); getByTestId('6'); - expect(renderSpy).toBeCalledTimes(2); + if (!process.env.STRICT_MODE) { + expect(renderSpy).toBeCalledTimes(2); + } }); it('can handle controlled setValue behavior', () => { From 9ff42e478b39310d874880a0f7bece1817376b1c Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 01:24:58 -0700 Subject: [PATCH 23/53] useEffectEvent in useFormattedTextField --- .../textfield/src/useFormattedTextField.ts | 119 +++++++++--------- 1 file changed, 57 insertions(+), 62 deletions(-) diff --git a/packages/@react-aria/textfield/src/useFormattedTextField.ts b/packages/@react-aria/textfield/src/useFormattedTextField.ts index 27809eff638..f12903db1c7 100644 --- a/packages/@react-aria/textfield/src/useFormattedTextField.ts +++ b/packages/@react-aria/textfield/src/useFormattedTextField.ts @@ -11,7 +11,7 @@ */ import {AriaTextFieldProps} from '@react-types/textfield'; -import {mergeProps} from '@react-aria/utils'; +import {mergeProps, useEffectEvent} from '@react-aria/utils'; import {RefObject, useEffect, useRef} from 'react'; import {TextFieldAria, useTextField} from './useTextField'; @@ -29,81 +29,76 @@ function supportsNativeBeforeInputEvent() { } export function useFormattedTextField(props: AriaTextFieldProps, state: FormattedTextFieldState, inputRef: RefObject): TextFieldAria { - - let stateRef = useRef(state); - stateRef.current = state; - // All browsers implement the 'beforeinput' event natively except Firefox // (currently behind a flag as of Firefox 84). React's polyfill does not // run in all cases that the native event fires, e.g. when deleting text. // Use the native event if available so that we can prevent invalid deletions. // We do not attempt to polyfill this in Firefox since it would be very complicated, // the benefit of doing so is fairly minor, and it's going to be natively supported soon. + let onBeforeInputFallback = useEffectEvent((e: InputEvent) => { + let input = inputRef.current; + + // Compute the next value of the input if the event is allowed to proceed. + // See https://www.w3.org/TR/input-events-2/#interface-InputEvent-Attributes for a full list of input types. + let nextValue: string; + switch (e.inputType) { + case 'historyUndo': + case 'historyRedo': + // Explicitly allow undo/redo. e.data is null in this case, but there's no need to validate, + // because presumably the input would have already been validated previously. + return; + case 'deleteContent': + case 'deleteByCut': + case 'deleteByDrag': + nextValue = input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd); + break; + case 'deleteContentForward': + // This is potentially incorrect, since the browser may actually delete more than a single UTF-16 + // character. In reality, a full Unicode grapheme cluster consisting of multiple UTF-16 characters + // or code points may be deleted. However, in our currently supported locales, there are no such cases. + // If we support additional locales in the future, this may need to change. + nextValue = input.selectionEnd === input.selectionStart + ? input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd + 1) + : input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd); + break; + case 'deleteContentBackward': + nextValue = input.selectionEnd === input.selectionStart + ? input.value.slice(0, input.selectionStart - 1) + input.value.slice(input.selectionStart) + : input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd); + break; + case 'deleteSoftLineBackward': + case 'deleteHardLineBackward': + nextValue = input.value.slice(input.selectionStart); + break; + default: + if (e.data != null) { + nextValue = + input.value.slice(0, input.selectionStart) + + e.data + + input.value.slice(input.selectionEnd); + } + break; + } + + // If we did not compute a value, or the new value is invalid, prevent the event + // so that the browser does not update the input text, move the selection, or add to + // the undo/redo stack. + if (nextValue == null || !state.validate(nextValue)) { + e.preventDefault(); + } + }); + useEffect(() => { if (!supportsNativeBeforeInputEvent()) { return; } let input = inputRef.current; - - let onBeforeInput = (e: InputEvent) => { - let state = stateRef.current; - - // Compute the next value of the input if the event is allowed to proceed. - // See https://www.w3.org/TR/input-events-2/#interface-InputEvent-Attributes for a full list of input types. - let nextValue: string; - switch (e.inputType) { - case 'historyUndo': - case 'historyRedo': - // Explicitly allow undo/redo. e.data is null in this case, but there's no need to validate, - // because presumably the input would have already been validated previously. - return; - case 'deleteContent': - case 'deleteByCut': - case 'deleteByDrag': - nextValue = input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd); - break; - case 'deleteContentForward': - // This is potentially incorrect, since the browser may actually delete more than a single UTF-16 - // character. In reality, a full Unicode grapheme cluster consisting of multiple UTF-16 characters - // or code points may be deleted. However, in our currently supported locales, there are no such cases. - // If we support additional locales in the future, this may need to change. - nextValue = input.selectionEnd === input.selectionStart - ? input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd + 1) - : input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd); - break; - case 'deleteContentBackward': - nextValue = input.selectionEnd === input.selectionStart - ? input.value.slice(0, input.selectionStart - 1) + input.value.slice(input.selectionStart) - : input.value.slice(0, input.selectionStart) + input.value.slice(input.selectionEnd); - break; - case 'deleteSoftLineBackward': - case 'deleteHardLineBackward': - nextValue = input.value.slice(input.selectionStart); - break; - default: - if (e.data != null) { - nextValue = - input.value.slice(0, input.selectionStart) + - e.data + - input.value.slice(input.selectionEnd); - } - break; - } - - // If we did not compute a value, or the new value is invalid, prevent the event - // so that the browser does not update the input text, move the selection, or add to - // the undo/redo stack. - if (nextValue == null || !state.validate(nextValue)) { - e.preventDefault(); - } - }; - - input.addEventListener('beforeinput', onBeforeInput, false); + input.addEventListener('beforeinput', onBeforeInputFallback, false); return () => { - input.removeEventListener('beforeinput', onBeforeInput, false); + input.removeEventListener('beforeinput', onBeforeInputFallback, false); }; - }, [inputRef, stateRef]); + }, [onBeforeInputFallback]); let onBeforeInput = !supportsNativeBeforeInputEvent() ? e => { From e68a3a08a4369c48eca61d3579567974c75825fb Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 11:47:53 -0700 Subject: [PATCH 24/53] Fix calendar Incorporates #4035 --- .../calendar/src/useCalendarBase.ts | 20 +++++++++---------- .../@react-aria/focus/src/useFocusRing.ts | 2 +- .../@react-aria/i18n/src/useDateFormatter.ts | 2 ++ .../@react-aria/interactions/src/utils.ts | 10 ++++++---- .../@react-aria/utils/src/useUpdateEffect.ts | 7 +++++++ .../calendar/test/Calendar.test.js | 16 ++++++++++----- .../calendar/test/RangeCalendar.test.js | 6 ++++-- .../calendar/src/useCalendarState.ts | 6 +++--- .../calendar/src/useRangeCalendarState.ts | 6 +++--- .../@react-stately/datepicker/src/utils.ts | 14 ++++++++----- 10 files changed, 55 insertions(+), 34 deletions(-) diff --git a/packages/@react-aria/calendar/src/useCalendarBase.ts b/packages/@react-aria/calendar/src/useCalendarBase.ts index 3c7ba57b9d6..2cb5c9a97b2 100644 --- a/packages/@react-aria/calendar/src/useCalendarBase.ts +++ b/packages/@react-aria/calendar/src/useCalendarBase.ts @@ -21,7 +21,7 @@ import {hookData, useSelectedDateDescription, useVisibleRangeDescription} from ' // @ts-ignore import intlMessages from '../intl/*.json'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; -import {useRef} from 'react'; +import {useRef, useState} from 'react'; export interface CalendarAria { /** Props for the calendar grouping element. */ @@ -71,17 +71,17 @@ export function useCalendarBase(props: CalendarPropsBase & DOMProps & AriaLabeli }); // If the next or previous buttons become disabled while they are focused, move focus to the calendar body. - let nextFocused = useRef(false); + let [nextFocused, setNextFocused] = useState(false); let nextDisabled = props.isDisabled || state.isNextVisibleRangeInvalid(); - if (nextDisabled && nextFocused.current) { - nextFocused.current = false; + if (nextDisabled && nextFocused) { + setNextFocused(false); state.setFocused(true); } - let previousFocused = useRef(false); + let [previousFocused, setPreviousFocused] = useState(false); let previousDisabled = props.isDisabled || state.isPreviousVisibleRangeInvalid(); - if (previousDisabled && previousFocused.current) { - previousFocused.current = false; + if (previousDisabled && previousFocused) { + setPreviousFocused(false); state.setFocused(true); } @@ -100,15 +100,13 @@ export function useCalendarBase(props: CalendarPropsBase & DOMProps & AriaLabeli onPress: () => state.focusNextPage(), 'aria-label': stringFormatter.format('next'), isDisabled: nextDisabled, - onFocus: () => nextFocused.current = true, - onBlur: () => nextFocused.current = false + onFocusChange: setNextFocused }, prevButtonProps: { onPress: () => state.focusPreviousPage(), 'aria-label': stringFormatter.format('previous'), isDisabled: previousDisabled, - onFocus: () => previousFocused.current = true, - onBlur: () => previousFocused.current = false + onFocusChange: setPreviousFocused }, errorMessageProps: { id: errorMessageId diff --git a/packages/@react-aria/focus/src/useFocusRing.ts b/packages/@react-aria/focus/src/useFocusRing.ts index 6e813ba6294..d0ea4efc528 100644 --- a/packages/@react-aria/focus/src/useFocusRing.ts +++ b/packages/@react-aria/focus/src/useFocusRing.ts @@ -73,7 +73,7 @@ export function useFocusRing(props: AriaFocusRingProps = {}): FocusRingAria { return { isFocused, - isFocusVisible: state.current.isFocused && isFocusVisibleState, + isFocusVisible: isFocusVisibleState, focusProps: within ? focusWithinProps : focusProps }; } diff --git a/packages/@react-aria/i18n/src/useDateFormatter.ts b/packages/@react-aria/i18n/src/useDateFormatter.ts index cbf63e35457..14a3a6643e2 100644 --- a/packages/@react-aria/i18n/src/useDateFormatter.ts +++ b/packages/@react-aria/i18n/src/useDateFormatter.ts @@ -25,6 +25,8 @@ export interface DateFormatterOptions extends Intl.DateTimeFormatOptions { */ export function useDateFormatter(options?: DateFormatterOptions): DateFormatter { // Reuse last options object if it is shallowly equal, which allows the useMemo result to also be reused. + // Using a ref during render is ok here because it's only an optimization โ€“ both values are equivalent. + // If a render is thrown away, it'll still work the same no matter if the next render is the same or not. let lastOptions = useRef(null); if (options && lastOptions.current && isEqual(options, lastOptions.current)) { options = lastOptions.current; diff --git a/packages/@react-aria/interactions/src/utils.ts b/packages/@react-aria/interactions/src/utils.ts index 80e0099223f..3e3d9394006 100644 --- a/packages/@react-aria/interactions/src/utils.ts +++ b/packages/@react-aria/interactions/src/utils.ts @@ -11,7 +11,7 @@ */ import {FocusEvent as ReactFocusEvent, useCallback, useRef} from 'react'; -import {useLayoutEffect} from '@react-aria/utils'; +import {useEffectEvent, useLayoutEffect} from '@react-aria/utils'; export class SyntheticFocusEvent implements ReactFocusEvent { nativeEvent: FocusEvent; @@ -64,10 +64,8 @@ export class SyntheticFocusEvent implements ReactFocusEvent(onBlur: (e: ReactFocusEvent) => void) { let stateRef = useRef({ isFocused: false, - onBlur, observer: null as MutationObserver }); - stateRef.current.onBlur = onBlur; // Clean up MutationObserver on unmount. See below. // eslint-disable-next-line arrow-body-style @@ -81,6 +79,10 @@ export function useSyntheticBlurEvent(onBlur: (e: ReactFocusEv }; }, []); + let dispatchBlur = useEffectEvent((e: SyntheticFocusEvent) => { + onBlur?.(e); + }); + // This function is called during a React onFocus event. return useCallback((e: ReactFocusEvent) => { // React does not fire onBlur when an element is disabled. https://github.com/facebook/react/issues/9142 @@ -101,7 +103,7 @@ export function useSyntheticBlurEvent(onBlur: (e: ReactFocusEv if (target.disabled) { // For backward compatibility, dispatch a (fake) React synthetic event. - stateRef.current.onBlur?.(new SyntheticFocusEvent('blur', e)); + dispatchBlur(new SyntheticFocusEvent('blur', e)); } // We no longer need the MutationObserver once the target is blurred. diff --git a/packages/@react-aria/utils/src/useUpdateEffect.ts b/packages/@react-aria/utils/src/useUpdateEffect.ts index b70d66d42ad..567ec60c6bf 100644 --- a/packages/@react-aria/utils/src/useUpdateEffect.ts +++ b/packages/@react-aria/utils/src/useUpdateEffect.ts @@ -17,6 +17,13 @@ export function useUpdateEffect(effect: EffectCallback, dependencies: any[]) { const isInitialMount = useRef(true); const lastDeps = useRef(null); + useEffect(() => { + isInitialMount.current = true; + return () => { + isInitialMount.current = false; + }; + }, []); + useEffect(() => { if (isInitialMount.current) { isInitialMount.current = false; diff --git a/packages/@react-spectrum/calendar/test/Calendar.test.js b/packages/@react-spectrum/calendar/test/Calendar.test.js index 813c9991316..2b9dccd1e2b 100644 --- a/packages/@react-spectrum/calendar/test/Calendar.test.js +++ b/packages/@react-spectrum/calendar/test/Calendar.test.js @@ -10,6 +10,8 @@ * governing permissions and limitations under the License. */ +import {act} from '@testing-library/react'; + jest.mock('@react-aria/live-announcer'); import {announce} from '@react-aria/live-announcer'; import {Calendar} from '../'; @@ -21,12 +23,11 @@ import {useLocale} from '@react-aria/i18n'; let keyCodes = {'Enter': 13, ' ': 32, 'PageUp': 33, 'PageDown': 34, 'End': 35, 'Home': 36, 'ArrowLeft': 37, 'ArrowUp': 38, 'ArrowRight': 39, 'ArrowDown': 40}; describe('Calendar', () => { - beforeEach(() => { - jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb()); + beforeAll(() => { + jest.useFakeTimers(); }); - afterEach(() => { - window.requestAnimationFrame.mockRestore(); + act(() => {jest.runAllTimers();}); }); describe('basics', () => { @@ -388,13 +389,13 @@ describe('Calendar', () => { }); }); - // These tests only work against v3 describe('announcing', () => { it('announces when the current month changes', () => { let {getAllByLabelText} = render(); let nextButton = getAllByLabelText('Next')[0]; triggerPress(nextButton); + act(() => {jest.runAllTimers();}); expect(announce).toHaveBeenCalledTimes(1); expect(announce).toHaveBeenCalledWith('July 2019'); @@ -405,6 +406,7 @@ describe('Calendar', () => { let newDate = getByText('17'); triggerPress(newDate); + act(() => {jest.runAllTimers();}); expect(announce).toHaveBeenCalledTimes(1); expect(announce).toHaveBeenCalledWith('Selected Date: Monday, June 17, 2019', 'polite', 4000); @@ -418,6 +420,8 @@ describe('Calendar', () => { expect(selectedDate).toHaveFocus(); fireEvent.keyDown(grid, {key: 'ArrowRight'}); + fireEvent.keyUp(document.activeElement, {key: 'ArrowRight'}); + act(() => {jest.runAllTimers();}); expect(getByLabelText('Thursday, June 6, 2019', {exact: false})).toHaveFocus(); }); @@ -426,6 +430,7 @@ describe('Calendar', () => { let newDate = getByText('17'); triggerPress(newDate); + act(() => {jest.runAllTimers();}); expect(announce).toHaveBeenCalledTimes(1); expect(announce).toHaveBeenCalledWith('Selected Date: Saturday, February 17, 5 BC', 'polite', 4000); @@ -433,6 +438,7 @@ describe('Calendar', () => { announce.mockReset(); let nextButton = getAllByLabelText('Next')[0]; triggerPress(nextButton); + act(() => {jest.runAllTimers();}); expect(announce).toHaveBeenCalledTimes(1); expect(announce).toHaveBeenCalledWith('March 5 BC'); diff --git a/packages/@react-spectrum/calendar/test/RangeCalendar.test.js b/packages/@react-spectrum/calendar/test/RangeCalendar.test.js index 0446a2b43c8..c84d22e2653 100644 --- a/packages/@react-spectrum/calendar/test/RangeCalendar.test.js +++ b/packages/@react-spectrum/calendar/test/RangeCalendar.test.js @@ -28,9 +28,12 @@ function type(key) { } describe('RangeCalendar', () => { - beforeEach(() => { + beforeAll(() => { jest.useFakeTimers(); }); + afterEach(() => { + act(() => {jest.runAllTimers();}); + }); describe('basics', () => { it.each` @@ -110,7 +113,6 @@ describe('RangeCalendar', () => { expect(grid).not.toHaveAttribute('aria-activedescendant'); }); - // v2 doesn't pass this test - it starts by showing the end date instead of the start date. it('should show selected dates across multiple months', () => { let {getByRole, getByLabelText, getAllByLabelText, getAllByRole} = render(); diff --git a/packages/@react-stately/calendar/src/useCalendarState.ts b/packages/@react-stately/calendar/src/useCalendarState.ts index fe6aab36958..f1ba53bf660 100644 --- a/packages/@react-stately/calendar/src/useCalendarState.ts +++ b/packages/@react-stately/calendar/src/useCalendarState.ts @@ -112,12 +112,12 @@ export function useCalendarState(props: Calenda }, [startDate, visibleDuration]); // Reset focused date and visible range when calendar changes. - let lastCalendarIdentifier = useRef(calendar.identifier); - if (calendar.identifier !== lastCalendarIdentifier.current) { + let [lastCalendarIdentifier, setLastCalendarIdentifier] = useState(calendar.identifier); + if (calendar.identifier !== lastCalendarIdentifier) { let newFocusedDate = toCalendar(focusedDate, calendar); setStartDate(alignCenter(newFocusedDate, visibleDuration, locale, minValue, maxValue)); setFocusedDate(newFocusedDate); - lastCalendarIdentifier.current = calendar.identifier; + setLastCalendarIdentifier(calendar.identifier); } if (isInvalid(focusedDate, minValue, maxValue)) { diff --git a/packages/@react-stately/calendar/src/useRangeCalendarState.ts b/packages/@react-stately/calendar/src/useRangeCalendarState.ts index 2bf7e8bb549..bf586f867ca 100644 --- a/packages/@react-stately/calendar/src/useRangeCalendarState.ts +++ b/packages/@react-stately/calendar/src/useRangeCalendarState.ts @@ -91,10 +91,10 @@ export function useRangeCalendarState(props: Ra }; // If the visible range changes, we need to update the available range. - let lastVisibleRange = useRef(calendar.visibleRange); - if (!isEqualDay(calendar.visibleRange.start, lastVisibleRange.current.start) || !isEqualDay(calendar.visibleRange.end, lastVisibleRange.current.end)) { + let [lastVisibleRange, setLastVisibleRange] = useState(calendar.visibleRange); + if (!isEqualDay(calendar.visibleRange.start, lastVisibleRange.start) || !isEqualDay(calendar.visibleRange.end, lastVisibleRange.end)) { updateAvailableRange(anchorDate); - lastVisibleRange.current = calendar.visibleRange; + setLastVisibleRange(calendar.visibleRange); } let setAnchorDate = (date: CalendarDate) => { diff --git a/packages/@react-stately/datepicker/src/utils.ts b/packages/@react-stately/datepicker/src/utils.ts index b815dcd1830..dab768eee23 100644 --- a/packages/@react-stately/datepicker/src/utils.ts +++ b/packages/@react-stately/datepicker/src/utils.ts @@ -12,7 +12,7 @@ import {Calendar, now, Time, toCalendar, toCalendarDate, toCalendarDateTime} from '@internationalized/date'; import {DatePickerProps, DateValue, Granularity, TimeValue} from '@react-types/datepicker'; -import {useRef} from 'react'; +import {useRef, useState} from 'react'; export function isInvalid(value: DateValue, minValue: DateValue, maxValue: DateValue) { return value != null && ( @@ -130,12 +130,11 @@ export function createPlaceholderDate(placeholderValue: DateValue, granularity: export function useDefaultProps(v: DateValue, granularity: Granularity): [Granularity, string] { // Compute default granularity and time zone from the value. If the value becomes null, keep the last values. - let lastValue = useRef(v); - if (v) { - lastValue.current = v; + let [lastValue, setLastValue] = useState<[Granularity, string] | null>(null); + if (!v && lastValue) { + return lastValue; } - v = lastValue.current; let defaultTimeZone = (v && 'timeZone' in v ? v.timeZone : undefined); granularity = granularity || (v && 'minute' in v ? 'minute' : 'day'); @@ -144,5 +143,10 @@ export function useDefaultProps(v: DateValue, granularity: Granularity): [Granul throw new Error('Invalid granularity ' + granularity + ' for value ' + v.toString()); } + // If the granularity or time zone changed, update the last value. + if (!lastValue || lastValue[0] !== granularity || lastValue[1] !== defaultTimeZone) { + setLastValue([granularity, defaultTimeZone]); + } + return [granularity, defaultTimeZone]; } From e97e49e9b1d42a4eb3f6d0b67556b65dafe5629e Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 13:42:41 -0700 Subject: [PATCH 25/53] Get rid of refs in render in color area/wheel and sliders --- .../@react-aria/color/src/useColorArea.ts | 57 ++++++++++--------- .../color/src/useColorAreaState.ts | 29 +++++----- .../color/src/useColorWheelState.ts | 15 ++--- .../slider/src/useSliderState.ts | 24 +++++--- 4 files changed, 68 insertions(+), 57 deletions(-) diff --git a/packages/@react-aria/color/src/useColorArea.ts b/packages/@react-aria/color/src/useColorArea.ts index 2f0cf672c32..98d770380a2 100644 --- a/packages/@react-aria/color/src/useColorArea.ts +++ b/packages/@react-aria/color/src/useColorArea.ts @@ -16,7 +16,7 @@ import {DOMAttributes} from '@react-types/shared'; import {focusWithoutScrolling, isAndroid, isIOS, mergeProps, useGlobalListeners, useLabels} from '@react-aria/utils'; // @ts-ignore import intlMessages from '../intl/*.json'; -import React, {ChangeEvent, InputHTMLAttributes, RefObject, useCallback, useRef} from 'react'; +import React, {ChangeEvent, InputHTMLAttributes, RefObject, useCallback, useRef, useState} from 'react'; import {useColorAreaGradient} from './useColorAreaGradient'; import {useFocus, useFocusWithin, useKeyboard, useMove} from '@react-aria/interactions'; import {useLocale, useLocalizedStringFormatter} from '@react-aria/i18n'; @@ -62,13 +62,13 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) let {direction, locale} = useLocale(); - let focusedInputRef = useRef(null); - + let [focusedInput, setFocusedInput] = useState<'x' | 'y' | null>(null); let focusInput = useCallback((inputRef:RefObject = inputXRef) => { if (inputRef.current) { focusWithoutScrolling(inputRef.current); } }, [inputXRef]); + let [valueChangedViaKeyboard, setValueChangedViaKeyboard] = useState(false); let {xChannel, yChannel, zChannel} = state.channels; let xChannelStep = state.xChannelStep; @@ -87,28 +87,31 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) e.preventDefault(); // remember to set this and unset it so that onChangeEnd is fired state.setDragging(true); - valueChangedViaKeyboard.current = true; + setValueChangedViaKeyboard(true); + let dir; switch (e.key) { case 'PageUp': state.incrementY(state.yChannelPageStep); - focusedInputRef.current = inputYRef.current; + dir = 'y'; break; case 'PageDown': state.decrementY(state.yChannelPageStep); - focusedInputRef.current = inputYRef.current; + dir = 'y'; break; case 'Home': direction === 'rtl' ? state.incrementX(state.xChannelPageStep) : state.decrementX(state.xChannelPageStep); - focusedInputRef.current = inputXRef.current; + dir = 'x'; break; case 'End': direction === 'rtl' ? state.decrementX(state.xChannelPageStep) : state.incrementX(state.xChannelPageStep); - focusedInputRef.current = inputXRef.current; + dir = 'x'; break; } state.setDragging(false); - if (focusedInputRef.current) { - focusInput(focusedInputRef.current ? focusedInputRef : inputXRef); + if (dir) { + let input = dir === 'x' ? inputXRef : inputYRef; + focusInput(input); + setFocusedInput(dir); } } }); @@ -148,9 +151,10 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) } else if (deltaY < 0) { incrementY(deltaYValue); } - valueChangedViaKeyboard.current = valueChanged; + setValueChangedViaKeyboard(valueChanged); // set the focused input based on which axis has the greater delta - focusedInputRef.current = valueChanged && Math.abs(deltaY) > Math.abs(deltaX) ? inputYRef.current : inputXRef.current; + focusedInput = valueChanged && Math.abs(deltaY) > Math.abs(deltaX) ? 'y' : 'x'; + setFocusedInput(focusedInput); } else { currentPosition.current.x += (direction === 'rtl' ? -1 : 1) * deltaX / width ; currentPosition.current.y += deltaY / height; @@ -160,17 +164,16 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) onMoveEnd() { isOnColorArea.current = undefined; state.setDragging(false); - focusInput(focusedInputRef.current ? focusedInputRef : inputXRef); + let input = focusedInput === 'x' ? inputXRef : inputYRef; + focusInput(input); } }; let {moveProps: movePropsThumb} = useMove(moveHandler); - let valueChangedViaKeyboard = useRef(false); let {focusWithinProps} = useFocusWithin({ onFocusWithinChange: (focusWithin:boolean) => { if (!focusWithin) { - valueChangedViaKeyboard.current = false; - focusedInputRef.current === undefined; + setValueChangedViaKeyboard(false); } } }); @@ -198,7 +201,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) let onThumbDown = (id: number | null) => { if (!state.isDragging) { currentPointer.current = id; - valueChangedViaKeyboard.current = false; + setValueChangedViaKeyboard(false); focusInput(); state.setDragging(true); if (typeof PointerEvent !== 'undefined') { @@ -213,7 +216,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) let onThumbUp = (e) => { let id = e.pointerId ?? e.changedTouches?.[0].identifier; if (id === currentPointer.current) { - valueChangedViaKeyboard.current = false; + setValueChangedViaKeyboard(false); focusInput(); state.setDragging(false); currentPointer.current = undefined; @@ -238,7 +241,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) } if (x >= 0 && x <= 1 && y >= 0 && y <= 1 && !state.isDragging && currentPointer.current === undefined) { isOnColorArea.current = true; - valueChangedViaKeyboard.current = false; + setValueChangedViaKeyboard(false); currentPointer.current = id; state.setColorFromPoint(x, y); @@ -258,7 +261,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) let id = e.pointerId ?? e.changedTouches?.[0].identifier; if (isOnColorArea.current && id === currentPointer.current) { isOnColorArea.current = false; - valueChangedViaKeyboard.current = false; + setValueChangedViaKeyboard(false); currentPointer.current = undefined; state.setDragging(false); focusInput(); @@ -314,13 +317,13 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) let {focusProps: xInputFocusProps} = useFocus({ onFocus: () => { - focusedInputRef.current = inputXRef.current; + setFocusedInput('x'); } }); let {focusProps: yInputFocusProps} = useFocus({ onFocus: () => { - focusedInputRef.current = inputYRef.current; + setFocusedInput('y'); } }); @@ -328,7 +331,7 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) function getAriaValueTextForChannel(channel:ColorChannel) { return ( - valueChangedViaKeyboard.current ? + valueChangedViaKeyboard ? stringFormatter.format('colorNameAndValue', {name: state.value.getChannelName(channel, locale), value: state.value.formatChannelValue(channel, locale)}) : [ @@ -407,13 +410,13 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) 'aria-valuetext': getAriaValueTextForChannel(xChannel), disabled: isDisabled, value: state.value.getChannelValue(xChannel), - tabIndex: (isMobile || !focusedInputRef.current || focusedInputRef.current === inputXRef.current ? undefined : -1), + tabIndex: (isMobile || !focusedInput || focusedInput === 'x' ? undefined : -1), /* So that only a single "2d slider" control shows up when listing form elements for screen readers, add aria-hidden="true" to the unfocused control when the value has not changed via the keyboard, but remove aria-hidden to reveal the input for each channel when the value has changed with the keyboard. */ - 'aria-hidden': (isMobile || !focusedInputRef.current || focusedInputRef.current === inputXRef.current || valueChangedViaKeyboard.current ? undefined : 'true'), + 'aria-hidden': (isMobile || !focusedInput || focusedInput === 'x' || valueChangedViaKeyboard ? undefined : 'true'), onChange: (e: ChangeEvent) => { state.setXValue(parseFloat(e.target.value)); } @@ -431,13 +434,13 @@ export function useColorArea(props: AriaColorAreaOptions, state: ColorAreaState) 'aria-orientation': 'vertical', disabled: isDisabled, value: state.value.getChannelValue(yChannel), - tabIndex: (isMobile || (focusedInputRef.current && focusedInputRef.current === inputYRef.current) ? undefined : -1), + tabIndex: (isMobile || focusedInput === 'y' ? undefined : -1), /* So that only a single "2d slider" control shows up when listing form elements for screen readers, add aria-hidden="true" to the unfocused input when the value has not changed via the keyboard, but remove aria-hidden to reveal the input for each channel when the value has changed with the keyboard. */ - 'aria-hidden': (isMobile || (focusedInputRef.current && focusedInputRef.current === inputYRef.current) || valueChangedViaKeyboard.current ? undefined : 'true'), + 'aria-hidden': (isMobile || focusedInput === 'y' || valueChangedViaKeyboard ? undefined : 'true'), onChange: (e: ChangeEvent) => { state.setYValue(parseFloat(e.target.value)); } diff --git a/packages/@react-stately/color/src/useColorAreaState.ts b/packages/@react-stately/color/src/useColorAreaState.ts index fe704376f54..b402983f85b 100644 --- a/packages/@react-stately/color/src/useColorAreaState.ts +++ b/packages/@react-stately/color/src/useColorAreaState.ts @@ -85,13 +85,16 @@ export function useColorAreaState(props: ColorAreaProps): ColorAreaState { defaultValue = DEFAULT_COLOR; } - let [color, setColor] = useControlledState(value && normalizeColor(value), defaultValue && normalizeColor(defaultValue), onChange); + let [color, setColorState] = useControlledState(value && normalizeColor(value), defaultValue && normalizeColor(defaultValue), onChange); let valueRef = useRef(color); - valueRef.current = color; + let setColor = (color: Color) => { + valueRef.current = color; + setColorState(color); + }; let channels = useMemo(() => - valueRef.current.getColorSpaceAxes({xChannel, yChannel}), - [xChannel, yChannel] + color.getColorSpaceAxes({xChannel, yChannel}), + [color, xChannel, yChannel] ); let xChannelRange = color.getChannelRange(channels.xChannel); @@ -100,7 +103,7 @@ export function useColorAreaState(props: ColorAreaProps): ColorAreaState { let {minValue: minValueY, maxValue: maxValueY, step: stepY, pageSize: pageSizeY} = yChannelRange; let [isDragging, setDragging] = useState(false); - let isDraggingRef = useRef(false).current; + let isDraggingRef = useRef(false); let xValue = color.getChannelValue(channels.xChannel); let yValue = color.getChannelValue(channels.yChannel); @@ -108,15 +111,15 @@ export function useColorAreaState(props: ColorAreaProps): ColorAreaState { if (v === xValue) { return; } - valueRef.current = color.withChannelValue(channels.xChannel, v); - setColor(valueRef.current); + let newColor = color.withChannelValue(channels.xChannel, v); + setColor(newColor); }; let setYValue = (v: number) => { if (v === yValue) { return; } - valueRef.current = color.withChannelValue(channels.yChannel, v); - setColor(valueRef.current); + let newColor = color.withChannelValue(channels.yChannel, v); + setColor(newColor); }; return { @@ -127,9 +130,7 @@ export function useColorAreaState(props: ColorAreaProps): ColorAreaState { yChannelPageStep: pageSizeY, value: color, setValue(value) { - let c = normalizeColor(value); - valueRef.current = c; - setColor(c); + setColor(normalizeColor(value)); }, xValue, setXValue, @@ -171,8 +172,8 @@ export function useColorAreaState(props: ColorAreaProps): ColorAreaState { setYValue(snapValueToStep(yValue - stepSize, minValueY, maxValueY, stepY)); }, setDragging(isDragging) { - let wasDragging = isDraggingRef; - isDraggingRef = isDragging; + let wasDragging = isDraggingRef.current; + isDraggingRef.current = isDragging; if (onChangeEnd && !isDragging && wasDragging) { onChangeEnd(valueRef.current); diff --git a/packages/@react-stately/color/src/useColorWheelState.ts b/packages/@react-stately/color/src/useColorWheelState.ts index 566c8a1117f..23036fe8a38 100644 --- a/packages/@react-stately/color/src/useColorWheelState.ts +++ b/packages/@react-stately/color/src/useColorWheelState.ts @@ -99,14 +99,17 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { defaultValue = DEFAULT_COLOR; } - let [value, setValue] = useControlledState(normalizeColor(props.value), normalizeColor(defaultValue), onChange); + let [value, setValueState] = useControlledState(normalizeColor(props.value), normalizeColor(defaultValue), onChange); let valueRef = useRef(value); - valueRef.current = value; + let setValue = (value: Color) => { + valueRef.current = value; + setValueState(value); + }; let channelRange = value.getChannelRange('hue'); let {minValue: minValueX, maxValue: maxValueX, step: step, pageSize: pageStep} = channelRange; let [isDragging, setDragging] = useState(false); - let isDraggingRef = useRef(false).current; + let isDraggingRef = useRef(false); let hue = value.getChannelValue('hue'); function setHue(v: number) { @@ -117,7 +120,6 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { v = roundToStep(mod(v, 360), step); if (hue !== v) { let color = value.withChannelValue('hue', v); - valueRef.current = color; setValue(color); } } @@ -128,7 +130,6 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { pageStep, setValue(v) { let color = normalizeColor(v); - valueRef.current = color; setValue(color); }, hue, @@ -159,8 +160,8 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { } }, setDragging(isDragging) { - let wasDragging = isDraggingRef; - isDraggingRef = isDragging; + let wasDragging = isDraggingRef.current; + isDraggingRef.current = isDragging; if (onChangeEnd && !isDragging && wasDragging) { onChangeEnd(valueRef.current); diff --git a/packages/@react-stately/slider/src/useSliderState.ts b/packages/@react-stately/slider/src/useSliderState.ts index d23e4c9407a..801759db3cc 100644 --- a/packages/@react-stately/slider/src/useSliderState.ts +++ b/packages/@react-stately/slider/src/useSliderState.ts @@ -182,20 +182,26 @@ export function useSliderState(props: SliderStateOp let onChange = createOnChange(props.value, props.defaultValue, props.onChange); let onChangeEnd = createOnChange(props.value, props.defaultValue, props.onChangeEnd); - const [values, setValues] = useControlledState( + const [values, setValuesState] = useControlledState( value, defaultValue, onChange ); - const [isDraggings, setDraggings] = useState(new Array(values.length).fill(false)); + const [isDraggings, setDraggingsState] = useState(new Array(values.length).fill(false)); const isEditablesRef = useRef(new Array(values.length).fill(true)); const [focusedIndex, setFocusedIndex] = useState(undefined); - // These refs should be ok as long as we never read them during render. - const valuesRef = useRef(null); - valuesRef.current = values; - const isDraggingsRef = useRef(null); - isDraggingsRef.current = isDraggings; + const valuesRef = useRef(values); + const isDraggingsRef = useRef(isDraggings); + let setValues = (values: number[]) => { + valuesRef.current = values; + setValuesState(values); + }; + + let setDraggings = (draggings: boolean[]) => { + isDraggingsRef.current = draggings; + setDraggingsState(draggings); + }; function getValuePercent(value: number) { return (value - minValue) / (maxValue - minValue); @@ -225,8 +231,8 @@ export function useSliderState(props: SliderStateOp // Round value to multiple of step, clamp value between min and max value = snapValueToStep(value, thisMin, thisMax, step); - valuesRef.current = replaceIndex(valuesRef.current, index, value); - setValues(valuesRef.current); + let newValues = replaceIndex(values, index, value); + setValues(newValues); } function updateDragging(index: number, dragging: boolean) { From f6e9d71b4940a1fbb632a4a3bfdf6e82f87e827a Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 13:57:53 -0700 Subject: [PATCH 26/53] Move useClipboard to useEffectEvent --- packages/@react-aria/dnd/src/useClipboard.ts | 97 +++++++++----------- 1 file changed, 44 insertions(+), 53 deletions(-) diff --git a/packages/@react-aria/dnd/src/useClipboard.ts b/packages/@react-aria/dnd/src/useClipboard.ts index 8c9fb4989e7..bb437bde7f9 100644 --- a/packages/@react-aria/dnd/src/useClipboard.ts +++ b/packages/@react-aria/dnd/src/useClipboard.ts @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {chain} from '@react-aria/utils'; +import {chain, useEffectEvent} from '@react-aria/utils'; import {DOMAttributes, DragItem, DropItem} from '@react-types/shared'; import {readFromDataTransfer, writeToDataTransfer} from './utils'; import {useEffect, useRef} from 'react'; @@ -64,9 +64,6 @@ function addGlobalEventListener(event, fn) { * data types, and integrates with the operating system native clipboard. */ export function useClipboard(options: ClipboardProps): ClipboardResult { - let ref = useRef(options); - ref.current = options; - let isFocusedRef = useRef(false); let {focusProps} = useFocus({ onFocusChange: (isFocused) => { @@ -74,64 +71,58 @@ export function useClipboard(options: ClipboardProps): ClipboardResult { } }); - useEffect(() => { - let onBeforeCopy = (e: ClipboardEvent) => { - // Enable the "Copy" menu item in Safari if this element is focused and copying is supported. - let options = ref.current; - if (isFocusedRef.current && options.getItems) { - e.preventDefault(); - } - }; - - let onCopy = (e: ClipboardEvent) => { - let options = ref.current; - if (!isFocusedRef.current || !options.getItems) { - return; - } - + let onBeforeCopy = useEffectEvent((e: ClipboardEvent) => { + // Enable the "Copy" menu item in Safari if this element is focused and copying is supported. + if (isFocusedRef.current && options.getItems) { e.preventDefault(); - writeToDataTransfer(e.clipboardData, options.getItems()); - options.onCopy?.(); - }; + } + }); - let onBeforeCut = (e: ClipboardEvent) => { - let options = ref.current; - if (isFocusedRef.current && options.onCut && options.getItems) { - e.preventDefault(); - } - }; + let onCopy = useEffectEvent((e: ClipboardEvent) => { + if (!isFocusedRef.current || !options.getItems) { + return; + } - let onCut = (e: ClipboardEvent) => { - let options = ref.current; - if (!isFocusedRef.current || !options.onCut || !options.getItems) { - return; - } + e.preventDefault(); + writeToDataTransfer(e.clipboardData, options.getItems()); + options.onCopy?.(); + }); + let onBeforeCut = useEffectEvent((e: ClipboardEvent) => { + if (isFocusedRef.current && options.onCut && options.getItems) { e.preventDefault(); - writeToDataTransfer(e.clipboardData, options.getItems()); - options.onCut(); - }; + } + }); - let onBeforePaste = (e: ClipboardEvent) => { - let options = ref.current; - // Unfortunately, e.clipboardData.types is not available in this event so we always - // have to enable the Paste menu item even if the type of data is unsupported. - if (isFocusedRef.current && options.onPaste) { - e.preventDefault(); - } - }; + let onCut = useEffectEvent((e: ClipboardEvent) => { + if (!isFocusedRef.current || !options.onCut || !options.getItems) { + return; + } - let onPaste = (e: ClipboardEvent) => { - let options = ref.current; - if (!isFocusedRef.current || !options.onPaste) { - return; - } + e.preventDefault(); + writeToDataTransfer(e.clipboardData, options.getItems()); + options.onCut(); + }); + let onBeforePaste = useEffectEvent((e: ClipboardEvent) => { + // Unfortunately, e.clipboardData.types is not available in this event so we always + // have to enable the Paste menu item even if the type of data is unsupported. + if (isFocusedRef.current && options.onPaste) { e.preventDefault(); - let items = readFromDataTransfer(e.clipboardData); - options.onPaste(items); - }; + } + }); + + let onPaste = useEffectEvent((e: ClipboardEvent) => { + if (!isFocusedRef.current || !options.onPaste) { + return; + } + e.preventDefault(); + let items = readFromDataTransfer(e.clipboardData); + options.onPaste(items); + }); + + useEffect(() => { return chain( addGlobalEventListener('beforecopy', onBeforeCopy), addGlobalEventListener('copy', onCopy), @@ -140,7 +131,7 @@ export function useClipboard(options: ClipboardProps): ClipboardResult { addGlobalEventListener('beforepaste', onBeforePaste), addGlobalEventListener('paste', onPaste) ); - }, []); + }, [onBeforeCopy, onCopy, onBeforeCut, onCut, onBeforePaste, onPaste]); return { clipboardProps: focusProps From 38afc92e8fc14406992da504c49971b68dbc4a33 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 13:58:09 -0700 Subject: [PATCH 27/53] Move useEvent to useEffectEvent --- packages/@react-aria/utils/src/useEvent.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/@react-aria/utils/src/useEvent.ts b/packages/@react-aria/utils/src/useEvent.ts index f7c1a6564f3..9985045e619 100644 --- a/packages/@react-aria/utils/src/useEvent.ts +++ b/packages/@react-aria/utils/src/useEvent.ts @@ -10,7 +10,8 @@ * governing permissions and limitations under the License. */ -import {RefObject, useEffect, useRef} from 'react'; +import {RefObject, useEffect} from 'react'; +import {useEffectEvent} from './useEffectEvent'; export function useEvent( ref: RefObject, @@ -18,9 +19,7 @@ export function useEvent( handler: (this: Document, ev: GlobalEventHandlersEventMap[K]) => any, options?: boolean | AddEventListenerOptions ) { - let handlerRef = useRef(handler); - handlerRef.current = handler; - + let handleEvent = useEffectEvent(handler); let isDisabled = handler == null; useEffect(() => { @@ -29,11 +28,9 @@ export function useEvent( } let element = ref.current; - let handler = (e: GlobalEventHandlersEventMap[K]) => handlerRef.current.call(this, e); - - element.addEventListener(event, handler, options); + element.addEventListener(event, handleEvent, options); return () => { - element.removeEventListener(event, handler, options); + element.removeEventListener(event, handleEvent, options); }; - }, [ref, event, options, isDisabled]); + }, [ref, event, options, isDisabled, handleEvent]); } From cdf4461df707ce7095d007b12c780e00541515fb Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 13:59:30 -0700 Subject: [PATCH 28/53] Move useTabListState auto selection to a useEffect Doing this in render was actually wrong anyway because if you had the tabs in a controlled state, it would fire onSelectionChange during render which would result in react erroring about updating a different component while another was rendering. Doing it in useEffect is the right time. --- .../tabs/src/useTabListState.ts | 44 ++++++++++--------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/packages/@react-stately/tabs/src/useTabListState.ts b/packages/@react-stately/tabs/src/useTabListState.ts index 8d8c6eca49b..281a1ac59b5 100644 --- a/packages/@react-stately/tabs/src/useTabListState.ts +++ b/packages/@react-stately/tabs/src/useTabListState.ts @@ -13,7 +13,7 @@ import {CollectionStateBase} from '@react-types/shared'; import {SingleSelectListState, useSingleSelectListState} from '@react-stately/list'; import {TabListProps} from '@react-types/tabs'; -import {useRef} from 'react'; +import {useEffect, useRef} from 'react'; export interface TabListStateOptions extends Omit, 'children'>, CollectionStateBase {} @@ -39,30 +39,32 @@ export function useTabListState(props: TabListStateOptions) } = state; let lastSelectedKey = useRef(currentSelectedKey); - // Ensure a tab is always selected (in case no selected key was specified or if selected item was deleted from collection) - let selectedKey = currentSelectedKey; - if (selectionManager.isEmpty || !collection.getItem(selectedKey)) { - selectedKey = collection.getFirstKey(); - // loop over tabs until we find one that isn't disabled and select that - while (state.disabledKeys.has(selectedKey) && selectedKey !== collection.getLastKey()) { - selectedKey = collection.getKeyAfter(selectedKey); - } - // if this check is true, then every item is disabled, it makes more sense to default to the first key than the last - if (state.disabledKeys.has(selectedKey) && selectedKey === collection.getLastKey()) { + useEffect(() => { + // Ensure a tab is always selected (in case no selected key was specified or if selected item was deleted from collection) + let selectedKey = currentSelectedKey; + if (selectionManager.isEmpty || !collection.getItem(selectedKey)) { selectedKey = collection.getFirstKey(); - } + // loop over tabs until we find one that isn't disabled and select that + while (state.disabledKeys.has(selectedKey) && selectedKey !== collection.getLastKey()) { + selectedKey = collection.getKeyAfter(selectedKey); + } + // if this check is true, then every item is disabled, it makes more sense to default to the first key than the last + if (state.disabledKeys.has(selectedKey) && selectedKey === collection.getLastKey()) { + selectedKey = collection.getFirstKey(); + } - if (selectedKey != null) { - // directly set selection because replace/toggle selection won't consider disabled keys - selectionManager.setSelectedKeys([selectedKey]); + if (selectedKey != null) { + // directly set selection because replace/toggle selection won't consider disabled keys + selectionManager.setSelectedKeys([selectedKey]); + } } - } - // If the tablist doesn't have focus and the selected key changes or if there isn't a focused key yet, change focused key to the selected key if it exists. - if (selectedKey != null && selectionManager.focusedKey == null || (!selectionManager.isFocused && selectedKey !== lastSelectedKey.current)) { - selectionManager.setFocusedKey(selectedKey); - } - lastSelectedKey.current = selectedKey; + // If the tablist doesn't have focus and the selected key changes or if there isn't a focused key yet, change focused key to the selected key if it exists. + if (selectedKey != null && selectionManager.focusedKey == null || (!selectionManager.isFocused && selectedKey !== lastSelectedKey.current)) { + selectionManager.setFocusedKey(selectedKey); + } + lastSelectedKey.current = selectedKey; + }); return { ...state, From c158e6fdaa1c67738a971b0dc39e1781b2006248 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 14:03:43 -0700 Subject: [PATCH 29/53] Fix table resizing ref in render --- .../table/src/useTableColumnResize.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/@react-aria/table/src/useTableColumnResize.ts b/packages/@react-aria/table/src/useTableColumnResize.ts index 5fe36baf96d..6567a6fe57a 100644 --- a/packages/@react-aria/table/src/useTableColumnResize.ts +++ b/packages/@react-aria/table/src/useTableColumnResize.ts @@ -65,7 +65,8 @@ export function useTableColumnResize(props: AriaTableColumnResizeProps, st let {column: item, triggerRef, isDisabled, onResizeStart, onResize, onResizeEnd, 'aria-label': ariaLabel} = props; const stringFormatter = useLocalizedStringFormatter(intlMessages); let id = useId(); - let isResizing = useRef(false); + let isResizing = state.resizingColumn === item.key; + let isResizingRef = useRef(isResizing); let lastSize = useRef(null); let editModeEnabled = state.tableState.isKeyboardNavigationDisabled; @@ -97,13 +98,13 @@ export function useTableColumnResize(props: AriaTableColumnResizeProps, st }); let startResize = useCallback((item) => { - if (!isResizing.current) { + if (!isResizingRef.current) { lastSize.current = state.updateResizedColumns(item.key, state.getColumnWidth(item.key)); state.startResize(item.key); onResizeStart?.(lastSize.current); } - isResizing.current = true; - }, [isResizing, onResizeStart, state]); + isResizingRef.current = true; + }, [isResizingRef, onResizeStart, state]); let resize = useCallback((item, newWidth) => { let sizes = state.updateResizedColumns(item.key, newWidth); @@ -112,7 +113,7 @@ export function useTableColumnResize(props: AriaTableColumnResizeProps, st }, [onResize, state]); let endResize = useCallback((item) => { - if (isResizing.current) { + if (isResizingRef.current) { if (lastSize.current == null) { lastSize.current = state.updateResizedColumns(item.key, state.getColumnWidth(item.key)); } @@ -120,9 +121,9 @@ export function useTableColumnResize(props: AriaTableColumnResizeProps, st state.endResize(); onResizeEnd?.(lastSize.current); } - isResizing.current = false; + isResizingRef.current = false; lastSize.current = null; - }, [isResizing, onResizeEnd, state]); + }, [isResizingRef, onResizeEnd, state]); const columnResizeWidthRef = useRef(0); const {moveProps} = useMove({ @@ -174,7 +175,7 @@ export function useTableColumnResize(props: AriaTableColumnResizeProps, st if (modality === 'virtual' && (typeof window !== 'undefined' && 'ontouchstart' in window)) { modality = 'touch'; } - let description = triggerRef?.current == null && (modality === 'keyboard' || modality === 'virtual') && !isResizing.current ? stringFormatter.format('resizerDescription') : undefined; + let description = triggerRef?.current == null && (modality === 'keyboard' || modality === 'virtual') && !isResizing ? stringFormatter.format('resizerDescription') : undefined; let descriptionProps = useDescription(description); let ariaProps = { 'aria-label': ariaLabel, @@ -267,6 +268,6 @@ export function useTableColumnResize(props: AriaTableColumnResizeProps, st }, ariaProps ), - isResizing: state.resizingColumn === item.key + isResizing }; } From 994b84da1c04c0a1b48aaf0629fc36cf49b87a57 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 14:06:46 -0700 Subject: [PATCH 30/53] Fix actiongroup From #4005 --- .../actiongroup/src/useActionGroupItem.ts | 18 +++++----- .../actiongroup/test/ActionGroup.test.js | 35 +++++++++++++++++++ 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/packages/@react-aria/actiongroup/src/useActionGroupItem.ts b/packages/@react-aria/actiongroup/src/useActionGroupItem.ts index 89b73c6259e..4c999cc8803 100644 --- a/packages/@react-aria/actiongroup/src/useActionGroupItem.ts +++ b/packages/@react-aria/actiongroup/src/useActionGroupItem.ts @@ -11,9 +11,9 @@ */ import {DOMAttributes, FocusableElement} from '@react-types/shared'; -import {Key, RefObject, useEffect, useRef} from 'react'; +import {Key, RefObject, useEffect} from 'react'; import {ListState} from '@react-stately/list'; -import {mergeProps} from '@react-aria/utils'; +import {mergeProps, useEffectEvent} from '@react-aria/utils'; import {PressProps} from '@react-aria/interactions'; export interface AriaActionGroupItemProps { @@ -43,18 +43,18 @@ export function useActionGroupItem(props: AriaActionGroupItemProps, state: Li } let isFocused = props.key === state.selectionManager.focusedKey; - let lastRender = useRef({isFocused, state}); - lastRender.current = {isFocused, state}; + let onRemovedWithFocus = useEffectEvent(() => { + if (isFocused) { + state.selectionManager.setFocusedKey(null); + } + }); // If the focused item is removed from the DOM, reset the focused key to null. - // eslint-disable-next-line arrow-body-style useEffect(() => { return () => { - if (lastRender.current.isFocused) { - lastRender.current.state.selectionManager.setFocusedKey(null); - } + onRemovedWithFocus(); }; - }, []); + }, [onRemovedWithFocus]); return { buttonProps: mergeProps(buttonProps, { diff --git a/packages/@react-spectrum/actiongroup/test/ActionGroup.test.js b/packages/@react-spectrum/actiongroup/test/ActionGroup.test.js index 7697edc9c25..2d5a405d0c5 100644 --- a/packages/@react-spectrum/actiongroup/test/ActionGroup.test.js +++ b/packages/@react-spectrum/actiongroup/test/ActionGroup.test.js @@ -795,6 +795,41 @@ describe('ActionGroup', function () { expect(buttons[1]).toHaveAttribute('tabIndex', '0'); }); + it('moves focus if the focused button was removed', function () { + let onAction = jest.fn(); + let tree = render( + + + One + Two + Three + Four + + + ); + + let actiongroup = tree.getByRole('toolbar'); + let buttons = within(actiongroup).getAllByRole('button'); + expect(buttons[0]).toHaveAttribute('tabIndex', '0'); + expect(buttons[1]).toHaveAttribute('tabIndex', '0'); + + act(() => buttons[2].focus()); + tree.rerender( + + + One + Two + Four + + + ); + actiongroup = tree.getByRole('toolbar'); + buttons = within(actiongroup).getAllByRole('button'); + expect(buttons[0]).toHaveAttribute('tabIndex', '0'); + expect(buttons[1]).toHaveAttribute('tabIndex', '0'); + expect(buttons[2]).toHaveAttribute('tabIndex', '0'); + }); + it('passes aria labeling props through to menu button if it is the only child', function () { jest.spyOn(HTMLElement.prototype, 'offsetWidth', 'get').mockImplementation(function () { if (this instanceof HTMLButtonElement) { From 405f8b3ac04fa8936a5e5737baa9079d211c2168 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 14:19:42 -0700 Subject: [PATCH 31/53] Centralize useDeepMemo hook We have a few places where we do a deep equal check as an optimization so that a later useMemo returns cached values more often. This fails the linter but in this case it is actually safe. This centralizes that logic in one place. --- .../calendar/src/useCalendarCell.ts | 10 ++----- .../@react-aria/i18n/src/useDateFormatter.ts | 13 +++------ packages/@react-aria/utils/src/index.ts | 1 + packages/@react-aria/utils/src/useDeepMemo.ts | 27 +++++++++++++++++++ 4 files changed, 33 insertions(+), 18 deletions(-) create mode 100644 packages/@react-aria/utils/src/useDeepMemo.ts diff --git a/packages/@react-aria/calendar/src/useCalendarCell.ts b/packages/@react-aria/calendar/src/useCalendarCell.ts index 6d93e064bf3..70eab28d02d 100644 --- a/packages/@react-aria/calendar/src/useCalendarCell.ts +++ b/packages/@react-aria/calendar/src/useCalendarCell.ts @@ -13,7 +13,7 @@ import {CalendarDate, isEqualDay, isSameDay, isToday} from '@internationalized/date'; import {CalendarState, RangeCalendarState} from '@react-stately/calendar'; import {DOMAttributes} from '@react-types/shared'; -import {focusWithoutScrolling, getScrollParent, scrollIntoViewport, useDescription} from '@react-aria/utils'; +import {focusWithoutScrolling, getScrollParent, scrollIntoViewport, useDeepMemo, useDescription} from '@react-aria/utils'; import {getEraFormat, hookData} from './utils'; import {getInteractionModality, usePress} from '@react-aria/interactions'; // @ts-ignore @@ -102,13 +102,7 @@ export function useCalendarCell(props: AriaCalendarCellProps, state: CalendarSta // For performance, reuse the same date object as before if the new date prop is the same. // This allows subsequent useMemo results to be reused. - let lastDate = useRef(null); - if (lastDate.current && isEqualDay(date, lastDate.current)) { - date = lastDate.current; - } - - lastDate.current = date; - + date = useDeepMemo(date, isEqualDay); let nativeDate = useMemo(() => date.toDate(state.timeZone), [date, state.timeZone]); // aria-label should be localize Day of week, Month, Day and Year without Time. diff --git a/packages/@react-aria/i18n/src/useDateFormatter.ts b/packages/@react-aria/i18n/src/useDateFormatter.ts index 14a3a6643e2..8b062f4b699 100644 --- a/packages/@react-aria/i18n/src/useDateFormatter.ts +++ b/packages/@react-aria/i18n/src/useDateFormatter.ts @@ -11,8 +11,9 @@ */ import {DateFormatter} from '@internationalized/date'; +import {useDeepMemo} from '@react-aria/utils'; import {useLocale} from './context'; -import {useMemo, useRef} from 'react'; +import {useMemo} from 'react'; export interface DateFormatterOptions extends Intl.DateTimeFormatOptions { calendar?: string @@ -25,15 +26,7 @@ export interface DateFormatterOptions extends Intl.DateTimeFormatOptions { */ export function useDateFormatter(options?: DateFormatterOptions): DateFormatter { // Reuse last options object if it is shallowly equal, which allows the useMemo result to also be reused. - // Using a ref during render is ok here because it's only an optimization โ€“ both values are equivalent. - // If a render is thrown away, it'll still work the same no matter if the next render is the same or not. - let lastOptions = useRef(null); - if (options && lastOptions.current && isEqual(options, lastOptions.current)) { - options = lastOptions.current; - } - - lastOptions.current = options; - + options = useDeepMemo(options, isEqual); let {locale} = useLocale(); return useMemo(() => new DateFormatter(locale, options), [locale, options]); } diff --git a/packages/@react-aria/utils/src/index.ts b/packages/@react-aria/utils/src/index.ts index 362fae8c6f5..d63b4b1f4e0 100644 --- a/packages/@react-aria/utils/src/index.ts +++ b/packages/@react-aria/utils/src/index.ts @@ -35,3 +35,4 @@ export {scrollIntoView, scrollIntoViewport} from './scrollIntoView'; export {clamp, snapValueToStep} from '@react-stately/utils'; export {isVirtualClick, isVirtualPointerEvent} from './isVirtualEvent'; export {useEffectEvent} from './useEffectEvent'; +export {useDeepMemo} from './useDeepMemo'; diff --git a/packages/@react-aria/utils/src/useDeepMemo.ts b/packages/@react-aria/utils/src/useDeepMemo.ts new file mode 100644 index 00000000000..a861330728f --- /dev/null +++ b/packages/@react-aria/utils/src/useDeepMemo.ts @@ -0,0 +1,27 @@ +/* + * Copyright 2023 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +// eslint-disable rulesdir/pure-render + +import {useRef} from 'react'; + +export function useDeepMemo(value: T, isEqual: (a: T, b: T) => boolean): T { + // Using a ref during render is ok here because it's only an optimization โ€“ both values are equivalent. + // If a render is thrown away, it'll still work the same no matter if the next render is the same or not. + let lastValue = useRef(null); + if (value && lastValue.current && isEqual(value, lastValue.current)) { + value = lastValue.current; + } + + lastValue.current = value; + return value; +} From 5d01282378505ab41f8ed2a0fadf0de4d9d28d67 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 14:26:51 -0700 Subject: [PATCH 32/53] Refactor usePress to useEffectEvent --- .../@react-aria/interactions/src/usePress.ts | 215 +++++++++--------- 1 file changed, 112 insertions(+), 103 deletions(-) diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts index 52c7017da40..20056d50996 100644 --- a/packages/@react-aria/interactions/src/usePress.ts +++ b/packages/@react-aria/interactions/src/usePress.ts @@ -17,7 +17,7 @@ import {disableTextSelection, restoreTextSelection} from './textSelection'; import {DOMAttributes, FocusableElement, PointerType, PressEvents} from '@react-types/shared'; -import {focusWithoutScrolling, isVirtualClick, isVirtualPointerEvent, mergeProps, useGlobalListeners, useSyncRef} from '@react-aria/utils'; +import {focusWithoutScrolling, isVirtualClick, isVirtualPointerEvent, mergeProps, useEffectEvent, useGlobalListeners, useSyncRef} from '@react-aria/utils'; import {PressResponderContext} from './context'; import {RefObject, useContext, useEffect, useMemo, useRef, useState} from 'react'; @@ -105,8 +105,6 @@ export function usePress(props: PressHookProps): PressResult { ref: _, // Removing `ref` from `domProps` because TypeScript is dumb ...domProps } = usePressResponderContext(props); - let propsRef = useRef(null); - propsRef.current = {onPress, onPressChange, onPressStart, onPressEnd, onPressUp, isDisabled, shouldCancelOnPointerExit}; let [isPressed, setPressed] = useState(false); let ref = useRef({ @@ -122,109 +120,115 @@ export function usePress(props: PressHookProps): PressResult { let {addGlobalListener, removeAllGlobalListeners} = useGlobalListeners(); - let pressProps = useMemo(() => { + let triggerPressStart = useEffectEvent((originalEvent: EventBase, pointerType: PointerType) => { let state = ref.current; - let triggerPressStart = (originalEvent: EventBase, pointerType: PointerType) => { - let {onPressStart, onPressChange, isDisabled} = propsRef.current; - if (isDisabled || state.didFirePressStart) { - return; - } + if (isDisabled || state.didFirePressStart) { + return; + } - if (onPressStart) { - onPressStart({ - type: 'pressstart', - pointerType, - target: originalEvent.currentTarget as Element, - shiftKey: originalEvent.shiftKey, - metaKey: originalEvent.metaKey, - ctrlKey: originalEvent.ctrlKey, - altKey: originalEvent.altKey - }); - } + if (onPressStart) { + onPressStart({ + type: 'pressstart', + pointerType, + target: originalEvent.currentTarget as Element, + shiftKey: originalEvent.shiftKey, + metaKey: originalEvent.metaKey, + ctrlKey: originalEvent.ctrlKey, + altKey: originalEvent.altKey + }); + } - if (onPressChange) { - onPressChange(true); - } + if (onPressChange) { + onPressChange(true); + } - state.didFirePressStart = true; - setPressed(true); - }; + state.didFirePressStart = true; + setPressed(true); + }); - let triggerPressEnd = (originalEvent: EventBase, pointerType: PointerType, wasPressed = true) => { - let {onPressEnd, onPressChange, onPress, isDisabled} = propsRef.current; - if (!state.didFirePressStart) { - return; - } + let triggerPressEnd = useEffectEvent((originalEvent: EventBase, pointerType: PointerType, wasPressed = true) => { + let state = ref.current; + if (!state.didFirePressStart) { + return; + } - state.ignoreClickAfterPress = true; - state.didFirePressStart = false; - - if (onPressEnd) { - onPressEnd({ - type: 'pressend', - pointerType, - target: originalEvent.currentTarget as Element, - shiftKey: originalEvent.shiftKey, - metaKey: originalEvent.metaKey, - ctrlKey: originalEvent.ctrlKey, - altKey: originalEvent.altKey - }); - } + state.ignoreClickAfterPress = true; + state.didFirePressStart = false; + + if (onPressEnd) { + onPressEnd({ + type: 'pressend', + pointerType, + target: originalEvent.currentTarget as Element, + shiftKey: originalEvent.shiftKey, + metaKey: originalEvent.metaKey, + ctrlKey: originalEvent.ctrlKey, + altKey: originalEvent.altKey + }); + } - if (onPressChange) { - onPressChange(false); - } + if (onPressChange) { + onPressChange(false); + } - setPressed(false); - - if (onPress && wasPressed && !isDisabled) { - onPress({ - type: 'press', - pointerType, - target: originalEvent.currentTarget as Element, - shiftKey: originalEvent.shiftKey, - metaKey: originalEvent.metaKey, - ctrlKey: originalEvent.ctrlKey, - altKey: originalEvent.altKey - }); - } - }; + setPressed(false); + + if (onPress && wasPressed && !isDisabled) { + onPress({ + type: 'press', + pointerType, + target: originalEvent.currentTarget as Element, + shiftKey: originalEvent.shiftKey, + metaKey: originalEvent.metaKey, + ctrlKey: originalEvent.ctrlKey, + altKey: originalEvent.altKey + }); + } + }); - let triggerPressUp = (originalEvent: EventBase, pointerType: PointerType) => { - let {onPressUp, isDisabled} = propsRef.current; - if (isDisabled) { - return; - } + let triggerPressUp = useEffectEvent((originalEvent: EventBase, pointerType: PointerType) => { + if (isDisabled) { + return; + } - if (onPressUp) { - onPressUp({ - type: 'pressup', - pointerType, - target: originalEvent.currentTarget as Element, - shiftKey: originalEvent.shiftKey, - metaKey: originalEvent.metaKey, - ctrlKey: originalEvent.ctrlKey, - altKey: originalEvent.altKey - }); - } - }; + if (onPressUp) { + onPressUp({ + type: 'pressup', + pointerType, + target: originalEvent.currentTarget as Element, + shiftKey: originalEvent.shiftKey, + metaKey: originalEvent.metaKey, + ctrlKey: originalEvent.ctrlKey, + altKey: originalEvent.altKey + }); + } + }); - let cancel = (e: EventBase) => { - if (state.isPressed) { - if (state.isOverTarget) { - triggerPressEnd(createEvent(state.target, e), state.pointerType, false); - } - state.isPressed = false; - state.isOverTarget = false; - state.activePointerId = null; - state.pointerType = null; - removeAllGlobalListeners(); - if (!allowTextSelectionOnPress) { - restoreTextSelection(state.target); - } + let cancel = useEffectEvent((e: EventBase) => { + let state = ref.current; + if (state.isPressed) { + if (state.isOverTarget) { + triggerPressEnd(createEvent(state.target, e), state.pointerType, false); } - }; + state.isPressed = false; + state.isOverTarget = false; + state.activePointerId = null; + state.pointerType = null; + removeAllGlobalListeners(); + if (!allowTextSelectionOnPress) { + restoreTextSelection(state.target); + } + } + }); + let cancelOnPointerExit = useEffectEvent((e: EventBase) => { + if (shouldCancelOnPointerExit) { + cancel(e); + } + }); + + let pressProps = useMemo(() => { + let state = ref.current; let pressProps: DOMAttributes = { onKeyDown(e) { if (isValidKeyboardEvent(e.nativeEvent, e.currentTarget) && e.currentTarget.contains(e.target as Element)) { @@ -401,9 +405,7 @@ export function usePress(props: PressHookProps): PressResult { } else if (state.isOverTarget) { state.isOverTarget = false; triggerPressEnd(createEvent(state.target, e), state.pointerType, false); - if (propsRef.current.shouldCancelOnPointerExit) { - cancel(e); - } + cancelOnPointerExit(e); } }; @@ -491,9 +493,7 @@ export function usePress(props: PressHookProps): PressResult { if (state.isPressed && !state.ignoreEmulatedMouseEvents) { state.isOverTarget = false; triggerPressEnd(e, state.pointerType, false); - if (propsRef.current.shouldCancelOnPointerExit) { - cancel(e); - } + cancelOnPointerExit(e); } }; @@ -581,9 +581,7 @@ export function usePress(props: PressHookProps): PressResult { } else if (state.isOverTarget) { state.isOverTarget = false; triggerPressEnd(e, state.pointerType, false); - if (propsRef.current.shouldCancelOnPointerExit) { - cancel(e); - } + cancelOnPointerExit(e); } }; @@ -648,7 +646,18 @@ export function usePress(props: PressHookProps): PressResult { } return pressProps; - }, [addGlobalListener, isDisabled, preventFocusOnPress, removeAllGlobalListeners, allowTextSelectionOnPress]); + }, [ + addGlobalListener, + isDisabled, + preventFocusOnPress, + removeAllGlobalListeners, + allowTextSelectionOnPress, + cancel, + cancelOnPointerExit, + triggerPressEnd, + triggerPressStart, + triggerPressUp + ]); // Remove user-select: none in case component unmounts immediately after pressStart // eslint-disable-next-line arrow-body-style From 285ebca12b5bd38d7e888499a7ba1285753219c3 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 14:39:11 -0700 Subject: [PATCH 33/53] Convert useInteractOutside to useEffectEvent --- .../interactions/src/useInteractOutside.ts | 47 ++++++++++--------- .../@react-aria/interactions/src/utils.ts | 2 +- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index f2f3ee1e6e3..2adb3cc81d2 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -16,6 +16,7 @@ // See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions import {RefObject, SyntheticEvent, useEffect, useRef} from 'react'; +import {useEffectEvent} from '@react-aria/utils'; export interface InteractOutsideProps { ref: RefObject, @@ -33,33 +34,35 @@ export function useInteractOutside(props: InteractOutsideProps) { let {ref, onInteractOutside, isDisabled, onInteractOutsideStart} = props; let stateRef = useRef({ isPointerDown: false, - ignoreEmulatedMouseEvents: false, - onInteractOutside, - onInteractOutsideStart + ignoreEmulatedMouseEvents: false + }); + + let onPointerDown = useEffectEvent((e: SyntheticEvent) => { + if (onInteractOutside && isValidEvent(e, ref)) { + if (onInteractOutsideStart) { + onInteractOutsideStart(e); + } + stateRef.current.isPointerDown = true; + } + }); + + let triggerInteractOutside = useEffectEvent((e: SyntheticEvent) => { + if (onInteractOutside) { + onInteractOutside(e); + } }); - let state = stateRef.current; - state.onInteractOutside = onInteractOutside; - state.onInteractOutsideStart = onInteractOutsideStart; useEffect(() => { + let state = stateRef.current; if (isDisabled) { return; } - let onPointerDown = (e) => { - if (isValidEvent(e, ref) && state.onInteractOutside) { - if (state.onInteractOutsideStart) { - state.onInteractOutsideStart(e); - } - state.isPointerDown = true; - } - }; - // Use pointer events if available. Otherwise, fall back to mouse and touch events. if (typeof PointerEvent !== 'undefined') { let onPointerUp = (e) => { - if (state.isPointerDown && state.onInteractOutside && isValidEvent(e, ref)) { - state.onInteractOutside(e); + if (state.isPointerDown && isValidEvent(e, ref)) { + triggerInteractOutside(e); } state.isPointerDown = false; }; @@ -76,16 +79,16 @@ export function useInteractOutside(props: InteractOutsideProps) { let onMouseUp = (e) => { if (state.ignoreEmulatedMouseEvents) { state.ignoreEmulatedMouseEvents = false; - } else if (state.isPointerDown && state.onInteractOutside && isValidEvent(e, ref)) { - state.onInteractOutside(e); + } else if (state.isPointerDown && isValidEvent(e, ref)) { + triggerInteractOutside(e); } state.isPointerDown = false; }; let onTouchEnd = (e) => { state.ignoreEmulatedMouseEvents = true; - if (state.onInteractOutside && state.isPointerDown && isValidEvent(e, ref)) { - state.onInteractOutside(e); + if (state.isPointerDown && isValidEvent(e, ref)) { + triggerInteractOutside(e); } state.isPointerDown = false; }; @@ -102,7 +105,7 @@ export function useInteractOutside(props: InteractOutsideProps) { document.removeEventListener('touchend', onTouchEnd, true); }; } - }, [ref, state, isDisabled]); + }, [ref, isDisabled, onPointerDown, triggerInteractOutside]); } function isValidEvent(event, ref) { diff --git a/packages/@react-aria/interactions/src/utils.ts b/packages/@react-aria/interactions/src/utils.ts index 3e3d9394006..a6fc4a5a138 100644 --- a/packages/@react-aria/interactions/src/utils.ts +++ b/packages/@react-aria/interactions/src/utils.ts @@ -125,5 +125,5 @@ export function useSyntheticBlurEvent(onBlur: (e: ReactFocusEv stateRef.current.observer.observe(target, {attributes: true, attributeFilter: ['disabled']}); } - }, []); + }, [dispatchBlur]); } From 87f0b330a36be5b5b558ed19dfab4af21cc00c75 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 14:54:59 -0700 Subject: [PATCH 34/53] Fix dnd refs in render --- packages/@react-aria/dnd/src/useDrag.ts | 6 +-- packages/@react-aria/dnd/src/useDrop.ts | 62 ++++++++++++++++--------- 2 files changed, 43 insertions(+), 25 deletions(-) diff --git a/packages/@react-aria/dnd/src/useDrag.ts b/packages/@react-aria/dnd/src/useDrag.ts index bbf18742a67..5df92387b0b 100644 --- a/packages/@react-aria/dnd/src/useDrag.ts +++ b/packages/@react-aria/dnd/src/useDrag.ts @@ -79,7 +79,7 @@ export function useDrag(options: DragOptions): DragResult { }).current; state.options = options; let isDraggingRef = useRef(false); - let [, setDraggingState] = useState(false); + let [isDragging, setDraggingState] = useState(false); let setDragging = (isDragging) => { isDraggingRef.current = isDragging; setDraggingState(isDragging); @@ -265,7 +265,7 @@ export function useDrag(options: DragOptions): DragResult { }; let modality = useDragModality(); - let message = !isDraggingRef.current ? MESSAGES[modality].start : MESSAGES[modality].end; + let message = !isDragging ? MESSAGES[modality].start : MESSAGES[modality].end; let descriptionProps = useDescription(stringFormatter.format(message)); @@ -338,6 +338,6 @@ export function useDrag(options: DragOptions): DragResult { ...descriptionProps, onPress }, - isDragging: isDraggingRef.current + isDragging }; } diff --git a/packages/@react-aria/dnd/src/useDrop.ts b/packages/@react-aria/dnd/src/useDrop.ts index 4c2dbf97a18..fd928dac565 100644 --- a/packages/@react-aria/dnd/src/useDrop.ts +++ b/packages/@react-aria/dnd/src/useDrop.ts @@ -15,7 +15,7 @@ import * as DragManager from './DragManager'; import {DragTypes, globalAllowedDropOperations, globalDndState, readFromDataTransfer, setGlobalDnDState, setGlobalDropEffect} from './utils'; import {DROP_EFFECT_TO_DROP_OPERATION, DROP_OPERATION, DROP_OPERATION_ALLOWED, DROP_OPERATION_TO_DROP_EFFECT} from './constants'; import {DropActivateEvent, DropEnterEvent, DropEvent, DropExitEvent, DropMoveEvent, DropOperation, DragTypes as IDragTypes} from '@react-types/shared'; -import {isIPad, isMac, useLayoutEffect} from '@react-aria/utils'; +import {isIPad, isMac, useEffectEvent, useLayoutEffect} from '@react-aria/utils'; import {useVirtualDrop} from './useVirtualDrop'; export interface DropOptions { @@ -268,35 +268,53 @@ export function useDrop(options: DropOptions): DropResult { } }; - let optionsRef = useRef(options); - optionsRef.current = options; + let onDropEnter = useEffectEvent((e: DropEnterEvent) => { + if (typeof options.onDropEnter === 'function') { + options.onDropEnter(e); + } + }); + + let onDropExit = useEffectEvent((e: DropExitEvent) => { + if (typeof options.onDropExit === 'function') { + options.onDropExit(e); + } + }); + + let onDropActivate = useEffectEvent((e: DropActivateEvent) => { + if (typeof options.onDropActivate === 'function') { + options.onDropActivate(e); + } + }); + + let onKeyboardDrop = useEffectEvent((e: DropEvent) => { + if (typeof options.onDrop === 'function') { + options.onDrop(e); + } + }); + let getDropOperationKeyboard = useEffectEvent((types: IDragTypes, allowedOperations: DropOperation[]) => { + if (options.getDropOperation) { + return options.getDropOperation(types, allowedOperations); + } + + return allowedOperations[0]; + }); + + let {ref} = options; useLayoutEffect(() => DragManager.registerDropTarget({ - element: optionsRef.current.ref.current, - getDropOperation: optionsRef.current.getDropOperation, + element: ref.current, + getDropOperation: getDropOperationKeyboard, onDropEnter(e) { setDropTarget(true); - if (typeof optionsRef.current.onDropEnter === 'function') { - optionsRef.current.onDropEnter(e); - } + onDropEnter(e); }, onDropExit(e) { setDropTarget(false); - if (typeof optionsRef.current.onDropExit === 'function') { - optionsRef.current.onDropExit(e); - } + onDropExit(e); }, - onDrop(e) { - if (typeof optionsRef.current.onDrop === 'function') { - optionsRef.current.onDrop(e); - } - }, - onDropActivate(e) { - if (typeof optionsRef.current.onDropActivate === 'function') { - optionsRef.current.onDropActivate(e); - } - } - }), [optionsRef]); + onDrop: onKeyboardDrop, + onDropActivate + }), [ref, getDropOperationKeyboard, onDropEnter, onDropExit, onKeyboardDrop, onDropActivate]); let {dropProps} = useVirtualDrop(); From 4d4b9406ebbf5e614771a66c4a9f49070f19cabd Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 15:23:56 -0700 Subject: [PATCH 35/53] Fix refs in ActionBar and DialogContainer --- .../actionbar/src/ActionBar.tsx | 18 +++++++++++------- .../dialog/src/DialogContainer.tsx | 16 ++++++++-------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/packages/@react-spectrum/actionbar/src/ActionBar.tsx b/packages/@react-spectrum/actionbar/src/ActionBar.tsx index cc9966ef4ca..c10622729a6 100644 --- a/packages/@react-spectrum/actionbar/src/ActionBar.tsx +++ b/packages/@react-spectrum/actionbar/src/ActionBar.tsx @@ -21,7 +21,7 @@ import {FocusScope} from '@react-aria/focus'; // @ts-ignore import intlMessages from '../intl/*.json'; import {OpenTransition} from '@react-spectrum/overlays'; -import React, {ReactElement, Ref, useEffect, useRef} from 'react'; +import React, {ReactElement, Ref, useEffect, useRef, useState} from 'react'; import {SpectrumActionBarProps} from '@react-types/actionbar'; import styles from './actionbar.css'; import {Text} from '@react-spectrum/text'; @@ -65,9 +65,9 @@ function ActionBarInner(props: ActionBarInnerProps, ref: Ref 0) { - lastCount.current = selectedItemCount; + let [lastCount, setLastCount] = useState(selectedItemCount); + if ((selectedItemCount === 'all' || selectedItemCount > 0) && selectedItemCount !== lastCount) { + setLastCount(selectedItemCount); } let {keyboardProps} = useKeyboard({ @@ -80,8 +80,12 @@ function ActionBarInner(props: ActionBarInnerProps, ref: Ref { - announce(stringFormatter.format('actionsAvailable')); + if (isInitial.current) { + isInitial.current = false; + announce(stringFormatter.format('actionsAvailable')); + } }, [stringFormatter]); return ( @@ -120,9 +124,9 @@ function ActionBarInner(props: ActionBarInnerProps, ref: Ref - {lastCount.current === 'all' + {lastCount === 'all' ? stringFormatter.format('selectedAll') - : stringFormatter.format('selected', {count: lastCount.current})} + : stringFormatter.format('selected', {count: lastCount})}
diff --git a/packages/@react-spectrum/dialog/src/DialogContainer.tsx b/packages/@react-spectrum/dialog/src/DialogContainer.tsx index 3e3b3fe535e..e7f26ee5001 100644 --- a/packages/@react-spectrum/dialog/src/DialogContainer.tsx +++ b/packages/@react-spectrum/dialog/src/DialogContainer.tsx @@ -12,7 +12,7 @@ import {DialogContext} from './context'; import {Modal} from '@react-spectrum/overlays'; -import React, {ReactElement, useRef} from 'react'; +import React, {ReactElement, useState} from 'react'; import {SpectrumDialogContainerProps} from '@react-types/dialog'; import {useOverlayTriggerState} from '@react-stately/overlays'; @@ -30,15 +30,15 @@ export function DialogContainer(props: SpectrumDialogContainerProps) { isKeyboardDismissDisabled } = props; - let childArray = React.Children.toArray(children); - if (childArray.length > 1) { + let childCount = React.Children.count(children); + if (childCount > 1) { throw new Error('Only a single child can be passed to DialogContainer.'); } - let lastChild = useRef(null); - let child = React.isValidElement(childArray[0]) ? childArray[0] : null; - if (child) { - lastChild.current = child; + let [lastChild, setLastChild] = useState(null); + let child = React.isValidElement(children) ? children : null; + if (child && child !== lastChild) { + setLastChild(child); } let context = { @@ -63,7 +63,7 @@ export function DialogContainer(props: SpectrumDialogContainerProps) { isDismissable={isDismissable} isKeyboardDismissDisabled={isKeyboardDismissDisabled}> - {lastChild.current} + {lastChild} ); From aff5e71475d3ef1966b421f3778d37a54d2f9bb6 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 15:25:57 -0700 Subject: [PATCH 36/53] lint --- packages/@react-aria/calendar/src/useCalendarBase.ts | 2 +- packages/@react-aria/spinbutton/src/useSpinButton.ts | 6 +++--- packages/@react-aria/ssr/src/SSRProvider.tsx | 3 +++ packages/@react-aria/textfield/src/useFormattedTextField.ts | 2 +- packages/@react-aria/utils/src/useDeepMemo.ts | 2 +- packages/@react-aria/utils/src/useDrag1D.ts | 2 ++ packages/@react-aria/virtualizer/src/Virtualizer.tsx | 4 +++- packages/@react-aria/virtualizer/src/VirtualizerItem.tsx | 2 +- packages/@react-aria/virtualizer/src/useVirtualizerItem.ts | 4 ++-- packages/@react-spectrum/list/src/ListView.tsx | 2 ++ packages/@react-spectrum/table/src/TableView.tsx | 2 +- packages/@react-stately/calendar/src/useCalendarState.ts | 2 +- packages/@react-stately/datepicker/src/utils.ts | 2 +- packages/react-aria-components/src/GridList.tsx | 2 ++ packages/react-aria-components/src/ListBox.tsx | 2 ++ packages/react-aria-components/src/Table.tsx | 2 ++ 16 files changed, 28 insertions(+), 13 deletions(-) diff --git a/packages/@react-aria/calendar/src/useCalendarBase.ts b/packages/@react-aria/calendar/src/useCalendarBase.ts index 2cb5c9a97b2..87a11314b55 100644 --- a/packages/@react-aria/calendar/src/useCalendarBase.ts +++ b/packages/@react-aria/calendar/src/useCalendarBase.ts @@ -21,7 +21,7 @@ import {hookData, useSelectedDateDescription, useVisibleRangeDescription} from ' // @ts-ignore import intlMessages from '../intl/*.json'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; -import {useRef, useState} from 'react'; +import {useState} from 'react'; export interface CalendarAria { /** Props for the calendar grouping element. */ diff --git a/packages/@react-aria/spinbutton/src/useSpinButton.ts b/packages/@react-aria/spinbutton/src/useSpinButton.ts index 955b4a9b0c2..e7245c791c6 100644 --- a/packages/@react-aria/spinbutton/src/useSpinButton.ts +++ b/packages/@react-aria/spinbutton/src/useSpinButton.ts @@ -15,7 +15,7 @@ import {AriaButtonProps} from '@react-types/button'; import {DOMAttributes, InputBase, RangeInputBase, Validation, ValueBase} from '@react-types/shared'; // @ts-ignore import intlMessages from '../intl/*.json'; -import {useCallback, useEffect, useRef} from 'react'; +import {useEffect, useRef} from 'react'; import {useEffectEvent, useGlobalListeners} from '@react-aria/utils'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; @@ -148,7 +148,7 @@ export function useSpinButton( }, initialStepDelay ); - }, + } ); const onDecrementPressStart = useEffectEvent( @@ -164,7 +164,7 @@ export function useSpinButton( }, initialStepDelay ); - }, + } ); let cancelContextMenu = (e) => { diff --git a/packages/@react-aria/ssr/src/SSRProvider.tsx b/packages/@react-aria/ssr/src/SSRProvider.tsx index 920811b5c3c..38c4e9857c2 100644 --- a/packages/@react-aria/ssr/src/SSRProvider.tsx +++ b/packages/@react-aria/ssr/src/SSRProvider.tsx @@ -90,6 +90,7 @@ let componentIds = new WeakMap(); function useCounter(isDisabled = false) { let ctx = useContext(SSRContext); let ref = useRef(null); + // eslint-disable-next-line rulesdir/pure-render if (ref.current === null && !isDisabled) { // In strict mode, React renders components twice, and the ref will be reset to null on the second render. // This means our id counter will be incremented twice instead of once. This is a problem because on the @@ -119,9 +120,11 @@ function useCounter(isDisabled = false) { } } + // eslint-disable-next-line rulesdir/pure-render ref.current = ++ctx.current; } + // eslint-disable-next-line rulesdir/pure-render return ref.current; } diff --git a/packages/@react-aria/textfield/src/useFormattedTextField.ts b/packages/@react-aria/textfield/src/useFormattedTextField.ts index f12903db1c7..d4cb134f126 100644 --- a/packages/@react-aria/textfield/src/useFormattedTextField.ts +++ b/packages/@react-aria/textfield/src/useFormattedTextField.ts @@ -98,7 +98,7 @@ export function useFormattedTextField(props: AriaTextFieldProps, state: Formatte return () => { input.removeEventListener('beforeinput', onBeforeInputFallback, false); }; - }, [onBeforeInputFallback]); + }, [inputRef, onBeforeInputFallback]); let onBeforeInput = !supportsNativeBeforeInputEvent() ? e => { diff --git a/packages/@react-aria/utils/src/useDeepMemo.ts b/packages/@react-aria/utils/src/useDeepMemo.ts index a861330728f..504a6a80d4f 100644 --- a/packages/@react-aria/utils/src/useDeepMemo.ts +++ b/packages/@react-aria/utils/src/useDeepMemo.ts @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -// eslint-disable rulesdir/pure-render +/* eslint-disable rulesdir/pure-render */ import {useRef} from 'react'; diff --git a/packages/@react-aria/utils/src/useDrag1D.ts b/packages/@react-aria/utils/src/useDrag1D.ts index 618cdf0c61e..e907128c9b3 100644 --- a/packages/@react-aria/utils/src/useDrag1D.ts +++ b/packages/@react-aria/utils/src/useDrag1D.ts @@ -10,6 +10,8 @@ * governing permissions and limitations under the License. */ + /* eslint-disable rulesdir/pure-render */ + import {getOffset} from './getOffset'; import {Orientation} from '@react-types/shared'; import React, {HTMLAttributes, MutableRefObject, useRef} from 'react'; diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 92bbaed2475..590638e965f 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -48,7 +48,9 @@ function Virtualizer(props: VirtualizerProps, ref: Re sizeToFit, scrollDirection, transitionDuration, + // eslint-disable-next-line @typescript-eslint/no-unused-vars isLoading, + // eslint-disable-next-line @typescript-eslint/no-unused-vars onLoadMore, // eslint-disable-next-line @typescript-eslint/no-unused-vars focusedKey, @@ -98,7 +100,7 @@ interface VirtualizerOptions { shouldUseVirtualFocus?: boolean, autoFocus?: boolean, isLoading?: boolean, - onLoadMore?: () => void, + onLoadMore?: () => void } export function useVirtualizer(props: VirtualizerOptions, state: VirtualizerState, ref: RefObject) { diff --git a/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx b/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx index 8dbc228a4e6..066f918ee88 100644 --- a/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx +++ b/packages/@react-aria/virtualizer/src/VirtualizerItem.tsx @@ -11,7 +11,7 @@ */ import {Direction} from '@react-types/shared'; -import {LayoutInfo, ReusableView} from '@react-stately/virtualizer'; +import {LayoutInfo} from '@react-stately/virtualizer'; import React, {CSSProperties, ReactNode, useRef} from 'react'; import {useLocale} from '@react-aria/i18n'; import {useVirtualizerItem, VirtualizerItemOptions} from './useVirtualizerItem'; diff --git a/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts b/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts index c4132ef3bdc..848027f0b0b 100644 --- a/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts +++ b/packages/@react-aria/virtualizer/src/useVirtualizerItem.ts @@ -11,7 +11,7 @@ */ import {Key, RefObject, useCallback} from 'react'; -import {LayoutInfo, ReusableView, Size} from '@react-stately/virtualizer'; +import {LayoutInfo, Size} from '@react-stately/virtualizer'; import {useLayoutEffect} from '@react-aria/utils'; interface IVirtualizer { @@ -20,7 +20,7 @@ interface IVirtualizer { export interface VirtualizerItemOptions { layoutInfo: LayoutInfo, - virtualizer: IVirtualizer + virtualizer: IVirtualizer, ref: RefObject } diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index b8fbc4d0556..ebead8885ef 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -124,9 +124,11 @@ function ListView(props: SpectrumListViewProps, ref: DOMRef let isListDroppable = !!dragAndDropHooks?.useDroppableCollectionState; let dragHooksProvided = useRef(isListDraggable); let dropHooksProvided = useRef(isListDroppable); + // eslint-disable-next-line rulesdir/pure-render if (dragHooksProvided.current !== isListDraggable) { console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); } + // eslint-disable-next-line rulesdir/pure-render if (dropHooksProvided.current !== isListDroppable) { console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); } diff --git a/packages/@react-spectrum/table/src/TableView.tsx b/packages/@react-spectrum/table/src/TableView.tsx index d61d046d790..a6472af3e3f 100644 --- a/packages/@react-spectrum/table/src/TableView.tsx +++ b/packages/@react-spectrum/table/src/TableView.tsx @@ -39,8 +39,8 @@ import ListGripper from '@spectrum-icons/ui/ListGripper'; import {Nubbin} from './Nubbin'; import {ProgressCircle} from '@react-spectrum/progress'; import React, {DOMAttributes, HTMLAttributes, Key, ReactElement, useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react'; -import {Rect, ReusableView, useVirtualizerState} from '@react-stately/virtualizer'; import {Resizer} from './Resizer'; +import {ReusableView, useVirtualizerState} from '@react-stately/virtualizer'; import {RootDropIndicator} from './RootDropIndicator'; import {DragPreview as SpectrumDragPreview} from './DragPreview'; import styles from '@adobe/spectrum-css-temp/components/table/vars.css'; diff --git a/packages/@react-stately/calendar/src/useCalendarState.ts b/packages/@react-stately/calendar/src/useCalendarState.ts index f1ba53bf660..f22310898ae 100644 --- a/packages/@react-stately/calendar/src/useCalendarState.ts +++ b/packages/@react-stately/calendar/src/useCalendarState.ts @@ -30,7 +30,7 @@ import { import {CalendarProps, DateValue} from '@react-types/calendar'; import {CalendarState} from './types'; import {useControlledState} from '@react-stately/utils'; -import {useMemo, useRef, useState} from 'react'; +import {useMemo, useState} from 'react'; export interface CalendarStateOptions extends CalendarProps { /** The locale to display and edit the value according to. */ diff --git a/packages/@react-stately/datepicker/src/utils.ts b/packages/@react-stately/datepicker/src/utils.ts index dab768eee23..0cea2c7d670 100644 --- a/packages/@react-stately/datepicker/src/utils.ts +++ b/packages/@react-stately/datepicker/src/utils.ts @@ -12,7 +12,7 @@ import {Calendar, now, Time, toCalendar, toCalendarDate, toCalendarDateTime} from '@internationalized/date'; import {DatePickerProps, DateValue, Granularity, TimeValue} from '@react-types/datepicker'; -import {useRef, useState} from 'react'; +import {useState} from 'react'; export function isInvalid(value: DateValue, minValue: DateValue, maxValue: DateValue) { return value != null && ( diff --git a/packages/react-aria-components/src/GridList.tsx b/packages/react-aria-components/src/GridList.tsx index 615a57b7a22..3dd4257c422 100644 --- a/packages/react-aria-components/src/GridList.tsx +++ b/packages/react-aria-components/src/GridList.tsx @@ -91,9 +91,11 @@ function GridList(props: GridListProps, ref: ForwardedRef({state, props, listBoxRef}: ListBoxInnerProps) { let dragHooksProvided = useRef(isListDraggable); let dropHooksProvided = useRef(isListDroppable); + // eslint-disable-next-line rulesdir/pure-render if (dragHooksProvided.current !== isListDraggable) { console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); } + // eslint-disable-next-line rulesdir/pure-render if (dropHooksProvided.current !== isListDroppable) { console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); } diff --git a/packages/react-aria-components/src/Table.tsx b/packages/react-aria-components/src/Table.tsx index da440821e21..6d97481d865 100644 --- a/packages/react-aria-components/src/Table.tsx +++ b/packages/react-aria-components/src/Table.tsx @@ -243,9 +243,11 @@ function Table(props: TableProps, ref: ForwardedRef) { let isListDroppable = !!dragAndDropHooks?.useDroppableCollectionState; let dragHooksProvided = useRef(isListDraggable); let dropHooksProvided = useRef(isListDroppable); + // eslint-disable-next-line rulesdir/pure-render if (dragHooksProvided.current !== isListDraggable) { console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); } + // eslint-disable-next-line rulesdir/pure-render if (dropHooksProvided.current !== isListDroppable) { console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); } From 3aea9bab34dad8713d275be2044540a9bb4af2db Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 15:26:45 -0700 Subject: [PATCH 37/53] Fix ref in RAC animations --- packages/react-aria-components/src/utils.tsx | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/react-aria-components/src/utils.tsx b/packages/react-aria-components/src/utils.tsx index c837bf1c98e..34d24980abd 100644 --- a/packages/react-aria-components/src/utils.tsx +++ b/packages/react-aria-components/src/utils.tsx @@ -196,25 +196,25 @@ export function useExitAnimation(ref: RefObject, isOpen: boolean) { // State to trigger a re-render after animation is complete, which causes the element to be removed from the DOM. // Ref to track the state we're in, so we don't immediately reset isExiting to true after the animation. let [isExiting, setExiting] = useState(false); - let exitState = useRef('idle'); + let [exitState, setExitState] = useState('idle'); // If isOpen becomes false, set isExiting to true. - if (!isOpen && ref.current && exitState.current === 'idle') { + if (!isOpen && ref.current && exitState === 'idle') { isExiting = true; setExiting(true); - exitState.current = 'exiting'; + setExitState('exiting'); } // If we exited, and the element has been removed, reset exit state to idle. - if (!ref.current && exitState.current === 'exited') { - exitState.current = 'idle'; + if (!ref.current && exitState === 'exited') { + setExitState('idle'); } useAnimation( ref, isExiting, useCallback(() => { - exitState.current = 'exited'; + setExitState('exited'); setExiting(false); }, []) ); @@ -225,6 +225,10 @@ export function useExitAnimation(ref: RefObject, isOpen: boolean) { function useAnimation(ref: RefObject, isActive: boolean, onEnd: () => void) { let prevAnimation = useRef(null); if (isActive && ref.current) { + // This is ok because we only read it in the layout effect below, immediately after the commit phase. + // We could move this to another effect that runs every render, but this would be unnecessarily slow. + // We only need the computed style right before the animation becomes active. + // eslint-disable-next-line rulesdir/pure-render prevAnimation.current = window.getComputedStyle(ref.current).animation; } From 886084e826c1c5be5b817c553348a29e6a9ad2f1 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 15:36:17 -0700 Subject: [PATCH 38/53] Fix SideNav --- packages/@react-spectrum/sidenav/src/SideNav.tsx | 6 ++++-- .../@react-spectrum/sidenav/src/SideNavSection.tsx | 10 +++++----- packages/@react-types/sidenav/src/index.d.ts | 14 ++++++++++---- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/packages/@react-spectrum/sidenav/src/SideNav.tsx b/packages/@react-spectrum/sidenav/src/SideNav.tsx index da992e6eb20..bdec5286ff0 100644 --- a/packages/@react-spectrum/sidenav/src/SideNav.tsx +++ b/packages/@react-spectrum/sidenav/src/SideNav.tsx @@ -44,8 +44,10 @@ export function SideNav(props: SpectrumSideNavProps) { return ( c.viewType === 'header')}> + item={reusableView.content} + layoutInfo={reusableView.layoutInfo} + virtualizer={reusableView.virtualizer} + headerLayoutInfo={children.find(c => c.viewType === 'header').layoutInfo}> {renderChildren(children.filter(c => c.viewType === 'item'))} ); diff --git a/packages/@react-spectrum/sidenav/src/SideNavSection.tsx b/packages/@react-spectrum/sidenav/src/SideNavSection.tsx index e993a1e5226..70599676fb4 100644 --- a/packages/@react-spectrum/sidenav/src/SideNavSection.tsx +++ b/packages/@react-spectrum/sidenav/src/SideNavSection.tsx @@ -19,8 +19,7 @@ import {useListBoxSection} from '@react-aria/listbox'; import {useLocale} from '@react-aria/i18n'; export function SideNavSection(props: SideNavSectionProps) { - let {children, reusableView, header} = props; - let item = reusableView.content; + let {children, layoutInfo, headerLayoutInfo, virtualizer, item} = props; let {headingProps, groupProps} = useListBoxSection({ heading: item.rendered, 'aria-label': item['aria-label'] @@ -28,7 +27,8 @@ export function SideNavSection(props: SideNavSectionProps) { let headerRef = useRef(); useVirtualizerItem({ - reusableView: header, + layoutInfo: headerLayoutInfo, + virtualizer, ref: headerRef }); @@ -36,7 +36,7 @@ export function SideNavSection(props: SideNavSectionProps) { return ( -
+
{item.rendered &&
(props: SideNavSectionProps) {
+ style={layoutInfoToStyle(layoutInfo, direction)}> {children}
diff --git a/packages/@react-types/sidenav/src/index.d.ts b/packages/@react-types/sidenav/src/index.d.ts index 97a7017ad4c..a9dfc3c8388 100644 --- a/packages/@react-types/sidenav/src/index.d.ts +++ b/packages/@react-types/sidenav/src/index.d.ts @@ -11,8 +11,8 @@ */ import {AriaLabelingProps, CollectionBase, DOMProps, Expandable, MultipleSelection, Node, StyleProps} from '@react-types/shared'; -import {HTMLAttributes, ReactNode} from 'react'; -import {ReusableView} from '@react-stately/virtualizer'; +import {HTMLAttributes, Key, ReactNode} from 'react'; +import {LayoutInfo, Size} from '@react-stately/virtualizer'; export interface SideNavProps extends CollectionBase, Expandable, MultipleSelection { shouldFocusWrap?: boolean @@ -27,8 +27,14 @@ export interface SpectrumSideNavItemProps extends HTMLAttributes item: Node } +interface IVirtualizer { + updateItemSize(key: Key, size: Size): void +} + export interface SideNavSectionProps { - reusableView: ReusableView, unknown>, - header: ReusableView, unknown>, + layoutInfo: LayoutInfo, + headerLayoutInfo: LayoutInfo, + virtualizer: IVirtualizer, + item: Node, children?: ReactNode } From 3433338568f453ca97486fefc222e4a8484a90ac Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 15:37:04 -0700 Subject: [PATCH 39/53] Fix DialogContainer again --- packages/@react-spectrum/dialog/src/DialogContainer.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@react-spectrum/dialog/src/DialogContainer.tsx b/packages/@react-spectrum/dialog/src/DialogContainer.tsx index e7f26ee5001..70fa2a88f1b 100644 --- a/packages/@react-spectrum/dialog/src/DialogContainer.tsx +++ b/packages/@react-spectrum/dialog/src/DialogContainer.tsx @@ -30,8 +30,8 @@ export function DialogContainer(props: SpectrumDialogContainerProps) { isKeyboardDismissDisabled } = props; - let childCount = React.Children.count(children); - if (childCount > 1) { + let childArray = React.Children.toArray(children); + if (childArray.length > 1) { throw new Error('Only a single child can be passed to DialogContainer.'); } From 026148ac0f5b73c97a5995bc2ac84bdc974523d4 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 16:54:22 -0700 Subject: [PATCH 40/53] Fix React 16 --- .../@react-spectrum/dialog/src/DialogContainer.tsx | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/@react-spectrum/dialog/src/DialogContainer.tsx b/packages/@react-spectrum/dialog/src/DialogContainer.tsx index 70fa2a88f1b..ce2304172b6 100644 --- a/packages/@react-spectrum/dialog/src/DialogContainer.tsx +++ b/packages/@react-spectrum/dialog/src/DialogContainer.tsx @@ -36,7 +36,16 @@ export function DialogContainer(props: SpectrumDialogContainerProps) { } let [lastChild, setLastChild] = useState(null); - let child = React.isValidElement(children) ? children : null; + + // React.Children.toArray mutates the children, and we need them to be stable + // between renders so that the lastChild comparison works. + let child = null; + if (Array.isArray(children)) { + child = children.find(React.isValidElement); + } else if (React.isValidElement(children)) { + child = children; + } + if (child && child !== lastChild) { setLastChild(child); } From bb42ad9170590b9f94c56bbecf7815b6a6cb8a61 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 20:28:35 -0700 Subject: [PATCH 41/53] Fix useControlledState with suspense We cannot write to a ref in render, because aborted renders due to suspense should not prevent later onChange events from firing. Instead of using a ref, we mutate a local variable to prevent duplicate uncontrolled onChange calls. This way it resets every render and the value and setValue functions are always in sync with each other. It does mean that the setValue function changes every time the value does now, but we should not have been relying on it to be stable because it already changed whenever the onChange function identity changed. --- .../utils/src/useControlledState.ts | 36 ++--- .../utils/test/useControlledState.test.js | 144 ++++++++++++++++++ 2 files changed, 163 insertions(+), 17 deletions(-) diff --git a/packages/@react-stately/utils/src/useControlledState.ts b/packages/@react-stately/utils/src/useControlledState.ts index 2e394844b0a..4205c2bd3b2 100644 --- a/packages/@react-stately/utils/src/useControlledState.ts +++ b/packages/@react-stately/utils/src/useControlledState.ts @@ -18,26 +18,35 @@ export function useControlledState( onChange: (value: T, ...args: any[]) => void ): [T, (value: T, ...args: any[]) => void] { let [stateValue, setStateValue] = useState(value || defaultValue); - let ref = useRef(value !== undefined); - let wasControlled = ref.current; + + let isControlledRef = useRef(value !== undefined); + // If a component goes from controlled to uncontrolled we always want to warn + // even if a render is aborted, so using a ref here is ok. + // eslint-disable-next-line rulesdir/pure-render + let wasControlled = isControlledRef.current; let isControlled = value !== undefined; - // Internal state reference for useCallback - let stateRef = useRef(stateValue); if (wasControlled !== isControlled) { console.warn(`WARN: A component changed from ${wasControlled ? 'controlled' : 'uncontrolled'} to ${isControlled ? 'controlled' : 'uncontrolled'}.`); } - ref.current = isControlled; + // eslint-disable-next-line rulesdir/pure-render + isControlledRef.current = isControlled; + let currentValue = isControlled ? value : stateValue; let setValue = useCallback((value, ...args) => { let onChangeCaller = (value, ...onChangeArgs) => { if (onChange) { - if (!Object.is(stateRef.current, value)) { + if (!Object.is(currentValue, value)) { onChange(value, ...onChangeArgs); } } if (!isControlled) { - stateRef.current = value; + // If uncontrolled, mutate the currentValue local variable so that + // calling setState multiple times with the same value only emits onChange once. + // We do not use a ref for this because we specifically _do_ want the value to + // reset every render, and assigning to a ref in render breaks aborted suspended renders. + // eslint-disable-next-line react-hooks/exhaustive-deps + currentValue = value; } }; @@ -49,7 +58,7 @@ export function useControlledState( // if we're in an uncontrolled state, then we also return the value of myFunc which to setState looks as though it was just called with myFunc from the beginning // otherwise we just return the controlled value, which won't cause a rerender because React knows to bail out when the value is the same let updateFunction = (oldValue, ...functionArgs) => { - let interceptedValue = value(isControlled ? stateRef.current : oldValue, ...functionArgs); + let interceptedValue = value(isControlled ? currentValue : oldValue, ...functionArgs); onChangeCaller(interceptedValue, ...args); if (!isControlled) { return interceptedValue; @@ -63,14 +72,7 @@ export function useControlledState( } onChangeCaller(value, ...args); } - }, [isControlled, onChange]); - - // If a controlled component's value prop changes, we need to update stateRef - if (isControlled) { - stateRef.current = value; - } else { - value = stateValue; - } + }, [isControlled, currentValue, onChange]); - return [value, setValue]; + return [currentValue, setValue]; } diff --git a/packages/@react-stately/utils/test/useControlledState.test.js b/packages/@react-stately/utils/test/useControlledState.test.js index e904a397c9b..7661d6a8544 100644 --- a/packages/@react-stately/utils/test/useControlledState.test.js +++ b/packages/@react-stately/utils/test/useControlledState.test.js @@ -170,6 +170,7 @@ describe('useControlledState tests', function () { propValue = 'updated'; rerender(); + [value, setValue] = result.current; act(() => setValue((prevValue) => { expect(prevValue).toBe('updated'); @@ -232,4 +233,147 @@ describe('useControlledState tests', function () { rerender({value: 'controlledValue', defaultValue: 'defaultValue', onChange: onChangeSpy}); expect(consoleWarnSpy).toHaveBeenLastCalledWith('WARN: A component changed from uncontrolled to controlled.'); }); + + it('should work with suspense when controlled', () => { + if (parseInt(React.version, 10) < 18) { + return; + } + + const AsyncChild = React.lazy(() => new Promise(() => {})); + function Test(props) { + let [value, setValue] = useState(1); + let [showChild, setShowChild] = useState(false); + + return ( + <> + { + setValue(3); + setShowChild(true); + }} /> + { + setValue(v); + props.onChange(v); + }} /> + {showChild && } + + ); + } + + function Child(props) { + let [value, setValue] = useControlledState(props.value, props.defaultValue, props.onChange); + return ( + + ); + } + + function TransitionButton({onClick}) { + let [isPending, startTransition] = React.useTransition(); + return ( + + ); + } + + let onChange = jest.fn(); + let tree = render(); + let value = tree.getByTestId('value'); + let show = tree.getByTestId('show'); + + expect(value).toHaveTextContent('1'); + userEvent.click(value); + + // Clicking the button should update the value as normal. + expect(value).toHaveTextContent('2'); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenLastCalledWith(2); + + // Clicking the show button starts a transition. The new value of 3 + // will be thrown away by React since the component suspended. + expect(show).toHaveTextContent('Show'); + userEvent.click(show); + expect(show).toHaveTextContent('Loading'); + expect(value).toHaveTextContent('2'); + + // Since the previous render was thrown away, the current value shown + // to the user is still 2. Clicking the button should bump it to 3 again. + userEvent.click(value); + expect(value).toHaveTextContent('3'); + expect(onChange).toHaveBeenCalledTimes(2); + expect(onChange).toHaveBeenLastCalledWith(3); + }); + + it('should work with suspense when uncontrolled', async () => { + if (parseInt(React.version, 10) < 18) { + return; + } + + let resolve; + const AsyncChild = React.lazy(() => new Promise((r) => {resolve = r;})); + function Test(props) { + let [value, setValue] = useControlledState(undefined, 1, props.onChange); + let [showChild, setShowChild] = useState(false); + let [isPending, startTransition] = React.useTransition(); + + return ( + <> + + {showChild && } + + ); + } + + function LoadedComponent() { + return
Hello
; + } + + let onChange = jest.fn(); + let tree = render(); + let value = tree.getByTestId('value'); + + expect(value).toHaveTextContent('1'); + userEvent.click(value); + + // React aborts the render, so the value stays at 1. + expect(value).toHaveTextContent('1 (Loading)'); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenLastCalledWith(2); + + // Attempting to change the value will be aborted again. + userEvent.click(value); + expect(value).toHaveTextContent('1 (Loading)'); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenLastCalledWith(2); + + // Now resolve the suspended component. + // Value should now update to the latest one. + resolve({default: LoadedComponent}); + await act(() => Promise.resolve()); + expect(value).toHaveTextContent('2'); + + // Now incrementing works again. + userEvent.click(value); + expect(value).toHaveTextContent('3'); + expect(onChange).toHaveBeenCalledTimes(2); + expect(onChange).toHaveBeenLastCalledWith(3); + }); }); From c48dcfd64b2fe6fe414212f9f01ee81d75440757 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 20:48:25 -0700 Subject: [PATCH 42/53] Add lint rule From #3835 --- .eslintrc.js | 1 + bin/pure-render.js | 189 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 bin/pure-render.js diff --git a/.eslintrc.js b/.eslintrc.js index ee804849d99..7dd808efc45 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -169,6 +169,7 @@ module.exports = { 'rulesdir/sort-imports': [ERROR], 'rulesdir/imports': [ERROR], 'rulesdir/useLayoutEffectRule': [ERROR], + 'rulesdir/pure-render': [ERROR], // jsx-a11y rules 'jsx-a11y/accessible-emoji': ERROR, diff --git a/bin/pure-render.js b/bin/pure-render.js new file mode 100644 index 00000000000..4c84de44d5d --- /dev/null +++ b/bin/pure-render.js @@ -0,0 +1,189 @@ +/* + * Copyright 2020 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +// Copied from https://github.com/facebook/react/pull/24506 +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'enforces component render purity', + recommended: true, + url: 'https://beta.reactjs.org/learn/keeping-components-pure' + } + }, + create(context) { + return { + MemberExpression(member) { + // Look for member expressions that look like refs (i.e. `ref.current`). + if ( + member.object.type !== 'Identifier' || + member.computed || + member.property.type !== 'Identifier' || + member.property.name !== 'current' + ) { + return; + } + + // Find the parent function of this node, as well as any if statement matching against the ref value + // (i.e. lazy init pattern shown in React docs). + let node = member; + let fn; + let conditional; + while (node) { + if ( + node.type === 'FunctionDeclaration' || + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ) { + fn = node; + break; + } + + if ( + node.type === 'IfStatement' && + node.test.type === 'BinaryExpression' && + (node.test.operator === '==' || node.test.operator === '===') && + isMemberExpressionEqual(node.test.left, member) + ) { + conditional = node.test; + } + + node = node.parent; + } + + if (!fn) { + return; + } + + // Find the variable definition for the object. + const variable = getVariable(context.getScope(), member.object.name); + if (!variable) { + return; + } + + // Find the initialization of the variable and see if it's a call to useRef. + const refDefinition = variable.defs.find(def => { + const init = def.node.init; + if (!init) { + return false; + } + + return ( + init.type === 'CallExpression' && + ((init.callee.type === 'Identifier' && + init.callee.name === 'useRef') || + (init.callee.type === 'MemberExpression' && + init.callee.object.type === 'Identifier' && + init.callee.object.name === 'React' && + init.callee.property.type === 'Identifier' && + init.callee.property.name === 'useRef')) && + parentFunction(def.node) === fn + ); + }); + + if (refDefinition) { + // If within an if statement, check if comparing with the initial value passed to useRef. + // This indicates the lazy init pattern, which is allowed. + if (conditional) { + const init = refDefinition.node.init.arguments[0] || { + type: 'Identifier', + name: 'undefined' + }; + if (isLiteralEqual(conditional.operator, init, conditional.right)) { + return; + } + } + + // Otherwise, report an error for either writing or reading to this ref based on parent expression. + context.report({ + node: member, + message: + member.parent.type === 'AssignmentExpression' && + member.parent.left === member + ? 'Writing to refs during rendering is not allowed. Move this into a useEffect or useLayoutEffect. See https://beta.reactjs.org/apis/useref' + : 'Reading from refs during rendering is not allowed. See https://beta.reactjs.org/apis/useref' + }); + } + } + }; + } +}; + +function getVariable(scope, name) { + while (scope) { + const variable = scope.set.get(name); + if (variable) { + return variable; + } + + scope = scope.upper; + } +} + +function parentFunction(node) { + while (node) { + if ( + node.type === 'FunctionDeclaration' || + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ) { + return node; + } + + node = node.parent; + } +} + +function isMemberExpressionEqual(a, b) { + if (a === b) { + return true; + } + + return ( + a.type === 'MemberExpression' && + b.type === 'MemberExpression' && + a.object.type === 'Identifier' && + b.object.type === 'Identifier' && + a.object.name === b.object.name && + a.property.type === 'Identifier' && + b.property.type === 'Identifier' && + a.property.name === b.property.name + ); +} + +function isLiteralEqual(operator, a, b) { + let aValue, bValue; + if (a.type === 'Identifier' && a.name === 'undefined') { + aValue = undefined; + } else if (a.type === 'Literal') { + aValue = a.value; + } else { + return; + } + + if (b.type === 'Identifier' && b.name === 'undefined') { + bValue = undefined; + } else if (b.type === 'Literal') { + bValue = b.value; + } else { + return; + } + + if (operator === '===') { + return aValue === bValue; + } else if (operator === '==') { + // eslint-disable-next-line + return aValue == bValue; + } + + return false; +} From 1413f7c3690cb18a6655328eaf20221de79ca7f2 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 21 May 2023 21:27:04 -0700 Subject: [PATCH 43/53] Don't pass through autoFocus to virtualizer div --- packages/@react-aria/virtualizer/src/Virtualizer.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 590638e965f..0f0a52f6fe5 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -58,6 +58,8 @@ function Virtualizer(props: VirtualizerProps, ref: Re shouldUseVirtualFocus, // eslint-disable-next-line @typescript-eslint/no-unused-vars scrollToItem, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + autoFocus, ...otherProps } = props; From d2631cd929f3f3e9b30085b98701540fcd5b5bfe Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Wed, 24 May 2023 12:56:41 -0700 Subject: [PATCH 44/53] Move warnings to useEffect --- .../@react-spectrum/list/src/ListView.tsx | 19 ++++++++++--------- .../utils/src/useControlledState.ts | 19 ++++++++----------- .../react-aria-components/src/GridList.tsx | 16 ++++++++-------- .../react-aria-components/src/ListBox.tsx | 16 ++++++++-------- packages/react-aria-components/src/Table.tsx | 16 ++++++++-------- 5 files changed, 42 insertions(+), 44 deletions(-) diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index ebead8885ef..e18b6c559d9 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -26,7 +26,7 @@ import {ListState, useListState} from '@react-stately/list'; import listStyles from './styles.css'; import {ListViewItem} from './ListViewItem'; import {ProgressCircle} from '@react-spectrum/progress'; -import React, {Key, ReactElement, useContext, useMemo, useRef, useState} from 'react'; +import React, {Key, ReactElement, useContext, useEffect, useMemo, useRef, useState} from 'react'; import RootDropIndicator from './RootDropIndicator'; import {DragPreview as SpectrumDragPreview} from './DragPreview'; import {useCollator, useLocalizedStringFormatter} from '@react-aria/i18n'; @@ -124,14 +124,15 @@ function ListView(props: SpectrumListViewProps, ref: DOMRef let isListDroppable = !!dragAndDropHooks?.useDroppableCollectionState; let dragHooksProvided = useRef(isListDraggable); let dropHooksProvided = useRef(isListDroppable); - // eslint-disable-next-line rulesdir/pure-render - if (dragHooksProvided.current !== isListDraggable) { - console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); - } - // eslint-disable-next-line rulesdir/pure-render - if (dropHooksProvided.current !== isListDroppable) { - console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); - } + useEffect(() => { + if (dragHooksProvided.current !== isListDraggable) { + console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + if (dropHooksProvided.current !== isListDroppable) { + console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + }, [isListDraggable, isListDraggable]); + let domRef = useDOMRef(ref); let state = useListState({ ...props, diff --git a/packages/@react-stately/utils/src/useControlledState.ts b/packages/@react-stately/utils/src/useControlledState.ts index 4205c2bd3b2..e0925fa85be 100644 --- a/packages/@react-stately/utils/src/useControlledState.ts +++ b/packages/@react-stately/utils/src/useControlledState.ts @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {useCallback, useRef, useState} from 'react'; +import {useCallback, useEffect, useRef, useState} from 'react'; export function useControlledState( value: T, @@ -20,17 +20,14 @@ export function useControlledState( let [stateValue, setStateValue] = useState(value || defaultValue); let isControlledRef = useRef(value !== undefined); - // If a component goes from controlled to uncontrolled we always want to warn - // even if a render is aborted, so using a ref here is ok. - // eslint-disable-next-line rulesdir/pure-render - let wasControlled = isControlledRef.current; let isControlled = value !== undefined; - if (wasControlled !== isControlled) { - console.warn(`WARN: A component changed from ${wasControlled ? 'controlled' : 'uncontrolled'} to ${isControlled ? 'controlled' : 'uncontrolled'}.`); - } - - // eslint-disable-next-line rulesdir/pure-render - isControlledRef.current = isControlled; + useEffect(() => { + let wasControlled = isControlledRef.current; + if (wasControlled !== isControlled) { + console.warn(`WARN: A component changed from ${wasControlled ? 'controlled' : 'uncontrolled'} to ${isControlled ? 'controlled' : 'uncontrolled'}.`); + } + isControlledRef.current = isControlled; + }, [isControlled]); let currentValue = isControlled ? value : stateValue; let setValue = useCallback((value, ...args) => { diff --git a/packages/react-aria-components/src/GridList.tsx b/packages/react-aria-components/src/GridList.tsx index 3dd4257c422..c5824a8889d 100644 --- a/packages/react-aria-components/src/GridList.tsx +++ b/packages/react-aria-components/src/GridList.tsx @@ -91,14 +91,14 @@ function GridList(props: GridListProps, ref: ForwardedRef { + if (dragHooksProvided.current !== isListDraggable) { + console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + if (dropHooksProvided.current !== isListDroppable) { + console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + }, [isListDraggable, isListDroppable]); let dragState: DraggableCollectionState | undefined = undefined; let dropState: DroppableCollectionState | undefined = undefined; diff --git a/packages/react-aria-components/src/ListBox.tsx b/packages/react-aria-components/src/ListBox.tsx index 5e4e2de0111..2c9f6add034 100644 --- a/packages/react-aria-components/src/ListBox.tsx +++ b/packages/react-aria-components/src/ListBox.tsx @@ -132,14 +132,14 @@ function ListBoxInner({state, props, listBoxRef}: ListBoxInnerProps) { let dragHooksProvided = useRef(isListDraggable); let dropHooksProvided = useRef(isListDroppable); - // eslint-disable-next-line rulesdir/pure-render - if (dragHooksProvided.current !== isListDraggable) { - console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); - } - // eslint-disable-next-line rulesdir/pure-render - if (dropHooksProvided.current !== isListDroppable) { - console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); - } + useEffect(() => { + if (dragHooksProvided.current !== isListDraggable) { + console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + if (dropHooksProvided.current !== isListDroppable) { + console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + }, [isListDraggable, isListDroppable]); let dragState: DraggableCollectionState | undefined = undefined; let dropState: DroppableCollectionState | undefined = undefined; diff --git a/packages/react-aria-components/src/Table.tsx b/packages/react-aria-components/src/Table.tsx index 6d97481d865..0d361a63f41 100644 --- a/packages/react-aria-components/src/Table.tsx +++ b/packages/react-aria-components/src/Table.tsx @@ -243,14 +243,14 @@ function Table(props: TableProps, ref: ForwardedRef) { let isListDroppable = !!dragAndDropHooks?.useDroppableCollectionState; let dragHooksProvided = useRef(isListDraggable); let dropHooksProvided = useRef(isListDroppable); - // eslint-disable-next-line rulesdir/pure-render - if (dragHooksProvided.current !== isListDraggable) { - console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); - } - // eslint-disable-next-line rulesdir/pure-render - if (dropHooksProvided.current !== isListDroppable) { - console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); - } + useEffect(() => { + if (dragHooksProvided.current !== isListDraggable) { + console.warn('Drag hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + if (dropHooksProvided.current !== isListDroppable) { + console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); + } + }, [isListDraggable, isListDroppable]); let dragState: DraggableCollectionState | undefined = undefined; let dropState: DroppableCollectionState | undefined = undefined; From e852e5302aed501a7f9565de7850271421709342 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Wed, 24 May 2023 12:57:40 -0700 Subject: [PATCH 45/53] Use Object.is in useUpdateEffect --- packages/@react-aria/utils/src/useUpdateEffect.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-aria/utils/src/useUpdateEffect.ts b/packages/@react-aria/utils/src/useUpdateEffect.ts index 567ec60c6bf..fab098c0c4b 100644 --- a/packages/@react-aria/utils/src/useUpdateEffect.ts +++ b/packages/@react-aria/utils/src/useUpdateEffect.ts @@ -27,7 +27,7 @@ export function useUpdateEffect(effect: EffectCallback, dependencies: any[]) { useEffect(() => { if (isInitialMount.current) { isInitialMount.current = false; - } else if (!lastDeps.current || dependencies.some((dep, i) => dep !== lastDeps[i])) { + } else if (!lastDeps.current || dependencies.some((dep, i) => !Object.is(dep, lastDeps[i]))) { effect(); } lastDeps.current = dependencies; From 3a3b7a7cd8f25926f908b8c859bc2f5cf9093212 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Wed, 31 May 2023 12:00:53 -0700 Subject: [PATCH 46/53] Fix onLoadMore --- .../virtualizer/src/Virtualizer.tsx | 36 ++++++++++++------- .../src/MobileSearchAutocomplete.tsx | 5 +-- .../autocomplete/src/SearchAutocomplete.tsx | 4 +-- .../@react-spectrum/combobox/src/ComboBox.tsx | 4 +-- .../combobox/src/MobileComboBox.tsx | 5 +-- .../combobox/test/ComboBox.test.js | 14 ++++---- .../@react-spectrum/listbox/src/ListBox.tsx | 2 +- .../listbox/src/ListBoxBase.tsx | 18 +++++----- .../@react-spectrum/picker/src/Picker.tsx | 10 +++--- 9 files changed, 55 insertions(+), 43 deletions(-) diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 0f0a52f6fe5..4de8445566a 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -184,15 +184,6 @@ export function useVirtualizer(props: VirtualizerOptions // Handle scrolling, and call onLoadMore when nearing the bottom. let isLoadingRef = useRef(isLoading); let prevProps = useRef(props); - useLayoutEffect(() => { - // Only update isLoadingRef if props object actually changed, - // not if a local state change occurred. - if (props !== prevProps.current) { - isLoadingRef.current = isLoading; - prevProps.current = props; - } - }, [isLoading, props]); - let onVisibleRectChange = useCallback((rect: Rect) => { state.setVisibleRect(rect); @@ -207,14 +198,33 @@ export function useVirtualizer(props: VirtualizerOptions let lastContentSize = useRef(0); useLayoutEffect(() => { - if (!isLoadingRef.current && onLoadMore && !state.isAnimating) { - if (state.contentSize.height !== lastContentSize.current && state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) { + // If animating, wait until we're done. + if (state.isAnimating) { + return; + } + + // Only update isLoadingRef if props object actually changed, + // not if a local state change occurred. + let wasLoading = isLoadingRef.current; + if (props !== prevProps.current) { + isLoadingRef.current = isLoading; + prevProps.current = props; + } + + let shouldLoadMore = !isLoadingRef.current + && onLoadMore + && state.contentSize.height > 0 + && state.contentSize.height <= state.virtualizer.visibleRect.height + // Only try loading more if the content size changed, or if we just finished + // loading and still have room for more items. + && (wasLoading || state.contentSize.height !== lastContentSize.current); + + if (shouldLoadMore) { isLoadingRef.current = true; onLoadMore(); } - } lastContentSize.current = state.contentSize.height; - }, [state.contentSize, state.isAnimating, state.virtualizer, onLoadMore]); + }, [state.contentSize, state.isAnimating, state.virtualizer, isLoading, onLoadMore, props]); return { virtualizerProps: { diff --git a/packages/@react-spectrum/autocomplete/src/MobileSearchAutocomplete.tsx b/packages/@react-spectrum/autocomplete/src/MobileSearchAutocomplete.tsx index 770d67ef64d..fb2e69d210d 100644 --- a/packages/@react-spectrum/autocomplete/src/MobileSearchAutocomplete.tsx +++ b/packages/@react-spectrum/autocomplete/src/MobileSearchAutocomplete.tsx @@ -355,7 +355,8 @@ function SearchAutocompleteTray(props: SearchAutocompleteTrayProps) { let inputRef = useRef(null); let popoverRef = useRef(null); let listBoxRef = useRef(null); - let layout = useListBoxLayout(state); + let isLoading = loadingState === 'loading' || loadingState === 'loadingMore'; + let layout = useListBoxLayout(state, isLoading); let stringFormatter = useLocalizedStringFormatter(intlMessages); let {inputProps, listBoxProps, labelProps, clearButtonProps} = useSearchAutocomplete( @@ -582,7 +583,7 @@ function SearchAutocompleteTray(props: SearchAutocompleteTrayProps) { ref={listBoxRef} onScroll={onScroll} onLoadMore={onLoadMore} - isLoading={loadingState === 'loading' || loadingState === 'loadingMore'} /> + isLoading={isLoading} />
diff --git a/packages/@react-spectrum/autocomplete/src/SearchAutocomplete.tsx b/packages/@react-spectrum/autocomplete/src/SearchAutocomplete.tsx index 3a47dadca8c..2c761e9e83e 100644 --- a/packages/@react-spectrum/autocomplete/src/SearchAutocomplete.tsx +++ b/packages/@react-spectrum/autocomplete/src/SearchAutocomplete.tsx @@ -94,7 +94,7 @@ function _SearchAutocompleteBase(props: SpectrumSearchAutocomp defaultSelectedKey: undefined } ); - let layout = useListBoxLayout(state); + let layout = useListBoxLayout(state, loadingState === 'loadingMore'); let {inputProps, listBoxProps, labelProps, clearButtonProps, descriptionProps, errorMessageProps} = useSearchAutocomplete( { @@ -167,7 +167,7 @@ function _SearchAutocompleteBase(props: SpectrumSearchAutocomp layout={layout} state={state} shouldUseVirtualFocus - isLoading={loadingState === 'loadingMore'} + isLoading={loadingState === 'loading' || loadingState === 'loadingMore'} onLoadMore={onLoadMore} renderEmptyState={() => isAsync && ( diff --git a/packages/@react-spectrum/combobox/src/ComboBox.tsx b/packages/@react-spectrum/combobox/src/ComboBox.tsx index a58fb4e065b..a3fc601f34f 100644 --- a/packages/@react-spectrum/combobox/src/ComboBox.tsx +++ b/packages/@react-spectrum/combobox/src/ComboBox.tsx @@ -95,7 +95,7 @@ const ComboBoxBase = React.forwardRef(function ComboBoxBase(pr allowsEmptyCollection: isAsync } ); - let layout = useListBoxLayout(state); + let layout = useListBoxLayout(state, loadingState === 'loadingMore'); let {buttonProps, inputProps, listBoxProps, labelProps, descriptionProps, errorMessageProps} = useComboBox( { @@ -172,7 +172,7 @@ const ComboBoxBase = React.forwardRef(function ComboBoxBase(pr layout={layout} state={state} shouldUseVirtualFocus - isLoading={loadingState === 'loadingMore'} + isLoading={loadingState === 'loading' || loadingState === 'loadingMore'} onLoadMore={onLoadMore} renderEmptyState={() => isAsync && ( diff --git a/packages/@react-spectrum/combobox/src/MobileComboBox.tsx b/packages/@react-spectrum/combobox/src/MobileComboBox.tsx index dc909f8f207..68dc4d8793b 100644 --- a/packages/@react-spectrum/combobox/src/MobileComboBox.tsx +++ b/packages/@react-spectrum/combobox/src/MobileComboBox.tsx @@ -293,7 +293,8 @@ function ComboBoxTray(props: ComboBoxTrayProps) { let buttonRef = useRef>(); let popoverRef = useRef(); let listBoxRef = useRef(); - let layout = useListBoxLayout(state); + let isLoading = loadingState === 'loading' || loadingState === 'loadingMore'; + let layout = useListBoxLayout(state, isLoading); let stringFormatter = useLocalizedStringFormatter(intlMessages); let {inputProps, listBoxProps, labelProps} = useComboBox( @@ -508,7 +509,7 @@ function ComboBoxTray(props: ComboBoxTrayProps) { ref={listBoxRef} onScroll={onScroll} onLoadMore={onLoadMore} - isLoading={loadingState === 'loading' || loadingState === 'loadingMore'} /> + isLoading={isLoading} />
diff --git a/packages/@react-spectrum/combobox/test/ComboBox.test.js b/packages/@react-spectrum/combobox/test/ComboBox.test.js index 6fc46c3b4ba..3946a3c2de8 100644 --- a/packages/@react-spectrum/combobox/test/ComboBox.test.js +++ b/packages/@react-spectrum/combobox/test/ComboBox.test.js @@ -2022,7 +2022,13 @@ describe('ComboBox', function () { let scrollHeightSpy; beforeEach(() => { // clientHeight is needed for ScrollView's updateSize() - clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40); + clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(function () { + if (this.getAttribute('role') === 'listbox') { + return 100; + } + + return 40; + }); // scrollHeight is for useVirutalizerItem to mock its getSize() scrollHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 32); }); @@ -2055,7 +2061,6 @@ describe('ComboBox', function () { }); let listbox = getByRole('listbox'); expect(listbox).toBeVisible(); - jest.spyOn(listbox, 'clientHeight', 'get').mockImplementation(() => 100); // update size, virtualizer raf kicks in act(() => {jest.advanceTimersToNextTimer();}); // onLoadMore queued by previous timer, run it now @@ -2094,10 +2099,6 @@ describe('ComboBox', function () { expect(onOpenChange).toHaveBeenCalledTimes(0); expect(onLoadMore).toHaveBeenCalledTimes(0); - // this call and the one below are more correct for how the code should - // behave, the initial call would have a height of zero and after that a measureable height - clientHeightSpy.mockRestore(); - clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40); // open menu triggerPress(button); // use async act to resolve initial load @@ -2107,7 +2108,6 @@ describe('ComboBox', function () { }); let listbox = getByRole('listbox'); expect(listbox).toBeVisible(); - jest.spyOn(listbox, 'clientHeight', 'get').mockImplementation(() => 100); // update size, virtualizer raf kicks in act(() => {jest.advanceTimersToNextTimer();}); // onLoadMore queued by previous timer, run it now diff --git a/packages/@react-spectrum/listbox/src/ListBox.tsx b/packages/@react-spectrum/listbox/src/ListBox.tsx index 650d7133baa..26b67a1d709 100644 --- a/packages/@react-spectrum/listbox/src/ListBox.tsx +++ b/packages/@react-spectrum/listbox/src/ListBox.tsx @@ -19,7 +19,7 @@ import {useListState} from '@react-stately/list'; function ListBox(props: SpectrumListBoxProps, ref: DOMRef) { let state = useListState(props); - let layout = useListBoxLayout(state); + let layout = useListBoxLayout(state, props.isLoading); let domRef = useDOMRef(ref); return ( diff --git a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx index 5069148c6ef..303df72bd90 100644 --- a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx +++ b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx @@ -48,7 +48,7 @@ interface ListBoxBaseProps extends AriaListBoxOptions, DOMProps, AriaLabel } /** @private */ -export function useListBoxLayout(state: ListState): ListLayout { +export function useListBoxLayout(state: ListState, isLoading: boolean): ListLayout { let {scale} = useProvider(); let collator = useCollator({usage: 'search', sensitivity: 'base'}); let layout = useMemo(() => @@ -64,6 +64,14 @@ export function useListBoxLayout(state: ListState): ListLayout { layout.collection = state.collection; layout.disabledKeys = state.disabledKeys; + + useLayoutEffect(() => { + // Sync loading state into the layout. + if (layout.isLoading !== isLoading) { + layout.isLoading = isLoading; + layout.virtualizer?.relayoutNow(); + } + }, [layout, isLoading]); return layout; } @@ -78,14 +86,6 @@ function ListBoxBase(props: ListBoxBaseProps, ref: RefObject { - // Sync loading state into the layout. - if (layout.isLoading !== props.isLoading) { - layout.isLoading = props.isLoading; - layout.virtualizer.relayoutNow(); - } - }, [layout, props.isLoading]); - // This overrides collection view's renderWrapper to support heirarchy of items in sections. // The header is extracted from the children so it can receive ARIA labeling properties. type View = ReusableView, unknown>; diff --git a/packages/@react-spectrum/picker/src/Picker.tsx b/packages/@react-spectrum/picker/src/Picker.tsx index bbcb111952c..138e7dd9044 100644 --- a/packages/@react-spectrum/picker/src/Picker.tsx +++ b/packages/@react-spectrum/picker/src/Picker.tsx @@ -68,10 +68,13 @@ function Picker(props: SpectrumPickerProps, ref: DOMRef 0; + // We create the listbox layout in Picker and pass it to ListBoxBase below // so that the layout information can be cached even while the listbox is not mounted. // We also use the layout as the keyboard delegate for type to select. - let layout = useListBoxLayout(state); + let layout = useListBoxLayout(state, isLoadingMore); let {labelProps, triggerProps, valueProps, menuProps, descriptionProps, errorMessageProps} = useSelect({ ...props, keyboardDelegate: layout @@ -80,9 +83,6 @@ function Picker(props: SpectrumPickerProps, ref: DOMRef 0; - // On small screen devices, the listbox is rendered in a tray, otherwise a popover. let listbox = ( (props: SpectrumPickerProps, ref: DOMRef ); From c9ee39426597a4f8876e66964b06bcb423ff948b Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Wed, 31 May 2023 12:13:37 -0700 Subject: [PATCH 47/53] Fix Page Up/Page Down scroll into view --- .../@react-aria/selection/src/useSelectableCollection.ts | 6 ++++-- packages/@react-aria/selection/src/useSelectableItem.ts | 5 +---- packages/@react-aria/virtualizer/src/Virtualizer.tsx | 9 +++------ packages/@react-spectrum/table/src/TableView.tsx | 7 +------ 4 files changed, 9 insertions(+), 18 deletions(-) diff --git a/packages/@react-aria/selection/src/useSelectableCollection.ts b/packages/@react-aria/selection/src/useSelectableCollection.ts index 355370c4c14..88f6f043e64 100644 --- a/packages/@react-aria/selection/src/useSelectableCollection.ts +++ b/packages/@react-aria/selection/src/useSelectableCollection.ts @@ -366,10 +366,12 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions // When virtualized, Virtualizer handles this internally. useEffect(() => { let modality = getInteractionModality(); - if (!isVirtualized && manager.isFocused && manager.focusedKey != null && scrollRef?.current) { + if (manager.isFocused && manager.focusedKey != null && scrollRef?.current) { let element = scrollRef.current.querySelector(`[data-key="${manager.focusedKey}"]`) as HTMLElement; if (element) { - scrollIntoView(scrollRef.current, element); + if (!isVirtualized) { + scrollIntoView(scrollRef.current, element); + } if (modality === 'keyboard') { scrollIntoViewport(element, {containingElement: ref.current}); } diff --git a/packages/@react-aria/selection/src/useSelectableItem.ts b/packages/@react-aria/selection/src/useSelectableItem.ts index e151e7e89ff..7853501ebbd 100644 --- a/packages/@react-aria/selection/src/useSelectableItem.ts +++ b/packages/@react-aria/selection/src/useSelectableItem.ts @@ -266,10 +266,7 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte }; } - if (!isVirtualized) { - itemProps['data-key'] = key; - } - + itemProps['data-key'] = key; itemPressProps.preventFocusOnPress = shouldUseVirtualFocus; let {pressProps, isPressed} = usePress(itemPressProps); diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 4de8445566a..921ae2dafd5 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -130,9 +130,6 @@ export function useVirtualizer(props: VirtualizerOptions } else { virtualizer.scrollToItem(focusedKey, {duration: 0}); - if (modality === 'keyboard' && ref.current.contains(document.activeElement)) { - scrollIntoViewport(document.activeElement, {containingElement: ref.current}); - } } } @@ -220,9 +217,9 @@ export function useVirtualizer(props: VirtualizerOptions && (wasLoading || state.contentSize.height !== lastContentSize.current); if (shouldLoadMore) { - isLoadingRef.current = true; - onLoadMore(); - } + isLoadingRef.current = true; + onLoadMore(); + } lastContentSize.current = state.contentSize.height; }, [state.contentSize, state.isAnimating, state.virtualizer, isLoading, onLoadMore, props]); diff --git a/packages/@react-spectrum/table/src/TableView.tsx b/packages/@react-spectrum/table/src/TableView.tsx index a6472af3e3f..1380d372acf 100644 --- a/packages/@react-spectrum/table/src/TableView.tsx +++ b/packages/@react-spectrum/table/src/TableView.tsx @@ -612,8 +612,6 @@ function TableVirtualizer(props) { let column = collection.columns[0]; let virtualizer = state.virtualizer; - let modality = getInteractionModality(); - virtualizer.scrollToItem(key, { duration: 0, // Prevent scrolling to the top when clicking on column headers. @@ -628,10 +626,7 @@ function TableVirtualizer(props) { // Sync the scroll positions of the column headers and the body so scrollIntoViewport can // properly decide if the column is outside the viewport or not headerRef.current.scrollLeft = bodyRef.current.scrollLeft; - if (modality === 'keyboard') { - scrollIntoViewport(document.activeElement, {containingElement: domRef.current}); - } - }, [collection, domRef, bodyRef, headerRef, layout, state.virtualizer]); + }, [collection, bodyRef, headerRef, layout, state.virtualizer]); let memoedVirtualizerProps = useMemo(() => ({ tabIndex: otherProps.tabIndex, From 0e372ef229231bbe5c9d35e25c4c284ff064d6bf Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Wed, 31 May 2023 13:47:34 -0700 Subject: [PATCH 48/53] Focus collection when focusedKey becomes null --- .../@react-aria/selection/src/useSelectableCollection.ts | 8 ++++++++ packages/@react-stately/grid/src/useGridState.ts | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/@react-aria/selection/src/useSelectableCollection.ts b/packages/@react-aria/selection/src/useSelectableCollection.ts index 88f6f043e64..82d84736340 100644 --- a/packages/@react-aria/selection/src/useSelectableCollection.ts +++ b/packages/@react-aria/selection/src/useSelectableCollection.ts @@ -364,6 +364,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions // If not virtualized, scroll the focused element into view when the focusedKey changes. // When virtualized, Virtualizer handles this internally. + let lastFocusedKey = useRef(manager.focusedKey); useEffect(() => { let modality = getInteractionModality(); if (manager.isFocused && manager.focusedKey != null && scrollRef?.current) { @@ -377,6 +378,13 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions } } } + + // If the focused key becomes null (e.g. the last item is deleted), focus the whole collection. + if (manager.isFocused && manager.focusedKey == null && lastFocusedKey.current != null) { + focusSafely(ref.current); + } + + lastFocusedKey.current = manager.focusedKey; }, [isVirtualized, scrollRef, manager.focusedKey, manager.isFocused, ref]); let handlers = { diff --git a/packages/@react-stately/grid/src/useGridState.ts b/packages/@react-stately/grid/src/useGridState.ts index 19d801fd572..6686659fe7f 100644 --- a/packages/@react-stately/grid/src/useGridState.ts +++ b/packages/@react-stately/grid/src/useGridState.ts @@ -73,7 +73,7 @@ export function useGridState>(prop rows.length - 1); let newRow:GridNode; while (index >= 0) { - if (!selectionManager.isDisabled(rows[index].key)) { + if (!selectionManager.isDisabled(rows[index].key) && rows[index].type !== 'headerrow') { newRow = rows[index]; break; } From 8b794f691fee92ea65e501c902a2f875d0aa93fd Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Wed, 31 May 2023 14:08:07 -0700 Subject: [PATCH 49/53] lint --- packages/@react-aria/selection/src/useSelectableItem.ts | 1 - packages/@react-aria/virtualizer/src/Virtualizer.tsx | 2 +- packages/@react-spectrum/list/src/ListView.tsx | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/@react-aria/selection/src/useSelectableItem.ts b/packages/@react-aria/selection/src/useSelectableItem.ts index 7853501ebbd..aea047786b5 100644 --- a/packages/@react-aria/selection/src/useSelectableItem.ts +++ b/packages/@react-aria/selection/src/useSelectableItem.ts @@ -103,7 +103,6 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte key, ref, shouldSelectOnPressUp, - isVirtualized, shouldUseVirtualFocus, focus, isDisabled, diff --git a/packages/@react-aria/virtualizer/src/Virtualizer.tsx b/packages/@react-aria/virtualizer/src/Virtualizer.tsx index 921ae2dafd5..5e009310f51 100644 --- a/packages/@react-aria/virtualizer/src/Virtualizer.tsx +++ b/packages/@react-aria/virtualizer/src/Virtualizer.tsx @@ -13,7 +13,7 @@ import {Collection} from '@react-types/shared'; import {getInteractionModality} from '@react-aria/interactions'; import {Layout, Rect, ReusableView, useVirtualizerState, VirtualizerState} from '@react-stately/virtualizer'; -import {mergeProps, scrollIntoViewport, useLayoutEffect} from '@react-aria/utils'; +import {mergeProps, useLayoutEffect} from '@react-aria/utils'; import React, {FocusEvent, HTMLAttributes, Key, ReactElement, RefObject, useCallback, useEffect, useMemo, useRef} from 'react'; import {ScrollView} from './ScrollView'; import {VirtualizerItem} from './VirtualizerItem'; diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index e18b6c559d9..ab526776725 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -131,7 +131,7 @@ function ListView(props: SpectrumListViewProps, ref: DOMRef if (dropHooksProvided.current !== isListDroppable) { console.warn('Drop hooks were provided during one render, but not another. This should be avoided as it may produce unexpected behavior.'); } - }, [isListDraggable, isListDraggable]); + }, [isListDraggable, isListDroppable]); let domRef = useDOMRef(ref); let state = useListState({ From 2f40aa887891117542ae8ce326fae87611e8d437 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 1 Jun 2023 13:54:01 -0700 Subject: [PATCH 50/53] Enable strict mode by default in tests --- package.json | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 62fc0518afe..eeda223d84c 100644 --- a/package.json +++ b/package.json @@ -20,12 +20,11 @@ "build:chromatic": "CHROMATIC=1 build-storybook -c .chromatic -o dist/$(git rev-parse HEAD)/chromatic", "start:docs": "DOCS_ENV=dev parcel 'packages/@react-{spectrum,aria,stately}/*/docs/*.mdx' 'packages/react-aria-components/docs/*.mdx' 'packages/@internationalized/*/docs/*.mdx' 'packages/dev/docs/pages/**/*.mdx'", "build:docs": "DOCS_ENV=staging parcel build 'packages/@react-{spectrum,aria,stately}/*/docs/*.mdx' 'packages/react-aria-components/docs/*.mdx' 'packages/@internationalized/*/docs/*.mdx' 'packages/dev/docs/pages/**/*.mdx'", - "test": "yarn jest", - "test-strict": "cross-env STRICT_MODE=1 yarn jest", + "test": "cross-env STRICT_MODE=1 yarn jest", + "test-loose": "yarn jest", "build": "make build", - "test:ssr": "yarn jest --config jest.ssr.config.js", - "ci-test": "yarn jest --maxWorkers=2 && yarn test:ssr --runInBand", - "ci-test-16": "yarn jest --maxWorkers=2 && yarn test:ssr --runInBand", + "test:ssr": "cross-env STRICT_MODE=1 yarn jest --config jest.ssr.config.js", + "ci-test": "cross-env STRICT_MODE=1 yarn jest --maxWorkers=2 && cross-env STRICT_MODE=1 yarn test:ssr --runInBand", "lint": "concurrently \"yarn check-types\" \"eslint packages --ext .js,.ts,.tsx\" \"node scripts/lint-packages.js\"", "jest": "node scripts/jest.js", "copyrights": "babel-node --presets @babel/env ./scripts/addHeaders.js", From 1fceb38f9beade1f46776ccb12aa2d4c946064d2 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 1 Jun 2023 14:02:30 -0700 Subject: [PATCH 51/53] Enable strict mode by default in storybook, and hide in production builds --- .storybook/custom-addons/strictmode/index.js | 2 +- .storybook/custom-addons/strictmode/register.js | 2 +- .storybook/preview.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.storybook/custom-addons/strictmode/index.js b/.storybook/custom-addons/strictmode/index.js index 49415f62acf..d34d99489c0 100644 --- a/.storybook/custom-addons/strictmode/index.js +++ b/.storybook/custom-addons/strictmode/index.js @@ -4,7 +4,7 @@ import React, {StrictMode, useEffect, useState} from 'react'; function StrictModeDecorator(props) { let {children} = props; - let [isStrict, setStrict] = useState(getQueryParams()?.strict === 'true' || false); + let [isStrict, setStrict] = useState(getQueryParams()?.strict !== 'false'); useEffect(() => { let channel = addons.getChannel(); diff --git a/.storybook/custom-addons/strictmode/register.js b/.storybook/custom-addons/strictmode/register.js index b226fe1e0b3..554fb57ff6c 100644 --- a/.storybook/custom-addons/strictmode/register.js +++ b/.storybook/custom-addons/strictmode/register.js @@ -4,7 +4,7 @@ import React, {useEffect, useState} from 'react'; const StrictModeToolBar = ({api}) => { let channel = addons.getChannel(); - let [isStrict, setStrict] = useState(getQueryParams()?.strict === 'true' || false); + let [isStrict, setStrict] = useState(getQueryParams()?.strict !== 'false'); let onChange = () => { setStrict((old) => { channel.emit('strict/updated', !old); diff --git a/.storybook/preview.js b/.storybook/preview.js index 3765010afa6..e6297db4432 100644 --- a/.storybook/preview.js +++ b/.storybook/preview.js @@ -26,6 +26,6 @@ export const parameters = { export const decorators = [ withScrollingSwitcher, - withStrictModeSwitcher, + ...(process.env.NODE_ENV !== 'production' ? [withStrictModeSwitcher] : []), withProviderSwitcher ]; From 3703359a0e73e0da7cbc0320f589a40becd13ced Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 1 Jun 2023 14:37:31 -0700 Subject: [PATCH 52/53] Fix React canary --- .../@react-spectrum/toast/test/ToastContainer.test.js | 9 ++++++++- packages/@react-stately/table/src/TableHeader.ts | 7 ++++++- .../@react-stately/utils/test/useControlledState.test.js | 2 +- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/@react-spectrum/toast/test/ToastContainer.test.js b/packages/@react-spectrum/toast/test/ToastContainer.test.js index dd26498cba8..8a8fc728e98 100644 --- a/packages/@react-spectrum/toast/test/ToastContainer.test.js +++ b/packages/@react-spectrum/toast/test/ToastContainer.test.js @@ -326,7 +326,14 @@ describe('Toast Provider and Container', function () { return ( diff --git a/packages/@react-stately/table/src/TableHeader.ts b/packages/@react-stately/table/src/TableHeader.ts index b56befa7523..bff2d613f04 100644 --- a/packages/@react-stately/table/src/TableHeader.ts +++ b/packages/@react-stately/table/src/TableHeader.ts @@ -10,6 +10,7 @@ * governing permissions and limitations under the License. */ +import {CollectionBuilderContext} from './useTableState'; import {PartialNode} from '@react-stately/collections'; import React, {ReactElement} from 'react'; import {TableHeaderProps} from '@react-types/table'; @@ -18,8 +19,12 @@ function TableHeader(props: TableHeaderProps): ReactElement { // eslint-di return null; } -TableHeader.getCollectionNode = function* getCollectionNode(props: TableHeaderProps): Generator, void, any> { +TableHeader.getCollectionNode = function* getCollectionNode(props: TableHeaderProps, context: CollectionBuilderContext): Generator, void, any> { let {children, columns} = props; + + // Clear columns so they aren't double added in strict mode. + context.columns = []; + if (typeof children === 'function') { if (!columns) { throw new Error('props.children was a function but props.columns is missing'); diff --git a/packages/@react-stately/utils/test/useControlledState.test.js b/packages/@react-stately/utils/test/useControlledState.test.js index 7661d6a8544..cdf02763d5b 100644 --- a/packages/@react-stately/utils/test/useControlledState.test.js +++ b/packages/@react-stately/utils/test/useControlledState.test.js @@ -88,7 +88,7 @@ describe('useControlledState tests', function () { let TestComponent = (props) => { let [state, setState] = useControlledState(props.value, props.defaultValue, props.onChange); useEffect(() => renderSpy(), [state]); - return