Fix scrollToRow for non-virtualized Tables (#2239)

.changeset/giant-mayflies-wash.md

@@ -0,0 +1,5 @@
+---
+'@itwin/itwinui-react': patch
+---
+
+Fixed `scrollToRow` in un-virtualized `Table`. In virtualized `Table`, `scrollToRow` now scrolls to the top for consistent behavior.

apps/react-workshop/src/Table.test.ts

@@ -26,6 +26,7 @@ describe('Table', () => {
     'Localized',
     'No Data',
     'Resizable Columns',
+    'Scroll To Row',
     'Selectable Multi',
     'Selectable Single',
     'Sortable',
@@ -81,6 +82,9 @@ describe('Table', () => {
             cy.get('button').first().click({ force: true }); // force because the button is hidden
             break;
           }
+          case 'Scroll To Row': {
+            cy.wait(100);
+          }
         }
       });
 

packages/itwinui-css/src/table/base.scss

@@ -152,6 +152,9 @@
   min-inline-size: 100%;
   display: flex;
 
+  // To prevent the row from being overlapped by the tallest header when scrollToRow is used.
+  scroll-margin-block-start: calc(var(--_iui-table-header-size) + var(--iui-size-xl));
+
   .iui-table-header & {
     flex-grow: 1;
   }

packages/itwinui-react/src/core/Table/Table.test.tsx

@@ -3291,28 +3291,6 @@ it('should render selectable rows without select column', async () => {
   expect(onRowClick).toHaveBeenCalledTimes(3);
 });
 
-it('should scroll to selected item in non-virtualized table', async () => {
-  let scrolledElement: HTMLElement | null = null;
-  vi.spyOn(HTMLElement.prototype, 'scrollIntoView').mockImplementation(
-    function (this: HTMLElement) {
-      // eslint-disable-next-line @typescript-eslint/no-this-alias
-      scrolledElement = this;
-    },
-  );
-
-  const data = mockedData(50);
-  renderComponent({
-    data,
-    scrollToRow: (rows) => rows.findIndex((row) => row.original === data[25]),
-  });
-
-  expect(scrolledElement).toBeTruthy();
-  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-  expect(scrolledElement!.querySelector('.iui-table-cell')?.textContent).toBe(
-    data[25].name,
-  );
-});
-
 it('should render sticky columns correctly', () => {
   vi.spyOn(HTMLDivElement.prototype, 'scrollWidth', 'get').mockReturnValue(900);
   vi.spyOn(HTMLDivElement.prototype, 'clientWidth', 'get').mockReturnValue(500);

packages/itwinui-react/src/core/Table/Table.tsx

@@ -824,7 +824,7 @@ export const Table = <
 
   useLayoutEffect(() => {
     if (scrollToIndex) {
-      virtualizer.scrollToIndex(scrollToIndex, { align: 'center' });
+      virtualizer.scrollToIndex(scrollToIndex, { align: 'start' });
     }
   }, [virtualizer, scrollToIndex]);
 

testing/e2e/app/routes/Table/route.tsx

@@ -61,7 +61,7 @@ function EverythingElse() {
   const maxWidths = searchParams.getAll('maxWidth');
   const minWidths = searchParams.getAll('minWidth');
   const isSelectable = searchParams.get('isSelectable') === 'true';
-  const subRows = searchParams.get('subRows') === 'true';
+  const rows = searchParams.get('rows');
   const filter = searchParams.get('filter') === 'true';
   const selectSubRows = !(searchParams.get('selectSubRows') === 'false');
   const enableVirtualization = searchParams.get('virtualization') === 'true';
@@ -88,7 +88,16 @@ function EverythingElse() {
     return arr;
   }, [oneRow, empty]);
 
-  const data = subRows ? dataWithSubrows : baseData;
+  const data = (() => {
+    switch (rows) {
+      case 'subRows':
+        return dataWithSubrows;
+      case 'large':
+        return largeData;
+      default:
+        return baseData;
+    }
+  })();
 
   const isRowDisabled = React.useCallback(
     (rowData: Record<string, unknown>) => {
@@ -136,7 +145,7 @@ function EverythingElse() {
         columnResizeMode={columnResizeMode as 'fit' | 'expand' | undefined}
         selectSubRows={selectSubRows}
         enableVirtualization={enableVirtualization}
-        style={enableVirtualization ? { maxHeight: '90vh' } : undefined}
+        style={{ maxHeight: '90vh' }}
         scrollToRow={
           scroll
             ? (rows, data) =>
@@ -184,6 +193,13 @@ const baseData = [
   },
 ];
 
+const largeData = new Array(100).fill(0).map((_, i) => ({
+  index: i,
+  name: `Name${i}`,
+  description: `Description${i}`,
+  id: i,
+}));
+
 const dataWithSubrows = [
   {
     index: 1,

testing/e2e/app/routes/Table/spec.ts

@@ -1,5 +1,15 @@
 import { test, expect, type Page } from '@playwright/test';
 
+test('un-virtualized table should scroll to provided row', async ({ page }) => {
+  await page.goto('/Table?rows=large&scroll=true&scrollRow=50');
+
+  await expect(page.getByText('Name47')).not.toBeInViewport();
+  await expect(page.getByText('Name48')).toBeInViewport();
+  await expect(page.getByText('Name49')).toBeInViewport();
+  await expect(page.getByText('Name50')).toBeInViewport();
+  await expect(page.getByText('Name51')).toBeInViewport();
+});
+
 test.describe('Table sorting', () => {
   test('should work with keyboard', async ({ page }) => {
     await page.goto('/Table');
@@ -156,7 +166,7 @@ test.describe('Table resizing', () => {
   test('should resize correctly when column width is set lower than default min width', async ({
     page,
   }) => {
-    await page.goto('/Table?subRows=true');
+    await page.goto('/Table?rows=subRows');
 
     // resize first column
     {
@@ -227,7 +237,7 @@ test.describe('Table row selection', () => {
   test('Should select all sub rows when parent row is selected', async ({
     page,
   }) => {
-    await page.goto('/Table?isSelectable=true&subRows=true');
+    await page.goto('/Table?isSelectable=true&rows=subRows');
 
     const row2 = page.getByText('2Name2Description2');
     const row2SubRowExpander = row2.getByLabel('Toggle sub row');
@@ -253,7 +263,7 @@ test.describe('Table row selection', () => {
   test('Should show indeterminate when sub rows are disabled', async ({
     page,
   }) => {
-    await page.goto('/Table?isSelectable=true&subRows=true');
+    await page.goto('/Table?isSelectable=true&rows=subRows');
 
     const row3 = page.getByText('3Name3Description3');
     const row3SubRowExpander = row3.getByLabel('Toggle sub row');
@@ -275,7 +285,7 @@ test.describe('Table row selection', () => {
   test('Should show indeterminate when some sub rows are filtered out', async ({
     page,
   }) => {
-    await page.goto('/Table?isSelectable=true&subRows=true&filter=true');
+    await page.goto('/Table?isSelectable=true&rows=subRows&filter=true');
 
     await filter(page);
     const row2 = page.getByText('2Name2Description2');
@@ -318,7 +328,7 @@ test.describe('Table row selection', () => {
   test('parent checkbox state should become indeterminate when some filtered sub rows are deselected', async ({
     page,
   }) => {
-    await page.goto('/Table?isSelectable=true&subRows=true&filter=true');
+    await page.goto('/Table?isSelectable=true&rows=subRows&filter=true');
 
     //expand subRows
     const row2 = page.getByText('2Name2Description2');
@@ -354,7 +364,7 @@ test.describe('Table row selection', () => {
     page,
   }) => {
     await page.goto(
-      '/Table?isSelectable=true&subRows=true&selectSubRows=false',
+      '/Table?isSelectable=true&rows=subRows&selectSubRows=false',
     );
 
     const row2 = page
@@ -384,7 +394,7 @@ test.describe('Table row selection', () => {
     page,
   }) => {
     await page.goto(
-      '/Table?isSelectable=true&subRows=true&selectSubRows=false&stateReducer=true',
+      '/Table?isSelectable=true&rows=subRows&selectSubRows=false&stateReducer=true',
     );
 
     const row2 = page
@@ -485,10 +495,10 @@ test.describe('Virtual Scroll Tests', () => {
 
     const rows = page.getByRole('rowgroup').getByRole('row');
     const row50NameCell = page.getByText('Name50');
-    expect((await rows.all()).length).toBe(14);
-    await expect(rows.nth(0)).toContainText('Name43');
-    await expect(rows.nth(7)).toContainText('Name50');
-    await expect(rows.nth(13)).toContainText('Name56');
+    expect((await rows.all()).length).toBe(13);
+    await expect(rows.nth(0)).toContainText('Name49');
+    await expect(rows.nth(1)).toContainText('Name50');
+    await expect(rows.nth(12)).toContainText('Name61');
 
     await expect(row50NameCell).toBeInViewport();
   });