feat: add virtualization to S2 combobox and picker #8110

yihuiliao · 2025-04-17T18:27:48Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

rspbot · 2025-04-17T21:30:00Z

Build successful! 🎉

rspbot · 2025-04-17T22:19:38Z

Build successful! 🎉

rspbot · 2025-04-17T23:07:10Z

Build successful! 🎉

yihuiliao · 2025-04-17T21:40:09Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+  );
+}
+
+const Separator = /*#__PURE__*/ createLeafComponent('separator', function Separator(props: SeparatorProps & {size?: 'S' | 'M' | 'L' | 'XL'}, ref: ForwardedRef<HTMLElement>) {


it does feel a little overkill to import separator separately but it's at least a pretty simple component ...i can probably simplify the code since this isn't being used anywhere else except for picker + combobox. given that it's not interactive and just a visual thing, i don't think it has any accessibility requirements.

otherwise, i experimented a little bit with adding a bottom border which would work i think? i would need to play around with the linear-gradient so that the visible border aligns with the item text and it'd require some additional targeting of first-child/last-child but i think it should be possible. that said, not sure if it's worth the time figuring this out if this current solution works

What is the reason we can't use the S2 Divider which already renders the RAC Separator which is a LeafComponent?
Is it just the extra size requirement?

No, it was that it wasn't measuring the height correctly. We had some additional padding on the divider that wasn't being accounted for when measuring the layout. By copying over the S2 Divider, wrapping it in a div, we could account for the height correctly. The reason I had to copy it over was because if I just wrapped the S2 Divider in a div, it would throw an error. I had to do it inside the createLeafComponent.

I tried updating the ListLayout to have a separatorHeight but the reason that didn't work out was because even if we had display: 'none' set on the last separator, the collection would still account for the separatorHeight so there was additional empty space on the bottom that shouldn't have been there. In that case, we would need it to be an estimatedSeparatorHeight and involve slightly different logic similar to buildItem but I didn't test that out.

Overall, we decided that it was probably not worth our time spending too much time on just a separator so this is what we came up with.

what was the reason for the extra div? where was the extra padding coming from?

actually I think ListBox doesn't even allow separators. aXe currently fails on S2 prod storybook. In v3 we overrode the role to presentation, but RAC doesn't support that. Any reason it needs to be a separate element at all? Can it just be a border-bottom on the section?

Or actually it was margins, not padding. It was coming from here. The extra div was so I could set an explicit height.

We could potentially use border-bottom instead, but in order to align the separator with the margins we have on the menu items + heading on the left + right, we'd probably need something like border-image. For example, if we added a border (this just added it to the top, very basic, no styling, for demonstration purposes), understandably, the length of it is the width of the section. However, the separator has a length that is less than the width of the section. Right now, I'm able to account for that by adding a margin with edgeToText(). I suppose we could just calculate it with a percentage tho?

Since it doesn't need to be an actual divider, can we make a stripped down version like this?

const dividerStyle = style({ backgroundColor: { default: 'gray-200', forcedColors: 'ButtonBorder' }, borderRadius: 'full', height: '[2px]', width: 'full' }); export const Divider = /*#__PURE__*/ createLeafComponent('separator', function Divider({size}: {size?: 'S' | 'M' | 'L' | 'XL'}, ref: ForwardedRef<HTMLDivElement>) { return ( <div className={separatorWrapper({size})}> <div ref={ref} className={dividerStyle} /> </div> ); });

needs alignItems: 'center' in separatorWrapper too

packages/@react-spectrum/s2/src/ComboBox.tsx

rspbot · 2025-04-18T18:33:37Z

Build successful! 🎉

packages/@react-spectrum/s2/stories/Picker.stories.tsx

rspbot · 2025-04-22T00:49:58Z

Build successful! 🎉

rspbot · 2025-04-29T00:19:14Z

Build successful! 🎉

rspbot · 2025-05-01T18:11:50Z

Build successful! 🎉

rspbot · 2025-05-05T21:47:29Z

Build successful! 🎉

…ectrum into s2-combobox-picker-virtualizer

rspbot · 2025-05-06T17:33:54Z

Build successful! 🎉

packages/@react-spectrum/s2/package.json

packages/@react-spectrum/s2/src/ComboBox.tsx

LFDanLu · 2025-05-06T18:43:52Z

packages/@react-spectrum/s2/src/Picker.tsx

-      setPressed(false);
-    }, {once: true, capture: true});
-  };
+  let layout = new ListLayout({estimatedRowHeight: 32, estimatedHeadingHeight: 50, padding: 8});


