Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(a11y): enhance keyboard interaction in search mode #592

Merged
merged 25 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e2112e9
feat(a11y): enhance keyboard interaction in search mode
aojunhao123 Nov 3, 2024
0411b7a
refactor(a11y): synchronize activeKey with search state
aojunhao123 Nov 4, 2024
481c983
fix: lint fix
aojunhao123 Nov 5, 2024
a9efbc7
feat: default active first item
aojunhao123 Nov 5, 2024
9b51e4a
chore: remove useless test case
aojunhao123 Nov 5, 2024
f4f76ab
feat: default active first item
aojunhao123 Nov 5, 2024
b99d2fb
refactor: merge active effect logic
aojunhao123 Nov 5, 2024
644bf9d
chore: adjust code style
aojunhao123 Nov 5, 2024
89900b8
fix: type fix
aojunhao123 Nov 5, 2024
2659bd1
feat: adjust active effect logic
aojunhao123 Nov 5, 2024
bf62a42
fix: lint fix
aojunhao123 Nov 5, 2024
e3ef3e5
chore: remove useless code
aojunhao123 Nov 5, 2024
516d0b8
feat: flatten tree to match first node
aojunhao123 Nov 5, 2024
750de00
chore: add .vscode to gitignore file
aojunhao123 Nov 5, 2024
945f128
feat: improve flatten treeData
aojunhao123 Nov 5, 2024
467b056
feat: improve flatten treeData
aojunhao123 Nov 5, 2024
37d7cfc
chore: adjust code style
aojunhao123 Nov 5, 2024
86cf480
perf: optimize tree node searching with flattened data
aojunhao123 Nov 5, 2024
134f4f4
chore: add comment
aojunhao123 Nov 6, 2024
5d29cc0
revert: restore recursive node search
aojunhao123 Nov 6, 2024
4640c4d
chore: remove unnecessary logic
aojunhao123 Nov 6, 2024
229f4f7
chore: remove unnecessary logic
aojunhao123 Nov 6, 2024
9c7ad27
chore: adjust logic
aojunhao123 Nov 6, 2024
6f5a84c
test: use testing-library
aojunhao123 Nov 7, 2024
97bde6c
fix: keep active matched item when search
aojunhao123 Nov 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 73 additions & 29 deletions src/OptionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
(prev, next) => next[0] && prev[1] !== next[1],
);

// ========================== Active ==========================
// ========================== Active Key Effect ==========================
const [activeKey, setActiveKey] = React.useState<Key>(null);
const activeEntity = keyEntities[activeKey as SafeKey];

Expand All @@ -92,16 +92,34 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
};
}, [checkable, checkedKeys, halfCheckedKeys]);

// ========================== Scroll ==========================
// ========================== Scroll Effect ==========================
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
React.useEffect(() => {
// Single mode should scroll to current key
if (open && !multiple && checkedKeys.length) {
treeRef.current?.scrollTo({ key: checkedKeys[0] });
setActiveKey(checkedKeys[0]);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [open]);

// ========================== Events ==========================
const onListMouseDown: React.MouseEventHandler<HTMLDivElement> = event => {
event.preventDefault();
};

const onInternalSelect = (__: Key[], info: TreeEventInfo) => {
const { node } = info;

if (checkable && isCheckDisabled(node)) {
return;
}

onSelect(node.key, {
selected: !checkedKeys.includes(node.key),
});

if (!multiple) {
toggleOpen(false);
}
};

// ========================== Search ==========================
const lowerSearchValue = String(searchValue).toLowerCase();
const filterTreeNode = (treeNode: EventDataNode<any>) => {
Expand All @@ -122,13 +140,6 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
return searchValue ? searchExpandedKeys : expandedKeys;
}, [expandedKeys, searchExpandedKeys, treeExpandedKeys, searchValue]);

React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchValue]);

const onInternalExpand = (keys: Key[]) => {
setExpandedKeys(keys);
setSearchExpandedKeys(keys);
Expand All @@ -138,26 +149,59 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
}
};

// ========================== Events ==========================
const onListMouseDown: React.MouseEventHandler<HTMLDivElement> = event => {
event.preventDefault();
// ========================== Search Effect ==========================
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
}, [searchValue]);

// ========================== Get First Selectable Node ==========================
const getFirstMatchingNode = (
nodes: EventDataNode<any>[],
predicate: (node: EventDataNode<any>) => boolean,
): EventDataNode<any> | null => {
for (const node of nodes) {
if (predicate(node)) {
return node;
}
if (node[fieldNames.children]) {
const matchInChildren = getFirstMatchingNode(node[fieldNames.children], predicate);
if (matchInChildren) {
return matchInChildren;
}
}
}
return null;
};

