Skip to content

fix: open trigger force scroll #304

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 7, 2025
Merged

fix: open trigger force scroll #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a521a1a
fix: not trigger scroll when init
zombieJ Nov 7, 2025
799b9f5
test: update test case
zombieJ Nov 7, 2025
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
10 changes: 6 additions & 4 deletions src/DropdownMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import Menu, { MenuItem, MenuRef } from '@rc-component/menu';
import Menu, { MenuItem, type MenuRef } from '@rc-component/menu';
import React, { useEffect, useRef } from 'react';
import MentionsContext from './MentionsContext';
import type { DataDrivenOptionProps } from './Mentions';

export interface DropdownMenuProps {
prefixCls?: string;
options: DataDrivenOptionProps[];
opened: boolean;
}

/**
Expand All @@ -23,24 +24,25 @@ function DropdownMenu(props: DropdownMenuProps) {
onScroll,
} = React.useContext(MentionsContext);

const { prefixCls, options } = props;
const { prefixCls, options, opened } = props;
const activeOption = options[activeIndex] || {};
const menuRef = useRef<MenuRef>(null);

// Monitor the changes in ActiveIndex and scroll to the visible area if there are any changes
useEffect(() => {
if (activeIndex === -1 || !menuRef.current) {
if (activeIndex === -1 || !menuRef.current || !opened) {
return;
}

const activeItem = menuRef.current?.findItem?.({ key: activeOption.key });

if (activeItem) {
activeItem.scrollIntoView({
block: 'nearest',
inline: 'nearest',
});
}
}, [activeIndex, activeOption.key]);
}, [activeIndex, activeOption.key, opened]);

return (
<Menu
Expand Down
8 changes: 7 additions & 1 deletion src/KeywordTrigger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,14 @@ const KeywordTrigger: FC<KeywordTriggerProps> = props => {
} = props;

const dropdownPrefix = `${prefixCls}-dropdown`;
const [opened, setOpened] = React.useState(false);

const dropdownElement = (
<DropdownMenu prefixCls={dropdownPrefix} options={options} />
<DropdownMenu
prefixCls={dropdownPrefix}
options={options}
opened={opened}
/>
);

const dropdownPlacement = useMemo(() => {
Expand All @@ -95,6 +100,7 @@ const KeywordTrigger: FC<KeywordTriggerProps> = props => {
getPopupContainer={getPopupContainer}
popupClassName={popupClassName}
popupStyle={popupStyle}
afterOpenChange={setOpened}
>
{children}
</Trigger>
Expand Down
49 changes: 10 additions & 39 deletions tests/DropdownMenu.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('DropdownMenu', () => {
jest.useRealTimers();
});

it('should scroll into view when navigating with keyboard', () => {
it('should scroll into view when navigating with keyboard', async () => {
Copy link

@coderabbitai coderabbitai bot Nov 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

测试描述与实际行为不匹配。

测试名称为 "should scroll into view when navigating with keyboard"，但实际测试中并未模拟键盘导航（例如按下 Down/Up 箭头键）。测试只是模拟输入并等待定时器推进。建议将测试名称修改为更准确地反映其实际验证的行为，例如 "should scroll into view after dropdown opens"。

应用此差异来更新测试名称：

-  it('should scroll into view when navigating with keyboard', async () => {
+  it('should scroll into view after dropdown opens', async () => {
📝 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
it('should scroll into view when navigating with keyboard', async () => {
it('should scroll into view after dropdown opens', async () => {
🤖 Prompt for AI Agents 
In tests/DropdownMenu.spec.tsx around line 22, the test title "should scroll
into view when navigating with keyboard" is inaccurate because the test does not
simulate keyboard navigation; rename the test to reflect actual behavior, e.g.,
change the it(...) description to "should scroll into view after dropdown opens"
so the test name matches what is being verified.

// Setup component with UnstableContext for testing dropdown behavior
const { container } = render(
<UnstableContext.Provider value={{ open: true }}>
Expand All @@ -32,51 +32,22 @@ describe('DropdownMenu', () => {
.spyOn(HTMLElement.prototype, 'scrollIntoView')
.mockImplementation(jest.fn());

const textarea = container.querySelector('textarea')!;
// Trigger should not scroll
simulateInput(container, '@');
expect(scrollIntoViewMock).not.toHaveBeenCalled();

act(() => {
// First trigger the measuring state by typing @
simulateInput(container, '@');
jest.runAllTimers();
});

// Verify we're in measuring state
expectMeasuring(container, true);

act(() => {
// Press ArrowDown multiple times to make options overflow the visible area
for (let i = 0; i < 10; i++) {
fireEvent.keyDown(textarea, {
keyCode: KeyCode.DOWN,
which: KeyCode.DOWN,
});
}
jest.runAllTimers();
});
for (let i = 0; i < 10; i++) {
await act(async () => {
jest.advanceTimersByTime(1000);
await Promise.resolve();
});
}
Comment on lines +39 to +44
Copy link

@gemini-code-assist gemini-code-assist bot Nov 7, 2025

Choose a reason for hiding this comment

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

medium

This test has a few issues that could be improved:

  1. Confusing loop: The for loop advancing timers seems incorrect. To wait for the dropdown to open, a single act block that runs all timers is sufficient and much clearer.
  2. Misleading test name: The test is named ...navigating with keyboard but no longer simulates keyboard navigation. It now only tests the scroll behavior on open. Consider renaming the test to something like should scroll into view after dropdown opens.
  3. Reduced coverage: The part of the test that verified scrolling up with ArrowUp has been removed. It would be beneficial to have a test that covers keyboard navigation in both directions, perhaps in a separate test case.

Here's a suggestion to simplify the timer logic:

Suggested change
for (let i = 0; i < 10; i++) {
await act(async () => {
jest.advanceTimersByTime(1000);
await Promise.resolve();
});
}
// Wait for the dropdown to fully open by running all timers.
// This will trigger `afterOpenChange` and any subsequent effects.
await act(async () => {
jest.runAllTimers();
await Promise.resolve();
});


// Verify if scrollIntoView was called
expect(scrollIntoViewMock).toHaveBeenCalledWith({
block: 'nearest',
inline: 'nearest',
});
scrollIntoViewMock.mockClear();

act(() => {
// Press ArrowUp to verify scrolling up
for (let i = 0; i < 5; i++) {
fireEvent.keyDown(textarea, {
keyCode: KeyCode.UP,
which: KeyCode.UP,
});
}
jest.runAllTimers();
});

// Verify if scrollIntoView was called again
expect(scrollIntoViewMock).toHaveBeenCalledWith({
block: 'nearest',
inline: 'nearest',
});

scrollIntoViewMock.mockRestore();
});
Expand Down