Interesting that this had to be pulled out of the Virtualizer layout and layoutOptions since now this means we generate a new ListLayout every render. I assume this was to fix the scroll into view behavior?

Yeah, this was to fix the scroll into view. Without the change, the view would only scroll when we were closing the popover which isn't very useful.

It seemed like what was happening prior to this change was that when we initially open the picker without any selected item, we call layoutIfNeeded as we scroll to select an item which sets this.requestedRect. We want to preserve that value even when we close the popover but was happening was that we were basically resetting this value every time we closed the popover so this.requestedRect didn't have a size (everything was just zero). Then when we called this.buildCollection, at this line we would exit the loop early so we only had one node when we called getVisibleLayoutInfos

Might have some gaps in my understanding but that's what I found if that's helpful at all

Can we memo the ListLayout? looks like it doesn't have any dependencies

hmm this also goes against our common pattern for Virtualizer now
Could we instead move the Virtualizer up to wrap the PopoverBase?

rspbot · 2025-05-06T22:28:01Z

Build successful! 🎉

snowystinger · 2025-05-07T04:11:58Z

packages/@react-spectrum/autocomplete/test/SearchAutocomplete.test.js

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


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

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

snowystinger · 2025-05-07T04:21:14Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+  width: 'full',
+  boxSizing: 'border-box',
+  maxHeight: '[inherit]',
+  // TODO: Might help with horizontal scrolling happening on Windows, will need to check somehow. Otherwise, revert back to overflow: auto


what is the todo here? was something else being tried?

It was something that I thought might fix the issue with the horizontal scrollbar but I'm unable to see if it actually works since I can't reproduce the original issue

snowystinger · 2025-05-07T04:22:12Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+    }
+  },
+  position: 'relative',
+  // each menu item should take up the entire width, the subgrid will handle within the item


no longer relevant comment?

snowystinger · 2025-05-07T04:23:36Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+    '. checkmark icon label  .',
+    '.      .      .  description .'


Suggested change

'. checkmark icon label .',

'. . . description .'

'. checkmark icon label .',

'. . . description .'

snowystinger · 2025-05-07T04:24:11Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+  gridTemplateRows: {
+    // min-content prevents second row from 'auto'ing to a size larger then 0 when empty
+    default: 'auto minmax(0, min-content)',
+    ':has([slot=description])': 'auto auto'


pull out into constant? seems to be the common pattern we're adopting

snowystinger · 2025-05-07T04:31:26Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+  );
+}
+
+const Separator = /*#__PURE__*/ createLeafComponent('separator', function Separator(props: SeparatorProps & {size?: 'S' | 'M' | 'L' | 'XL'}, ref: ForwardedRef<HTMLElement>) {


What is the reason we can't use the S2 Divider which already renders the RAC Separator which is a LeafComponent?
Is it just the extra size requirement?

snowystinger · 2025-05-07T04:31:47Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+    <Separator
+      {...props}
+      className={mergeStyles(
+        divider({


divider only supports 'S' | 'M' | 'L', if XL is sent in, it'll treat it like M

I think it should be fine? Since we are passing the size as 'M', it should always use the 'M' styles regardless of ComboBox or Picker

snowystinger · 2025-05-07T04:34:34Z

packages/@react-spectrum/s2/src/Picker.tsx

-      setPressed(false);
-    }, {once: true, capture: true});
-  };
+  let layout = new ListLayout({estimatedRowHeight: 32, estimatedHeadingHeight: 50, padding: 8});


Can we memo the ListLayout? looks like it doesn't have any dependencies

snowystinger · 2025-05-07T04:37:02Z

packages/@react-spectrum/s2/src/Picker.tsx

-      setPressed(false);
-    }, {once: true, capture: true});
-  };
+  let layout = new ListLayout({estimatedRowHeight: 32, estimatedHeadingHeight: 50, padding: 8});


hmm this also goes against our common pattern for Virtualizer now
Could we instead move the Virtualizer up to wrap the PopoverBase?

snowystinger · 2025-05-07T04:38:18Z

packages/@react-spectrum/s2/src/TableView.tsx

