From e83bfe388c3f6b6e961b2cd330e1b1a02066a4f6 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Mon, 30 Oct 2023 14:39:21 +0000 Subject: [PATCH] chore(react-tree): stop unnecessary re-rendering when no actions are available --- ...-b6e16366-ba82-402d-b48d-bf2d6449bb78.json | 7 ++ .../react-tree/etc/react-tree.api.md | 1 + .../src/components/TreeItem/useTreeItem.tsx | 70 ++--------- .../TreeItem/useTreeItemContextValues.ts | 4 + .../TreeItemLayout/TreeItemLayout.test.tsx | 8 +- .../TreeItemLayout/useTreeItemLayout.tsx | 110 ++++++++++++++++-- .../TreeItemPersonaLayout.test.tsx | 6 + .../src/contexts/treeItemContext.ts | 12 ++ 8 files changed, 148 insertions(+), 70 deletions(-) create mode 100644 change/@fluentui-react-tree-b6e16366-ba82-402d-b48d-bf2d6449bb78.json diff --git a/change/@fluentui-react-tree-b6e16366-ba82-402d-b48d-bf2d6449bb78.json b/change/@fluentui-react-tree-b6e16366-ba82-402d-b48d-bf2d6449bb78.json new file mode 100644 index 00000000000000..d8f6065f22278e --- /dev/null +++ b/change/@fluentui-react-tree-b6e16366-ba82-402d-b48d-bf2d6449bb78.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "chore: stop unnecessary re-rendering when no actions are available", + "packageName": "@fluentui/react-tree", + "email": "bernardo.sunderhus@gmail.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-tree/etc/react-tree.api.md b/packages/react-components/react-tree/etc/react-tree.api.md index 95f4c04fe59013..1cfd3d51af3e2d 100644 --- a/packages/react-components/react-tree/etc/react-tree.api.md +++ b/packages/react-components/react-tree/etc/react-tree.api.md @@ -177,6 +177,7 @@ export type TreeItemContextValue = { expandIconRef: React_2.Ref; layoutRef: React_2.Ref; subtreeRef: React_2.Ref; + treeItemRef?: React_2.RefObject; itemType: TreeItemType; value: TreeItemValue; open: boolean; diff --git a/packages/react-components/react-tree/src/components/TreeItem/useTreeItem.tsx b/packages/react-components/react-tree/src/components/TreeItem/useTreeItem.tsx index c0b5b14a11a322..41e0a9c8aa424a 100644 --- a/packages/react-components/react-tree/src/components/TreeItem/useTreeItem.tsx +++ b/packages/react-components/react-tree/src/components/TreeItem/useTreeItem.tsx @@ -3,10 +3,10 @@ import * as ReactDOM from 'react-dom'; import { getIntrinsicElementProps, useId, - useMergedRefs, useEventCallback, slot, elementContains, + useMergedRefs, } from '@fluentui/react-utilities'; import type { TreeItemProps, TreeItemState, TreeItemValue } from './TreeItem.types'; import { Space } from '@fluentui/keyboard-keys'; @@ -51,18 +51,12 @@ export function useTreeItem_unstable(props: TreeItemProps, ref: React.Ref { - setAsideVisible(actionsElement === null); - }, []); - const actionsRef = React.useRef(null); const expandIconRef = React.useRef(null); const layoutRef = React.useRef(null); const subtreeRef = React.useRef(null); const selectionRef = React.useRef(null); + const treeItemRef = React.useRef(null); const open = useTreeContext_unstable(ctx => props.open ?? ctx.openItems.has(value)); const selectionMode = useTreeContext_unstable(ctx => ctx.selectionMode); @@ -200,51 +194,6 @@ export function useTreeItem_unstable(props: TreeItemProps, ref: React.Ref) => { - const isTargetFromSubtree = Boolean( - subtreeRef.current && elementContains(subtreeRef.current, event.target as Node), - ); - if (!isTargetFromSubtree) { - setActionsVisible(true); - } - }, []); - const setActionsInvisibleIfNotFromSubtree = React.useCallback( - (event: React.MouseEvent | React.FocusEvent) => { - const isTargetFromSubtree = Boolean( - subtreeRef.current && elementContains(subtreeRef.current, event.target as Node), - ); - const isRelatedTargetFromActions = Boolean( - actionsRef.current && elementContains(actionsRef.current, event.relatedTarget as Node), - ); - if (isRelatedTargetFromActions) { - return setActionsVisible(true); - } - if (!isTargetFromSubtree) { - return setActionsVisible(false); - } - }, - [], - ); - - const handleMouseOver = useEventCallback((event: React.MouseEvent) => { - onMouseOver?.(event); - setActionsVisibleIfNotFromSubtree(event); - }); - - const handleFocus = useEventCallback((event: React.FocusEvent) => { - onFocus?.(event); - setActionsVisibleIfNotFromSubtree(event); - }); - - const handleMouseOut = useEventCallback((event: React.MouseEvent) => { - onMouseOut?.(event); - setActionsInvisibleIfNotFromSubtree(event); - }); - const handleBlur = useEventCallback((event: React.FocusEvent) => { - onBlur?.(event); - setActionsInvisibleIfNotFromSubtree(event); - }); - const handleChange = useEventCallback((event: React.ChangeEvent) => { onChange?.(event); if (event.isDefaultPrevented()) { @@ -273,20 +222,23 @@ export function useTreeItem_unstable(props: TreeItemProps, ref: React.Ref ( subtreeRef: React.createRef(), actionsRef: React.createRef(), expandIconRef: React.createRef(), + treeItemRef: React.createRef(), isActionsVisible: true, isAsideVisible: true, itemType: 'leaf', @@ -29,6 +30,11 @@ describe('TreeItemLayout', () => { Component: TreeItemLayout, renderOptions: { wrapper: Wrapper }, displayName: 'TreeItemLayout', + disabledTests: [ + // This is disabled as aside and actions cannot be visible at the same time + 'component-has-static-classnames', + 'component-has-static-classnames-object', + ], requiredProps: { iconAfter: 'iconAfter', iconBefore: 'iconBefore', @@ -39,8 +45,6 @@ describe('TreeItemLayout', () => { }, }); - // TODO add more tests here, and create visual regression tests in /apps/vr-tests - it('renders a default state', () => { const result = render(Default TreeItemLayout); expect(result.container).toMatchSnapshot(); diff --git a/packages/react-components/react-tree/src/components/TreeItemLayout/useTreeItemLayout.tsx b/packages/react-components/react-tree/src/components/TreeItemLayout/useTreeItemLayout.tsx index 0d4e8a981281c7..9e4e302dc11142 100644 --- a/packages/react-components/react-tree/src/components/TreeItemLayout/useTreeItemLayout.tsx +++ b/packages/react-components/react-tree/src/components/TreeItemLayout/useTreeItemLayout.tsx @@ -1,10 +1,12 @@ import * as React from 'react'; import { - ExtractSlotProps, getIntrinsicElementProps, isResolvedShorthand, useMergedRefs, slot, + useEventCallback, + elementContains, + useControllableState, } from '@fluentui/react-utilities'; import { useTreeItemContext_unstable, useTreeContext_unstable } from '../../contexts'; import type { TreeItemLayoutProps, TreeItemLayoutSlots, TreeItemLayoutState } from './TreeItemLayout.types'; @@ -31,20 +33,63 @@ export const useTreeItemLayout_unstable = ( const layoutRef = useTreeItemContext_unstable(ctx => ctx.layoutRef); const selectionMode = useTreeContext_unstable(ctx => ctx.selectionMode); - const [isActionsVisibleExternal, actionsShorthand]: [boolean | undefined, TreeItemLayoutSlots['actions']] = + const [isActionsVisibleFromProps, actionsShorthand]: [boolean | undefined, TreeItemLayoutSlots['actions']] = isResolvedShorthand(props.actions) ? // .visible prop should not be propagated to the DOM [props.actions.visible, { ...props.actions, visible: undefined }] : [undefined, props.actions]; - const isActionsVisible = useTreeItemContext_unstable(ctx => ctx.isActionsVisible) || isActionsVisibleExternal; - const isAsideVisible = useTreeItemContext_unstable(ctx => ctx.isAsideVisible); + const [isActionsVisible, setIsActionsVisible] = useControllableState({ + state: isActionsVisibleFromProps, + initialState: false, + }); + const selectionRef = useTreeItemContext_unstable(ctx => ctx.selectionRef); const expandIconRef = useTreeItemContext_unstable(ctx => ctx.expandIconRef); const actionsRef = useTreeItemContext_unstable(ctx => ctx.actionsRef); + const actionsRefInternal = React.useRef(null); + const treeItemRef = useTreeItemContext_unstable(ctx => ctx.treeItemRef); + const subtreeRef = useTreeItemContext_unstable(ctx => ctx.subtreeRef); const checked = useTreeItemContext_unstable(ctx => ctx.checked); const isBranch = useTreeItemContext_unstable(ctx => ctx.itemType === 'branch'); + // FIXME: Asserting is required here, as converting this to RefObject on context type would be a breaking change + assertIsRefObject(treeItemRef); + // FIXME: Asserting is required here, as converting this to RefObject on context type would be a breaking change + assertIsRefObject(subtreeRef); + + const setActionsVisibleIfNotFromSubtree = React.useCallback( + (event: MouseEvent | FocusEvent) => { + const isTargetFromSubtree = Boolean( + subtreeRef.current && elementContains(subtreeRef.current, event.target as Node), + ); + if (!isTargetFromSubtree) { + setIsActionsVisible(true); + } + }, + [subtreeRef, setIsActionsVisible], + ); + + const setActionsInvisibleIfNotFromSubtree = React.useCallback( + (event: FocusEvent | MouseEvent) => { + const isRelatedTargetFromActions = Boolean( + actionsRefInternal.current && elementContains(actionsRefInternal.current, event.relatedTarget as Node), + ); + if (isRelatedTargetFromActions) { + setIsActionsVisible(true); + return; + } + const isTargetFromSubtree = Boolean( + subtreeRef.current && elementContains(subtreeRef.current, event.target as Node), + ); + if (!isTargetFromSubtree) { + setIsActionsVisible(false); + return; + } + }, + [subtreeRef, setIsActionsVisible], + ); + const expandIcon = slot.optional(props.expandIcon, { renderByDefault: isBranch, defaultProps: { @@ -60,14 +105,54 @@ export const useTreeItemLayout_unstable = ( const arrowNavigationProps = useArrowNavigationGroup({ circular: true, axis: 'horizontal' }); const actions = isActionsVisible ? slot.optional(actionsShorthand, { - defaultProps: { ...arrowNavigationProps, role: 'toolbar' } as ExtractSlotProps, + defaultProps: { ...arrowNavigationProps, role: 'toolbar' }, elementType: 'div', }) : undefined; - const actionsRefs = useMergedRefs(actions?.ref, actionsRef); + const actionsRefs = useMergedRefs(actions?.ref, actionsRef, actionsRefInternal); + const handleActionsBlur = useEventCallback((event: React.FocusEvent) => { + if (isResolvedShorthand(actionsShorthand)) { + actionsShorthand.onBlur?.(event); + } + const isRelatedTargetFromActions = Boolean(elementContains(event.currentTarget, event.relatedTarget as Node)); + setIsActionsVisible(isRelatedTargetFromActions); + }); if (actions) { actions.ref = actionsRefs; + actions.onBlur = handleActionsBlur; } + + const hasActions = Boolean(actionsShorthand); + + React.useEffect(() => { + if (treeItemRef.current && hasActions && isActionsVisibleFromProps === undefined) { + const treeItemElement = treeItemRef.current; + + const handleMouseOver = setActionsVisibleIfNotFromSubtree; + const handleMouseOut = setActionsInvisibleIfNotFromSubtree; + const handleFocus = setActionsVisibleIfNotFromSubtree; + const handleBlur = setActionsInvisibleIfNotFromSubtree; + + treeItemElement.addEventListener('mouseover', handleMouseOver); + treeItemElement.addEventListener('mouseout', handleMouseOut); + treeItemElement.addEventListener('focus', handleFocus); + treeItemElement.addEventListener('blur', handleBlur); + + return () => { + treeItemElement.removeEventListener('mouseover', handleMouseOver); + treeItemElement.removeEventListener('mouseout', handleMouseOut); + treeItemElement.removeEventListener('focus', handleFocus); + treeItemElement.removeEventListener('blur', handleBlur); + }; + } + }, [ + hasActions, + treeItemRef, + isActionsVisibleFromProps, + setActionsVisibleIfNotFromSubtree, + setActionsInvisibleIfNotFromSubtree, + ]); + return { components: { root: 'div', @@ -96,7 +181,7 @@ export const useTreeItemLayout_unstable = ( iconBefore: slot.optional(iconBefore, { defaultProps: { 'aria-hidden': true }, elementType: 'div' }), main: slot.always(main, { elementType: 'div' }), iconAfter: slot.optional(iconAfter, { defaultProps: { 'aria-hidden': true }, elementType: 'div' }), - aside: isAsideVisible + aside: !isActionsVisible ? slot.optional(props.aside, { defaultProps: { 'aria-hidden': true }, elementType: 'div' }) : undefined, actions, @@ -118,3 +203,14 @@ export const useTreeItemLayout_unstable = ( }), }; }; + +function assertIsRefObject(ref?: React.Ref): asserts ref is React.RefObject { + if (process.env.NODE_ENV !== 'production') { + if (typeof ref !== 'object' || ref === null || !('current' in ref)) { + throw new Error(` + @fluentui/react-tree [${useTreeItemLayout_unstable.name}]: + Internal Error: contextual ref is not a RefObject! Please report this bug immediately, as contextual refs should be RefObjects. + `); + } + } +} diff --git a/packages/react-components/react-tree/src/components/TreeItemPersonaLayout/TreeItemPersonaLayout.test.tsx b/packages/react-components/react-tree/src/components/TreeItemPersonaLayout/TreeItemPersonaLayout.test.tsx index 6c88f97e16281a..a600ee8ba9dad5 100644 --- a/packages/react-components/react-tree/src/components/TreeItemPersonaLayout/TreeItemPersonaLayout.test.tsx +++ b/packages/react-components/react-tree/src/components/TreeItemPersonaLayout/TreeItemPersonaLayout.test.tsx @@ -13,6 +13,7 @@ const Wrapper: React.FC = ({ children }) => ( subtreeRef: React.createRef(), actionsRef: React.createRef(), expandIconRef: React.createRef(), + treeItemRef: React.createRef(), isActionsVisible: true, isAsideVisible: true, itemType: 'leaf', @@ -29,6 +30,11 @@ describe('TreeItemPersonaLayout', () => { Component: TreeItemPersonaLayout, renderOptions: { wrapper: Wrapper }, displayName: 'TreeItemPersonaLayout', + disabledTests: [ + // This is disabled as aside and actions cannot be visible at the same time + 'component-has-static-classnames', + 'component-has-static-classnames-object', + ], requiredProps: { description: 'description', expandIcon: 'expandIcon', diff --git a/packages/react-components/react-tree/src/contexts/treeItemContext.ts b/packages/react-components/react-tree/src/contexts/treeItemContext.ts index 58526e81a1f717..a87c1058be7730 100644 --- a/packages/react-components/react-tree/src/contexts/treeItemContext.ts +++ b/packages/react-components/react-tree/src/contexts/treeItemContext.ts @@ -5,13 +5,24 @@ import { headlessTreeRootId } from '../utils/createHeadlessTree'; import { TreeSelectionValue } from '../Tree'; export type TreeItemContextValue = { + /** + * @deprecated - this value is irrelevant for the tree item + */ isActionsVisible: boolean; + /** + * @deprecated - this value is irrelevant for the tree item + */ isAsideVisible: boolean; selectionRef: React.Ref; actionsRef: React.Ref; expandIconRef: React.Ref; layoutRef: React.Ref; + // FIXME: this should be React.RefObject, + // but as it would be a breaking change, we need to keep it as is for now subtreeRef: React.Ref; + // FIXME: this is only marked as optional to avoid breaking changes + // it should always be provided internally + treeItemRef?: React.RefObject; itemType: TreeItemType; value: TreeItemValue; open: boolean; @@ -22,6 +33,7 @@ const defaultContextValue: TreeItemContextValue = { value: headlessTreeRootId, selectionRef: React.createRef(), layoutRef: React.createRef(), + treeItemRef: React.createRef(), subtreeRef: React.createRef(), actionsRef: React.createRef(), expandIconRef: React.createRef(),