Skip to content

feat: add virtualization to S2 combobox and picker #8110

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

Merged
merged 44 commits into from
May 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
27260f4
set overflow to visible on ListLayout
yihuiliao Apr 11, 2025
f8b1745
add separator height to list layout
yihuiliao Apr 15, 2025
b272469
change css for picker and combobox
yihuiliao Apr 15, 2025
7a2877c
Merge branch 'main' into s2-combobox-picker-virtualizer
yihuiliao Apr 15, 2025
5c423d4
fix separator height
yihuiliao Apr 17, 2025
fdf423f
fix picker's separator
yihuiliao Apr 17, 2025
711dff6
cleanup
yihuiliao Apr 17, 2025
286d8b8
Merge branch 'main' into s2-combobox-picker-virtualizer
yihuiliao Apr 17, 2025
0938fb9
update yarn lock
yihuiliao Apr 17, 2025
5fc224f
fix lint
yihuiliao Apr 17, 2025
18c65b2
remove workflow dependency
yihuiliao Apr 17, 2025
85b7edb
remove style from s1 theme oops
yihuiliao Apr 17, 2025
7f57506
fix lint
yihuiliao Apr 17, 2025
d493fc4
picker fixes
yihuiliao Apr 17, 2025
a69a794
picker cleanup
yihuiliao Apr 17, 2025
4523888
fix lint
yihuiliao Apr 17, 2025
3052001
fix line height in header
yihuiliao Apr 17, 2025
7f062b8
fix lint?
yihuiliao Apr 18, 2025
0c3a3b4
Merge branch 'main' into s2-combobox-picker-virtualizer
yihuiliao Apr 21, 2025
85440fc
update grid areas and fix edgeToText
yihuiliao Apr 24, 2025
48caf94
Merge branch 'main' into s2-combobox-picker-virtualizer
yihuiliao Apr 28, 2025
379fba7
fix install?
yihuiliao Apr 28, 2025
160e9a0
fix sizes
yihuiliao Apr 28, 2025
fac1920
fix lint
yihuiliao Apr 29, 2025
77b4b62
fix delay when opening many items S2 select
LFDanLu May 1, 2025
1bbd69c
sorta get selected item to scroll into view virtualized
yihuiliao May 1, 2025
bebc44b
fix tests
yihuiliao May 1, 2025
9976091
cleanup fix lint
yihuiliao May 1, 2025
9c51d0d
more cleanup
yihuiliao May 2, 2025
ebba44e
Merge branch 'main' into select-scrollview
yihuiliao May 5, 2025
cb52f58
fix overflow on windows potentially...
yihuiliao May 5, 2025
112df59
Merge branch 'select-scrollview' of https://github.com/adobe/react-sp…
yihuiliao May 5, 2025
3cbb286
fix s2 picker scroll selected item into view
yihuiliao May 6, 2025
923e260
fix lint
yihuiliao May 6, 2025
01ead63
update yarn lock
yihuiliao May 6, 2025
3ae8f29
remove comment
yihuiliao May 6, 2025
dd0a3f7
review follow-up
yihuiliao May 7, 2025
c5ddd4d
fix v3 picker selection and fix tests
yihuiliao May 8, 2025
08437c7
simplify separator
yihuiliao May 8, 2025
2ba7e3d
fix lint
yihuiliao May 8, 2025
cabcf11
Merge branch 'main' into s2-combobox-picker-virtualizer
yihuiliao May 8, 2025
6b45081
fix css according to theme update
yihuiliao May 8, 2025
ad58e65
consolidate some of the css
yihuiliao May 8, 2025
176868a
clean up raf
yihuiliao May 8, 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
19 changes: 18 additions & 1 deletion packages/@react-aria/selection/src/useSelectableCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions

