From f1ca11b37b63d5ee403cedce07080949901300b5 Mon Sep 17 00:00:00 2001 From: Tony Hallett Date: Sun, 22 Mar 2020 11:38:47 +0000 Subject: [PATCH 1/3] [TreeView] fix focus steal --- .../src/TreeItem/TreeItem.test.js | 27 +++++++++++++++++++ .../material-ui-lab/src/TreeView/TreeView.js | 5 ++++ 2 files changed, 32 insertions(+) diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js index 734e82968b2fdc..20534e5ed1aa75 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js @@ -982,4 +982,31 @@ describe('', () => { fireEvent(input, keydownEvent); expect(keydownEvent.preventDefault.callCount).to.equal(0); }); + + it('should not focus steal', () => { + function TestComponent() { + const [hide, setHide] = React.useState(false); + + return ( + + + + + {!hide ? : } + + + ); + } + + const { getByText, getByTestId, getByRole } = render(); + fireEvent.click(getByText('two')); + expect(getByTestId('two')).to.have.focus; + getByRole('button').focus(); + expect(getByRole('button')).to.have.focus; + fireEvent.click(getByRole('button')); + fireEvent.click(getByRole('button')); + expect(getByRole('button')).to.have.focus; + }); }); diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index b60bd1801900f5..d494e8a667bbe1 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -483,6 +483,11 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { } }, [expanded, childrenCalculated, isExpanded]); + React.useEffect(() => { + if (focused && !nodeMap.current[focused]) { + setFocused(null); + } + }, [children, focused, setFocused, nodeMap]); return ( Date: Sun, 22 Mar 2020 12:27:51 +0000 Subject: [PATCH 2/3] change side effect for derived state --- packages/material-ui-lab/src/TreeView/TreeView.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index d494e8a667bbe1..ee162fc11074b7 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -59,6 +59,11 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const [focused, setFocused] = React.useState(null); const nodeMap = React.useRef({}); + + if (focused && !nodeMap.current[focused]) { + setFocused(null); + } + const firstCharMap = React.useRef({}); const visibleNodes = React.useRef([]); @@ -483,11 +488,6 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { } }, [expanded, childrenCalculated, isExpanded]); - React.useEffect(() => { - if (focused && !nodeMap.current[focused]) { - setFocused(null); - } - }, [children, focused, setFocused, nodeMap]); return ( Date: Fri, 27 Mar 2020 18:21:42 +0100 Subject: [PATCH 3/3] refactor test and impl - groups statements into arrange-act-assert - removes indirection in act (a click does what?) - moves from gDSFP to effect during mutation --- .../src/TreeItem/TreeItem.test.js | 49 ++++++++++++------- .../material-ui-lab/src/TreeView/TreeView.js | 19 ++++--- 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js index b3f5ab843d73d0..4f04fb1e8fda38 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js @@ -3,7 +3,7 @@ import { expect } from 'chai'; import { spy } from 'sinon'; import { createMount, getClasses } from '@material-ui/core/test-utils'; import describeConformance from '@material-ui/core/test-utils/describeConformance'; -import { createEvent, createClientRender, fireEvent } from 'test/utils/createClientRender'; +import { act, createEvent, createClientRender, fireEvent } from 'test/utils/createClientRender'; import TreeItem from './TreeItem'; import TreeView from '../TreeView'; @@ -1007,29 +1007,42 @@ describe('', () => { }); it('should not focus steal', () => { - function TestComponent() { - const [hide, setHide] = React.useState(false); - - return ( - - - - - {!hide ? : } - - - ); + let setActiveItemMounted; + // a TreeItem whose mounted state we can control with `setActiveItemMounted` + function ControlledTreeItem(props) { + const [mounted, setMounted] = React.useState(true); + setActiveItemMounted = setMounted; + + if (!mounted) { + return null; + } + return ; } + const { getByText, getByTestId, getByRole } = render( + + + + + + + , + ); - const { getByText, getByTestId, getByRole } = render(); fireEvent.click(getByText('two')); + expect(getByTestId('two')).to.have.focus; + getByRole('button').focus(); + expect(getByRole('button')).to.have.focus; - fireEvent.click(getByRole('button')); - fireEvent.click(getByRole('button')); + + act(() => { + setActiveItemMounted(false); + }); + act(() => { + setActiveItemMounted(true); + }); + expect(getByRole('button')).to.have.focus; }); }); diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index ee162fc11074b7..a1d1491925d16b 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -56,14 +56,10 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { ...other } = props; const [tabbable, setTabbable] = React.useState(null); - const [focused, setFocused] = React.useState(null); + const [focusedNodeId, setFocusedNodeId] = React.useState(null); const nodeMap = React.useRef({}); - if (focused && !nodeMap.current[focused]) { - setFocused(null); - } - const firstCharMap = React.useRef({}); const visibleNodes = React.useRef([]); @@ -93,7 +89,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { ); const isTabbable = (id) => tabbable === id; - const isFocused = (id) => focused === id; + const isFocused = (id) => focusedNodeId === id; /* * Node Helpers @@ -134,7 +130,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const focus = (id) => { if (id) { setTabbable(id); - setFocused(id); + setFocusedNodeId(id); } }; @@ -186,7 +182,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { * Expansion Helpers */ - const toggleExpansion = (event, value = focused) => { + const toggleExpansion = (event, value = focusedNodeId) => { let newExpanded; if (expanded.indexOf(value) !== -1) { newExpanded = expanded.filter((id) => id !== value); @@ -436,6 +432,13 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { } }); nodeMap.current = newMap; + + setFocusedNodeId((oldFocusedNodeId) => { + if (oldFocusedNodeId === id) { + return null; + } + return oldFocusedNodeId; + }); }, [getNodesToRemove], );