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 20 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
8 changes: 0 additions & 8 deletions examples/basic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,6 @@ class Demo extends React.Component {
console.log('onPopupScroll:', evt.target);
}}
/>

aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
<h2>single select (just select children)</h2>
<TreeSelect
style={{ width: 300 }}
Expand All @@ -233,7 +232,6 @@ class Demo extends React.Component {
filterTreeNode={false}
onChange={this.onChangeChildren}
/>

<h2>multiple select</h2>
<TreeSelect
style={{ width: 300 }}
Expand All @@ -249,7 +247,6 @@ class Demo extends React.Component {
onSelect={this.onSelect}
allowClear
/>

<h2>check select</h2>
<TreeSelect
open
Expand Down Expand Up @@ -281,7 +278,6 @@ class Demo extends React.Component {
return `${valueList.length} rest...`;
}}
/>

<h2>labelInValue & show path</h2>
<TreeSelect
style={{ width: 500 }}
Expand All @@ -299,7 +295,6 @@ class Demo extends React.Component {
filterTreeNode={false}
onChange={this.onChangeLV}
/>

<h2>use treeDataSimpleMode</h2>
<TreeSelect
style={{ width: 300 }}
Expand All @@ -323,7 +318,6 @@ class Demo extends React.Component {
this.onSelect(...args);
}}
/>

<h2>Testing in extreme conditions (Boundary conditions test) </h2>
<TreeSelect
style={{ width: 200 }}
Expand All @@ -347,7 +341,6 @@ class Demo extends React.Component {
]}
onChange={(val, ...args) => console.log(val, ...args)}
/>

<h2>use TreeNode Component (not recommend)</h2>
<TreeSelect
style={{ width: 200 }}
Expand Down Expand Up @@ -384,7 +377,6 @@ class Demo extends React.Component {
</TreeNode>
<TreeNode value="same value3" title="same title" key="0-3" />
</TreeSelect>

<h2>title render</h2>
<TreeSelect<{ label: string }>
open
Expand Down
108 changes: 81 additions & 27 deletions src/OptionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import LegacyContext from './LegacyContext';
import TreeSelectContext from './TreeSelectContext';
import type { Key, SafeKey } from './interface';
import { getAllKeys, isCheckDisabled } from './utils/valueUtil';
import { flattenTreeData } from 'rc-tree/lib/utils/treeUtil';
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved

const HIDDEN_STYLE = {
width: 0,
Expand Down Expand Up @@ -76,7 +77,7 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
(prev, next) => next[0] && prev[1] !== next[1],
);

// ========================== Active ==========================
// ========================== Active Key ==========================
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
const [activeKey, setActiveKey] = React.useState<Key>(null);
const activeEntity = keyEntities[activeKey as SafeKey];

Expand All @@ -92,16 +93,36 @@ 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
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 的依赖数组需要更新

Scroll EffectuseEffect 中,依赖数组仅包含了 open,但是内部使用了 multiplecheckedKeys。这可能导致当 multiplecheckedKeys 发生变化时,useEffect 未能及时触发,可能引发逻辑错误。

建议将 multiplecheckedKeys 添加到依赖数组中:

 React.useEffect(() => {
   // 单选模式下应滚动到当前选中项
   if (open && !multiple && checkedKeys.length) {
     treeRef.current?.scrollTo({ key: checkedKeys[0] });
   }
-}, [open]);
+}, [open, multiple, checkedKeys]);
📝 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
// ========================== Scroll Effect ==========================
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
// ========================== Scroll Effect ==========================
React.useEffect(() => {
// Single mode should scroll to current key
if (open && !multiple && checkedKeys.length) {
treeRef.current?.scrollTo({ key: checkedKeys[0] });
}
}, [open, multiple, checkedKeys]);

}, [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 +143,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 +152,66 @@ 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));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [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 的依赖数组需要更新

Search EffectuseEffect 中,依赖数组仅包含了 searchValue,但内部使用了 treeDatafieldNames。如果 treeDatafieldNames 发生变化,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
// ========================== Search Effect ==========================
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchValue]);
// ========================== Search Effect ==========================
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchValue, treeData, fieldNames]);

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]);


