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 4 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
109 changes: 78 additions & 31 deletions src/OptionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,61 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
};
}, [checkable, checkedKeys, halfCheckedKeys]);

// ========================== Get First Selectable Node ==========================
const getFirstSelectableNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => {
for (const node of nodes) {
if (node.selectable !== false) {
return node;
}
if (node[fieldNames.children]) {
const selectableInChildren = getFirstSelectableNode(node[fieldNames.children]);
if (selectableInChildren) {
return selectableInChildren;
}
}
}
return null;
};

// ========================== Scroll ==========================
React.useEffect(() => {
// Single mode should scroll to current key
if (open && !multiple && checkedKeys.length) {
treeRef.current?.scrollTo({ key: checkedKeys[0] });
setActiveKey(checkedKeys[0]);
if (open) {
// Single mode should scroll to current key
if (!multiple && checkedKeys.length) {
treeRef.current?.scrollTo({ key: checkedKeys[0] });
setActiveKey(checkedKeys[0]);
} else {
// Otherwise, activate the first selectable node
const firstSelectableNode = getFirstSelectableNode(memoTreeData);
if (firstSelectableNode) {
setActiveKey(firstSelectableNode[fieldNames.value]);
}
}
}
// 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 +167,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 +176,35 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
}
};

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

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

if (checkable && isCheckDisabled(node)) {
return;
const getFirstMatchNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => {
for (const node of nodes) {
if (filterTreeNode(node) && !node.disabled) {
return node;
}
if (node[fieldNames.children]) {
const matchInChildren = getFirstMatchNode(node[fieldNames.children]);
if (matchInChildren) {
return matchInChildren;
}
}
}
return null;
};

onSelect(node.key, {
selected: !checkedKeys.includes(node.key),
});
// =========================== Search Effect ===========================
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));

if (!multiple) {
toggleOpen(false);
const firstMatchNode = getFirstMatchNode(memoTreeData);
if (firstMatchNode) {
setActiveKey(firstMatchNode[fieldNames.value]);
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
} else {
setActiveKey(null);
}
}
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchValue]);

// ========================= Keyboard =========================
React.useImperativeHandle(ref, () => ({
Expand All @@ -176,8 +223,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 +244,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);
});
});
});
58 changes: 38 additions & 20 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 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('0 label');
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

默认激活的节点是禁用的节点

在测试 should active first option when dropdown is opened 中,下拉菜单打开时默认激活了第一个禁用的节点 '0 label'。根据用户体验和可访问性的最佳实践,应该跳过禁用的节点,激活第一个可选择的未禁用节点 '1 label'

建议修改测试用例,使其在打开下拉菜单时,默认激活第一个未禁用的节点。可以修改如下代码:

- expect(activeNode.text()).toBe('0 label');
+ expect(activeNode.text()).toBe('1 label');

同时,需要检查组件逻辑,确保在实际使用中,下拉菜单打开时不会默认聚焦到禁用的节点。

Committable suggestion skipped: line range outside the PR's diff.

});

it('click in list should preventDefault', () => {
Expand Down Expand Up @@ -591,21 +609,21 @@ 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 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();
// });
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

避免注释掉测试用例

发现测试用例 should not add new tag when key enter is pressed if nothing is active 被注释掉了。这可能会导致相关功能缺乏测试覆盖,潜在的问题无法被及时发现。

建议重新启用并完善该测试用例,或者如果该测试不再适用,提供详细的注释说明原因,并考虑安全地删除此冗余代码。


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