@@ -194,6 +194,9 @@ export class S2TableLayout<T> extends TableLayout<T> {

  protected buildCollection(): LayoutNode[] {
    let [header, body] = super.buildCollection();
+    if (!header) {


@LFDanLu can you explain when this happens?

If I remember correctly, now that we are rendering the contents of the virtualized content even when we haven't fully processed the collection (aka the changes in Virtualizer and rect size check changes) there is a moment where we haven't processed the collection and thus can't perform build the header/body yet

devongovett · 2025-05-07T23:11:24Z

packages/@react-spectrum/s2/src/ComboBox.tsx

@@ -148,6 +149,114 @@ const iconStyles = style({
  }
 });

+export let listbox = style<{size: 'S' | 'M' | 'L' | 'XL'}>({


What are the differences between the styles here and the ones in menu? A bit unfortunate to duplicate them

Mainly that menu has some additional styles that Combobox and Picker didn't need but it wasn't actually affecting anything. I could probably revert the change and just use menu but I'd need to double check

devongovett · 2025-05-07T23:16:50Z

packages/@react-spectrum/s2/src/ComboBox.tsx

+  );
+}
+
+const Separator = /*#__PURE__*/ createLeafComponent('separator', function Separator(props: SeparatorProps & {size?: 'S' | 'M' | 'L' | 'XL'}, ref: ForwardedRef<HTMLElement>) {


what was the reason for the extra div? where was the extra padding coming from?

yihuiliao added 13 commits April 11, 2025 16:22

set overflow to visible on ListLayout

27260f4

add separator height to list layout

f8b1745

change css for picker and combobox

b272469

Merge branch 'main' into s2-combobox-picker-virtualizer

7a2877c

fix separator height

5c423d4

fix picker's separator

fdf423f

cleanup

711dff6

Merge branch 'main' into s2-combobox-picker-virtualizer

286d8b8

update yarn lock

0938fb9

fix lint

5fc224f

remove workflow dependency

18c65b2

remove style from s1 theme oops

85b7edb

fix lint

7f57506

picker fixes

d493fc4

yihuiliao added 2 commits April 17, 2025 15:31

picker cleanup

a69a794

fix lint

4523888

fix line height in header

3052001

yihuiliao commented Apr 17, 2025

View reviewed changes

fix lint?

7f062b8

yihuiliao commented Apr 18, 2025

View reviewed changes

packages/@react-spectrum/s2/stories/Picker.stories.tsx Show resolved Hide resolved

adobe deleted a comment from rspbot Apr 21, 2025

Merge branch 'main' into s2-combobox-picker-virtualizer

0c3a3b4

yihuiliao changed the title ~~wip: S2 combobox picker virtualizer~~ feat: add virtualization to S2 combobox and picker Apr 22, 2025

yihuiliao marked this pull request as ready for review April 22, 2025 00:37

adobe deleted a comment from rspbot Apr 23, 2025

yihuiliao added 5 commits April 24, 2025 16:13

update grid areas and fix edgeToText

85440fc

Merge branch 'main' into s2-combobox-picker-virtualizer

48caf94

fix install?

379fba7

fix sizes

160e9a0

fix lint

fac1920

fix delay when opening many items S2 select

77b4b62

yihuiliao added 6 commits May 1, 2025 16:54

sorta get selected item to scroll into view virtualized

1bbd69c

fix tests

bebc44b

cleanup fix lint

9976091

more cleanup

9c51d0d

Merge branch 'main' into select-scrollview

ebba44e

fix overflow on windows potentially...

cb52f58

Merge branch 'select-scrollview' of https://github.com/adobe/react-sp…

112df59

…ectrum into s2-combobox-picker-virtualizer

yihuiliao added the release label May 5, 2025

yihuiliao added 2 commits May 6, 2025 10:19

fix s2 picker scroll selected item into view

3cbb286

fix lint

923e260

yihuiliao mentioned this pull request May 6, 2025

fix: selected item in virtualized rac select to scroll into view #8178

Closed

5 tasks

LFDanLu reviewed May 6, 2025

View reviewed changes

yihuiliao added 2 commits May 6, 2025 14:59

update yarn lock

01ead63

remove comment

3ae8f29

snowystinger reviewed May 7, 2025

View reviewed changes

devongovett reviewed May 7, 2025

View reviewed changes

feat: add virtualization to S2 combobox and picker #8110

Are you sure you want to change the base?

feat: add virtualization to S2 combobox and picker #8110

Conversation

yihuiliao commented Apr 17, 2025

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Apr 17, 2025

rspbot commented Apr 17, 2025

rspbot commented Apr 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuiliao May 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuiliao May 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Apr 18, 2025

rspbot commented Apr 22, 2025

rspbot commented Apr 29, 2025

rspbot commented May 1, 2025

rspbot commented May 5, 2025

rspbot commented May 6, 2025

Choose a reason for hiding this comment

yihuiliao May 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented May 6, 2025

Choose a reason for hiding this comment

yihuiliao May 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuiliao May 7, 2025 •

edited

Loading

yihuiliao May 8, 2025 •

edited

Loading

yihuiliao May 6, 2025 •

edited

Loading

yihuiliao May 7, 2025 •

edited

Loading