const onInternalSelect = (__: Key[], info: TreeEventInfo) => {
const { node } = info;
// ========================== Get First Selectable Node ==========================
const getFirstMatchingNode = (
nodes: EventDataNode<any>[],
searchVal?: string,
): EventDataNode<any> | null => {
for (const node of nodes) {
if (node.disabled || node.selectable === false) {
continue;
}

if (checkable && isCheckDisabled(node)) {
if (searchVal) {
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
if (filterTreeNode(node)) {
return node;
}
} else {
return node;
}

if (node[fieldNames.children]) {
const matchInChildren = getFirstMatchingNode(node[fieldNames.children], searchVal);
if (matchInChildren) {
return matchInChildren;
}
}
}
return null;
};

// ========================== Active Key Effect ==========================
React.useEffect(() => {
if (!open) {
setActiveKey(null);
aojunhao123 marked this conversation as resolved.
Show resolved Hide resolved
return;
}

onSelect(node.key, {
selected: !checkedKeys.includes(node.key),
});
// Prioritize activating the searched node
if (searchValue) {
const firstNode = getFirstMatchingNode(treeData, searchValue);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
return;
}

if (!multiple) {
toggleOpen(false);
// If no search value, activate the first checked node
if (!multiple && checkedKeys.length) {
setActiveKey(checkedKeys[0]);
return;
}
};

// If no search value and no checked nodes, activate the first node
const firstNode = getFirstMatchingNode(treeData);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
}, [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 依赖项并优化逻辑

  1. 当前实现缺少重要的依赖项:treeDatafieldNamesmultiplecheckedKeys
  2. 激活键的设置逻辑可以更高效
 React.useEffect(() => {
   if (!open) {
     setActiveKey(null);
     return;
   }

   // Prioritize activating the searched node
   if (searchValue) {
     const firstNode = getFirstMatchingNode(treeData, searchValue);
     setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
     return;
   }

   // If no search value, activate the first checked node
   if (!multiple && checkedKeys.length) {
     setActiveKey(checkedKeys[0]);
     return;
   }

   // If no search value and no checked nodes, activate the first node
   const firstNode = getFirstMatchingNode(treeData);
   setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
-}, [open, searchValue]);
+}, [open, searchValue, treeData, fieldNames, multiple, checkedKeys]);
📝 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 Key Effect ==========================
React.useEffect(() => {
if (!open) {
setActiveKey(null);
return;
}
onSelect(node.key, {
selected: !checkedKeys.includes(node.key),
});
// Prioritize activating the searched node
if (searchValue) {
const firstNode = getFirstMatchingNode(treeData, searchValue);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
return;
}
if (!multiple) {
toggleOpen(false);
// If no search value, activate the first checked node
if (!multiple && checkedKeys.length) {
setActiveKey(checkedKeys[0]);
return;
}
};
// If no search value and no checked nodes, activate the first node
const firstNode = getFirstMatchingNode(treeData);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
}, [open, searchValue]);
// ========================== Active Key Effect ==========================
React.useEffect(() => {
if (!open) {
setActiveKey(null);
return;
}
// Prioritize activating the searched node
if (searchValue) {
const firstNode = getFirstMatchingNode(treeData, searchValue);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
return;
}
// If no search value, activate the first checked node
if (!multiple && checkedKeys.length) {
setActiveKey(checkedKeys[0]);
return;
}
// If no search value and no checked nodes, activate the first node
const firstNode = getFirstMatchingNode(treeData);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
}, [open, searchValue, treeData, fieldNames, multiple, checkedKeys]);


// ========================= Keyboard =========================
React.useImperativeHandle(ref, () => ({
Expand All @@ -176,8 +230,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 +251,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);
});
});
});
Loading