Reducing unnecessary Virtualizer onLoadMore calls impacting ComboBox

* adding tests for ComboBox onLoadMore not being called on every open

* updating a cardview onloadmore test fix

* getting the correct number of onLoadMore calls to happen

Co-authored-by: Daniel Lu <[email protected]>

Author: ktabors

packages/@react-aria/virtualizer/src/Virtualizer.tsx

@@ -85,7 +85,7 @@ function Virtualizer<T extends object, V>(props: VirtualizerProps<T, V>, ref: Re
 
   useLayoutEffect(() => {
     if (!isLoading && onLoadMore && !state.isAnimating) {
-      if (state.contentSize.height <= state.virtualizer.visibleRect.height) {
+      if (state.contentSize.height > 0 && state.contentSize.height <= state.virtualizer.visibleRect.height) {
         onLoadMore();
       }
     }

packages/@react-spectrum/card/test/CardView.test.js

@@ -1076,7 +1076,7 @@ describe('CardView', function () {
       let cards = tree.getAllByRole('gridcell');
       expect(cards).toBeTruthy();
       // Virtualizer calls onLoadMore twice due to initial layout
-      expect(onLoadMore).toHaveBeenCalledTimes(2);
+      expect(onLoadMore).toHaveBeenCalledTimes(1);
       triggerPress(cards[1]);
 
       act(() => {
@@ -1088,7 +1088,7 @@ describe('CardView', function () {
       let grid = tree.getByRole('grid');
       grid.scrollTop = 3000;
       fireEvent.scroll(grid);
-      expect(onLoadMore).toHaveBeenCalledTimes(3);
+      expect(onLoadMore).toHaveBeenCalledTimes(2);
     });
 
     it.each`

packages/@react-spectrum/combobox/stories/ComboBox.stories.tsx

@@ -16,6 +16,7 @@ import Add from '@spectrum-icons/workflow/Add';
 import Alert from '@spectrum-icons/workflow/Alert';
 import Bell from '@spectrum-icons/workflow/Bell';
 import {ButtonGroup} from '@react-spectrum/buttongroup';
+import {chain} from '@react-aria/utils';
 import {ComboBox, Item, Section} from '../';
 import Copy from '@spectrum-icons/workflow/Copy';
 import Draw from '@spectrum-icons/workflow/Draw';
@@ -562,7 +563,7 @@ function AsyncLoadingExample() {
       inputValue={list.filterText}
       onInputChange={list.setFilterText}
       loadingState={list.loadingState}
-      onLoadMore={list.loadMore}
+      onLoadMore={chain(action('onLoadMore'), list.loadMore)}
       onOpenChange={action('onOpenChange')}>
       {item => <Item key={item.name}>{item.name}</Item>}
     </ComboBox>

packages/@react-spectrum/combobox/test/ComboBox.test.js

@@ -14,6 +14,7 @@ jest.mock('@react-aria/live-announcer');
 import {act, fireEvent, render, screen, waitFor, within} from '@testing-library/react';
 import {announce} from '@react-aria/live-announcer';
 import {Button} from '@react-spectrum/button';
+import {chain} from '@react-aria/utils';
 import {ComboBox, Item, Section} from '../';
 import {Provider} from '@react-spectrum/provider';
 import React from 'react';
@@ -37,6 +38,7 @@ let onInputChange = jest.fn();
 let outerBlur = jest.fn();
 let onFocus = jest.fn();
 let onBlur = jest.fn();
+let onLoadMore = jest.fn();
 
 let defaultProps = {
   label: 'Test',
@@ -193,7 +195,8 @@ let AsyncComboBox = () => {
       inputValue={list.filterText}
       onInputChange={list.setFilterText}
       loadingState={list.loadingState}
-      onLoadMore={list.loadMore}>
+      onLoadMore={chain(list.loadMore, onLoadMore)}
+      onOpenChange={onOpenChange}>
       {(item) => <Item>{item.name}</Item>}
     </ComboBox>
   );
@@ -1975,6 +1978,104 @@ describe('ComboBox', function () {
     });
   });
 
+  describe('async', function () {
+    let clientHeightSpy;
+    let scrollHeightSpy;
+    beforeEach(() => {
+      // clientHeight is needed for ScrollView's updateSize()
+      clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40);
+      // scrollHeight is for useVirutalizerItem to mock its getSize()
+      scrollHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 32);
+    });
+    afterEach(() => {
+      clientHeightSpy.mockRestore();
+      scrollHeightSpy.mockRestore();
+      // This returns this to the value used by all the other tests
+      jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 1000);
+    });
+    it('onLoadMore is called on initial open', async () => {
+      let {getByRole} = render(
+        <Provider theme={theme}>
+          <AsyncComboBox />
+        </Provider>
+      );
+
+      let button = getByRole('button');
+
+      await waitFor(() => expect(load).toHaveBeenCalledTimes(1));
+      expect(onOpenChange).toHaveBeenCalledTimes(0);
+      expect(onLoadMore).toHaveBeenCalledTimes(0);
+
+      await act(async () => {
+        triggerPress(button);
+        jest.runAllTimers();
+      });
+
+      let listbox = getByRole('listbox');
+      expect(listbox).toBeVisible();
+
+      expect(onOpenChange).toHaveBeenCalledTimes(1);
+      expect(onOpenChange).toHaveBeenCalledWith(true, 'manual');
+      expect(onLoadMore).toHaveBeenCalledTimes(1);
+      await waitFor(() => expect(load).toHaveBeenCalledTimes(1));
+    });
+
+    it('onLoadMore is not called on when previously opened', async () => {
+      let {getByRole} = render(
+        <Provider theme={theme}>
+          <AsyncComboBox />
+        </Provider>
+      );
+
+      let button = getByRole('button');
+      let combobox = getByRole('combobox');
+      await waitFor(() => expect(load).toHaveBeenCalledTimes(1));
+      expect(onOpenChange).toHaveBeenCalledTimes(0);
+      expect(onLoadMore).toHaveBeenCalledTimes(0);
+
+      // this call and the one below are more correct for how the code should
+      // behave, the inital call would have a height of zero and after that a measureable height
+      clientHeightSpy.mockRestore();
+      clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40);
+      // open menu
+      await act(async () => {
+        triggerPress(button);
+        jest.runAllTimers();
+      });
+
+      expect(onOpenChange).toHaveBeenCalledTimes(1);
+      expect(onOpenChange).toHaveBeenCalledWith(true, 'manual');
+      expect(onLoadMore).toHaveBeenCalledTimes(1);
+
+      // close menu
+      act(() => {
+        combobox.blur();
+        jest.runAllTimers();
+      });
+
+      expect(onOpenChange).toHaveBeenCalledTimes(2);
+      expect(onOpenChange).toHaveBeenLastCalledWith(false, undefined);
+      expect(onLoadMore).toHaveBeenCalledTimes(1);
+
+      clientHeightSpy.mockRestore();
+      clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40);
+      // reopen menu
+      await act(async () => {
+        triggerPress(button);
+        jest.runAllTimers();
+      });
+
+      expect(onOpenChange).toHaveBeenCalledTimes(3);
+      expect(onOpenChange).toHaveBeenLastCalledWith(true, 'manual');
+      // Please test this in storybook.
+      // In virtualizer.tsx the onVisibleRectChange() causes this to be called twice
+      // because the browser limits the popover height via calculatePosition(),
+      // while the test doesn't, causing virtualizer to try to load more
+      expect(onLoadMore).toHaveBeenCalledTimes(2);
+      await waitFor(() => expect(load).toHaveBeenCalledTimes(1));
+    });
+  });
+
   describe('controlled combobox', function () {
     describe('controlled by both selectedKey and inputValue', function () {
       it('does not update state', function () {
@@ -3387,7 +3488,7 @@ describe('ComboBox', function () {
           </ComboBox>
         </Provider>
       );
-      
+
       let button = getByRole('button');
 
       act(() => {
@@ -4472,9 +4573,13 @@ describe('ComboBox', function () {
           trayInput.focus();
           fireEvent.change(trayInput, {target: {value: 'aard'}});
           jest.advanceTimersByTime(500);
-          let trayInputProgress = within(tray).getByRole('progressbar', {hidden: true});
-          expect(trayInputProgress).toBeTruthy();
-          expect(within(listbox).queryByRole('progressbar')).toBeNull();
+        });
+
+        let trayInputProgress = within(tray).getByRole('progressbar', {hidden: true});
+        expect(trayInputProgress).toBeTruthy();
+        expect(within(listbox).queryByRole('progressbar')).toBeNull();
+
+        await act(async () => {
           jest.runAllTimers();
         });