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 22 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ dist
build
lib
coverage
.vscode
yarn.lock
package-lock.json
Comment on lines 29 to 30
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议统一使用单一包管理器

同时忽略多个包管理器的锁文件(yarn.lock、package-lock.json、bun.lockb)可能导致团队成员使用不同的包管理器,这可能引起依赖版本不一致的问题。建议:

  1. 选择一个官方推荐的包管理器
  2. 只保留该包管理器的锁文件
  3. 在项目文档中明确规定使用的包管理器

Also applies to: 38-38

es
Expand Down
111 changes: 77 additions & 34 deletions src/OptionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,6 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
(prev, next) => next[0] && prev[1] !== next[1],
);

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

// ========================== Values ==========================
const mergedCheckedKeys = React.useMemo(() => {
if (!checkable) {
Expand All @@ -97,18 +93,29 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
// 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]);

// ========================== Search ==========================
const lowerSearchValue = String(searchValue).toLowerCase();
const filterTreeNode = (treeNode: EventDataNode<any>) => {
if (!lowerSearchValue) {
return false;
// ========================== 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);
}
return String(treeNode[treeNodeFilterProp]).toLowerCase().includes(lowerSearchValue);
};

// =========================== Keys ===========================
Expand All @@ -122,13 +129,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 +138,69 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
}
};

// ========================== Events ==========================
const onListMouseDown: React.MouseEventHandler<HTMLDivElement> = event => {
event.preventDefault();
// ========================== Search ==========================
const lowerSearchValue = String(searchValue).toLowerCase();
const filterTreeNode = (treeNode: EventDataNode<any>) => {
if (!lowerSearchValue) {
return false;
}
return String(treeNode[treeNodeFilterProp]).toLowerCase().includes(lowerSearchValue);
};

const onInternalSelect = (__: Key[], info: TreeEventInfo) => {
const { node } = info;
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchValue]);
Comment on lines +150 to +155
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

需要补充 useEffect 的依赖项

当前的依赖数组缺少 treeDatafieldNames,这可能导致搜索结果不准确。

  React.useEffect(() => {
    if (searchValue) {
      setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
    }
-  }, [searchValue]);
+  }, [searchValue, treeData, fieldNames]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

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


if (checkable && isCheckDisabled(node)) {
// ========================== Get First Selectable Node ==========================
const getFirstMatchingNode = (nodes: EventDataNode<any>[]): EventDataNode<any> | null => {
for (const node of nodes) {
if (!node.disabled && node.selectable !== false) {
return node;
}

if (node[fieldNames.children]) {
const matchInChildren = getFirstMatchingNode(node[fieldNames.children]);
zombieJ marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +172 to +173
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

需要增加空值检查

访问 node[fieldNames.children] 时缺少必要的空值检查,可能导致运行时错误。

建议修改如下:

- if (node[fieldNames.children]) {
+ if (node?.[fieldNames.children]?.length) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (node[fieldNames.children]) {
const matchInChildren = getFirstMatchingNode(node[fieldNames.children]);
if (node?.[fieldNames.children]?.length) {
const matchInChildren = getFirstMatchingNode(node[fieldNames.children]);

if (matchInChildren) {
return matchInChildren;
}
}
}
return null;
};

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

React.useEffect(() => {
if (!open) {
return;
}
let nextActiveKey = null;

onSelect(node.key, {
selected: !checkedKeys.includes(node.key),
});
const getFirstNode = () => {
const firstNode = getFirstMatchingNode(memoTreeData);
return firstNode ? firstNode[fieldNames.value] : null;
};

if (!multiple) {
toggleOpen(false);
// search mode active first node
if (searchValue) {
nextActiveKey = getFirstNode();
}
};
// single mode active first checked node
else if (!multiple && checkedKeys.length) {
nextActiveKey = checkedKeys[0];
}
// default active first node
else {
nextActiveKey = getFirstNode();
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
}

setActiveKey(nextActiveKey);
}, [open, searchValue]);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

活动键管理需要补充依赖项

活动键管理的逻辑符合需求,但useEffect缺少了关键依赖:

 React.useEffect(() => {
   if (!open) {
     return;
   }
   // ...
   setActiveKey(nextActiveKey);
-}, [open, searchValue]);
+}, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// ========================== Active ==========================
const [activeKey, setActiveKey] = React.useState<Key>(null);
const activeEntity = keyEntities[activeKey as SafeKey];
React.useEffect(() => {
if (!open) {
return;
}
let nextActiveKey = null;
onSelect(node.key, {
selected: !checkedKeys.includes(node.key),
});
const getFirstNode = () => {
const firstNode = getFirstMatchingNode(memoTreeData);
return firstNode ? firstNode[fieldNames.value] : null;
};
if (!multiple) {
toggleOpen(false);
// search mode active first node
if (searchValue) {
nextActiveKey = getFirstNode();
}
};
// single mode active first checked node
else if (!multiple && checkedKeys.length) {
nextActiveKey = checkedKeys[0];
}
// default active first node
else {
nextActiveKey = getFirstNode();
}
setActiveKey(nextActiveKey);
}, [open, searchValue]);
// ========================== Active ==========================
const [activeKey, setActiveKey] = React.useState<Key>(null);
const activeEntity = keyEntities[activeKey as SafeKey];
React.useEffect(() => {
if (!open) {
return;
}
let nextActiveKey = null;
const getFirstNode = () => {
const firstNode = getFirstMatchingNode(memoTreeData);
return firstNode ? firstNode[fieldNames.value] : null;
};
// search mode active first node
if (searchValue) {
nextActiveKey = getFirstNode();
}
// single mode active first checked node
else if (!multiple && checkedKeys.length) {
nextActiveKey = checkedKeys[0];
}
// default active first node
else {
nextActiveKey = getFirstNode();
}
setActiveKey(nextActiveKey);
}, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]);


// ========================= Keyboard =========================
React.useImperativeHandle(ref, () => ({
Expand All @@ -176,8 +219,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 +240,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
98 changes: 98 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,101 @@ 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
open
onSelect={onSelect}
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();
onSelect.mockReset();

wrapper.search('3');
wrapper.find('input').first().simulate('keyDown', { which: KeyCode.ENTER });
expect(onSelect).toHaveBeenCalledWith('3', expect.anything());
onSelect.mockReset();
});

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