From c3ba50940c9aa158b21f6c0d78056985f4a7c1c0 Mon Sep 17 00:00:00 2001 From: mollykreis <20542556+mollykreis@users.noreply.github.com> Date: Tue, 23 Jan 2024 18:00:35 -0600 Subject: [PATCH] Fix bug where table rows are not rendered correctly when the table is reattached to the DOM (#1766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request ## ๐Ÿคจ Rationale Fixes #1753 ## ๐Ÿ‘ฉโ€๐Ÿ’ป Implementation The bug in the existing code is that the table's viewport always reset to a scroll position of 0 when reconnected, but the virutalizer maintained its state of being scrolled to a certain position. To fix this problem, I updated the table's viewport scroll position to match the virtualizer's scroll position once the state of the virtualizer has been updated upon reconnection of the `nimble-table` element. ## ๐Ÿงช Testing - Manually tested - Wrote some new unit tests that verify that the scroll position and rendered rows are as expected when the table is disconnected and reconnected. This includes tests that update the data while the table is not connected. ## โœ… Checklist - [ ] I have updated the project documentation to reflect my changes or determined no changes are needed. --------- Co-authored-by: Milan Raj --- ...-b7ec8e55-4c71-40d3-95d2-471852f4ef99.json | 7 + .../src/table/models/virtualizer.ts | 1 + .../src/table/tests/table.spec.ts | 153 +++++++++++++++++- 3 files changed, 154 insertions(+), 7 deletions(-) create mode 100644 change/@ni-nimble-components-b7ec8e55-4c71-40d3-95d2-471852f4ef99.json diff --git a/change/@ni-nimble-components-b7ec8e55-4c71-40d3-95d2-471852f4ef99.json b/change/@ni-nimble-components-b7ec8e55-4c71-40d3-95d2-471852f4ef99.json new file mode 100644 index 0000000000..b65c41ab8f --- /dev/null +++ b/change/@ni-nimble-components-b7ec8e55-4c71-40d3-95d2-471852f4ef99.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Fix bug where table rows are not rendered correctly when the table is reattached to the DOM", + "packageName": "@ni/nimble-components", + "email": "20542556+mollykreis@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/packages/nimble-components/src/table/models/virtualizer.ts b/packages/nimble-components/src/table/models/virtualizer.ts index 42e56bdbd3..a676c4d856 100644 --- a/packages/nimble-components/src/table/models/virtualizer.ts +++ b/packages/nimble-components/src/table/models/virtualizer.ts @@ -56,6 +56,7 @@ export class Virtualizer { public connect(): void { this.viewportResizeObserver.observe(this.table.viewport); this.updateVirtualizer(); + this.table.viewport.scrollTo({ top: this.virtualizer!.scrollOffset }); } public disconnect(): void { diff --git a/packages/nimble-components/src/table/tests/table.spec.ts b/packages/nimble-components/src/table/tests/table.spec.ts index db35223414..55f0f07bd0 100644 --- a/packages/nimble-components/src/table/tests/table.spec.ts +++ b/packages/nimble-components/src/table/tests/table.spec.ts @@ -50,13 +50,17 @@ const simpleTableData = [ } ] as const; -const largeTableData = Array.from(Array(500), (_, i) => { - return { - stringData: `string ${i}`, - numericData: i, - moreStringData: 'foo' - }; -}); +function createLargeData(dataLength: number): SimpleTableRecord[] { + return Array.from(Array(dataLength), (_, i) => { + return { + stringData: `string ${i}`, + numericData: i, + moreStringData: 'foo' + }; + }); +} + +const largeTableData = createLargeData(500); // prettier-ignore async function setup(): Promise>> { @@ -1452,4 +1456,139 @@ describe('Table', () => { expect(element.validity.invalidColumnConfiguration).toBeFalse(); }); }); + + describe('detaching and reattaching', () => { + let element: Table; + let connect: () => Promise; + let disconnect: () => Promise; + let pageObject: TablePageObject; + const largeData200 = createLargeData(200); + const largeData400 = createLargeData(400); + + beforeEach(async () => { + ({ element, connect, disconnect } = await setup()); + element.idFieldName = 'stringData'; + await connect(); + pageObject = new TablePageObject(element); + }); + + afterEach(async () => { + await disconnect(); + }); + + function getFirstRenderedRowDataIndex( + data: readonly SimpleTableRecord[] + ): number { + const lastRenderedRowId = pageObject.getRecordId(0); + return data.findIndex(x => x.stringData === lastRenderedRowId); + } + + async function setDataAndScrollToBottom( + data: readonly SimpleTableRecord[] + ): Promise { + await element.setData(data); + await waitForUpdatesAsync(); + await pageObject.scrollToLastRowAsync(); + } + + async function disconnectAndReconnect( + updatesWhileDisconnected: { + data?: readonly SimpleTableRecord[], + height?: string + } = { data: undefined, height: undefined } + ): Promise { + await disconnect(); + if (updatesWhileDisconnected.data !== undefined) { + await element.setData(updatesWhileDisconnected.data); + } + if (updatesWhileDisconnected.height) { + element.style.height = updatesWhileDisconnected.height; + } + await connect(); + await waitForUpdatesAsync(); + } + + it('maintains scroll position if data does not change', async () => { + await setDataAndScrollToBottom(largeData200); + const scrollTopBeforeDisconnect = element.viewport.scrollTop; + const firstRenderedRowBeforeDisconnect = getFirstRenderedRowDataIndex(largeData200); + + await disconnectAndReconnect(); + + expect(element.viewport.scrollTop).toBe(scrollTopBeforeDisconnect); + const firstRenderedRowAfterReconnect = getFirstRenderedRowDataIndex(largeData200); + expect(firstRenderedRowAfterReconnect).toBe( + firstRenderedRowBeforeDisconnect + ); + }); + + it('updates scroll position if data length is reduced while not attached', async () => { + await setDataAndScrollToBottom(largeData400); + const scrollTopBeforeDisconnect = element.viewport.scrollTop; + const firstRenderedRowBeforeDisconnect = getFirstRenderedRowDataIndex(largeData400); + + await disconnectAndReconnect({ data: largeData200 }); + + expect(element.viewport.scrollTop).toBeGreaterThan(0); + expect(element.viewport.scrollTop).toBeLessThan( + scrollTopBeforeDisconnect + ); + const firstRenderedRowAfterReconnect = getFirstRenderedRowDataIndex(largeData200); + expect(firstRenderedRowAfterReconnect).toBeGreaterThan(0); + expect(firstRenderedRowAfterReconnect).toBeLessThan( + firstRenderedRowBeforeDisconnect + ); + }); + + it('maintains scroll position if data length is increased while not attached', async () => { + await setDataAndScrollToBottom(largeData200); + const scrollTopBeforeDisconnect = element.viewport.scrollTop; + const firstRenderedRowBeforeDisconnect = getFirstRenderedRowDataIndex(largeData200); + + await disconnectAndReconnect({ data: largeData400 }); + + expect(element.viewport.scrollTop).toBe(scrollTopBeforeDisconnect); + const firstRenderedRowAfterReconnect = getFirstRenderedRowDataIndex(largeData400); + expect(firstRenderedRowAfterReconnect).toBe( + firstRenderedRowBeforeDisconnect + ); + }); + + it('updates scroll position if data is cleared while not attached', async () => { + await setDataAndScrollToBottom(largeData200); + + await disconnectAndReconnect({ data: [] }); + + expect(element.viewport.scrollTop).toBe(0); + expect(pageObject.getRenderedRowCount()).toBe(0); + }); + + it('adjusts the number of rendered rows when the table height increases while not attached', async () => { + element.style.height = '500px'; + await element.setData(largeData200); + await waitForUpdatesAsync(); + const renderedRowCountBeforeDisconnect = pageObject.getRenderedRowCount(); + + await disconnectAndReconnect({ height: '700px' }); + + const renderedRowCountAfterReconnect = pageObject.getRenderedRowCount(); + expect(renderedRowCountAfterReconnect).toBeGreaterThan( + renderedRowCountBeforeDisconnect + ); + }); + + it('adjusts the number of rendered rows when the table height decreases while not attached', async () => { + element.style.height = '500px'; + await element.setData(largeData200); + await waitForUpdatesAsync(); + const renderedRowCountBeforeDisconnect = pageObject.getRenderedRowCount(); + + await disconnectAndReconnect({ height: '200px' }); + + const renderedRowCountAfterReconnect = pageObject.getRenderedRowCount(); + expect(renderedRowCountAfterReconnect).toBeLessThan( + renderedRowCountBeforeDisconnect + ); + }); + }); });