From bcf578943178b6130e2f2232e7c942e07a711c75 Mon Sep 17 00:00:00 2001
From: joshwooding <12938082+joshwooding@users.noreply.github.com>
Date: Sun, 26 Jul 2020 22:59:26 +0100
Subject: [PATCH] Prevent programmatic focus, add more tests and make disabled
styling less specific
---
.../pages/components/tree-view/tree-view.md | 2 +-
.../material-ui-lab/src/TreeItem/TreeItem.js | 19 +-
.../src/TreeItem/TreeItem.test.js | 276 ++++++++++--------
.../material-ui-lab/src/TreeView/TreeView.js | 1 +
4 files changed, 166 insertions(+), 132 deletions(-)
diff --git a/docs/src/pages/components/tree-view/tree-view.md b/docs/src/pages/components/tree-view/tree-view.md
index 6e45e84e5d41eb..be6fe4ebce20ae 100644
--- a/docs/src/pages/components/tree-view/tree-view.md
+++ b/docs/src/pages/components/tree-view/tree-view.md
@@ -66,7 +66,7 @@ The behaviour of disabled tree items depends on the `disabledItemsFocusable` pro
If it is false:
- Mouse clicks will not focus disabled items.
-- Arrow keys will focus disabled items and, the next non-disabled item will be focused.
+- Arrow keys will not focus disabled items and, the next non-disabled item will be focused.
- Typing the first character of a disabled item's label will not focus the item.
- Mouse or keyboard interaction will not expand/collapse disabled items.
- Mouse or keyboard interaction will not select disabled items.
diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js
index 3e12c3f6976c1a..05cece7b48d87b 100644
--- a/packages/material-ui-lab/src/TreeItem/TreeItem.js
+++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js
@@ -37,6 +37,10 @@ export const styles = (theme) => ({
backgroundColor: 'transparent',
},
},
+ '&$disabled': {
+ opacity: theme.palette.action.disabledOpacity,
+ backgroundColor: 'transparent',
+ },
'&$focused': {
backgroundColor: theme.palette.action.focus,
},
@@ -59,12 +63,6 @@ export const styles = (theme) => ({
),
},
},
- '&$disabled': {
- opacity: theme.palette.action.disabledOpacity,
- '&:hover': {
- backgroundColor: 'transparent',
- },
- },
},
/* Pseudo-class applied to the content element when expanded. */
expanded: {},
@@ -126,6 +124,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
isSelected,
isDisabled,
multiSelect,
+ disabledItemsFocusable,
mapFirstChar,
unMapFirstChar,
registerNode,
@@ -209,7 +208,9 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
}
};
+ const wasClick = React.useRef(false);
const handleMouseDown = (event) => {
+ wasClick.current = true;
if (event.shiftKey || event.ctrlKey || event.metaKey) {
// Prevent text selection
event.preventDefault();
@@ -276,9 +277,11 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
tree.focus();
}
- if (!focused && event.currentTarget === event.target) {
+ const unfocusable = !disabledItemsFocusable && disabled;
+ if (!wasClick.current && !focused && event.currentTarget === event.target && !unfocusable) {
focus(event, nodeId);
}
+ wasClick.current = false;
};
// Using focusin to avoid blurring the tree.
@@ -289,7 +292,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
};
}
return undefined;
- }, [focus, focused, nodeId, nodeRef, treeId]);
+ }, [focus, focused, nodeId, nodeRef, treeId, disabledItemsFocusable, disabled]);
return (
', () => {
});
describe('keyboard', () => {
- it('should prevent selection by keyboard', () => {
- const { getByRole, getByTestId } = render(
-
-
- ,
- );
+ describe('`disabledItemsFocusable=true`', () => {
+ it('should prevent selection by keyboard', () => {
+ const { getByRole, getByTestId } = render(
+
+
+ ,
+ );
- fireEvent.focusIn(getByTestId('one'));
- expect(getByTestId('one')).toHaveVirtualFocus();
- fireEvent.keyDown(getByRole('tree'), { key: ' ' });
- expect(getByTestId('one')).not.to.have.attribute('aria-selected');
+ fireEvent.focusIn(getByTestId('one'));
+ expect(getByTestId('one')).toHaveVirtualFocus();
+ fireEvent.keyDown(getByRole('tree'), { key: ' ' });
+ expect(getByTestId('one')).not.to.have.attribute('aria-selected');
+ });
+
+ it('should not prevent next node being range selected by keyboard', () => {
+ const { getByRole, getByTestId } = render(
+
+
+
+
+
+ ,
+ );
+
+ fireEvent.focusIn(getByTestId('one'));
+ expect(getByTestId('one')).toHaveVirtualFocus();
+ fireEvent.keyDown(getByRole('tree'), { key: 'ArrowDown', shiftKey: true });
+ expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
+ expect(getByTestId('two')).to.have.attribute('aria-selected', 'true');
+ expect(getByTestId('two')).toHaveVirtualFocus();
+ });
+
+ it('should prevent range selection by keyboard + arrow down', () => {
+ const { getByRole, getByTestId } = render(
+
+
+
+ ,
+ );
+
+ fireEvent.focusIn(getByTestId('one'));
+ expect(getByTestId('one')).toHaveVirtualFocus();
+ fireEvent.keyDown(getByRole('tree'), { key: 'ArrowDown', shiftKey: true });
+ expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
+ expect(getByTestId('two')).to.have.attribute('aria-selected', 'false');
+ expect(getByTestId('two')).toHaveVirtualFocus();
+ });
});
- it('should not prevent next node being range selected by keyboard', () => {
- const { getByRole, getByTestId } = render(
-
-
-
-
-
- ,
- );
+ describe('`disabledItemsFocusable=false`', () => {
+ it('should select the next non disabled node by keyboard + arrow down', () => {
+ const { getByRole, getByTestId } = render(
+
+
+
+
+ ,
+ );
- fireEvent.focusIn(getByTestId('one'));
- expect(getByTestId('one')).toHaveVirtualFocus();
- fireEvent.keyDown(getByRole('tree'), { key: 'ArrowDown', shiftKey: true });
- expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
- expect(getByTestId('two')).to.have.attribute('aria-selected', 'true');
- expect(getByTestId('two')).toHaveVirtualFocus();
+ fireEvent.focusIn(getByTestId('one'));
+ expect(getByTestId('one')).toHaveVirtualFocus();
+ fireEvent.keyDown(getByRole('tree'), { key: 'ArrowDown', shiftKey: true });
+ expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
+ expect(getByTestId('two')).to.have.attribute('aria-selected', 'false');
+ expect(getByTestId('three')).toHaveVirtualFocus();
+ expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
+ expect(getByTestId('two')).to.have.attribute('aria-selected', 'false');
+ expect(getByTestId('three')).to.have.attribute('aria-selected', 'true');
+ });
});
it('should prevent range selection by keyboard + space', () => {
@@ -1704,63 +1744,36 @@ describe('', () => {
expect(getByTestId('four')).to.have.attribute('aria-selected', 'true');
expect(getByTestId('five')).to.have.attribute('aria-selected', 'true');
});
-
- specify(
- 'if `disabledItemsFocusable=true` should prevent range selection by keyboard + arrow down',
- () => {
- const { getByRole, getByTestId } = render(
-
-
-
- ,
- );
-
- fireEvent.focusIn(getByTestId('one'));
- expect(getByTestId('one')).toHaveVirtualFocus();
- fireEvent.keyDown(getByRole('tree'), { key: 'ArrowDown', shiftKey: true });
- expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
- expect(getByTestId('two')).to.have.attribute('aria-selected', 'false');
- expect(getByTestId('two')).toHaveVirtualFocus();
- },
- );
-
- specify(
- 'if `disabledItemsFocusable=false` should select the next non disabled node by keyboard + arrow down',
- () => {
- const { getByRole, getByTestId } = render(
-
-
-
-
- ,
- );
-
- fireEvent.focusIn(getByTestId('one'));
- expect(getByTestId('one')).toHaveVirtualFocus();
- fireEvent.keyDown(getByRole('tree'), { key: 'ArrowDown', shiftKey: true });
- expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
- expect(getByTestId('two')).to.have.attribute('aria-selected', 'false');
- expect(getByTestId('three')).toHaveVirtualFocus();
- expect(getByTestId('one')).to.have.attribute('aria-selected', 'false');
- expect(getByTestId('two')).to.have.attribute('aria-selected', 'false');
- expect(getByTestId('three')).to.have.attribute('aria-selected', 'true');
- },
- );
});
});
describe('focus', () => {
describe('`disabledItemsFocusable=true`', () => {
it('should prevent focus by mouse', () => {
- const { getByText, getByTestId } = render(
+ const { getByRole, getByText, getByTestId } = render(
+
+ ,
+ );
+
+ fireEvent.click(getByText('two'));
+ act(() => {
+ getByRole('tree').focus();
+ });
+ expect(getByTestId('two')).not.toHaveVirtualFocus();
+ });
+
+ it('should not prevent programmatic focus', () => {
+ const { getByText, getByTestId } = render(
+
+
,
);
- fireEvent.click(getByText('one'));
- expect(getByTestId('one')).not.toHaveVirtualFocus();
+ fireEvent.focusIn(getByText('one'));
+ expect(getByTestId('one')).toHaveVirtualFocus();
});
it('should not prevent focus by type-ahead', () => {
@@ -1815,14 +1828,29 @@ describe('', () => {
describe('`disabledItemsFocusable=false`', () => {
it('should prevent focus by mouse', () => {
- const { getByText, getByTestId } = render(
+ const { getByRole, getByText, getByTestId } = render(
-
+
,
);
fireEvent.click(getByText('one'));
+ act(() => {
+ getByRole('tree').focus();
+ });
+ expect(getByTestId('one')).not.toHaveVirtualFocus();
+ });
+
+ it('should prevent programmatic focus', () => {
+ const { getByText, getByTestId } = render(
+
+
+
+ ,
+ );
+
+ fireEvent.focusIn(getByText('one'));
expect(getByTestId('one')).not.toHaveVirtualFocus();
});
@@ -1879,68 +1907,70 @@ describe('', () => {
});
describe('expansion', () => {
- it('should prevent expansion on click', () => {
- const { getByText, getByTestId } = render(
-
-
-
-
- ,
- );
+ describe('`disabledItemsFocusable=true`', () => {
+ it('should prevent expansion on enter', () => {
+ const { getByRole, getByTestId } = render(
+
+
+
+
+
+ ,
+ );
- fireEvent.click(getByText('one'));
- expect(getByTestId('one')).to.have.attribute('aria-expanded', 'false');
- });
+ fireEvent.focusIn(getByTestId('two'));
+ expect(getByTestId('two')).toHaveVirtualFocus();
+ expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
+ fireEvent.keyDown(getByRole('tree'), { key: 'Enter' });
+ expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
+ });
- it('should prevent expansion on enter', () => {
- const { getByRole, getByTestId } = render(
-
-
-
-
-
- ,
- );
+ it('should prevent expansion on right arrow', () => {
+ const { getByRole, getByTestId } = render(
+
+
+
+
+
+ ,
+ );
- fireEvent.focusIn(getByTestId('two'));
- expect(getByTestId('two')).toHaveVirtualFocus();
- expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
- fireEvent.keyDown(getByRole('tree'), { key: 'Enter' });
- expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
- });
+ fireEvent.focusIn(getByTestId('two'));
+ expect(getByTestId('two')).toHaveVirtualFocus();
+ expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
+ fireEvent.keyDown(getByRole('tree'), { key: 'ArrowRight' });
+ expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
+ });
- it('should prevent expansion on right arrow', () => {
- const { getByRole, getByTestId } = render(
-
-
-
-
-
- ,
- );
+ it('should prevent collapse on left arrow', () => {
+ const { getByRole, getByTestId } = render(
+
+
+
+
+
+ ,
+ );
- fireEvent.focusIn(getByTestId('two'));
- expect(getByTestId('two')).toHaveVirtualFocus();
- expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
- fireEvent.keyDown(getByRole('tree'), { key: 'ArrowRight' });
- expect(getByTestId('two')).to.have.attribute('aria-expanded', 'false');
+ fireEvent.focusIn(getByTestId('two'));
+ expect(getByTestId('two')).toHaveVirtualFocus();
+ expect(getByTestId('two')).to.have.attribute('aria-expanded', 'true');
+ fireEvent.keyDown(getByRole('tree'), { key: 'ArrowLeft' });
+ expect(getByTestId('two')).to.have.attribute('aria-expanded', 'true');
+ });
});
- it('should prevent collapse on left arrow', () => {
- const { getByRole, getByTestId } = render(
-
-
-
-
+ it('should prevent expansion on click', () => {
+ const { getByText, getByTestId } = render(
+
+
+
,
);
- fireEvent.focusIn(getByTestId('two'));
- expect(getByTestId('two')).toHaveVirtualFocus();
- expect(getByTestId('two')).to.have.attribute('aria-expanded', 'true');
- fireEvent.keyDown(getByRole('tree'), { key: 'ArrowLeft' });
- expect(getByTestId('two')).to.have.attribute('aria-expanded', 'true');
+ fireEvent.click(getByText('one'));
+ expect(getByTestId('one')).to.have.attribute('aria-expanded', 'false');
});
});
diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js
index 351599c73ab7c1..3ca22a9f18aff6 100644
--- a/packages/material-ui-lab/src/TreeView/TreeView.js
+++ b/packages/material-ui-lab/src/TreeView/TreeView.js
@@ -767,6 +767,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) {
selectNode: disableSelection ? noopSelection : selectNode,
selectRange: disableSelection ? noopSelection : selectRange,
multiSelect,
+ disabledItemsFocusable,
mapFirstChar,
unMapFirstChar,
registerNode,