const onInternalSelect = (__: Key[], info: TreeEventInfo) => {
const { node } = info;
const getFirstSelectableNode = (nodes: EventDataNode<any>): EventDataNode<any> | null =>
getFirstMatchingNode(nodes, node => node.selectable !== false && !node.disabled);

if (checkable && isCheckDisabled(node)) {
const getFirstMatchNode = (nodes: EventDataNode<any>): EventDataNode<any> | null =>
getFirstMatchingNode(nodes, node => filterTreeNode(node) && !node.disabled);

// ========================== Active Key Effect ==========================
React.useEffect(() => {
if (searchValue) {
const firstMatchNode = getFirstMatchNode(memoTreeData);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有没有 searchValue 你的 matchNode 逻辑应该都是一样的。要么都是 selectable 的,要么都是非 disabled 的。可以考虑直接从 virtualList 里获取,这样也不需要递归,并且给出来的 TreeNode 本来就是过滤完的。

setActiveKey(firstMatchNode ? firstMatchNode[fieldNames.value] : null);
return;
}

onSelect(node.key, {
selected: !checkedKeys.includes(node.key),
});

if (!multiple) {
toggleOpen(false);
if (open) {
if (!multiple && checkedKeys.length) {
setActiveKey(checkedKeys[0]);
} else {
const firstSelectableNode = getFirstSelectableNode(memoTreeData);
if (firstSelectableNode) {
setActiveKey(firstSelectableNode[fieldNames.value]);
}
}
return;
}
};
setActiveKey(null);
}, [open, searchValue]);

// ========================= Keyboard =========================
React.useImperativeHandle(ref, () => ({
Expand All @@ -176,8 +220,8 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
// >>> Select item
case KeyCode.ENTER: {
if (activeEntity) {
const { selectable, value } = activeEntity?.node || {};
if (selectable !== false) {
const { selectable, value, disabled } = activeEntity?.node || {};
if (selectable !== false && !disabled) {
onInternalSelect(null, {
node: { key: activeKey },
selected: !checkedKeys.includes(value),
Expand All @@ -197,10 +241,10 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
}));

const loadDataFun = useMemo(
() => searchValue ? null : (loadData as any),
() => (searchValue ? null : (loadData as any)),
[searchValue, treeExpandedKeys || expandedKeys],
([preSearchValue], [nextSearchValue, nextExcludeSearchExpandedKeys]) =>
preSearchValue !== nextSearchValue && !!(nextSearchValue || nextExcludeSearchExpandedKeys)
preSearchValue !== nextSearchValue && !!(nextSearchValue || nextExcludeSearchExpandedKeys),
);

// ========================== Render ==========================
Expand Down
93 changes: 93 additions & 0 deletions tests/Select.SearchInput.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import React, { useState } from 'react';
import { mount } from 'enzyme';
import TreeSelect, { TreeNode } from '../src';
import KeyCode from 'rc-util/lib/KeyCode';

describe('TreeSelect.SearchInput', () => {
it('select item will clean searchInput', () => {
Expand Down Expand Up @@ -198,4 +199,96 @@ describe('TreeSelect.SearchInput', () => {
nodes.first().simulate('click');
expect(called).toBe(1);
});

describe('keyboard events', () => {
it('should select first matched node when press enter', () => {
const onSelect = jest.fn();
const wrapper = mount(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

用 testing lib

<TreeSelect
showSearch
onSelect={onSelect}
open
treeData={[
{ value: '1', label: '1' },
{ value: '2', label: '2', disabled: true },
{ value: '3', label: '3' },
]}
/>,
);

// Search and press enter, should select first matched non-disabled node
wrapper.search('1');
wrapper.find('input').first().simulate('keyDown', { which: KeyCode.ENTER });
expect(onSelect).toHaveBeenCalledWith('1', expect.anything());

onSelect.mockReset();

// Search disabled node and press enter, should not select
wrapper.search('2');
wrapper.find('input').first().simulate('keyDown', { which: KeyCode.ENTER });
expect(onSelect).not.toHaveBeenCalled();
});

it('should not select node when no matches found', () => {
const onSelect = jest.fn();
const wrapper = mount(
<TreeSelect
showSearch
onSelect={onSelect}
open
treeData={[
{ value: '1', label: '1' },
{ value: '2', label: '2' },
]}
/>,
);

// Search non-existent value and press enter, should not select any node
wrapper.search('not-exist');
wrapper.find('input').first().simulate('keyDown', { which: KeyCode.ENTER });
expect(onSelect).not.toHaveBeenCalled();
});

it('should ignore enter press when all matched nodes are disabled', () => {
const onSelect = jest.fn();
const wrapper = mount(
<TreeSelect
showSearch
onSelect={onSelect}
open
treeData={[
{ value: '1', label: '1', disabled: true },
{ value: '2', label: '2', disabled: true },
]}
/>,
);

// When all matched nodes are disabled, press enter should not select any node
wrapper.search('1');
wrapper.find('input').first().simulate('keyDown', { which: KeyCode.ENTER });
expect(onSelect).not.toHaveBeenCalled();
});

it('should activate first matched node when searching', () => {
const wrapper = mount(
<TreeSelect
showSearch
open
treeData={[
{ value: '1', label: '1' },
{ value: '2', label: '2', disabled: true },
{ value: '3', label: '3' },
]}
/>,
);

// When searching, first matched non-disabled node should be activated
wrapper.search('1');
expect(wrapper.find('.rc-tree-select-tree-treenode-active').text()).toBe('1');

// Should skip disabled nodes
wrapper.search('2');
expect(wrapper.find('.rc-tree-select-tree-treenode-active').length).toBe(0);
});
});
});
44 changes: 23 additions & 21 deletions tests/Select.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -438,13 +438,13 @@ describe('TreeSelect.basic', () => {
keyUp(KeyCode.DOWN);
keyDown(KeyCode.ENTER);
keyUp(KeyCode.ENTER);
matchValue(['parent']);
matchValue(['child']);

keyDown(KeyCode.UP);
keyUp(KeyCode.UP);
keyDown(KeyCode.ENTER);
keyUp(KeyCode.ENTER);
matchValue(['parent', 'child']);
matchValue(['child', 'parent']);
});

it('selectable works with keyboard operations', () => {
Expand All @@ -467,12 +467,12 @@ describe('TreeSelect.basic', () => {

keyDown(KeyCode.DOWN);
keyDown(KeyCode.ENTER);
expect(onChange).toHaveBeenCalledWith(['parent'], expect.anything(), expect.anything());
onChange.mockReset();
expect(onChange).not.toHaveBeenCalled();

keyDown(KeyCode.UP);
keyDown(KeyCode.ENTER);
expect(onChange).not.toHaveBeenCalled();
expect(onChange).toHaveBeenCalledWith(['parent'], expect.anything(), expect.anything());
onChange.mockReset();
});

it('active index matches value', () => {
Expand Down Expand Up @@ -535,6 +535,24 @@ describe('TreeSelect.basic', () => {
keyDown(KeyCode.UP);
expect(wrapper.find('.rc-tree-select-tree-treenode-active').text()).toBe('11 label');
});

it('should active first un-disabled option when dropdown is opened', () => {
const treeData = [
{ key: '0', value: '0', title: '0 label', disabled: true },
{ key: '1', value: '1', title: '1 label' },
{ key: '2', value: '2', title: '2 label' },
];

const wrapper = mount(<TreeSelect treeData={treeData} />);

expect(wrapper.find('.rc-tree-select-tree-treenode-active')).toHaveLength(0);

wrapper.openSelect();

const activeNode = wrapper.find('.rc-tree-select-tree-treenode-active');
expect(activeNode).toHaveLength(1);
expect(activeNode.text()).toBe('1 label');
});
});

it('click in list should preventDefault', () => {
Expand Down Expand Up @@ -591,22 +609,6 @@ describe('TreeSelect.basic', () => {
expect(container.querySelector('.rc-tree-select-selector').textContent).toBe('parent 1-0');
});

it('should not add new tag when key enter is pressed if nothing is active', () => {
const onSelect = jest.fn();

const wrapper = mount(
<TreeSelect open treeDefaultExpandAll multiple onSelect={onSelect}>
<TreeNode value="parent 1-0" title="parent 1-0">
<TreeNode value="leaf1" title="my leaf" disabled />
<TreeNode value="leaf2" title="your leaf" disabled />
</TreeNode>
</TreeSelect>,
);

wrapper.find('input').first().simulate('keydown', { which: KeyCode.ENTER });
expect(onSelect).not.toHaveBeenCalled();
});

it('should not select parent if some children is disabled', () => {
const onChange = jest.fn();

Expand Down
Loading