// Scroll the focused element into view when the focusedKey changes.
let lastFocusedKey = useRef(manager.focusedKey);
let raf = useRef<number | null>(null);
useEffect(() => {
if (manager.isFocused && manager.focusedKey != null && (manager.focusedKey !== lastFocusedKey.current || didAutoFocusRef.current) && scrollRef.current && ref.current) {
let modality = getInteractionModality();
Expand All @@ -516,8 +517,16 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions
}

if (modality === 'keyboard' || didAutoFocusRef.current) {
scrollIntoView(scrollRef.current, element);

if (raf.current) {
cancelAnimationFrame(raf.current);
}

raf.current = requestAnimationFrame(() => {
if (scrollRef.current) {
scrollIntoView(scrollRef.current, element);
}
});
// Avoid scroll in iOS VO, since it may cause overlay to close (i.e. RAC submenu)
if (modality !== 'virtual') {
scrollIntoViewport(element, {containingElement: ref.current});
Expand All @@ -534,6 +543,14 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions
didAutoFocusRef.current = false;
});

useEffect(() => {
return () => {
if (raf.current) {
cancelAnimationFrame(raf.current);
}
};
}, []);

// Intercept FocusScope restoration since virtualized collections can reuse DOM nodes.
useEvent(ref, 'react-aria-focus-scope-restore', e => {
e.preventDefault();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ describe('SearchAutocomplete', function () {
user = userEvent.setup({delay: null, pointerMap});
jest.spyOn(window.HTMLElement.prototype, 'clientWidth', 'get').mockImplementation(() => 1000);
jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 1000);
jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 50);
Copy link
Member

Choose a reason for hiding this comment

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

is this going to be problematic for users of v3 in their own tests?
will we need to call out in release?

Copy link
Member Author

@yihuiliao yihuiliao May 7, 2025

Choose a reason for hiding this comment

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

Probably something we'd like to call out but we also we have this section in are docs where we do mock the scrollHeight for virtualized components

Copy link
Member

Choose a reason for hiding this comment

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

just my 2 cents, would be good to call out in the release notes. Just spent some time trying to figure out why one of my Picker tests were failing and it turned out it also needed this scrollHeight mock lol

window.HTMLElement.prototype.scrollIntoView = jest.fn();
simulateDesktop();
jest.useFakeTimers();
Expand Down
4 changes: 4 additions & 0 deletions packages/@react-spectrum/card/test/CardView.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ describe('CardView', function () {
jest.useFakeTimers();
});

beforeEach(() => {
jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 50);
});

afterEach(() => {
jest.clearAllMocks();
act(() => jest.runAllTimers());
Expand Down
5 changes: 5 additions & 0 deletions packages/@react-spectrum/color/test/ColorPicker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,14 @@ describe('ColorPicker', function () {

beforeAll(() => {
user = userEvent.setup({delay: null, pointerMap});
jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 50);
jest.useFakeTimers();
});

afterAll(function () {
jest.restoreAllMocks();
});

afterEach(() => {
act(() => {jest.runAllTimers();});
});
Expand Down
1 change: 1 addition & 0 deletions packages/@react-spectrum/combobox/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ describe('ComboBox', function () {
});

beforeEach(() => {
jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 50);
load = jest
.fn()
.mockImplementationOnce(getFilterItems)
Expand Down
9 changes: 9 additions & 0 deletions packages/@react-spectrum/list/test/ListView.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ describe('ListView', function () {
afterEach(function () {
fireEvent.keyDown(document.activeElement, {key: 'Escape'});
fireEvent.keyUp(document.activeElement, {key: 'Escape'});
act(() => {jest.runAllTimers();});
jest.clearAllMocks();
});

Expand Down Expand Up @@ -1453,27 +1454,33 @@ describe('ListView', function () {

// scroll us down far enough that item 0 isn't in the view
moveFocus('ArrowDown');
act(() => jest.runAllTimers());
expect(scrollIntoView).toHaveBeenLastCalledWith(grid, getRow(tree, 'Item 1'));
grid.scrollTop = 40;
fireEvent.scroll(grid);
moveFocus('ArrowDown');
act(() => jest.runAllTimers());
expect(scrollIntoView).toHaveBeenLastCalledWith(grid, getRow(tree, 'Item 2'));
grid.scrollTop = 80;
fireEvent.scroll(grid);
moveFocus('ArrowDown');
act(() => jest.runAllTimers());
expect(scrollIntoView).toHaveBeenLastCalledWith(grid, getRow(tree, 'Item 3'));
grid.scrollTop = 120;
fireEvent.scroll(grid);

moveFocus('ArrowUp');
act(() => jest.runAllTimers());
expect(scrollIntoView).toHaveBeenLastCalledWith(grid, getRow(tree, 'Item 2'));
grid.scrollTop = 80;
fireEvent.scroll(grid);
moveFocus('ArrowUp');
act(() => jest.runAllTimers());
expect(scrollIntoView).toHaveBeenLastCalledWith(grid, getRow(tree, 'Item 1'));
grid.scrollTop = 40;
fireEvent.scroll(grid);
moveFocus('ArrowUp');
act(() => jest.runAllTimers());
expect(scrollIntoView).toHaveBeenLastCalledWith(grid, getRow(tree, 'Item 0'));
});

Expand Down Expand Up @@ -1506,6 +1513,7 @@ describe('ListView', function () {
});

focusRow(tree, 'Item 1');
act(() => jest.runAllTimers());
expect(document.activeElement).toBe(getRow(tree, 'Item 1'));
expect(scrollIntoView).toHaveBeenLastCalledWith(grid, document.activeElement);
});
Expand Down Expand Up @@ -1539,6 +1547,7 @@ describe('ListView', function () {

// Moving focus should scroll the new focused item into view
moveFocus('ArrowDown');
act(() => jest.runAllTimers());
expect(document.activeElement).toBe(getRow(tree, 'Item 1'));
expect(scrollIntoView).toHaveBeenLastCalledWith(body, document.activeElement);
});
Expand Down
4 changes: 3 additions & 1 deletion packages/@react-spectrum/picker/test/Picker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {User} from '@react-aria/test-utils';
import userEvent from '@testing-library/user-event';

describe('Picker', function () {
let offsetWidth, offsetHeight;
let offsetWidth, offsetHeight, scrollHeight;
let onSelectionChange = jest.fn();
let testUtilUser = new User();
let user;
Expand All @@ -40,13 +40,15 @@ describe('Picker', function () {
user = userEvent.setup({delay: null, pointerMap});
offsetWidth = jest.spyOn(window.HTMLElement.prototype, 'clientWidth', 'get').mockImplementation(() => 1000);
offsetHeight = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 1000);
scrollHeight = jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 50);
simulateDesktop();
jest.useFakeTimers();
});

afterAll(function () {
offsetWidth.mockReset();
offsetHeight.mockReset();
scrollHeight.mockReset();
});

afterEach(() => {
Expand Down
5 changes: 5 additions & 0 deletions packages/@react-spectrum/picker/test/TempUtilTest.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,14 @@ describe('Picker/Select ', function () {

beforeAll(function () {
user = userEvent.setup({delay: null, pointerMap});
jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 50);
simulateDesktop();
});

afterAll(function () {
jest.restoreAllMocks();
});

describe('with real timers', function () {
beforeAll(function () {
jest.useRealTimers();
Expand Down
Loading